Change in vdsm[master]: netinfo: Replace misused asserts with InvalidConfiguration

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

netinfo: Replace misused asserts with InvalidConfiguration

The code was assuming that asserts are always available, and if running
with in 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.

The asserts are replaced now with new InvalidConfiguration exception and
the error messages are improved.

Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Signed-off-by: Nir Soffer 
---
M lib/vdsm/netinfo.py
1 file changed, 17 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/61/34361/1

diff --git a/lib/vdsm/netinfo.py b/lib/vdsm/netinfo.py
index d818249..e848abc 100644
--- a/lib/vdsm/netinfo.py
+++ b/lib/vdsm/netinfo.py
@@ -84,6 +84,10 @@
 DUMMY_BRIDGE  # Appease flake8 since dummy bridge should be exported from here

+class InvalidConfiguration(Exception):
+    """ Raise when getting an invalid conifguration """
+
+
 def _visible_devs(predicate):
     """Returns a list of visible (vdsm manageable) links for which the
     predicate is True"""
@@ -834,8 +838,9 @@
         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"
+            if len(bondings) != 1:
+                raise InvalidConfiguration(
+                    "Unexpected configuration: More than one bonding per nic")
             return bondings[0]
         return None

@@ -853,13 +858,20 @@

for port in ports:
             if port in self.vlans:
-                assert vlan is None
+                if vlan is not None:
+                    raise InvalidConfiguration(
+                        "Unexpected vlan: %s exepected: None" % (vlan,))
                 nic = getVlanDevice(port)
                 vlan = getVlanID(port)
-                assert self.vlans[port]['iface'] == nic
+                if self.vlans[port]['iface'] != nic:
+                    raise InvalidConfiguration(
+                        "Unexpected iface: %s expected: %s" %
+                        (self.vlans[port]['iface'], nic))
                 port = nic
             if port in self.bondings:
-                assert bonding is None
+                if bonding is not None:
+                    raise InvalidConfiguration(
+                        "Unexpected bonding: %s expected: None" % bonding)
                 bonding = port
                 lnics += self.bondings[bonding]['slaves']
             elif port in self.nics:

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13070/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12912/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12122/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_network_functional_tests_gerrit/208... : There was an infra issue, please contact [e-mail]

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

Good catch! :-) I have a note how to improve your nice messages even more.

http://gerrit.ovirt.org/#/c/34361/1/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 859:         for port in ports:
Line 860:             if port in self.vlans:
Line 861:                 if vlan is not None:
Line 862:                     raise InvalidConfiguration(
Line 863:                         "Unexpected vlan: %s exepected: None" % (vlan,))
I recommend a comma to separate values visually :-)

"Unexpected vlan: %s, expected: None" % vlan)
Line 864:                 nic = getVlanDevice(port)
Line 865:                 vlan = getVlanID(port)
Line 866:                 if self.vlans[port]['iface'] != nic:
Line 867:                     raise InvalidConfiguration(

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/34361/1/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 859:         for port in ports:
Line 860:             if port in self.vlans:
Line 861:                 if vlan is not None:
Line 862:                     raise InvalidConfiguration(
Line 863:                         "Unexpected vlan: %s exepected: None" % (vlan,))
> I recommend a comma to separate values visually :-)

