Thu, Apr 30, 2020 at 09:22:54PM CEST, aloughla(a)redhat.com wrote:
From: Aniss Loughlam <aloughla(a)redhat.com>
Signed-off-by: Aniss Loughlam <aloughla(a)redhat.com>
Few overall comments. I did not receive the first patch of the series.
(the subject of the patch says 2/2). Could you confirm that you have
sent the first patch of the series?
Maybe because of that I was not able to apply the patch:
Applying: docs:add Recipes.ENRT.BondRecipe documentation
.git/rebase-apply/patch:16: trailing whitespace.
.git/rebase-apply/patch:18: trailing whitespace.
.git/rebase-apply/patch:34: trailing whitespace.
.git/rebase-apply/patch:49: trailing whitespace.
device using two slave device eth0 and eth1 on host1
error: patch failed: lnst/Recipes/ENRT/BondRecipe.py:79
error: lnst/Recipes/ENRT/BondRecipe.py: patch does not apply
From the log you can see that you also have trailing whitespaces in the patch.
These are unnecessary whitespace characters at the end of a line. Please add
a checker in your development environment (or simply add a syntax highlight
for these patterns) to avoid this in future.
---
lnst/Recipes/ENRT/BondRecipe.py | 111 ++++++++++++++++++++++++++++++++
1 file changed, 111 insertions(+)
diff --git a/lnst/Recipes/ENRT/BondRecipe.py b/lnst/Recipes/ENRT/BondRecipe.py
index 07d7cfc..0941100 100644
--- a/lnst/Recipes/ENRT/BondRecipe.py
+++ b/lnst/Recipes/ENRT/BondRecipe.py
@@ -11,6 +11,33 @@ from lnst.Devices import BondDevice
class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
BaseEnrtRecipe):
+ """
+ This recipe implements Enrt testing for a network scenario that looks
+ as follows
+
+ .. code-block:: none
+
+ .--------.
+ .----------------+ |
+ | .-------+ switch +-------.
+ | | '--------' |
+ .-------------------. |
+ | | bond0 | | |
+ | .---'--. .---'--. | .---'--.
+ .----|-| eth0 |-| eth1 |-|----. .----| eth0 |----.
+ | | '------' '------' | | | '------' |
+ | '-------------------' | | |
+ | | | |
+ | host1 | | host2 |
+ '-----------------------------' '----------------'
+
+ All sub configurations are included via Mixin classes.
+
+ The actual test machinery is implemented in the :any:`BaseEnrtRecipe` class.
+ """
+
+
+
host1 = HostReq()
host1.eth0 = DeviceReq(label="net1",
driver=RecipeParam("driver"))
host1.eth1 = DeviceReq(label="net1",
driver=RecipeParam("driver"))
@@ -28,6 +55,10 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
miimon_value = IntParam(mandatory=True)
def test_wide_configuration(self):
+ """
+ Test wide configuration for this recipe involves creating a bond0
+ device using two slave device eth0 and eth1 on host1
+ """
I believe there's much more here to document. Please check the
SimpleNetworkRecipe and VlansRecipe to get the idea.
Be more verbose about what is being configured, e.g. do not mention
bond0 because the reader does not know what it is. So, "involves
creating a bonding device ..." is better.
Also, there are important test parameters that user should be aware of:
bonding_mode and miimon parameter!
Finally as in other already documented recipes, please include the IP
address configuration.
host1, host2 = self.matched.host1, self.matched.host2
host1.bond0 = BondDevice(mode=self.params.bonding_mode,
miimon=self.params.miimon_value)
@@ -53,6 +84,10 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
return configuration
def generate_test_wide_description(self, config):
+ """
+ Test wide description is extended with the configured VLAN tunnels
+ and their IP addresses
+ """
host1, host2 = self.matched.host1, self.matched.host2
desc = super().generate_test_wide_description(config)
desc += [
@@ -79,39 +114,115 @@ class BondRecipe(CommonHWSubConfigMixin, OffloadSubConfigMixin,
return desc
def test_wide_deconfiguration(self, config):
+ "" # overriding the parent docstring
This should not be necessary anymore. I sent a patch for the sphinx
configuration that disables automatic inclusion of parent methods docstrings.
Hmm, compared to the VlansRecipe doc, you can improve on that, too.
Check the VlansRecipe and do similar here.
del config.test_wide_devices
super().test_wide_deconfiguration(config)
def generate_ping_endpoints(self, config):
+ """
+ The ping endpoints are host1.bond0 and host2.eht0
+ """
You have a typo in host2.eth0.
return [PingEndpoints(self.matched.host1.bond0,
self.matched.host2.eth0)]
def generate_perf_endpoints(self, config):
+ """
+ The perf endpoints for this recipe are the host1.bond0 and host2.eth0
+
+ Returned as::
+
+ [(self.matched.host1.bond0, self.matched.host2.eth0)]
+
+ """
I'd be more reader-friendly here. The docstring you wrote is actually
what user can read from the code (but unfortunately does not provide any
useful information) and just copies the return value.
The documentation should help the reader to understand the scenario
without reading the code.
In this case I'd write:
"The perf endpoints for this recipe are the bonding device on the first
host and the matched ethernet device on the second host."
return [(self.matched.host1.bond0, self.matched.host2.eth0)]
@property
def offload_nics(self):
+ """
+ The `offload_nics` property value for this scenario is a list of the physical
+ devices carrying data of the configured bond:
+
+ | host1.eth0, host.eth1
+ | host2.eth0
+
+ For detailed explanation of this property see :any:`OffloadSubConfigMixin`
+ class and :any:`OffloadSubConfigMixin.offload_nics`.
+ """
Based on the docstring the device list is only host1.eth0 and host1.eth1.
The host2.eth0 does not fit the description. Notice also the typo you have
in host.eth1, should be host1.eth1.
I wouldn't use the "carrying" here (yeah, I know I used it in
VlansRecipe) but instead I'd simplify to:
"is a list of the physical NICs bonded by the configured bonding device on
the first host and the matched ethernet device on the second host"
return [self.matched.host1.bond0, self.matched.host2.eth0]
@property
def mtu_hw_config_dev_list(self):
+ """
+ The `mtu_hw_config_dev_list` property value for this scenario is a
+ list of the configured bond device and the underlying physical
+ devices:
+
+ | host1.eth0, host1.eth1, host1.bond0
+ | host2.eth0
+
+ For detailed explanation of this property see :any:`MTUHWConfigMixin`
+ class and :any:`MTUHWConfigMixin.mtu_hw_config_dev_list`.
+ """
How about "... is a list containing the configured bond device ..." ?
I'm proposing this since "... a list of the configured bond device ..."
sounds a bit odd to me. But not sure, as I'm not the native speaker.
return [self.matched.host1.bond0, self.matched.host2.eth0]
@property
def coalescing_hw_config_dev_list(self):
+ """
+ The `mtu_hw_config_dev_list` property value for this scenario is a
+ is a list of the physical devices carrying data of the configured
+ bond:
+
+ | host1.eth0, host.eth1
+ | host2.eth0
+
+ For detailed explanation of this property see :any:`CoalescingHWConfigMixin`
+ class and :any:`CoalescingHWConfigMixin.coalescing_hw_config_dev_list`.
+ """
Same comment as for the offload_nics property.
return [self.matched.host1.eth0, self.matched.host1.eth1,
self.matched.host2.eth0]
@property
def dev_interrupt_hw_config_dev_list(self):
+ """
+ The `dev_interrupt_hw_config_dev_list` property value for this scenario
+ is a list of the physical devices carrying data of the configured bond:
+
+ | host1.eth0, host.eth1
+ | host2.eth0
+
+ For detailed explanation of this property see :any:`DevInterruptHWConfigMixin`
+ class and :any:`DevInterruptHWConfigMixin.dev_interrupt_hw_config_dev_list`.
+ """
Same comment as for the offload_nics property.
return [self.matched.host1.eth0, self.matched.host1.eth1,
self.matched.host2.eth0]
@property
def parallel_stream_qdisc_hw_config_dev_list(self):
+ """
+ The `parallel_stream_qdisc_hw_config_dev_list` property value for
+ this scenario is a list of the physical devices carrying data of the
+ configured bond:
+
+ | host1.eth0, host.eth1
+ | host2.eth0
+
+ For detailed explanation of this property see
+ :any:`ParallelStreamQDiscHWConfigMixin` class and
+
:any:`ParallelStreamQDiscHWConfigMixin.parallel_stream_qdisc_hw_config_dev_list`.
+ """
Same comment as for the offload_nics property.
return [self.matched.host1.eth0, self.matched.host1.eth1,
self.matched.host2.eth0]
@property
def no_pause_frames_dev_list(self):
+ """
+ The `no pause frames dev list` property value for this scenario is a list of
+ the physical devices carrying data of the configured bond:
+
+ | host1.eth0, host1.eht1
+ | host2.eth0
+
+ For detailed explanation of this property see
+ :any:`class DisablePauseFramesHWConfigMixin`.
+ """
Same comment as for the offload_nics property.
return [self.matched.host1.eth0, self.matched.host1.eth1,
self.matched.host2.eth0]
--
2.24.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/lnst-developers@lists.fedora...