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

Hello Dan Kenigsberg,

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

    http://gerrit.ovirt.org/25681

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/81/25681/1

diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 57fe97e..6b2441c 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -125,6 +125,10 @@
     def setUp(self):
         self.vdsm_net = VdsProxy()
 
+    def assertMtu(self, mtu, *elems):
+        for elem in elems:
+            self.assertEquals(mtu, self.vdsm_net.getMtu(elem))
+
     @cleanupNet
     @permutations([[True], [False]])
     @RequireDummyMod
@@ -215,6 +219,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
@@ -909,10 +950,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':
@@ -928,8 +965,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,
@@ -940,8 +977,8 @@
 
                 self.assertTrue(self.vdsm_net.networkExists(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)},
@@ -949,8 +986,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':
@@ -959,7 +996,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:
@@ -967,7 +1004,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 5b7c20a..b614183 100644
--- a/vdsm/netconf/__init__.py
+++ b/vdsm/netconf/__init__.py
@@ -110,6 +110,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)
@@ -118,3 +119,4 @@
                 self.configApplier.setBondingMtu(iface.name, maxMtu)
             else:
                 self.configApplier.setIfaceMtu(iface.name, maxMtu)
+        return maxMtu
diff --git a/vdsm/netconf/ifcfg.py b/vdsm/netconf/ifcfg.py
index df07d58..06bdba4 100644
--- a/vdsm/netconf/ifcfg.py
+++ b/vdsm/netconf/ifcfg.py
@@ -35,6 +35,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
 import libvirtCfg
@@ -149,9 +150,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()
@@ -166,18 +169,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/25681
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.3
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifffad45193ccd047acdefed7412bc9ce6303874a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.3
Gerrit-Owner: Antoni Segura Puimedon <asegurap(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: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

Yaniv Bronhaim has submitted this change and it was merged.

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;
Reviewed-on: http://gerrit.ovirt.org/25681
---
M tests/functional/networkTests.py
M vdsm/netconf/__init__.py
M vdsm/netconf/ifcfg.py
3 files changed, 74 insertions(+), 19 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ifffad45193ccd047acdefed7412bc9ce6303874a
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.3
Gerrit-Owner: Antoni Segura Puimedon <asegurap(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: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;

Antoni Segura Puimedon has posted comments on this change.

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


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifffad45193ccd047acdefed7412bc9ce6303874a
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.3
Gerrit-Owner: Antoni Segura Puimedon <asegurap(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: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Yaniv Bronhaim has posted comments on this change.

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


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifffad45193ccd047acdefed7412bc9ce6303874a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.3
Gerrit-Owner: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com&gt;
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

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


Patch Set 1: Code-Review+2

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

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