test/store_test: get rid off assert_death.

Looks like death assertions aren't 100% reliable and might cause
deadlock sometimes.
Hence getting rid of them and enabling optional sending exception from
*Store::queue_transaction()

Fixes: https://tracker.ceph.com/issues/61193
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
(cherry picked from commit 46f01d832487d3a3183783d41450fd8f49347097)

diff --git a/src/common/options/global.yaml.in b/src/common/options/global.yaml.in
index a0ebf3576c9bfc88f2d98d1069696dcab28e73d2..b3c5130c4003090bdd612c83b8f76cce00508c4c 100644 (file)
--- a/src/common/options/global.yaml.in
+++ b/src/common/options/global.yaml.in
@@ -6360,3 +6360,9 @@ options:
   default: 0
   services:
   - mgr
+- name: objectstore_debug_throw_on_failed_txc
+  type: bool
+  level: dev
+  desc: Enables exception throwing instead of process abort on transaction submission error.
+  default: false
+  with_legacy: false

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index ebf70f76e68cefdb8b645bac2e7a39533828354e..07386063db5dfeccdd7869512dedae5daf6c1317 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -7787,6 +7787,7 @@ int BlueStore::_mount()
 
 int BlueStore::umount()
 {
+  dout(5) << __func__ << dendl;
   ceph_assert(_kv_only || mounted);
   _osr_drain_all();
 
@@ -14366,7 +14367,13 @@ void BlueStore::_txc_add_transaction(TransContext *txc, Transaction *t)
            << " not handled on operation " << op->op
            << " (op " << pos << ", counting from 0)" << dendl;
       _dump_transaction<0>(cct, t);
-      ceph_abort_msg("unexpected error");
+      if (!g_conf().get_val<bool>("objectstore_debug_throw_on_failed_txc")) {
+       ceph_abort_msg("unexpected error");
+      } else {
+       txc->osr->undo_queue(txc);
+       delete txc;
+       throw r;
+      }
     }
 
     // these operations implicity create the object
@@ -14612,7 +14619,13 @@ void BlueStore::_txc_add_transaction(TransContext *txc, Transaction *t)
              << dendl;
         derr << msg << dendl;
         _dump_transaction<0>(cct, t);
-       ceph_abort_msg("unexpected error");
+       if (!g_conf().get_val<bool>("objectstore_debug_throw_on_failed_txc")) {
+         ceph_abort_msg("unexpected error");
+       } else {
+         txc->osr->undo_queue(txc);
+         delete txc;
+         throw r;
+       }
       }
     }
   }

diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h
index 02a3bd51c87bd29e720c2061914afb2a26de42bc..a91bf8414eb4fd89a2d4504cf23a2ae80a1d67f3 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -2045,6 +2045,12 @@ private:
       txc->seq = ++last_seq;
       q.push_back(*txc);
     }
+    void undo_queue(TransContext* txc) {
+      std::lock_guard l(qlock);
+      ceph_assert(&q.back() == txc);
+      --last_seq;
+      q.pop_back();
+    }
 
     void drain() {
       std::unique_lock l(qlock);

diff --git a/src/os/kstore/KStore.cc b/src/os/kstore/KStore.cc
index 9526a756419cc80722afd44613544fa2ea28ad1c..7158486ca388527189c1fa4429c76dbaea3b3eb9 100644 (file)
--- a/src/os/kstore/KStore.cc
+++ b/src/os/kstore/KStore.cc
@@ -2285,7 +2285,13 @@ void KStore::_txc_add_transaction(TransContext *txc, Transaction *t)
       f.close_section();
       f.flush(*_dout);
       *_dout << dendl;
-      ceph_abort_msg("unexpected error");
+      if (!g_conf().get_val<bool>("objectstore_debug_throw_on_failed_txc")) {
+       ceph_abort_msg("unexpected error");
+      } else {
+       txc->osr->undo_queue(txc);
+       delete txc;
+       throw r;
+      }
     }
 
     // object operations
@@ -2534,7 +2540,13 @@ void KStore::_txc_add_transaction(TransContext *txc, Transaction *t)
        f.close_section();
        f.flush(*_dout);
        *_dout << dendl;
-       ceph_abort_msg("unexpected error");
+       if (!g_conf().get_val<bool>("objectstore_debug_throw_on_failed_txc")) {
+         ceph_abort_msg("unexpected error");
+       } else {
+         txc->osr->undo_queue(txc);
+         delete txc;
+         throw r;
+       }
       }
     }
   }

diff --git a/src/os/kstore/KStore.h b/src/os/kstore/KStore.h
index d7d95acdfbde5bcdc8801ab899910eb7770a4c2e..808543cefdef6dcd9ed8724b3f8b52c792af0a48 100644 (file)
--- a/src/os/kstore/KStore.h
+++ b/src/os/kstore/KStore.h
@@ -276,6 +276,11 @@ public:
       std::lock_guard<std::mutex> l(qlock);
       q.push_back(*txc);
     }
+    void undo_queue(TransContext* txc) {
+      std::lock_guard<std::mutex> l(qlock);
+      ceph_assert(&q.back() == txc);
+      q.pop_back();
+    }
 
     void flush() {
       std::unique_lock<std::mutex> l(qlock);

diff --git a/src/os/memstore/MemStore.cc b/src/os/memstore/MemStore.cc
index e0a607270d12ade1fe9ae336998fecfc579fd5ea..4f705afccf0faee50a4431db9b349eca22c4fc78 100644 (file)
--- a/src/os/memstore/MemStore.cc
+++ b/src/os/memstore/MemStore.cc
@@ -1032,7 +1032,11 @@ void MemStore::_do_transaction(Transaction& t)
        f.close_section();
        f.flush(*_dout);
        *_dout << dendl;
-       ceph_abort_msg("unexpected error");
+        if (!g_conf().get_val<bool>("objectstore_debug_throw_on_failed_txc")) {
+         ceph_abort_msg("unexpected error");
+        } else {
+         throw r;
+        }
       }
     }
 

diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc
index 54dc29c7db8d67d74bfd1ae8665fc33c4b9e4de9..96f6aff4c7b1a14f76ffba5706ce6ed4d330b195 100644 (file)
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -3246,7 +3246,8 @@ TEST_P(StoreTest, SimpleCloneTest) {
   int r;
   coll_t cid;
 
-  SetDeathTestStyle("threadsafe");
+  SetVal(g_conf(), "objectstore_debug_throw_on_failed_txc", "true");
+  g_conf().apply_changes(nullptr);
 
   auto ch = store->create_new_collection(cid);
   {
@@ -3532,8 +3533,12 @@ TEST_P(StoreTest, SimpleCloneTest) {
     ObjectStore::Transaction t;
     t.remove_collection(cid);
     cerr << "Invalid rm coll" << std::endl;
-    PrCtl unset_dumpable;
-    EXPECT_DEATH(queue_transaction(store, ch, std::move(t)), "");
+    try {
+      queue_transaction(store, ch, std::move(t));
+      FAIL() << "remove_collection failed to return ENOTEMPTY.";
+    } catch (int err) {
+      ASSERT_EQ(err, -ENOTEMPTY);
+    }
   }
   {
     ObjectStore::Transaction t;
@@ -3555,8 +3560,12 @@ TEST_P(StoreTest, SimpleCloneTest) {
     t.remove(cid, hoid);
     t.remove(cid, hoid2);
     t.remove_collection(cid);
-    PrCtl unset_dumpable;
-    EXPECT_DEATH(queue_transaction(store, ch, std::move(t)), "");
+    try {
+      queue_transaction(store, ch, std::move(t));
+      FAIL() << "remove_collection failed to return ENOTEMPTY.";
+    } catch (int err) {
+      ASSERT_EQ(err, -ENOTEMPTY);
+    }
   }
   {
     ObjectStore::Transaction t;

diff --git a/src/test/objectstore/store_test_fixture.cc b/src/test/objectstore/store_test_fixture.cc
index a3bdc7a36ac3a0bf8e40cc2bb8c7b189636aeb59..0cffd79a709d2349d50b59d81e0336871cd5c2ee 100644 (file)
--- a/src/test/objectstore/store_test_fixture.cc
+++ b/src/test/objectstore/store_test_fixture.cc
@@ -77,10 +77,6 @@ void StoreTestFixture::TearDown()
   // config settings. Hence setting it to 'unsafe' here as test case is closing.
   g_conf()._clear_safe_to_start_threads();
   PopSettings(0);
-  if (!orig_death_test_style.empty()) {
-    ::testing::FLAGS_gtest_death_test_style = orig_death_test_style;
-    orig_death_test_style.clear();
-  }
 }
 
 void StoreTestFixture::SetVal(ConfigProxy& _conf, const char* key, const char* val)

diff --git a/src/test/objectstore/store_test_fixture.h b/src/test/objectstore/store_test_fixture.h
index 3f25fd493d0d3c8e7876f91a070943b151b4b37f..0495c21bd327cebb09f49c98380279948a26b9bc 100644 (file)
--- a/src/test/objectstore/store_test_fixture.h
+++ b/src/test/objectstore/store_test_fixture.h
@@ -13,8 +13,6 @@ class StoreTestFixture : virtual public ::testing::Test {
   std::stack<std::pair<std::string, std::string>> saved_settings;
   ConfigProxy* conf = nullptr;
 
-  std::string orig_death_test_style;
-
 public:
   std::unique_ptr<ObjectStore> store;
   ObjectStore::CollectionHandle ch;
@@ -25,12 +23,6 @@ public:
 
   void SetUp() override;
   void TearDown() override;
-  void SetDeathTestStyle(const char* new_style) {
-    if (orig_death_test_style.empty()) {
-      orig_death_test_style = ::testing::FLAGS_gtest_death_test_style;
-    }
-    ::testing::FLAGS_gtest_death_test_style = new_style;
-  }
 
   void SetVal(ConfigProxy& conf, const char* key, const char* val);
   struct SettingsBookmark {