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: allow system users to read SLO parts #13561

Merged
merged 3 commits into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@cbodley
Contributor

cbodley commented Feb 21, 2017

multisite data sync relies on fetching the object as the system user

Fixes: http://tracker.ceph.com/issues/19027

@cbodley cbodley requested review from yehudasa and rzarzynski Feb 21, 2017

@rzarzynski

LGTM.

@yehudasa

This comment has been minimized.

Member

yehudasa commented Feb 21, 2017

@cbodley @rzarzynski should we be sync the virtual object or the manifest itself? If the latter, we shouldn't iterate over the parts

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Feb 21, 2017

@yehudasa: good point. If someone needs to go with an SLO, then the virtual object is most likely to large to handle it in the usual way. I think we should sync manifests separately.

I'm not sure whether we offer support for Swift's multipart-manifest=get at the moment. RadosGW is compiling... ;-)

@cbodley

This comment has been minimized.

Contributor

cbodley commented Feb 21, 2017

in testing the sync, it looks like the GET request to fetch the base object is reading all of its data, and also its slo manifest attribute. after sync, when reading that object on the secondary zone, it's seeing that slo attribute and reading data from its parts instead. so we may want to add a special header to the GET request to avoid fetching data from the parts during sync

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 8, 2017

@rzarzynski thanks for pointing out multipart-manifest=get! the skip_manifest flag used by swift to implement that is exactly what i needed for sync requests

i've updated the pr so that RGWRados::fetch_remote_obj() adds a rgwx-sync-manifest header, which sets skip_manifest to avoid fetching object data from the slo parts

still needs testing, but pushed for early feedback

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 13, 2017

updated after further testing. the skip_manifest flag was only applying to RGW_ATTR_USER_MANIFEST and not RGW_ATTR_SLO_MANIFEST. with that fixed, data sync no longer fetches any object data from the manifest object

to test, i uploaded a big file to the master zone:

$ dd bs=1M count=1024 if=/dev/zero of=big.iso
$ swift -A http://localhost:8000/auth/1.0 -U test:tester -K testing upload --use-slo -S 128M CONTAINER big.iso

after sync, a stat on the rados object on the secondary zone shows size=0:

$ bin/rados -c run/c2/ceph.conf stat -p na-2.rgw.buckets.data a393580b-1e6b-487e-93a1-810e35979d1b.4109.1_big.iso                              
na-2.rgw.buckets.data/a393580b-1e6b-487e-93a1-810e35979d1b.4109.1_big.iso mtime 2017-03-13 11:27:30.000000, size 0
@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 20, 2017

cbodley added some commits Feb 21, 2017

rgw: allow system users to read SLO parts
multisite data sync relies on fetching the object as the system user

Fixes: http://tracker.ceph.com/issues/19027

Signed-off-by: Casey Bodley <cbodley@redhat.com>
rgw: RGWGetObj applies skip_manifest flag to SLO
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 23, 2017

@yehudasa can you please review? the initial commit allow system users to read SLO parts is not necessary for the multisite sync case, but i still think a system user should be able to read an SLO object locally

@@ -7001,7 +7001,7 @@ int RGWRados::stat_remote_obj(RGWObjectCtx& obj_ctx,
int ret = conn->get_obj(user_id, info, src_obj, pmod, unmod_ptr,
dest_mtime_weight.zone_short_id, dest_mtime_weight.pg_ver,
true /* prepend_meta */, true /* GET */, true /* rgwx-stat */,
&cb, &in_stream_req);
false /* sync manifest */, &cb, &in_stream_req);

This comment has been minimized.

@yehudasa

yehudasa Mar 23, 2017

Member

@cbodley why is it false here?

This comment has been minimized.

@cbodley

cbodley Mar 23, 2017

Contributor

hmm, i was thinking that stat requests wouldn't make it that far in RGWGetObj::execute(). but it looks like the if (get_type() == RGW_OP_STAT_OBJ) check comes after the manifest handling, which probably isn't what we want

i'll change this flag to true, and propose a change to RGW_OP_STAT_OBJ handling in a separate pr

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 23, 2017

updated, and proposed a change to RGW_OP_STAT_OBJ handling in #14109

@yehudasa

This comment has been minimized.

Member

yehudasa commented Mar 28, 2017

@cbodley lgtm

@cbodley cbodley merged commit 5284ec9 into ceph:master Mar 28, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@cbodley cbodley deleted the cbodley:wip-19027 branch Mar 28, 2017

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