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

Fedora: be more efficient for detecting brick mux setting #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obnoxxx
Copy link

@obnoxxx obnoxxx commented Feb 20, 2019

Previously, we were grep-ing through the output of gluster volume info
for the value of the cluster.brick-multiplex setting. But it is highly
inefficient to use gluster volume list to obtain a global setting.
With many volumes, this can take unnecessarily long. Furthermore, since
this is invoked right after the start of glusterd, glusterd may not be
done initializing.

This patch changes the retrieval of the brick multiplex setting
to use the command gluster volume get all cluster.brick-multiplex
which is exactly made for this purpose and won't get the whole list
of volumes.

Previously, we were grep-ing through the output of gluster volume info
for the value of the cluster.brick-multiplex setting. But it is highly
inefficient to use `gluster volume list` to obtain a global setting.
With many volumes, this can take unnecessarily long. Furthermore, since
this is invoked right after the start of glusterd, glusterd may not be
done initializing.

This patch changes the retrieval of the brick multiplex setting
to use the command `gluster volume get all cluster.brick-multiplex`
which is exactly made for this purpose and won't get the whole list
of volumes.

Signed-off-by: Michael Adam <obnox@redhat.com>
@obnoxxx
Copy link
Author

obnoxxx commented Feb 20, 2019

CAVEAT: needs to be tested

@obnoxxx
Copy link
Author

obnoxxx commented Feb 20, 2019

@atinmu this should roughly be what we discussed on chat today. Please take a look!

Copy link

@atinmu atinmu left a comment

Choose a reason for hiding this comment

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

Approach looks good and the same as what we discussef. I’d request the functional correctness of the script to be validated by other maintainers.

@humblec
Copy link

humblec commented Feb 21, 2019

@obnoxxx @atinmu one other thought here. Do we need to fetch current setting at all ? rather based on the user request go ahead and try it. Thoughts ?

Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

Looks good to me, but the difference between off/disable needs to be explained.

GLUSTER_BRICKMULTIPLEX_SETTING="$( \
gluster --mode=script volume get all cluster.brick-multiplex \
| grep '^cluster.brick-multiplex' \
| awk '{print $2}')"
Copy link
Member

Choose a reason for hiding this comment

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

you can skip the grep if you do this:

awk '/^cluster.brick-multiplex/ {print $2}'

case "$GLUSTER_BRICKMULTIPLEX" in
[nN] | [nN][Oo] )
gluster v info | grep 'cluster.brick-multiplex: off' > $GLUSTERFS_LOG_CONT_DIR/brickmultiplexing
if [[ ${?} == 0 ]]; then
if [[ "${GLUSTER_BRICKMULTIPLEX_SETTING}" == "disable" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Before the check was for "off", now it is "disable"? What does gluster --mode=script volume get all cluster.brick-multiplex really return?

@humblec
Copy link

humblec commented Apr 30, 2019

@obnoxxx ping :)

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

Successfully merging this pull request may close these issues.

4 participants