Skip to content

Add proxmox cloud provider #1352

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

Closed
wants to merge 6 commits into from

Conversation

dermorz
Copy link
Contributor

@dermorz dermorz commented Jul 11, 2022

What this PR does / why we need it:
This is the first part of implementing proxmox as a new cloud provider in KKP.

Reference: kubermatic/kubermatic#9406

Which issue(s) this PR fixes (optional, in fixes #<issue number> format, will close the issue(s) when PR gets merged):
Fixes #1325

Special notes for your reviewer:
Proxmox support is experimental and more a tech demo than anything close to being production ready. There are quite a few assumptions made about the Proxmox setup which need to be handled later on with follow-up tasks making things configurable.

TODO:

  • Initial Testing
  • Create MachineDeployment example yaml
  • Improved docs
  • Add release notes to PR
  • Some refactoring/clean-up (deferred to after succesful testing)
  • Node selection: Random pick from nodes with sufficient resources
  • Cloud-init user-data

Optional Release Note:

Proxmox VE cloud provider support (experimental)

@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. sig/osm Denotes a PR or issue as being assigned to SIG OSM. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 11, 2022
@dermorz dermorz force-pushed the add-proxmox-cloud-provider branch from 4c93f13 to fe23392 Compare July 14, 2022 14:12
@dermorz dermorz force-pushed the add-proxmox-cloud-provider branch from 0206312 to e3b21de Compare July 27, 2022 10:37
@kubermatic-bot kubermatic-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 29, 2022
@dermorz dermorz force-pushed the add-proxmox-cloud-provider branch from 41f7de3 to 9c20603 Compare August 29, 2022 10:11
@dermorz dermorz force-pushed the add-proxmox-cloud-provider branch from c389120 to eb6aed2 Compare September 8, 2022 13:00
@dermorz dermorz requested review from embik September 8, 2022 13:16
@dermorz dermorz changed the title WIP: Add proxmox cloud provider Add proxmox cloud provider Sep 8, 2022
@kubermatic-bot kubermatic-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2022
@dermorz
Copy link
Contributor Author

dermorz commented Sep 8, 2022

/retest

@dermorz
Copy link
Contributor Author

dermorz commented Sep 9, 2022

/retest-required

Copy link
Member

@SimonTheLeg SimonTheLeg left a comment

Choose a reason for hiding this comment

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

tiny doc nitpicks from my side.


For VM templates to be available on all nodes, they need to be added to the `ha-manager`.

Example for creating a VM template:
Copy link
Member

Choose a reason for hiding this comment

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

tiny nitpick: I think a link to the qm tool would be useful. Maybe it could be easily integrated like this

Suggested change
Example for creating a VM template:
Example for creating a VM template using [qm](https://pve.proxmox.com/pve-docs/qm.1.html):

docs/proxmox.md Outdated
Comment on lines 87 to 94
Proxmox currently does not support the upload of "snippets" via API, but these snippets are used for
cloud-init user-data which are required for the machine-controller to function. This provider
implementation needs to copy the generated user-data yaml file to every proxmox node where a VM is
created or migrated to.

* A storage needs to be enabled for content `snippets` (e.g. `local`)
* SSH private key of a user that exists on all nodes and has write permission to the path where
snippets are stored (e.g. `/var/lib/vz/snippets`)
Copy link
Member

Choose a reason for hiding this comment

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

Reading this as a first time reader of the guide: I am not 100% sure if there is a call to action here for me. What I mean by that is: do I as the end-user have to do something?

@mfranczy
Copy link
Contributor

mfranczy commented Sep 9, 2022

@dermorz please update the what-works section by adding there Proxmox and highlight that it's a technology preview. Could you as well change KubeVirt (experimental) to KubeVirt (Technology Preview)?

Maybe this section needs to be refreshed a little bit to be more readable, but probably not in that PR.

@dermorz
Copy link
Contributor Author

dermorz commented Sep 12, 2022

/retest-required

Add scaffold for proxmox cloud provider

TODO: Implement remaining interface methods

TerminalError on Validate()

First implementation of Create (proxmox provider)

Return vm ip addresses

Complete Server return value

Implement provider.Cleanup() for proxmox

Slight refactoring

Extend config values and start extending validation

JSON roundtrip to cast to typed response

Some refactoring

Replace CreateVm with CloneVm(template)

Wording on errors

Implement provider.Get() for proxmox

Drop unused return values (YAGNI)

Use wrapper func that returns the expected error

Docstring for Server struct

Implement Metrics and Migrate methods

Some refactoring

Implement last method of Provider interface

Improve proxmox docs

Minor code styling

Fix proxmox provider import

Worship the linter gods

Add ssh upload of cloud-init userdata

I should run the linter before pushing

Adjust example vm template creation

Add default value for DiskName

Rename ENV for api endpoint

This was stupid :)

Fix copy/paste error

.yaml extension for userdata files

Start/Stop VM after/before creating/deleting

Resize to value of DiskSizeGB

Lint: Proper error wrapping

Strange that this is not set in the response…

Names of VMs/templates are ambiguous :(

Skip IP of loop device

Userdata adjustments

Wait for QEMU Agent to be up

We assume DHCP

Update example vm template creation snippet

Add ProviderID() as no-op to fulfill changed interface

Pick target node from all available nodes w/ sufficient resources

Pick random target node without any further logic
Clean up: Remove unused code

Add state of implementation to proxmox.md
@dermorz dermorz force-pushed the add-proxmox-cloud-provider branch from ae0e8ec to d8d8984 Compare October 27, 2022 08:34
@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dermorz
Once this PR has been reviewed and has the lgtm label, please assign moadqassem for approval by writing /assign @moadqassem in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Moritz Bracht <[email protected]>
@kubermatic-bot
Copy link
Contributor

@dermorz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-machine-controller-e2e-azure-custom-image-reference 74b2b5d link true /test pull-machine-controller-e2e-azure-custom-image-reference

Full PR test history

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@embik
Copy link
Member

embik commented Oct 27, 2022

@dermorz I forgot about this before, but would it be possible to add a Prow job for e2e testing?

@kubermatic-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
After a furter 30 days, they will turn rotten.
Mark the issue as fresh with /remove-lifecycle stale.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@kubermatic-bot kubermatic-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 25, 2023
@kubermatic-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

@kubermatic-bot kubermatic-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 24, 2023
@kubermatic-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

@kubermatic-bot
Copy link
Contributor

@kubermatic-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lifecycle/rotten Denotes an issue or PR that has aged beyond stale. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. sig/osm Denotes a PR or issue as being assigned to SIG OSM. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Proxmox as supported cloud provider
6 participants