[WIP] MGMT-19741: Add Nutanix support #2550

eliorerz · 2025-01-23T18:35:21Z

openshift-ci-robot · 2025-01-23T18:35:25Z

@eliorerz: This pull request references MGMT-19741 which is a valid jira issue.

In response to this:

/cc @eliorerz

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-01-23T18:35:27Z

@eliorerz: GitHub didn't allow me to request PR reviews from the following users: eliorerz.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @eliorerz

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-sigs/prow repository.

openshift-ci · 2025-01-23T18:35:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eliorerz
Once this PR has been reviewed and has the lgtm label, please assign dlom for approval. For more information see the 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:

OWNERS

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

codecov · 2025-01-23T20:24:19Z

Codecov Report

Attention: Patch coverage is 0% with 532 lines in your changes missing coverage. Please review.

Project coverage is 48.99%. Comparing base (5d3f4d7) to head (fa1957c).

Files with missing lines	Patch %	Lines
pkg/controller/machinepool/nutanixactuator.go	0.00%	153 Missing ⚠️
pkg/clusterresource/nutanix.go	0.00%	104 Missing ⚠️
contrib/pkg/deprovision/nutanix.go	0.00%	59 Missing ⚠️
contrib/pkg/createcluster/create.go	0.00%	53 Missing ⚠️
...g/controller/clusterpool/clusterpool_controller.go	0.00%	28 Missing ⚠️
pkg/install/generate.go	0.00%	28 Missing ⚠️
pkg/nutanixclient/client.go	0.00%	27 Missing ⚠️
pkg/installmanager/installmanager.go	0.00%	23 Missing ⚠️
.../v1/clusterdeployment_validating_admission_hook.go	0.00%	12 Missing and 1 partial ⚠️
...s/hive/v1/machinepool_validating_admission_hook.go	0.00%	11 Missing and 1 partial ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2550      +/-   ##
==========================================
- Coverage   49.78%   48.99%   -0.79%     
==========================================
  Files         281      286       +5     
  Lines       33029    33560     +531     
==========================================
  Hits        16444    16444              
- Misses      15252    15781     +529     
- Partials     1333     1335       +2

Files with missing lines	Coverage Δ
pkg/constants/constants.go	`100.00% <ø> (ø)`
...shift/hive/apis/hive/v1/clusterdeployment_types.go	`0.00% <ø> (ø)`
...hift/hive/apis/hive/v1/clusterdeprovision_types.go	`0.00% <ø> (ø)`
...m/openshift/hive/apis/hive/v1/machinepool_types.go	`0.00% <ø> (ø)`
contrib/pkg/deprovision/deprovision.go	`0.00% <0.00%> (ø)`
...g/controller/machinepool/machinepool_controller.go	`56.57% <0.00%> (-0.14%)`	⬇️
pkg/controller/utils/credentials.go	`0.00% <0.00%> (ø)`
...oller/clusterdeployment/installconfigvalidation.go	`84.84% <0.00%> (-15.16%)`	⬇️
.../clusterdeployment/clusterdeployment_controller.go	`66.62% <0.00%> (-0.41%)`	⬇️
contrib/pkg/utils/nutanix/nutanix.go	`0.00% <0.00%> (ø)`
... and 10 more

2uasimojo

Sorry, I didn't make it very far today. I'll keep hammering away at it next week.

2uasimojo · 2025-01-23T18:46:00Z

apis/hive/v1/nutanix/machinepools.go

@@ -0,0 +1,55 @@
+package nutanix


Suggest adding a nutanix SME to help review this and the CD Platform file, as I don't have the context to know whether these fields are correct and necessary/sufficient.

Also wouldn't be a terrible idea to get an API reviewer's eyes on this. I'm not an expert, but I think it may be best practice to include a +optional/+required tag even when it's the default; and it may be appropriate for some of the fields to be pointers rather than scalars if "unspecified" has different semantics than the nil value.

2uasimojo · 2025-01-23T18:56:42Z

apis/hive/v1/nutanix/machinepools.go

+type MachinePool struct {
+
+	// NumVcpusPerSocket is the total number of virtual processor cores (per socket) to assign a vm.
+	NumVcpusPerSocket int64 `json:"vcpus"`


You may also get a linter warning about this, but unless there's a good reason to do otherwise (e.g. we're copy-pasting an upstream type def (in which case we should look into just importing it)) the json names should match the go field names (modulo case mapping rules).

Several fields affected in this file.

apis/hive/v1/nutanix/machinepools.go

2uasimojo · 2025-01-23T23:15:56Z

apis/vendor/github.com/openshift/api/machine/v1/types_alibabaprovider.go

Self: It's weird that this (and others for platforms hive doesn't support) have been pulled in via this PR. Need to investigate further.

It's because the new types are using defs from openshift/api/machine/v1 (which, annoyingly, pulls in machine/v1beta1). This is definitely preferable to copying in those defs, though it's disappointing that it drags in a bunch of stuff we don't need.

Size-wise:

efried@efried-thinkpadp16vgen1:~/go/src/github.com/openshift/hive$ git show --name-status | awk '/^A.*vendor/ {print $2}' | xargs du -c 4 apis/vendor/github.com/openshift/api/machine/v1/Makefile 4 apis/vendor/github.com/openshift/api/machine/v1/common.go 4 apis/vendor/github.com/openshift/api/machine/v1/doc.go 4 apis/vendor/github.com/openshift/api/machine/v1/register.go 20 apis/vendor/github.com/openshift/api/machine/v1/types_alibabaprovider.go 4 apis/vendor/github.com/openshift/api/machine/v1/types_aws.go 28 apis/vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.go 16 apis/vendor/github.com/openshift/api/machine/v1/types_nutanixprovider.go 12 apis/vendor/github.com/openshift/api/machine/v1/types_powervsprovider.go 36 apis/vendor/github.com/openshift/api/machine/v1/zz_generated.deepcopy.go 4 apis/vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yaml 48 apis/vendor/github.com/openshift/api/machine/v1/zz_generated.swagger_doc_generated.go 4 apis/vendor/github.com/openshift/api/machine/v1beta1/Makefile 4 apis/vendor/github.com/openshift/api/machine/v1beta1/doc.go 4 apis/vendor/github.com/openshift/api/machine/v1beta1/register.go 16 apis/vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.go 32 apis/vendor/github.com/openshift/api/machine/v1beta1/types_azureprovider.go 20 apis/vendor/github.com/openshift/api/machine/v1beta1/types_gcpprovider.go 24 apis/vendor/github.com/openshift/api/machine/v1beta1/types_machine.go 8 apis/vendor/github.com/openshift/api/machine/v1beta1/types_machinehealthcheck.go 12 apis/vendor/github.com/openshift/api/machine/v1beta1/types_machineset.go 16 apis/vendor/github.com/openshift/api/machine/v1beta1/types_provider.go 12 apis/vendor/github.com/openshift/api/machine/v1beta1/types_vsphereprovider.go 52 apis/vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.go 4 apis/vendor/github.com/openshift/api/machine/v1beta1/zz_generated.featuregated-crd-manifests.yaml 80 apis/vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/doc.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/machinepools.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/platform.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/zz_generated.deepcopy.go 4 vendor/github.com/openshift/installer/pkg/destroy/nutanix/OWNERS 4 vendor/github.com/openshift/installer/pkg/destroy/nutanix/doc.go 8 vendor/github.com/openshift/installer/pkg/destroy/nutanix/nutanix.go 4 vendor/github.com/openshift/installer/pkg/destroy/nutanix/register.go 508 total

So we're adding about half a meg to the repo. Most of it (488K) is under apis/, which is where our downstreams take a hit. But it's under openshift/api, which seems acceptable -- particularly considering it's likely they'll be importing that anyway.

