Skip to content
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

Proxmox VE provider #1844

Closed
wants to merge 2 commits into from
Closed

Proxmox VE provider #1844

wants to merge 2 commits into from

Conversation

arcln
Copy link
Contributor

@arcln arcln commented Mar 28, 2024

Hello,

This PR implements the proxmoxve provider.

The provider mounts the cloud-init drive and parses the user-data file assuming it is an ignition config.

If the file is not an ignition config, it will not fail. It will just ignore it and say that the config is empty.
That is because Proxmox VE will create a user-data by default containing some cloud-init config in YAML.
This config will be picked up by afterburn anyway, so we don't need ignition to crash for that.

This PR is the missing puzzle piece to provide Proxmox VE images for Flatcar (see flatcar/scripts#1783)

Whats missing for the PR to be ready :

  • tests
  • docs and changelog
  • commit formatting

I will work on that soon.

@prestist
Copy link
Collaborator

Hello! I just noticed you had "draft" in your PR title. You can make this a draft PR rather then a full on pr. and when ready you can set it to be no longer a draft. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request#converting-a-pull-request-to-a-draft

}

config, report, err := util.ParseConfig(f.Logger, data)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I think we can keep it in line with other providers and drop this by check and directly return util.ParseConfig(f.Logger, data). This should work because at least in Flatcar the cloud-init detection is present and will result in Ignition skipping this. I don't know how FCOS with vanilla Ignition handles unknown user data content and what the desired behavior there is - e.g., maybe it makes sense to require the user to pass an empty config because the default cloud-init contents won't be supported.

if err != nil {
// Proxmox VE will populate user-data with a cloud-init YAML config by default.
// If such config is present, we should not return an error,
// and instead just ignore it and let Afterburn pick it up later.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About afterburn supporting parts of the cloud-init config: I have left a comment on the afterburn PR and I think it would be more consistent and less controversial if such a feature is not introduced. To get this merged without a large discussion I would recommend to stick to the established behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Pothos, cloud-init is really the by default behavior of ProxmoxVE, and is even the only supported option through the GUI.
I understand the preference for minimal disturbance, but these would basically mean that afterburn/ignition based OSes will not support ProxmoxVE. Or at the very least that API only configuration is supported.

Could we further discuss this on the Afterburn PR ?
My understanding is that this Ignition PR to support ProxmoxVE makes sense from your point of view ?

@arcln arcln marked this pull request as draft April 2, 2024 09:41
@arcln arcln changed the title Draft: Proxmox VE provider Proxmox VE provider Apr 2, 2024
@jlebon
Copy link
Member

jlebon commented Apr 12, 2024

See also #1790. The approach here was suggested in coreos/fedora-coreos-tracker#1652 (comment) also and... maybe we'll have to do that but yuck it's really unfortunate. Can we keep discussing in coreos/fedora-coreos-tracker#1652? Let's agree on the final approach there and then make sure we don't have multiple people working on the Ignition part.

@arcln
Copy link
Contributor Author

arcln commented Apr 23, 2024

After taking a closer look, #1790 is the same PR as this one and is older. I am closing this pull request as it is redundant. We will continue to support and discuss implementation in #1790.

@abuisine
Copy link

Hi ! We are waiting for an ignition release in order to include ignition's proxmox support in Flatcar's OS.
Do you have any visibility on the next release date ?
Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants