From 238fb9542c4365ae13f6b4f0c2cd6ffcd427e561 Mon Sep 17 00:00:00 2001

From: Gris Ge <fge@redhat.com>

Date: Wed, 1 Apr 2020 20:26:40 +0800

Subject: [PATCH] bond: Don't merge bond options when bond mode changed

When changing bond mode, user is required to provide all bond options

and no bond options will be merged from current state.

When changing bond mode, full deactivation is required in NetworkManager

do to bug https://bugzilla.redhat.com/show_bug.cgi?id=1819137

Procedure to fix this issue:

    * Generate a metadata if bond mode changed.

    * After merge, use desire bond option only if bond mode changed.

    * In NM profile modification, deactivate the interface before

      modification if bond mode changed.

Added integration test case and unit test cases.

Signed-off-by: Gris Ge <fge@redhat.com>

---

 libnmstate/appliers/bond.py | 62 +++++++++++++++++++++++++++++++++++++

 libnmstate/metadata.py      |  2 ++

 libnmstate/nm/applier.py    | 18 +++++++++++

 libnmstate/state.py         |  9 ++++++

 4 files changed, 91 insertions(+)

diff --git a/libnmstate/appliers/bond.py b/libnmstate/appliers/bond.py

index 5602adb..2a3f0a1 100644

--- a/libnmstate/appliers/bond.py

+++ b/libnmstate/appliers/bond.py

@@ -18,9 +18,16 @@

 #

 import contextlib

+import logging

 from libnmstate.schema import Bond

 from libnmstate.schema import BondMode

+from libnmstate.schema import Interface

+from libnmstate.schema import InterfaceState

+from libnmstate.schema import InterfaceType

+

+BOND_MODE_CHANGED_METADATA = "_bond_mode_changed"

+NON_UP_STATES = (InterfaceState.DOWN, InterfaceState.ABSENT)

 class BondNamedOptions:

@@ -126,3 +133,58 @@ def fix_bond_option_arp_monitor(cur_iface_state):

         and "arp_ip_target" not in bond_options

     ):

         bond_options["arp_ip_target"] = ""

+

+

+def generate_bond_mode_change_metadata(desire_state, current_state):

+    for iface_name, iface_state in desire_state.interfaces.items():

+        current_iface_state = current_state.interfaces.get(iface_name, {})

+        if (

+            iface_state.get(

+                Interface.TYPE, current_iface_state.get(Interface.TYPE)

+            )

+            != InterfaceType.BOND

+        ):

+            continue

+        if iface_state.get(Interface.STATE) in NON_UP_STATES:

+            # Ignore bond mode change on absent/down interface

+            continue

+        current_bond_mode = current_iface_state.get(

+            Bond.CONFIG_SUBTREE, {}

+        ).get(Bond.MODE)

+        desire_bond_mode = iface_state.get(Bond.CONFIG_SUBTREE, {}).get(

+            Bond.MODE

+        )

+        if (

+            desire_bond_mode

+            and current_bond_mode

+            and desire_bond_mode != current_bond_mode

+        ):

+            logging.warning(

+                "Discarding all current bond options as interface "

+                f"{iface_name} has bond mode changed"

+            )

+            iface_state[BOND_MODE_CHANGED_METADATA] = True

+

+

+def remove_bond_mode_change_metadata(iface_state):

+    iface_state.pop(BOND_MODE_CHANGED_METADATA, None)

+

+

+def is_bond_mode_changed(iface_state):

+    return iface_state.get(BOND_MODE_CHANGED_METADATA)

+

+

+def discard_merged_data_on_mode_change(merged_iface_state, desire_iface_state):

+    """

+    When bond mode changed, use original desire bond options instead of merging

+    from current state.

+    """

+    if is_bond_mode_changed(merged_iface_state):

+        if merged_iface_state.get(Bond.CONFIG_SUBTREE, {}).get(

+            Bond.OPTIONS_SUBTREE

+        ):

+            merged_iface_state[Bond.CONFIG_SUBTREE][

+                Bond.OPTIONS_SUBTREE

+            ] = desire_iface_state[Bond.CONFIG_SUBTREE].get(

+                Bond.OPTIONS_SUBTREE, {}

+            )

diff --git a/libnmstate/metadata.py b/libnmstate/metadata.py

index b268ba3..bdc8b3c 100644

--- a/libnmstate/metadata.py

+++ b/libnmstate/metadata.py

