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

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has uploaded a new change for review.

Change subject: improve add/del bridgesless network
......................................................................

improve add/del bridgesless network

This patch will improve the commit
1a24550b968ca48bb808e263368743f4c91adde4

Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Signed-off-by: Douglas Schilling Landgraf 
---
M vdsm/configNetwork.py
M vdsm/netinfo.py
M vdsm_reg/deployUtil.py.in
3 files changed, 41 insertions(+), 14 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/15/3615/1
--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf

Shahar Havivi has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 1: (4 inline comments)

....................................................
File vdsm/configNetwork.py
Line 323:         self._removeFile(self.NET_CONF_PREF + bridge)
why did you remove the check?

Line 737:         subprocess.call([constants.EXT_BRCTL, 'delbr', network])
why isn't this code in removeBridge? the same for removeVlan...

....................................................
File vdsm/netinfo.py
Line 99:     mtu = "0"
I am not sure zero default ok...
the default is 1500...

Line 266:             if len(ports(netname)) != 0:
this is the old version that we use the ports instead of libvirt network,
the reason that we change to libvirt is that we have networks that have no actual bridge.
Dan what do you say?

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 1: (3 inline comments)

....................................................
File vdsm/configNetwork.py
Line 323:         self._removeFile(self.NET_CONF_PREF + bridge)
Indeed, it's a mistake from the middle of the night ;-) Anyway, I will provide a improved patch.

Line 737:         subprocess.call([constants.EXT_BRCTL, 'delbr', network])
I can move there, but if I understood correctly it's "configWriter", it's about managing config files only.

....................................................
File vdsm/netinfo.py
Line 99:     mtu = "0"
if the bridge doesn't exist, I don't think it's correct to setup mtu to 1500 as default value.

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/netinfo.py
Line 274:                 # interface alrady removed!
ouch, typo.. new patch

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/netinfo.py
Line 94:         ports = os.listdir(bridgePath)
os.path.join(bridgePath)  is do nothing in your case.
I think you meant to:

bridgePath = os.path.join('/sys/class/net' + bridge + 'brif')
if os.path.exists(bridgePath):
     ...

Line 103:         mtu = file(ifacePath).readline().rstrip()
again, there is no need in os.path.join(ifacePath), just ifacePath

Line 113: 
same as above

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(6 inline comments)

....................................................
Commit Message
Line 11: 
it seems that this patch fixes several problems. If so please split it, and explain the solved issues.

....................................................
File vdsm/configNetwork.py
Line 746:         removeLibvirtNetwork(vlandev)
this is very confusing, since vlandev is NOT a libvirt network. why is this needed?

....................................................
File vdsm/netinfo.py
Line 93:     if os.path.exists(os.path.join(bridgePath)):
why do you expect this to fail? I think that this function should be called only on existing bridges. We can think, however, to change the semantics, catch the exception and return []

Line 274:                 # interface already removed!
if the network is removed, why should we report it at all?

....................................................
File vdsm_reg/deployUtil.py.in
Line 42: sys.path.append("/usr/share/vdsm")
messing with sys.path during runtime is evil. let us try hard to avoid this.

oh, and import would fail if /usr/share/vdsm does not exist yet.

Line 900:             configNetwork.createLibvirtNetwork(mgtBridge, bridged=True)
I would very much prefer it if skipLibvirt is adhered to, instead.

now you create a network only to delete it one line below...

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 3: (9 inline comments)

....................................................
Commit Message
Line 11: 
I will provide a specific patch.

....................................................
File vdsm/configNetwork.py
Line 746:         removeLibvirtNetwork(vlandev)
Indeed, I will provide a new patch not touching in these lines.

....................................................
File vdsm/netinfo.py
Line 93:     if os.path.exists(os.path.join(bridgePath)):
This part of change will be aborted, not required anymore.

Line 94:         ports = os.listdir(bridgePath)
This part of change will be aborted, not required anymore.

Line 103:         mtu = file(ifacePath).readline().rstrip()
This part of change will be aborted, not required anymore.

Line 113: 
This part of change will be aborted, not required anymore.

Line 274:                 # interface already removed!
Correct, new patch won't touch in this part.

....................................................
File vdsm_reg/deployUtil.py.in
Line 42: sys.path.append("/usr/share/vdsm")
ok, agreed.

Line 900:             configNetwork.createLibvirtNetwork(mgtBridge, bridged=True)
Ok, I will send a new patch to review.

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 4: (1 inline comment)

....................................................
Commit Message
Line 7: improve add/del bridgesless network
you handled only the 'del' part, right?
Is it solve the whole problem?

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 4: I would prefer that you didn't submit this

(1 inline comment)

a more descriptive commit message could be fun.

....................................................
File vdsm/configNetwork.py
Line 706: def delNetwork(network, vlan=None, bonding=None, nics=None, force=False, configWriter=None, **options):
have you checked that no one calling delNetwork and passing args by order?

please avoid adding long lines.

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: improve add/del bridgesless network
......................................................................

Patch Set 4: (2 inline comments)

....................................................
Commit Message
Line 7: improve add/del bridgesless network
Yes, I have follow Dan's suggestion to use the skipLibvirt approach and I have figure out a better solution.

....................................................
File vdsm/configNetwork.py
Line 706: def delNetwork(network, vlan=None, bonding=None, nics=None, force=False, configWriter=None, **options):
> have you checked that no one calling delNetwork and passing args by order?

Yes. I will prepare a second patch to improve it too.
> please avoid adding long lines.

OK

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

Change in vdsm[master]: delNetwork(): Add skipLibvirt validation

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: delNetwork(): Add skipLibvirt validation
......................................................................

Patch Set 5: Verified

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

Change in vdsm[master]: delNetwork(): Add skipLibvirt validation

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: delNetwork(): Add skipLibvirt validation
......................................................................

Patch Set 5: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi

Change in vdsm[master]: delNetwork(): Add skipLibvirt validation

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: delNetwork(): Add skipLibvirt validation
......................................................................

delNetwork(): Add skipLibvirt validation

Do not try to delete an interface from libvirt which was not added.
Introduced by: 1a24550b968ca48bb808e263368743f4c91adde4

Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Signed-off-by: Douglas Schilling Landgraf 
---
M vdsm/configNetwork.py
1 file changed, 11 insertions(+), 5 deletions(-)

Approvals:
  Douglas Schilling Landgraf: Verified
  Dan Kenigsberg: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/3615
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I7f170eaced2d6f756b84cb52e9efce6dc9722405
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Douglas Schilling Landgraf 
Gerrit-Reviewer: Igor Lvovsky 
Gerrit-Reviewer: Shahar Havivi