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

common/ceph_context: 'config diff get' option added #10736

Merged
merged 5 commits into from May 18, 2017

Conversation

Projects
None yet
5 participants
@oliveiradan
Contributor

oliveiradan commented Aug 15, 2016

This is a feature request from Sebastien (on our ceph-devel list) so we can get the 'diff (current and default) of an config option': # config diff get mon_data_avail_warn
(1):

./ceph --admin-daemon out/osd.1.asok config diff get mon_data_avail_crit 
{
    "diff": {
        "current": {
            "mon_data_avail_crit": "1"
        },
        "defaults": {
            "mon_data_avail_crit": "5"
        }
    },
    "unknown": []
}

(2):

./ceph --admin-daemon out/osd.1.asok config diff get mon_data_avail_warn
{
    "diff": {
        "current": {
            "mon_data_avail_warn": "10"
        },
        "defaults": {
            "mon_data_avail_warn": "30"
        }
    },
    "unknown": []
}

(3):

 ./ceph --admin-daemon out/osd.1.asok config diff get num_client
{
    "diff": {
        "current": {
            "num_client": "1"
        },
        "defaults": {
            "num_client": "1"
        }
    },
    "unknown": []
}
f->close_section(); //-- unknown
} //-- else if (!cmd_getval(this, cmdmap, "var", ceph_setting))
} //-- else if (command == "config diff get")
else if (command == "log flush") {

This comment has been minimized.

@badone

badone Aug 17, 2016

Contributor

Can we align this better?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

We already had some misaligned block, so I ended up fixing some up the function in order to see this one properly.

f->open_object_section("diff");
f->open_object_section("current");
for (map<string, pair<string, string> >::iterator p = diff.begin(); p != diff.end(); ++p) {

This comment has been minimized.

@badone

badone Aug 17, 2016

Contributor

Could we use C++11 style auto and range-based for loops instead of the older style?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Done.

char local_buf[4096];
char other_buf[4096];
for (int i = 0; i < NUM_CONFIG_OPTIONS; i++) {
if (wasItFound) break; //-- If we already found it, we are done.

This comment has been minimized.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Done, thanks.

char *local_val = local_buf;
err = _get_val(opt->name, &local_val, sizeof(local_buf));
if (err != 0) continue;

This comment has been minimized.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Done, thanks.

char *local_val = local_buf;
err = _get_val(opt->name, &local_val, sizeof(local_buf));
if (err != 0) continue;
if (strcmp(local_val, other_val)) diff->insert(make_pair(opt->name, make_pair(local_val, other_val)));

This comment has been minimized.

This comment has been minimized.

@tchaikov

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Done, thanks.

if (err != 0) continue;
if (strcmp(local_val, other_val)) diff->insert(make_pair(opt->name, make_pair(local_val, other_val)));
else {
if (show_unchanged) diff->insert(make_pair(opt->name, make_pair(local_val, other_val)));

This comment has been minimized.

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

is it possible that ceph_setting can match multiple times?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

I have not seen any cases where config_optionsp[] would have duplicated values. If so, we would need to think about changing the array in question to a hash (as I suggested during the work done for http://tracker.ceph.com/issues/14934).

std::string ceph_setting;
if (!cmd_getval(this, cmdmap, "var", ceph_setting)) {
f->dump_string("error", "syntax error: 'config diff get <var>'");
} //-- if (!cmd_getval(this, cmdmap, "var", ceph_setting))

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

nit, could remove the comment, the if block is small enough.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

I understand that, but if we have nested blocks, it would look better/more organized. If not really allowed, I will take them off.

This comment has been minimized.

@tchaikov

tchaikov Aug 19, 2016

Contributor

our code guidelines do not forbid this explicitly. and i agree this is a gray area, but see https://google.github.io/styleguide/cppguide.html#Implementation_Comments. the scope of the if block is obvious, imo. and the comments are distracting.

@@ -156,8 +156,13 @@ struct md_config_t {
void show_config(Formatter *f);
/// obtain a diff between our config values and another md_config_t values
void diff(const md_config_t *other,
map<string,pair<string,string> > *diff, set<string> *unknown);
void diff(const md_config_t *other, map<string,pair<string,string> > *diff,

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

could you leave this unchanged if you are not touching it in this commit?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Done.

if (wasItFound) break; //-- If we already found it, we are done.
config_option *opt = &config_optionsp[i];
std::string ceph_setting_name(opt->name);
if (ceph_setting == ceph_setting_name) {

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

nit, could just use opt->name for comparison.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Thanks.

{
Mutex::Locker l(lock);
bool wasItFound = false;

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

our naming convention does not encourage wasItFound, might want to use hello_world instead.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Done, thanks.

void diff(const md_config_t *other, map<string,pair<string,string> > *diff,
set<string> *unknown);
//-- obtain a diff between our config values and another md_config_t values

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

could you use /// to start the comment for a function or method. /// is used by doxygen to start a short description for a function.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Done. Thanks for the heads up.

else {
if (show_unchanged) diff->insert(make_pair(opt->name, make_pair(local_val, other_val)));
} //-- if (strcmp(local_val, other_val)) diff->insert(make_pair(opt->name, make_pair(local_val, other_val)))
wasItFound = true;

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

why not just break here?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Yep, I agreed. Thanks!

@@ -1227,6 +1227,47 @@ void md_config_t::diff(
}
}
void md_config_t::diff_setting(const md_config_t *other,

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

maybe be we can consolidate diff() and diff_setting()?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Humm, we only decided to have a different function diff_setting() because we would need to change the diff() signature and behavior if a setting was passed or not. But it is a possibility.

This comment has been minimized.

@tchaikov

tchaikov Aug 19, 2016

Contributor

yes, as you are almost duplicating the diff() method, so it might be worthy a try.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 22, 2016

Contributor

Well, md_config_t::diff_setting(const md_config_t *other, map<string, pair<string, string>> *diff, const string& ceph_setting, bool show_unchanged) and void md_config_t::diff(const md_config_t *other, map<string,pair<string,string> > *diff, set<string> *unknown) quite different signatures, so I wonder whether or not the trade offs on consolidating them would make maintenance troublesome.

This comment has been minimized.

@tchaikov

tchaikov Sep 13, 2016

Contributor

they can be very different, if diff is not a map. and in your case, diff could have been a pair<string, string>, because the queried setting can only be matched once at most.

@@ -1227,6 +1227,47 @@ void md_config_t::diff(
}
}
void md_config_t::diff_setting(const md_config_t *other,
std::map<std::string, std::pair<std::string, std::string>> *diff,
std::set<std::string> *unknown, const std::string& ceph_setting, bool show_unchanged)

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

nit, the std:: is not necessary, as we are using std::string and using std::map at the top of this source file.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

Yes, but probably also not wrong. However, I went ahead and remove it to expedite the process.

@badone

This comment has been minimized.

Contributor

badone commented Aug 18, 2016

Could we get a test for this also please @oliveiradan ?

int err = other->get_val(opt->name, &other_val, sizeof(other_buf));
if (err < 0) {
if (err == -ENOENT) {
unknown->insert(opt->name);

This comment has been minimized.

@tchaikov

tchaikov Aug 18, 2016

Contributor

if only one item can be matched, why bother returning unknown?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

As md_config_t::diff() does it, we just wanted to keep it the same way/behavior. However it does make sense and I can remove it.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 19, 2016

Contributor

@badone, but 'get a test' do you mean "Add a test for it in the test_config.cc"?

Thanks.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Aug 18, 2016

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#preparing-and-sending-patches, in this case, the prefix could be "common/ceph_context: ".

and would be ideal if you could put more details about the feature in your commit message?

@tchaikov tchaikov added needs-test and removed needs-review labels Aug 18, 2016

@badone badone added common and removed tools labels Aug 18, 2016

@badone

This comment has been minimized.

Contributor

badone commented Aug 19, 2016

@oliveiradan yes, add a test or tests for the new functionality where appropriate.

f->open_object_section("diff");
f->open_object_section("current");
//for (map<string, pair<string, string> >::iterator p = diff.begin(); p != diff.end(); ++p) {

This comment has been minimized.

@tchaikov

tchaikov Aug 19, 2016

Contributor

please remove the commented out code.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 22, 2016

Contributor

Done.

@@ -156,9 +156,13 @@ struct md_config_t {
void show_config(Formatter *f);
/// obtain a diff between our config values and another md_config_t values
void diff(const md_config_t *other,
void diff(const md_config_t *other,

This comment has been minimized.

@tchaikov

tchaikov Aug 19, 2016

Contributor

please remove the trailing space.

This comment has been minimized.

@oliveiradan

oliveiradan Aug 22, 2016

Contributor

Done.

@@ -326,6 +326,7 @@ void CephContext::do_command(std::string command, cmdmap_t& cmdmap,
}
lgeneric_dout(this, 1) << "do_command '" << command << "' '"
<< ss.str() << dendl;

This comment has been minimized.

@tchaikov

tchaikov Aug 19, 2016

Contributor

again, could you remove the formatting changes if you are not touch them?

This comment has been minimized.

@oliveiradan

oliveiradan Aug 22, 2016

Contributor

Done.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Aug 19, 2016

and would be ideal if you could put more details about the feature in your commit message?

it is not addressed.

@oliveiradan oliveiradan changed the title from Sebastien's [Feature request] config diff on a single option with the… to common/ceph_context: 'config diff get' option added Aug 23, 2016

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Sep 14, 2016

What am I missing?

"what we have at the top of the PR" is in the github comment, not in the commit message.

f->dump_stream("error") << "error getting '" << var << "': " << cpp_strerror(r);
} else {
f->dump_string(var.c_str(), buf);
}

This comment has been minimized.

@tchaikov

tchaikov Sep 28, 2016

Contributor

could you revert this formatting change?

This comment has been minimized.

@tchaikov
char other_buf[4096];
for (int i = 0; i < NUM_CONFIG_OPTIONS; i++) {
config_option *opt = &config_optionsp[i];
std::string ceph_setting_name(opt->name);

This comment has been minimized.

@tchaikov

tchaikov Sep 28, 2016

Contributor

this variable is never used.

@@ -1227,6 +1227,46 @@ void md_config_t::diff(
}
}
void md_config_t::diff_setting(const md_config_t *other,
map<string, pair<string, string>> *diff,
const string& ceph_setting, bool show_unchanged)

This comment has been minimized.

@tchaikov

tchaikov Sep 28, 2016

Contributor

we can add show_unchanged later once we want to pass false.

@@ -1227,6 +1227,46 @@ void md_config_t::diff(
}
}
void md_config_t::diff_setting(const md_config_t *other,
map<string, pair<string, string>> *diff,

This comment has been minimized.

@tchaikov

tchaikov Sep 28, 2016

Contributor

as we discussed, in your case, at most only one item will be returned. so either we can consolidate these two diff() methods, or we don't need to use a map here.

@@ -431,7 +431,8 @@ test -d gmon && $SUDO rm -rf gmon/*
# figure machine's ip
HOSTNAME=`hostname -s`
#HOSTNAME=`hostname -s`
HOSTNAME=`hostname | awk -F "." '{print $1}'`

This comment has been minimized.

@tchaikov

tchaikov Nov 9, 2016

Contributor

why hostname -s does not work for you?

This comment has been minimized.

@oliveiradan

oliveiradan Nov 10, 2016

Contributor

Sorry, this shouldn't be there... it was from my lab at home. I will take it out.

{
Mutex::Locker l(lock);
bool ceph_param_search = (!ceph_setting.empty()) ? true : false;

This comment has been minimized.

@tchaikov

tchaikov Nov 9, 2016

Contributor
bool ceph_param_search = !ceph_setting.empty();

would suffice. and IMHO, no need to introduce ceph_param_search just for checking if ceph_setting is empty or has the default value.

This comment has been minimized.

@oliveiradan

oliveiradan Nov 10, 2016

Contributor

Absolutely correct. Thanks... I think i was spending too much time with Python these days. ;-)

memset(local_buf, 0, sizeof(local_buf));
memset(other_buf, 0, sizeof(other_buf));
char *other_val = other_buf;
int err = other->get_val(opt.name, &other_val, sizeof(other_buf));
if (err < 0) {
if (ceph_param_search) {

This comment has been minimized.

@tchaikov

tchaikov Nov 9, 2016

Contributor

why would you want to continue here? as ceph_setting == opt.name if ceph_param_search.

This comment has been minimized.

@oliveiradan

oliveiradan Nov 10, 2016

Contributor

If we didn't find the param, why spend time going through the rest of the code, as opposed to spend time looking at the next one?

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

if we didn't find the param, maybe we should instead break?

@@ -1264,6 +1288,12 @@ void md_config_t::diff(
if (strcmp(local_val, other_val))
diff->insert(make_pair(opt.name, make_pair(local_val, other_val)));
else {

This comment has been minimized.

@tchaikov

tchaikov Nov 9, 2016

Contributor

could just put

else if {
  ...
}

less indent.

@@ -1236,21 +1236,45 @@ bool md_config_t::expand_meta(std::string &origval,
}
void md_config_t::diff(
const md_config_t *other,
map<string, pair<string, string> > *diff,
set<string> *unknown)

This comment has been minimized.

@tchaikov

tchaikov Nov 9, 2016

Contributor

why do we need this method as we have diff_helper() whose last param is optional?

This comment has been minimized.

@oliveiradan

oliveiradan Nov 10, 2016

Contributor

@tchaikov , we the same function, with 2 different signatures, where once accepts the 'setting' we are searching for. Would you rather have only 1 function (as it was before) and the optional param for the 'setting' search, which would force us to call it passing a param with ""?

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

which would force us to call it passing a param with ""

you added an optional param of ceph_setting, so i guess we are already ready?

f->dump_stream("error") << "error getting '" << var << "': " << cpp_strerror(r);
} else {
f->dump_string(var.c_str(), buf);
}

This comment has been minimized.

@tchaikov

@tchaikov tchaikov changed the title from common/ceph_context: 'config diff get' option added to 9common/ceph_context: 'config diff get' option added Nov 11, 2016

@liewegas liewegas changed the title from 9common/ceph_context: 'config diff get' option added to common/ceph_context: 'config diff get' option added Mar 10, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 10, 2017

ping

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 20, 2017

@badone

This comment has been minimized.

Contributor

badone commented May 4, 2017

@oliveiradan ping, would you like some assistance here to get this completed?

oliveiradan added some commits Aug 19, 2016

common: *config diff get* option added
Signed-off-by: Daniel Oliveira <doliveira@suse.com>
common/ceph_context: config diff get option refactored
Signed-off-by: Daniel Oliveira <doliveira@suse.com>
@oliveiradan

This comment has been minimized.

Contributor

oliveiradan commented May 4, 2017

I've talked to @badone and already rebased it. It is in the process of rebuilding it so I can run the tests again tomorrow morning.

void md_config_t::diff(
const md_config_t *other,
map<string, pair<string, string> > *diff,
set<string> *unknown, const string& ceph_setting = string(""))

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

s/ceph_setting/setting/ as in this context, it's always "ceph" setting.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 4, 2017

@oliveiradan very sorry for the latency. i just lost track of this PR. =(

oliveiradan added some commits May 9, 2017

Last changes for PR #10736
Signed-off-by: Daniel Oliveira <doliveira@suse.com>
@@ -496,7 +496,39 @@ void CephContext::do_command(std::string command, cmdmap_t& cmdmap,
f->dump_string("option", *p);
}
f->close_section(); // unknown
} else if (command == "log flush") {
}
else if (command == "config diff get") {

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

please move the else { to the previous line, so it looks like

} else if {
...
if (!cmd_getval(this, cmdmap, "var", ceph_setting)) {
f->dump_string("error", "syntax error: 'config diff get <var>'");
}
else {

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

ditto

f->close_section(); //-- diff
} //-- else if (!cmd_getval(this, cmdmap, "var", ceph_setting))
} //-- else if (command == "config diff get")
else if (command == "log flush") {

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

ditto.

dismissed

memset(local_buf, 0, sizeof(local_buf));
memset(other_buf, 0, sizeof(other_buf));
char *other_val = other_buf;
int err = other->get_val(opt.name, &other_val, sizeof(other_buf));
if (err < 0) {
if (!setting.empty()) {

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

in this case, setting == opt.name, and we should not error out with an error indicating that config_options has setting but other does not. continue does not help here. it will end up with ENOENT.

This comment has been minimized.

@oliveiradan

oliveiradan May 15, 2017

Contributor

Ok, thanks for the heads up. The checking (unneeded) in question was removed.

oliveiradan added a commit to oliveiradan/ceph that referenced this pull request May 15, 2017

Latest Changes based on comments:
#10736 (comment)
#10736 (comment)
#10736 (comment)

Signed-off-by: Daniel Oliveira <doliveira@suse.com>
@badone

badone approved these changes May 16, 2017

LGTM if it builds and passes the tests.

@badone badone added the needs-qa label May 16, 2017

@yuriw yuriw merged commit a56871e into ceph:master May 18, 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

@oliveiradan oliveiradan deleted the oliveiradan:SebastienHan_config_diff_get_fr branch Feb 2, 2018

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