Feel free to post the next revision with this change. As a bonus, I'll let you verify this patch :-)
Line 864:                 nic = getVlanDevice(port)
Line 865:                 vlan = getVlanID(port)
Line 866:                 if self.vlans[port]['iface'] != nic:
Line 867:                     raise InvalidConfiguration(

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/34361/1/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 859:         for port in ports:
Line 860:             if port in self.vlans:
Line 861:                 if vlan is not None:
Line 862:                     raise InvalidConfiguration(
Line 863:                         "Unexpected vlan: %s exepected: None" % (vlan,))
> Feel free to post the next revision with this change. As a bonus, I'll let

I take this as an opportunity to experiment with Python's -O flag.
Line 864:                 nic = getVlanDevice(port)
Line 865:                 vlan = getVlanID(port)
Line 866:                 if self.vlans[port]['iface'] != nic:
Line 867:                     raise InvalidConfiguration(

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 2:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13187/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13029/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12239/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_network_functional_tests_gerrit/209... : There was an infra issue, please contact [e-mail]

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 2: Code-Review-1

(2 comments)

Last version contains a regression, but previous version need a a fix too.

http://gerrit.ovirt.org/#/c/34361/2/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 859:         for port in ports:
Line 860:             if port in self.vlans:
Line 861:                 if vlan is not None:
Line 862:                     raise InvalidConfiguration(
Line 863:                         "Unexpected vlan: %s, expected: None" % vlan)
This is very bad change - if vlan is a tuple or a list with one item, we will get the single item in %s. If there are multiple items, this assert will raise, hiding the original error.

>>> v = ("single value",)
    >>> "%s" % (v,)
    "('single value',)"
    >>> "%s" % v
    'single value'
    >>> mv = ('value 1', 'value 2')
    >>> "%s" % (mv,)
    "('value 1', 'value 2')"
    >>> "%s" % mv
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: not all arguments converted during string formatting

Feel free to revert this change :-)
Line 864:                 nic = getVlanDevice(port)
Line 865:                 vlan = getVlanID(port)
Line 866:                 if self.vlans[port]['iface'] != nic:
Line 867:                     raise InvalidConfiguration(

Line 870:                 port = nic
Line 871:             if port in self.bondings:
Line 872:                 if bonding is not None:
Line 873:                     raise InvalidConfiguration(
Line 874:                         "Unexpected bonding: %s, expected: None" % bonding)
Now I see that I used the same bad format - bonding must be (bonding,)
Line 875:                 bonding = port
Line 876:                 lnics += self.bondings[bonding]['slaves']
Line 877:             elif port in self.nics:
Line 878:                 lnics.append(port)

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

Replace % with str.format()?

http://gerrit.ovirt.org/#/c/34361/2/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 859:         for port in ports:
Line 860:             if port in self.vlans:
Line 861:                 if vlan is not None:
Line 862:                     raise InvalidConfiguration(
Line 863:                         "Unexpected vlan: %s, expected: None" % vlan)
> This is very bad change - if vlan is a tuple or a list with one item, we wi

What do you think about adopting the str.format function, which has dealt with the unfortunate ambiguity of the (deprecated) % operator, treating a scalar the same way as a single-item iterable?
> > > v = 'single value'
> > > '{}'.format(v)

'single value'
> > > v = ('single value',)
> > > '{}'.format(v)

"('single value',)"
> > > mv = ('value 1', 'value 2')
> > > '{}'.format(mv)

"('value 1', 'value 2')"
Line 864:                 nic = getVlanDevice(port)
Line 865:                 vlan = getVlanID(port)
Line 866:                 if self.vlans[port]['iface'] != nic:
Line 867:                     raise InvalidConfiguration(

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/34361/2/lib/vdsm/netinfo.py
File lib/vdsm/netinfo.py:

Line 859:         for port in ports:
Line 860:             if port in self.vlans:
Line 861:                 if vlan is not None:
Line 862:                     raise InvalidConfiguration(
Line 863:                         "Unexpected vlan: %s, expected: None" % vlan)
> What do you think about adopting the str.format function, which has dealt w

Aaand of course, EL6's Python 2.6 needs '{0}' in the format string.
Line 864:                 nic = getVlanDevice(port)
Line 865:                 vlan = getVlanID(port)
Line 866:                 if self.vlans[port]['iface'] != nic:
Line 867:                     raise InvalidConfiguration(

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13198/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13040/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12250/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_network_functional_tests_gerrit/209... : There was an infra issue, please contact [e-mail]

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3: Code-Review+1

Nir's original intent is now fulfilled :-) Please see comments on previous revisions for the rationale to use str.format().

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13198/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13040/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12250/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_network_functional_tests_gerrit/209... : There was an infra issue, please contact [e-mail]

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

Thanks Ondřej!

Do you know how to trigger these failures for verification?

Ido Barkan has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3: Code-Review+1

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3: Code-Review-1

Nir, guys,

I don't think we need to correct this old code. Instead, we shall replace it when the time comes and not care about the checks here, they have become redundant.

getNicsVlanAndBondingForNetwork is only used in showNetwork and delNetwork to extract information from netinfo.NetInfo, in not a trivial way I might add.

getBondingForNic appears in objectivizeNetwork and Bond._objectivizeSlaves.

The bottom line here is that if something was broken in network configuration we would have already caught that when creating past networks.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

Ondřej, if you are sure that we cannot reach these asserts, lets remove them.

If they are possible, I think this fix is correct.

We should not have asserts in production code, it creates hidden untested code path when the assert are disabled.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

ping?

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

can network developers reach some consensus about this?

Ido Barkan has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

Ondrej, you are probably right, but I see no reason why not fix this now. If you think we should remove this code, please send a following patch.

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

In getBondingForNic, the assert can be removed.

# 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

getNicsVlanAndBondingForNetwork is too hard to read (because of its complex return value) so I'd prefer to rewrite the function than to add checks to it.

Ido Barkan has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

Gladly! But begin with writing the tests please ☺

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

The patch can now be abandoned in favour of https://gerrit.ovirt.org/45072 and https://gerrit.ovirt.org/45095, both of which remove the asserts and try to make the affected functions more understandable.

[Nir Soffer]

Nir Soffer has abandoned this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Abandoned

Replaced by  https://gerrit.ovirt.org/45072 and https://gerrit.ovirt.org/45095

[e-mail] has posted comments on this change.

Change subject: netinfo: Replace misused asserts with InvalidConfiguration
......................................................................

Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found