Change in vdsm[ovirt-3.4]: ifcfg: Do not take bond/nic down when removing one of their ...

Wednesday, 12 March 2014

Hello Dan Kenigsberg,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/25680

to review the following change.

Change subject: ifcfg: Do not take bond/nic down when removing one of their multiple nets
......................................................................

ifcfg: Do not take bond/nic down when removing one of their multiple nets

When taking removing one of the multiple networks that sit on top of
a bond/nic, it is necessary to adjust the mtu of the devices to the
new maximum required by the remaining networks that use the devices.
Example
    netA has mtu 2500 and is set over bond5.10
    netB has mtu 2000 and is set over bond5.10

    when both are set, bond5.10, bond5 and its link devices will
    have mtu 2500. When netA is removed that should be adjusted
    to 2000.

For ifcfg to apply the adjustment, it was necessary to ifdown
the nic/bond and up it again so it would read the new configuration.
The problem is that the netB of the example would see a loss of
connectivity which could affect the users. This patch addresses that
by using the ipwrapper to make the mtu adjustment and only take
down the device when no remaining networks use it.

Change-Id: Ifffad45193ccd047acdefed7412bc9ce6303874a
Bug-Url: https://bugzilla.redhat.com/1060781
Signed-off-by: Antoni S. Puimedon <asegurap(a)redhat.com&gt;
Reviewed-on: http://gerrit.ovirt.org/25003
Reviewed-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M tests/functional/networkTests.py
M vdsm/netconf/__init__.py
M vdsm/netconf/ifcfg.py
3 files changed, 73 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/80/25680/1

diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index bdfbb9f..4bc5fcf 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -232,6 +232,10 @@
                 self.vdsm_net._vlanInRunningConfig(devName, vlanId),
                 '%s found unexpectedly in running config' % vlanName)
 
+    def assertMtu(self, mtu, *elems):
+        for elem in elems:
+            self.assertEquals(mtu, self.vdsm_net.getMtu(elem))
+
     @cleanupNet
     @permutations([[True], [False]])
     @RequireDummyMod
@@ -321,6 +325,43 @@
             status, msg = self.vdsm_net.setupNetworks(
                 {},
                 {BONDING_NAME: {'remove': True}}, {'connectivityCheck':
False})
+            self.assertEqual(status, SUCCESS, msg)
+
+    @cleanupNet
+    @RequireDummyMod
+    @ValidateRunningAsRoot
+    def testSetupNetworksDelOneOfBondNets(self):
+        NETA_NAME = NETWORK_NAME + 'A'
+        NETB_NAME = NETWORK_NAME + 'B'
+        NETA_DICT = {'bonding': BONDING_NAME, 'bridged': False,
'mtu': '1600',
+                     'vlan': '4090'}
+        NETB_DICT = {'bonding': BONDING_NAME, 'bridged': False,
'mtu': '2000',
+                     'vlan': '4091'}
+        with dummyIf(2) as nics:
+            status, msg = self.vdsm_net.setupNetworks(
+                {NETA_NAME: NETA_DICT,
+                 NETB_NAME: NETB_DICT},
+                {BONDING_NAME: {'nics': nics}}, NOCHK)
+            self.assertEqual(status, SUCCESS, msg)
+            self.assertNetworkExists(NETA_NAME)
+            self.assertNetworkExists(NETB_NAME)
+            self.assertBondExists(BONDING_NAME, nics)
+            self.assertMtu(NETB_DICT['mtu'], BONDING_NAME)
+
+            with nonChangingOperstate(BONDING_NAME):
+                status, msg = self.vdsm_net.setupNetworks(
+                    {NETB_NAME: {'remove': True}}, {}, NOCHK)
+
+            self.assertEqual(status, SUCCESS, msg)
+            self.assertNetworkExists(NETA_NAME)
+            self.assertNetworkDoesntExist(NETB_NAME)
+            # Check that the mtu of the bond has been adjusted to the smaller
+            # NETA value
+            self.assertMtu(NETA_DICT['mtu'], BONDING_NAME)
+
+            status, msg = self.vdsm_net.setupNetworks(
+                {NETA_NAME: {'remove': True}},
+                {BONDING_NAME: {'remove': True}}, NOCHK)
             self.assertEqual(status, SUCCESS, msg)
 
     @cleanupNet
@@ -1005,10 +1046,6 @@
         JUMBO = '9000'
         MIDI = '4000'
 
-        def assertMtu(mtu, *elems):
-            for elem in elems:
-                self.assertEquals(mtu, self.vdsm_net.getMtu(elem))
-
         with dummyIf(3) as nics:
             with self.vdsm_net.pinger():
                 networks = {NETWORK_NAME + '1':
@@ -1024,8 +1061,8 @@
 
                 self.assertEquals(status, SUCCESS, msg)
 
-                assertMtu(MIDI, NETWORK_NAME + '2', BONDING_NAME, nics[0],
-                          nics[1])
+                self.assertMtu(MIDI, NETWORK_NAME + '2', BONDING_NAME, nics[0],
+                               nics[1])
 
                 network = {NETWORK_NAME + '3':
                            dict(bonding=BONDING_NAME, vlan='300', mtu=JUMBO,
@@ -1035,8 +1072,8 @@
                 self.assertEquals(status, SUCCESS, msg)
 
                 self.assertNetworkExists(NETWORK_NAME + '3', bridged=bridged)
-                assertMtu(JUMBO, NETWORK_NAME + '3', BONDING_NAME, nics[0],
-                          nics[1])
+                self.assertMtu(JUMBO, NETWORK_NAME + '3', BONDING_NAME,
+                               nics[0], nics[1])
 
                 status, msg = self.vdsm_net.setupNetworks({NETWORK_NAME + '3':
                                                            dict(remove=True)},
@@ -1044,8 +1081,8 @@
 
                 self.assertEquals(status, SUCCESS, msg)
 
-                assertMtu(MIDI, NETWORK_NAME + '2', BONDING_NAME, nics[0],
-                          nics[1])
+                self.assertMtu(MIDI, NETWORK_NAME + '2', BONDING_NAME, nics[0],
+                               nics[1])
 
                 # Keep last custom MTU on the interfaces
                 status, msg = self.vdsm_net.setupNetworks({NETWORK_NAME + '2':
@@ -1054,7 +1091,7 @@
 
                 self.assertEquals(status, SUCCESS, msg)
 
-                assertMtu(MIDI, BONDING_NAME, nics[0], nics[1])
+                self.assertMtu(MIDI, BONDING_NAME, nics[0], nics[1])
 
                 # Add additional nic to the bond
                 status, msg = self.vdsm_net.setupNetworks({}, {BONDING_NAME:
@@ -1062,7 +1099,7 @@
 
                 self.assertEquals(status, SUCCESS, msg)
 
-                assertMtu(MIDI, BONDING_NAME, nics[0], nics[1], nics[2])
+                self.assertMtu(MIDI, BONDING_NAME, nics[0], nics[1], nics[2])
 
                 status, msg = self.vdsm_net.setupNetworks({NETWORK_NAME + '1':
                                                            dict(remove=True)},
diff --git a/vdsm/netconf/__init__.py b/vdsm/netconf/__init__.py
index db84a9b..fe375e9 100644
--- a/vdsm/netconf/__init__.py
+++ b/vdsm/netconf/__init__.py
@@ -139,6 +139,7 @@
         :param ifaceVlans: vlan devices using the interface 'iface'
         :type ifaceVlans: iterable
 
+        :return mtu value that was applied
         """
         ifaceMtu = netinfo.getMtu(iface.name)
         maxMtu = netinfo.getMaxMtu(ifaceVlans, None)
@@ -147,6 +148,7 @@
                 self.configApplier.setBondingMtu(iface.name, maxMtu)
             else:
                 self.configApplier.setIfaceMtu(iface.name, maxMtu)
+        return maxMtu
 
 
 def getEthtoolOpts(name):
diff --git a/vdsm/netconf/ifcfg.py b/vdsm/netconf/ifcfg.py
index 37e15aa..16587d9 100644
--- a/vdsm/netconf/ifcfg.py
+++ b/vdsm/netconf/ifcfg.py
@@ -34,6 +34,7 @@
 from sourceRoute import DynamicSourceRoute
 from vdsm.config import config
 from vdsm import constants
+from vdsm import ipwrapper
 from vdsm import netinfo
 from vdsm import utils
 from vdsm.netconfpersistence import RunningConfig
@@ -169,9 +170,11 @@
     def _ifaceDownAndCleanup(self, iface, _netinfo):
         """Returns True iff the iface is to be removed."""
         DynamicSourceRoute.addInterfaceTracking(iface)
-        ifdown(iface.name)
+        to_be_removed = not _netinfo.ifaceUsers(iface.name)
+        if to_be_removed:
+            ifdown(iface.name)
         self._removeSourceRoute(iface)
-        return not _netinfo.ifaceUsers(iface.name)
+        return to_be_removed
 
     def removeBond(self, bonding):
         _netinfo = netinfo.NetInfo()
@@ -188,18 +191,30 @@
                                                  netinfo.DEFAULT_MTU)
                 ifup(bonding.name)
         else:
-            self._setNewMtu(bonding,
-                            _netinfo.getVlanDevsForIface(bonding.name))
-            ifup(bonding.name)
+            set_mtu = self._setNewMtu(
+                bonding, _netinfo.getVlanDevsForIface(bonding.name))
+            # Since we are not taking the device up again, ifcfg will not be
+            # read at this point and we need to set the live mtu value.
+            # Note that ip link set dev bondX mtu Y sets Y on all its links
+            if set_mtu is not None:
+                ipwrapper.linkSet(bonding.name, ['mtu', str(set_mtu)])
 
     def removeNic(self, nic):
         _netinfo = netinfo.NetInfo()
         to_be_removed = self._ifaceDownAndCleanup(nic, _netinfo)
         if to_be_removed:
             self.configApplier.removeNic(nic.name)
+            if nic.name in _netinfo.nics:
+                ifup(nic.name)
+            else:
+                logging.warning('host interface %s missing', nic.name)
         else:
-            self._setNewMtu(nic, _netinfo.getVlanDevsForIface(nic.name))
-        ifup(nic.name)
+            set_mtu = self._setNewMtu(nic,
+                                      _netinfo.getVlanDevsForIface(nic.name))
+            # Since we are not taking the device up again, ifcfg will not be
+            # read at this point and we need to set the live mtu value
+            if set_mtu is not None:
+                ipwrapper.linkSet(nic.name, ['mtu', str(set_mtu)])
 
     def _getFilePath(self, fileType, device):
         return os.path.join(netinfo.NET_CONF_DIR, '%s-%s' % (fileType, device))


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifffad45193ccd047acdefed7412bc9ce6303874a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.4
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011