Mark Wu has uploaded a new change for review.
Change subject: Extract bonding options building into a separate function. ......................................................................
Extract bonding options building into a separate function.
To make setupNetworks shorter, move the bonding options building code into a separate function.
Signed-off-by: Mark Wu wudxw@linux.vnet.ibm.com Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e --- M vdsm/configNetwork.py 1 file changed, 30 insertions(+), 27 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/85/8885/1
diff --git a/vdsm/configNetwork.py b/vdsm/configNetwork.py index 3940446..6de38e3 100755 --- a/vdsm/configNetwork.py +++ b/vdsm/configNetwork.py @@ -1249,6 +1249,30 @@ del bondings[bond]
+def _buildBondOptions(bondName, bondings): + logger = logging.getLogger("_buildBondOptions") + + # We need to use the newest host info + _ni = netinfo.NetInfo() + + bond = {} + if bondings.get(bondName): + bond['nics'] = bondings[bondName]['nics'] + bond['bondingOptions'] = bondings[bondName].get('options', None) + elif bondName in _ni.bondings: + # We may not receive any information about the bonding device if it is + # unchanged. In this case check whether this bond exists on host and + # take its parameters. + logger.debug("Fetching bond %r info", bondName) + existingBond = _ni.bondings[bondName] + bond['nics'] = existingBond['slaves'] + bond['bondingOptions'] = existingBond['cfg'].get('BONDING_OPTS', None) + else: + raise ConfigNetworkError(ne.ERR_BAD_PARAMS, "No given bonding option, \ + nor existing bond %s found." % bondName) + return bond + + def setupNetworks(networks={}, bondings={}, **options): """Add/Edit/Remove configuration for networks and bondings.
@@ -1292,12 +1316,6 @@ _netinfo = netinfo.NetInfo() configWriter = ConfigWriter() networksAdded = set() - # keep set netsWithNewBonds to be able remove - # a new added network if connectivity check fail. - # If a new network needs to be created on top of existing bond, - # we will need to keep the bond on rollback flow, - # else we will break the new created bond. - netsWithNewBonds = set()
logger.debug("Setting up network according to configuration: " "networks:%r, bondings:%r, options:%r" % (networks, @@ -1327,28 +1345,10 @@ # Check whether bonds should be resized _editBondings(bondings, configWriter)
- # We need to use the newest host info - _ni = netinfo.NetInfo() for network, networkAttrs in networks.iteritems(): d = dict(networkAttrs) if 'bonding' in d: - # we may not receive any information - # about the bonding device if it is unchanged - # In this case check whether this bond exists - # on host and take its parameters - if bondings.get(d['bonding']): - d['nics'] = bondings[d['bonding']]['nics'] - d['bondingOptions'] = \ - bondings[d['bonding']].get('options', None) - # we create a new bond - if network in networksAdded: - netsWithNewBonds.add(network) - elif d['bonding'] in _ni.bondings: - logger.debug("Updating bond %r info", d['bonding']) - d['nics'] = _ni.bondings[d['bonding']]['slaves'] - d['bondingOptions'] = \ - _ni.bondings[d['bonding']]['cfg'].get( - 'BONDING_OPTS', None) + d.update(_buildBondOptions(d['bonding'], bondings)) else: d['nics'] = [d.pop('nic')] d['force'] = force @@ -1363,8 +1363,11 @@ CONNECTIVITY_TIMEOUT_DEFAULT))): logger.info('Connectivity check failed, rolling back') for network in networksAdded: - delNetwork(network, force=True, - implicitBonding=network in netsWithNewBonds) + # If the new added network was created on top of + # existing bond, we need to keep the bond on rollback + # flow, else we will break the new created bond. + isNew = networkAttrs['bonding'] in bondings + delNetwork(network, force=True, implicitBonding=isNew) raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, 'connectivity check failed') except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(2 inline comments)
I still have to run some tests on this. But some small things I was able to see without delving to deep into it.
.................................................... File vdsm/configNetwork.py Line 1267: existingBond = _ni.bondings[bondName] Line 1268: bond['nics'] = existingBond['slaves'] Line 1269: bond['bondingOptions'] = existingBond['cfg'].get('BONDING_OPTS', None) Line 1270: else: Line 1271: raise ConfigNetworkError(ne.ERR_BAD_PARAMS, "No given bonding option, \ I'll have to review the implications of adding this new exceptional error. In any case, the line should read:
raise ConfigNetworkError(ne.ERR_BAD_PARAMS, "No given bonding option, " "nor existing bond %s found." % bondName)
Note that trailing backslash is not necessary ;-). Line 1272: nor existing bond %s found." % bondName) Line 1273: return bond Line 1274: Line 1275:
Line 1365: for network in networksAdded: Line 1366: # If the new added network was created on top of Line 1367: # existing bond, we need to keep the bond on rollback Line 1368: # flow, else we will break the new created bond. Line 1369: isNew = networkAttrs['bonding'] in bondings The isNew variable is only used once, I would rather have delNetwork(network, force=True, implicitBonding=networkAttrs['bonding'] in bondings) Line 1370: delNetwork(network, force=True, implicitBonding=isNew) Line 1371: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1372: 'connectivity check failed') Line 1373: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 2:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/219/ (2/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 2:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/253/ (1/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/219/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/253/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 3:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/254/ (1/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 3:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/220/ (2/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 3:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/220/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/254/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(1 inline comment)
Sorry for the long time it took me to do a second, in depth review of this patch. I like a lot the fact that this reduces the size of setupNetworks and splits the bonding option builgin responsability away from it.
With a couple of tweaks I think we could take it in soon. Thanks for the refactoring ;-)
.................................................... File vdsm/configNetwork.py Line 1392: # If the new added network was created on top of Line 1393: # existing bond, we need to keep the bond on rollback Line 1394: # flow, else we will break the new created bond. Line 1395: isNew = networkAttrs['bonding'] in bondings Line 1396: delNetwork(network, force=True, implicitBonding=isNew) isNew is used only once. I think we could just replace the previous two lines with:
delNetwork( network, force=True, implicitBonding=networkAttrs['bonding'] in bondings ) or delNetwork(network, force=True, implicitBonding=\ networkAttrs['bonding'] in bondings)
better yet: delNetwork(network, force=True, implicitBonding= networkAttrs['bonding'] in bondings)
Apart from that, this 'for' does not runs all the time with the networkAttrs or the last added network in the previour 'for'. Shouldn't this get the networkAttrs corresponding to the current network from networksAdded? Line 1397: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1398: 'connectivity check failed') Line 1399: except: Line 1400: configWriter.restoreBackups()
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 4:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/704/ (2/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 4:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/669/ (1/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 4: Fails
Build Failed
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/669/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/704/ : FAILURE
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Mark Wu has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 4: Verified
It's rebased to the latest master branch, and verified in the flowing cases: 1. create a network on top of non-existing bonding 2. create a network on top of existing bonding 3. create a network on top of existing bonding, and make the connectivity checking failed. The new added network is removed with bonding device remained as expected.
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 5:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/705/ (2/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 5:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/670/ (1/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/670/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/705/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Mark Wu has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 5: Verified
It's rebased to the latest master branch, and verified in the flowing cases: 1. create a network on top of non-existing bonding 2. create a network on top of existing bonding 3. create a network on top of existing bonding, and make the connectivity checking failed. The new added network is removed with bonding device remained as expected.
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 5: Fails; I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1428: # If the new added network was created on top of Line 1429: # existing bond, we need to keep the bond on rollback Line 1430: # flow, else we will break the new created bond. Line 1431: delNetwork(network, force=True, Line 1432: implicitBonding=networks[network]['bonding'] This will cause a KeyError 'bonding' when a connectivity check fails and one of the networks added is not a bond. I would suggest doing: implicitBonding=networks[network].get('bonding') in bondings Line 1433: in bondings) Line 1434: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1435: 'connectivity check failed') Line 1436: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Mark Wu has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 5: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1428: # If the new added network was created on top of Line 1429: # existing bond, we need to keep the bond on rollback Line 1430: # flow, else we will break the new created bond. Line 1431: delNetwork(network, force=True, Line 1432: implicitBonding=networks[network]['bonding'] Good catch! I will fix it in next patch. Thanks! I was focusing the bonding case too much and neglecting the non-bonding case. It seems we need a comprehensive functional testsuite for network configuration. So, Toni, do you know any functional test cases based on vdsm api or engine rest api? Line 1433: in bondings) Line 1434: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1435: 'connectivity check failed') Line 1436: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 5: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1428: # If the new added network was created on top of Line 1429: # existing bond, we need to keep the bond on rollback Line 1430: # flow, else we will break the new created bond. Line 1431: delNetwork(network, force=True, Line 1432: implicitBonding=networks[network]['bonding'] We definitely need to add some functional test for networking. I was thinking to reuse stuff in xmlrpc functional tests, like for example the VM creation for those network tests that concern VM networks(vmUpdateDevice). Line 1433: in bondings) Line 1434: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1435: 'connectivity check failed') Line 1436: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/719/ (2/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/754/ (1/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/719/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/754/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: Verified; Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() this can be quite arduous if there are multiple networks/interfaces on host. your patch calls this per each network that has bonding. Was this intentional? If not, how about passing _ni as an arg? Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
Line 1428: # If the new added network was created on top of Line 1429: # existing bond, we need to keep the bond on rollback Line 1430: # flow, else we will break the new created bond. Line 1431: delNetwork(network, force=True, Line 1432: implicitBonding=networks[network]. I am a bit confused with this seemingly unrelated change of semantics. It might be right, it might be wrong, but I do not see that it is related to the refactoring.
Or maybe I'm missing something, and the semantics has not changed? Line 1433: get('bonding') in bondings) Line 1434: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1435: 'connectivity check failed') Line 1436: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() I would favor having netinfo as a parameter in a following patch to all these functions that get it by themselves (like the two on top). Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (1 inline comment)
our code here is fragile, and needs an urgent refactoring but I'm not yet convinced that this patch maintains our hard-earned functionality.
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() however, this patch - as it stands - introduces a regression of sorts. I'd rather start with the optimization and continue with this refactoring (or push them both simultaneously).
On another matter, Toni: are you convinced about change in line 1432? it seems to me that it does not keep the differentiation between new and edited networks. Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (2 inline comments)
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() I think it is a good idea to rebase this patch on one that does away with the different netinfo instance per method.
On the other matter. There is indeed a change of semantics that troubled me when I first reviewed it and I tried quite a few of scenarios (with the last fix I proposed) to see if it broke something. I tend to agree that this patch would be much easier to take in if it would not do this semantics change, and if necessary that would stand on its own patch. Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
Line 1428: # If the new added network was created on top of Line 1429: # existing bond, we need to keep the bond on rollback Line 1430: # flow, else we will break the new created bond. Line 1431: delNetwork(network, force=True, Line 1432: implicitBonding=networks[network]. Maybe adding the condition that it should be also in networksAdded. Line 1433: get('bonding') in bondings) Line 1434: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1435: 'connectivity check failed') Line 1436: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1428: # If the new added network was created on top of Line 1429: # existing bond, we need to keep the bond on rollback Line 1430: # flow, else we will break the new created bond. Line 1431: delNetwork(network, force=True, Line 1432: implicitBonding=networks[network]. Ok. Let's see if we get it cleared up so we see if the implicitBonding semantics change.
The old code was just checking that for each network of networksAdded: - is in netsWithNewBonds. That means: + network is member of networksAdded + network has a bonding attribute present in bondings.
netsWithNewBonds being defined as a list of networks that are being added and have bonding defined in bondings.
The new code checks that each network of networksAdded: - has a bonding attribute present in bondings.
So in the end, I think they are the same (if I did not miss anything). Line 1433: get('bonding') in bondings) Line 1434: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1435: 'connectivity check failed') Line 1436: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Mark Wu has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (2 inline comments)
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() Ok, I will rebase this patch to your 'Reduce the amount of netinfo instantiation.' patch. Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
Line 1428: # If the new added network was created on top of Line 1429: # existing bond, we need to keep the bond on rollback Line 1430: # flow, else we will break the new created bond. Line 1431: delNetwork(network, force=True, Line 1432: implicitBonding=networks[network]. Toni, thanks a lot for your explanation! Line 1433: get('bonding') in bondings) Line 1434: raise ConfigNetworkError(ne.ERR_LOST_CONNECTION, Line 1435: 'connectivity check failed') Line 1436: except:
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Mark Wu has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() This function is called after bonding update, so it expects a fresh netinfo instance. This code existing in the original code. I just split it from the setupNetwork function. Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() formerly, it was called outside of the loop, and only once. Now, it's called bet net with bond. Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Mark Wu has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 6: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 1314: def _buildBondOptions(bondName, bondings): Line 1315: logger = logging.getLogger("_buildBondOptions") Line 1316: Line 1317: # We need to use the newest host info Line 1318: _ni = netinfo.NetInfo() Yes, you're right. I will fix it in next patch. Thanks. Line 1319: Line 1320: bond = {} Line 1321: if bondings.get(bondName): Line 1322: bond['nics'] = bondings[bondName]['nics']
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/812/ (2/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/777/ (1/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/777/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/812/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7: Looks good to me, but someone else must approve
(2 inline comments)
I have a minor comment to keep consistency with other methods, but since the evolution moves away from there and _buildOptions has but one client, feel free to ignore my comments.
.................................................... File vdsm/configNetwork.py Line 1318: Line 1319: del bondings[bond] Line 1320: Line 1321: Line 1322: def _buildBondOptions(bondName, bondings, _netinfo): I would put _netinfo as having a default value of None, to allow for calling _buildOptions with an implicit netinfo full refresh. However, it is likely that it would end up being removed in later patches. Line 1323: logger = logging.getLogger("_buildBondOptions") Line 1324: Line 1325: bond = {} Line 1326: if bondings.get(bondName):
Line 1320: Line 1321: Line 1322: def _buildBondOptions(bondName, bondings, _netinfo): Line 1323: logger = logging.getLogger("_buildBondOptions") Line 1324: I would add here the:
if _netinfo is None: _netinfo = netinfo.NetInfo() Line 1325: bond = {} Line 1326: if bondings.get(bondName): Line 1327: bond['nics'] = bondings[bondName]['nics'] Line 1328: bond['bondingOptions'] = bondings[bondName].get('options', None)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7: (1 inline comment)
Ignore the previous comments. I think this one makes much more sense.
.................................................... File vdsm/configNetwork.py Line 1318: Line 1319: del bondings[bond] Line 1320: Line 1321: Line 1322: def _buildBondOptions(bondName, bondings, _netinfo): What about, instead we avoid passing _netinfo and only pass to this method _netinfo.bondings? It would reduce the coupling of the two modules and make for a simpler code. Maybe a signature like:
def _buildBondOptions(bondName, currentBondings, newBondings):
where currentBondings would be _netinfo.bondings and, newBondings would be what currently is the bondings param. Line 1323: logger = logging.getLogger("_buildBondOptions") Line 1324: Line 1325: bond = {} Line 1326: if bondings.get(bondName):
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7: Verified
Cherry-picked on top current master and Mark's reversal of the parted patch.
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7: Looks good to me, approved
thanks, Toni! you've convinced me that this is safe.
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7:
@Danken: Sorry that I forgot to mention that the cherry-pick on the current master HEAD needs a minor manual merge.
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 7: I would prefer that you didn't submit this
@Toni - do not be sorry, just send a fix ;-)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 8:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/837/ (2/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 8:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/802/ (1/2)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 8:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/802/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/837/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Antoni Segura Puimedon has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 8: Verified; Looks good to me, but someone else must approve
@Mark, Since it was my suggestion to rebase it on top of my branch, which is not ready to merge, but this patch is, I took the liberty of rebasing the patch on top of the current master HEAD. I hope you don't mind.
@Dan, Done ;-)
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Extract bonding options building into a separate function. ......................................................................
Patch Set 8: Looks good to me, approved
I pretty sure that Mark won't mind.
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Extract bonding options building into a separate function. ......................................................................
Extract bonding options building into a separate function.
To make setupNetworks shorter, move the bonding options building code into a separate function.
Signed-off-by: Mark Wu wudxw@linux.vnet.ibm.com Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Signed-off-by: Antoni S. Puimedon asegurap@redhat.com --- M vdsm/configNetwork.py 1 file changed, 27 insertions(+), 24 deletions(-)
Approvals: Antoni Segura Puimedon: Verified; Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/8885 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I77fefefcefa05f5bd0d7fa2755357d88b7aa615e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: Antoni Segura Puimedon asegurap@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Mark Wu wudxw@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org