Add support for OpenNebula as a cloud provider #1450

nilsding · 2022-09-29T08:05:23Z

What this PR does / why we need it:

This PR adds support for OpenNebula as a cloud provider. Similar to OpenStack it can be hosted on your own infrastructure.

We're using OpenNebula for many years already, and adding (basic?) support for it seemed to be easy enough.

What type of PR is this?
/kind feature

Special notes for your reviewer:
I'm not too fond of GoLang, so if there's anything that's not idiomatic please point it out.

Also I would still like to figure out automated end-to-end tests, any support with that is welcome. I tested it manually on a small kubernetes cluster (just some basic creation of the VMs) and it seems to work fine so far.

Does this PR introduce a user-facing change? Then add your Release Note here:

Add support for OpenNebula as a cloud provider

Documentation:

TBD

(?)

kubermatic-bot · 2022-09-29T08:05:39Z

Hi @nilsding. Thanks for your PR.

I'm waiting for a kubermatic member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

akurz · 2023-04-25T09:48:40Z

Hi,

we (adidas Runtastic) are testing this provider now for some month in some small user-clusters and it works fine for us. A review and approval would be much aprechiated @xrstf or @ahmedwaleedmalik - Thanks in advance!

Cheers,
Andreas

embik

Overall this looks good, just a few comments that I think could be addressed.

embik · 2023-04-25T10:29:49Z

test/e2e/provisioning/all_e2e_test.go

+		fmt.Sprintf("<< YOUR_DATASTORE_NAME >>=%s", oneDatastore),
+		fmt.Sprintf("<< YOUR_NETWORK_NAME >>=%s", oneNetwork),


Would it be an option to also prefix these with ONE_ instead of YOUR_, so we know that these variable belong to OpenNebula setups?

Sure, can do :)

embik · 2023-04-25T10:30:59Z

test/e2e/provisioning/testdata/machinedeployment-opennebula.yaml

+            vcpu: 2
+            memory: 1024
+
+            image: "<< OS_IMAGE >>"


similar to the other comment, can we maybe make sure that this is prefixed to be specific for OpenNebula? e.g. ONE_OS_IMAGE? This way we can avoid collision with the OpenStack image reference.

I think I'll rename this to ONE_IMAGE then, as OS_ is the prefix for OpenStack

.gitignore

embik · 2023-04-25T10:33:50Z

pkg/cloudprovider/provider/opennebula/provider.go

