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 <nsoffer(a)redhat.com&gt;
---
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:


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;

[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/... : There was
an infra issue, please contact infra(a)ovirt.org

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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(


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

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(


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[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/... : There was
an infra issue, please contact infra(a)ovirt.org

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

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(


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

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(


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[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/... : There was
an infra issue, please contact infra(a)ovirt.org

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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().

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[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/... : There was
an infra issue, please contact infra(a)ovirt.org

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Ido Barkan has posted comments on this change.

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


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

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


Patch Set 3:

ping?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni(a)midokura.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: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt(a)midokura.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

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.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt(a)midokura.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(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: 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.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt(a)midokura.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

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 ☺

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt(a)midokura.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(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: 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.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt(a)midokura.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[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

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt(a)midokura.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org

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

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


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99ee49aa7e2364f57112e2452e3eab3940b6b00a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt(a)midokura.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Ido Barkan <ibarkan(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No