From 62fed9946937cbdda4b6e100a50fc05e9d94ab47 Mon Sep 17 00:00:00 2001 From: Yuval Lifshitz Date: Thu, 4 Sep 2025 10:53:07 +0000 Subject: [PATCH] rgw/logging: allow committing empty objects Fixes: https://tracker.ceph.com/issues/72542 Signed-off-by: Yuval Lifshitz --- doc/radosgw/s3/bucketops.rst | 2 +- src/rgw/driver/rados/rgw_sal_rados.cc | 38 ++++++++++++++++++-------- src/rgw/radosgw-admin/radosgw-admin.cc | 16 ++++------- src/rgw/rgw_bucket_logging.cc | 37 +++++++++++++++++-------- src/rgw/rgw_rest_bucket_logging.cc | 23 ++++++++-------- 5 files changed, 70 insertions(+), 46 deletions(-) diff --git a/doc/radosgw/s3/bucketops.rst b/doc/radosgw/s3/bucketops.rst index 0ae9c60dcc3..21f834afdf5 100644 --- a/doc/radosgw/s3/bucketops.rst +++ b/doc/radosgw/s3/bucketops.rst @@ -919,7 +919,7 @@ Flush Bucket Logging -------------------- Flushes logging object for a given source bucket (if not flushed, the logging objects are written lazily to the log bucket). -Returns the name of the object that was flushed. An empty name will be returned if no object needs to be flushed. +Returns the name of the object that was flushed. Flushing will happen even if the logging object is empty. Syntax ~~~~~~ diff --git a/src/rgw/driver/rados/rgw_sal_rados.cc b/src/rgw/driver/rados/rgw_sal_rados.cc index fe8094316fb..fcc8f631e0e 100644 --- a/src/rgw/driver/rados/rgw_sal_rados.cc +++ b/src/rgw/driver/rados/rgw_sal_rados.cc @@ -1138,7 +1138,7 @@ int RadosBucket::get_logging_object_name(std::string& obj_name, return -EIO; } bufferlist bl; - const int ret = rgw_get_system_obj(store->svc()->sysobj, + if (const int ret = rgw_get_system_obj(store->svc()->sysobj, data_pool, obj_name_oid, bl, @@ -1147,13 +1147,12 @@ int RadosBucket::get_logging_object_name(std::string& obj_name, y, dpp, nullptr, - nullptr); - if (ret < 0) { - if (ret == -ENOENT) { - ldpp_dout(dpp, 20) << "INFO: logging object name '" << obj_name_oid << "' not found. ret = " << ret << dendl; - return ret; + nullptr); ret < 0) { + if (ret != -ENOENT) { + ldpp_dout(dpp, 1) << "ERROR: failed to get logging object name from '" << obj_name_oid << "'. ret = " << ret << dendl; + } else { + ldpp_dout(dpp, 20) << "INFO: logging object name does not exist at '" << obj_name_oid << "'" << dendl; } - ldpp_dout(dpp, 1) << "ERROR: failed to get logging object name from '" << obj_name_oid << "'. ret = " << ret << dendl; return ret; } obj_name = bl.to_str(); @@ -1274,7 +1273,7 @@ int RadosBucket::commit_logging_object(const std::string& obj_name, std::map obj_attrs; ceph::real_time mtime; bufferlist bl_data; - if (const auto ret = rgw_get_system_obj(store->svc()->sysobj, + if (auto ret = rgw_get_system_obj(store->svc()->sysobj, data_pool, temp_obj_name, bl_data, @@ -1284,13 +1283,28 @@ int RadosBucket::commit_logging_object(const std::string& obj_name, dpp, &obj_attrs, nullptr); ret < 0) { - if (ret == -ENOENT) { - ldpp_dout(dpp, 5) << "WARNING: temporary logging object '" << temp_obj_name << "' does not exists" << dendl; - } else { + if (ret != -ENOENT) { ldpp_dout(dpp, 1) << "ERROR: failed to read logging data when committing object '" << temp_obj_name << ". error: " << ret << dendl; + return ret; + } + ldpp_dout(dpp, 20) << "INFO: temporary logging object '" << temp_obj_name << "' does not exist. committing it empty" << dendl; + // creating an empty object + if (ret = rgw_put_system_obj(dpp, store->svc()->sysobj, + data_pool, + temp_obj_name, + bl_data, // empty bufferlist + true, // exclusive + nullptr, + ceph::real_time::clock::now(), + y); ret < 0) { + if (ret == -EEXIST) { + ldpp_dout(dpp, 5) << "WARNING: race detected in committing an empty logging object '" << temp_obj_name << dendl; + } else { + ldpp_dout(dpp, 1) << "ERROR: failed to commit empty logging object '" << temp_obj_name << "'. error: " << ret << dendl; + } + return ret; } - return ret; } uint64_t size = bl_data.length(); diff --git a/src/rgw/radosgw-admin/radosgw-admin.cc b/src/rgw/radosgw-admin/radosgw-admin.cc index 377665ef5b3..c7fc57d50f4 100644 --- a/src/rgw/radosgw-admin/radosgw-admin.cc +++ b/src/rgw/radosgw-admin/radosgw-admin.cc @@ -7812,22 +7812,18 @@ int main(int argc, const char **argv) std::string obj_name; RGWObjVersionTracker objv_tracker; ret = target_bucket->get_logging_object_name(obj_name, configuration.target_prefix, null_yield, dpp(), &objv_tracker); - if (ret < 0) { - cerr << "ERROR: failed to get pending logging object name from target bucket '" << configuration.target_bucket << "'" << std::endl; + if (ret < 0 && ret != -ENOENT) { + cerr << "ERROR: failed to get pending logging object name from target bucket '" << configuration.target_bucket << + "'. error: " << cpp_strerror(-ret) << std::endl; return -ret; } std::string old_obj; const auto region = driver->get_zone()->get_zonegroup().get_api_name(); ret = rgw::bucketlogging::rollover_logging_object(configuration, target_bucket, obj_name, dpp(), region, bucket, null_yield, true, &objv_tracker, &old_obj); if (ret < 0) { - if (ret == -ENOENT) { - cerr << "WARNING: no pending logging object '" << obj_name << "'. nothing to flush"; - ret = 0; - } else { - cerr << "ERROR: failed flush pending logging object '" << obj_name << "'"; - } - cerr << " to target bucket '" << configuration.target_bucket << "'. " - << " last committed object is '" << old_obj << "'" << std::endl; + cerr << "ERROR: failed to flush pending logging object '" << obj_name << "' to target bucket '" << configuration.target_bucket << "'. " + << " last committed object is '" << old_obj << + "'. error: " << cpp_strerror(-ret) << std::endl; return -ret; } cout << "flushed pending logging object '" << old_obj diff --git a/src/rgw/rgw_bucket_logging.cc b/src/rgw/rgw_bucket_logging.cc index b681924aa56..c511e044fac 100644 --- a/src/rgw/rgw_bucket_logging.cc +++ b/src/rgw/rgw_bucket_logging.cc @@ -368,25 +368,40 @@ int rollover_logging_object(const configuration& conf, "', bucket= '" << target_bucket->get_key() << "'" << dendl; return -EINVAL; } - const auto old_obj = obj_name; - const int ret = new_logging_object(conf, target_bucket, obj_name, dpp, region, source_bucket, y, old_obj, objv_tracker); - if (ret == -ECANCELED) { - ldpp_dout(dpp, 20) << "INFO: rollover already performed for object '" << old_obj << "' to logging bucket '" << - target_bucket->get_key() << "'. ret = " << ret << dendl; - return 0; - } else if (ret < 0) { - ldpp_dout(dpp, 1) << "ERROR: failed to rollover object '" << old_obj << "' to logging bucket '" << - target_bucket->get_key() << "'. ret = " << ret << dendl; + + auto old_obj = obj_name.empty() ? std::nullopt : std::optional(obj_name); + + auto handle_error = [&dpp, &old_obj, &target_bucket](int ret) { + if (ret == -ECANCELED) { + ldpp_dout(dpp, 20) << "INFO: rollover already performed for logging object '" << old_obj << "' to logging bucket '" << + target_bucket->get_key() << "'. ret = " << ret << dendl; + return 0; + } + if (ret < 0) { + ldpp_dout(dpp, 1) << "ERROR: failed to rollover logging object '" << old_obj << "' to logging bucket '" << + target_bucket->get_key() << "'. ret = " << ret << dendl; + } + return ret; + }; + + if (const int ret = handle_error(new_logging_object(conf, target_bucket, obj_name, dpp, region, source_bucket, y, old_obj, objv_tracker)); ret < 0) { return ret; } - if (const int ret = target_bucket->commit_logging_object(old_obj, y, dpp, conf.target_prefix, last_committed); ret < 0) { + if (!old_obj) { + // first time logging old == new + old_obj = obj_name; + if (const int ret = handle_error(new_logging_object(conf, target_bucket, obj_name, dpp, region, source_bucket, y, old_obj, objv_tracker)); ret < 0) { + return ret; + } + } + if (const int ret = target_bucket->commit_logging_object(*old_obj, y, dpp, conf.target_prefix, last_committed); ret < 0) { if (must_commit) { return ret; } ldpp_dout(dpp, 5) << "WARNING: failed to commit object '" << old_obj << "' to logging bucket '" << target_bucket->get_key() << "'. ret = " << ret << dendl; // we still want to write the new records to the new object even if commit failed - // will try to commit again next time + // TODO: should add commit retry mechanism } return 0; } diff --git a/src/rgw/rgw_rest_bucket_logging.cc b/src/rgw/rgw_rest_bucket_logging.cc index 89f78558280..28294119456 100644 --- a/src/rgw/rgw_rest_bucket_logging.cc +++ b/src/rgw/rgw_rest_bucket_logging.cc @@ -384,23 +384,22 @@ class RGWPostBucketLoggingOp : public RGWDefaultResponseOp { std::string obj_name; RGWObjVersionTracker objv_tracker; op_ret = target_bucket->get_logging_object_name(obj_name, configuration.target_prefix, y, this, &objv_tracker); - if (op_ret < 0) { - ldpp_dout(this, 1) << "ERROR: failed to get pending logging object name from target bucket '" << target_bucket_id << "'" << dendl; + if (op_ret < 0 && op_ret != -ENOENT) { + ldpp_dout(this, 1) << "ERROR: failed to get pending logging object name from target bucket '" << target_bucket_id << + "'. error: " << op_ret << dendl; return; } + if (op_ret == -ENOENT) { + // no pending object - nothing to flush + ldpp_dout(this, 5) << "INFO: no pending logging object in target bucket '" << target_bucket_id << "'. new object should be created" << dendl; + } const auto region = driver->get_zone()->get_zonegroup().get_api_name(); op_ret = rgw::bucketlogging::rollover_logging_object(configuration, target_bucket, obj_name, this, region, source_bucket, y, true, &objv_tracker, &old_obj); if (op_ret < 0) { - if (op_ret == -ENOENT) { - ldpp_dout(this, 5) << "WARNING: no pending logging object '" << obj_name << "'. nothing to flush" - << " to target bucket '" << target_bucket_id << "'. " - << " last committed object is '" << old_obj << "'" << dendl; - op_ret = 0; - } else { - ldpp_dout(this, 1) << "ERROR: failed flush pending logging object '" << obj_name << "'" - << " to target bucket '" << target_bucket_id << "'. " - << " last committed object is '" << old_obj << "'" << dendl; - } + ldpp_dout(this, 1) << "ERROR: failed to flush pending logging object '" << obj_name << "'" + << " to target bucket '" << target_bucket_id << "'. " + << " last committed object is '" << old_obj << + "'. error: " << op_ret << dendl; return; } ldpp_dout(this, 20) << "INFO: flushed pending logging object '" << old_obj -- 2.39.5