@@ -53,6 +53,7 @@ def generate_ifaces_metadata(desired_state, current_state):

     metadata is generated on interfaces, usable by the provider when

     configuring the interface.

     """

+    bond.generate_bond_mode_change_metadata(desired_state, current_state)

     _generate_link_master_metadata(

         desired_state.interfaces,

         current_state.interfaces,

@@ -91,6 +92,7 @@ def remove_ifaces_metadata(ifaces_state):

         iface_state.pop(MASTER, None)

         iface_state.pop(MASTER_TYPE, None)

         iface_state.pop(BRPORT_OPTIONS, None)

+        bond.remove_bond_mode_change_metadata(iface_state)

         iface_state.get(Interface.IPV4, {}).pop(ROUTES, None)

         iface_state.get(Interface.IPV6, {}).pop(ROUTES, None)

         iface_state.get(Interface.IPV4, {}).pop(DNS_METADATA, None)

diff --git a/libnmstate/nm/applier.py b/libnmstate/nm/applier.py

index 1291469..1ca0647 100644

--- a/libnmstate/nm/applier.py

+++ b/libnmstate/nm/applier.py

@@ -18,6 +18,7 @@

 #

 import base64

+import logging

 import hashlib

 import itertools

 from operator import attrgetter

@@ -28,6 +29,7 @@ from libnmstate.schema import InterfaceState

 from libnmstate.schema import InterfaceType

 from libnmstate.schema import LinuxBridge as LB

 from libnmstate.schema import OVSBridge as OvsB

+from libnmstate.appliers.bond import is_bond_mode_changed

 from . import bond

 from . import bridge

@@ -169,6 +171,7 @@ def set_ifaces_admin_state(ifaces_desired_state, con_profiles=()):

     master_ifaces_to_edit = set()

     ifaces_to_edit = set()

     remove_devs_actions = {}

+    devs_to_deactivate_beforehand = []

     for iface_desired_state in ifaces_desired_state:

         ifname = iface_desired_state[Interface.NAME]

@@ -199,6 +202,18 @@ def set_ifaces_admin_state(ifaces_desired_state, con_profiles=()):

                     new_ifaces_to_activate.add(ifname)

         else:

             if iface_desired_state[Interface.STATE] == InterfaceState.UP:

+                if is_bond_mode_changed(iface_desired_state):

+                    # NetworkManager leaves leftover in sysfs for bond

+                    # options when changing bond mode, bug:

+                    # https://bugzilla.redhat.com/show_bug.cgi?id=1819137

+                    # Workaround: delete the bond interface from kernel and

+                    # create again via full deactivation beforehand.

+                    logging.debug(

+                        f"Bond interface {ifname} is changing bond mode, "

+                        "will do full deactivation before applying changes"

+                    )

+                    devs_to_deactivate_beforehand.append(nmdev)

+

                 if _is_master_iface(iface_desired_state):

                     master_ifaces_to_edit.add(

                         (nmdev, con_profiles_by_devname[ifname].profile)

@@ -230,6 +245,9 @@ def set_ifaces_admin_state(ifaces_desired_state, con_profiles=()):

                     )

                 )

+    for dev in devs_to_deactivate_beforehand:

+        device.deactivate(dev)

+

     # Do not remove devices that are marked for editing.

     for dev, _ in itertools.chain(master_ifaces_to_edit, ifaces_to_edit):

         remove_devs_actions.pop(dev, None)

diff --git a/libnmstate/state.py b/libnmstate/state.py

index 98be33d..12bc414 100644

--- a/libnmstate/state.py

+++ b/libnmstate/state.py

@@ -390,10 +390,19 @@ class State:

         entries that appear only on one state are ignored.

         This is a reverse recursive update operation.

         """

+        origin_self_state = State(self.state)

         other_state = State(other_state.state)

         for name in self.interfaces.keys() & other_state.interfaces.keys():

             dict_update(other_state.interfaces[name], self.interfaces[name])

             self._ifaces_state[name] = other_state.interfaces[name]

+            iface_state = self.interfaces[name]

+            if iface_state.get(Interface.TYPE) == InterfaceType.BOND:

+                origin_self_iface_state = origin_self_state.interfaces.get(

+                    name

+                )

+                bond.discard_merged_data_on_mode_change(

+                    iface_state, origin_self_iface_state

+                )

     def complement_master_interfaces_removal(self, other_state):

         """

-- 

2.18.2