SPLAT-1808: Add vSphere multi disk support #2028

vr4manta · 2024-09-16T13:38:23Z

Changes

Create new feature gate for vSphere multi disk
Create initial CRD changes for adding additional disks to vSphere machines

Blocks

Notes

This is being done as part of vSphere work for this feature
MAO Changes: SPLAT-1811: Add vSphere multi disk support machine-api-operator#1290

openshift-ci · 2024-09-16T13:38:46Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-09-16T13:38:46Z

Hello @vr4manta! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

vr4manta · 2024-09-17T13:14:54Z

/test all

kannon92 · 2024-09-17T20:21:35Z

Do you have a Jira ticket for this work? I'm trying to research disk support in openshift and this is of interest to me.

vr4manta · 2024-09-18T11:29:07Z

/retest

vr4manta · 2024-09-18T11:30:39Z

Do you have a Jira ticket for this work? I'm trying to research disk support in openshift and this is of interest to me.

@kannon92 , We are investigating this as well. I am using our spike to track the work and am getting stories / epic information for you. I'll attach the JIRAs to the PR above as well.

Spike: https://issues.redhat.com/browse/SPLAT-1789

rvanderp3

overall lgtm, i left a couple of comments. also, will there be a need for additional unit tests?

rvanderp3 · 2024-09-23T14:07:21Z

machine/v1beta1/types_vsphereprovider.go

+	// Disks is a list of non OS disks to be attached to the VM.
+	// +openshift:enable:FeatureGate=VSphereMultiDisk
+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate="",maxItems=0
+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiDisk,maxItems=15


is a max of 15 related to any documented limitation?

just put it in place for now since scsi controller can only house up to 16 if I remember correctly (so 1 for OS disk). Current design was to keep it simple and we just auto add to primarly scsi controller.

This limit will need to be explained in the godoc

I will update godoc to contain this info.

rvanderp3 · 2024-09-23T14:08:13Z

machine/v1beta1/types_vsphereprovider.go

@@ -172,6 +179,15 @@ type NetworkDeviceSpec struct {
 	AddressesFromPools []AddressesFromPool `json:"addressesFromPools,omitempty"`
 }

+// VSphereDisk describes additional disks for vSphere.
+type VSphereDisk struct {
+	// The device name exposed. Used to identify the disk.


if not defined, what is the behavior? is a disk created for you?

If you do not specify a device name, it will still create the disk with the size you defined. The deviceName field is to be used by admin to sort of remind them what disk is meant to be used for (if they so desire).

vr4manta · 2024-09-23T14:13:09Z

overall lgtm, i left a couple of comments. also, will there be a need for additional unit tests?

Unit tests are in the webhook for MAO. The CEL is being ignored based on current behavior of provider specs.

vr4manta · 2024-09-23T15:56:09Z

/assign @JoelSpeed
Ready for review

openshift-ci-robot · 2024-09-23T17:52:54Z

@vr4manta: This pull request references SPLAT-1808 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

SPLAT-1808

Changes

Create new feature gate for vSphere multi disk

Create initial CRD changes for adding additional disks to vSphere machines

Notes

This is being done as part of vSphere work for this feature

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.

rvanderp3 · 2024-09-23T19:09:59Z

/lgtm

openshift-ci · 2024-09-23T19:10:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rvanderp3, vr4manta
Once this PR has been reviewed and has the lgtm label, please ask for approval from joelspeed. 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:

OWNERS

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

JoelSpeed

Is there a paired MAO PR that implements validations for this new API field that I can review?

JoelSpeed · 2024-09-24T13:18:12Z

features/features.go

+					reportProblemsToJiraComponent("splat").
+					contactPerson("vr4manta").
+					productScope(ocpSpecific).
+					enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).


Are you ready for techpreview or would you prefer just to start in dev preview for now?

I can start in Dev if you prefer. I have the initial logic completed for creating extra disks for masters and workers. This is just the first step of the bigger picture, but i'll take your guidance on this.

PR for MAO: openshift/machine-api-operator#1290. I'll also put up in top comment.

Do you think this effort is likely to be long and span multiple releases?

JoelSpeed · 2024-09-24T13:19:01Z

machine/v1beta1/types_vsphereprovider.go

+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate="",maxItems=0
+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiDisk,maxItems=15


