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 
---
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

[e-mail] 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'])

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.

Ido Barkan has posted comments on this change.

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

Patch Set 1: Code-Review+1

[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.

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.