[[PATCH] Fix TeamDevice configuration] Fixed issues with team device configuration. Team devices now are created with a config dict that gets converted to json when configuring them on the host.

From: Perry Gagne <pgagne(a)redhat.com&gt;

Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
---
 lnst/Devices/TeamDevice.py      | 49 ++++++++++++++++-----------------
 lnst/Recipes/ENRT/TeamRecipe.py | 11 ++++----
 2 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/lnst/Devices/TeamDevice.py b/lnst/Devices/TeamDevice.py
index 4f6eee5..3a60125 100644
--- a/lnst/Devices/TeamDevice.py
+++ b/lnst/Devices/TeamDevice.py
@@ -9,44 +9,43 @@ published by the Free Software Foundation; see COPYING for details.
 __author__ = """
 olichtne(a)redhat.com (Ondrej Lichtner)
 """
-
-import re
+import json
 from lnst.Common.ExecCmd import exec_cmd
-from lnst.Common.Utils import bool_it
+from lnst.Common.DeviceError import DeviceError, DeviceConfigError
 from lnst.Devices.MasterDevice import MasterDevice
 
-def prepare_json_str(json_str):
-    if not json_str:
-        return "{}"
-    json_str = json_str.replace('"', '\\"')
-    json_str = re.sub('\s+', ' ', json_str)
-    return json_str
 
-#TODO rework with pyroute2? don't know how json and teamd works with that...
 class TeamDevice(MasterDevice):
     _name_template = "t_team"
 
     def __init__(self, ifmanager, *args, **kwargs):
+        self._config = {}
+        self._dbus = False
         super(TeamDevice, self).__init__(ifmanager, *args, **kwargs)
 
-        self._config = kwargs.get("config", None)
-        self._dbus = not bool_it(kwargs.get("disable_dbus", False))
-
     @property
     def config(self):
         return self._config
 
+    @config.setter
+    def config(self, v: dict):
+        if not isinstance(v, dict):
+            raise DeviceConfigError("team device config must be dict")
+        self._config = v
+
     @property
     def dbus(self):
         return self._dbus
 
-    def _create(self):
-        teamd_config = prepare_json_str(self.config)
+    @dbus.setter
+    def dbus(self, v: bool):
+        if not isinstance(v, bool):
+            raise DeviceConfigError("team dbus setting must be bool")
+        self._dbus = v
 
-        exec_cmd("teamd -r -d -c \"%s\" -t %s %s" %\
-                    (teamd_config,
-                     self.name,
-                     " -D" if self.dbus else ""))
+    def _create(self):
+        teamd_json = json.dumps(self.config)
+        exec_cmd(f"teamd -r -d -c '{teamd_json}' -t {self.name}
{'-D' if self.dbus else ''}")
 
         retry = 0
         while self._nl_msg is None and retry < 5:
@@ -56,11 +55,11 @@ class TeamDevice(MasterDevice):
     def destroy(self):
         exec_cmd("teamd -k -t %s" % self.name)
 
-    def slave_add(self, dev, port_config=None):
-        exec_cmd("teamdctl %s %s port config update %s \"%s\"" %\
-                    (" -D" if self.dbus else "",
-                     self.name,
-                     dev.name,
-                     prepare_json_str(port_config)))
+    def slave_add(self, dev, port_config={}):
+        if not isinstance(port_config, dict):
+            raise DeviceConfigError(f"team link {dev.name} port config must be
dict")
+
+        port_json = json.dumps(port_config)
+        exec_cmd(f"teamdctl {'-D' if self.dbus else ''} {self.name}
port config update {dev.name} '{port_json}'")
 
         dev.master = self
diff --git a/lnst/Recipes/ENRT/TeamRecipe.py b/lnst/Recipes/ENRT/TeamRecipe.py
index e083b90..ccad5e7 100644
--- a/lnst/Recipes/ENRT/TeamRecipe.py
+++ b/lnst/Recipes/ENRT/TeamRecipe.py
@@ -9,8 +9,9 @@ from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
 from lnst.RecipeCommon.Ping.PingEndpoints import PingEndpoints
 from lnst.Devices import TeamDevice
 
+
 class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
-    BaseEnrtRecipe):
+                 BaseEnrtRecipe):
     host1 = HostReq()
     host1.eth0 = DeviceReq(label="tnet",
driver=RecipeParam("driver"))
     host1.eth1 = DeviceReq(label="tnet",
driver=RecipeParam("driver"))
@@ -30,10 +31,8 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
     def test_wide_configuration(self):
         host1, host2 = self.matched.host1, self.matched.host2
 
-        #The config argument needs to be used with a team device normally
-        #(e.g  to specify the runner mode), but it is not used here due to
-        #a bug in the TeamDevice module
-        host1.team0 = TeamDevice()
+        teamd_config = {'runner': {'name': self.params.runner_name}}
+        host1.team0 = TeamDevice(config=teamd_config)
 
         configuration = super().test_wide_configuration()
         configuration.test_wide_devices = [host1.team0, host2.eth0]
@@ -90,7 +89,7 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
 
     def generate_perf_endpoints(self, config):
         return [(self.matched.host1.team0, self.matched.host2.eth0),
-            (self.matched.host2.eth0, self.matched.host1.team0)]
+                (self.matched.host2.eth0, self.matched.host1.team0)]
 
     @property
     def offload_nics(self):
-- 
2.21.1

[Jan Tluka]

A generic comment on the patch email format. The subject seems
corrupted, it should be of the following format:
[PATCH] ...

Also the subject contains quite a long string, I believe the last part of it
should have been part of the patch description rather than subject.

Last thing is that you put 'From: ...' into patch description, which
should not be there.

More comments on the patch code changes inline.

Tue, Apr 14, 2020 at 08:40:37PM CEST, pgagne(a)redhat.com wrote:
> From: Perry Gagne <pgagne(a)redhat.com&gt;
>
> Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
> ---
>  lnst/Devices/TeamDevice.py      | 49 ++++++++++++++++-----------------
>  lnst/Recipes/ENRT/TeamRecipe.py | 11 ++++----
>  2 files changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/lnst/Devices/TeamDevice.py b/lnst/Devices/TeamDevice.py
> index 4f6eee5..3a60125 100644
> --- a/lnst/Devices/TeamDevice.py
> +++ b/lnst/Devices/TeamDevice.py
> @@ -9,44 +9,43 @@ published by the Free Software Foundation; see COPYING for details.
>  __author__ = """
>  olichtne(a)redhat.com (Ondrej Lichtner)
>  """
> -
> -import re
> +import json
>  from lnst.Common.ExecCmd import exec_cmd
> -from lnst.Common.Utils import bool_it
> +from lnst.Common.DeviceError import DeviceError, DeviceConfigError
>  from lnst.Devices.MasterDevice import MasterDevice
>  
> -def prepare_json_str(json_str):
> -    if not json_str:
> -        return "{}"
> -    json_str = json_str.replace('"', '\\"')
> -    json_str = re.sub('\s+', ' ', json_str)
> -    return json_str
>  
> -#TODO rework with pyroute2? don't know how json and teamd works with that... 
Please do not remove the TODO unless you resolved it or you're sure this
is not doable. Maybe you already discussed this with Ondrej, in that
case ignore my comment.

>  class TeamDevice(MasterDevice):
>      _name_template = "t_team"
>  
>      def __init__(self, ifmanager, *args, **kwargs):
> +        self._config = {}
> +        self._dbus = False
>          super(TeamDevice, self).__init__(ifmanager, *args, **kwargs)
>  
> -        self._config = kwargs.get("config", None)
> -        self._dbus = not bool_it(kwargs.get("disable_dbus", False))
> -
>      @property
>      def config(self):
>          return self._config
>  
> +    @config.setter
> +    def config(self, v: dict):
> +        if not isinstance(v, dict):
> +            raise DeviceConfigError("team device config must be dict")
> +        self._config = v
> +
>      @property
>      def dbus(self):
>          return self._dbus
>  
> -    def _create(self):
> -        teamd_config = prepare_json_str(self.config)
> +    @dbus.setter
> +    def dbus(self, v: bool):
> +        if not isinstance(v, bool):
> +            raise DeviceConfigError("team dbus setting must be bool")
> +        self._dbus = v
>  
> -        exec_cmd("teamd -r -d -c \"%s\" -t %s %s" %\
> -                    (teamd_config,
> -                     self.name,
> -                     " -D" if self.dbus else ""))
> +    def _create(self):
> +        teamd_json = json.dumps(self.config)
> +        exec_cmd(f"teamd -r -d -c '{teamd_json}' -t {self.name}
> {'-D' if self.dbus else ''}")
>  
>          retry = 0
>          while self._nl_msg is None and retry < 5:
> @@ -56,11 +55,11 @@ class TeamDevice(MasterDevice):
>      def destroy(self):
>          exec_cmd("teamd -k -t %s" % self.name)
>  
> -    def slave_add(self, dev, port_config=None):
> -        exec_cmd("teamdctl %s %s port config update %s \"%s\"" %\
> -                    (" -D" if self.dbus else "",
> -                     self.name,
> -                     dev.name,
> -                     prepare_json_str(port_config)))
> +    def slave_add(self, dev, port_config={}):
> +        if not isinstance(port_config, dict):
> +            raise DeviceConfigError(f"team link {dev.name} port config must be
> dict")
> +
> +        port_json = json.dumps(port_config)
> +        exec_cmd(f"teamdctl {'-D' if self.dbus else ''} {self.name}
> port config update {dev.name} '{port_json}'")
>  
>          dev.master = self
> diff --git a/lnst/Recipes/ENRT/TeamRecipe.py b/lnst/Recipes/ENRT/TeamRecipe.py
> index e083b90..ccad5e7 100644
> --- a/lnst/Recipes/ENRT/TeamRecipe.py
> +++ b/lnst/Recipes/ENRT/TeamRecipe.py
> @@ -9,8 +9,9 @@ from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
>  from lnst.RecipeCommon.Ping.PingEndpoints import PingEndpoints
>  from lnst.Devices import TeamDevice
>  
> +
>  class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
> -    BaseEnrtRecipe):
> +                 BaseEnrtRecipe): 
You've added another newline before the class definition, likely
unnecessary.

Also, you (or your IDE) reindented the list of derived from classes.
Again, unnecessary change.

>      host1 = HostReq()
>      host1.eth0 = DeviceReq(label="tnet",
> driver=RecipeParam("driver"))
>      host1.eth1 = DeviceReq(label="tnet",
> driver=RecipeParam("driver"))
> @@ -30,10 +31,8 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
>      def test_wide_configuration(self):
>          host1, host2 = self.matched.host1, self.matched.host2
>  
> -        #The config argument needs to be used with a team device normally
> -        #(e.g  to specify the runner mode), but it is not used here due to
> -        #a bug in the TeamDevice module
> -        host1.team0 = TeamDevice()
> +        teamd_config = {'runner': {'name': self.params.runner_name}}
> +        host1.team0 = TeamDevice(config=teamd_config)
>  
>          configuration = super().test_wide_configuration()
>          configuration.test_wide_devices = [host1.team0, host2.eth0]
> @@ -90,7 +89,7 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
>  
>      def generate_perf_endpoints(self, config):
>          return [(self.matched.host1.team0, self.matched.host2.eth0),
> -            (self.matched.host2.eth0, self.matched.host1.team0)]
> +                (self.matched.host2.eth0, self.matched.host1.team0)] 
Once again, an unnecessary indentation change.

>  
>      @property
>      def offload_nics(self):
> -- 
> 2.21.1
> _______________________________________________
> LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...

[Jan Tluka]

Wed, Apr 15, 2020 at 10:34:15AM CEST, jtluka(a)redhat.com wrote:
> Tue, Apr 14, 2020 at 08:40:37PM CEST, pgagne(a)redhat.com wrote:
> >From: Perry Gagne <pgagne(a)redhat.com&gt;
> >
> >Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
> >---
> > lnst/Devices/TeamDevice.py      | 49 ++++++++++++++++-----------------
> > lnst/Recipes/ENRT/TeamRecipe.py | 11 ++++----
> > 2 files changed, 29 insertions(+), 31 deletions(-)
> >
> >diff --git a/lnst/Devices/TeamDevice.py b/lnst/Devices/TeamDevice.py
> >index 4f6eee5..3a60125 100644
> >--- a/lnst/Devices/TeamDevice.py
> >+++ b/lnst/Devices/TeamDevice.py
> >@@ -9,44 +9,43 @@ published by the Free Software Foundation; see COPYING for details.
> > __author__ = """
> > olichtne(a)redhat.com (Ondrej Lichtner)
> > """
> >-
> >-import re
> >+import json
> > from lnst.Common.ExecCmd import exec_cmd
> >-from lnst.Common.Utils import bool_it
> >+from lnst.Common.DeviceError import DeviceError, DeviceConfigError
> > from lnst.Devices.MasterDevice import MasterDevice
> > 
> >-def prepare_json_str(json_str):
> >-    if not json_str:
> >-        return "{}"
> >-    json_str = json_str.replace('"', '\\"')
> >-    json_str = re.sub('\s+', ' ', json_str)
> >-    return json_str
> > 
> >-#TODO rework with pyroute2? don't know how json and teamd works with that...
>
> Please do not remove the TODO unless you resolved it or you're sure this
> is not doable. Maybe you already discussed this with Ondrej, in that
> case ignore my comment. 
So I looked at the pyroute2 code and the team driver is supported.
The problem seems to be that IPRoute does not support passing the teamd
config yet and the active-backup runner is used as default. Might be
worth to check with pyroute maintainer.

<cut> <!-- netlink/rtnl/ifinfmsg/proxy.py -->
def manage_team(msg):

    if msg['header']['type'] != RTM_NEWLINK:
        raise ValueError('wrong command type')

    config = {'device': msg.get_attr('IFLA_IFNAME'),
              'runner': {'name': 'activebackup'},
              'link_watch': {'name': 'ethtool'}}

    with open(os.devnull, 'w') as fnull:
        subprocess.check_call(['teamd', '-d', '-n', '-c',
json.dumps(config)],
                              stdout=fnull,
                              stderr=fnull)
</cut>

[Jan Tluka]

Thu, Apr 16, 2020 at 09:48:37AM CEST, jtluka(a)redhat.com wrote:
> So I looked at the pyroute2 code and the team driver is supported.
> The problem seems to be that IPRoute does not support passing the teamd
> config yet and the active-backup runner is used as default. Might be
> worth to check with pyroute maintainer.
>  
I created a github issue for this:
https://github.com/svinota/pyroute2/issues/699

-Jan

[Ondrej Lichtner]

On Thu, Apr 16, 2020 at 09:58:51AM +0200, Jan Tluka wrote:
>  Thu, Apr 16, 2020 at 09:48:37AM CEST, jtluka(a)redhat.com wrote:
>  >
>  >So I looked at the pyroute2 code and the team driver is supported.
>  >The problem seems to be that IPRoute does not support passing the teamd
>  >config yet and the active-backup runner is used as default. Might be
>  >worth to check with pyroute maintainer.
>  >
>  
>  I created a github issue for this:
>  https://github.com/svinota/pyroute2/issues/699
>  
>  -Jan 
Thanks, in the meantime I would just leave the TODO in there, eventually
I'd like to have the ability for the TeamDevice class _getters_ to
return the real value of the configuration of the device - in case a
different process changed something for example.

Currently the getters just return the values stored privately in the
class instance e.g.:

    @property
    def config(self):
        return self._config

If something changes the config this will still return just the value
that was used to create the device.

When we're able to retrieve the updated config value I'd also like to be
able to access them directly not manually through a dict object, e.g.:

NO:

    device_mode = team_device.config["mode"]

YES:

    device_mode = team_device.mode


And maybe something similar with the setters as well:

    team_device.mode = new_mode


I don't know if this fine grained control is easy/hard/impossible, but
that's kind of the larger point behind the TODO note here.

-Ondrej

[Jan Tluka]

Thu, Apr 16, 2020 at 09:58:51AM CEST, jtluka(a)redhat.com wrote:
> Thu, Apr 16, 2020 at 09:48:37AM CEST, jtluka(a)redhat.com wrote:
> >
> >So I looked at the pyroute2 code and the team driver is supported.
> >The problem seems to be that IPRoute does not support passing the teamd
> >config yet and the active-backup runner is used as default. Might be
> >worth to check with pyroute maintainer.
> >
>
> I created a github issue for this:
> https://github.com/svinota/pyroute2/issues/699
>  
Pete reacted superfast and just added support for the team device.
I've tested locally and it works fine [1].