2uasimojo

Obviously you're not quite finished yet, but this is an awesome start.

In large part, I'm going to be avoiding in-depth review of the platform-specific esoterica of this work, assuming that you and QE will be testing the functionality, and if it works, it works :)

One specific area I want to bring up: You have some code in here that would exist exclusively to support clusterpools, but you may not necessarily want to do that right away. It's the kind of thing that could be added later if you were looking for an opportunity to split this effort into smaller chunks.

Similarly, a hibernation actuator is absent from this PR -- which is fine as it's also something that lends itself to a separate chunk of work.

2uasimojo · 2025-01-26T17:33:31Z

apis/vendor/github.com/openshift/api/machine/v1/types_alibabaprovider.go

It's because the new types are using defs from openshift/api/machine/v1 (which, annoyingly, pulls in machine/v1beta1). This is definitely preferable to copying in those defs, though it's disappointing that it drags in a bunch of stuff we don't need.

Size-wise:

efried@efried-thinkpadp16vgen1:~/go/src/github.com/openshift/hive$ git show --name-status | awk '/^A.*vendor/ {print $2}' | xargs du -c 4 apis/vendor/github.com/openshift/api/machine/v1/Makefile 4 apis/vendor/github.com/openshift/api/machine/v1/common.go 4 apis/vendor/github.com/openshift/api/machine/v1/doc.go 4 apis/vendor/github.com/openshift/api/machine/v1/register.go 20 apis/vendor/github.com/openshift/api/machine/v1/types_alibabaprovider.go 4 apis/vendor/github.com/openshift/api/machine/v1/types_aws.go 28 apis/vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.go 16 apis/vendor/github.com/openshift/api/machine/v1/types_nutanixprovider.go 12 apis/vendor/github.com/openshift/api/machine/v1/types_powervsprovider.go 36 apis/vendor/github.com/openshift/api/machine/v1/zz_generated.deepcopy.go 4 apis/vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yaml 48 apis/vendor/github.com/openshift/api/machine/v1/zz_generated.swagger_doc_generated.go 4 apis/vendor/github.com/openshift/api/machine/v1beta1/Makefile 4 apis/vendor/github.com/openshift/api/machine/v1beta1/doc.go 4 apis/vendor/github.com/openshift/api/machine/v1beta1/register.go 16 apis/vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.go 32 apis/vendor/github.com/openshift/api/machine/v1beta1/types_azureprovider.go 20 apis/vendor/github.com/openshift/api/machine/v1beta1/types_gcpprovider.go 24 apis/vendor/github.com/openshift/api/machine/v1beta1/types_machine.go 8 apis/vendor/github.com/openshift/api/machine/v1beta1/types_machinehealthcheck.go 12 apis/vendor/github.com/openshift/api/machine/v1beta1/types_machineset.go 16 apis/vendor/github.com/openshift/api/machine/v1beta1/types_provider.go 12 apis/vendor/github.com/openshift/api/machine/v1beta1/types_vsphereprovider.go 52 apis/vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.go 4 apis/vendor/github.com/openshift/api/machine/v1beta1/zz_generated.featuregated-crd-manifests.yaml 80 apis/vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/doc.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/machinepools.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/platform.go 4 vendor/github.com/openshift/hive/apis/hive/v1/nutanix/zz_generated.deepcopy.go 4 vendor/github.com/openshift/installer/pkg/destroy/nutanix/OWNERS 4 vendor/github.com/openshift/installer/pkg/destroy/nutanix/doc.go 8 vendor/github.com/openshift/installer/pkg/destroy/nutanix/nutanix.go 4 vendor/github.com/openshift/installer/pkg/destroy/nutanix/register.go 508 total

So we're adding about half a meg to the repo. Most of it (488K) is under apis/, which is where our downstreams take a hit. But it's under openshift/api, which seems acceptable -- particularly considering it's likely they'll be importing that anyway.

2uasimojo · 2025-01-26T17:40:01Z

pkg/installmanager/installmanager.go

@@ -76,6 +61,23 @@ import (
 	k8slabels "github.com/openshift/hive/pkg/util/labels"
 	"github.com/openshift/hive/pkg/util/scheme"
 	yamlutils "github.com/openshift/hive/pkg/util/yaml"
+	"github.com/openshift/installer/pkg/destroy/aws"


nit: This delta seems unnecessary. We usually deliberately keep the hive imports in their own section.

I'm curious if this was done by some tool you're using that reorganizes imports. I use the one in VSCode, which wouldn't do this unless I removed the newline between the sections.

I'm not asking you to change it back (though @suhanime might :P )

2uasimojo · 2025-01-26T17:44:46Z

apis/hive/v1/nutanix/machinepools.go

+//type BootType string
+
+//const (
+//	// VMs Disk Types
+//	DiskDeviceTypeCDROM DiskDeviceType = "CDROM"
+//	DiskDeviceTypeDISK  DiskDeviceType = "DISK"
+//
+//	// VMs Boot Types
+//	NutanixBootTypeUEFI       BootType = "UEFI"
+//	NutanixBootTypeLegacy     BootType = "LEGACY"
+//	NutanixBootTypeSecureBoot BootType = "SECURE_BOOT"
+//)


Reminder to clean this up.

contrib/pkg/createcluster/create.go

2uasimojo · 2025-01-26T18:40:23Z

contrib/pkg/createcluster/create.go

+			return nil, fmt.Errorf("no %s env var set, cannot proceed", constants.NutanixUsernameEnvVar)
+		}
+
+		nutanixPassword := os.Getenv(constants.NutanixPasswordEnvVar)


I know there's precedent for grabbing these creds via env, but it would be preferable to use a file if that's feasible. I'm not asking you to invent a config file format, but does nutanix already have one that it supports?

I'm not familiar with one but we can store that in a JSON or yaml formats. It will be easy to read and to maintain

2uasimojo · 2025-01-26T20:58:02Z

pkg/controller/clusterdeployment/installconfigvalidation_test.go

