8:47 a.m.
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 <dougsland(a)redhat.com>
---
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 <dougsland(a)redhat.com>
4:14 a.m.
New subject: Change in vdsm[master]: delNetwork(): Add skipLibvirt validation
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 <dougsland(a)redhat.com>
---
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4:14 a.m.
New subject: Change in vdsm[master]: delNetwork(): Add skipLibvirt validation
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
11:41 p.m.
New subject: Change in vdsm[master]: delNetwork(): Add skipLibvirt validation
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
9:58 p.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
5:25 p.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
3:18 p.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
2:57 p.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
6:46 a.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
1:34 a.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
11:44 p.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
11:41 p.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
8:58 a.m.
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 <dougsland(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com>
4452
days inactive
4454
days old
vdsm-patches@lists.fedorahosted.org
12 comments
4 participants
participants (4)
-
Dan Kenigsberg -
Douglas Schilling Landgraf -
Igor Lvovsky -
shavivi＠redhat.com