Code review comment for ~rafaeldtinoco/ubuntu/+source/fence-agents:lp1865523-bionic

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

+ + d/p/lp1865523-00-fencing-add-consistency-cmdline-stdin.patch:
+ - makes sure cmdline and stdin accept same args

This is a high amount of changes.
They look correct, but I'm afraid it is too much.
Plenty of old arguments got now deprecated - if fence is e.g. putting out warnings that might break people.
E.g. people used to use ipaddr and after the SRU they would get a deprecation warning.

Can we break this down to just those changes we actually need or are these massively intertwined?
If it can be reduced to just a few arguments it might be more review-able to check for the consequences e.g. of marking things deprecated.

+ + d/p/lp1865523-01-fence_scsi-add-FIPS-support.patch:
+ - handle hashlib FIPS exceptions in place of generic error

Sounds bad to refer to FIPS support.
Would we hit this bug (then keep it) or was this just needed to have patches apply for context (then remove this patch and modify the other).

+ + d/p/lp1865523-02-fix-for-ignored-options.patch:
+ - change agents opt names so pacemaker has better compat

This is guly, it is all whitespace but adding "on_target".
The upstream patch is different.
In general whenever you are not 100% as-upstreams patch please use "backport" instead of "upstream" in the origin tag and explain why you changed it.
If you have reduced the big patch to just what is needed that is fine, but please explain what/why.
That is true for all patches in this series

+ + d/p/lp1865523-03-Maintain-ABI-compatibility.patch:
+ - keep ABI compatibility: deal with old and new agent opt names

Odd, but ok (same tag/explain changes as above thou)

+ + d/p/lp1865523-04-fence_scsi-Remove-period.patch:
+ - minor string correction

THe patch says "existing version works fine", but I'm still scared can you make an A/B tests of orig and changed version please ?

+ + d/p/lp1865523-05-fence_scsi-fix-plug-parameter.patch:
+ - use "plug" argument instead of "nodename"
+ - keep previous "nodename" if no "plug" is given

I was scared first but glad that it had compat code in the patch.

+ + d/p/lp1865523-06-fence_scsi-fix-python3-encoding.patch:
+ - encodes before calculating hash

The shebang is variable
  1 #!@PYTHON@ -tt
Would this be a problem for python2?

+ + d/p/lp1865523-07-fence_scsi-fix-incorrect-SCSI-key.patch:
+ - issue calculating SCSI key on > 10 nodes
+ + d/p/lp1865523-08-fence_scsi-node-ID-new-format.patch:
+ - detect node ID using new format (plug + nodename)

These LGTM

+ + d/p/lp1865523-09-fence-metadata-update.xml:
+ - updates all agents XML definitions for build tests

That one really should get some more description and patch-tags.
I know it is the regen due to Ubuntu-patches, but still make it clear in the patch please.

review: Needs Fixing

« Back to merge proposal