-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cleanup Vultr support #987
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
Merged
+50
−95
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
f085a9d
Cleanup and improve Vultr support
2d6d354
Merge remote-tracking branch 'Canonical/main' into vultr-cleanup
016db0d
Merge pull request #17 from eb3095/vultr-cleanup
ddymko 07b1643
Merge branch 'main' into vultr-cleanup
eb3095 27b0651
Merge branch 'main' into vultr-cleanup
eb3095 78798da
Merge branch 'main' into vultr-cleanup
eb3095 b4c67a8
Merge branch 'main' into vultr-cleanup
eb3095 7f823bf
Merge branch 'main' into vultr-cleanup
eb3095 49cf2e3
Fix instanceid and store entire md
eb3095 ae3469c
Simplify logic
eb3095 1bd799b
Merge branch 'main' into vultr-cleanup
eb3095 b5b73ff
Merge branch 'main' into vultr-cleanup
eb3095 0306180
Merge branch 'main' into vultr-cleanup
eb3095 ffe48f4
Merge branch 'main' into vultr-cleanup
eb3095 409242b
Merge branch 'main' into vultr-cleanup
eb3095 9170d33
Merge branch 'main' into vultr-cleanup
eb3095 dd35a9b
Merge branch 'main' into vultr-cleanup
eb3095 829985b
Merge branch 'main' into vultr-cleanup
eb3095 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This change int types from dict -> list makes me a little leery in this unittest as cloud-init handles those vendordata types differently depending on list vs dict processing in convert_vendordata. I'm not certain if this is just a unittest change for you or if Vultr platform actually provides as vendordata to the VM has changed type. Please provide
sudo cloud-init query vendordatato confirm for us what structure of data is really surfaced.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.
So basically before we were providing a json dict with defined endpoints for each script since we were doing little modifications here and there. With the new format we moved to just providing a json array of strings each being a specific script or config.
So how we provide it would be an array of strings, here I left it a dic to be be easily modifiable if things changed or needs to be adjusted and convert it to the expected format bellow. If you would prefer I just leave it in the format that is fed to cloud-init I can go ahead and make those changes.
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 for the clarification here. I think this is ok, but I really need to see the output of both
sudo cloud-init query vendordataand possiblysudo cloud-init query merged_cfg. Normally vendordata in datasources is structured such that it merges on top of (and overrides) config defaults that are originally present in /etc/cloud/cloud.cfg and /etc/cloud/cloud.cfg.d/*.cfg. Your format seems to diverge from this typical behavior for Datasources. Your first iteration of DatasourceVultr also seems to diverge from this format anyway, so I don't think you are regressing anything. But, I think we might have to consider how your are using vendordata because this seems a bit different than how almost all other platforms treat vendordata. The docs remind us that vendordata can be disabled by user-data and that vendordata shouldn't be "required" to make a system operational. I don't know if that necessarily applies to your case, but it's a tension we should be aware of here.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.
So, in terms of how we are using vendordata. We are using it pass preferred settings (updates, packages, basic settings) and settings that the user chose for deployment. Outside of the root password, nothing is actually required for the system to be operational.
We do add scripts in afterwards to handle specific tasks at boot such as configuring second drives, network optimizations that do not exist as options in the config, and misc other things. The reason we do this is because we offer a marketplace and custom ISO features that allow users to end up with an image not made by us that we have no insight into but we would like the same tweaks and customizations/features we have available for our own images to still be available in these cases with the least user action required. Cloud-init vendordata looked like a suitable way to provide the startup scripts we have traditionally used to users own images with no modification required.
I am aware that user-data overwrites this. I wasn't sure if it was merged in or if the presence of user-data overrided the existing vendordata altogether, and if this applied only to that actual cloud-init config or the entire array, including scripts. If I could get clarification on that it would greatly help! Regardless the vendordata only results in an optimal system at this current moment and the system should be functional whether or not it runs (if not inaccessible barring the user accounted for a password in user-data).
We were also looking at this as an avenue to using the widely available cloud images without having to worry about building them with our requirements and having a way to make changes for compatibility purposes to OS's that need it without needing to directly modify the image.
Let me know if you have any suggestions or comment on how we intended to use this here.
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.
@blackboxsw was wondering if I could get a follow up to this portion.
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.
This is a perfectly applicable use of vendor data. Thanks for the context.
Thank you. This too makes sense. I just setup a Vultr account to poke around to see the images you are hosting don't yet expose cloud-init to the end-user in the compute VM launched. :/ At some point if there is access to a Vultr instance under test with access to your metadata service I'd ❤️ to poke around for a few minutes to get my bearings, but that doesn't need to happen for this PR to land.
I also didn't grok initially that you were already packing a list in as vendordata_raw in the first iteration of Vultr datasource. So I got a bit prickly thinking that list wouldn't work for the vendordata handling, but I was wrong in that regard.
Correct, user-data provided #cloud-config values will overwrite/override specific #cloud-config values from vendor-data or meta-data, it will not blow away the whole of the vendordata list of "parts".
But, user-data can also complete disable honoring any vendordata at all by providing the following
If this is the case on a VM you'll see the log (and none of your vendor scripts would be run I believe.
2021-09-23 04:06:19,234 - stages.py[DEBUG]: vendordata consumption is disabled.If not set
enabled: falseby a user, I'd expect to see2021-09-23 04:13:26,355 - stages.py[DEBUG]: vendordata will be consumed. disabled_handlers=NoneAs long as your aren't getting WARNING logs in /var/log/cloud-init.log or /var/log/cloud-init-output.log for failures to handle the parts of your vendordata in the "init-network/consume-vendor-data" section of logs your should be in good shape here with providing both scripts and #cloud-config items in your vendordata_raw list.
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, I am not too worried about being able to disable it. I am aware of that and enjoy the idea the user always has an option to turn it off completely!
As for the account, I'll get you setup first thing. Just do me a favor and open a ticket from your account and request it be forwarded to the Marketplace queue.