Shahar Havivi has uploaded a new change for review.
Change subject: add/del network - add bridgesless network ......................................................................
add/del network - add bridgesless network
Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 --- M vdsm/clientIF.py M vdsm/configNetwork.py M vdsm/netinfo.py 3 files changed, 123 insertions(+), 58 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/48/848/1 -- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 1: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 2: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(3 inline comments)
.................................................... File vdsm/configNetwork.py Line 240: open(conffile, 'w').write("""DEVICE=%s.%s\nONBOOT=yes\nVLAN=yes\nBOOTPROTO=none\nBRIDGE=%s\n""" % (pipes.quote(iface), vlanId, pipes.quote(network))) 1. Use contexts, don't assume the GC will close files for you 2. The string looks like it would enjoy sitting in a private constant at the top of the module instead of all cramped up in here.
Line 505: <bridge name='%s'/></network>''' % (netName, network) xmlencode args
Line 553: bridged = False It's the second time I see this. I think it shoul've been extracted to a method. let's try storage.misc.parseBool
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 2: (3 inline comments)
.................................................... File vdsm/configNetwork.py Line 240: open(conffile, 'w').write("""DEVICE=%s.%s\nONBOOT=yes\nVLAN=yes\nBOOTPROTO=none\nBRIDGE=%s\n""" % (pipes.quote(iface), vlanId, pipes.quote(network))) 1. as you can see all the vdsm code in this module are at this format, I add my change as this module to be compatible with the rest of the code, look at the diff... 2. the same as #1
Line 505: <bridge name='%s'/></network>''' % (netName, network) Done
Line 553: bridged = False Done
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 3: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Ayal Baron has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(11 inline comments)
I got tired of reviewing in the middle (not patch author's fault, the code was convoluted to begin with).
.................................................... File vdsm/configNetwork.py Line 260: open(conffile, 'w').write("""DEVICE=%s.%s\nONBOOT=yes\nVLAN=yes\nBOOTPROTO=none\n""" % (pipes.quote(iface), vlanId)) I would split the content from the writing to make this section clearer and remove dupication. something like:
net = """DEVICE=%s.%s\nONBOOT=yes\nVLAN=yes\nBOOTPROTO=none\n%s""" % (pipes.quote(iface), vlanId, 'BRIDGE=%s\n' % pipes.quote(network) if bridged else "") open(conffile, 'w').write(net)
(or split net line into 2 if you don't like using x if whatever else something else)
Line 375: if bridged: You should turn bridged into a boolean before using it, otherwise 'false' would be analysed as True. something like: bridged = misc.parseBool(bridged)
Line 443: def addNetwork(network, vlan=None, bonding=None, nics=None, ipaddr=None, netmask=None, gateway=None, This function is a disaster and the patch is just making it that much worse. Should split internal functionalities into separate functions, so that flow is clear. It is waaaaaaay too long.
Line 447: bridged = utils.tobool(options.get('bridged', 'True')) addNetwork is the old API, why are we adding support for bridgeless networks to it and not only to setupNetworks?
And in any event, I hate the usage of 'options', it is wrong. why is 'bridged' an option but 'force' is not nor any other parameter (incl. 'optional' ones)?
Line 450: if not _isTrue(force) and bridged: is there no validation for bridgeless?
Shouldn't block this patch, but instead of not _isTrue which has weird semantics, why not use misc.parseBool for boolean params in API and then just say "if not force"? e.g. force = misc.parseBool(force) if not force: (same applies to bridge)
Line 468: configWriter.addVlan(vlan, bonding or nics[0], network, bridged) 'bonding or nics[0]' repeats in this function, why not define ifbond = bonding or nics[0] at the beginning and then use it?
Line 501: and bridged: why is dhcp dependent on bridge? and what's the point in doing this if a second later you rub ifup(network) if it is bridged? (I'm assuming the logic here is first ifup would do the work in a separate process and second would immediately return, but this is convoluted, need to make the code more straightforward)
Line 510: ifaceNetwork = bonding or nics[0] I see it's already defined, so need to move this up and use it all over
Line 513: if not utils.tobool(options.get('skipLibvirt', False)): Again, not related to this patch but,
Again usage of options for a hidden option... In the least there should be a comment in the docstring about the options...
I see we have 3 implementations of the same things: misc.parseBool util.tobool configNetwork._isTrue
Argh.
Line 516: def createLibvirtNetwork(network, bridged, iface): Looks (to me) like the logical order of the params should be changed to (network, iface, bridged) - i.e. devices first then logic modifier
Also, I'm not certain about this, but since you have if bridged many times in the calling function, why not just accept the 'forward mode' as a param and avoid the different paths in the function?
Line 589: if configWriter is None: Why not move if configWriter up and then unite the if bridged clauses into a single clause? i.e. if configWriter... if bridged: assert... if network: ....
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 4: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 5: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 5: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Peter V. Saveliev has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 6: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 6: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 7: Fails
problem with bond with vlans, will send a new patch
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 8: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Peter V. Saveliev has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 8: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Ayal Baron has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 8: (11 inline comments)
.................................................... File vdsm/API.py Line 1086: options['bridged'] = bridged options['bridged'] = util.tobool(bridged)
(I'm not sure if this should be done here or in the bindings)
Line 1101: options['bridged'] = bridged options['bridged'] = util.tobool(bridged)
.................................................... File vdsm/clientIF.py Line 118: configNetwork.createLibvirtNetwork(network, True, None) it would be easier on the eye if you pass the vars as kwargs (e.g. bridged=True, whatever=None)
.................................................... File vdsm_cli/vdsClient.py Line 1391: bridge = params.get('bridge', '') why didn't you change the name here to network as you did in the implementation? 'bridge' here is misleading
.................................................... File vdsm/configNetwork.py Line 264: if _isTrue(bridged): addVlan should be called with bridged as boolean, not string. so just need to be: "if bridged:"
Line 508: if _isTrue(bridged): This is a private method that should be called with bridged as boolean not string, so just needs to be: "if bridged:"
Line 583: skipLibvirt = _isTrue(options.get('skipLibvirt', False)) skipLibvirt should also be boolean at this point
Line 591: _addNetworkValidation(_netinfo, bridge=network if _isTrue(bridged) else None, vlan=vlan, bonding=bonding, nics=nics, same for bridged
Line 614: if _isTrue(bridged): same
Line 626: if not bonding and _isTrue(bridged): same
.................................................... File vdsm/netinfo.py Line 57: def bridgeless(): shouldn't this be 'getNetsByPrefix' or something? and accept prefix as param so then it would be generic? i.e. something like: def getNetsByPrefix(netPrefix): ... if name.startswith(netPrefix): ...
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 8: (11 inline comments)
.................................................... File vdsm/API.py Line 1086: options['bridged'] = bridged I am removing this code and passing it in the options{}
Line 1101: options['bridged'] = bridged the same...
.................................................... File vdsm/clientIF.py Line 118: configNetwork.createLibvirtNetwork(network, True, None) Done
.................................................... File vdsm_cli/vdsClient.py Line 1391: bridge = params.get('bridge', '') this is not my code, the is bridge and bridged ie bridge=bridgename and bridged=True/False any way it will be pass in options so I will discard the changes in this file
.................................................... File vdsm/configNetwork.py Line 264: if _isTrue(bridged): Done
Line 508: if _isTrue(bridged): Done
Line 583: skipLibvirt = _isTrue(options.get('skipLibvirt', False)) I will do it in a different patch, this need to be tested
Line 591: _addNetworkValidation(_netinfo, bridge=network if _isTrue(bridged) else None, vlan=vlan, bonding=bonding, nics=nics, Done
Line 614: if _isTrue(bridged): Done
Line 626: if not bonding and _isTrue(bridged): Done
.................................................... File vdsm/netinfo.py Line 57: def bridgeless(): I wanted it to be the same as vlans(), bridgeless() methods
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 9: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 9: (6 inline comments)
minor comments and questions... review unfinished. I wish we could talk about this patch.
.................................................... File vdsm/clientIF.py Line 118: configNetwork.createLibvirtNetwork(network, True, None) having argument names here would increase readability.
.................................................... File vdsm_cli/vdsClient.py Line 1424: for k in ['bridge', 'vlan', 'bond', 'nics', ]: do you really want to own this line in `git blame`? ;-)
.................................................... File vdsm/netinfo.py Line 26: from fnmatch import fnmatch imports from stdlib should come first. libvirtconnection later.
Line 59: Get dict of birdgeless networks I don't see anything here which is specific to bridgeless network. Cannot this be safely renamed to "networks()" ?
Line 348: nics = [] getNicsVlanAndBondingForBridgeless() looks like a repetition of this code. Cannot this be unified with something like
if bridged: ports = self.networks[network]['ports'] else: ports = bridgeless()[network]
Line 354: port = nic unrelated to your patch, but this looks very bad - how come are we changing port within the iteration??
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 9: (6 inline comments)
.................................................... File vdsm/clientIF.py Line 118: configNetwork.createLibvirtNetwork(network, True, None) Done
.................................................... File vdsm_cli/vdsClient.py Line 1424: for k in ['bridge', 'vlan', 'bond', 'nics', ]: Done
.................................................... File vdsm/netinfo.py Line 26: from fnmatch import fnmatch Done
Line 59: Get dict of birdgeless networks actually it is only bridgeless networks, in bridged networks we have no interfaces this is the reason for the if statement in line 71
Line 348: nics = [] Done
Line 354: port = nic right, I will fix it
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 10: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 10: (1 inline comment)
.................................................... File vdsm/configNetwork.py Line 583: skipLibvirt = _isTrue(options.get('skipLibvirt', False)) _isTrue() is deprecated now, please use utils.tobool() instead
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 11: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 11: I would prefer that you didn't submit this
(6 inline comments)
minor comments, mostly
.................................................... File vdsm/clientIF.py Line 119: # remove bridged networks that their bridge not exists ouch, this seems unrelated to bridgeless, and should probably go into vdsm-restore-net script. Either way, please separate into another patch.
.................................................... File vdsm/configNetwork.py Line 29: from xml.sax.saxutils import escape import order - libvirt is not (yet!) part of python's stdlib
Line 586: bondingOptions=bondingOptions, bridged=bridged, skipLibvirt=skipLibvirt) please break long lines
.................................................... File vdsm/netinfo.py Line 26: from xml.dom import minidom nonstandard order
Line 245: for netname in getnetworks().keys(): I'd rather cache the results of getnetworks, poor libvirt should not suffer so much.
Line 387: for netname in getnetworks().keys(): use iteritems.
but list comprehension is even cooler!
return [ netname for (netname, net) in getnetworks().iteritems() if 'bridge' in net ]
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Peter V. Saveliev has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 11: I would prefer that you didn't submit this
have no issues beside of ones that Dan said in comments.
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 11: (6 inline comments)
.................................................... File vdsm/clientIF.py Line 119: # remove bridged networks that their bridge not exists Done
.................................................... File vdsm/configNetwork.py Line 29: from xml.sax.saxutils import escape Done
Line 586: bondingOptions=bondingOptions, bridged=bridged, skipLibvirt=skipLibvirt) Done
.................................................... File vdsm/netinfo.py Line 26: from xml.dom import minidom Done
Line 245: for netname in getnetworks().keys(): Done
Line 387: for netname in getnetworks().keys(): Done
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 12: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 13: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 14: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 14: I would prefer that you didn't submit this
(4 inline comments)
.................................................... File vdsm/clientIF.py Line 118: configNetwork.createLibvirtNetwork(network=bridge, bridged=True, iface=None) it would be nicer if iface was None by default
.................................................... File vdsm/netinfo.py Line 247: for netname in nets.keys(): iterkeys() is cooler
Line 248: d['networks'][netname] = {} I find that building a dictionary like that is cumbersome. why not have
if nets[netname]['bridged']: d['networks'][netname] = {ports:, stp....} else d['networks'][netname] = {interface:}
Line 281: for vlan in vlans() ]) I originally thought that we'd have a new item specific for bridges, so that even bridged networks could have longer names. please consider. maybe in another patch.
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 14: (4 inline comments)
.................................................... File vdsm/clientIF.py Line 118: configNetwork.createLibvirtNetwork(network=bridge, bridged=True, iface=None) Done
.................................................... File vdsm/netinfo.py Line 247: for netname in nets.keys(): Done
Line 248: d['networks'][netname] = {} Done
Line 281: for vlan in vlans() ]) sure, its in my mind
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Shahar Havivi has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 15: Verified
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: add/del network - add bridgesless network ......................................................................
Patch Set 17: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: add/del network - add bridgesless network ......................................................................
add/del network - add bridgesless network
Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 --- M vdsm/clientIF.py M vdsm/configNetwork.py M vdsm/netinfo.py 3 files changed, 128 insertions(+), 90 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/848 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: Id7a3efea92312ac628e0373a5c29fbb1669058f4 Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi shavivi@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Peter V. Saveliev peet@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Shahar Havivi shavivi@redhat.com
vdsm-patches@lists.fedorahosted.org