Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com
Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Shahar Havivi shav...@redhat.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com
Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Shahar Havivi shav...@redhat.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com
Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com
Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Shahar Havivi shav...@redhat.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com
Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Shahar Havivi shav...@redhat.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com
Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Shahar Havivi shav...@redhat.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvov...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 waaay 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 shav...@redhat.com
Gerrit-Reviewer: Ayal Baron aba...@redhat.com
Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com
Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com
Gerrit-Reviewer: Shahar Havivi shav...@redhat.com
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Peter V. Saveliev p...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
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 shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: add/del network - add bridgesless network
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 shav...@redhat.com Gerrit-Reviewer: Shahar Havivi shav...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/vdsm-patches
