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

vstart: make -k with optional mon_num (part 2) #8305

Merged
merged 2 commits into from Apr 29, 2016

Conversation

tchaikov
Copy link
Contributor

vstart: use default CEPH_NUM_* if awk fails

see #8251

@tchaikov
Copy link
Contributor Author

@dillaman could you help take a look? thanks!

CEPH_NUM_OSD=`awk -F= '/CEPH_NUM_OSD/{print $2}' $conf_fn`
CEPH_NUM_MDS=`awk -F= '/CEPH_NUM_MDS/{print $2}' $conf_fn`
CEPH_NUM_RGW=`awk -F= '/CEPH_NUM_RGW/{print $2}' $conf_fn`
MON=`awk -F= '/CEPH_NUM_MON/{print $2}' $conf_fn`

Choose a reason for hiding this comment

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

Can you just do something like: CEPH_NUM_MON=awk -F= '/CEPH_NUM_MON/{print $2}' $conf_fn before line 79? If the awk fails, it will fall back to the defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we go this way, we will always continue with whatever CEPH_NUM_{MON,OSD,MDS,RGW} the last vstart run used, even if "-k" is not passed in. in other words,

$ MON=1 OSD=1 MDS=0 ./vstart.sh
$ ./stop
$ MON=1 OSD=2 MDS=0 ./vstart.sh
# what? we have only 1 OSD?

if we move these lines after line 91, we overwrite the options for a non "-k" vstart cluster.

Choose a reason for hiding this comment

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

Fair enough.

@dillaman dillaman self-assigned this Mar 30, 2016
@trociny
Copy link
Contributor

trociny commented Mar 30, 2016

This comment is rather for the initial PR (part 1) and can be freely ignored... Anyway, I don't not like very much when sed/awk/... is used to extract values from files like this (that can be parsed by a specialized parser). E.g. the current awk parsing will break when ceph.conf happens to include CEPH_NUM_MON word more than one time.

Instead, we could add e.g. "client.vstart.sh" section to the generated ceph.conf:

[client.vstart.sh]
  ceph num mon = 5
  ...

And parse it using ceph-conf:

MON=`ceph-conf -c $conf_fn --name client.vstart.sh ceph_num_mon`

@tchaikov
Copy link
Contributor Author

@trociny thanks for the suggestion! i will update this PR later on.

@tchaikov
Copy link
Contributor Author

changelog

  • store the number of {osd,mon,mds,rgw} as settings in the section of "[client.vstart.sh]"
  • read the remembered using ceph-conf instead of parsing the ceph.conf using awk.

hope this is okay to introduce the dependency on ceph-conf. but guess we don't use "-k" in our testing, so it's relatively safe.

@dillaman
Copy link

dillaman commented Apr 5, 2016

My only comment is that perhaps given the distance between usages of "client.vstart.sh", should it use an environment variable "constant"?

@tchaikov
Copy link
Contributor Author

tchaikov commented Apr 5, 2016

changelog

  • use a variable for "client.vstart.sh" instead of hardwiring it to "client.vstart.sh" at two separated places

@tchaikov
Copy link
Contributor Author

@dillaman ping?

@@ -229,6 +231,10 @@ case $1 in
cephx=0
;;
-k )
if [ ! -r $conf_fn ]; then
echo "can not use old configuration: $conf_fn not readable." >&2

Choose a reason for hiding this comment

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

Super minor: s/can not/cannot/g

@tchaikov
Copy link
Contributor Author

changelog

  • s/can not/cannot/

@dillaman thanks for the review, fixed and repushed.

add a section named "client.vstart.sh" for storing the
num_{osd,mon,mds,rgw} settings. and read them using
ceph-conf if the "-k" option is passed in. if we fail to do so, fall
back to the default settings.

Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

@dillaman ping?

@dillaman dillaman merged commit 4f3765c into ceph:master Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants