config: expand tilde for ~/.ceph/$cluster.conf #20774

rishabh-d-dave · 2018-03-07T12:04:53Z

Fixes: http://tracker.ceph.com/issues/23215
Signed-off-by: Rishabh Dave ridave@redhat.com

wjwithagen · 2018-03-08T01:22:46Z

I would very much not do this in application code.
~ expansion is most certainly a shell type thing.
So best just leave it there.

The fact that it is somewhere in the CEPH config is IMHO a problem in itself.
If anything needs to be done like that I suggest making it expand $home, like things as $cluster, that keeps things consistant

And even then the way it is now implemented is that EVERY ~ is replaced, even if it is not at the beginning of the file-path. Which is way much more than what is done by any shell expansion.

rishabh-d-dave · 2018-03-08T09:27:18Z

The fact that it is somewhere in the CEPH config is IMHO a problem in itself.
If anything needs to be done like that I suggest making it expand $home, like things as $cluster, that keeps things consistant

Why do you think it is about ceph config?

It is not about tilde somewhere in ceph config. It is with the fact that ~/.ceph/$cluster.conf is checked and processed for '$' properly but never for '~'. As a result, files like ~/.ceph/ceph.conf are never opened by the rados code even if they are meant to be opened [1]. This [2] where the bug is reproduced.

And even then the way it is now implemented is that EVERY ~ is replaced, even if it is not at the beginning of the file-path. Which is way much more than what is done by any shell expansion.

No. My code specifically looks if tilde is at the zeroth index of the given string s. So, not every tilde would be replaced.

[1] https://github.com/ceph/ceph/blob/master/src/common/config.cc#L45
[2] #20598 (comment)

wjwithagen · 2018-03-08T21:29:50Z

Well my argument is that is should not have been used in this format to begin with, and the PR should be to remove ~/.ceph/$cluster.conf from

./src/common/config.cc:44:static const char *CEPH_CONF_FILE_DEFAULT = "$data_dir/config, /etc/ceph/$cluster.conf, ~/.ceph/$cluster.conf, $cluster.conf"

And if anything like this was required, replace it by $home/.ceph/$cluster.conf.

wjwithagen · 2018-03-08T21:46:43Z

src/common/config.cc

@@ -248,6 +248,9 @@ int md_config_t::parse_config_files(const char *conf_files,
      } else {
 	cfl.erase(p++);  // ignore this item
      }