@@ -1,17 +1,18 @@
 package clusterdeployment

 import (
+	hivev1aws "github.com/openshift/hive/apis/hive/v1/aws"


Definitely don't agree with this delta, as it

splits up hive imports

gloms hive imports with core imports.

2uasimojo · 2025-01-26T21:20:07Z

pkg/controller/machinepool/machinepool_controller.go

@@ -1259,6 +1259,10 @@ func (r *ReconcileMachinePool) createActuator(
 	logger log.FieldLogger,
 ) (Actuator, error) {
 	switch {
+	case cd.Spec.Platform.Nutanix != nil:


nit: can we put this case in its alpha-ordered spot in the switch?

2uasimojo · 2025-01-26T21:25:03Z

pkg/controller/machinepool/nutanixactuator.go

+	actuator := &NutanixActuator{
+		client: client,
+		scheme: scheme,
+		//nutanixClient: nutanixClient,


Let's talk about whether you're actually going to need this. In general, the major cloud bit -- managing VMs -- is done way down the line by MAO, using the cloud creds on the spoke, operating on the MachineSets we generate. Exceptions exist: for example, in GCP and IBMCloud we need to discover zone names. But by default I would say YAGNI.

2uasimojo · 2025-01-26T21:30:46Z

pkg/controller/machinepool/nutanixactuator.go

+	installerMachineSets, err := installvsphere.MachineSets(
+		cd.Spec.ClusterMetadata.InfraID,
+		ic,
+		computePool,
+		// With FailureDomains[0].Topology.Template set, this param is ignored.
+		"HIVE_BUG!",
+		workerRole,
+		workerUserDataName,
+	)


copypasta, looks like this still needs some work.

When we get down to it though, I think we should take the nutanix installer team up on their offer to help out by reviewing this func.

The general principle is that our code here needs to dummy up the inputs to the platform-specific MachineSets generator. We can not rely on having access to the original install-config.yaml for the cluster, which is sometimes annoying and awkward, forcing us to either assume certain values or duplicate them in the (hive) MachinePool CRD. Since we're making this actuator from scratch, we should take the opportunity to evaluate those things carefully; else we can end up painting ourselves into a corner where e.g. we have to support bogus/deprecated/obsolete fields "forever".

2uasimojo · 2025-01-26T21:38:38Z

pkg/install/generate.go

This file will want code for the uninstaller as well. Examples for other platforms are referenced in a switch starting around L519. And that'll correspond to a hiveutil deprovision nutanix subcommand, with code under contrib/pkg/deprovision, plugged into deprovision.go.

# Conflicts: # go.sum

openshift-ci · 2025-02-17T23:01:54Z

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

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`fa1957c`	link	true	`/test security`
ci/prow/e2e-vsphere	`fa1957c`	link	true	`/test e2e-vsphere`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 23, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2025

eliorerz force-pushed the NO-ISSUE-add-nutanix-support branch 2 times, most recently from 89c5e08 to f254bcb Compare January 23, 2025 19:12

2uasimojo reviewed Jan 23, 2025

View reviewed changes

2uasimojo reviewed Jan 26, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2025

eliorerz force-pushed the NO-ISSUE-add-nutanix-support branch 14 times, most recently from 512216c to 6127f44 Compare February 14, 2025 08:34

eliorerz force-pushed the NO-ISSUE-add-nutanix-support branch 2 times, most recently from 90385a3 to 507c803 Compare February 17, 2025 10:21

eliorerz added 3 commits February 17, 2025 14:04

Add nutanix support

0b458c4

Add nutanix support

9f40403

# Conflicts: # go.sum

Add nutanix support

70b9256

eliorerz added 4 commits February 17, 2025 14:05

Add nutanix support

73e2a91

Remove gpu and datadisks from nutanix machinepool

5995726

Add Nutanix PC certificate

cdcb035

Update MachinePool

3584490

eliorerz force-pushed the NO-ISSUE-add-nutanix-support branch from 507c803 to 3584490 Compare February 17, 2025 12:07

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2025

Set region

fa1957c

eliorerz mentioned this pull request Feb 19, 2025

HIVE-2777: Implement hive Nutanix provisioning #2573

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] MGMT-19741: Add Nutanix support #2550

[WIP] MGMT-19741: Add Nutanix support #2550

eliorerz commented Jan 23, 2025

openshift-ci-robot commented Jan 23, 2025 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Jan 23, 2025

openshift-ci bot commented Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

2uasimojo left a comment

2uasimojo Jan 23, 2025

2uasimojo Jan 23, 2025

2uasimojo Jan 23, 2025

2uasimojo Jan 26, 2025

2uasimojo left a comment

2uasimojo Jan 26, 2025

2uasimojo Jan 26, 2025

2uasimojo Jan 26, 2025

2uasimojo Jan 26, 2025

eliorerz Feb 19, 2025

2uasimojo Jan 26, 2025

2uasimojo Jan 26, 2025

2uasimojo Jan 26, 2025

2uasimojo Jan 26, 2025

2uasimojo Jan 26, 2025

openshift-ci bot commented Feb 17, 2025

[WIP] MGMT-19741: Add Nutanix support #2550

Are you sure you want to change the base?

[WIP] MGMT-19741: Add Nutanix support #2550

Conversation

eliorerz commented Jan 23, 2025

openshift-ci-robot commented Jan 23, 2025 • edited by openshift-ci bot Loading

openshift-ci bot commented Jan 23, 2025

openshift-ci bot commented Jan 23, 2025

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

2uasimojo 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

2uasimojo 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

Choose a reason for hiding this comment

openshift-ci bot commented Feb 17, 2025

openshift-ci-robot commented Jan 23, 2025 •

edited by openshift-ci bot

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading