Re: [PATCH v2 5/6] Devices.Device: modify _ip_add_one method to handle peer addresses

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 = 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),
  

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 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
     """
  

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