Change in vdsm[master]: netinfo: remove an assertion from getBondingForNic

Ondřej Svoboda has uploaded a new change for review.

Change subject: netinfo: remove an assertion from getBondingForNic
......................................................................

netinfo: remove an assertion from getBondingForNic

The following commands show that a NIC cannot be enslaved to multiple bonds:

 # ip link add type dummy
 # ip link add type bond
 # ip link add type bond
 # echo +dummy0 > /sys/class/net/bond0/bonding/slaves
 # echo +dummy0 > /sys/class/net/bond1/bonding/slaves
 -bash: echo: write error: Device or resource busy

The reason to remove assertions is quoted from Nir Soffer's
https://gerrit.ovirt.org/34361/ (which this patch replaces,
partially):

The code was assuming that asserts are always available, and if running
within optimized mode, the code would skip the assert and use invalid
configuration, possibly corrupting application state or failing with
unrelated errors, hiding the root cause of the error.

Change-Id: I0b39d3b0040c86a8c4c4babe4af03b9cf8f6bdd4
Signed-off-by: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
---
M lib/vdsm/netinfo.py
1 file changed, 2 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/72/45072/1

diff --git a/lib/vdsm/netinfo.py b/lib/vdsm/netinfo.py
index dfe8994..bac6ec5 100644
--- a/lib/vdsm/netinfo.py
+++ b/lib/vdsm/netinfo.py
@@ -897,11 +897,8 @@
     def getBondingForNic(self, nic):
         bondings = [b for (b, attrs) in self.bondings.iteritems() if
                     nic in attrs['slaves']]
-        if bondings:
-            assert len(bondings) == 1, \
-                "Unexpected configuration: More than one bonding per nic"
-            return bondings[0]
-        return None
+        # only a single bond can have the NIC as a slave
+        return bondings[0] if bondings else None
 
     def getNicsVlanAndBondingForNetwork(self, network):
         vlan = None


-- 
To view, visit https://gerrit.ovirt.org/45072
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0b39d3b0040c86a8c4c4babe4af03b9cf8f6bdd4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;

automation(a)ovirt.org has posted comments on this change.

Change subject: netinfo: remove an assertion from getBondingForNic
......................................................................


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

-- 
To view, visit https://gerrit.ovirt.org/45072
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b39d3b0040c86a8c4c4babe4af03b9cf8f6bdd4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: remove an assertion from getBondingForNic
......................................................................


Patch Set 1: Verified+1

The correctness of the change is shown in the commit message.

-- 
To view, visit https://gerrit.ovirt.org/45072
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b39d3b0040c86a8c4c4babe4af03b9cf8f6bdd4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Ido Barkan has posted comments on this change.

Change subject: netinfo: remove an assertion from getBondingForNic
......................................................................


Patch Set 1: Code-Review+1

-- 
To view, visit https://gerrit.ovirt.org/45072
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b39d3b0040c86a8c4c4babe4af03b9cf8f6bdd4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: netinfo: remove an assertion from getBondingForNic
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/45072/1/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 898
Line 899
Line 900
Line 901
Line 902
This change is wrong.

asserts are designed exactly for this use case; It is impossible to have more then one
bonding, so we assert that this is indeed true. This assert protect us from the
impossible, and document our intent.

Without the assert, if our assumption is incorrect, we will silently ignore the reality.


-- 
To view, visit https://gerrit.ovirt.org/45072
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b39d3b0040c86a8c4c4babe4af03b9cf8f6bdd4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: remove an assertion from getBondingForNic
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/45072/1/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 898
Line 899
Line 900
Line 901
Line 902
>  This change is wrong. 
What is the likelihood that bondings, as
implemented in the Linux kernel, began to accept NICs already enslaved to other bonds? I
think we would not like to hear such news. I think (im)possible Linux bugs should not be
caught in VDSM.

Please not that we're not checking a user's configuration, this is no longer true
(if it ever has). These functions are only convenience wrappers/accessors to information
acquired from the kernel.


-- 
To view, visit https://gerrit.ovirt.org/45072
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b39d3b0040c86a8c4c4babe4af03b9cf8f6bdd4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes