10:19 a.m.
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 <asegurap(a)redhat.com>
---
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...')
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
8:35 a.m.
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 <asegurap(a)redhat.com>
Reviewed-on: http://gerrit.ovirt.org/25343
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com>
---
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
7:05 a.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
6:38 p.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
5:46 p.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
3:16 a.m.
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')
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
10:25 a.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
10:20 a.m.
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).
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
3:38 a.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
2:50 a.m.
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.
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
2:42 a.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
6:27 p.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
10:28 a.m.
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
--
To view, visit http://gerrit.ovirt.org/25343
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia375c2e0e4a1896ab99d734e3203a5ef49570f36
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
3750
days inactive
3767
days old
vdsm-patches@lists.fedorahosted.org
12 comments
3 participants
participants (3)
-
asegurap＠redhat.com -
Dan Kenigsberg -
Jenkins CI RO