SPLAT-1811: Add vSphere multi disk support #1290

vr4manta · 2024-09-18T12:19:56Z

Changes

Add logic for creating additional disks to new machines

Prerequisites

SPLAT-1808: Add vSphere multi disk support api#2028

openshift-ci · 2024-09-18T12:20:05Z

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-robot · 2024-09-23T17:56:05Z

@vr4manta: This pull request references SPLAT-1811 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-1811

Changes

Add logic for creating additional disks to new machines

Prerequisites

SPLAT-1808: Add vSphere multi disk support api#2028

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.

vr4manta · 2024-09-23T19:18:00Z

/test all

JoelSpeed · 2024-09-24T16:35:04Z

pkg/controller/vsphere/reconciler.go

 	// Currently, MAPI does not provide any API knobs to configure additional volumes for a VM.
 	// So, we are expecting the VM to have only one disk, which is OS disk.


This comment will need to be updated

Will do. Thanks!

JoelSpeed · 2024-09-24T16:37:15Z

pkg/controller/vsphere/reconciler.go

+func getAdditionalDiskSpecs(s *machineScope, devices object.VirtualDeviceList, datastore *object.Datastore) ([]types.BaseVirtualDeviceConfigSpec, error) {
+	var diskSpecs []types.BaseVirtualDeviceConfigSpec
+
+	klog.InfoS("About to iterate through disks", "disk", s.providerSpec.Disks)


I'll update those. Thanks!

I removed this message.

JoelSpeed · 2024-09-24T16:37:27Z

pkg/controller/vsphere/reconciler.go

+
+	klog.InfoS("About to iterate through disks", "disk", s.providerSpec.Disks)
+
+	klog.Infof("Feature Gates: %v", s.featureGates)


I'll update those. Thanks!

I removed these logs messages.

JoelSpeed · 2024-09-24T16:38:08Z

pkg/controller/vsphere/reconciler.go

+	if len(s.providerSpec.Disks) > 0 && !s.featureGates.Enabled(featuregate.Feature(apifeatures.FeatureGateVSphereMultiDisk)) {
+		return nil, machinecontroller.InvalidMachineConfiguration(
+			"machines cannot contain additional disks due to VSphereMultiDisk feature gate being disabled")
+	}


This needs to happen at the providerSpec validation webhook level as well

Ok, I'll copy this up this up there too. Thanks!

JoelSpeed · 2024-09-24T16:39:23Z

pkg/controller/vsphere/machine_scope_test.go

@@ -547,7 +547,8 @@ func TestNodeGetter(t *testing.T) {
 	testEnv := &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "..", "..", "install"),
-			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests")},
+			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests"),
+			filepath.Join("..", "..", "..", "third_party", "cluster-api", "crds")},


What new CRDs are needed?

It seems that once I fixed loading of feature gates, there are some existing tests that start to fail due to

E1113 15:26:22.870540 2519608 actuator.go:65] "test" error: test: reconciler failed to Delete machine: unable to remove finalizer for IP address claims: unable to list IPAddressClaims: no matches for kind "IPAddressClaim" in version "ipam.cluster.x-k8s.io/v1beta1"
actuator_test.go:387:
Unexpected error:
<*fmt.wrapError | 0xc00073f880>:
test: reconciler failed to Delete machine: unable to remove finalizer for IP address claims: unable to list IPAddressClaims: no matches for kind "IPAddressClaim" in version "ipam.cluster.x-k8s.io/v1beta1"
{
msg: "test: reconciler failed to Delete machine: unable to remove finalizer for IP address claims: unable to list IPAddressClaims: no matches for kind "IPAddressClaim" in version "ipam.cluster.x-k8s.io/v1beta1"",
err: <*fmt.wrapError | 0xc000f9dba0>{
msg: "unable to remove finalizer for IP address claims: unable to list IPAddressClaims: no matches for kind "IPAddressClaim" in version "ipam.cluster.x-k8s.io/v1beta1"",
err: <*fmt.wrapError | 0xc000f9db80>{
msg: "unable to list IPAddressClaims: no matches for kind "IPAddressClaim" in version "ipam.cluster.x-k8s.io/v1beta1"",
err: <*meta.NoKindMatchError | 0xc000b54d80>{
GroupKind: {
Group: "ipam.cluster.x-k8s.io",
Kind: "IPAddressClaim",
},
SearchedVersions: ["v1beta1"],
},
},
},
}
occurred

I was able to fix this by including the CRDs that are provided by the cluster-capi-operator. Sadly, these are not in a package that go can extract full directory due to no .go file in the same directory as the CRD yaml files. Maybe in a separate PR we can fix the other repo to put a dummy go file in there so we can remove this. Note, we'll still need the filepath join logic, but we can remove what was put into the third_party directory.

I have gone through and removed this one entry but the other locations are still needed. I think I added this here as part of my original debug.

JoelSpeed · 2024-09-24T16:40:11Z

pkg/controller/vsphere/reconciler_test.go

