From 9b4f615f9b092c16f5f853c4ff1cbd81f89d66bf Mon Sep 17 00:00:00 2001
From: Yuval Lifshitz <ylifshit@redhat.com>
Date: Tue, 13 Feb 2024 16:36:51 +0000
Subject: [PATCH] rgw/notifications: delete persistent queue only if topic is
 deleted

Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
(cherry picked from commit 666e79f1fb78fe8128791e9e23159571f76cfe70)
---
 src/rgw/driver/rados/rgw_notify.cc |  2 +-
 src/rgw/rgw_admin.cc               | 13 ++++++------
 src/rgw/rgw_rest_pubsub.cc         | 32 +++++++++++++++---------------
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/src/rgw/driver/rados/rgw_notify.cc b/src/rgw/driver/rados/rgw_notify.cc
index 19d4c0cbb6e..3fe441eec14 100644
--- a/src/rgw/driver/rados/rgw_notify.cc
+++ b/src/rgw/driver/rados/rgw_notify.cc
@@ -1065,7 +1065,7 @@ static inline bool notification_match(reservation_t& res,
       ldpp_dout(res.dpp, 1)
           << "INFO: failed to load topic: " << topic_cfg.name
           << ". error: " << ret
-          << " while resrving persistent notification event" << dendl;
+          << " while reserving persistent notification event" << dendl;
       if (ret == -ENOENT) {
         // either the topic is deleted but the corresponding notification still
         // exist or in v2 mode the notification could have synced first but
diff --git a/src/rgw/rgw_admin.cc b/src/rgw/rgw_admin.cc
index cc6f6d5cfea..560b6ac7cc9 100644
--- a/src/rgw/rgw_admin.cc
+++ b/src/rgw/rgw_admin.cc
@@ -10776,12 +10776,6 @@ next:
       cerr << "ERROR: Run 'topic rm' from master zone " << std::endl;
       return -EINVAL;
     }
-    ret = rgw::notify::remove_persistent_topic(
-        dpp(), static_cast<rgw::sal::RadosStore*>(driver)->getRados()->get_notif_pool_ctx(), topic_name, null_yield);
-    if (ret < 0) {
-      cerr << "ERROR: could not remove persistent topic: " << cpp_strerror(-ret) << std::endl;
-      return -ret;
-    }
 
     RGWPubSub ps(driver, tenant, *site);
 
@@ -10790,6 +10784,13 @@ next:
       cerr << "ERROR: could not remove topic: " << cpp_strerror(-ret) << std::endl;
       return -ret;
     }
+    
+    ret = rgw::notify::remove_persistent_topic(
+        dpp(), static_cast<rgw::sal::RadosStore*>(driver)->getRados()->get_notif_pool_ctx(), topic_name, null_yield);
+    if (ret < 0 && ret != -ENOENT) {
+      cerr << "ERROR: could not remove persistent topic: " << cpp_strerror(-ret) << std::endl;
+      return -ret;
+    }
   }
 
   if (opt_cmd == OPT::PUBSUB_NOTIFICATION_RM) {
diff --git a/src/rgw/rgw_rest_pubsub.cc b/src/rgw/rgw_rest_pubsub.cc
index 138150b0027..585eb68caf9 100644
--- a/src/rgw/rgw_rest_pubsub.cc
+++ b/src/rgw/rgw_rest_pubsub.cc
@@ -814,35 +814,35 @@ void RGWPSDeleteTopicOp::execute(optional_yield y) {
     op_ret = verify_topic_owner_or_policy(
         s, result, driver->get_zone()->get_zonegroup().get_name(),
         rgw::IAM::snsDeleteTopic);
-    if (op_ret != 0) {
+    if (op_ret < 0) {
       ldpp_dout(this, 1) << "no permission to remove topic '" << topic_name
                          << "'" << dendl;
       return;
     }
-  } else {
+    op_ret = ps.remove_topic(this, topic_name, y);
+    if (op_ret < 0 && op_ret != -ENOENT) {
+      ldpp_dout(this, 1) << "failed to remove topic '" << topic_name << ", ret=" << op_ret << dendl;
+      return;
+    }
+    ldpp_dout(this, 1) << "successfully removed topic '" << topic_name << "'" << dendl;
+  } else if (op_ret != -ENOENT) {
     ldpp_dout(this, 1) << "failed to fetch topic '" << topic_name
                        << "' with error: " << op_ret << dendl;
-    if (op_ret == -ENOENT) {
-      // its not an error if no topics exist, just a no-op
-      op_ret = 0;
-    }
     return;
   }
+  if (op_ret == -ENOENT) {
+    // its not an error if no topics exist, just a no-op
+    op_ret = 0;
+  }
   // upon deletion it is not known if topic is persistent or not
   // will try to delete the persistent topic anyway
-  op_ret = rgw::notify::remove_persistent_topic(topic_name, s->yield);
-  if (op_ret != -ENOENT && op_ret < 0) {
+  // doing this regardless of the topic being previously deleted
+  // to allow for cleanup if only the queue deletion failed
+  if (const auto ret = rgw::notify::remove_persistent_topic(topic_name, s->yield); ret < 0 && ret != -ENOENT) {
     ldpp_dout(this, 1) << "DeleteTopic Action failed to remove queue for "
                           "persistent topics. error:"
-                       << op_ret << dendl;
-    return;
-  }
-  op_ret = ps.remove_topic(this, topic_name, y);
-  if (op_ret < 0) {
-    ldpp_dout(this, 1) << "failed to remove topic '" << topic_name << ", ret=" << op_ret << dendl;
-    return;
+                       << ret << dendl;
   }
-  ldpp_dout(this, 1) << "successfully removed topic '" << topic_name << "'" << dendl;
 }
 
 using op_generator = RGWOp*(*)(bufferlist);
-- 
2.39.5