fix(NetworkManager): Fix network activator#5620
Conversation
| return _alter_interface(cmd, device_name) | ||
|
|
||
| @classmethod | ||
| def bring_up_interfaces(cls, device_names: Iterable[str]) -> bool: |
There was a problem hiding this comment.
Where does this get called from? Given that this is a new function I assume there are no existing calls to this function?
There was a problem hiding this comment.
There's a base class impl here: https://github.com/canonical/cloud-init/blob/main/cloudinit/net/activators.py#L74
so this will override it for network manager.
It gets called from https://github.com/canonical/cloud-init/blob/main/cloudinit/distros/__init__.py#L466
a-dubs
left a comment
a-dubs
left a comment
There was a problem hiding this comment.
Left one question regarding how this new code is used
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
It's ok as-is, but I'm not sure we need the extra systemctl call.
We should also wait to see if RH QA can test this code path.
| "Expected NetworkManager SubState=running, but detected: %s", | ||
| state, | ||
| ) | ||
| return _alter_interface( |
There was a problem hiding this comment.
was there supposed to be an else here? The final service code only does the reload or restart if network manager is already started.....though if it isn't running already we have much bigger problems.
There was a problem hiding this comment.
was there supposed to be an
elsehere? The final service code only does the reload or restart if network manager is already started.....though if it isn't running already we have much bigger problems.
The intent here was just to warn about the bigger problems in the case that it isn't already running.
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.) |
github-actions
Bot
added
the
stale-pr
They are in agreement with this approach. I'd like to make a small change, however, based on the reported log from the testing it looks like we need an .rstrip() to chomp a newline to avoid this unnecessary warning. |
holmanb
force-pushed
the
holmanb/networkmanager-activator
branch
from
38e5452 to
7de64d3
Compare
Reload NetworkManager configuration rather than bringing up interfaces. This enables NetworkManager to configure interfaces which are not yet available in userspace, and fixes a race that was previously worked around in a service file.
holmanb
force-pushed
the
holmanb/networkmanager-activator
branch
from
7de64d3 to
122643a
Compare
|
I just rebased on main due to a conflict and applied the strip which led to a couple of minor test refactors. @TheRealFalcon would you mind re-reviewing please (assuming tests pass)? |
Reload NetworkManager configuration rather than bringing up interfaces. This enables NetworkManager to configure interfaces which are not yet available in userspace, and fixes a race that was previously worked around in a service file.
3 participants
Proposed Commit Message
Additional Context
#5512