-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve autopkgtest stability with systemd 253 & iproute 6.4 #377
Conversation
The RPM build CI failure on fedora:rawhide seems unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just left some non-blocking questions/suggestions.
My main question was: does the iproute2 change affect other parts of the code, like netplan status?
@@ -278,6 +279,15 @@ def start_dnsmasq(self, ipv6_mode, iface): | |||
else: | |||
self.poll_text(dnsmasq_log, 'DHCP, IP range') | |||
|
|||
def iface_json(self, iface: str) -> dict: | |||
'''Return iproute2's (detailed) JSON representation''' | |||
out = subprocess.check_output(['ip', '-j', '-d', 'a', 'show', 'dev', iface], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: should we handle a possible call against an interface that doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's necessarily needed. This is only part of the integration tests, so all we could do is: bail out.
If the interface doesn't exist, this will crash with an error message, too, as-is.
IMHO, no need to over-engineer this, but rather keep it small and simple in the test framework. All the input is controlled, as it originates from our own test scripts, not random user input.
@@ -278,6 +279,15 @@ def start_dnsmasq(self, ipv6_mode, iface): | |||
else: | |||
self.poll_text(dnsmasq_log, 'DHCP, IP range') | |||
|
|||
def iface_json(self, iface: str) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: does this change affect other parts of netplan, like netplan status?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. netplan status
has been using iproute2's JSON integration from the start. This change only affects the base class of Netplan's integration tests.
But we want to migrate more parts of our integration tests to use this JSON interface. As other issues of the same type might occur in the future when we keep parsing the CLI output.
Starting with the systemd v253 update NetworkManager started taking implicit control of certain interfaces, blocking itself (after a restart) from managing those interface through its configure netplan-* connection profile. https://bugs.debian.org/1039071
iproute2 v6.4 changed its CLI output, the JSON output is expected to stay stable, so we should migrate to using iface_json() over time. https://bugs.debian.org/1040004
iproute2 v6.4 changed its CLI output, the JSON output is expected to stay stable, so we should migrate to using iface_json() over time. https://bugs.debian.org/1040004
Thank you! I rebased and addressed your comments. I think this is ready to be merged, once CI is green. |
Description
https://bugs.debian.org/1039071
https://bugs.debian.org/1040004
Also, resolves a race condition where the following text is already gone from the buffer at the time when we check the assertions. We see such failures a lot in the Ubuntu autopkgtests:
Checklist
make check
successfully.make check-coverage
).