Add Vultr support #827

ddymko · 2021-03-03T00:38:39Z

Proposed Commit Message

    Add support for Vultr cloud.

    This PR adds in support so that cloud-init can run on instances 
    deployed on Vultr cloud. This was originally brought up in #628.

    Co-authored-by: Eric Benner <ebenner@vultr.com>

Additional Context

Greets from Vultr 👋

This PR adds in support so that cloud-init can run on instances deploy on Vultr instances. This was originally brought up in #628

Please let me know if there are any updates or additions that may need to be made.

Test Steps

Checklist:

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

ddymko · 2021-03-03T00:40:34Z

@smoser as per the other PR pinging you for review of this one

smoser

Thanks for the pull request.
I have some questions/comments in line. I guess the big things are:

avoid the use of globals.
lots of INFO that are DEBUG
some of the datasources have a 'main', which can be used for easy debugging. They're inconsistent, but for a developer its a useful way to poke around. see SmartOS or GCE for examples.
perhaps the vendor_script stuff that you're doing could make use of activate_datasource (see 9e904bb for an explanation).
if the instance_id is available locally anywhere (through dmi data?) then it would be great to have a check_instance_id method in the datasource.

Again, thanks for the PR. It will be great to have proper Vultr support in cloud-init.

smoser · 2021-03-03T13:42:53Z

cloudinit/sources/DataSourceVultr.py

+    'timeout': 2,
+    'wait': 2
+}
+CONFIG = BUILTIN_DS_CONFIG.copy()


i think we'd rather avoid the global here, and it is kind of confusing how it is used. What were you hoping to accomplish here?

smoser · 2021-03-03T13:44:50Z

cloudinit/sources/DataSourceVultr.py

+
+import cloudinit.sources.helpers.vultr as vultr
+
+LOGGER = log.getLogger(__name__)


use LOG for consistency that is the variable name we use everywhere else.

smoser · 2021-03-03T13:47:49Z

cloudinit/sources/DataSourceVultr.py

+            LOGGER.info("Machine is not a Vultr instance")
+            return False
+
+        LOGGER.info("Machine is a Vultr instance")


the majority of these log statements are more debug than info.

info is very sparingly used in cloud-init (it probably should be used more, and logging in cloud-init can definitely use an overhaul, but these are debug).

smoser · 2021-03-03T13:48:36Z

cloudinit/sources/DataSourceVultr.py

+        self.metadata_full = md
+        self.metadata['instanceid'] = self.metadata_full['instanceid']
+        self.metadata['local-hostname'] = re.sub(
+            r'\W+', '', self.metadata_full['hostname'])


the hostname from the metadata service has trailing whitespace ?

smoser · 2021-03-03T13:54:50Z

cloudinit/sources/helpers/vultr.py

+
+# Write vendor startup script
+def write_vendor_script(fname, content):
+    os.makedirs("/var/lib/scripts/vendor/", exist_ok=True)


this feels like it probably can be accomplished with generic vendor data... not needing a datasource specific solution.

other comments on the implementation, this is really just write_file

smoser · 2021-03-03T14:00:54Z

cloudinit/sources/helpers/vultr.py

+
+
+# Get kernel parameters
+def get_kernel_parameters():


util.get_cmdline()

smoser · 2021-03-03T14:05:22Z

cloudinit/sources/helpers/vultr.py

+
+
+# Close EphermalDHCP so its not left open
+def close_ephermeral():


the spelling is wrong here ephemeral not ephermeral.

smoser · 2021-03-03T14:09:34Z

cloudinit/sources/helpers/vultr.py

+    global METADATA
+
+    if not METADATA:
+        # Bring up interface in local


use a context for this rather htan the global EHP state.

I think you're basically just doing:

with EphemeralDHCPv4(connectivity_url=params['url']): v1 = fetch_metadata(params)

and that should take care of EPH entirely.

cloudinit/sources/helpers/vultr.py

smoser · 2021-03-03T14:13:57Z

cloudinit/sources/helpers/vultr.py

+
+    # Define vendor script
+    vendor_script = []
+    vendor_script.append("!/bin/bash")


append ?

and thats not even a proper shebang. odd.

ddymko · 2021-03-03T14:33:53Z

@smoser Thanks for the review...we'll start work on updating all of these issues/concerns

smoser · 2021-03-05T14:56:45Z

