Skip to content

Commit 494fb20

Browse files
yuvalifyuvalif
committed
rgw/logging: deleteting the object holding the temp object name on cleanup
* in case of prefix per source this would prevent leaking this object * in case of share prefix, it would prevent data loss when other source buckets will try to commit an already comitted temporary object * when updatign the "last committed" attribute, the object must exist. this is so that commit without rollover (in case of cleanup) won't recreate the deleted object * some refactoring of try-catch code to have less nesting Fixes: https://tracker.ceph.com/issues/73675 Signed-off-by: Yuval Lifshitz <[email protected]>
1 parent cf44913 commit 494fb20

File tree

2 files changed

+102
-58
lines changed

2 files changed

+102
-58
lines changed

‎src/rgw/driver/rados/rgw_sal_rados.cc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1140,6 +1140,8 @@ int set_committed_logging_object(RadosStore* store,
11401140
bufferlist bl;
11411141
bl.append(last_committed);
11421142
librados::ObjectWriteOperation op;
1143+
// if object does not exist, we should not create the attribute
1144+
op.assert_exists();
11431145
op.setxattr(RGW_ATTR_COMMITTED_LOGGING_OBJ, std::move(bl));
11441146
return rgw_rados_operate(dpp, io_ctx, obj_name_oid, std::move(op), y);
11451147
}
@@ -1174,6 +1176,10 @@ int RadosBucket::get_logging_object_name(std::string& obj_name,
11741176
}
11751177
return ret;
11761178
}
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+
}
11771183
obj_name = bl.to_str();
11781184
return 0;
11791185
}

‎src/rgw/rgw_bucket_logging.cc‎

