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

Add man page for diskless SBD option (bsc#1138353) #25

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

Conversation

liangxin1300
Copy link

Thanks!

Copy link

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hi @liangxin1300 . One question:
What will happen if the -S and -s /dev/my-sbd-device are used together?
As far as I know SBD will use the device defined in -s, isn't it?
Maybe we should consider adding something in the description to explain this

@liangxin1300
Copy link
Author

Hi @arbulu89
According to code logic in bootstrap.py, "-S" mean enable diskless sbd which priority is lower than "-s",
So using them together will equal to use "-s /dev/sbd-device" only, no bad side affect.
I prefer don't explain this in man page since there are too many option combine possibilities.
What do you think?

@arbulu89
Copy link

Hi @arbulu89
According to code logic in bootstrap.py, "-S" mean enable diskless sbd which priority is lower than "-s",
So using them together will equal to use "-s /dev/sbd-device" only, no bad side affect.
I prefer don't explain this in man page since there are too many option combine possibilities.
What do you think?

Ok, I don't see much problem on that since the priorities are clear. Should be document this somewhere else? We shoulnd't be checking the code every time we need to know the parameters usage hehe

@zzhou1
Copy link

zzhou1 commented Aug 8, 2019

I incline to reject this PR, since I disagree the manual effort to duplicate information from the root source of ha-cluster-init --help.

I propose either delete this man page, or auto-generated based on ha-cluster-init --help.

@ab-mohamed
Copy link

Hi @arbulu89
According to code logic in bootstrap.py, "-S" mean enable diskless sbd which priority is lower than "-s",
So using them together will equal to use "-s /dev/sbd-device" only, no bad side affect.
I prefer don't explain this in man page since there are too many option combine possibilities.
What do you think?

This is good information I would suggest to add as a side note inside the man page. It will preserve a lot of incoming enquiries to the solutions architects and support teams.

Hi @arbulu89
According to code logic in bootstrap.py, "-S" mean enable diskless sbd which priority is lower than "-s",
So using them together will equal to use "-s /dev/sbd-device" only, no bad side affect.
I prefer don't explain this in man page since there are too many option combine possibilities.
What do you think?

Ok, I don't see much problem on that since the priorities are clear. Should be document this somewhere else? We shoulnd't be checking the code every time we need to know the parameters usage hehe

+1!
I would suggest having this information as a "NOTE" inside the man page showing the difference between using '-S' and '-s' options and what happened if the user uses both of them.

Best regards,
Ab

@ab-mohamed
Copy link

I incline to reject this PR, since I disagree the manual effort to duplicate information from the root source of ha-cluster-init --help.

I propose either delete this man page, or auto-generated based on ha-cluster-init --help.

Hi Zzhou1,
man page is important for the end-users. It is the first source to consult before forking questions.
I would strongly recommend keeping the man page.

Best regards,
Ab

@liangxin1300
Copy link
Author

Hi @ab-mohamed , how does it look like this time?
I added this line Using -s device and -S together will equal to use -s device only.

@ab-mohamed
Copy link

Hi @ab-mohamed , how does it look like this time?
I added this line Using -s device and -S together will equal to use -s device only.

I would suggest the following form under '-S' section:

NOTE: Using -s device and -S together will equal to use -s device only.

Best regards,
Ab

@zzhou1
Copy link

zzhou1 commented Aug 13, 2019

Hi Zzhou1,
man page is important for the end-users. It is the first source to consult before forking questions.
I would strongly recommend keeping the man page.

Understand, and fully agree.

Well, the real situation is the man page is far more out-dated. The most up-to-date and detailed description actually is --help.

The current PR is a duplicate effort and also error prone. That sounds waste time.

The short term I feel make sense to delete the man page, instead of shipping a out-dated and misleading one. The error of no man page will naturally lead users to use "--help" on the other hand.

In long run, better to auto generate the man page, eg. resource-agents is a good example.

BR,
Roger

@krig
Copy link
Collaborator

krig commented Aug 13, 2019

Actually the man pages arealready generated from the help output, I guess maybe it does not cover the old aliases though. See the spec file:
https://build.opensuse.org/package/view_file/openSUSE:Backports:SLE-15-SP1/ha-cluster-bootstrap/ha-cluster-bootstrap.spec?expand=1

@ab-mohamed
Copy link

Hi Zzhou1,
man page is important for the end-users. It is the first source to consult before forking questions.
I would strongly recommend keeping the man page.

Understand, and fully agree.

Well, the real situation is the man page is far more out-dated. The most up-to-date and detailed description actually is --help.

The current PR is a duplicate effort and also error prone. That sounds waste time.

The short term I feel make sense to delete the man page, instead of shipping a out-dated and misleading one. The error of no man page will naturally lead users to use "--help" on the other hand.

In long run, better to auto generate the man page, eg. resource-agents is a good example.

BR,
Roger

Hi Roger,

Could you please check Kristoff's update. I still not agree to delete the man page. we could generate it from the help output, but (based on my experience with customers) deleting the man page and count in that user will use --help instead will not be the customer's option instead of opening a support case or asking us directly.

Best regards,
Ab

@liangxin1300
Copy link
Author

liangxin1300 commented Aug 14, 2019

Actually the man pages arealready generated from the help output, I guess maybe it does not cover the old aliases though. See the spec file:
https://build.opensuse.org/package/view_file/openSUSE:Backports:SLE-15-SP1/ha-cluster-bootstrap/ha-cluster-bootstrap.spec?expand=1

According to this spec file, we've defined mkhelp function, but just use it to generate man page for "geo-*", maybe we should just change the spec file, by using mkhelp function to generate other parts of man page like 'init' and 'join'

@ab-mohamed
Copy link

Actually the man pages arealready generated from the help output, I guess maybe it does not cover the old aliases though. See the spec file:
https://build.opensuse.org/package/view_file/openSUSE:Backports:SLE-15-SP1/ha-cluster-bootstrap/ha-cluster-bootstrap.spec?expand=1

According to this spec file, we've defined mkhelp function, but just use it to generate man page for "geo-*", maybe we should just change the spec file, by using mkhelp function to generate other parts of man page like 'init' and 'join'

Great! May we have a try for that and see the output in a testing RPM?

Best regards,
Ab

@zzhou1
Copy link

zzhou1 commented Aug 14, 2019

Actually the man pages arealready generated from the help output, I guess maybe it does not cover the old aliases though. See the spec file:
https://build.opensuse.org/package/view_file/openSUSE:Backports:SLE-15-SP1/ha-cluster-bootstrap/ha-cluster-bootstrap.spec?expand=1

Thanks Kristoffer for this low hanging fruit !!!
And, then, this will be only a SPEC change, not have to change any code here, hehe ;)

