rgw/sfs: recognize delete-marker on DeleteObj

[giubacc]

RGWDeleteObj::execute() relies on the return code from rgw::sal::Object::get_obj_state() to ascertain the object being deleted is a regular object rather than a delete-marker.
The rule is to return -ENOENT to signal a delete-marker.
get_obj_state() in rgw/sfs was returning always 0 for any object, therefore also a delete-marker fell into a regular object in RGWDeleteObj::execute().
This is wrong because, when dealing with object lock checks, a delete-marker is always allowed to be deleted regardless of the object's retention mode. Returing 0 was incorrectly preventing a delete-marker to be deleted for object-lock protected objects.

Fixes: https://github.com/aquarist-labs/s3gw/issues/690

Contribution Guidelines

• To sign and title your commits, please refer to Submitting Patches to Ceph.

Checklist

• Tracker (select at least one)
  • References tracker ticket
  • Very recent bug; references commit where it was introduced
  • New feature (ticket optional)
  • Doc update (no ticket needed)
  • Code cleanup (no ticket needed)
• Documentation (select at least one)
  • Updates relevant documentation
  • No doc update is appropriate
• Tests (select at least one)
  • Includes unit test(s)
  • Includes integration test(s)
  • Includes bug reproducer
  • No tests

[giubacc]

Unfortunately, this patch even if it seems correct, breaks some tests:

test_lifecycle_deletemarker_expiration
test_multi_object_delete
test_multi_objectv2_delete
test_versioning_concurrent_multi_object_delete
test_versioning_multi_object_delete
test_versioning_multi_object_delete_with_marker
test_versioning_multi_object_delete_with_marker_create

Reasons to say this is correct are:

rgw_op.cc::void RGWDeleteObj::execute(optional_yield y)
...
        if (check_obj_lock) {
          /* check if obj exists, read orig attrs */
          if (op_ret == -ENOENT) {
            /* object maybe delete_marker, skip check_obj_lock*/
            check_obj_lock = false;
          } else {
            return;
          }
        }
...

pay especially attention to the comment: /* object maybe delete_marker, skip check_obj_lock*/ that seems to suggest that a delete-marker is recognized with a -ENOENT return code.
A delete-marker is always allowed to be deleted even when there is an object-lock protection over an object.
This is exactly why this patch has been provided, to fix the test_object_lock_delete_object_with_retention_and_marker test (we currently don't allow a delete marker to be deleted under object locking; this is wrong) .
The hypothesis that returning an -ENOENT return code is the way to signal a delete-marker seems to find confirmation with the DB::Object::get_obj_state implementation:

      if (ent.flags & rgw_bucket_dir_entry::FLAG_DELETE_MARKER) {
        ret = -ENOENT;
        goto out;
      }

The rados implementation is less straightforward but the -ENOENT is returned as well in cases that seem not errors but legit special cases.

@jecluis @0xavi0 @tserong @irq0
Suggestions how to proceed here?

[jecluis]

Unfortunately, this patch even if it seems correct, breaks some tests:

test_lifecycle_deletemarker_expiration
test_multi_object_delete
test_multi_objectv2_delete
test_versioning_concurrent_multi_object_delete
test_versioning_multi_object_delete
test_versioning_multi_object_delete_with_marker
test_versioning_multi_object_delete_with_marker_create

What causes these tests to fail?

[giubacc]

Unfortunately, this patch even if it seems correct, breaks some tests:

test_lifecycle_deletemarker_expiration
test_multi_object_delete
test_multi_objectv2_delete
test_versioning_concurrent_multi_object_delete
test_versioning_multi_object_delete
test_versioning_multi_object_delete_with_marker
test_versioning_multi_object_delete_with_marker_create

What causes these tests to fail?

The problem seems to be that all the occurrences like this:

  ret = obj->get_obj_state(dpp, &obj_state, null_yield, true);
  if (ret < 0) {
    return ret;
  }

Are not adequate when dealing with a delete-marker.
If the rule for get_obj_state is to return -ENOENT for delete markers, all the (ret < 0) guards are not correct, because they should contemplate the -ENOENT return code.
test_lifecycle_deletemarker_expiration is failing due to this, I haven't checked all the other cases, but I assume they can all be traced back to this case .

[giubacc]

opened upstream issue https://tracker.ceph.com/issues/63542 asking for clarifications