@ddymko inspired by your work here, i dusted off my vultr accout to build a https://github.com/cirros-dev/cirros release. (next you can add support to cirros for vultr 😉 )

Requested Changes

ddymko · 2021-03-05T17:58:36Z

@smoser I can't take the credit...most of the work was done by @Oogy and @eb3095.

As for Cirros please let us know if there is any way we could help!

smoser · 2021-03-08T20:14:26Z

tools/ds-identify

-CloudSigma CloudStack DigitalOcean AliYun Ec2 GCE OpenNebula OpenStack \
-OVF SmartOS Scaleway Hetzner IBMCloud Oracle Exoscale RbxCloud UpCloud"
+CloudSigma CloudStack DigitalOcean Vultr AliYun Ec2 GCE OpenNebula OpenStack \
+OVF SmartOS Scaleway Hetzner IBMCloud Oracle Exoscale RbxCloud upCloud"


you broke upcloud here.

smoser · 2021-03-08T20:27:27Z

cloudinit/sources/DataSourceVultr.py

+
+        LOG.debug("Machine is a Vultr instance")
+
+        config = vultr.generate_config(BUILTIN_DS_CONFIG)


why would you not use self.ds_config here ?

smoser · 2021-03-08T20:31:54Z

cloudinit/sources/DataSourceVultr.py

+    # Compare subid as instance id
+    def check_instance_id(self, sys_cfg):
+        if not vultr.is_vultr():
+            return None


this function should return a boolean. so return False here.

smoser · 2021-03-08T20:32:00Z

cloudinit/sources/DataSourceVultr.py

+
+        # Baremetal has no way to implement this in local
+        if vultr.is_baremetal():
+            return None


same. False.

smoser · 2021-03-08T20:34:10Z

cloudinit/sources/DataSourceVultr.py

+    @property
+    def network_config(self):
+        config = vultr.generate_network_config(BUILTIN_DS_CONFIG)
+        config_raw = json.dumps(config)


this seems unnecessarily loud to log the dumpped config here. but if you wanted to do it, just skip the 'config_raw' assignment and use 'json.dumps()` and do something like:

LOG.debug("Generated Network: %s", json.dumps(config))

smoser · 2021-03-08T20:35:04Z

cloudinit/sources/DataSourceVultr.py

+    config = vultr.generate_config(BUILTIN_DS_CONFIG)
+    sysinfo = vultr.get_sysinfo()
+
+    print(json.dumps(sysinfo, indent=1))


util.json_dumps() will pretty-print. also sorts keys and other htings that you probably wanted anyway.

smoser · 2021-03-08T20:35:47Z

cloudinit/sources/helpers/vultr.py

+            # Fetch the metadata
+            v1 = fetch_metadata(params)
+    except (NoDHCPLeaseError) as exc:
+        LOG.error("DHCP failed, cannot continue. Exception: %s",


did this really need a line break ? probably not.

ddymko · 2021-04-01T23:34:28Z

@eb3095 merged your latest changes

eb3095 · 2021-04-01T23:47:34Z

@smoser when you get around to that review I had a question about vendor-data and the network config. I am currently adding the network config to the vendor-data kind of like how user-data functions. Is this required? Do I need to even do that? Seems like the networking config is pulled in a separate function.

smoser · 2021-04-02T00:35:01Z

@smoser when you get around to that review I had a question about vendor-data and the network config. I am currently adding the network config to the vendor-data kind of like how user-data functions. Is this required? Do I need to even do that? Seems like the networking config is pulled in a separate function.

If I understand the question correctly, then the answer is 'no'. the only place that network_config has to be present is returned by DataSource.network_config()

Cleanup Cleanup Cleanup Cleanup Cleanup Cleanup

Cleanup

smoser · 2021-04-12T15:49:42Z

cloudinit/sources/DataSourceVultr.py

+        BUILTIN_DS_CONFIG['timeout'] = self.ds_cfg.get(
+            'timeout', BUILTIN_DS_CONFIG['timeout'])
+        BUILTIN_DS_CONFIG['wait'] = self.ds_cfg.get(
+            'wait', BUILTIN_DS_CONFIG['wait'])


I'm pretty sure these lines (32-39) are not needed.

>>> import cloudinit.util as util >>> util.mergemanydict([{}, BUILTIN_DS_CONFIG]) {'url': 'http://169.254.169.254', 'retries': 30, 'timeout': 2, 'wait': 2} >>> util.mergemanydict([{'url': 'http://1.1.1.1/foo'}, BUILTIN_DS_CONFIG]) {'url': 'http://1.1.1.1/foo', 'retries': 30, 'timeout': 2, 'wait': 2}

Am I wrong?

yeah, i'm pretty sure they are not required anymore. As you're just updating BUILTIN_DS_CONFIG. but since we do not use that global anywhere else (except the main, which won't have executed this code), the lines are not needed.

i think they were left over from when you were using a global more.

Indeed, just left over code I missed. Its removed now.

…oudinit

Remove excess lines

smoser

I really think I'm down to one change request ('interfaces' rather than 'md' as variable name).

thank you so much for your patience.

…oudinit

Cleanup variable usage in network configuration

smoser

OK. last request.

Can you squash everything and commit with the commit message in the PR?

I'm requesting that for 2 reasons:
a.) so i dont mess it up.
b.) I went to do this and saw:

Co-authored-by: Eric Benner <ebenner@vultr.com>
Co-authored-by: eb3095 <45504889+eb3095@users.noreply.github.com>

We'd like to have real human names in 'Author' and any other such references.

So in my head, ideally the commit would look like the following, but with
@ddymko 's @vultr email address if you could.

Author: David Dymko <dymkod@gmail.com>
Date: Some Date

    Add support for Vultr cloud.

    This PR adds in support so that cloud-init can run on instances 
    deployed on Vultr cloud. This was originally brought up in #628.

    Co-authored-by: Eric Benner <erics-vultr@>

ddymko · 2021-04-13T17:10:04Z

@smoser I can do that but wouldn't be it be easier to do that from github to squash and merge and update the message?

I also don't use @vultr email for github that won't be a problem will it?

smoser · 2021-04-13T17:49:45Z

@smoser I can do that but wouldn't be it be easier to do that from github to squash and merge and update the message?

well 2 things:
a.) when i hit 'squash and merge', i can't change the 'author'
b.) If you update the co-authored-by, i assume you will do it right (and have Eric's permission to do so). If I do it... who knows.

I also don't use @vultr email for github that won't be a problem will it?

i don't think so... although i guess you might lose the link to you if it does that based on known email addresses. I'm ok with you keeping the @gmail if you want.

ddymko · 2021-04-13T17:57:51Z

Yeah, I don't mind using the @gmail

As for the proposed commit message. I updated the message here:
#827 (comment)

I'll work on cleaning up the commits into a single one

Vultr support was added under canonical#827. This enables it by default in the debian package.

Vultr support was added under #827. This enables it by default in the debian package.

Vultr support was added under canonical#827. This enables it by default in the debian package.

ddymko added 5 commits March 1, 2021 13:46

merged in upstream

991444a

merge conflicts

584fd40

fixing merge issues

a787657

cleaning up readme

7431c4c

added ddymko to cla signers

e7ca197

smoser requested changes Mar 3, 2021

View reviewed changes

Eric Benner added 2 commits March 4, 2021 15:26

Requested Changes

fc769fc

Add to signers

c70c644

Oogy mentioned this pull request Mar 5, 2021

Requested Changes vultr/cloud-init#8

Merged

3 tasks

Eric Benner and others added 5 commits March 5, 2021 11:39

Make new requested changes

2d04eff

Make new requested changes

ca7149f

Make new requested changes

41476a0

Merge pull request #8 from eb3095/vultr-cloudinit

671ede5

Requested Changes

Merge branch 'master' into vultr-cloudinit

66813df

ddymko requested a review from smoser March 5, 2021 18:10

Merge branch 'master' into vultr-cloudinit

a4e69ca

smoser reviewed Mar 8, 2021

View reviewed changes

Eric Benner and others added 3 commits March 8, 2021 15:37

Fix upcload case

8f0c479

Merge remote-tracking branch 'upstream/master' into vultr-cloudinit

0f9e0e8

Make remaining changes

f7c9dca

Eric Benner and others added 3 commits April 2, 2021 12:03

Cleanup

6f1698e

Cleanup Cleanup Cleanup Cleanup Cleanup Cleanup

Merge pull request #14 from eb3095/vultr-cloudinit

5be0868

Cleanup

Merge branch 'master' into vultr-cloudinit

d2924aa

smoser reviewed Apr 12, 2021

View reviewed changes

Eric Benner and others added 4 commits April 12, 2021 13:42

Merge remote-tracking branch 'upstream/vultr-cloudinit' into vultr-cl…

9971254

…oudinit

Remove excess lines

70f4e9e

Merge pull request #15 from eb3095/vultr-cloudinit

db18ba1

Remove excess lines

Merge branch 'master' into vultr-cloudinit

e4b1348

smoser requested changes Apr 13, 2021

View reviewed changes

Eric Benner and others added 3 commits April 13, 2021 10:58

Merge remote-tracking branch 'upstream/vultr-cloudinit' into vultr-cl…

c7edc51

…oudinit

Cleanup variable usage for network generation

07e9f5a

Merge pull request #16 from eb3095/vultr-cloudinit

eab53d1

Cleanup variable usage in network configuration

smoser mentioned this pull request Apr 13, 2021

add Equinix Metal metadata service #680

Closed

3 tasks

smoser approved these changes Apr 13, 2021

View reviewed changes

smoser requested changes Apr 13, 2021

View reviewed changes

smoser approved these changes Apr 13, 2021

View reviewed changes

smoser merged commit 0ae0b1d into canonical:master Apr 13, 2021

smoser mentioned this pull request Apr 20, 2021

Add Vultr to default datasource_list. #879

Merged

smoser added a commit to smoser/cloud-init that referenced this pull request Apr 20, 2021

Add Vultr to default datasource_list.

d44f4e2

Vultr support was added under canonical#827. This enables it by default in the debian package.

smoser added a commit to smoser/cloud-init that referenced this pull request Apr 20, 2021

Add Vultr to default datasource_list.

27654fe

Vultr support was added under canonical#827. This enables it by default in the debian package.

smoser added a commit that referenced this pull request Apr 27, 2021

Add Vultr to default datasource_list. (#879)

4560240

Vultr support was added under #827. This enables it by default in the debian package.

ddymko deleted the vultr-cloudinit branch September 17, 2021 19:03

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 21.2 #3874

Closed

holmanb pushed a commit to holmanb/cloud-init that referenced this pull request Feb 7, 2024

Add Vultr to default datasource_list. (canonical#879)

1ad90aa

Vultr support was added under canonical#827. This enables it by default in the debian package.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Vultr support #827

Add Vultr support #827

ddymko commented Mar 3, 2021 •

edited

Loading

ddymko commented Mar 3, 2021

smoser left a comment

smoser Mar 3, 2021

smoser Mar 3, 2021

smoser Mar 3, 2021

smoser Mar 3, 2021

smoser Mar 3, 2021

smoser Mar 3, 2021

smoser Mar 3, 2021

smoser Mar 3, 2021

smoser Mar 3, 2021

ddymko commented Mar 3, 2021

smoser commented Mar 5, 2021

ddymko commented Mar 5, 2021

smoser Mar 8, 2021

smoser Mar 8, 2021

smoser Mar 8, 2021

smoser Mar 8, 2021

smoser Mar 8, 2021

smoser Mar 8, 2021

smoser Mar 8, 2021

ddymko commented Apr 1, 2021

eb3095 commented Apr 1, 2021

smoser commented Apr 2, 2021

smoser Apr 12, 2021

smoser Apr 12, 2021

eb3095 Apr 12, 2021

smoser left a comment

smoser left a comment •

edited

Loading

ddymko commented Apr 13, 2021 •

edited

Loading

smoser commented Apr 13, 2021

ddymko commented Apr 13, 2021


		import cloudinit.sources.helpers.vultr as vultr

		LOGGER = log.getLogger(__name__)



		# Close EphermalDHCP so its not left open
		def close_ephermeral():


		LOG.debug("Machine is a Vultr instance")

		config = vultr.generate_config(BUILTIN_DS_CONFIG)

Add Vultr support #827

Add Vultr support #827

Conversation

ddymko commented Mar 3, 2021 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Checklist:

ddymko commented Mar 3, 2021

smoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddymko commented Mar 3, 2021

smoser commented Mar 5, 2021

ddymko commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddymko commented Apr 1, 2021

eb3095 commented Apr 1, 2021

smoser commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoser left a comment

Choose a reason for hiding this comment

smoser left a comment • edited Loading

Choose a reason for hiding this comment

ddymko commented Apr 13, 2021 • edited Loading

smoser commented Apr 13, 2021

ddymko commented Apr 13, 2021

ddymko commented Mar 3, 2021 •

edited

Loading

smoser left a comment •

edited

Loading

ddymko commented Apr 13, 2021 •

edited

Loading