rgw: fix abort multipart in lc when enable index shard

[joke-lee]

rgw_override_bucket_index_max_shards = 128
rgw_enable_lc_threads = false
rgw_lifecycle_work_time = "00:00-24:00"
rgw lc debug interval = 1

we set shard num 128

#!/usr/bin/env python
# -*- coding:utf-8 -*-
from boto3.session import Session
import boto3
access_key = "user1"
secret_key = "user1"
session = Session(access_key, secret_key)
url = "http://127.0.0.1"
config_dict = { 'signature_version' : 's3', 'connect_timeout': 30000, 'read_timeout': 30000}
config = boto3.session.Config(**config_dict)
s3_client = session.client('s3', endpoint_url=url, config=config)
bucket = "test1"
key = "异形契约.mp4"
mpu = s3_client.create_multipart_upload(Bucket=bucket, Key=key)

and get which shard the obj store in

for i in `./bin/rados -p default.rgw.buckets.index ls -c ceph.conf`; do echo $i "\t start";./bin/rados -p default.rgw.buckets.index listomapkeys $i -c ceph.conf; echo "\t end";done

...
.dir.96e298ca-3b00-4aff-8faf-c13d35e14925.14133.1.39
_multipart_异形契约.mp4.2~7Be__tcdECK1K_OpIfYr645lzGoCxAZ.meta
...

the obj in shard 39

set the lc rule

from boto3.session import Session
import boto3
access_key = "user1"
secret_key = "user1"
session = Session(access_key, secret_key)
url = "http://127.0.0.1"
config_dict = { 'signature_version' : 's3', 'connect_timeout': 30000, 'read_timeout': 30000}
config = boto3.session.Config(**config_dict)
s3_client = session.client('s3', endpoint_url=url, config=config)
s3_client.put_bucket_lifecycle(Bucket='test1', LifecycleConfiguration={
'Rules': [
{
    'ID': 'test',
    'Prefix': '',
    'Status': 'Enabled',
    'AbortIncompleteMultipartUpload': {
                    'DaysAfterInitiation': 1
                },
    'Expiration': {
                    'ExpiredObjectDeleteMarker': True
                }

},
]
}
)

and the abort will never execute

  uint32_t current_shard;
  if (shard_id >= 0) {
    current_shard = shard_id;
  } else if (my_start.empty()) {
    current_shard = 0u;
  } else {
    current_shard =
      rgw_bucket_shard_index(my_start.name, num_shards);  <== the old code go here and get the  current_shard value 101, so the obj in 101-128 shard will execute abort but the obj in shard 39 will  never execute,
  }

Signed-off-by: yuliyang yuliyang@cmss.chinamobile.com

[joke-lee]

hi @cbodley , would you mind take a review? thanks

[mattbenjamin]

@ivancich could you have a look at this, as well? i.e., as a potential reproducer downstream

[joke-lee]

hi @cbodley , would you mind take a review? thanks

[ivancich]

This is an interesting issue and leads to an important insight.

Looking at the code, the issue seems to be using the start parameter in cls_bucket_list_unordered in two different ways -- as a marker to allow sequential calls to move through the entire listing vs. as a prefix to jump past uninteresting listings and get to the interesting ones.

The code works fine in the former. But because the listing is unordered, using start as a prefix fails. It's worth noting that in the ordered case cls_bucket_list_ordered it will work fine in both conditions.

The proposed solution seems to be a hack, treating a start of _multipart_ in a special manner.

The simpler solution would be to call cls_bucket_list_ordered in the case where we want to do a prefix search and expect all entries that begin with _multipart_ to emerge sequentially, as that would allow the loop on the caller to exit when it encounters the first entry that does not have the prefix _multipart_.

If we really do want to use cls_bucket_list_unordered, then the calling code needs to be aware of the semantics. For the first call it needs to send the empty string as start, and use it as a marker as it moves through the listings. The calling code also needs to be aware that there will be non-_multipart_ entries interspersed among the _multipart_ entries and that it will need to exhaust the listing.

I'd like to discuss this with Matt and Casey and at this moment I do not know what would be the better solution. I do not believe that the proposed solution is ideal, though.

[ivancich]

@mattbenjamin , @cbodley Would you mind looking at my review comment and weighing in? As you'll see, I think we need to either change the call to the cls_bucket_list_ordered variant or make sure the caller handles the semantics of the ..._unordered call.

Either way I think I need to better document cls_bucket_list_unordered to emphasize that start should not be used as a prefix and sequences of similarly prefixed entries should never be assumed.

[ivancich]

I've put together what I believe is a more general fix to the issue you idenified (thank you!). I tried to add you as a reviewer but was unable to and I'm not sure why.

#26658

[ivancich]

For the reasons I outlined previously, I do not think this is appropriate for merging.

In my previous comments I did mis-describe the issue. I thought the prefix was being sent in explicitly as the marker. What's actually happening, though, is more complex in that the namespace is being moved into the marker.

[ivancich]

@joke-lee After you look at the alternate solution, if you agree, perhaps we can close this PR. Again, thank you for your help here; it was instrumental.

[ivancich]

Thank you @joke-lee for uncovering this issue. As I explored the problem I realized the issues were quite deep. I think we have a solution that handles the deeper issues here: #26658 .

So I'm going to close this PR. If you think I've closed it in error please let me know.