Fix allow classes filtering + hotplug #235
Conversation
``` /etc/network/interfaces auto eth0 iface eth0 ``` current ifupdown2 behaviour: ``` ifquery -l --allow=auto <no result> ``` ifupdown1 behaviour: ``` ifquery -l --allow=auto eth0 ``` Code taken from: CumulusNetworks#201 Fix CumulusNetworks#201
Currently, when ifquery list/ifup/idown is used with allow=class + ifaces list, ifupdown2 is merging list of all interfaces of the class + the ifaces list. ifupdown1 behaviour is to filter the ifaces list with only the allowed class. Also current ifupdown2 man page :"--allow CLASS ignore non-"allow-CLASS" interfaces" ``` /etc/network/interfaces allow-hotplug eth0 auto eth0 iface eth0 auto eth1 iface eth1 ``` Current: ``` ifquery -l --allow=hotplug eth0 eth1 eth0 eth0 eth1 ifup --allow=hotplug eth1 -d debug: scheduling '['pre-up', 'up', 'post-up']' for ['eth0', 'eth1'] ``` After this patch, like ifupdown1: ``` ifquery -l --allow=hotplug eth0 eth1 eth0 ifup --allow=hotplug eth1 -d error: main exception: no ifaces found matching given allow lists ``` Fixes CumulusNetworks#206
- ifup/ifdown : remove start of ifup@iface systemd service - remove check of allow=hotplug with ifquery as we already filter in ifup/ifdown - remove check_program of ifup/ifdown, as ifup/ifdown are provided in ifupdown2 package too. Fixes CumulusNetworks#213 Fixes https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=988050
|
@julienfortin Do you have time to look at this ? It's fixing 3 issues. |
|
@aderumier Looks good to me. Could you please post some logs (+ config examples) of the differences before/after your patch? |
|
@julienfortin yes, sure, I'll send more examples and log this week. |
|
Hi, |
| filtered_ifacenames = [i for i in list(self.ifaceobjdict.keys()) | ||
| # filter non allowed classes interfaces | ||
|
|
||
| filtered_ifacenames = [i for i in list(ifacenames) |
There was a problem hiding this comment.
You should not apply list over ifacenames if it's already a list and if ifacenames is a generator the code is wrong as we´ll get an empty generator on the second iteration.
Either ifacenames might be a generator or not, the code must follow this principle.
# here ifacename is a list we do not need casting
# nor bothering with iterator duplication
filtered_ifacenames = [i for i in ifacenames ...]
or (as a simple example with a generator)
ifacename_list = list(ifacenames)
filtered_ifacenames = [i for i in ifacename_list ... ]
# ...
for intf in ifacename_list:
# ...
(you can also use tee to loop twice or just make one loop over filtered_ifacenames: https://docs.python.org/3/library/itertools.html#itertools.tee)
There was a problem hiding this comment.
thanks. I'll try to rework patch this week.
| if "auto" not in self.allow_classes: | ||
| self.allow_classes["auto"] = [] |
There was a problem hiding this comment.
a simpliest way of doing the same is:
| if "auto" not in self.allow_classes: | |
| self.allow_classes["auto"] = [] | |
| self.allow_classes.setdefault('auto', []) |
| r = utils.parse_iface_range(a) | ||
| if r: | ||
| if len(r) == 3: | ||
| # eg swp1.[2-4], r = "swp1.", 2, 4) | ||
| for i in range(r[1], r[2]+1): | ||
| self.auto_ifaces.append('%s%d' %(r[0], i)) | ||
| ifname = '%s%d' %(r[0], i) | ||
| self.auto_ifaces.append(ifname) | ||
| self.allow_classes["allow"].append(ifname) | ||
| elif len(r) == 4: | ||
| for i in range(r[1], r[2]+1): | ||
| # eg swp[2-4].100, r = ("swp", 2, 4, ".100") | ||
| self.auto_ifaces.append('%s%d%s' %(r[0], i, r[3])) | ||
| ifname = '%s%d%s' %(r[0], i, r[3]) | ||
| self.auto_ifaces.append(ifname) | ||
| self.allow_classes["auto"].append(ifname) | ||
| self.auto_ifaces.append(a) | ||
| self.allow_classes["auto"].append(a) |
There was a problem hiding this comment.
Also 😉 I would like to suggest changing the existing code here in order to enhance its readability. Take a look at the code snippet below:
r = utils.parse_iface_range(a)
if r:
prefix, start, end = r[0], r[1], r[2] + 1
suffix = '' if len(r) <= 3 else r[3]
ifnames = [f'{prefix}{i}{suffix}' for i in range(start, end)]
self.auto_ifaces.extend(ifnames)
self.allow_classes["allow"].extend(ifnames)
self.auto_ifaces.append(a)
self.allow_classes["auto"].append(a)Hope to see this PR being merged. :-)
There was a problem hiding this comment.
Please note that this is just an example and it only works for the first chunk of allow_classes['allow']. It won't function as it did before. I don't fully understand why we should extend the "auto" class when there is a suffix, and the "allow" class when there isn't. However, I haven't delved deeply into the code yet.
|
I'm currently working on this PR, and I'm encountering difficulties with some of the changes. Firstly, I believe the approach in the networkinterfaces is incorrect. If "auto" and "allow-auto" are aliases, they should be pointing to each other and using the same block of code, rather than adding specific modifications for some cases. Secondly, appending to the allow_classes seems more like a monkey patch and may not demonstrate a thorough understanding of the codebase (not saying that I do, but I'm trying to :) ). Introducing an "allow" named class might also create conflicts with an actual class named "allow" (e.g., allow-allow). Thirdly, the changes in the ifupdownmain._get_filtered_ifacenames_with_classes make ifquery unaware of ifacenames when no -l is provided. This means that ifquery --allow=auto won't work anymore. Considering these points, I believe it would be better to create another PR. You can find a commit for now at this link: commit link. Edit: blame me for third point, I did not understood that ifupdown is doing what you did. my bad. But if we do wish to make thing like ifupdown we should as well change the behavior of |
|
Hi, But fell free to open a new PR :) |
Hi,
currently ifquery/ifup/ifdown --allow= don't work like ifupdown1.
It should filter interfaces when an interfaces list is provided. (as in ifupdown2 man documentation).
This patch serie also add missing udev hotplug script.