Merge pull request ceph#66110 from yuvalif/wip-yuval-73675

yuvalif · web-flow · commit 28307aae62e6 · 2025-11-11T13:39:52.000+02:00
rgw/logging: fix source bucket cleanup process
diff --git a/src/rgw/driver/rados/rgw_sal_rados.cc b/src/rgw/driver/rados/rgw_sal_rados.cc
@@ -1140,6 +1140,8 @@ int set_committed_logging_object(RadosStore* store,
   bufferlist bl;
   bl.append(last_committed);
   librados::ObjectWriteOperation op;
+  // if object does not exist, we should not create the attribute
+  op.assert_exists();
   op.setxattr(RGW_ATTR_COMMITTED_LOGGING_OBJ, std::move(bl));
   return rgw_rados_operate(dpp, io_ctx, obj_name_oid, std::move(op), y);
 }
@@ -1174,6 +1176,10 @@ int RadosBucket::get_logging_object_name(std::string& obj_name,
     }
     return ret;
   }
+  if (bl.length() == 0) {
+    ldpp_dout(dpp, 1) << "ERROR: logging object name at '" << obj_name_oid << "' is empty" << dendl;
+    return -ENODATA;
+  }
   obj_name = bl.to_str();
   return 0;
 }
diff --git a/src/rgw/radosgw-admin/radosgw-admin.cc b/src/rgw/radosgw-admin/radosgw-admin.cc
@@ -7884,6 +7884,12 @@ int main(int argc, const char **argv)
       return 0;
     }
 
+    // make sure that the logging source attribute is up-to-date
+    if (ret = rgw::bucketlogging::update_bucket_logging_sources(dpp(), target_bucket, bucket->get_key(), true, null_yield); ret < 0) {
+      cerr << "WARNING: failed to update logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
+        << "' in logging target '" << target_bucket->get_key() << "'. error: " << cpp_strerror(ret) << std::endl;
+    }
+
     std::string obj_name;
     RGWObjVersionTracker objv_tracker;
     ret = target_bucket->get_logging_object_name(obj_name, configuration.target_prefix, null_yield, dpp(), &objv_tracker);
diff --git a/src/rgw/rgw_bucket_logging.cc b/src/rgw/rgw_bucket_logging.cc
@@ -328,14 +328,6 @@ int commit_logging_object(const configuration& conf,
       << ret << dendl;
     return ret;
   }
-  return commit_logging_object(conf, target_bucket, dpp, y, last_committed);
-}
-
-int commit_logging_object(const configuration& conf,
-    const std::unique_ptr<rgw::sal::Bucket>& target_bucket,
-    const DoutPrefixProvider *dpp,
-    optional_yield y,
-    std::string* last_committed) {
   std::string obj_name;
   if (const int ret = target_bucket->get_logging_object_name(obj_name, conf.target_prefix, y, dpp, nullptr); ret < 0) {
     ldpp_dout(dpp, 1) << "ERROR: failed to get name of logging object of logging bucket '" <<
@@ -509,6 +501,12 @@ int log_record(rgw::sal::Driver* driver,
     return ret;
   }
 
+  // make sure that the logging source attribute is up-to-date
+  if (ret = update_bucket_logging_sources(dpp, target_bucket, s->bucket->get_key(), true, y); ret < 0) {
+    ldpp_dout(dpp, 5) << "WARNING: could not update bucket logging source '" <<
+      s->bucket->get_key() << "' in logging bucket '" << target_bucket_id << "' attribute, during record logging. ret = " << ret << dendl;
+  }
+
   const auto region = driver->get_zone()->get_zonegroup().get_api_name();
   std::string obj_name;
   RGWObjVersionTracker objv_tracker;
@@ -795,8 +793,7 @@ int update_bucket_logging_sources(const DoutPrefixProvider* dpp, std::unique_ptr
       ldpp_dout(dpp, 5) << "WARNING: failed to decode logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
         << "' for bucket '" << bucket->get_key() << "' when updating logging sources. error: " << err.what() << dendl;
     }
-    ldpp_dout(dpp, 20) << "INFO: logging source '" << src_bucket_id << "' already " <<
-      (add ? "added to" : "removed from") << " bucket '" << bucket->get_key() << "'" << dendl;
+    // no change
     return 0;
   }, y);
 }
@@ -810,53 +807,59 @@ int bucket_deletion_cleanup(const DoutPrefixProvider* dpp,
   // and also delete the object holding the pending object name
   auto& attrs = bucket->get_attrs();
   if (const auto iter = attrs.find(RGW_ATTR_BUCKET_LOGGING_SOURCES); iter != attrs.end()) {
+    source_buckets sources;
     try {
-      source_buckets sources;
       ceph::decode(sources, iter->second);
-      for (const auto& source : sources) {
-        std::unique_ptr<rgw::sal::Bucket> src_bucket;
-        if (const int ret = driver->load_bucket(dpp, source, &src_bucket, y); ret < 0) {
-          ldpp_dout(dpp, 5) << "WARNING: failed to get logging source bucket '" << source << "' for logging bucket '" <<
+    } catch (buffer::error& err) {
+      ldpp_dout(dpp, 5) << "WARNING: failed to decode logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
+        << "' for logging bucket '" << bucket->get_key() << "'. error: " << err.what() << dendl;
+      return -EIO;
+    }
+    // since we removed the attribute, we should not return error code from now on
+    // any retry, would find no attribute and return success
+    for (const auto& source : sources) {
+      std::unique_ptr<rgw::sal::Bucket> src_bucket;
+      if (const int ret = driver->load_bucket(dpp, source, &src_bucket, y); ret < 0) {
+        ldpp_dout(dpp, 5) << "WARNING: failed to get logging source bucket '" << source << "' for logging bucket '" <<
+          bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
+        continue;
+      }
+      auto& src_attrs = src_bucket->get_attrs();
+      if (const auto iter = src_attrs.find(RGW_ATTR_BUCKET_LOGGING); iter != src_attrs.end()) {
+        configuration conf;
+        try {
+          auto bl_iter = iter->second.cbegin();
+          decode(conf, bl_iter);
+        } catch (buffer::error& err) {
+          ldpp_dout(dpp, 5) << "WARNING: failed to decode logging attribute '" << RGW_ATTR_BUCKET_LOGGING
+            << "' of bucket '" << src_bucket->get_key() << "' during cleanup. error: " << err.what() << dendl;
+          continue;
+        }
+        std::string obj_name;
+        RGWObjVersionTracker objv;
+        if (const int ret = bucket->get_logging_object_name(obj_name, conf.target_prefix, y, dpp, &objv); ret < 0) {
+          ldpp_dout(dpp, 5) << "WARNING: failed to get logging object name for logging bucket '" << bucket->get_key() <<
+            "' during cleanup. ret = " << ret << dendl;
+          continue;
+        }
+        if (const int ret = bucket->remove_logging_object_name(conf.target_prefix, y, dpp, &objv); ret < 0) {
+          ldpp_dout(dpp, 5) << "WARNING: failed to delete object holding bucket logging object name for logging bucket '" <<
             bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
           continue;
         }
-        auto& src_attrs = src_bucket->get_attrs();
-        if (const auto iter = src_attrs.find(RGW_ATTR_BUCKET_LOGGING); iter != src_attrs.end()) {
-          configuration conf;
-          try {
-            auto bl_iter = iter->second.cbegin();
-            decode(conf, bl_iter);
-            std::string obj_name;
-            RGWObjVersionTracker objv;
-            if (const int ret = bucket->get_logging_object_name(obj_name, conf.target_prefix, y, dpp, &objv); ret < 0) {
-              ldpp_dout(dpp, 5) << "WARNING: failed to get logging object name for logging bucket '" << bucket->get_key() <<
-                "' during cleanup. ret = " << ret << dendl;
-              continue;
-            }
-            if (const int ret = bucket->remove_logging_object(obj_name, y, dpp); ret < 0) {
-              ldpp_dout(dpp, 5) << "WARNING: failed to delete pending logging object '" << obj_name << "' for logging bucket '" <<
-                bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
-              continue;
-            }
-            ldpp_dout(dpp, 20) << "INFO: successfully deleted pending logging object '" << obj_name << "' from deleted logging bucket '" <<
-                bucket->get_key() << "'" << dendl;
-            if (const int ret = bucket->remove_logging_object_name(conf.target_prefix, y, dpp, &objv); ret < 0) {
-              ldpp_dout(dpp, 5) << "WARNING: failed to delete object holding bucket logging object name for logging bucket '" <<
-                bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
-              continue;
-            }
-            ldpp_dout(dpp, 20) << "INFO: successfully deleted object holding bucket logging object name from deleted logging bucket '" <<
-              bucket->get_key() << "'" << dendl;
-          } catch (buffer::error& err) {
-            ldpp_dout(dpp, 5) << "WARNING: failed to decode logging attribute '" << RGW_ATTR_BUCKET_LOGGING
-              << "' of bucket '" << src_bucket->get_key() << "' during cleanup. error: " << err.what() << dendl;
-          }
+        ldpp_dout(dpp, 20) << "INFO: successfully deleted object holding bucket logging object name from deleted logging bucket '" <<
+          bucket->get_key() << "'" << dendl;
+        if (const int ret = bucket->remove_logging_object(obj_name, y, dpp); ret < 0) {
+          ldpp_dout(dpp, 5) << "WARNING: failed to delete pending logging object '" << obj_name << "' for logging bucket '" <<
+            bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
+          continue;
         }
+        ldpp_dout(dpp, 20) << "INFO: successfully deleted pending logging object '" << obj_name << "' from deleted logging bucket '" <<
+            bucket->get_key() << "'" << dendl;
+      } else {
+        ldpp_dout(dpp, 5) << "WARNING: no logging attribute '" << RGW_ATTR_BUCKET_LOGGING
+          << "' found in logging source bucket '" << src_bucket->get_key() << "' during cleanup" << dendl;
       }
-    } catch (buffer::error& err) {
-      ldpp_dout(dpp, 5) << "WARNING: failed to decode logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
-        << "' for logging bucket '" << bucket->get_key() << "'. error: " << err.what() << dendl;
-      return -EIO;
     }
   }
 
@@ -881,7 +884,7 @@ int source_bucket_cleanup(const DoutPrefixProvider* dpp,
         conf = std::move(tmp_conf);
       } catch (buffer::error& err) {
         ldpp_dout(dpp, 5) << "WARNING: failed to decode existing logging attribute '" << RGW_ATTR_BUCKET_LOGGING
-          << "' of bucket '" << bucket->get_key() << "' during cleanup. error: " << err.what() << dendl;
+          << "' of bucket '" << bucket->get_key() << "' during source cleanup. error: " << err.what() << dendl;
         return -EIO;
       }
       if (remove_attr) {
@@ -894,34 +897,67 @@ int source_bucket_cleanup(const DoutPrefixProvider* dpp,
   }, y); ret < 0) {
     if (remove_attr) {
       ldpp_dout(dpp, 5) << "WARNING: failed to remove logging attribute '" << RGW_ATTR_BUCKET_LOGGING <<
-        "' from bucket '" << bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
+        "' from bucket '" << bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
     }
     return ret;
   }
+  if (last_committed) {
+    last_committed->clear();
+  }
   if (!conf) {
     // no logging attribute found
     return 0;
   }
-  const auto& info = bucket->get_info();
-  if (const int ret = commit_logging_object(*conf, dpp, driver, info.bucket.tenant, y, last_committed); ret < 0) {
-    ldpp_dout(dpp, 5) << "WARNING: could not commit pending logging object of bucket '" <<
-      bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
-  } else {
-    ldpp_dout(dpp, 20) << "INFO: successfully committed pending logging object of bucket '" << bucket->get_key() << "'" << dendl;
-  }
+  // since we removed the attribute, we should not return error code from now on
+  // any retry, would find no attribute and return success
   rgw_bucket target_bucket_id;
+  const auto& info = bucket->get_info();
   if (const int ret = get_bucket_id(conf->target_bucket, info.bucket.tenant, target_bucket_id); ret < 0) {
     ldpp_dout(dpp, 5) << "WARNING: failed to parse logging bucket '" <<
-      conf->target_bucket << "' during cleanup. ret = " << ret << dendl;
+      conf->target_bucket << "' during source cleanup. ret = " << ret << dendl;
+    return 0;
+  }
+  std::unique_ptr<rgw::sal::Bucket> target_bucket;
+  const int ret = driver->load_bucket(dpp, target_bucket_id, &target_bucket, y);
+  if (ret < 0) {
+    ldpp_dout(dpp, 5) << "WARNING: failed to get logging bucket '" << target_bucket_id  <<
+      "' when doing source cleanup. ret = " << ret << dendl;
     return 0;
   }
-  if (const int ret = update_bucket_logging_sources(dpp, driver, target_bucket_id, bucket->get_key(), false, y); ret < 0) {
+  if (const int ret = update_bucket_logging_sources(dpp, target_bucket, bucket->get_key(), false, y); ret < 0) {
     ldpp_dout(dpp, 5) << "WARNING: could not update bucket logging source '" <<
-      bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
+      bucket->get_key() << "' in logging bucket '" << target_bucket_id << "' attribute, during source cleanup. ret = " << ret << dendl;
+  }
+  std::string obj_name;
+  RGWObjVersionTracker objv;
+  if (const int ret = target_bucket->get_logging_object_name(obj_name, conf->target_prefix, y, dpp, &objv); ret < 0) {
+    ldpp_dout(dpp, 5) << "WARNING: failed to get logging object name for logging bucket '" << bucket->get_key() <<
+      "' during source cleanup. ret = " << ret << dendl;
+    // if name cannot be fetched, we should not commit or remove the object holding the name
+    // it is better to leak an object than to cause a possible data loss
     return 0;
   }
-  ldpp_dout(dpp, 20) << "INFO: successfully updated bucket logging source '" <<
-    bucket->get_key() << "' during cleanup"<< dendl;
+  if (const int ret = target_bucket->remove_logging_object_name(conf->target_prefix, y, dpp, &objv); ret < 0) {
+    ldpp_dout(dpp, 5) << "WARNING: failed to delete object holding bucket logging object name for bucket '" <<
+      bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
+    // this could be because of a race someone else trying to commit the object
+    // (another bucket cleanup with shared prefix, rollover or an explicit commit)
+    // and since committing the object twice will cause data loss, we should not try to commit it here
+    return 0;
+  }
+  ldpp_dout(dpp, 20) << "INFO: successfully deleted object holding bucket logging object name for bucket '" <<
+      bucket->get_key() << "' during source cleanup" << dendl;
+  // since the object holding the name was deleted, we cannot fetch the last commited name from it
+  if (const int ret = target_bucket->commit_logging_object(obj_name, y, dpp, conf->target_prefix, nullptr); ret < 0) {
+    ldpp_dout(dpp, 5) << "WARNING: could not commit pending logging object of bucket '" <<
+      bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
+    return 0;
+  }
+  ldpp_dout(dpp, 20) << "INFO: successfully committed pending logging object of bucket '" <<
+    bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
+  if (last_committed) {
+    *last_committed = obj_name;
+  }
   return 0;
 }
 
diff --git a/src/rgw/rgw_bucket_logging.h b/src/rgw/rgw_bucket_logging.h
@@ -186,27 +186,6 @@ int rollover_logging_object(const configuration& conf,
     std::string* last_committed,
     std::string* err_message = nullptr);
 
-// commit the pending log object to the log bucket
-// use this for cleanup, when new pending object is not needed
-// and target bucket is known
-// if "last_committed" is not null, it will be set to the name of the last committed object
-int commit_logging_object(const configuration& conf,
-    const std::unique_ptr<rgw::sal::Bucket>& target_bucket,
-    const DoutPrefixProvider *dpp,
-    optional_yield y,
-    std::string* last_committed);
-
-// commit the pending log object to the log bucket
-// use this for cleanup, when new pending object is not needed
-// and target bucket shoud be loaded based on the configuration
-// if "last_committed" is not null, it will be set to the name of the last committed object
-int commit_logging_object(const configuration& conf,
-    const DoutPrefixProvider *dpp,
-    rgw::sal::Driver* driver,
-    const std::string& tenant_name,
-    optional_yield y,
-    std::string* last_committed);
-
 // return the oid of the object holding the name of the temporary logging object
 // bucket - log bucket
 // prefix - logging prefix from configuration. should be used when multiple buckets log into the same log bucket
diff --git a/src/rgw/rgw_rest_bucket_logging.cc b/src/rgw/rgw_rest_bucket_logging.cc
@@ -417,6 +417,11 @@ class RGWPostBucketLoggingOp : public RGWDefaultResponseOp {
 
   void execute(optional_yield y) override {
     const auto& target_bucket_id = target_bucket->get_key();
+    // make sure that the logging source attribute is up-to-date
+    if (const auto ret = rgw::bucketlogging::update_bucket_logging_sources(this, target_bucket, source_bucket->get_key(), true, y); ret < 0) {
+      ldpp_dout(this, 5) << "WARNING: failed to update logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
+        << "' in logging target '" << target_bucket_id << "'. error: " << ret << dendl;
+    }
     std::string obj_name;
     RGWObjVersionTracker objv_tracker;
     op_ret = target_bucket->get_logging_object_name(obj_name, configuration.target_prefix, y, this, &objv_tracker);

Original file line number	Diff line number	Diff line change
`@@ -1140,6 +1140,8 @@ int set_committed_logging_object(RadosStore* store,`
`1140`	`1140`	`bufferlist bl;`
`1141`	`1141`	`bl.append(last_committed);`
`1142`	`1142`	`librados::ObjectWriteOperation op;`
	`1143`	`+ // if object does not exist, we should not create the attribute`
	`1144`	`+ op.assert_exists();`
`1143`	`1145`	`op.setxattr(RGW_ATTR_COMMITTED_LOGGING_OBJ, std::move(bl));`
`1144`	`1146`	`return rgw_rados_operate(dpp, io_ctx, obj_name_oid, std::move(op), y);`
`1145`	`1147`	`}`
`@@ -1174,6 +1176,10 @@ int RadosBucket::get_logging_object_name(std::string& obj_name,`
`1174`	`1176`	`}`
`1175`	`1177`	`return ret;`
`1176`	`1178`	`}`
	`1179`	`+ if (bl.length() == 0) {`
	`1180`	`+ ldpp_dout(dpp, 1) << "ERROR: logging object name at '" << obj_name_oid << "' is empty" << dendl;`
	`1181`	`+ return -ENODATA;`
	`1182`	`+ }`
`1177`	`1183`	`obj_name = bl.to_str();`
`1178`	`1184`	`return 0;`
`1179`	`1185`	`}`