+	// create VM from the generated template above
+	vmID, err := controller.VMs().Create(tpl.String(), false)
+	if err != nil {
+		return nil, err


I think it would be helpful to contextualise the error message returned by the upstream SDK here, e.g. with this:

Suggested change

return nil, err

return nil, fmt.Errorf("failed to create VM: %w", err)

embik · 2023-04-25T10:34:32Z

pkg/cloudprovider/provider/opennebula/provider.go

+
+	vm, err := controller.VM(vmID).Info(false)
+	if err != nil {
+		return nil, err


Same here. I'm not exactly sure what this does, but the error should probably look something like this:

Suggested change

return nil, err

return nil, fmt.Errorf("failed to fetch VM information: %w", err)

embik · 2023-04-25T10:40:03Z

pkg/cloudprovider/provider/opennebula/provider.go

+	client := getClient(c)
+	controller := goca.NewController(client)
+
+	vmctrl := controller.VM(instance.(*openNebulaInstance).vm.ID)


I have to say I'm not a huge fan of the type assertions here and later in the code since it can introduce run-time panics. I understand it's required to get the OpenNebula specific information, but e.g. the AWS provider implements a private version of get to return the provider-specific instance, which in turn is then called by the public Get interface implementation:

machine-controller/pkg/cloudprovider/provider/aws/provider.go

Line 877 in df72929

func (p *provider) get(ctx context.Context, machine *clusterv1alpha1.Machine) (*awsInstance, error) {

Have you considered that approach, so you can get the actual instance type you need here?

No, this hasn't come across my mind, will try to adapt this then.

nilsding

thanks for the initial review :)

.gitignore

nilsding · 2023-04-25T11:04:22Z

test/e2e/provisioning/all_e2e_test.go

+		fmt.Sprintf("<< YOUR_DATASTORE_NAME >>=%s", oneDatastore),
+		fmt.Sprintf("<< YOUR_NETWORK_NAME >>=%s", oneNetwork),


Sure, can do :)

nilsding · 2023-04-25T11:05:00Z

test/e2e/provisioning/testdata/machinedeployment-opennebula.yaml

+            vcpu: 2
+            memory: 1024
+
+            image: "<< OS_IMAGE >>"


I think I'll rename this to ONE_IMAGE then, as OS_ is the prefix for OpenStack

nilsding · 2023-04-25T11:11:13Z

pkg/cloudprovider/provider/opennebula/provider.go

+	client := getClient(c)
+	controller := goca.NewController(client)
+
+	vmctrl := controller.VM(instance.(*openNebulaInstance).vm.ID)


No, this hasn't come across my mind, will try to adapt this then.

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

embik · 2023-05-02T06:41:45Z

/ok-to-test

embik · 2023-05-02T07:33:01Z

@nilsding please address the linter issues as well. The PR should be good to go afterwards.

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

embik

/approve

kubermatic-bot · 2023-05-05T08:14:59Z

LGTM label has been added.

Git tree hash: 457cb4afe9416faa791b8155ae0674826563b67b

kubermatic-bot · 2023-05-05T08:15:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: embik, nilsding

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [embik]

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

embik · 2023-05-05T08:15:29Z

Thank you for your contribution, @nilsding!

embik · 2023-05-05T10:16:00Z

/retest

kubermatic-triage-bot · 2023-05-05T12:36:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubermatic-triage-bot · 2023-05-05T14:56:05Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubermatic-triage-bot · 2023-05-05T15:56:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubermatic-triage-bot · 2023-05-05T17:16:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

* add cloud provider for opennebula Signed-off-by: Georg Gadinger <nilsding@nilsding.org> * use a flatcar image in example opennebula machinedeployment Signed-off-by: Georg Gadinger <nilsding@nilsding.org> * opennebula: update function signatures to include logger Signed-off-by: Georg Gadinger <nilsding@nilsding.org> * opennebula: update after review Signed-off-by: Georg Gadinger <nilsding@nilsding.org> * opennebula: add SET_HOSTNAME to context Signed-off-by: Georg Gadinger <nilsding@nilsding.org> * opennebula: fix lints Signed-off-by: Georg Gadinger <nilsding@nilsding.org> --------- Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

kubermatic-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 29, 2022

nilsding force-pushed the add-opennebula-support branch from 4e99e1b to 94806ba Compare November 16, 2022 14:06

kubermatic-bot added dco-signoff: no Denotes that at least one commit in the pull request doesn't have a valid DCO signoff message. and removed dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. labels Nov 16, 2022

nilsding force-pushed the add-opennebula-support branch from 94806ba to 538bdbc Compare November 16, 2022 14:10

kubermatic-bot added dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. and removed dco-signoff: no Denotes that at least one commit in the pull request doesn't have a valid DCO signoff message. labels Nov 16, 2022

nilsding force-pushed the add-opennebula-support branch from 538bdbc to 2d09954 Compare January 11, 2023 06:29

nilsding force-pushed the add-opennebula-support branch from 2d09954 to 00f566a Compare March 20, 2023 08:56

nilsding added 2 commits April 24, 2023 10:29

add cloud provider for opennebula

d7f6920

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

use a flatcar image in example opennebula machinedeployment

8f670ea

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

nilsding force-pushed the add-opennebula-support branch from 00f566a to 8f670ea Compare April 24, 2023 08:29

opennebula: update function signatures to include logger

df72929

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

embik requested changes Apr 25, 2023

View reviewed changes

kubermatic-bot assigned embik Apr 25, 2023

nilsding commented Apr 25, 2023

View reviewed changes

nilsding added 2 commits April 25, 2023 18:45

opennebula: update after review

6744c62

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

opennebula: add SET_HOSTNAME to context

bf52858

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

kubermatic-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 2, 2023

opennebula: fix lints

d9ab5e3

Signed-off-by: Georg Gadinger <nilsding@nilsding.org>

nilsding requested a review from embik May 5, 2023 07:54

embik approved these changes May 5, 2023

View reviewed changes

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2023

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2023

kubermatic-bot merged commit c92595c into kubermatic:main May 5, 2023

nilsding mentioned this pull request May 8, 2023

Add support for the new OpenNebula cloud provider kubermatic/operating-system-manager#288

Merged

al-lac mentioned this pull request Sep 1, 2023

Add support for OpenNebula Machine Deployments kubermatic/dashboard#6199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for OpenNebula as a cloud provider #1450

Add support for OpenNebula as a cloud provider #1450

nilsding commented Sep 29, 2022

kubermatic-bot commented Sep 29, 2022

akurz commented Apr 25, 2023

embik left a comment

embik Apr 25, 2023

nilsding Apr 25, 2023

embik Apr 25, 2023

nilsding Apr 25, 2023

embik Apr 25, 2023 •

edited

Loading

embik Apr 25, 2023

embik Apr 25, 2023 •

edited

Loading

nilsding Apr 25, 2023

nilsding left a comment

nilsding Apr 25, 2023

nilsding Apr 25, 2023

nilsding Apr 25, 2023

embik commented May 2, 2023

embik commented May 2, 2023

embik left a comment

kubermatic-bot commented May 5, 2023

kubermatic-bot commented May 5, 2023

embik commented May 5, 2023

embik commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

		fmt.Sprintf("<< YOUR_DATASTORE_NAME >>=%s", oneDatastore),
		fmt.Sprintf("<< YOUR_NETWORK_NAME >>=%s", oneNetwork),

	return nil, err
	return nil, fmt.Errorf("failed to create VM: %w", err)

	return nil, err
	return nil, fmt.Errorf("failed to fetch VM information: %w", err)

Add support for OpenNebula as a cloud provider #1450

Add support for OpenNebula as a cloud provider #1450

Conversation

nilsding commented Sep 29, 2022

kubermatic-bot commented Sep 29, 2022

akurz commented Apr 25, 2023

embik 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

embik Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embik Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsding 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

embik commented May 2, 2023

embik commented May 2, 2023

embik left a comment

Choose a reason for hiding this comment

kubermatic-bot commented May 5, 2023

kubermatic-bot commented May 5, 2023

embik commented May 5, 2023

embik commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

kubermatic-triage-bot commented May 5, 2023

embik Apr 25, 2023 •

edited

Loading

embik Apr 25, 2023 •

edited

Loading