-
Notifications
You must be signed in to change notification settings - Fork 815
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
net: Passthough v2 netconfigs in netplan systems #1650
Conversation
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 left two comments inline. Neither are strongly held though. Otherwise, looks good.
cloudinit/distros/__init__.py
Outdated
@@ -230,6 +236,16 @@ def _apply_network_from_network_config(self, netconfig, bring_up=True): | |||
def generate_fallback_config(self): | |||
return net.generate_fallback_config() | |||
|
|||
def _is_netplan(self) -> bool: |
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.
Since most of this method is doing the same thing as the beginnning of _write_network_state
, I think it might make more sense to fetch the renderer one time at the beginning of apply_network_config
and pass it where it's needed.
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.
Done.
cloudinit/distros/__init__.py
Outdated
@@ -241,7 +257,11 @@ def apply_network_config(self, netconfig, bring_up=False) -> bool: | |||
""" | |||
# This method is preferred to apply_network which only takes | |||
# a much less complete network config format (interfaces(5)). | |||
network_state = parse_net_config_data(netconfig) | |||
if self._is_netplan() and netconfig.get("version") == 2: |
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 like how the network rendering abstraction is leaking into the distro code here. Do you think it's possible to push this section of code down into either parse_net_config_data
or even into NetworkStateInterpreter
?
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.
Logic moved to NetworkStateInterpreter
. The downside is that we have to pass the renderer around, but it looks more cohesive in this way. Thanks for the comment.
Thanks, @TheRealFalcon, for the review. I have addressed the comments and this PR is ready to be reviewed. |
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.
Thanks @aciba90 , nice work here.
The isinstance
checks feel a little awkward. Long term, it might make sense for us to add a name property to the renderers and check that instead, but not required for this PR.
Proposed Commit Message
Additional Context
SC-1120
https://bugs.launchpad.net/cloud-init/+bug/1978543
Netplan Passthrough
Test Steps
Checklist: