Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: abort multipart if upload meta object doesn't exist #19918

Merged
merged 1 commit into from Jan 18, 2018

Conversation

fangyuxiangGL
Copy link
Contributor

@fangyuxiangGL fangyuxiangGL commented Jan 11, 2018

Hi @cbodley

I am not sure if the phenomenon mentioned below is an issue.

For testing, we configured a lc rule that will delete residual multipart uploaded a day ago, then we configured rgw_lc_debug_interval to 300, which meant that lc rule will delete residual multipart uploaded 300 seconds ago.

Then we started to upload a big object use s3 multipart and got 'invalid part' error finally, which was caused by lc process for the uploading time exceeded 300 seconds and it was reasonable.

But the result was that there were some residual parts from the upload and they can't be deleted by lc process any more, the reason was that upload meta object was deleted from the bucket index in last process by lc, so they will be forever residual parts.

So is it reasonable that we test if upload meta object is existing before set current part to its omap? The part will fail if upload meta object is not existing.

@fangyuxiangGL fangyuxiangGL changed the title rgw: multipart abort if upload meta object doesn't exist rgw: abort multipart if upload meta object doesn't exist Jan 11, 2018
@cbodley
Copy link
Contributor

cbodley commented Jan 11, 2018

So is it reasonable that we test if upload meta object is existing before set current part to its omap? The part will fail if upload meta object is not existing.

that does sound reasonable, yeah. i'm guessing we could end up in the same situation with a racing AbortMultipart too? my question is, after detecting EEXIST on the meta object with your approach here, do we still leak the part objects themselves? we might want some cleanup logic in that case that schedules the part for gc (similar to abort_multipart_upload())

@@ -3130,8 +3130,7 @@ int RGWPutObjProcessor_Multipart::do_complete(size_t accounted_size,

store->obj_to_raw(s->bucket_info.placement_rule, meta_obj, &raw_meta_obj);

r = store->omap_set(raw_meta_obj, p, bl);

r = store->omap_set(raw_meta_obj, p, bl, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could use a clarifying comment, ex:

  const bool must_exist = true; // detect races with abort
  r = store->omap_set(raw_meta_obj, p, bl, must_exist);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbodley
great idea!

@fangyuxiangGL
Copy link
Contributor Author

fangyuxiangGL commented Jan 11, 2018

yeah. i'm guessing we could end up in the same situation with a racing AbortMultipart too?

Did you mean that HTTP AbortMultipart race with HTTP PutObject?

my question is, after detecting EEXIST on the meta object with your approach here

yeah, my approach is to detect the meta object exist, in other words meta object related to the upload-id must exist when a part is being uploaded.

do we still leak the part objects themselves?

could you explain what part objects point to?

Signed-off-by: fang yuxiang fang.yuxiang@eisoo.com
@fangyuxiangGL
Copy link
Contributor Author

fangyuxiangGL commented Jan 11, 2018

@cbodley

I think i got u, thanks!

There is one kind of race conditon.
t1 AbortMultipart-send_gc_chain
t2 PutObject-omap_set (successful !!!)
t3 AbortMultipart-delete_obj
then the part uploaded by PutObject will be leaked.

A perfect method maybe need iterate the rados pool upload meta object. Do you have better idea?

@cbodley
Copy link
Contributor

cbodley commented Jan 11, 2018

i had something a bit different in mind:

  • PutObj starts, read_obj_policy() finds the multipart meta obj, starts transfering data for the part
  • AbortMultipart comes in, completes, and removes the meta obj
  • PutObj finishes writing the data, and omap_set on the meta obj fails with ENOENT

my idea was to, when PutObj detects ENOENT there, schedule the part for gc

@fangyuxiangGL
Copy link
Contributor Author

fangyuxiangGL commented Jan 15, 2018

PutObj starts, read_obj_policy() finds the multipart meta obj, starts transfering data for the part
AbortMultipart comes in, completes, and removes the meta obj
PutObj finishes writing the data, and omap_set on the meta obj fails with ENOENT
my idea was to, when PutObj detects ENOENT there, schedule the part for gc

@cbodley If I am not mistaken, the process you mentioned above may not cause part leaked.
Codes in RGWPutObjProcessor_Aio::~RGWPutObjProcessor_Aio() will cleanup rados objects written in that part:

  for (iter = written_objs.begin(); iter != written_objs.end(); ++iter) {
    const rgw_raw_obj& obj = *iter;
    if (!head_obj.empty() && obj == raw_head) {
      ldout(store->ctx(), 5) << "NOTE: we should not process the head object (" << obj << ") here" << dendl;
      need_to_remove_head = true;
      continue;
    }

    int r = store->delete_raw_obj(obj);
    if (r < 0 && r != -ENOENT) {
      ldout(store->ctx(), 5) << "WARNING: failed to remove obj (" << obj << "), leaked" << dendl;
    }
  }

@fangyuxiangGL
Copy link
Contributor Author

fangyuxiangGL commented Jan 15, 2018

I think this is a kind of race conditon that will cause data leak, but it may be low probability.

There is one kind of race conditon.
t1 AbortMultipart-send_gc_chain
t2 PutObject-omap_set (successful !!!)
t3 AbortMultipart-delete_obj
then the part uploaded by PutObject will be leaked.

@yuriw
Copy link
Contributor

yuriw commented Jan 16, 2018

@yuriw yuriw merged commit 47713dc into ceph:master Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants