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>

diff --git a/src/common/options/global.yaml.in b/src/common/options/global.yaml.in
index 3a3a4a1372916bf656eeb4beecaa9b15e4d37db7..cbe8959411837027aefa8826913176b3ed28351f 100644 (file)
--- a/src/common/options/global.yaml.in
+++ b/src/common/options/global.yaml.in
@@ -6343,3 +6343,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 e3e4833f1a2da2e563b0076f6de5fdc6fb023313..3d9f680ba6c2517f4fee3544675caa39f8999389 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -7852,6 +7852,7 @@ int BlueStore::_mount()
 
 int BlueStore::umount()
 {
+  dout(5) << __func__ << dendl;
   ceph_assert(_kv_only || mounted);
   _osr_drain_all();
 
@@ -14434,7 +14435,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
@@ -14680,7 +14687,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 c3c53264ec1de1b6e78eaf86bb436e659e8cb433..486c7cd4de095628fb8203948c09e1448cd48b2f 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -2055,6 +2055,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 9e3c7acd73b428880a6330445879436acd54325b..30a7606fd2fb3a6bc00e740a8ad62e798d7a35ed 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 99e99dcba0413998cc892420fa1328e3629033be..8ada7524dbcd142146a397f3ccf60f1c98d0d28d 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 482d302830416b84a0887f29b39b71d21caa26a7..d23c17a019dc2ea22c79d455a1bd7966323c70f2 100644 (file)
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -3245,7 +3245,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);
   {
@@ -3531,8 +3532,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;
@@ -3554,8 +3559,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 {