Xin will propose the SEPC change firstly toward Factory and backport it to those related SLE versions then.

According to this spec file, we've defined mkhelp function, but just use it to generate man page for "geo-*", maybe we should just change the spec file, by using mkhelp function to generate other parts of man page like 'init' and 'join'

Great! May we have a try for that and see the output in a testing RPM?

Much appreciate your feedback indeed!

Thanks,
Roger

Best regards,
Ab

@krig
Copy link
Collaborator

krig commented Aug 14, 2019

Sounds good!

@ab-mohamed
Copy link

Actually the man pages arealready generated from the help output, I guess maybe it does not cover the old aliases though. See the spec file:
https://build.opensuse.org/package/view_file/openSUSE:Backports:SLE-15-SP1/ha-cluster-bootstrap/ha-cluster-bootstrap.spec?expand=1

Thanks Kristoffer for this low hanging fruit !!!
And, then, this will be only a SPEC change, not have to change any code here, hehe ;)

Xin will propose the SEPC change firstly toward Factory and backport it to those related SLE versions then.

According to this spec file, we've defined mkhelp function, but just use it to generate man page for "geo-*", maybe we should just change the spec file, by using mkhelp function to generate other parts of man page like 'init' and 'join'

Great! May we have a try for that and see the output in a testing RPM?

Much appreciate your feedback indeed!

Thanks,
Roger

Best regards,
Ab

Great that we all agree on the same action now. ;)

@liangxin1300 Is it possible to have the testing RPM today? :)

Best regards,
Ab

@liangxin1300
Copy link
Author

@ab-mohamed I've created request to Factory, you can temporary update it from my home branch here: https://build.opensuse.org/package/binaries/home:XinLiang:branches:network:ha-clustering:Factory/ha-cluster-bootstrap/SLE_15_SP1

@ab-mohamed
Copy link

@ab-mohamed I've created request to Factory, you can temporary update it from my home branch here: https://build.opensuse.org/package/binaries/home:XinLiang:branches:network:ha-clustering:Factory/ha-cluster-bootstrap/SLE_15_SP1

Hi @liangxin1300,

Thank you for your update.

Is https://download.opensuse.org/repositories/home:/XinLiang:/branches:/network:/ha-clustering:/Factory/SLE_15/noarch/ha-cluster-bootstrap-0.5-101.1.noarch.rpm the correct link and RPM version for SLE15 as I am using SLE15 instead of SLE15 SP1?

Best regards,
Ab

@liangxin1300
Copy link
Author

@ab-mohamed
Copy link

ab-mohamed commented Aug 15, 2019

For SL15, you can use this https://build.opensuse.org/package/binaries/home:XinLiang:branches:network:ha-clustering:Factory/ha-cluster-bootstrap/SLE_15
but I think them are same

Hi Xin,

Here are my testing steps and results executed on SLES 15:

  1. Current 'ha-cluster-bootstrap' RPM version:
node1:~ # rpm -qa | grep ha-cluster-bootstrap
ha-cluster-bootstrap-0.5-3.46.noarch

node2:~ # rpm -qa | grep ha-cluster-bootstrap
ha-cluster-bootstrap-0.5-3.46.noarch
  1. Updating 'ha-cluster-bootstrap' RPM version:
node1:~ # rpm -Uvh /tmp/updated/ha-cluster-bootstrap-0.5-101.1.noarch.rpm 
warning: /tmp/updated/ha-cluster-bootstrap-0.5-101.1.noarch.rpm: Header V3 RSA/SHA256 Signature, key ID 1e4baf7a: NOKEY
Preparing...                          ################################# [100%]
Updating / installing...
   1:ha-cluster-bootstrap-0.5-101.1   ################################# [ 50%]
Cleaning up / removing...
   2:ha-cluster-bootstrap-0.5-3.46    ################################# [100%]


node2:~ # rpm -Uvh /tmp/updated/ha-cluster-bootstrap-0.5-101.1.noarch.rpm
warning: /tmp/updated/ha-cluster-bootstrap-0.5-101.1.noarch.rpm: Header V3 RSA/SHA256 Signature, key ID 1e4baf7a: NOKEY
Preparing...                          ################################# [100%]
Updating / installing...
   1:ha-cluster-bootstrap-0.5-101.1   ################################# [ 50%]
Cleaning up / removing...
   2:ha-cluster-bootstrap-0.5-3.46    ################################# [100%]
  1. Checking 'ha-cluster-init' against -S option:
node1:~ # man ha-cluster-init
[OUTPUT TRIMMED]
       -S, --enable-sbd
              Enable SBD even if no SBD device is configured (diskless mode)
[OUTPUT TRIMMED]

@liangxin1300 : Well done!
Could you please push the updated RPM version to the official SLE Repos as the last step for this bug?
I have updated Bugzilla also with my comment.

Best regards,
Ab

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.

5 participants