providers/proxmoxve: Add Proxmox VE provider #1790

b- · 2024-01-20T01:33:35Z

This is just ripped from providers/ibmcloud with strings changed, and I honestly didn't try building yet (as I wanted to just upload something before I forget to get the ball rolling...) but it should work, at least in a basic sense, because the ibmcloud image works on Proxmox VE as-is!

...with a few fun caveat: ignition must be supplied as cloud-init (nocloud) data. This means that to do it "the Proxmox way" you need to:

Enable snippets on a Proxmox storage provider that supports them (such as a local folder or an NFS mount).
Upload your Ignition file to the snippets storage (for example, /var/lib/vz/snippets/example.ign if you simply enable Snippets on the stock local filesystem storage provider)
Use the qm cli command, or the Proxmox VE API, to set custom cloud-init data to the uploaded ignition file (for example, ssh root@pve.example.com qm set 30001 -cicustom user=local:snippets/butane.ign

OR

Create an ISO file with the volume label cidata and a file /user-data with the contents of your ignition data. (This is manually creating the cloud-init ISO, and may work on other hypervisors with a proxmoxve image)
Upload the ISO file to ISO storage for your Proxmox server
Set an optical drive on your virtual machine to that ISO file.

The above manual way can be automated in Go by consuming the go-proxmox module and calling the VirtualMachine.CloudInit() method, as done in my experimental gomox cli utility

b- · 2024-01-20T02:07:36Z

Related issues: coreos/fedora-coreos-tracker#736 and coreos/fedora-coreos-tracker#1652

This uses coreos/afterburn#1023 and coreos/ignition#1790 to provide Proxmox images. This pulls in flatcar/bootengine#91 and flatcar/init#115 to run afterburn for hostname, network, SSH key, and metadata attribute setup. The afterburn support for the SSH key and hostname parses the user-data when it's cloud-init. The coreos-cloudinit support is not there but can be added in addition: We need to add a new provider that varies from the existing config drive support because the file is called user-data and not user_data, and it needs to look for a filesystem label cidata and not config-2.

arcln · 2024-05-29T16:48:15Z

After trying this PR, it turns out that the proxmoxve ID is not recognized by ignition. That is because the proxmox provider code is not imported in

ignition/internal/register/providers.go

Line 15 in 32221a9

package register

I cannot push to this PR, can someone please add the missing import and build again?

Thanks

This uses coreos/afterburn#1023 and coreos/ignition#1790 to provide Proxmox images. This pulls in flatcar/bootengine#91 and flatcar/init#115 to run afterburn for hostname, network, SSH key, and metadata attribute setup. The afterburn support for the SSH key and hostname parses the user-data when it's cloud-init. The coreos-cloudinit support is not there but can be added in addition: We need to add a new provider that varies from the existing config drive support because the file is called user-data and not user_data, and it needs to look for a filesystem label cidata and not config-2.

travier · 2024-06-05T10:08:14Z

Thanks, I pushed this one line fix.

This uses coreos/afterburn#1023 and coreos/ignition#1790 to provide Proxmox images. This pulls in flatcar/bootengine#91 and flatcar/init#115 to run afterburn for hostname, network, SSH key, and metadata attribute setup. The afterburn support for the SSH key and hostname parses the user-data when it's cloud-init. The coreos-cloudinit support is not there but can be added in addition: We need to add a new provider that varies from the existing config drive support because the file is called user-data and not user_data, and it needs to look for a filesystem label cidata and not config-2.

arcln · 2024-06-17T12:23:05Z

@travier I have successfully tested this, I think it can be merged

tormath1

LGTM and it has been tested on Flatcar. Any chance to add a changelog entry:

ignition/docs/release-notes.md

Line 11 in b2ea352

### Features

?

tormath1 · 2024-06-20T12:38:23Z

internal/providers/proxmoxve/proxmoxve.go

+// The OpenStack provider fetches configurations from the userdata available in
+// both the config-drive as well as the network metadata service. Whichever
+// responds first is the config that is used.
+// NOTE: This provider is still EXPERIMENTAL.


I'm not sure this is accurate. It might come from copy-paste?

@arcln can you have a quick look ?

I was the one who copy-pasted this, and my rationale for not updating the comment was that the same vestigial OpenStack comment lives on inside internal/providers/ibmcloud/ibmcloud.go

I probably should have updated it. I'm pretty sure this Proxmox provider doesn't attempt to look for a network server.

Ah, @b- legal stuff is always a bit spooky, from my reading I believe that keeping the original licensing note to be accurate.

prestist · 2024-06-26T15:22:58Z

Looks like all we need is to add a release note entry and it will be gtg! Thank you everyone for your hardwork!

abuisine · 2024-06-27T11:26:15Z

Can we give a hand on this to accelerate ?

jlebon · 2024-06-27T19:23:36Z

internal/providers/proxmoxve/proxmoxve.go

+	dispatch := func(name string, fn func() ([]byte, error)) {
+		raw, err := fn()
+		if err != nil {
+			switch err {
+			case context.Canceled:
+			case context.DeadlineExceeded:
+				f.Logger.Err("timed out while fetching config from %s", name)
+			default:
+				f.Logger.Err("failed to fetch config from %s: %v", name, err)
+			}
+			return
+		}
+
+		data = raw
+		cancel()
+	}
+
+	go dispatch(
+		"config drive (cidata)", func() ([]byte, error) {
+			return fetchConfigFromDevice(f.Logger, ctx, filepath.Join(distro.DiskByLabelDir(), deviceLabel))
+		},
+	)
+
+	<-ctx.Done()
+	if ctx.Err() == context.DeadlineExceeded {
+		f.Logger.Info("cidata drive was not available in time. Continuing without a config...")
+	}


This construction is not desirable if we can avoid it. It's currently only used on platforms where it's possible for the drive to not exist and so we have to time out. The core problem is that it's inherently racy to wait around for a drive to show up. So if we know that there will always be a drive, then we should just indefinitely wait for it.

fetchConfigFromDevice() is already looping waiting for the device to show up so we can call it directly from here without using a context (and get rid of the context and select there). See e.g. the powervs, nutanix, or kubevirt provider code for examples of this pattern.

Or: is it the case that on Proxmox VE, no config drive may be attached? (Doesn't seem like it from my understanding.)

I confirm that it is possible not to attach a drive. It actually is the default behavior.

Hmm, I don't follow. If Proxmox uses the drive the pass its own metadata, doesn't it need to provide it even if the user didn't provide any user-data? Or are you saying if users use the lower-level APIs it's possible?

But anyway, that's enough to require a timeout in that case.

Let me clarify, you can face :

no drive at all attached to the guest VM

a drive containing cloud-config as user data

a drive containing ignition json as user data

jlebon · 2024-06-27T19:27:08Z

internal/providers/proxmoxve/proxmoxve.go

+		return nil, nil
+	}
+
+	return os.ReadFile(filepath.Join(mnt, cidataPath))


Don't we need to implement the "if the user-data is a cloud-config, then ignore it" here?

@arcln would have a better memory than mine, but I think we did implement this in another PR and that it was considered useless as ignition does verify if the content is a valid ignition configuration.

I don't understand how this can work. ParseConfig will error out because it's not a valid Ignition config and Ignition will exit nonzero, which will break boot.

In the other PR, it looks like we tried to workaround this by just ignoring errors completely, but that doesn't seem right either. We should explicitly check if the first line is #cloud-config, if yes, gracefully ignore, if no, then try to parse as an Ignition config and fail if it's not.

Again, I am not sure about this, let's wait for @arcln feedback

b- mentioned this pull request Jan 20, 2024

Implement Proxmox VE (proxmoxve) Support coreos/fedora-coreos-tracker#1652

Open

15 tasks

jlebon mentioned this pull request Apr 12, 2024

Proxmox VE provider #1844

Closed

3 tasks

arcln mentioned this pull request Apr 24, 2024

OEM: Provide Proxmox images flatcar/scripts#1783

Open

2 tasks

feat(proxmoxve): initial commit

3051bc8

travier force-pushed the feat/proxmoxve branch from 0571efd to 3051bc8 Compare June 5, 2024 09:57

tormath1 reviewed Jun 20, 2024

View reviewed changes

jlebon reviewed Jun 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

providers/proxmoxve: Add Proxmox VE provider #1790

providers/proxmoxve: Add Proxmox VE provider #1790

b- commented Jan 20, 2024 •

edited

Loading

b- commented Jan 20, 2024 •

edited

Loading

arcln commented May 29, 2024

travier commented Jun 5, 2024

arcln commented Jun 17, 2024

tormath1 left a comment

tormath1 Jun 20, 2024

abuisine Jun 21, 2024

b- Jun 21, 2024

prestist Jun 21, 2024

prestist commented Jun 26, 2024

abuisine commented Jun 27, 2024

jlebon Jun 27, 2024

abuisine Jun 28, 2024

jlebon Jun 28, 2024

abuisine Jun 28, 2024

jlebon Jun 27, 2024

abuisine Jun 28, 2024

jlebon Jun 28, 2024

abuisine Jun 28, 2024

providers/proxmoxve: Add Proxmox VE provider #1790

Are you sure you want to change the base?

providers/proxmoxve: Add Proxmox VE provider #1790

Conversation

b- commented Jan 20, 2024 • edited Loading

b- commented Jan 20, 2024 • edited Loading

arcln commented May 29, 2024

travier commented Jun 5, 2024

arcln commented Jun 17, 2024

tormath1 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

prestist commented Jun 26, 2024

abuisine commented Jun 27, 2024

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

b- commented Jan 20, 2024 •

edited

Loading

b- commented Jan 20, 2024 •

edited

Loading