This patch set adds a recipe to perform a simple L2TP tunnel test.
A minor modification to the BaseTunnelRecipe was required to enable users to bypass running the packet assert test if it's not required.
The Device.ip_add() method was updated to support the configuration of addresses with peer address included. This is a typical use case of the L2TP deployments.
I have also found several issues in the LNST code while implementing the test, so I'm including them here.
Most important one is handling of RTM_NEWADDR netlink update message that contained a bug for the point-to-point network address assignment.
v2: logical reordering of the patches
Jan Tluka (6): Recipes.ENRT.BaseTunnelRecipe: handle empty packet assert configs RecipeCommon.Ping.Evaluators.RatePingEvaluator: fix the rate_text RatePingEvaluator: handle None result of the Ping test Devices.Device: fix RTM_NEWADDR handling Devices.Device: modify _ip_add_one method to handle peer addresses Recipes.ENRT: add L2TPTunnelRecipe
docs/source/l2tp_tunnel_recipe.rst | 6 + docs/source/specific_scenarios.rst | 1 + lnst/Devices/Device.py | 50 ++++-- .../Ping/Evaluators/RatePingEvaluator.py | 8 +- lnst/Recipes/ENRT/BaseTunnelRecipe.py | 15 +- lnst/Recipes/ENRT/L2TPTunnelRecipe.py | 147 ++++++++++++++++++ 6 files changed, 209 insertions(+), 18 deletions(-) create mode 100644 docs/source/l2tp_tunnel_recipe.rst create mode 100755 lnst/Recipes/ENRT/L2TPTunnelRecipe.py
Some of the recipes inheriting from the BaseTunnelRecipe may not require packet assert test. This could be solved by not including the PacketAssertTestAndEvaluate in the BaseTunnelRecipe inheritance but I plan to send additional tests that use the packet assert test and it's probably better to add a condition to execute the test.
The BaseTunnelRecipe is modified to perform the packet assert test only if the get_packet_assert_config() of the derived class returns anything other than None value.
Signed-off-by: Jan Tluka jtluka@redhat.com --- lnst/Recipes/ENRT/BaseTunnelRecipe.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/lnst/Recipes/ENRT/BaseTunnelRecipe.py b/lnst/Recipes/ENRT/BaseTunnelRecipe.py index 4a354a9d..d4bb6962 100644 --- a/lnst/Recipes/ENRT/BaseTunnelRecipe.py +++ b/lnst/Recipes/ENRT/BaseTunnelRecipe.py @@ -111,11 +111,15 @@ class BaseTunnelRecipe(
def ping_test(self, ping_configs): pa_config = self.get_packet_assert_config(ping_configs[0]) - self.packet_assert_test_start(pa_config) - self.ctl.wait(2) + if pa_config is not None: + self.packet_assert_test_start(pa_config) + self.ctl.wait(2) ping_result = super().ping_test(ping_configs) - self.ctl.wait(2) - pa_result = self.packet_assert_test_stop() + if pa_config is not None: + self.ctl.wait(2) + pa_result = self.packet_assert_test_stop() + else: + pa_result = None
result = ((ping_result, pa_config, pa_result),)
@@ -124,7 +128,8 @@ class BaseTunnelRecipe( def ping_report_and_evaluate(self, results): for res in results: super().ping_report_and_evaluate(res[0]) - self.packet_assert_evaluate_and_report(res[1], res[2]) + if res[1] is not None: + self.packet_assert_evaluate_and_report(res[1], res[2])
def get_packet_assert_config(self, ping_config): """
On Tue, May 04, 2021 at 11:21:12AM +0200, Jan Tluka wrote:
Some of the recipes inheriting from the BaseTunnelRecipe may not require packet assert test. This could be solved by not including the PacketAssertTestAndEvaluate in the BaseTunnelRecipe inheritance but I plan to send additional tests that use the packet assert test and it's probably better to add a condition to execute the test.
"some tunnel recipes don't require packet assert testing"
is this because of the legacy recipe implementation - it wasn't implemented in legacy so let's not do it here?
Or is there a more techincal reason? e.g. L2TP tunnel cannot be detected by packet assert?
I wondering if it would maybe make sense to include packet assert test for all tunnel recipes regardless of if we did this in legacy?
-Ondrej
The BaseTunnelRecipe is modified to perform the packet assert test only if the get_packet_assert_config() of the derived class returns anything other than None value.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Recipes/ENRT/BaseTunnelRecipe.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/lnst/Recipes/ENRT/BaseTunnelRecipe.py b/lnst/Recipes/ENRT/BaseTunnelRecipe.py index 4a354a9d..d4bb6962 100644 --- a/lnst/Recipes/ENRT/BaseTunnelRecipe.py +++ b/lnst/Recipes/ENRT/BaseTunnelRecipe.py @@ -111,11 +111,15 @@ class BaseTunnelRecipe(
def ping_test(self, ping_configs): pa_config = self.get_packet_assert_config(ping_configs[0])
self.packet_assert_test_start(pa_config)self.ctl.wait(2)
if pa_config is not None:self.packet_assert_test_start(pa_config)self.ctl.wait(2) ping_result = super().ping_test(ping_configs)
self.ctl.wait(2)pa_result = self.packet_assert_test_stop()
if pa_config is not None:self.ctl.wait(2)pa_result = self.packet_assert_test_stop()else:pa_result = None result = ((ping_result, pa_config, pa_result),)@@ -124,7 +128,8 @@ class BaseTunnelRecipe( def ping_report_and_evaluate(self, results): for res in results: super().ping_report_and_evaluate(res[0])
self.packet_assert_evaluate_and_report(res[1], res[2])
if res[1] is not None:self.packet_assert_evaluate_and_report(res[1], res[2])def get_packet_assert_config(self, ping_config): """
-- 2.26.3 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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.fedorahos... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
Fri, May 07, 2021 at 03:16:05PM CEST, olichtne@redhat.com wrote:
On Tue, May 04, 2021 at 11:21:12AM +0200, Jan Tluka wrote:
Some of the recipes inheriting from the BaseTunnelRecipe may not require packet assert test. This could be solved by not including the PacketAssertTestAndEvaluate in the BaseTunnelRecipe inheritance but I plan to send additional tests that use the packet assert test and it's probably better to add a condition to execute the test.
"some tunnel recipes don't require packet assert testing"
is this because of the legacy recipe implementation - it wasn't implemented in legacy so let's not do it here?
Or is there a more techincal reason? e.g. L2TP tunnel cannot be detected by packet assert?
I wondering if it would maybe make sense to include packet assert test for all tunnel recipes regardless of if we did this in legacy?
The reason was that it was not present in the legacy implementation. I'll check and try to implement a packet assert test here. Then we can get rid of this hack.
Thanks for review!
J.
-Ondrej
The rate_text string in this branch is formatted too early. Also insufficient number of values for formatting is used causing crashes.
Signed-off-by: Jan Tluka jtluka@redhat.com --- lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py b/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py index 6760d32a..6ed7d02f 100644 --- a/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py +++ b/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py @@ -43,9 +43,7 @@ class RatePingEvaluator(BaseResultEvaluator): )
if self.rate is not None: - rate_text = 'measured rate {} is {} rate({})'.format( - ping_rate, self.rate) - + rate_text = 'measured rate {} is {} rate({})' if ping_rate != int(self.rate): result_status = False result_text.append(
If Ping test does not succeed, the result data does not contain the 'rate' key and would fail with a traceback. This patch handles this situation and creates a fail result with explanatory message.
Signed-off-by: Jan Tluka jtluka@redhat.com --- lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py b/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py index 6ed7d02f..365329f9 100644 --- a/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py +++ b/lnst/RecipeCommon/Ping/Evaluators/RatePingEvaluator.py @@ -13,6 +13,10 @@ class RatePingEvaluator(BaseResultEvaluator): )
def evaluate_results(self, recipe, result): + if result is None or 'rate' not in result: + recipe.add_result(False, 'Insufficient data for the evaluation of Ping test') + return + result_status = True ping_rate = int(result['rate'])
The handler of RTM_NEWADDR incorrectly fetches the IFA_ADDRESS attribute to update the device address. This is not an issue for IPv6 or typical update of the device address. However it is a bug for the assignment of address with peer address included (typical for point to point networks), for example:
ip address add 192.168.100.1/24 peer 192.169.100.2 dev enp7s0
In this case, the IFA_ADDRESS attribute value is the peer address value and the device address is the value of IFA_LOCAL.
When the peer address is not specified, both IFA_LOCAL and IFA_ADDRESS contain the same value which is the reason this has been working without any issues previously.
The fix is to use the IFA_LOCAL attribute value to update the device address for IPv4 scenarios.
Signed-off-by: Jan Tluka jtluka@redhat.com --- lnst/Devices/Device.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index c31a018f..68b5f1af 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -25,7 +25,7 @@ from lnst.Common.ExecCmd import exec_cmd, ExecCmdFail from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.DeviceError import DeviceFeatureNotSupported -from lnst.Common.IpAddress import ipaddress +from lnst.Common.IpAddress import ipaddress, AF_INET from lnst.Common.HWAddress import hwaddress
from pyroute2.netlink.rtnl import RTM_NEWLINK @@ -205,8 +205,23 @@ class Device(object, metaclass=DeviceMeta): if nl_msg['header']['type'] == RTM_NEWLINK: self._nl_msg = nl_msg elif nl_msg['header']['type'] == RTM_NEWADDR: - addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'), - flags=nl_msg.get_attr("IFA_FLAGS")) + if nl_msg['family'] == AF_INET: + """ + from if_addr.h: + /* + * Important comment: + * IFA_ADDRESS is prefix address, rather than local interface address. + * It makes no difference for normally configured broadcast interfaces, + * but for point-to-point IFA_ADDRESS is DESTINATION address, + * local address is supplied in IFA_LOCAL attribute. + */ + """ + addr = ipaddress(nl_msg.get_attr('IFA_LOCAL'), + flags=nl_msg.get_attr("IFA_FLAGS")) + else: + addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'), + flags=nl_msg.get_attr("IFA_FLAGS")) + addr.prefixlen = nl_msg["prefixlen"]
if addr not in self._ip_addrs:
On Tue, May 04, 2021 at 11:21:15AM +0200, Jan Tluka wrote:
The handler of RTM_NEWADDR incorrectly fetches the IFA_ADDRESS attribute to update the device address. This is not an issue for IPv6 or typical update of the device address. However it is a bug for the assignment of address with peer address included (typical for point to point networks), for example:
ip address add 192.168.100.1/24 peer 192.169.100.2 dev enp7s0
In this case, the IFA_ADDRESS attribute value is the peer address value and the device address is the value of IFA_LOCAL.
When the peer address is not specified, both IFA_LOCAL and IFA_ADDRESS contain the same value which is the reason this has been working without any issues previously.
The fix is to use the IFA_LOCAL attribute value to update the device address for IPv4 scenarios.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Devices/Device.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index c31a018f..68b5f1af 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -25,7 +25,7 @@ from lnst.Common.ExecCmd import exec_cmd, ExecCmdFail from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.DeviceError import DeviceFeatureNotSupported -from lnst.Common.IpAddress import ipaddress +from lnst.Common.IpAddress import ipaddress, AF_INET from lnst.Common.HWAddress import hwaddress
from pyroute2.netlink.rtnl import RTM_NEWLINK @@ -205,8 +205,23 @@ class Device(object, metaclass=DeviceMeta): if nl_msg['header']['type'] == RTM_NEWLINK: self._nl_msg = nl_msg elif nl_msg['header']['type'] == RTM_NEWADDR:
addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),flags=nl_msg.get_attr("IFA_FLAGS"))
if nl_msg['family'] == AF_INET:"""from if_addr.h:/** Important comment:* IFA_ADDRESS is prefix address, rather than local interface address.* It makes no difference for normally configured broadcast interfaces,* but for point-to-point IFA_ADDRESS is DESTINATION address,* local address is supplied in IFA_LOCAL attribute.*/"""addr = ipaddress(nl_msg.get_attr('IFA_LOCAL'),flags=nl_msg.get_attr("IFA_FLAGS"))else:addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),flags=nl_msg.get_attr("IFA_FLAGS"))
Based on your explanation and the if_addr.h comment, shouldn't we **always** read the IFA_LOCAL attribute?
-Ondrej
addr.prefixlen = nl_msg["prefixlen"] if addr not in self._ip_addrs:-- 2.26.3 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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.fedorahos... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
Fri, May 07, 2021 at 03:31:18PM CEST, olichtne@redhat.com wrote:
On Tue, May 04, 2021 at 11:21:15AM +0200, Jan Tluka wrote:
The handler of RTM_NEWADDR incorrectly fetches the IFA_ADDRESS attribute to update the device address. This is not an issue for IPv6 or typical update of the device address. However it is a bug for the assignment of address with peer address included (typical for point to point networks), for example:
ip address add 192.168.100.1/24 peer 192.169.100.2 dev enp7s0
In this case, the IFA_ADDRESS attribute value is the peer address value and the device address is the value of IFA_LOCAL.
When the peer address is not specified, both IFA_LOCAL and IFA_ADDRESS contain the same value which is the reason this has been working without any issues previously.
The fix is to use the IFA_LOCAL attribute value to update the device address for IPv4 scenarios.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Devices/Device.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index c31a018f..68b5f1af 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -25,7 +25,7 @@ from lnst.Common.ExecCmd import exec_cmd, ExecCmdFail from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.DeviceError import DeviceFeatureNotSupported -from lnst.Common.IpAddress import ipaddress +from lnst.Common.IpAddress import ipaddress, AF_INET from lnst.Common.HWAddress import hwaddress
from pyroute2.netlink.rtnl import RTM_NEWLINK @@ -205,8 +205,23 @@ class Device(object, metaclass=DeviceMeta): if nl_msg['header']['type'] == RTM_NEWLINK: self._nl_msg = nl_msg elif nl_msg['header']['type'] == RTM_NEWADDR:
addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),flags=nl_msg.get_attr("IFA_FLAGS"))
if nl_msg['family'] == AF_INET:"""from if_addr.h:/** Important comment:* IFA_ADDRESS is prefix address, rather than local interface address.* It makes no difference for normally configured broadcast interfaces,* but for point-to-point IFA_ADDRESS is DESTINATION address,* local address is supplied in IFA_LOCAL attribute.*/"""addr = ipaddress(nl_msg.get_attr('IFA_LOCAL'),flags=nl_msg.get_attr("IFA_FLAGS"))else:addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),flags=nl_msg.get_attr("IFA_FLAGS"))Based on your explanation and the if_addr.h comment, shouldn't we **always** read the IFA_LOCAL attribute?
-Ondrej
Yes, we should. We should query IFA_ADDRESS only for IPv6 [1].
J.
[1] https://github.com/tgraf/libnl/blob/master/lib/route/addr.c#L259
Mon, May 10, 2021 at 12:25:27PM CEST, jtluka@redhat.com wrote:
Fri, May 07, 2021 at 03:31:18PM CEST, olichtne@redhat.com wrote:
On Tue, May 04, 2021 at 11:21:15AM +0200, Jan Tluka wrote:
The handler of RTM_NEWADDR incorrectly fetches the IFA_ADDRESS attribute to update the device address. This is not an issue for IPv6 or typical update of the device address. However it is a bug for the assignment of address with peer address included (typical for point to point networks), for example:
ip address add 192.168.100.1/24 peer 192.169.100.2 dev enp7s0
In this case, the IFA_ADDRESS attribute value is the peer address value and the device address is the value of IFA_LOCAL.
When the peer address is not specified, both IFA_LOCAL and IFA_ADDRESS contain the same value which is the reason this has been working without any issues previously.
The fix is to use the IFA_LOCAL attribute value to update the device address for IPv4 scenarios.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Devices/Device.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index c31a018f..68b5f1af 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -25,7 +25,7 @@ from lnst.Common.ExecCmd import exec_cmd, ExecCmdFail from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError from lnst.Common.DeviceError import DeviceFeatureNotSupported -from lnst.Common.IpAddress import ipaddress +from lnst.Common.IpAddress import ipaddress, AF_INET from lnst.Common.HWAddress import hwaddress
from pyroute2.netlink.rtnl import RTM_NEWLINK @@ -205,8 +205,23 @@ class Device(object, metaclass=DeviceMeta): if nl_msg['header']['type'] == RTM_NEWLINK: self._nl_msg = nl_msg elif nl_msg['header']['type'] == RTM_NEWADDR:
addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),flags=nl_msg.get_attr("IFA_FLAGS"))
if nl_msg['family'] == AF_INET:"""from if_addr.h:/** Important comment:* IFA_ADDRESS is prefix address, rather than local interface address.* It makes no difference for normally configured broadcast interfaces,* but for point-to-point IFA_ADDRESS is DESTINATION address,* local address is supplied in IFA_LOCAL attribute.*/"""addr = ipaddress(nl_msg.get_attr('IFA_LOCAL'),flags=nl_msg.get_attr("IFA_FLAGS"))else:addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),flags=nl_msg.get_attr("IFA_FLAGS"))Based on your explanation and the if_addr.h comment, shouldn't we **always** read the IFA_LOCAL attribute?
-Ondrej
Yes, we should. We should query IFA_ADDRESS only for IPv6 [1].
J.
[1] https://github.com/tgraf/libnl/blob/master/lib/route/addr.c#L259
FYI, I double checked this. The patch is correct.
For IPv4 we should read the IFA_LOCAL, but for IPv6 only the IFA_ADDRESS is populated:
$ ip -4 addr add dev lo 192.168.10.10/24
{'attrs': [('IFA_ADDRESS', '192.168.10.10'), ('IFA_LOCAL', '192.168.10.10'), ('IFA_LABEL', 'lo'), ('IFA_FLAGS', 128), [root@lnst-devel lnst]# ('IFA_CACHEINFO', {'ifa_preferred': 4294967295, 'ifa_valid': 4294967295, 'cstamp': 2507106, 'tstamp': 2507106})], 'event': 'RTM_NEWADDR', 'family': 2, 'flags': 128, 'header': {'error': None, 'flags': 0, 'length': 76, 'pid': 3993, 'sequence_number': 1620746196, 'stats': Stats(qsize=0, delta=0, delay=0), 'target': 'localhost', 'type': 20}, 'index': 1, 'prefixlen': 24, 'scope': 0}
$ ip -6 addr add dev lo fc00::6/64
{'attrs': [('IFA_ADDRESS', 'fc00::6'), ('IFA_CACHEINFO', {'ifa_preferred': 4294967295, 'ifa_valid': 4294967295, 'cstamp': 2475861, 'tstamp': 2475861}), ('IFA_FLAGS', 192)], 'event': 'RTM_NEWADDR', 'family': 10, 'flags': 192, 'header': {'error': None, 'flags': 0, 'length': 72, 'pid': 0, 'sequence_number': 0, 'stats': Stats(qsize=0, delta=0, delay=0), 'target': 'localhost', 'type': 20}, 'index': 1, 'prefixlen': 64, 'scope': 0}
-Jan
The addr parameter of the _ip_add_one() method can now take either a string specifying a device address or a tuple that includes both the device address and peer address to support point-to-point networks.
Signed-off-by: Jan Tluka jtluka@redhat.com --- lnst/Devices/Device.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 68b5f1af..b98d9235 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -569,15 +569,30 @@ class Device(object, metaclass=DeviceMeta): self._ip_addrs = []
def _ip_add_one(self, addr): - ip = ipaddress(addr) + if type(addr) is tuple: + ip = ipaddress(addr[0]) + peer = ipaddress(addr[1]) + else: + ip = ipaddress(addr) + peer = None + if ip not in self.ips: - self._ipr_wrapper("addr", "add", index=self.ifindex, - address=str(ip), mask=ip.prefixlen) + kwargs = dict( + index=self.ifindex, + local=str(ip), + address=str(ip), + mask=ip.prefixlen + ) + if peer: + kwargs['address'] = str(peer) + + self._ipr_wrapper("addr", "add", **kwargs) + for i in range(5): logging.debug("Waiting for ip address to be added {} of 5".format(i)) time.sleep(1) self._if_manager.rescan_devices() - if addr in self.ips: + if ip in self.ips: break else: raise DeviceError("Failed to configure ip address {}".format(str(ip))) @@ -586,9 +601,9 @@ class Device(object, metaclass=DeviceMeta): """add an ip address or a list of ip addresses
Args: - addr -- an address accepted by the ipaddress factory method - or a list of addresses accepted by the ipaddress - factory method + addr -- an address or a tuple (address, peer_address) accepted + by the ipaddress factory method or a list of the previous + two types """
if isinstance(addr, list):
On Tue, May 04, 2021 at 11:21:16AM +0200, Jan Tluka wrote:
The addr parameter of the _ip_add_one() method can now take either a string specifying a device address or a tuple that includes both the device address and peer address to support point-to-point networks.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Devices/Device.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 68b5f1af..b98d9235 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -569,15 +569,30 @@ class Device(object, metaclass=DeviceMeta): self._ip_addrs = []
def _ip_add_one(self, addr):
ip = ipaddress(addr)
if type(addr) is tuple:ip = ipaddress(addr[0])peer = ipaddress(addr[1])else:ip = ipaddress(addr)peer = Noneif ip not in self.ips:
self._ipr_wrapper("addr", "add", index=self.ifindex,address=str(ip), mask=ip.prefixlen)
kwargs = dict(index=self.ifindex,local=str(ip),address=str(ip),
I think you have a copy paste typo here, both local and address are str(ip)
-Ondrej
mask=ip.prefixlen)if peer:kwargs['address'] = str(peer)self._ipr_wrapper("addr", "add", **kwargs)for i in range(5): logging.debug("Waiting for ip address to be added {} of 5".format(i)) time.sleep(1) self._if_manager.rescan_devices()
if addr in self.ips:
if ip in self.ips: break else: raise DeviceError("Failed to configure ip address {}".format(str(ip)))@@ -586,9 +601,9 @@ class Device(object, metaclass=DeviceMeta): """add an ip address or a list of ip addresses
Args:
addr -- an address accepted by the ipaddress factory methodor a list of addresses accepted by the ipaddressfactory method
addr -- an address or a tuple (address, peer_address) acceptedby the ipaddress factory method or a list of the previoustwo types """
I'm also not sure about this type overloading that now accepts three different types.
I checked and it seems that this was added in 2017 but nowhere in the lnst repository do we actually call the ip_add method with a list of addresses. So to me this seems like something that we thought maybe could be interesting but ended up not really being useful.
At this point I'm not sure what the use case for that even would be that would make it more readable than just calling a single ip add multiple times.
With that I think it may be more relevant to just refactor the "ip_add" method back to the single add use case, and continuing that, instead extend the parameter list so that the method accepts a "peer" kwarg.
Doing this we won't need to "parse" the tuple in the single ip add method.
What do you think? Can you think of a good use case of the bulk ip address add operation? This was added by jpirko, maybe there were some switchdev or other mellanox recipes that used this?
-Ondrej
if isinstance(addr, list):-- 2.26.3 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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.fedorahos... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
Fri, May 07, 2021 at 03:50:15PM CEST, olichtne@redhat.com wrote:
On Tue, May 04, 2021 at 11:21:16AM +0200, Jan Tluka wrote:
The addr parameter of the _ip_add_one() method can now take either a string specifying a device address or a tuple that includes both the device address and peer address to support point-to-point networks.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Devices/Device.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 68b5f1af..b98d9235 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -569,15 +569,30 @@ class Device(object, metaclass=DeviceMeta): self._ip_addrs = []
def _ip_add_one(self, addr):
ip = ipaddress(addr)
if type(addr) is tuple:ip = ipaddress(addr[0])peer = ipaddress(addr[1])else:ip = ipaddress(addr)peer = Noneif ip not in self.ips:
self._ipr_wrapper("addr", "add", index=self.ifindex,address=str(ip), mask=ip.prefixlen)
kwargs = dict(index=self.ifindex,local=str(ip),address=str(ip),I think you have a copy paste typo here, both local and address are str(ip)
-Ondrej
Good catch! Will fix that.
mask=ip.prefixlen)if peer:kwargs['address'] = str(peer)self._ipr_wrapper("addr", "add", **kwargs)for i in range(5): logging.debug("Waiting for ip address to be added {} of 5".format(i)) time.sleep(1) self._if_manager.rescan_devices()
if addr in self.ips:
if ip in self.ips: break else: raise DeviceError("Failed to configure ip address {}".format(str(ip)))@@ -586,9 +601,9 @@ class Device(object, metaclass=DeviceMeta): """add an ip address or a list of ip addresses
Args:
addr -- an address accepted by the ipaddress factory methodor a list of addresses accepted by the ipaddressfactory method
addr -- an address or a tuple (address, peer_address) acceptedby the ipaddress factory method or a list of the previoustwo types """I'm also not sure about this type overloading that now accepts three different types.
I did not like it either. I just did not want to break the API. But I'm 100% for refactoring this.
I checked and it seems that this was added in 2017 but nowhere in the lnst repository do we actually call the ip_add method with a list of addresses. So to me this seems like something that we thought maybe could be interesting but ended up not really being useful.
At this point I'm not sure what the use case for that even would be that would make it more readable than just calling a single ip add multiple times.
With that I think it may be more relevant to just refactor the "ip_add" method back to the single add use case, and continuing that, instead extend the parameter list so that the method accepts a "peer" kwarg.
Doing this we won't need to "parse" the tuple in the single ip add method.
What do you think? Can you think of a good use case of the bulk ip address add operation? This was added by jpirko, maybe there were some switchdev or other mellanox recipes that used this?
Will try to reach out to Mellanox developers and also will take a look at the switchdev use cases. If they don't use that I'll refactor the code.
J.
-Ondrej
if isinstance(addr, list):-- 2.26.3 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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.fedorahos... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
Mon, May 10, 2021 at 12:30:42PM CEST, jtluka@redhat.com wrote:
Fri, May 07, 2021 at 03:50:15PM CEST, olichtne@redhat.com wrote:
On Tue, May 04, 2021 at 11:21:16AM +0200, Jan Tluka wrote:
The addr parameter of the _ip_add_one() method can now take either a string specifying a device address or a tuple that includes both the device address and peer address to support point-to-point networks.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Devices/Device.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 68b5f1af..b98d9235 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -569,15 +569,30 @@ class Device(object, metaclass=DeviceMeta): self._ip_addrs = []
def _ip_add_one(self, addr):
ip = ipaddress(addr)
if type(addr) is tuple:ip = ipaddress(addr[0])peer = ipaddress(addr[1])else:ip = ipaddress(addr)peer = Noneif ip not in self.ips:
self._ipr_wrapper("addr", "add", index=self.ifindex,address=str(ip), mask=ip.prefixlen)
kwargs = dict(index=self.ifindex,local=str(ip),address=str(ip),I think you have a copy paste typo here, both local and address are str(ip)
-Ondrej
Good catch! Will fix that.
Actually no. This is not a typo.
In the next 'if peer' block this gets overwritten. I believe I did this intentionally to coever both IPv4 and IPv6 configuration.
If you think I should rework this a bit so that this is more readable, I'll do that.
J.
mask=ip.prefixlen)if peer:kwargs['address'] = str(peer)self._ipr_wrapper("addr", "add", **kwargs)for i in range(5): logging.debug("Waiting for ip address to be added {} of 5".format(i)) time.sleep(1) self._if_manager.rescan_devices()
if addr in self.ips:
if ip in self.ips: break else: raise DeviceError("Failed to configure ip address {}".format(str(ip)))@@ -586,9 +601,9 @@ class Device(object, metaclass=DeviceMeta): """add an ip address or a list of ip addresses
Args:
addr -- an address accepted by the ipaddress factory methodor a list of addresses accepted by the ipaddressfactory method
addr -- an address or a tuple (address, peer_address) acceptedby the ipaddress factory method or a list of the previoustwo types """I'm also not sure about this type overloading that now accepts three different types.
I did not like it either. I just did not want to break the API. But I'm 100% for refactoring this.
I checked and it seems that this was added in 2017 but nowhere in the lnst repository do we actually call the ip_add method with a list of addresses. So to me this seems like something that we thought maybe could be interesting but ended up not really being useful.
At this point I'm not sure what the use case for that even would be that would make it more readable than just calling a single ip add multiple times.
With that I think it may be more relevant to just refactor the "ip_add" method back to the single add use case, and continuing that, instead extend the parameter list so that the method accepts a "peer" kwarg.
Doing this we won't need to "parse" the tuple in the single ip add method.
What do you think? Can you think of a good use case of the bulk ip address add operation? This was added by jpirko, maybe there were some switchdev or other mellanox recipes that used this?
Will try to reach out to Mellanox developers and also will take a look at the switchdev use cases. If they don't use that I'll refactor the code.
J.
-Ondrej
if isinstance(addr, list):-- 2.26.3 _______________________________________________ LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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.fedorahos... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org To unsubscribe send an email to lnst-developers-leave@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.fedorahos... Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
On Mon, May 10, 2021 at 05:19:07PM +0200, Jan Tluka wrote:
Mon, May 10, 2021 at 12:30:42PM CEST, jtluka@redhat.com wrote:
Fri, May 07, 2021 at 03:50:15PM CEST, olichtne@redhat.com wrote:
On Tue, May 04, 2021 at 11:21:16AM +0200, Jan Tluka wrote:
The addr parameter of the _ip_add_one() method can now take either a string specifying a device address or a tuple that includes both the device address and peer address to support point-to-point networks.
Signed-off-by: Jan Tluka jtluka@redhat.com
lnst/Devices/Device.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py index 68b5f1af..b98d9235 100644 --- a/lnst/Devices/Device.py +++ b/lnst/Devices/Device.py @@ -569,15 +569,30 @@ class Device(object, metaclass=DeviceMeta): self._ip_addrs = []
def _ip_add_one(self, addr):
ip = ipaddress(addr)
if type(addr) is tuple:ip = ipaddress(addr[0])peer = ipaddress(addr[1])else:ip = ipaddress(addr)peer = Noneif ip not in self.ips:
self._ipr_wrapper("addr", "add", index=self.ifindex,address=str(ip), mask=ip.prefixlen)
kwargs = dict(index=self.ifindex,local=str(ip),address=str(ip),I think you have a copy paste typo here, both local and address are str(ip)
-Ondrej
Good catch! Will fix that.
Actually no. This is not a typo.
In the next 'if peer' block this gets overwritten. I believe I did this intentionally to coever both IPv4 and IPv6 configuration.
If you think I should rework this a bit so that this is more readable, I'll do that.
J.
Ah, yeah... I'm not sure i fully understand this local vs address vs peer and ipv4 and ipv6 situation so I'm going to leave the decision to you, whatever makes the most sense.
Going just by how the code "looks"I think maybe in the "init" part of the method we could do:
if type(addr) is tuple: ip = ipaddress(addr[0]) peer = ipaddress(addr[1]) else: ip = ipaddress(addr) peer = ipaddress(addr) # and a comment explaining this here?
And then you can use ip and peer in the rest of the method without any conditions?
There's also an option to just use a ternary operator:
address=str(ip) if peer is None else str(peer),
But like I've said, I'm not sure what the correct logic here is...
-Ondrej
This adds a simple L2TP tunnel test.
Signed-off-by: Jan Tluka jtluka@redhat.com --- docs/source/l2tp_tunnel_recipe.rst | 6 ++ docs/source/specific_scenarios.rst | 1 + lnst/Recipes/ENRT/L2TPTunnelRecipe.py | 147 ++++++++++++++++++++++++++ 3 files changed, 154 insertions(+) create mode 100644 docs/source/l2tp_tunnel_recipe.rst create mode 100755 lnst/Recipes/ENRT/L2TPTunnelRecipe.py
diff --git a/docs/source/l2tp_tunnel_recipe.rst b/docs/source/l2tp_tunnel_recipe.rst new file mode 100644 index 00000000..b967a939 --- /dev/null +++ b/docs/source/l2tp_tunnel_recipe.rst @@ -0,0 +1,6 @@ +L2TPTunnelRecipe +^^^^^^^^^^^^^^^^ + +.. autoclass:: lnst.Recipes.ENRT.L2TPTunnelRecipe.L2TPTunnelRecipe + :members: + :show-inheritance: diff --git a/docs/source/specific_scenarios.rst b/docs/source/specific_scenarios.rst index fb0b4eee..0cf999cf 100644 --- a/docs/source/specific_scenarios.rst +++ b/docs/source/specific_scenarios.rst @@ -8,3 +8,4 @@ Specific ENRT scenarios vlans_recipe vlans_over_bond_recipe gre_tunnel_recipe + l2tp_tunnel_recipe diff --git a/lnst/Recipes/ENRT/L2TPTunnelRecipe.py b/lnst/Recipes/ENRT/L2TPTunnelRecipe.py new file mode 100755 index 00000000..080eb2b1 --- /dev/null +++ b/lnst/Recipes/ENRT/L2TPTunnelRecipe.py @@ -0,0 +1,147 @@ +from lnst.Controller import HostReq, DeviceReq, RecipeParam +from lnst.Common.IpAddress import AF_INET +from lnst.Common.Parameters import ChoiceParam, StrParam +from lnst.RecipeCommon.L2TPManager import L2TPManager +from lnst.Devices import L2TPSessionDevice +from lnst.RecipeCommon.Ping.PingEndpoints import PingEndpoints +from lnst.Recipes.ENRT.BaseTunnelRecipe import BaseTunnelRecipe +from lnst.Recipes.ENRT.ConfigMixins.CommonHWSubConfigMixin import ( + CommonHWSubConfigMixin, +) + + +class L2TPTunnelRecipe(CommonHWSubConfigMixin, BaseTunnelRecipe): + """ + This class implements a recipe that configures a simple L2TP tunnel with + one tunnel session between two hosts. + + .. code-block:: none + + .--------. + .------| switch |-----. + | '--------' | + | | + .-------|------. .-------|------. + | .--'-. | | .--'-. | + | |eth0| | | |eth0| | + | '----' | | '----' | + | | | | | | | | + | ----' '--- | | ----' '--- | + | L2TP tunnel | | L2TPtunnel | + | ---------- | | ---------- | + | | | | + | host1 | | host2 | + '--------------' '--------------' + + The actual test machinery is implemented in the :any:`BaseEnrtRecipe` class. + + The test wide configuration is implemented in the :any:`BaseTunnelRecipe` + class. + + :param l2tp_encapsulation: + (mandatory test parameter) the encapsulation mode for the L2TP tunnel, + can be either **udp** or **ip** + """ + + host1 = HostReq() + host1.eth0 = DeviceReq(label="net1", driver=RecipeParam("driver")) + + host2 = HostReq() + host2.eth0 = DeviceReq(label="net1", driver=RecipeParam("driver")) + + l2tp_encapsulation = ChoiceParam( + type=StrParam, choices=set(["udp", "ip"]), mandatory=True + ) + + def configure_underlying_network(self, configuration): + """ + The underlying network for the tunnel consists of the Ethernet + devices on the matched hosts. + """ + host1 = self.matched.host1 + host2 = self.matched.host2 + + for i, device in enumerate([host1.eth0, host2.eth0]): + device.ip_add("192.168.200." + str(i + 1) + "/24") + device.up() + configuration.test_wide_devices.append(device) + + configuration.tunnel_endpoints = (host1.eth0, host2.eth0) + + def create_tunnel(self, configuration): + """ + One L2TP tunnel is configured on both hosts using the + :any:`L2TPManager`. Each host configures one L2TP session for the + tunnel. IPv4 addresses are assigned to the l2tp session devices. + """ + endpoint1, endpoint2 = configuration.tunnel_endpoints + host1 = endpoint1.netns + host2 = endpoint2.netns + ip_filter = {"family": AF_INET} + endpoint1_ip = endpoint1.ips_filter(**ip_filter)[0] + endpoint2_ip = endpoint2.ips_filter(**ip_filter)[0] + + host1.l2tp = host1.init_class(L2TPManager) + host2.l2tp = host2.init_class(L2TPManager) + + host1.l2tp.create_tunnel( + tunnel_id=1000, + peer_tunnel_id=1000, + encap=self.params.l2tp_encapsulation, + local=str(endpoint1_ip), + remote=str(endpoint2_ip), + udp_sport=5000, + udp_dport=5000, + ) + host2.l2tp.create_tunnel( + tunnel_id=1000, + peer_tunnel_id=1000, + encap=self.params.l2tp_encapsulation, + local=str(endpoint2_ip), + remote=str(endpoint1_ip), + udp_sport=5000, + udp_dport=5000, + ) + host1.l2tp_session1 = L2TPSessionDevice( + tunnel_id=1000, + session_id=2000, + peer_session_id=2000, + ) + host2.l2tp_session1 = L2TPSessionDevice( + tunnel_id=1000, + session_id=2000, + peer_session_id=2000, + ) + + for device in [host1.l2tp_session1, host2.l2tp_session1]: + device.up() + + ip1 = "10.42.1.1/8" + ip2 = "10.42.1.2/8" + host1.l2tp_session1.ip_add((ip1, ip2)) + host2.l2tp_session1.ip_add((ip2, ip1)) + + configuration.tunnel_devices.extend([host1.l2tp_session1, host2.l2tp_session1]) + + def test_wide_deconfiguration(self, config): + for host in [self.matched.host1, self.matched.host2]: + host.l2tp.cleanup() + + super().test_wide_deconfiguration(config) + + def generate_ping_endpoints(self, config): + """ + The ping endpoints for this recipe are simply the tunnel endpoints + + Returned as:: + + [PingEndpoints(self.matched.host1.l2tp_session1, self.matched.host2.l2tp_session1)] + """ + return [ + PingEndpoints( + self.matched.host1.l2tp_session1, self.matched.host2.l2tp_session1 + ) + ] + + def get_packet_assert_config(self, ping_config): + return None
lnst-developers@lists.fedorahosted.org
-
Jan Tluka -
Ondrej Lichtner