Mon, May 10, 2021 at 12:30:42PM CEST, jtluka(a)redhat.com wrote:
Fri, May 07, 2021 at 03:50:15PM CEST, olichtne(a)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(a)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(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...
>> Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
>
_______________________________________________
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...
Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure