-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
feat(vm): Add cloud-init network-config support #197
Conversation
700ba88
to
c4be509
Compare
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.
Hey @groggemans! Thanks for contributing this!
I have a minor feedback on the code, please see comments below.
But my main question is, have you had a chance to test this change in the provider on PVE?
I'm a bit concerned about how cloud-init network handling is implemented in the Proxmox, and my quick test didn't work (although I may have some other networking issues in my environment atm).
@@ -76,6 +76,7 @@ const ( | |||
dvResourceVirtualEnvironmentVMInitializationUserAccountPassword = "" | |||
dvResourceVirtualEnvironmentVMInitializationUserDataFileID = "" | |||
dvResourceVirtualEnvironmentVMInitializationVendorDataFileID = "" | |||
dvResourceVirtualEnvironmentVMInitializationNetworkConfigFileID = "" |
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.
Could we change "NetworkConfigFile" -> "NetworkDataFile" here and below, and argument network_config_file_id
-> network_data_file_id
as well?
I know it looks a bit out of context this way, but I'd like to keep consistency in naming with other cloud-init parts (at the lines above)
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.
Updated the code so it's using the network data naming!
I've tested the modification on my proxmox cluster and it worked as expected. I just provided only user_data_file_id
and network_data_file_id
parameters (as in the example) and it created the cloud-init drive with both of these files. Combining the network-config file with ip_config
or dns related configuration in the initialization
block is probably not going to work. (but also wouldn't really make sense)
Because we are not specifying any network related configuration on the PVE level, it isn't creating the network-config file, and we are free to provide it ourselves. The build in generation system still uses cloud-init network config v1, but I've been testing this with Ubuntu and thus provided my config in the v2 syntax. The nice thing is that we are no longer limited to what PVE supports as it's no longer managing the file in this case.
I did just notice that second runs are still a bit misbehaving and it wants to re-add the network_data_file_id
attribute again. So I might have forgotten something to prevent that behavior. (ignoring it does seem to give me a functioning system, so probably easy to solve)
While testing I also noticed that the tag ordering is also causing issues similar to #185, because pve is expecting them in alphabetical order.
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.
Awesome! I also found an error in my netplan config that caused issues on apply in my tests, so everything is working now.
I've added the new argument to the docs and also fixed the re-apply use case, so I think the PR is good to go.
I'll open a separate ticket for tags reordering, this bugs me too...
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.
Great, thank you!
c4be509
to
ea056d6
Compare
Adds the option to specify a
network-config
cloud-init file just like you can already specify user/vendor data.The currently provided
ip_config
blocks under theinitialization
block for the VM resource are rather limited, and implementing all possible network options and features would not really make sense. With this modifications users can write the cloud-init network config file themselves if they want to, supporting way more advanced network setups.Using it is the same as with the user/vendor data files;
The backend code already supported this, so I mainly had to expose the option to the user.
Community Note
Relates OR Closes #0000