-Jan

[1] https://github.com/svinota/pyroute2/issues/699#issuecomment-614597997

[Jan Tluka]

Tue, Apr 14, 2020 at 08:40:37PM CEST, pgagne(a)redhat.com wrote:
> From: Perry Gagne <pgagne(a)redhat.com&gt;
>
> Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
> ---
>  lnst/Devices/TeamDevice.py      | 49 ++++++++++++++++-----------------
>  lnst/Recipes/ENRT/TeamRecipe.py | 11 ++++----
>  2 files changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/lnst/Devices/TeamDevice.py b/lnst/Devices/TeamDevice.py
> index 4f6eee5..3a60125 100644
> --- a/lnst/Devices/TeamDevice.py
> +++ b/lnst/Devices/TeamDevice.py
> @@ -9,44 +9,43 @@ published by the Free Software Foundation; see COPYING for details.
>  __author__ = """
>  olichtne(a)redhat.com (Ondrej Lichtner)
>  """
> -
> -import re
> +import json
>  from lnst.Common.ExecCmd import exec_cmd
> -from lnst.Common.Utils import bool_it
> +from lnst.Common.DeviceError import DeviceError, DeviceConfigError
>  from lnst.Devices.MasterDevice import MasterDevice
>  
> -def prepare_json_str(json_str):
> -    if not json_str:
> -        return "{}"
> -    json_str = json_str.replace('"', '\\"')
> -    json_str = re.sub('\s+', ' ', json_str)
> -    return json_str
>  
> -#TODO rework with pyroute2? don't know how json and teamd works with that...
>  class TeamDevice(MasterDevice):
>      _name_template = "t_team"
>  
>      def __init__(self, ifmanager, *args, **kwargs):
> +        self._config = {}
> +        self._dbus = False
>          super(TeamDevice, self).__init__(ifmanager, *args, **kwargs)
>  
> -        self._config = kwargs.get("config", None)
> -        self._dbus = not bool_it(kwargs.get("disable_dbus", False))
> -
>      @property
>      def config(self):
>          return self._config
>  
> +    @config.setter
> +    def config(self, v: dict):
> +        if not isinstance(v, dict):
> +            raise DeviceConfigError("team device config must be dict")
> +        self._config = v
> +
>      @property
>      def dbus(self):
>          return self._dbus
>  
> -    def _create(self):
> -        teamd_config = prepare_json_str(self.config)
> +    @dbus.setter
> +    def dbus(self, v: bool):
> +        if not isinstance(v, bool):
> +            raise DeviceConfigError("team dbus setting must be bool")
> +        self._dbus = v
>  
> -        exec_cmd("teamd -r -d -c \"%s\" -t %s %s" %\
> -                    (teamd_config,
> -                     self.name,
> -                     " -D" if self.dbus else ""))
> +    def _create(self):
> +        teamd_json = json.dumps(self.config)
> +        exec_cmd(f"teamd -r -d -c '{teamd_json}' -t {self.name}
> {'-D' if self.dbus else ''}") 
Please try to keep lines up to 80 characters where possible.

>  
>          retry = 0
>          while self._nl_msg is None and retry < 5:
> @@ -56,11 +55,11 @@ class TeamDevice(MasterDevice):
>      def destroy(self):
>          exec_cmd("teamd -k -t %s" % self.name)
>  
> -    def slave_add(self, dev, port_config=None):
> -        exec_cmd("teamdctl %s %s port config update %s \"%s\"" %\
> -                    (" -D" if self.dbus else "",
> -                     self.name,
> -                     dev.name,
> -                     prepare_json_str(port_config)))
> +    def slave_add(self, dev, port_config={}):
> +        if not isinstance(port_config, dict):
> +            raise DeviceConfigError(f"team link {dev.name} port config must be
> dict")
> +
> +        port_json = json.dumps(port_config)
> +        exec_cmd(f"teamdctl {'-D' if self.dbus else ''} {self.name}
> port config update {dev.name} '{port_json}'") 
Same here.

>  
>          dev.master = self
> diff --git a/lnst/Recipes/ENRT/TeamRecipe.py b/lnst/Recipes/ENRT/TeamRecipe.py
> index e083b90..ccad5e7 100644
> --- a/lnst/Recipes/ENRT/TeamRecipe.py
> +++ b/lnst/Recipes/ENRT/TeamRecipe.py
> @@ -9,8 +9,9 @@ from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import (
>  from lnst.RecipeCommon.Ping.PingEndpoints import PingEndpoints
>  from lnst.Devices import TeamDevice
>  
> +
>  class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
> -    BaseEnrtRecipe):
> +                 BaseEnrtRecipe):
>      host1 = HostReq()
>      host1.eth0 = DeviceReq(label="tnet",
> driver=RecipeParam("driver"))
>      host1.eth1 = DeviceReq(label="tnet",
> driver=RecipeParam("driver"))
> @@ -30,10 +31,8 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
>      def test_wide_configuration(self):
>          host1, host2 = self.matched.host1, self.matched.host2
>  
> -        #The config argument needs to be used with a team device normally
> -        #(e.g  to specify the runner mode), but it is not used here due to
> -        #a bug in the TeamDevice module
> -        host1.team0 = TeamDevice()
> +        teamd_config = {'runner': {'name': self.params.runner_name}}
> +        host1.team0 = TeamDevice(config=teamd_config)
>  
>          configuration = super().test_wide_configuration()
>          configuration.test_wide_devices = [host1.team0, host2.eth0]
> @@ -90,7 +89,7 @@ class TeamRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
>  
>      def generate_perf_endpoints(self, config):
>          return [(self.matched.host1.team0, self.matched.host2.eth0),
> -            (self.matched.host2.eth0, self.matched.host1.team0)]
> +                (self.matched.host2.eth0, self.matched.host1.team0)]
>  
>      @property
>      def offload_nics(self):
> -- 
> 2.21.1
> _______________________________________________
> LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> To unsubscribe send an email to lnst-developers-leave(a)lists.fedorahosted.org
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives:
> https://lists.fedorahosted.org/archives/list/[e-mail]...

[Ondrej Lichtner]

On Wed, Apr 15, 2020 at 11:42:23AM +0200, Jan Tluka wrote:
>  Tue, Apr 14, 2020 at 08:40:37PM CEST, pgagne(a)redhat.com wrote:
>  >From: Perry Gagne <pgagne(a)redhat.com&gt;
>  >+    def _create(self):
>  >+        teamd_json = json.dumps(self.config)
>  >+        exec_cmd(f"teamd -r -d -c '{teamd_json}' -t {self.name}
> {'-D' if self.dbus else ''}")
>  
>  Please try to keep lines up to 80 characters where possible.
>   
Would it look nicer like this?

    exec_cmd(
        f"teamd -r -d "
        f"-c '{teamd_json}'"
        f"-t {self.name}"
        f"{'-D' if self.dbus else ''}"
    )

AFAIK this should also work, it also IMO makes the multiple "variable
inserts/expressions" (what are those called?) a little easier to read
when they're separated on individual lines.

The ternary operator can also be done like this:

    "-D" if self.dbus else ""

This way you're not using strings within strings with expressions.

Not sure what the best practices for f-strings are tbh, just thinking
"out loud" about what looks nice and readable...

-Ondrej

[Ondrej Lichtner]

On Tue, Apr 14, 2020 at 02:40:37PM -0400, pgagne(a)redhat.com wrote:
>  From: Perry Gagne <pgagne(a)redhat.com&gt;
>  
>  Signed-off-by: Perry Gagne <pgagne(a)redhat.com&gt;
>  ---
>   lnst/Devices/TeamDevice.py      | 49 ++++++++++++++++-----------------
>   lnst/Recipes/ENRT/TeamRecipe.py | 11 ++++----
>   2 files changed, 29 insertions(+), 31 deletions(-)
>   
I don't have any additional comments other than the generic:

* fix the commit message
* separate the formatting changes into its own commit if you want to
  include them now, rather than waiting for when the setup for formatting
  standard is done. Otherwise just skip them.

-Ondrej