+    } else if (s.find("~") == 0) {
+	s.replace(s.find("~"), 1, string(getenv("HOME")));


Could replace s.replace(s.find("~"), 1, string(getenv("HOME")));
by s.replace(0, 1, string(getenv("HOME")));

rishabh-d-dave · 2018-03-09T08:11:09Z

src/common/config.cc

@@ -248,6 +248,9 @@ int md_config_t::parse_config_files(const char *conf_files,
      } else {
 	cfl.erase(p++);  // ignore this item
      }
+    } else if (s.find("$home") != string::npos) {
+	s.replace(0, 5, string(getenv("HOME")));


Would it be more appropriate to move this part to somewhere in expand_meta()?

@rishabh-d-dave
I think that would make sense, it would then be more universially applicable.

liewegas

Works for me (I have no opinion on $home vs ~)

liewegas · 2018-03-14T02:13:40Z

retest this please

rishabh-d-dave · 2018-03-14T06:48:40Z

@wjwithagen, @liewegas I don't why make check failed. Locally, it's passing for me -
ceph-make-check-for-PR-20774.txt

wjwithagen · 2018-03-14T09:25:55Z

@rishabh-d-dave
Looks like ceph-conf fails.
Could be transient, but might have to do with ~ change, which it this case the $home translation does not work out the same as in your work environment.
And that could be that because ~ was non functional before your change.
But now it is, and the $home/ceph.conf does not actually contain what you need in the jenkins test env.

Fixing bug like this, where Jenkins does something different are not easy.
Might want to set to DNM, and submit some debug code, to later remove it once you fixed it.

tchaikov

            raise Error('error executing ceph-conf', e, err)
        if ret == 1:
            # config entry not found
            return None
        elif ret != 0:
>           raise Error('getting variable from configuration failed')
E           Error: Error: getting variable from configuration failed

in run-tox-ceph-disk

tchaikov · 2018-03-15T06:35:54Z

retest this please.

tchaikov · 2018-03-15T06:36:16Z

http://pulpito.ceph.com/kchai-2018-03-14_04:42:43-rados-wip-kefu-testing-2018-03-14-1106-distro-basic-mira/

SLOW_OPS failures are introduced by mon,mgr: make osd_metric more popular and report slow ops to mgr #20660
the coredump is addressed by mgr/MgrClient: guard send_pgstats() with lock #20909

rishabh-d-dave · 2018-03-15T10:29:50Z

Before ceph-conf runs this time, the chosen ceph conf file would be displayed and it would be checked if it exists. WRT to how Jenkins runs the test, I'm not sure if the debugging code would really help.

@wjwithagen

Might want to set to DNM, and submit some debug code, to later remove it once you fixed it.

What's DNM?

tchaikov · 2018-03-15T11:19:33Z

@rishabh-d-dave "DNM" stands for "do not merge". @wjwithagen suggested to add this label to your PR so we don't merge it by accident.

rishabh-d-dave · 2018-03-21T11:26:16Z

retest this please

rishabh-d-dave · 2018-03-22T10:00:28Z

@wjwithagen @tchaikov From the output of the debug code, ~/.ceph/ceph.conf was never chosen from CEPH_CONF_FILE_DEFAULT. IMO, this must mean that the errors are not due to the changes.

liewegas

Please drop the debug commit and rebase. When I test the first commit only locally it looks ok

liewegas · 2018-03-23T01:03:50Z

src/common/config.cc

@@ -1156,6 +1169,8 @@ Option::value_t md_config_t::_expand_meta(
 	out += stringify(getpid());
      } else if (var == "cctid") {
 	out += stringify((unsigned long long)this);
+      } else if (var == "home") {
+	out = string(getenv("HOME"));


Small issue: getenv() can return NULL, and std:;string crashes when passed null. Change to

const char *home = getenv("HOME"); out = home ? std::string(home) : std::string();

or similar

And expand "$home" to get the actual path to ceph.conf. Fixes: http://tracker.ceph.com/issues/23215 Signed-off-by: Rishabh Dave <ridave@redhat.com>

tchaikov · 2018-03-27T14:44:16Z

batrick added common needs-review labels Mar 8, 2018

wjwithagen reviewed Mar 8, 2018

View reviewed changes

rishabh-d-dave force-pushed the fix-tilde-expansion branch from b370fdd to 26424b7 Compare March 9, 2018 08:09

rishabh-d-dave commented Mar 9, 2018

View reviewed changes

rishabh-d-dave force-pushed the fix-tilde-expansion branch 2 times, most recently from 9a1f7e1 to 697959c Compare March 13, 2018 08:14

liewegas approved these changes Mar 14, 2018

View reviewed changes

liewegas added needs-qa and removed needs-review labels Mar 14, 2018

tchaikov added the wip-kefu-testing label Mar 14, 2018

tchaikov requested changes Mar 15, 2018

View reviewed changes

tchaikov removed needs-qa wip-kefu-testing labels Mar 15, 2018

rishabh-d-dave force-pushed the fix-tilde-expansion branch from 697959c to 4006afe Compare March 15, 2018 10:25

ajarr added the DNM label Mar 15, 2018

liewegas requested changes Mar 23, 2018

View reviewed changes

config: replace '~' by '$home' in CEPH_CONF_FILE_DEFAULT

3b46884

And expand "$home" to get the actual path to ceph.conf. Fixes: http://tracker.ceph.com/issues/23215 Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave force-pushed the fix-tilde-expansion branch from 4006afe to 3b46884 Compare March 23, 2018 05:08

liewegas approved these changes Mar 24, 2018

View reviewed changes

liewegas added needs-qa and removed DNM labels Mar 24, 2018

tchaikov approved these changes Mar 26, 2018

View reviewed changes

tchaikov added the wip-kefu-testing label Mar 26, 2018

tchaikov merged commit fc86c2b into ceph:master Mar 27, 2018

rishabh-d-dave deleted the fix-tilde-expansion branch February 19, 2019 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: expand tilde for ~/.ceph/$cluster.conf #20774

config: expand tilde for ~/.ceph/$cluster.conf #20774

rishabh-d-dave commented Mar 7, 2018

wjwithagen commented Mar 8, 2018

rishabh-d-dave commented Mar 8, 2018

wjwithagen commented Mar 8, 2018

wjwithagen Mar 8, 2018

rishabh-d-dave Mar 9, 2018

wjwithagen Mar 10, 2018

liewegas left a comment

liewegas commented Mar 14, 2018

rishabh-d-dave commented Mar 14, 2018 •

edited

wjwithagen commented Mar 14, 2018

tchaikov left a comment

tchaikov commented Mar 15, 2018

tchaikov commented Mar 15, 2018

rishabh-d-dave commented Mar 15, 2018

tchaikov commented Mar 15, 2018

rishabh-d-dave commented Mar 21, 2018

rishabh-d-dave commented Mar 22, 2018 •

edited

liewegas left a comment

liewegas Mar 23, 2018

rishabh-d-dave Mar 23, 2018

tchaikov commented Mar 27, 2018

config: expand tilde for ~/.ceph/$cluster.conf #20774

config: expand tilde for ~/.ceph/$cluster.conf #20774

Conversation

rishabh-d-dave commented Mar 7, 2018

wjwithagen commented Mar 8, 2018

rishabh-d-dave commented Mar 8, 2018

wjwithagen commented Mar 8, 2018

wjwithagen Mar 8, 2018

Choose a reason for hiding this comment

rishabh-d-dave Mar 9, 2018

Choose a reason for hiding this comment

wjwithagen Mar 10, 2018

Choose a reason for hiding this comment

liewegas left a comment

Choose a reason for hiding this comment

liewegas commented Mar 14, 2018

rishabh-d-dave commented Mar 14, 2018 • edited

wjwithagen commented Mar 14, 2018

tchaikov left a comment

Choose a reason for hiding this comment

tchaikov commented Mar 15, 2018

tchaikov commented Mar 15, 2018

rishabh-d-dave commented Mar 15, 2018

tchaikov commented Mar 15, 2018

rishabh-d-dave commented Mar 21, 2018

rishabh-d-dave commented Mar 22, 2018 • edited

liewegas left a comment

Choose a reason for hiding this comment

liewegas Mar 23, 2018

Choose a reason for hiding this comment

rishabh-d-dave Mar 23, 2018

Choose a reason for hiding this comment

tchaikov commented Mar 27, 2018

rishabh-d-dave commented Mar 14, 2018 •

edited

rishabh-d-dave commented Mar 22, 2018 •

edited