On Wed, Jul 24, 2019 at 07:29:08AM -0400, Christos Sfakianakis 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.
>
> Additionally:
> lnst/Recipes/ENRT/VirtOvsVxlanRecipe.py
> this recipe does not contain baremetal NICs in offload_nics list
> note that old master does not even contain offloading, but maybe
> that's ok
>
Regarding the offload values, the default case is 'all-on' (unless overwritten
in
the specific recipe). If you think it makes sense to avoid touching offloads completely
for phase 3 (like for the old code), I can redefine the offload_nics property to return
an empty list.
You can also just not inherit from the OffloadSubconfigMixin class and
remove the offload_nics property. That would make it a bit more in line
with the older recipes since they didn't support the feature at all.
-Ondrej