Exit with code 1 instead of aborting when the OSD fails to open its devices

Blockstore errors at startup, such as a data device that cannot be opened,
escaped from the etcd callback that creates the blockstore, and configuration
errors escaped from osd_t construction. vitastor-osd then died with
"terminate called after throwing std::runtime_error" and a core dump. Print
the error and exit with code 1, so that a startup error can be told from a
crash. Other exceptions still abort.

.gitea/workflows/test.yml

@@ -1913,6 +1913,24 @@ jobs:
           echo ""
         done
 
+  test_osd_open_error:
+    runs-on: ubuntu-latest
+    needs: build
+    container: ${{env.TEST_IMAGE}}:${{github.sha}}
+    steps:
+    - name: Run test
+      id: test
+      timeout-minutes: 3
+      run: /root/vitastor/tests/test_osd_open_error.sh
+    - name: Print logs
+      if: always() && steps.test.outcome == 'failure'
+      run: |
+        for i in /root/vitastor/testdata/*.log /root/vitastor/testdata/*.txt; do
+          echo "-------- $i --------"
+          cat $i
+          echo ""
+        done
+
   test_enospc:
     runs-on: ubuntu-latest
     needs: build

src/blockstore/v1/flush.cpp

@@ -127,13 +127,6 @@ void journal_flusher_t::loop()
         co[i].loop();
 }
 
-// Versions restart from 1 after a deletion, so the queued version of an object may be a flushed
-// delete above the one being queued now. The flusher would skip that version and never flush this one
-bool journal_flusher_t::is_gone(object_id oid, uint64_t version)
-{
-    return bs->dirty_db.find((obj_ver_id){ .oid = oid, .version = version }) == bs->dirty_db.end();
-}
-
 void journal_flusher_t::enqueue_flush(obj_ver_id ov)
 {
 #ifdef BLOCKSTORE_DEBUG
@@ -142,7 +135,7 @@ void journal_flusher_t::enqueue_flush(obj_ver_id ov)
     auto it = flush_versions.find(ov.oid);
     if (it != flush_versions.end())
     {
-        if (it->second < ov.version || is_gone(ov.oid, it->second))
+        if (it->second < ov.version)
             it->second = ov.version;
     }
     else
@@ -165,7 +158,7 @@ void journal_flusher_t::unshift_flush(obj_ver_id ov, bool force)
     auto it = flush_versions.find(ov.oid);
     if (it != flush_versions.end())
     {
-        if (it->second < ov.version || is_gone(ov.oid, it->second))
+        if (it->second < ov.version)
             it->second = ov.version;
     }
     else
@@ -238,50 +231,6 @@ void journal_flusher_t::release_trim()
     trim_wanted--;
 }
 
-// The flusher puts back entries whose journal sector isn't written yet and stops if it finds
-// nothing else. Writing the sector makes them flushable, and nothing else may wake it then
-void journal_flusher_t::wakeup_on_journal_written()
-{
-    if (!dequeuing && flush_queue.size() && (flush_queue.size() >= flusher_start_threshold || trim_wanted > 0))
-    {
-        dequeuing = true;
-        bs->ringloop->wakeup();
-    }
-}
-
-// An object's newest stable version is reachable when the object is being flushed, or when
-// it's queued under a version that is still in dirty_db and not older than that one: the
-// flusher skips a queued version it can't find, and flushes only up to the queued version
-std::vector<obj_ver_id> journal_flusher_t::find_unflushable_stable()
-{
-    std::vector<obj_ver_id> lost;
-    auto & dirty_db = bs->dirty_db;
-    auto it = dirty_db.begin();
-    while (it != dirty_db.end())
-    {
-        object_id oid = it->first.oid;
-        uint64_t stable_ver = 0;
-        for (; it != dirty_db.end() && it->first.oid == oid; it++)
-        {
-            if (IS_STABLE(it->second.state))
-                stable_ver = it->first.version;
-        }
-        if (!stable_ver || sync_to_repeat.find(oid) != sync_to_repeat.end())
-            continue;
-        auto fv = flush_versions.find(oid);
-        if (fv != flush_versions.end() && fv->second >= stable_ver &&
-            dirty_db.find((obj_ver_id){ .oid = oid, .version = fv->second }) != dirty_db.end())
-            continue;
-        lost.push_back((obj_ver_id){ .oid = oid, .version = stable_ver });
-    }
-    return lost;
-}
-
-std::vector<obj_ver_id> blockstore_impl_t::find_unflushable_stable()
-{
-    return flusher->find_unflushable_stable();
-}
-
 void journal_flusher_t::dump_diagnostics()
 {
     const char *unflushable_type = "";

src/blockstore/v1/flush.h

@@ -120,7 +120,6 @@ class journal_flusher_t
     bool try_find_older(std::map<obj_ver_id, dirty_entry>::iterator & dirty_end, obj_ver_id & cur);
     bool try_find_other(std::map<obj_ver_id, dirty_entry>::iterator & dirty_end, obj_ver_id & cur);
     bool may_advance_except_one();
-    bool is_gone(object_id oid, uint64_t version);
 
 public:
     journal_flusher_t(blockstore_impl_t *bs);
@@ -133,11 +132,9 @@ public:
     void mark_trim_possible();
     void request_trim();
     void release_trim();
-    void wakeup_on_journal_written();
     void enqueue_flush(obj_ver_id oid);
     void unshift_flush(obj_ver_id oid, bool force);
     void remove_flush(object_id oid);
     void dump_diagnostics();
     bool is_mutated(uint64_t clean_loc);
-    std::vector<obj_ver_id> find_unflushable_stable();
 };

src/blockstore/v1/impl.h

@@ -346,9 +346,6 @@ public:
     // Print diagnostics to stdout
     void dump_diagnostics();
 
-    // Stable dirty versions that no flush will ever reach. Scans all of dirty_db: for tests only
-    std::vector<obj_ver_id> find_unflushable_stable();
-
     // Get diagnostic string for an operation
     std::string get_op_diag(blockstore_op_t *op);
 

src/blockstore/v1/write.cpp

@@ -925,7 +925,6 @@ void blockstore_impl_t::mark_not_dirty(int journal_sector)
         (new_ds + (new_ds >= journal.used_start ? 0 : journal.len)))
     {
         journal.dirty_start = new_ds;
-        flusher->wakeup_on_journal_written();
     }
 }
 

src/osd/osd.cpp

@@ -236,7 +236,16 @@ void osd_t::init_blockstore(std::function<void()> on_init)
     if (!json_is_true(this->config["disable_blockstore"]))
     {
         auto bs_cfg = json_to_string_map(this->config);
-        this->bs = bs_factory(bs_cfg);
+        try
+        {
+            this->bs = bs_factory(bs_cfg);
+        }
+        catch (std::exception & e)
+        {
+            fprintf(stderr, "[OSD %ju] %s\n", this->osd_num, e.what());
+            force_stop(1);
+            return;
+        }
         // Pre-configure pool PG shards
         for (auto & pool_item: st_cli->pool_config)
         {

src/osd/osd_main.cpp

@@ -67,13 +67,21 @@ int main(int narg, char *args[])
     ring_loop_t *ringloop = new ring_loop_t(RINGLOOP_DEFAULT_SIZE);
     epoll_manager_t *epmgr = new epoll_manager_t(ringloop);
     auto st_cli = new etcd_state_client_http_t(epmgr->tfd);
-    osd = new osd_t(
-        config, ringloop, epmgr->tfd, std::unique_ptr<etcd_state_client_t>(st_cli),
-        [ringloop, tfd = epmgr->tfd](blockstore_config_t & cfg)
-        {
-            return blockstore_i::create(cfg, ringloop, tfd);
-        }
-    );
+    try
+    {
+        osd = new osd_t(
+            config, ringloop, epmgr->tfd, std::unique_ptr<etcd_state_client_t>(st_cli),
+            [ringloop, tfd = epmgr->tfd](blockstore_config_t & cfg)
+            {
+                return blockstore_i::create(cfg, ringloop, tfd);
+            }
+        );
+    }
+    catch (std::exception & e)
+    {
+        fprintf(stderr, "%s\n", e.what());
+        return 1;
+    }
     osd->start();
     while (1)
     {

src/test/test_blockstore_chaos.cpp

@@ -66,8 +66,6 @@ struct chaos_probs_t
 };
 
 static chaos_probs_t probs;
-// Check the store's internal state after every drain, which takes full scans of it
-static bool hardened = false;
 
 // What a single step decided to do
 enum chaos_action_t
@@ -116,28 +114,6 @@ static void dump_extra(v1::blockstore_impl_t *bs)
 {
 }
 
-// A stable version no flush reaches pins the journal for good, and the store only shows that
-// as writes refused with EAGAIN - expected under pressure - so --hardened checks it explicitly
-static void check_flush_liveness(blockstore_impl_t *bs, uint64_t step_no)
-{
-}
-
-static void check_flush_liveness(v1::blockstore_impl_t *bs, uint64_t step_no)
-{
-    if (!hardened)
-        return;
-    auto lost = bs->find_unflushable_stable();
-    if (!lost.size())
-        return;
-    fprintf(stderr, "step %ju: %zu stable version(s) will never be flushed:", step_no, lost.size());
-    for (auto & ov: lost)
-        fprintf(stderr, " %jx:%jx v%ju", ov.oid.inode, ov.oid.stripe, ov.version);
-    fprintf(stderr, "\n");
-    bs->dump_diagnostics();
-    fflush(stdout);
-    abort();
-}
-
 // What we believe about one object.
 //
 // `submitted` is the highest version we ever enqueued a write for - not necessarily
@@ -1076,7 +1052,6 @@ struct chaos_t
     void do_restart()
     {
         drain();
-        check_flush_liveness(bs, step_no);
         restarts++;
         std::vector<std::vector<uint8_t>> before(OBJ_COUNT);
         std::vector<uint64_t> before_ver(OBJ_COUNT, 0);
@@ -1226,7 +1201,6 @@ struct chaos_t
             step();
         // Everything still in flight must complete, and everything durable must still be there
         drain();
-        check_flush_liveness(bs, step_no);
         for (int obj = 0; obj < OBJ_COUNT; obj++)
             check_object(obj);
     }
@@ -1309,9 +1283,6 @@ static const char *help_text =
     "  --steps <n>         Steps per seed (default %ju)\n"
     "  --config <n>        Force configuration <n> instead of deriving it from the seed\n"
     "  --max-inflight <n>  Operations in flight at once (default %u)\n"
-    "  --hardened          After every drain, also check the store's internal state, which\n"
-    "                      takes full scans of it: in store v1, that every stable version\n"
-    "                      gets flushed\n"
     "  -v, --trace         Trace every operation and every simulated disk request\n"
     "  -h, --help          Show this help\n"
     "\n"
@@ -1412,8 +1383,6 @@ int main(int narg, char *args[])
         }
         else if (!strcmp(opt, "-v") || !strcmp(opt, "--trace"))
             trace = true;
-        else if (!strcmp(opt, "--hardened"))
-            hardened = true;
         else if (!strcmp(opt, "--seed"))
         {
             take(v);

tests/run_tests.sh

@@ -149,6 +149,8 @@ OLD=1 ./test_resize_auto.sh
 
 ./test_osd_tags.sh
 
+./test_osd_open_error.sh
+
 ./test_enospc.sh
 SCHEME=xor ./test_enospc.sh
 IMMEDIATE_COMMIT=1 ./test_enospc.sh

tests/test_osd_open_error.sh

@@ -0,0 +1,14 @@
+#!/bin/bash -ex
+
+# An OSD that cannot open its data device must exit with code 1, not abort with a core dump
+
+. `dirname $0`/common.sh
+
+wait_etcd
+
+code=0
+build/src/osd/vitastor-osd --osd_num 1 --data_device ./testdata/bin/missing --etcd_address $ETCD_URL &> ./testdata/osd1.log || code=$?
+grep -q 'Failed to open data device' ./testdata/osd1.log || format_error "OSD did not report the open error"
+[[ $code -eq 1 ]] || format_error "OSD exited with code $code instead of 1"
+
+format_green OK