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: fix leaks with incomplete multiparts #15630

Merged
merged 4 commits into from Jul 12, 2017

Conversation

Projects
None yet
6 participants
@Abhishekvrshny
Contributor

Abhishekvrshny commented Jun 12, 2017

@Abhishekvrshny

This comment has been minimized.

Contributor

Abhishekvrshny commented Jun 12, 2017

jenkins, test this please

}
if (!objs.empty()) {
ldout(s->cct, 0) << "ERROR: bucket " << s->bucket_name << " not empty, incomplete multiparts found" << dendl;
op_ret = -ENOTEMPTY;

This comment has been minimized.

@cbodley

cbodley Jun 14, 2017

Contributor

is this the behavior in aws? it's not clear to me, based on http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketDELETE.html

This comment has been minimized.

@Abhishekvrshny

Abhishekvrshny Jun 15, 2017

Contributor

@cbodley Right, this is not compatible with current AWS behaviour. I even tried deleting a bucket in S3 with an incomplete multipart upload and it was successful. What I am not sure is whether, S3 would clean this up in background or charge me for the leaked objects, as mentioned here
Do you recommend aborting the incomplete uploads here without throwing any error?

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

Do you recommend aborting the incomplete uploads here without throwing any error?

that sounds like the best approach to me

This comment has been minimized.

@Abhishekvrshny

Abhishekvrshny Jun 15, 2017

Contributor

Sure, sounds good. I'll re-submit the changes to the PR. Do you have any other comments?

@cbodley

consider moving some of this logic into a separate abort_bucket_multiparts() function to avoid duplicating the loops

do {
objs.clear();
string prefix = "", marker = "", delimiter = "";

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

std::string defaults to an empty string already, the = "" bits are unnecessary

if (!entry.mp.from_meta(key.name))
continue;
entry.obj = *iter;
abort_multipart_upload(store,cct,&obj_ctx,info,entry.mp);

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

nit: spaces after the commas here

list_op.params.delim = delim;
list_op.params.marker = marker;
list_op.params.ns = RGW_OBJ_NS_MULTIPART;
list_op.params.filter = filter;

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

i'm guessing that all callers of list_bucket_multiparts() will want to use a MultipartMetaFilter. can we just put one on the stack here so callers don't have to pass one in?

  MultipartMetaFilter filter;
  list_op.params.filter = &filter;
if (!objs.empty() && delete_children) {
vector<rgw_bucket_dir_entry>::iterator iter;
RGWMultipartUploadEntry entry;
for (iter = objs.begin(); iter != objs.end(); ++iter) {

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

consider a ranged-for loop here: for (const auto& obj : objs) {

continue;
entry.obj = *iter;
abort_multipart_upload(store,cct,&obj_ctx,info,entry.mp);
ldout(store->ctx(),0) << "WARNING : cleaned incomplete multipart" << dendl;

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

this could get spammy, maybe just count these up and print a single warning after the loops? i.e. WARNING: aborted 15 incomplete multipart uploads

return -ENOTEMPTY;
}
} while (!objs.empty());

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

} while (is_truncated); is generally used here

op_ret = -ENOTEMPTY;
return;
}

This comment has been minimized.

@cbodley

cbodley Jun 15, 2017

Contributor

shouldn't this be in a do-while loop like the others?

@Abhishekvrshny

This comment has been minimized.

Contributor

Abhishekvrshny commented Jun 16, 2017

@cbodley : Re-pushed recommended changes to the PR, which includes:

  • On deleting a bucket which just has incomplete multiparts, they are now aborted and bucket deletion is successful.
  • Added abort_bucket_multiparts() function in rgw_multi.cc to avoid repetitive loops.
  • Made other misc. changes as suggested.
if (!objs.empty() && !delete_children) {
lderr(store->ctx()) << "ERROR: could not remove non-empty bucket " << bucket.name << dendl;
return -ENOTEMPTY;
}

This comment has been minimized.

@cbodley

cbodley Jun 16, 2017

Contributor

i'm not sure why this if (delete_children) { block needed to change? when delete_children is false, we shouldn't need to do this object listing

store->delete_bucket() below should handle the check for ENOTEMPTY

This comment has been minimized.

@Abhishekvrshny

Abhishekvrshny Jun 16, 2017

Contributor

store->delete_bucket() does handle ENOTEMPTY, but it wouldn't be nice to abort multiparts first and then return ENOTEMPTY in case regular objects are found in the bucket. Right now, this behaviour is same as that from S3 API.

This comment has been minimized.

@cbodley

cbodley Jun 16, 2017

Contributor

gotcha, thanks. though i suppose that could happen anyway, if a concurrent upload came in during the call to abort_bucket_multiparts()

This comment has been minimized.

@Abhishekvrshny

Abhishekvrshny Jun 17, 2017

Contributor

Right. But I guess such racy conditions exist at different other places too, like an object may be uploaded between https://github.com/ceph/ceph/blob/master/src/rgw/rgw_op.cc#L2667 and https://github.com/ceph/ceph/blob/master/src/rgw/rgw_op.cc#L2686, and yet bucket would be deleted. There has to be some sort of read lock while a delete operation is in progress. What do you think? Can we take that as a separate issue to fix?

This comment has been minimized.

@Abhishekvrshny

This comment has been minimized.

@cbodley

cbodley Jun 20, 2017

Contributor

@Abhishekvrshny yeah, i don't see this as a blocker

}
num_deleted++;
}
ldout(store->ctx(),0) << "WARNING : aborted " << num_deleted << " incomplete multipart uploads" << dendl;

