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 structures rework #11485

Merged
merged 29 commits into from Mar 9, 2017
Merged

rgw structures rework #11485

merged 29 commits into from Mar 9, 2017

Conversation

yehudasa
Copy link
Member

@yehudasa yehudasa commented Oct 13, 2016

Create new types:

  • rgw_pool (to represent pools instead of rgw_bucket)
  • rgw_raw_obj (to represent the raw rados object instead of rgw_obj)
  • rgw_obj_index_key

Don't store placement info in rgw_bucket (if possible, still keeping explicit placement for backward compatibility when dealing with older data). head object will always reside in the bucket's default placement target pool, (from the zone config), tail depends on what's in the manifest.

Rework rgw_obj; rename some of the fields, don't refer to oid as object anymore and don't auto generate the oid. Don't encode the oid, but the name, ns, and instance. Split rgw_obj_key and rgw_obj_index_key, and rgw_obj handles both as needed.

Pools can now be defined with a namespace ([:ns]), and consolidate a few of the default pools.

@yehudasa yehudasa force-pushed the wip-bucket-cleanup branch 2 times, most recently from d459032 to 71cf9c2 Compare October 19, 2016 15:34
@yehudasa yehudasa force-pushed the wip-bucket-cleanup branch 4 times, most recently from 0001d84 to 66703bc Compare November 21, 2016 22:24
@yehudasa yehudasa force-pushed the wip-bucket-cleanup branch 2 times, most recently from 6ff550a to b4e1249 Compare December 16, 2016 23:43
@yehudasa yehudasa changed the title [DNM] rgw code cleanup [DNM] rgw structures rework Dec 16, 2016
@cbodley
Copy link
Contributor

cbodley commented Dec 19, 2016

@yehudasa did you forget to commit test_rgw_common.cc? getting this error from cmake:

CMake Error at src/test/rgw/CMakeLists.txt:25 (add_library):
  Cannot find source file:

    test_rgw_common.cc

@yehudasa
Copy link
Member Author

@cbodley fixed now

Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

looking really good!

the new structure names make it much clearer what each piece is for and how it's used

the namespace changes on top are really elegant, using escape characters for the parsing, and having a single rgw_init_ioctx() to handle the pool creation and the call to set_namespace() 👍

your commit messages made it a lot easier to review (thanks!), and the unit tests are a big help for validation


map<string, bufferlist> attrset;
RGWRawObjState() {}
RGWRawObjState(const RGWRawObjState& rhs) : obj (rhs.obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't look like the copy ctor is doing anything special - can we just let the compiler generate it?

ENCODE_FINISH(bl);
}

void decode_from_bucket(bufferlist::iterator& bl);
Copy link
Contributor

Choose a reason for hiding this comment

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

not used or implemented?

@@ -121,7 +121,8 @@ class RGWReadDataSyncStatusMarkersCR : public RGWShardCollectCR {

RGWDataSyncEnv *env;
const int num_shards;
int shard_id{0};
int shard_id{0};;

Copy link
Contributor

Choose a reason for hiding this comment

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

extra ;

}
map<string, bufferlist> attrs = s->bucket_attrs;
attrs[RGW_ATTR_CORS] = cors_bl;
op_ret = rgw_bucket_set_attrs(store, s->bucket_info, attrs, &s->bucket_info.objv_tracker);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the is_object_op case not needed here (and RGWDeleteCORS)?

const rgw_obj& src_obj,
int versioning_status,
uint16_t bilog_flags = 0,
const ceph::real_time& expiration_time = ceph::real_time());

/** Delete a raw object.*/
int delete_raw_obj(const rgw_raw_obj& obj);

Copy link
Contributor

Choose a reason for hiding this comment

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

is there supposed to be a semantic difference between this and delete_system_obj()? they are basically equivant, since objv_tracker defaults to null

Copy link
Member Author

Choose a reason for hiding this comment

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

trying to separate the notion of a raw obj vs a system obj. Maybe this can be reworked later to avoid duplicate code though.

RGWObjectCtx obj_ctx(store);

RGWBucketInfo bucket_info;
int ret = store->get_bucket_instance_info(obj_ctx, bucket, bucket_info, NULL, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the callers (the RGWRados::bucket_index_*() functions, at least) already have a RGWBucketInfo - can BucketShard::init() take that instead of rgw_bucket to avoid this extra io?

/*
* Ceph - scalable distributed file system
*
* Copyright (C) 2013 eNovance SAS <licensing@enovance.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

bad copy-paste?

ns->clear();
return;
}
if (key[1] == '_') {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there anything before key[0] and key[1] that checks the string length?

* part of the given namespace, it returns false.
*/
static bool oid_to_key_in_ns(const string& oid, rgw_obj_key *key, const string& ns) {
string obj_ns;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused variable

@@ -1424,6 +1425,83 @@ bool RGWUserCaps::is_valid_cap_type(const string& tp)
return false;
}

static ssize_t unescape_str(const string& s, ssize_t ofs, char esc_char, char special_char, string *dest)
Copy link
Contributor

Choose a reason for hiding this comment

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

string::npos is a size_t, so that's probably a better fit for the return type

@yehudasa
Copy link
Member Author

@cbodley PR that adds ragweed support here: #13644

@yehudasa
Copy link
Member Author

@yehudasa yehudasa changed the title [DNM] rgw structures rework rgw structures rework Mar 3, 2017
@yehudasa yehudasa changed the title rgw structures rework [DNM] rgw structures rework Mar 3, 2017
@yehudasa yehudasa changed the title [DNM] rgw structures rework rgw structures rework Mar 8, 2017
@yehudasa yehudasa force-pushed the wip-bucket-cleanup branch 2 times, most recently from 1047662 to a9ec5e8 Compare March 9, 2017 17:15
Pools are represented by rgw_pool (and not rgw_bucket anymore),
and we use rgw_raw_obj to reference rados objs and all 'system'
objects (vs rgw_obj that is used for rgw objects).

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
This drags in multiple related changes that are needed in order to
support that.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Two main changes here:
1. Newly created rgw_bucket does not have a predetermined placement pools
assigned to it. The placement_id param in the objects themselves points
at where the data is located. This affects object's tail location, head
is located where the bucket instance's placement rule points at.
2. Modify object manifest to use rgw_raw_obj instead of rgw_obj.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Test rgw_raw_obj and upgrade of old rgw_obj, rgw_bucket and
old manifest.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
got broken through the rgw_bucket cleanup related work

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Instead of storing the oid and the name, just store the name
and calculate it when needed (same goes to locator). Also give more
coherent names to the various fields.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Use rgw_obj_index_key to represent entries in bucket index (typedef of
cls_rgw_obj_key). Get rid of RGWObjEnt, it was duplicate of rgw_bucket_dir_entry
anyway.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Bucket's placement rule is in the bucket instance's info. Object's
placement rule is in the manifest

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
add a namespace field to the rgw_pool struct

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Use rgw_pool all around, and replace librados::create_ioctx() with
helper that also sets the namespace.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Fix crash due to code cleanup. Changes scope of obj ref.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Need to parse the bucket id off the entry and then set it on the
bucket struct.

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
Fixes: http://tracker.ceph.com/issues/19249

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>
@yehudasa yehudasa merged commit 3d29012 into ceph:master Mar 9, 2017
@smithfarm
Copy link
Contributor

smithfarm commented Mar 22, 2017

@yehudasa I understand you'd like to backport a9ec5e8 from this PR, but to which stable release(s)?

(Backport field of http://tracker.ceph.com/issues/19249 is currently empty)

manifest->set_tail_bucket(_b);
manifest->set_head(_h, 0);
manifest->set_tail_placement(placement_rule, _b);
manifest->set_head(placement_rule, _obj, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

so the tail stripes and head object will still stored as a same placement_rule?

@fangyuxiangGL
Copy link
Contributor

@yehudasa

"head object will always reside in the bucket's default placement target pool, (from the zone config), tail depends on what's in the manifest."

head object and tail stripes still have same rados store way as your code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants