Change in vdsm[master]: add/del network - add bridgesless network

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

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 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Igor Lvovsky]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Igor Lvovsky]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Igor Lvovsky]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Peter V. Saveliev 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shahar Havivi