Lines changed: 96 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -503,8 +503,8 @@ int log_record(rgw::sal::Driver* driver,
503503

504504
// make sure that the logging source attribute is up-to-date
505505
if (ret = update_bucket_logging_sources(dpp, target_bucket, s->bucket->get_key(), true, y); ret < 0) {
506-
ldpp_dout(dpp, 5) << "WARNING: failed to update logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
507-
<< "' in logging bucket '" << target_bucket_id << "'. error: " << ret << dendl;
506+
ldpp_dout(dpp, 5) << "WARNING: could not update bucket logging source '" <<
507+
s->bucket->get_key() << "' in logging bucket '" << target_bucket_id << "' attribute, during record logging. ret = " << ret << dendl;
508508
}
509509

510510
const auto region = driver->get_zone()->get_zonegroup().get_api_name();
@@ -793,8 +793,7 @@ int update_bucket_logging_sources(const DoutPrefixProvider* dpp, std::unique_ptr
793793
ldpp_dout(dpp, 5) << "WARNING: failed to decode logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
794794
<< "' for bucket '" << bucket->get_key() << "' when updating logging sources. error: " << err.what() << dendl;
795795
}
796-
ldpp_dout(dpp, 20) << "INFO: logging source '" << src_bucket_id << "' already " <<
797-
(add ? "added to" : "removed from") << " bucket '" << bucket->get_key() << "'" << dendl;
796+
// no change
798797
return 0;
799798
}, y);
800799
}
@@ -808,53 +807,59 @@ int bucket_deletion_cleanup(const DoutPrefixProvider* dpp,
808807
// and also delete the object holding the pending object name
809808
auto& attrs = bucket->get_attrs();
810809
if (const auto iter = attrs.find(RGW_ATTR_BUCKET_LOGGING_SOURCES); iter != attrs.end()) {
810+
source_buckets sources;
811811
try {
812-
source_buckets sources;
813812
ceph::decode(sources, iter->second);
814-
for (const auto& source : sources) {
815-
std::unique_ptr<rgw::sal::Bucket> src_bucket;
816-
if (const int ret = driver->load_bucket(dpp, source, &src_bucket, y); ret < 0) {
817-
ldpp_dout(dpp, 5) << "WARNING: failed to get logging source bucket '" << source << "' for logging bucket '" <<
813+
} catch (buffer::error& err) {
814+
ldpp_dout(dpp, 5) << "WARNING: failed to decode logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
815+
<< "' for logging bucket '" << bucket->get_key() << "'. error: " << err.what() << dendl;
816+
return -EIO;
817+
}
818+
// since we removed the attribute, we should not return error code from now on
819+
// any retry, would find no attribute and return success
820+
for (const auto& source : sources) {
821+
std::unique_ptr<rgw::sal::Bucket> src_bucket;
822+
if (const int ret = driver->load_bucket(dpp, source, &src_bucket, y); ret < 0) {
823+
ldpp_dout(dpp, 5) << "WARNING: failed to get logging source bucket '" << source << "' for logging bucket '" <<
824+
bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
825+
continue;
826+
}
827+
auto& src_attrs = src_bucket->get_attrs();
828+
if (const auto iter = src_attrs.find(RGW_ATTR_BUCKET_LOGGING); iter != src_attrs.end()) {
829+
configuration conf;
830+
try {
831+
auto bl_iter = iter->second.cbegin();
832+
decode(conf, bl_iter);
833+
} catch (buffer::error& err) {
834+
ldpp_dout(dpp, 5) << "WARNING: failed to decode logging attribute '" << RGW_ATTR_BUCKET_LOGGING
835+
<< "' of bucket '" << src_bucket->get_key() << "' during cleanup. error: " << err.what() << dendl;
836+
continue;
837+
}
838+
std::string obj_name;
839+
RGWObjVersionTracker objv;
840+
if (const int ret = bucket->get_logging_object_name(obj_name, conf.target_prefix, y, dpp, &objv); ret < 0) {
841+
ldpp_dout(dpp, 5) << "WARNING: failed to get logging object name for logging bucket '" << bucket->get_key() <<
842+
"' during cleanup. ret = " << ret << dendl;
843+
continue;
844+
}
845+
if (const int ret = bucket->remove_logging_object_name(conf.target_prefix, y, dpp, &objv); ret < 0) {
846+
ldpp_dout(dpp, 5) << "WARNING: failed to delete object holding bucket logging object name for logging bucket '" <<
818847
bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
819848
continue;
820849
}
821-
auto& src_attrs = src_bucket->get_attrs();
822-
if (const auto iter = src_attrs.find(RGW_ATTR_BUCKET_LOGGING); iter != src_attrs.end()) {
823-
configuration conf;
824-
try {
825-
auto bl_iter = iter->second.cbegin();
826-
decode(conf, bl_iter);
827-
std::string obj_name;
828-
RGWObjVersionTracker objv;
829-
if (const int ret = bucket->get_logging_object_name(obj_name, conf.target_prefix, y, dpp, &objv); ret < 0) {
830-
ldpp_dout(dpp, 5) << "WARNING: failed to get logging object name for logging bucket '" << bucket->get_key() <<
831-
"' during cleanup. ret = " << ret << dendl;
832-
continue;
833-
}
834-
if (const int ret = bucket->remove_logging_object(obj_name, y, dpp); ret < 0) {
835-
ldpp_dout(dpp, 5) << "WARNING: failed to delete pending logging object '" << obj_name << "' for logging bucket '" <<
836-
bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
837-
continue;
838-
}
839-
ldpp_dout(dpp, 20) << "INFO: successfully deleted pending logging object '" << obj_name << "' from deleted logging bucket '" <<
840-
bucket->get_key() << "'" << dendl;
841-
if (const int ret = bucket->remove_logging_object_name(conf.target_prefix, y, dpp, &objv); ret < 0) {
842-
ldpp_dout(dpp, 5) << "WARNING: failed to delete object holding bucket logging object name for logging bucket '" <<
843-
bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
844-
continue;
845-
}
846-
ldpp_dout(dpp, 20) << "INFO: successfully deleted object holding bucket logging object name from deleted logging bucket '" <<
847-
bucket->get_key() << "'" << dendl;
848-
} catch (buffer::error& err) {
849-
ldpp_dout(dpp, 5) << "WARNING: failed to decode logging attribute '" << RGW_ATTR_BUCKET_LOGGING
850-
<< "' of bucket '" << src_bucket->get_key() << "' during cleanup. error: " << err.what() << dendl;
851-
}
850+
ldpp_dout(dpp, 20) << "INFO: successfully deleted object holding bucket logging object name from deleted logging bucket '" <<
851+
bucket->get_key() << "'" << dendl;
852+
if (const int ret = bucket->remove_logging_object(obj_name, y, dpp); ret < 0) {
853+
ldpp_dout(dpp, 5) << "WARNING: failed to delete pending logging object '" << obj_name << "' for logging bucket '" <<
854+
bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
855+
continue;
852856
}
857+
ldpp_dout(dpp, 20) << "INFO: successfully deleted pending logging object '" << obj_name << "' from deleted logging bucket '" <<
858+
bucket->get_key() << "'" << dendl;
859+
} else {
860+
ldpp_dout(dpp, 5) << "WARNING: no logging attribute '" << RGW_ATTR_BUCKET_LOGGING
861+
<< "' found in logging source bucket '" << src_bucket->get_key() << "' during cleanup" << dendl;
853862
}
854-
} catch (buffer::error& err) {
855-
ldpp_dout(dpp, 5) << "WARNING: failed to decode logging sources attribute '" << RGW_ATTR_BUCKET_LOGGING_SOURCES
856-
<< "' for logging bucket '" << bucket->get_key() << "'. error: " << err.what() << dendl;
857-
return -EIO;
858863
}
859864
}
860865

@@ -879,7 +884,7 @@ int source_bucket_cleanup(const DoutPrefixProvider* dpp,
879884
conf = std::move(tmp_conf);
880885
} catch (buffer::error& err) {
881886
ldpp_dout(dpp, 5) << "WARNING: failed to decode existing logging attribute '" << RGW_ATTR_BUCKET_LOGGING
882-
<< "' of bucket '" << bucket->get_key() << "' during cleanup. error: " << err.what() << dendl;
887+
<< "' of bucket '" << bucket->get_key() << "' during source cleanup. error: " << err.what() << dendl;
883888
return -EIO;
884889
}
885890
if (remove_attr) {
@@ -892,34 +897,67 @@ int source_bucket_cleanup(const DoutPrefixProvider* dpp,
892897
}, y); ret < 0) {
893898
if (remove_attr) {
894899
ldpp_dout(dpp, 5) << "WARNING: failed to remove logging attribute '" << RGW_ATTR_BUCKET_LOGGING <<
895-
"' from bucket '" << bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
900+
"' from bucket '" << bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
896901
}
897902
return ret;
898903
}
904+
if (last_committed) {
905+
last_committed->clear();
906+
}
899907
if (!conf) {
900908
// no logging attribute found
901909
return 0;
902910
}
903-
const auto& info = bucket->get_info();
904-
if (const int ret = commit_logging_object(*conf, dpp, driver, info.bucket.tenant, y, last_committed); ret < 0) {
905-
ldpp_dout(dpp, 5) << "WARNING: could not commit pending logging object of bucket '" <<
906-
bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
907-
} else {
908-
ldpp_dout(dpp, 20) << "INFO: successfully committed pending logging object of bucket '" << bucket->get_key() << "'" << dendl;
909-
}
911+
// since we removed the attribute, we should not return error code from now on
912+
// any retry, would find no attribute and return success
910913
rgw_bucket target_bucket_id;
914+
const auto& info = bucket->get_info();
911915
if (const int ret = get_bucket_id(conf->target_bucket, info.bucket.tenant, target_bucket_id); ret < 0) {
912916
ldpp_dout(dpp, 5) << "WARNING: failed to parse logging bucket '" <<
913-
conf->target_bucket << "' during cleanup. ret = " << ret << dendl;
917+
conf->target_bucket << "' during source cleanup. ret = " << ret << dendl;
918+
return 0;
919+
}
920+
std::unique_ptr<rgw::sal::Bucket> target_bucket;
921+
const int ret = driver->load_bucket(dpp, target_bucket_id, &target_bucket, y);
922+
if (ret < 0) {
923+
ldpp_dout(dpp, 5) << "WARNING: failed to get logging bucket '" << target_bucket_id <<
924+
"' when doing source cleanup. ret = " << ret << dendl;
914925
return 0;
915926
}
916-
if (const int ret = update_bucket_logging_sources(dpp, driver, target_bucket_id, bucket->get_key(), false, y); ret < 0) {
927+
if (const int ret = update_bucket_logging_sources(dpp, target_bucket, bucket->get_key(), false, y); ret < 0) {
917928
ldpp_dout(dpp, 5) << "WARNING: could not update bucket logging source '" <<
918-
bucket->get_key() << "' during cleanup. ret = " << ret << dendl;
929+
bucket->get_key() << "' in logging bucket '" << target_bucket_id << "' attribute, during source cleanup. ret = " << ret << dendl;
930+
}
931+
std::string obj_name;
932+
RGWObjVersionTracker objv;
933+
if (const int ret = target_bucket->get_logging_object_name(obj_name, conf->target_prefix, y, dpp, &objv); ret < 0) {
934+
ldpp_dout(dpp, 5) << "WARNING: failed to get logging object name for logging bucket '" << bucket->get_key() <<
935+
"' during source cleanup. ret = " << ret << dendl;
936+
// if name cannot be fetched, we should not commit or remove the object holding the name
937+
// it is better to leak an object than to cause a possible data loss
938+
return 0;
939+
}
940+
if (const int ret = target_bucket->remove_logging_object_name(conf->target_prefix, y, dpp, &objv); ret < 0) {
941+
ldpp_dout(dpp, 5) << "WARNING: failed to delete object holding bucket logging object name for bucket '" <<
942+
bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
943+
// this could be because of a race someone else trying to commit the object
944+
// (another bucket cleanup with shared prefix, rollover or an explicit commit)
945+
// and since committing the object twice will cause data loss, we should not try to commit it here
919946
return 0;
920947
}
921-
ldpp_dout(dpp, 20) << "INFO: successfully updated bucket logging source '" <<
922-
bucket->get_key() << "' during cleanup"<< dendl;
948+
ldpp_dout(dpp, 20) << "INFO: successfully deleted object holding bucket logging object name for bucket '" <<
949+
bucket->get_key() << "' during source cleanup" << dendl;
950+
// since the object holding the name was deleted, we cannot fetch the last commited name from it
951+
if (const int ret = target_bucket->commit_logging_object(obj_name, y, dpp, conf->target_prefix, nullptr); ret < 0) {
952+
ldpp_dout(dpp, 5) << "WARNING: could not commit pending logging object of bucket '" <<
953+
bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
954+
return 0;
955+
}
956+
ldpp_dout(dpp, 20) << "INFO: successfully committed pending logging object of bucket '" <<
957+
bucket->get_key() << "' during source cleanup. ret = " << ret << dendl;
958+
if (last_committed) {
959+
*last_committed = obj_name;
960+
}
923961
return 0;
924962
}
925963

0 commit comments

Comments
 (0)