Antoni Segura Puimedon has uploaded a new change for review.
Change subject: addNetwork script: Prevent empty params from reaching RunningConfig ......................................................................
addNetwork script: Prevent empty params from reaching RunningConfig
The consumers of the addNetwork script pass empty parameters, e.g. "" in order to fill the positional arguments that the script takes. The problem with that is that those parameters were being passed to addNetwork without modification, and that includes the _alterNetworkConfig wrapper that makes addNetwork requests reach runningConfig (and eventually PersistentConfig).
Due to the issue above, a stored network could contain entries like 'bonding': '' that would mess with selective network restoration.
Change-Id: If6d56eefc05cdb7456f80b7ec13d0be8ad087aa3 Bug-Url: https://bugzilla.redhat.com/1144639 Signed-off-by: Antoni S. Puimedon asegurap@redhat.com --- M vdsm/network/api.py 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/10/33510/1
diff --git a/vdsm/network/api.py b/vdsm/network/api.py index 8879245..f2a2cd6 100755 --- a/vdsm/network/api.py +++ b/vdsm/network/api.py @@ -822,6 +822,10 @@ kwargs = _parseKwargs(sys.argv[3:]) if 'nics' in kwargs: kwargs['nics'] = kwargs['nics'].split(',') + # Remove empty keys so that they are not taken by _alterRunningConfig + for key, value in kwargs.items(): + if key != 'bondingOptions' and value == '': + del kwargs[key] addNetwork(bridge, **kwargs) elif sys.argv[1] == 'del': bridge = sys.argv[2]
oVirt Jenkins CI Server has posted comments on this change.
Change subject: addNetwork script: Prevent empty params from reaching RunningConfig ......................................................................
Patch Set 1:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11725/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12669/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_network_functional_tests_gerrit/200... : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12514/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: addNetwork script: Prevent empty params from reaching RunningConfig ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
Great catch of an ancient bug. I think that the implementation can be more precise.
http://gerrit.ovirt.org/#/c/33510/1//COMMIT_MSG Commit Message:
Line 6: Line 7: addNetwork script: Prevent empty params from reaching RunningConfig Line 8: Line 9: The consumers of the addNetwork script pass empty parameters, e.g. "" Line 10: in order to fill the positional arguments that the script takes. The The only positional argument where this is relevant to are vlan, bonding and nics. bridge is required to be non-empty, and anything past nics is a kwarg.
Let's be prudent and drop only these 3 if they have an empty value. Line 11: problem with that is that those parameters were being passed to Line 12: addNetwork without modification, and that includes the Line 13: _alterNetworkConfig wrapper that makes addNetwork requests reach Line 14: runningConfig (and eventually PersistentConfig).
http://gerrit.ovirt.org/#/c/33510/1/vdsm/network/api.py File vdsm/network/api.py:
Line 821: bridge = sys.argv[2] Line 822: kwargs = _parseKwargs(sys.argv[3:]) Line 823: if 'nics' in kwargs: Line 824: kwargs['nics'] = kwargs['nics'].split(',') Line 825: # Remove empty keys so that they are not taken by _alterRunningConfig after nics' conversion above, we should be worried only about vlan and bonding. Please drop only them if they are empty. Line 826: for key, value in kwargs.items(): Line 827: if key != 'bondingOptions' and value == '': Line 828: del kwargs[key] Line 829: addNetwork(bridge, **kwargs)
Douglas Schilling Landgraf has posted comments on this change.
Change subject: addNetwork script: Prevent empty bond and vlan from reaching RunningConfig ......................................................................
Patch Set 2: Verified+1 Code-Review+1
Thanks Toni!!
Dan Kenigsberg has submitted this change and it was merged.
Change subject: addNetwork script: Prevent empty bond and vlan from reaching RunningConfig ......................................................................
addNetwork script: Prevent empty bond and vlan from reaching RunningConfig
The consumers of the addNetwork script pass empty parameters, e.g. "" in order to fill the positional arguments that the script takes. The problem with that is that those parameters were being passed to addNetwork without modification, and that includes the _alterNetworkConfig wrapper that makes addNetwork requests reach runningConfig (and eventually PersistentConfig).
Due to the issue above, a stored network could contain entries like 'bonding': '' and 'vlan': '' that would mess with selective network restoration.
Change-Id: If6d56eefc05cdb7456f80b7ec13d0be8ad087aa3 Bug-Url: https://bugzilla.redhat.com/1144639 Signed-off-by: Antoni S. Puimedon asegurap@redhat.com Reviewed-on: http://gerrit.ovirt.org/33510 Reviewed-by: Douglas Schilling Landgraf dougsland@redhat.com Tested-by: Douglas Schilling Landgraf dougsland@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com --- M vdsm/network/api.py 1 file changed, 6 insertions(+), 0 deletions(-)
Approvals: Douglas Schilling Landgraf: Verified; Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, approved
Dan Kenigsberg has posted comments on this change.
Change subject: addNetwork script: Prevent empty bond and vlan from reaching RunningConfig ......................................................................
Patch Set 2: Code-Review+2
oVirt Jenkins CI Server has posted comments on this change.
Change subject: addNetwork script: Prevent empty bond and vlan from reaching RunningConfig ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_create-rpms_merged_test_debug/244/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/5877/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4037/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/47/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/43/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/49/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/23/ : SUCCESS
oVirt Jenkins CI Server has posted comments on this change.
Change subject: addNetwork script: Prevent empty bond and vlan from reaching RunningConfig ......................................................................
Patch Set 2:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11734/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12678/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_network_functional_tests_gerrit/200... : There was an infra issue, please contact infra@ovirt.org
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12523/ : SUCCESS
vdsm-patches@lists.fedorahosted.org