This comment has been minimized.

@cbodley

cbodley Jun 16, 2017

Contributor

can enclose this in if (num_deleted) {}

This comment has been minimized.

@Abhishekvrshny

Abhishekvrshny Jun 16, 2017

Contributor

sure, would do this.

@Abhishekvrshny

This comment has been minimized.

Contributor

Abhishekvrshny commented Jun 20, 2017

}
num_deleted++;
}
if (num_deleted) { ldout(store->ctx(),0) << "WARNING : aborted " << num_deleted << " incomplete multipart uploads" << dendl; }

This comment has been minimized.

@cbodley

cbodley Jun 20, 2017

Contributor

thanks, but please add line breaks (according to https://github.com/ceph/ceph/blob/master/CodingStyle)

      if (num_deleted) {
        ldout(store->ctx(),0) << "WARNING : aborted " << num_deleted << " incomplete multipart uploads" << dendl;
      }
@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 20, 2017

looking good, i'll run this through teuthology

Abhishekvrshny added some commits Jun 16, 2017

rgw: add functions to list and abort multipart uploads
* list_bucket_multiparts()
* abort_bucket_multiparts()

Fixes: http://tracker.ceph.com/issues/17164
Signed-off-by: Abhishek Varshney <abhishek.varshney@flipkart.com>
rgw: In RGWListBucketMultiparts::execute(), use list_bucket_multipart()
Fixes: http://tracker.ceph.com/issues/17164
Signed-off-by: Abhishek Varshney <abhishek.varshney@flipkart.com>
rgw: abort incomplete multiparts on bucket delete
Fixes: http://tracker.ceph.com/issues/17164
Signed-off-by: Abhishek Varshney <abhishek.varshney@flipkart.com>
rgw: fix leaks with incomplete multiparts in delete bucket
Fixes: http://tracker.ceph.com/issues/17164
Signed-off-by: Abhishek Varshney <abhishek.varshney@flipkart.com>
@Abhishekvrshny

This comment has been minimized.

Contributor

Abhishekvrshny commented Jun 20, 2017

@cbodley : Thanks. I just re-pushed it with line breaks.

@cbodley cbodley added the needs-qa label Jun 23, 2017

@Abhishekvrshny

This comment has been minimized.

Contributor

Abhishekvrshny commented Jul 9, 2017

@cbodley Did this go through teuthology runs?

@yuriw yuriw merged commit b932d7f into ceph:master Jul 12, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details
@zhouyuan

This comment has been minimized.

Contributor

zhouyuan commented Sep 11, 2017

@Abhishekvrshny
is it possible to have this backported to Jewel also?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Sep 11, 2017

@zhouyuan i am updating the tracker ticket. and you can also help backport it, the stable release team will pick it up in the test batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment