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.
>
> 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.
> lnst/Recipes/ENRT/VirtualBridgeVlanInGuestMirroredRecipe.py
> offload_nics contain guest's vlan devices but the original recipe
> contain guest ethernet devices, also both host NICs are missing in the
> list
Thanks for noticing this, this is actually an error which had existed before this
patchset. In the previous design of next branch, we acted upon endpoints entirely,
meaning that the subconfigs were applied to the endpoints only. I will revisit all
recipes
to make sure the offload list is in agreement with master.
> lnst/Recipes/ENRT/VirtualBridgeVlanInGuestRecipe.py
> offload_nics list should contain both baremetal NICs (not vlan
> devices) and guest's ethernet NIC instead of vlan device
>
Same as above
> in mtu_hw_config_dev_list host2.vlan0 should be removed as it does
> not exist
>
It exists, it is defined in line 47.
You are right! Sorry :-)
> dev_interrupt_hw_config_dev_list()
> there's a logic I don't really understand, you do following:
> self.params.dev_intr_cpu = 0
> why?
>
This is in line with lines 70-76 of the old python code for this recipe.
If perf_tool_cpu is defined, we force-pin to cpu 0 and, to do this, I overwrite
the dev_intr_cpu to be 0 no matter its real_value (note that the default value is
0 anyways,line 33). If perf_tool_cpu is not defined, I inform the tester that
whatever the dev_intr_cpu is, it will be ignored and then return the empty list.
We must not do that. If a user requests specific device interrupt pin setting
we must accept it. The old code is behaving as you described but that is
because we stopped using it since the day we replaced it with
virtual_guest_vlan_in_*_mirrored variants and these contain the code we
should follow on next in non-mirrored recipes.
> .../ENRT/VirtualBridgeVlanInHostRecipe.py
> offload_nics list should contain both baremetal NICs (not vlan
> devices) and guest's ethernet NIC instead of vlan device
>
I will have to correct this as noted above.
> same question on self.params.dev_intr_cpu = 0 as in previous recipe
>
Same reasoning as for the previous recipe holds here.
>
> .../ENRT/VirtualBridgeVlansOverBondRecipe.py
> this recipe does not contain baremetal NICs in offload_nics list
>
Will have to correct this by revisiting the old code's devices.
> .../VirtualOvsBridgeVlanInGuestMirroredRecipe.py
> offload_nics contain guest's vlan devices but the original recipe
> contain guest ethernet devices, also both host NICs are missing in the
> list
>
Same as above
> .../ENRT/VirtualOvsBridgeVlanInGuestRecipe.py
> offload_nics list should contain both baremetal NICs (not vlan
> devices) and guest's ethernet NIC instead of vlan device
>
> in mtu_hw_config_dev_list host2.vlan0 should be removed as it does
> not exist
>
It is defined in line 47
ACK. You're right.
> same question on self.params.dev_intr_cpu = 0 as in previous
recipe
>
Same reasoning as before.
> .../VirtualOvsBridgeVlanInHostMirroredRecipe.py
> this recipe does not contain baremetal NICs in offload_nics list
>
Will correct it
> .../ENRT/VirtualOvsBridgeVlanInHostRecipe.py
> offload_nics list should contain both baremetal NICs (not vlan
> devices) and guest's ethernet NIC instead of vlan device
>
Will correct it
> same question on self.params.dev_intr_cpu = 0 as in previous recipe
>
Same reasoning as before
> .../VirtualOvsBridgeVlansOverBondRecipe.py
> this recipe does not contain baremetal NICs in offload_nics list
>
Will correct it
> lnst/Recipes/ENRT/VlansOverBondRecipe.py
> offload_nics should return only physical NICs not vlan devices
>
Will correct it
> double check mtu*dev_list after you change the above, in this patch
> it contains correct list but after you change offload_nics you need
> to fix mtu*list
Thanks for the warning, I will go through all of these again to avoid cross-referencing
>
> lnst/Recipes/ENRT/VlansOverTeamRecipe.py
> offload_nics should return only physical NICs not vlan devices
>
Will correct it
> double check mtu*dev_list after you change the above, in this patch
> it contains correct list but after you change offload_nics you need
> to fix mtu*list
>
Sure
> lnst/Recipes/ENRT/VlansRecipe.py
> offload_nics should return only physical NICs not vlan devices
>
Will correct it
> double check mtu*dev_list after you change the above, in this patch
> it contains correct list but after you change offload_nics you need
> to fix mtu*list
>
Sure
> lnst/Recipes/ENRT/VxlanMulticastRecipe.py
> and
> lnst/Recipes/ENRT/VxlanRemoteRecipe.py
>
> I'm a bit unsure about the devices in offload_nics list, usually you
> configure it directly on physical NIC (an exception is team/bonding)
> note that old master does not even contain offloading, but maybe
> that's ok
>
Again, if you ack the complete removal of offload tuning for phase3 (as in
master), I will return empty lists in these cases.
I'd say let's remove it.
>>
>>
>> -Jan
>>
>>
>>
>Christos