On Wed, Jul 24, 2019 at 05:21:16PM +0200, Ondrej Lichtner wrote:
On Wed, Jul 24, 2019 at 02:41:14PM +0200, Jan Tluka wrote:
> Wed, Jul 24, 2019 at 01:29:08PM CEST, csfakian(a)redhat.com wrote:
> >
> >
> >----- Original Message -----
> >> From: "Jan Tluka" <jtluka(a)redhat.com>
> >> To: csfakian(a)redhat.com
> >> Cc: lnst-developers(a)lists.fedorahosted.org
> >> Sent: Wednesday, July 24, 2019 12:05:08 PM
> >> Subject: Re: [PATCH-next v2 8/8] lnst.Recipes.ENRT: rework recipes to adopt
to 334e2c..7bde5c
> >>
> >> Tue, Jul 23, 2019 at 06:19:26PM CEST, csfakian(a)redhat.com wrote:
> >> >From: Christos Sfakianakis <csfakian(a)redhat.com>
> >> >
> >> >Rewrite recipes to be compatible with the above patches.
> >> >Rename SimplePerfRecipe to SimpleNetworkRecipe.
> >> >
> >> >Signed-off-by: Christos Sfakianakis <csfakian(a)redhat.com>
> >> >---
> >> > lnst/Recipes/ENRT/BondRecipe.py | 137 +++++---
> >> > lnst/Recipes/ENRT/DoubleBondRecipe.py | 140 +++++---
> >> > lnst/Recipes/ENRT/DoubleTeamRecipe.py | 159 ++++++----
> >> > lnst/Recipes/ENRT/IpsecEspAeadRecipe.py | 280 ++++++++--------
> >> > lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py | 297 ++++++++---------
> >> > lnst/Recipes/ENRT/NoVirtOvsVxlanRecipe.py | 130 +++++---
> >> > lnst/Recipes/ENRT/PingFloodRecipe.py | 21 +-
> >> > .../ENRT/ShortLivedConnectionsRecipe.py | 87 ++---
> >> > lnst/Recipes/ENRT/SimpleMacsecRecipe.py | 252 +++++++--------
> >> > ...lePerfRecipe.py => SimpleNetworkRecipe.py} | 41 +--
> >> > lnst/Recipes/ENRT/TeamRecipe.py | 134 +++++---
> >> > lnst/Recipes/ENRT/TeamVsBondRecipe.py | 162 ++++++----
> >> > lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py | 230 +++++++++++---
> >> > .../VirtualBridgeVlanInGuestMirroredRecipe.py | 191 ++++++-----
> >> > .../ENRT/VirtualBridgeVlanInGuestRecipe.py | 167 ++++++----
> >> > .../VirtualBridgeVlanInHostMirroredRecipe.py | 178 +++++++----
> >> > .../ENRT/VirtualBridgeVlanInHostRecipe.py | 155 ++++++---
> >> > .../ENRT/VirtualBridgeVlansOverBondRecipe.py | 298
+++++++++++++-----
> >> > ...rtualOvsBridgeVlanInGuestMirroredRecipe.py | 193 +++++++-----
> >> > .../ENRT/VirtualOvsBridgeVlanInGuestRecipe.py | 164 ++++++----
> >> > ...irtualOvsBridgeVlanInHostMirroredRecipe.py | 155 +++++----
> >> > .../ENRT/VirtualOvsBridgeVlanInHostRecipe.py | 144 ++++++---
> >> > .../VirtualOvsBridgeVlansOverBondRecipe.py | 277 ++++++++++------
> >> > lnst/Recipes/ENRT/VlansOverBondRecipe.py | 267 +++++++++++-----
> >> > lnst/Recipes/ENRT/VlansOverTeamRecipe.py | 269 +++++++++++-----
> >> > lnst/Recipes/ENRT/VlansRecipe.py | 230 ++++++++++----
> >> > lnst/Recipes/ENRT/VxlanMulticastRecipe.py | 155 ++++++---
> >> > lnst/Recipes/ENRT/VxlanRemoteRecipe.py | 115 +++++--
> >> > lnst/Recipes/ENRT/__init__.py | 2 +-
> >> > 29 files changed, 3195 insertions(+), 1835 deletions(-)
> >> > rename lnst/Recipes/ENRT/{SimplePerfRecipe.py =>
SimpleNetworkRecipe.py}
> >> > (73%)
> >>
> >> A note on the commit description ($subj). When we push the changes
> >> the commit ids will be different or am I wrong? I'd remove them or
> >> replace with 'to adopt previous changes'.
> >>
> >Ok, will remove these hashes from the subject line
> >> I have one more comment. I asked Ondrej why following pattern is in
> >> SimpleNetworkRecipe:
> >>
> >> + @property
> >> + def offload_nics(self):
> >> + return [self.matched.host1.bond0, self.matched.host2.bond0]
> >> +
> >> + @property
> >> + def mtu_hw_config_dev_list(self):
> >> + return self.offload_nics
> >> +
> >>
> >> I meant why we use offload_nics for mtu_hwconfig_dev_list. Ondrej said
> >> that the idea is to avoid copy-paste errors. I'm fine with that.
> >>
> >> But when I checked the rest of the code, there's a mixture of approach
> >> there.
> >>
> >> E.g. following recipes does not follow this pattern:
> >> lnst/Recipes/ENRT/IpsecEspAeadRecipe.py
> >> lnst/Recipes/ENRT/IpsecEspAhCompRecipe.py
> >> lnst/Recipes/ENRT/NoVirtOvsVxlanRecipe.py
> >> lnst/Recipes/ENRT/SimpleMacsecRecipe.py
> >>
> >> I'd like to have this consistent.
> >Ok, I will avoid referencing one property via another to avoid confusion and
> >misleads.
>
> Actually Ondrej wanted the other approach. It is okay to reference
> one property via another.
Ok so I think for now it's safer for each property to create it's own
list explicitly. Reason being that I think we might want to re-evaluate
which nics get configured and also how the whole HwConfigMixin class
works.
So I think I prefer this:
@property
def offload_nics(self):
return [self.matched.host1.bond0, self.matched.host2.bond0]
@property
def mtu_hw_config_dev_list(self):
return [self.matched.host1.bond0, self.matched.host2.bond0]
-Ondrej