Make the whole field featuregated, rather than feature gated limits (though note this isn't actually part of a CRD so these markers are just for show at the moment)

Ok, I can do that. Yep, I had to make all this logic in the webhook of MAPI operator due to provider specs not being proper CRDs.

JoelSpeed · 2024-09-24T13:19:17Z

machine/v1beta1/types_vsphereprovider.go

+	// Disks is a list of non OS disks to be attached to the VM.
+	// +openshift:enable:FeatureGate=VSphereMultiDisk
+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate="",maxItems=0
+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiDisk,maxItems=15


This limit will need to be explained in the godoc

JoelSpeed · 2024-09-24T13:20:04Z

machine/v1beta1/types_vsphereprovider.go

@@ -70,6 +70,13 @@ type VSphereMachineProviderSpec struct {
 	// When using LinkedClone, if no snapshots exist for the source template, falls back to FullClone.
 	// +optional
 	CloneMode CloneMode `json:"cloneMode,omitempty"`
+	// Disks is a list of non OS disks to be attached to the VM.


Godoc should explain validations, and should start with the serialised form (disks) of the field name.

What does a user need to do for each disk here, do they need to specify any software side configuration?

Currently the logic is to just create the disk and attach to vsphere VMs. What user does after that can be anything. They can use LSO (local storage operator) or the future auto mount that we are currently working on to put these disks into certain locations.

JoelSpeed · 2024-09-24T13:20:28Z

machine/v1beta1/types_vsphereprovider.go

+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiDisk,maxItems=15
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf | size(self) == size(oldSelf)",message="additional disks cannot be added or removed once set"
+	// +optional
+	Disks []*VSphereDisk `json:"disks,omitempty"`


Why []*VsphereDisk instead of []VSphereDisk?

Just my preference. I can change to not be pointer if you prefer.

I removed pointer.

JoelSpeed · 2024-09-24T13:22:17Z

machine/v1beta1/types_vsphereprovider.go

+	// +openshift:enable:FeatureGate=VSphereMultiDisk
+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate="",maxItems=0
+	// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiDisk,maxItems=15
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf | size(self) == size(oldSelf)",message="additional disks cannot be added or removed once set"


What does this actually mean? This is part of the vSphere providerSpec on the Machine right? So, we don't expect it to be changed once the machine is created anyway, that's true of most fields

When this is present on a MachineSet though, then it should be mutable right?

Yes that is correct. I expect machines to be not mutable, but for machineset, I'll follow the patterns of other fields in the machine def section.

JoelSpeed · 2024-09-24T13:22:50Z

machine/v1beta1/types_vsphereprovider.go

+type VSphereDisk struct {
+	// The device name exposed. Used to identify the disk.
+	// +optional
+	DeviceName *string `json:"deviceName,omitempty"`


You won't need a pointer here

Ok, I'll update that.

JoelSpeed · 2024-09-24T13:23:14Z

machine/v1beta1/types_vsphereprovider.go

@@ -172,6 +179,15 @@ type NetworkDeviceSpec struct {
 	AddressesFromPools []AddressesFromPool `json:"addressesFromPools,omitempty"`
 }

+// VSphereDisk describes additional disks for vSphere.
+type VSphereDisk struct {
+	// The device name exposed. Used to identify the disk.


Godoc should start with the serialised version.

What are the requirements of this string? Max/Min/Characters/Patterns?

This is just a generic name for admin to use to identify the disk definition.

I updated godoc with these details.

JoelSpeed · 2024-09-24T13:24:15Z

machine/v1beta1/types_vsphereprovider.go

+	// The device name exposed. Used to identify the disk.
+	// +optional
+	DeviceName *string `json:"deviceName,omitempty"`
+	// SizeGB is the size of the disk (in GB).


Serialised version to start.

What's the minimum and maximum values here?

Do we mean GB or GiB?

Good catch. I'll update that.

JoelSpeed · 2024-09-24T13:24:36Z

machine/v1beta1/types_vsphereprovider.go

+	// +optional
+	DeviceName *string `json:"deviceName,omitempty"`
+	// SizeGB is the size of the disk (in GB).
+	SizeGB int64 `json:"sizeGb"`


Can you add an explicit +kubebuilder:validation:Required marker please

ok. I will add. thanks.

JoelSpeed · 2024-09-24T13:25:34Z

Is this feature currently supported in Cluster API?

vr4manta · 2024-09-24T13:46:32Z

Is this feature currently supported in Cluster API?

No. CAPI only supports additional disks that are created in the template.

openshift-ci-robot · 2024-09-24T14:00:53Z

@vr4manta: This pull request references SPLAT-1808 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

SPLAT-1808

Changes

Create new feature gate for vSphere multi disk

Create initial CRD changes for adding additional disks to vSphere machines

Blocks

SPLAT-1811: Add vSphere multi disk support machine-api-operator#1290

SPLAT-1809: Added vSphere multi disk support installer#9035

SPLAT-1810: API bump for vSphere multi disk library-go#1797

Notes

This is being done as part of vSphere work for this feature

MAO Changes: SPLAT-1811: Add vSphere multi disk support machine-api-operator#1290

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 · 2024-09-24T15:08:03Z

New changes are detected. LGTM label has been removed.

JoelSpeed · 2024-09-24T16:32:13Z

No. CAPI only supports additional disks that are created in the template.

So, if we merge this feature, this will create a feature gap between MAPV and CAPV? How would you suggest we migrate users using this feature?

We likely need to implement this upstream in CAPV before we can do any migration right?

vr4manta · 2024-09-24T16:55:20Z

No. CAPI only supports additional disks that are created in the template.

So, if we merge this feature, this will create a feature gap between MAPV and CAPV? How would you suggest we migrate users using this feature?

We likely need to implement this upstream in CAPV before we can do any migration right?

Yes. We will need to work with CAPV to see what they have thought about in the past for dynamic disk configs for VMs. The installer enhancement for this feature currently uses the postProvision hook after CAPI machines (control plane) are created and adds the disks to the VMs. I have been trying to follow what some of the other providers have for naming conventions, but each platform seems to use slightly different field names. For vSphere specific, it would be good to see if we can use the current additionalDisks field to add them if they do not exist in the template.

vr4manta · 2024-10-02T12:38:13Z

/hold
Looking into upstream enhancement (CAPV) vs having a a drift in potential configs.

openshift-ci · 2024-10-11T18:58:08Z

@vr4manta: 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/e2e-aws-serial-techpreview	`acc042b`	link	true	`/test e2e-aws-serial-techpreview`
ci/prow/e2e-upgrade	`acc042b`	link	true	`/test e2e-upgrade`

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 bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2024

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 18, 2024

vr4manta force-pushed the vsphere_disk branch from 70f7f59 to 682c24c Compare September 18, 2024 11:50

This was referenced Sep 18, 2024

SPLAT-1809: Added vSphere multi disk support openshift/installer#9035

Draft

SPLAT-1811: Add vSphere multi disk support openshift/machine-api-operator#1290

Draft

vr4manta changed the title ~~WIP: Add vSphere multi disk support~~ Add vSphere multi disk support Sep 23, 2024

vr4manta marked this pull request as ready for review September 23, 2024 13:36

openshift-ci 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 23, 2024

openshift-ci bot requested review from JoelSpeed and knobunc September 23, 2024 13:39

rvanderp3 reviewed Sep 23, 2024

View reviewed changes

openshift-ci bot assigned JoelSpeed Sep 23, 2024

vr4manta changed the title ~~Add vSphere multi disk support~~ SPLAT-1808: Add vSphere multi disk support Sep 23, 2024

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

openshift-ci bot assigned rvanderp3 Sep 23, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2024

vr4manta mentioned this pull request Sep 24, 2024

SPLAT-1813: Create CI jobs for vSphere multi disk. openshift/release#57095

Draft

JoelSpeed reviewed Sep 24, 2024

View reviewed changes

vr4manta force-pushed the vsphere_disk branch from 682c24c to ec53929 Compare September 24, 2024 15:07

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2024

vr4manta force-pushed the vsphere_disk branch from ec53929 to 434fb5d Compare September 24, 2024 17:29

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2024

vr4manta added 3 commits October 9, 2024 09:12

Add vSphere multi disk support

112d87a

Add vSphere multi disk support

981dd64

Removed deviceName

acc042b

vr4manta force-pushed the vsphere_disk branch from 434fb5d to acc042b Compare October 11, 2024 15:31

		// +openshift:validation:FeatureGateAwareMaxItems:featureGate="",maxItems=0
		// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiDisk,maxItems=15

SPLAT-1808: Add vSphere multi disk support #2028

Are you sure you want to change the base?

SPLAT-1808: Add vSphere multi disk support #2028

Conversation

vr4manta commented Sep 16, 2024 • edited Loading

Changes

Blocks

Notes

openshift-ci bot commented Sep 16, 2024

openshift-ci bot commented Sep 16, 2024

vr4manta commented Sep 17, 2024

kannon92 commented Sep 17, 2024

vr4manta commented Sep 18, 2024

vr4manta commented Sep 18, 2024

rvanderp3 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

vr4manta commented Sep 23, 2024

vr4manta commented Sep 23, 2024

openshift-ci-robot commented Sep 23, 2024 • edited by openshift-ci bot Loading

Changes

Notes

rvanderp3 commented Sep 23, 2024

openshift-ci bot commented Sep 23, 2024

JoelSpeed 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Sep 24, 2024

vr4manta commented Sep 24, 2024

openshift-ci-robot commented Sep 24, 2024 • edited by openshift-ci bot Loading

Changes

Blocks

Notes

openshift-ci bot commented Sep 24, 2024

JoelSpeed commented Sep 24, 2024

vr4manta commented Sep 24, 2024

vr4manta commented Oct 2, 2024

openshift-ci bot commented Oct 11, 2024

vr4manta commented Sep 16, 2024 •

edited

Loading

openshift-ci-robot commented Sep 23, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 24, 2024 •

edited by openshift-ci bot

Loading