Change in vdsm[master]: setupNetworks: Fix incorrect mtu overriding when adding mult...

Antoni Segura Puimedon has uploaded a new change for review.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

setupNetworks: Fix incorrect mtu overriding when adding multiple networks

When configuring several networks over a bond, if the iteration over
the networks to be added made networks with lower MTUs to be added
after those with higher ones, the last mtu, regardless of value would
be set.

This was because the _netinfo object is passed from addNetworks to
addNetworks and the objectivize of the succeeding addNetworks would
not see the higher MTUs set by the preceding addNetworks.

Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1072411
Signed-off-by: Antoni S. Puimedon 
---
M vdsm/configNetwork.py
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/43/25343/1

diff --git a/vdsm/configNetwork.py b/vdsm/configNetwork.py
index ea875f9..2b6162b 100755
--- a/vdsm/configNetwork.py
+++ b/vdsm/configNetwork.py
@@ -644,6 +644,7 @@
             logger.debug("Adding network %r" % network)
             addNetwork(network, configurator=configurator,
                        implicitBonding=True, _netinfo=_netinfo, **d)
+            _netinfo.updateDevices()  # Things like a bond mtu can change

if utils.tobool(options.get('connectivityCheck', True)):
             logger.debug('Checking connectivity...')

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 1:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6616/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7518/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/25343/1//COMMIT_MSG
Commit Message:

Line 12: be set.
Line 13: 
Line 14: This was because the _netinfo object is passed from addNetworks to
Line 15: addNetworks and the objectivize of the succeeding addNetworks would
Line 16: not see the higher MTUs set by the preceding addNetworks.
Was this introduced by the recent scaling work? Can you think of a functional test that could have revealed it earlier? (I'd expect testSetupNetworksMtus, but it missed the bug)
Line 17: 
Line 18: Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Line 19: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1072411

Antoni Segura Puimedon has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/25343/1//COMMIT_MSG
Commit Message:

Line 12: be set.
Line 13: 
Line 14: This was because the _netinfo object is passed from addNetworks to
Line 15: addNetworks and the objectivize of the succeeding addNetworks would
Line 16: not see the higher MTUs set by the preceding addNetworks.
> Was this introduced by the recent scaling work? Can you think of a function

It was. This is entirely due to re-using _netinfo instances introduced in scale.

I tried to think about a way to reproduce it in a test. The problem is to have a setupNetworks command that processes first the higher and then the lower. Maybe passing an ordereddict would do it. I'll try.
Line 17: 
Line 18: Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Line 19: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1072411

Antoni Segura Puimedon has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 1:

we use a set for the networks to add, so that makes testing this by functional testing quite shaky.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/25343/1//COMMIT_MSG
Commit Message:

Line 12: be set.
Line 13: 
Line 14: This was because the _netinfo object is passed from addNetworks to
Line 15: addNetworks and the objectivize of the succeeding addNetworks would
Line 16: not see the higher MTUs set by the preceding addNetworks.
> It was. This is entirely due to re-using _netinfo instances introduced in s

Please mention the offending commit in the message.

I hate tests that depend on the "randomness" of hash() , but if you pile up enough networks with different MTUs, you have to be exponentially unlucky to get the least MTU treated last.
(but ordereddict is cooler)
Line 17: 
Line 18: Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Line 19: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1072411

Antoni Segura Puimedon has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 2:

I'll do a manual verification by stopping the the setupNetworks between network addition and tweaking the bond configuration so it triggers a reconfigure (using the mtu test I just added).

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6694/ : SUCCESS

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7595/ : SUCCESS

Antoni Segura Puimedon has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 2: Verified+1

Verified with help of the debugger.

When running the test added in this patch, i.e., testLowerMtuDoesNotOverride
with the debugger, previous to this patch, if a network with lower mtu is
configured after one with higher mtu and the bond is deemed to be in need of
reconfiguring (which I simulate for verification by executing 'ip link set dev
bond0 down') the Bond entity has the lower mtu and the override happens.

After this patch, each addNetwork is run with updated netinfo device
information. As a result, objectivizeNetwork will have the bridge and the vlan
entities with the lower mtu, but the bond will use the maximum mtu between
the current and the one to configure, avoiding the bug. (This was tested as
well with the 'ip link set dev bond0 down')

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/25343/2/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 253:                 '%s found unexpectedly in running config' % vlanName)
Line 254: 
Line 255:     def assertMtu(self, mtu, *elems):
Line 256:         for elem in elems:
Line 257:             self.assertEquals(int(mtu), int(self.vdsm_net.getMtu(elem)))
coercing the test-based mtu into int is dependable,  but Vdsm returns a non-int MTU, we'd better explode.
Line 258: 
Line 259:     @cleanupNet
Line 260:     @permutations([[True], [False]])
Line 261:     @RequireDummyMod

Antoni Segura Puimedon has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/25343/2/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 253:                 '%s found unexpectedly in running config' % vlanName)
Line 254: 
Line 255:     def assertMtu(self, mtu, *elems):
Line 256:         for elem in elems:
Line 257:             self.assertEquals(int(mtu), int(self.vdsm_net.getMtu(elem)))
> coercing the test-based mtu into int is dependable,  but Vdsm returns a non

vdsm returns always a non-int MTU for backwards compatibility reasons. The api schema says it should be uint (and it should!) but the older engines expected strings so we left it as it was.
Line 258: 
Line 259:     @cleanupNet
Line 260:     @permutations([[True], [False]])
Line 261:     @RequireDummyMod

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.ovirt.org/#/c/25343/2/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 253:                 '%s found unexpectedly in running config' % vlanName)
Line 254: 
Line 255:     def assertMtu(self, mtu, *elems):
Line 256:         for elem in elems:
Line 257:             self.assertEquals(int(mtu), int(self.vdsm_net.getMtu(elem)))
> vdsm returns always a non-int MTU for backwards compatibility reasons. The

darn.
Line 258: 
Line 259:     @cleanupNet
Line 260:     @permutations([[True], [False]])
Line 261:     @RequireDummyMod

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: setupNetworks: Fix incorrect mtu overriding when adding multiple networks
......................................................................

setupNetworks: Fix incorrect mtu overriding when adding multiple networks

When configuring several networks over a bond, if the iteration over
the networks to be added made networks with lower MTUs to be added
after those with higher ones, the last mtu, regardless of value would
be set.

This was because the _netinfo object is passed from addNetworks to
addNetworks and the objectivize of the succeeding addNetworks would
not see the higher MTUs set by the preceding addNetworks. This would
only affect the bond MTU when some bond setting would be detected
as changed and it would trigger bond reconfiguring (for a list of
what is checked for changes, see netmodels.py:Bond.configure).

Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1072411
Signed-off-by: Antoni S. Puimedon 
Reviewed-on: http://gerrit.ovirt.org/25343
Reviewed-by: Dan Kenigsberg 
---
M tests/functional/networkTests.py
M vdsm/configNetwork.py
2 files changed, 39 insertions(+), 1 deletion(-)

Approvals:
  Antoni Segura Puimedon: Verified
  Dan Kenigsberg: Looks good to me, approved