+			err = gates.Set(fmt.Sprintf("%v=%v", features.FeatureGateVSphereStaticIPs, tc.staticIPFeatureGateEnabled))
+			if err != nil {


Nit, should inline this

Suggested change

err = gates.Set(fmt.Sprintf("%v=%v", features.FeatureGateVSphereStaticIPs, tc.staticIPFeatureGateEnabled))

if err != nil {

if err := gates.Set(fmt.Sprintf("%v=%v", features.FeatureGateVSphereStaticIPs, tc.staticIPFeatureGateEnabled)); err != nil {

Ok, I'll update that.

JoelSpeed · 2024-09-24T16:43:18Z

third_party/cluster-api/crd/ipam.cluster.x-k8s.io_ipaddressclaims.yaml

Is this related to this change?

These are needed for the unit tests to run. Not sure why they were not needed before, but I think it has to do with now I am setting the featuregate infos during the tests.

vr4manta · 2024-11-14T15:19:52Z

/test all

openshift-ci · 2024-12-11T11:24:47Z

@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-operator	`d5ccb46`	link	true	`/test e2e-aws-operator`
ci/prow/e2e-vsphere-operator	`d5ccb46`	link	false	`/test e2e-vsphere-operator`
ci/prow/e2e-azure-operator	`d5ccb46`	link	false	`/test e2e-azure-operator`
ci/prow/okd-scos-e2e-aws-ovn	`d5ccb46`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/golint	`d5ccb46`	link	true	`/test golint`
ci/prow/e2e-metal-ipi-ovn-dualstack	`d5ccb46`	link	false	`/test e2e-metal-ipi-ovn-dualstack`
ci/prow/e2e-openstack	`d5ccb46`	link	false	`/test e2e-openstack`
ci/prow/e2e-aws-ovn-upgrade	`d5ccb46`	link	true	`/test e2e-aws-ovn-upgrade`
ci/prow/verify-crds-sync	`d5ccb46`	link	true	`/test verify-crds-sync`
ci/prow/goimports	`d5ccb46`	link	true	`/test goimports`
ci/prow/e2e-metal-ipi-upgrade	`d5ccb46`	link	false	`/test e2e-metal-ipi-upgrade`
ci/prow/e2e-metal-ipi-virtualmedia	`d5ccb46`	link	false	`/test e2e-metal-ipi-virtualmedia`
ci/prow/e2e-vsphere-ovn-upgrade	`d5ccb46`	link	false	`/test e2e-vsphere-ovn-upgrade`
ci/prow/e2e-metal-ipi	`d5ccb46`	link	false	`/test e2e-metal-ipi`
ci/prow/e2e-gcp-operator	`d5ccb46`	link	false	`/test e2e-gcp-operator`
ci/prow/e2e-vsphere-ovn	`d5ccb46`	link	false	`/test e2e-vsphere-ovn`
ci/prow/e2e-azure-ovn	`d5ccb46`	link	false	`/test e2e-azure-ovn`
ci/prow/e2e-metal-ipi-ovn-ipv6	`d5ccb46`	link	false	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-gcp-ovn	`d5ccb46`	link	false	`/test e2e-gcp-ovn`
ci/prow/e2e-nutanix	`d5ccb46`	link	false	`/test e2e-nutanix`
ci/prow/e2e-vsphere-ovn-serial	`d5ccb46`	link	false	`/test e2e-vsphere-ovn-serial`
ci/prow/e2e-aws-ovn	`d5ccb46`	link	true	`/test e2e-aws-ovn`
ci/prow/security	`d5ccb46`	link	true	`/test security`

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 · 2024-12-12T17:56:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign radekmanak 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

JoelSpeed · 2024-10-18T13:02:57Z

pkg/controller/vsphere/reconciler.go

 // VM os disks filename contains the machine name in it
 // and has the format like "[DATASTORE] path-within-datastore/machine-name.vmdk".
 // This is based on vSphere behavior, an OS disk file gets a name that equals the target VM name during the clone operation.
 func filterOutVmOsDisk(attachedDisks []attachedDisk, machine *machinev1.Machine) []attachedDisk {
 	var disks []attachedDisk
+	regex, _ := regexp.Compile(fmt.Sprintf(".*\\/%s(_\\d*)?.vmdk", machine.GetName()))


What is this regex doing differently? What will the additional disks names look like?

JoelSpeed · 2024-12-13T15:23:01Z

pkg/controller/vsphere/reconciler.go

+	if len(s.providerSpec.DataDisks) > 0 && !s.featureGates.Enabled(featuregate.Feature(apifeatures.FeatureGateVSphereMultiDisk)) {
+		return nil, machinecontroller.InvalidMachineConfiguration(
+			"machines cannot contain additional disks due to VSphereMultiDisk feature gate being disabled")
+	}


Is this same check in the validation webhook?

JoelSpeed · 2024-12-13T15:25:48Z

pkg/controller/vsphere/reconciler.go

+		if !used {
+			unit32 := int32(unit)
+			vd.UnitNumber = &unit32
+			klog.Infof("Assigned unit number %d = %d", unit, *vd.UnitNumber)


Wondering if we are getting a little too much on the logs here? This seems rather low level for logging default

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 18, 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

vr4manta force-pushed the vsphere_disk branch from 7bb2ca6 to 7e46607 Compare September 18, 2024 12:33

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

vr4manta force-pushed the vsphere_disk branch 4 times, most recently from b05d938 to cb52ddf Compare September 23, 2024 12:33

vr4manta changed the title ~~WIP: Vsphere disk~~ SPLAT-1811: Vsphere disk 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

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

vr4manta force-pushed the vsphere_disk branch from cb52ddf to da0d90c Compare September 23, 2024 18:55

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

vr4manta mentioned this pull request Sep 24, 2024

SPLAT-1808: Add vSphere multi disk support openshift/api#2028

Open

JoelSpeed reviewed Sep 24, 2024

View reviewed changes

vr4manta force-pushed the vsphere_disk branch from da0d90c to cd3cb81 Compare September 24, 2024 19:10

vr4manta force-pushed the vsphere_disk branch from cd3cb81 to 2d44f66 Compare October 14, 2024 15:15

vr4manta force-pushed the vsphere_disk branch 2 times, most recently from be37562 to ad6f989 Compare November 14, 2024 15:08

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

vr4manta force-pushed the vsphere_disk branch 2 times, most recently from 4e8092d to d5ccb46 Compare November 14, 2024 15:19

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

vr4manta force-pushed the vsphere_disk branch from 1d28c85 to aa778a0 Compare December 13, 2024 12:29

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

vr4manta force-pushed the vsphere_disk branch 2 times, most recently from 12880d9 to 73a04b8 Compare December 13, 2024 12:45

vr4manta added 2 commits December 13, 2024 07:46

Vendor updates

c4d927a

Added vSphere support for additional disks

62a3cbb

vr4manta force-pushed the vsphere_disk branch from 73a04b8 to 62a3cbb Compare December 13, 2024 12:46

JoelSpeed reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPLAT-1811: Add vSphere multi disk support #1290

SPLAT-1811: Add vSphere multi disk support #1290

vr4manta commented Sep 18, 2024 •

edited

Loading

openshift-ci bot commented Sep 18, 2024

openshift-ci-robot commented Sep 23, 2024 •

edited by openshift-ci bot

Loading

Changes

Prerequisites

vr4manta commented Sep 23, 2024

JoelSpeed Sep 24, 2024

vr4manta Nov 13, 2024

JoelSpeed Sep 24, 2024

vr4manta Nov 13, 2024

vr4manta Nov 14, 2024

JoelSpeed Sep 24, 2024

vr4manta Nov 13, 2024

vr4manta Nov 14, 2024

JoelSpeed Sep 24, 2024

vr4manta Nov 13, 2024

JoelSpeed Sep 24, 2024

vr4manta Nov 13, 2024

vr4manta Nov 14, 2024

JoelSpeed Sep 24, 2024

vr4manta Nov 13, 2024

JoelSpeed Sep 24, 2024

vr4manta Nov 14, 2024

vr4manta commented Nov 14, 2024

openshift-ci bot commented Dec 11, 2024

openshift-ci bot commented Dec 12, 2024

JoelSpeed Oct 18, 2024

JoelSpeed Dec 13, 2024

JoelSpeed Dec 13, 2024

		// Currently, MAPI does not provide any API knobs to configure additional volumes for a VM.
		// So, we are expecting the VM to have only one disk, which is OS disk.


		klog.InfoS("About to iterate through disks", "disk", s.providerSpec.Disks)

		klog.Infof("Feature Gates: %v", s.featureGates)

		err = gates.Set(fmt.Sprintf("%v=%v", features.FeatureGateVSphereStaticIPs, tc.staticIPFeatureGateEnabled))
		if err != nil {

	err = gates.Set(fmt.Sprintf("%v=%v", features.FeatureGateVSphereStaticIPs, tc.staticIPFeatureGateEnabled))
	if err != nil {
	if err := gates.Set(fmt.Sprintf("%v=%v", features.FeatureGateVSphereStaticIPs, tc.staticIPFeatureGateEnabled)); err != nil {

SPLAT-1811: Add vSphere multi disk support #1290

Are you sure you want to change the base?

SPLAT-1811: Add vSphere multi disk support #1290

Conversation

vr4manta commented Sep 18, 2024 • edited Loading

Changes

Prerequisites

openshift-ci bot commented Sep 18, 2024

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

Changes

Prerequisites

vr4manta commented Sep 23, 2024

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

vr4manta commented Nov 14, 2024

openshift-ci bot commented Dec 11, 2024

openshift-ci bot commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vr4manta commented Sep 18, 2024 •

edited

Loading

openshift-ci-robot commented Sep 23, 2024 •

edited by openshift-ci bot

Loading