Added support for additionalVolumes in VPCMachines #2275

anshuman-agarwala · 2025-03-27T13:58:53Z

What this PR does / why we need it:
Adds AdditionalVolume support for VPC Machines

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

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Added AdditionalVolume support for VPC Machines

k8s-ci-robot · 2025-03-27T13:59:03Z

Hi @anshuman-agarwala. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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-sigs/prow repository.

netlify · 2025-03-27T13:59:10Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`dcb4bae`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/6807194f40394c0008a57a6c
😎 Deploy Preview	https://deploy-preview-2275.cluster-api-ibmcloud.sigs.k8s.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mkumatag · 2025-03-27T17:54:06Z

/ok-to-test

Prajyot-Parab · 2025-04-08T05:38:12Z

/cc @Karthik-K-N @Amulyam24

Karthik-K-N · 2025-04-08T12:40:22Z

cloud/scope/machine.go

+	}
+	result, _, err := m.IBMVPCClient.GetVolumeAttachments(&options)
+	if err != nil {
+		return nil, err


Suggested change

return nil, err

return nil, fmt.Errorf("error getting volume attachements: %w, err)

In general wrap and return any child errors. Log error only if we are not returning error back.

Karthik-K-N · 2025-04-08T12:52:08Z

cloud/scope/machine.go

+	volumeOptions := vpcv1.CreateVolumeOptions{}
+	// TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest
+	if vpcVolume.Profile != "custom" {
+		volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
+			Profile: &vpcv1.VolumeProfileIdentityByName{
+				Name: &vpcVolume.Profile,
+			},
+			Zone: &vpcv1.ZoneIdentity{
+				Name: &m.IBMVPCMachine.Spec.Zone,
+			},
+			Capacity: &vpcVolume.SizeGiB,
+		}
+	} else {
+		volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
+			Iops: &vpcVolume.Iops,
+			Profile: &vpcv1.VolumeProfileIdentityByName{
+				Name: &vpcVolume.Profile,
+			},
+			Zone: &vpcv1.ZoneIdentity{
+				Name: &m.IBMVPCMachine.Spec.Zone,
+			},
+			Capacity: &vpcVolume.SizeGiB,
+		}
+	}


Suggested change

volumeOptions := vpcv1.CreateVolumeOptions{}

// TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest

if vpcVolume.Profile != "custom" {

volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{

Profile: &vpcv1.VolumeProfileIdentityByName{

Name: &vpcVolume.Profile,

},

Zone: &vpcv1.ZoneIdentity{

Name: &m.IBMVPCMachine.Spec.Zone,

},

Capacity: &vpcVolume.SizeGiB,

}

} else {

volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{

Iops: &vpcVolume.Iops,

Profile: &vpcv1.VolumeProfileIdentityByName{

Name: &vpcVolume.Profile,

},

Zone: &vpcv1.ZoneIdentity{

Name: &m.IBMVPCMachine.Spec.Zone,

},

Capacity: &vpcVolume.SizeGiB,

}

}

volumeOptions := vpcv1.CreateVolumeOptions{

VolumePrototype: vpcv1.VolumePrototype{

Profile: &vpcv1.VolumeProfileIdentityByName{

Name: &vpcVolume.Profile,

},

Zone: &vpcv1.ZoneIdentity{

Name: &m.IBMVPCMachine.Spec.Zone,

},

Capacity: &vpcVolume.SizeGiB,

},

}

if vpcVolume.Profile == "custom" {

volumeOptions.VolumePrototype.Iops = &vpcVolume.Iops,

}

will this work?

I tried that initially, was getting this error: volumeOptions.VolumePrototype.Iops undefined (type vpcv1.VolumePrototypeIntf has no field or method Iops)

Not sure whats missing then!

Karthik-K-N · 2025-04-08T12:52:54Z

cloud/scope/machine.go

+
+	volumeResult, _, err := m.IBMVPCClient.CreateVolume(&volumeOptions)
+	if err != nil {
+		return err


lets wrap the error and return here and everywhere.

Karthik-K-N · 2025-04-08T12:53:58Z

cloud/scope/machine.go

+	attachmentOptions := vpcv1.CreateInstanceVolumeAttachmentOptions{
+		InstanceID: &m.IBMVPCMachine.Status.InstanceID,
+		Volume: &vpcv1.VolumeAttachmentPrototypeVolume{
+			ID: volumeResult.ID,


is it sure that err and volumeResult wont be nil at the same time?

Yes, according to the API docs: https://cloud.ibm.com/apidocs/vpc/latest#create-volume volume id will always be returned.

Karthik-K-N · 2025-04-08T12:55:54Z

cloud/scope/machine_test.go

+
+	dummyVolumeAttachments := vpcv1.VolumeAttachmentCollection{
+		VolumeAttachments: []vpcv1.VolumeAttachment{{
+			Name: new(string),


I think we can use some good name?

Refactored the name

Still I see this

Name: new(string),

Did I miss anything?

Sorry, I think I misunderstood what you meant the first time, fixed it now.

Karthik-K-N · 2025-04-08T12:57:34Z

cloud/scope/machine_test.go

+		ID:   &volumeID,
+	}
+
+	volumeCreationError := errors.New("Error while creating volume")


Suggested change

volumeCreationError := errors.New("Error while creating volume")

volumeCreationError := errors.New("error while creating volume")

Lets not capitalize the errors

Karthik-K-N · 2025-04-08T12:58:16Z

controllers/ibmvpcmachine_controller.go

+	// Handle Additional Volumes
+	err = r.reconcileAdditionalVolumes(machineScope)
+	if err != nil {
+		return ctrl.Result{}, err


lets wrap and return error

Karthik-K-N · 2025-04-09T05:51:04Z

api/v1beta2/ibmvpcmachine_types.go

@@ -78,6 +78,14 @@ type IBMVPCMachineSpec struct {
 	// SSHKeys is the SSH pub keys that will be used to access VM.
 	// ID will take higher precedence over Name if both specified.
 	SSHKeys []*IBMVPCResourceReference `json:"sshKeys,omitempty"`
+
+	// AdditionalVolumes is the list of additional volumes attached to the instance


Suggested change

// AdditionalVolumes is the list of additional volumes attached to the instance

// additionalVolumes is the list of additional volumes attached to the instance

We are supposed to use the user facing name in comment so user will find it easy when we use go doc.

Karthik-K-N · 2025-04-09T05:53:49Z

cloud/scope/machine.go

+	if err != nil {
+		return nil, fmt.Errorf("error while getting volume attachments: %w", err)
+	}
+	return result.VolumeAttachments, nil


Is it sure that result wont be nil?

Yes, it will return an empty slice if there are no attachments.

Karthik-K-N · 2025-04-09T05:55:55Z

cloud/scope/machine.go

+	volumeOptions := vpcv1.CreateVolumeOptions{}
+	// TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest
+	if vpcVolume.Profile != "custom" {
+		volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
+			Profile: &vpcv1.VolumeProfileIdentityByName{
+				Name: &vpcVolume.Profile,
+			},
+			Zone: &vpcv1.ZoneIdentity{
+				Name: &m.IBMVPCMachine.Spec.Zone,
+			},
+			Capacity: &vpcVolume.SizeGiB,
+		}
+	} else {
+		volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
+			Iops: &vpcVolume.Iops,
+			Profile: &vpcv1.VolumeProfileIdentityByName{
+				Name: &vpcVolume.Profile,
+			},
+			Zone: &vpcv1.ZoneIdentity{
+				Name: &m.IBMVPCMachine.Spec.Zone,
+			},
+			Capacity: &vpcVolume.SizeGiB,
+		}
+	}


Not sure whats missing then!

Karthik-K-N · 2025-04-09T05:56:57Z

cloud/scope/machine_test.go

+
+	dummyVolumeAttachments := vpcv1.VolumeAttachmentCollection{
+		VolumeAttachments: []vpcv1.VolumeAttachment{{
+			Name: new(string),


Still I see this

Name: new(string),

Did I miss anything?

Karthik-K-N · 2025-04-09T05:57:46Z

controllers/ibmvpcmachine_controller.go

+	}
+	createdVolumeList, err := machineScope.GetVolumeAttachments()
+	if err != nil {
+		return err


Lets wrap the error.

The error is already wrapped when it's getting returned from GetVolumeAttachments, shall I wrap it again here?

I think that would be better to understand the flow and follow the pattern

It's already following the pattern, we're returning error directly in other places in the code when the error is already wrapped by the called method.

Karthik-K-N

LGTM

@Prajyot-Parab Please take look.

Amulyam24 · 2025-04-10T05:23:00Z

test/e2e/data/templates/cluster-template-vpc.yaml

@@ -402,6 +402,14 @@ spec:
    spec:
      bootVolume:
        sizeGiB: ${IBMVPC_WORKER_BOOT_VOLUME_SIZEGIB:=20}
+      additionalVolumes:


This template is generated via generate-e2e-templates target

cluster-api-provider-ibmcloud/Makefile

Line 189 in 982d67e

generate-e2e-templates: $(KUSTOMIZE) ## Generate E2E cluster templates

and will get overriden if changes are made here.

Are we planning to test provisioning additional volumes by default in e2e cluster creation?

I added it because I thought it would help validate my changes. If adding it to the default e2e test is not needed then shall I remove this?

@anshuman-agarwala you have validated the changes already right? also we can add it to default, but it has to be in sync with generate-e2e-templates target so that it wont get overriden as @Amulyam24 said.

Yes have validated the changes manually.
I've added the the additional volumes to the template.

@anshuman-agarwala, the changes should not directly be made to the template as they are generated via generate-templates. Please check the templates dir and add wherever applicable in either cluster-template or bases/vpc dir.

You need to update in appropriate places as @Amulyam24 said then run make generate and make generate-e2e-templates targets to get updated templates and add them to this PR.

Prajyot-Parab · 2025-04-10T07:09:10Z

controllers/ibmvpcmachine_controller.go

+	if len(machineVolumes) == 0 {
+		return nil
+	}
+	createdVolumeList, err := machineScope.GetVolumeAttachments()


Suggested change

createdVolumeList, err := machineScope.GetVolumeAttachments()

volumeAttachmentList, err := machineScope.GetVolumeAttachments()

update accordingly.

Prajyot-Parab · 2025-04-10T07:09:40Z

controllers/ibmvpcmachine_controller.go

+	if err != nil {
+		return err
+	}
+	createdVolumeNames := sets.New[string]()


Suggested change

createdVolumeNames := sets.New[string]()

volumeAttachmentNames := sets.New[string]()

update accordingly.

Amulyam24

@anshuman-agarwala, do you plan to add UT for controller?

Amulyam24 · 2025-04-11T10:36:35Z

cloud/scope/machine.go

@@ -1155,3 +1155,63 @@ func (m *MachineScope) APIServerPort() int32 {
 	}
 	return infrav1beta2.DefaultAPIServerPort
 }
+
+// GetVolumeAttachments returns the volumeattachments for the instance.


Suggested change

// GetVolumeAttachments returns the volumeattachments for the instance.

// GetVolumeAttachments returns the volume attachments for the instance.

anshuman-agarwala · 2025-04-11T12:02:24Z

@anshuman-agarwala, do you plan to add UT for controller?

Yes, I missed those, thanks for the catch!

anshuman-agarwala · 2025-04-11T12:17:44Z

@anshuman-agarwala, do you plan to add UT for controller?

Yes, I missed those, thanks for the catch!

Looking at the existing UTs, I don't think there's anything I can add to the controller UT for my changes.

Amulyam24 · 2025-04-14T09:16:30Z

@anshuman-agarwala, do you plan to add UT for controller?

Yes, I missed those, thanks for the catch!

Looking at the existing UTs, I don't think there's anything I can add to the controller UT for my changes.

I was wondering if UT could be added for reconcileAdditionalVolumes?

Amulyam24 · 2025-04-21T05:16:08Z

I was wondering if UT could be added for reconcileAdditionalVolumes?

@anshuman-agarwala, did you get a chance to look into this?

anshuman-agarwala · 2025-04-22T04:23:08Z

I was wondering if UT could be added for reconcileAdditionalVolumes?

@anshuman-agarwala, did you get a chance to look into this?

Added UTs for reconcileAdditionalVolumes,
Sorry about the delay!

Amulyam24 · 2025-04-22T07:13:53Z

Thanks @anshuman-agarwala!

/lgtm

cc @Prajyot-Parab

Prajyot-Parab

/lgtm

k8s-ci-robot · 2025-04-22T07:33:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anshuman-agarwala, Prajyot-Parab

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~~ [Prajyot-Parab]

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

k8s-ci-robot added area/provider/ibmcloud cncf-cla: yes labels Mar 27, 2025

k8s-ci-robot requested review from mkumatag and Prajyot-Parab March 27, 2025 13:59

k8s-ci-robot added the needs-ok-to-test label Mar 27, 2025

k8s-ci-robot added the size/L label Mar 27, 2025

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 27, 2025

anshuman-agarwala force-pushed the additional-disk branch from bbb9c56 to 1f3ceed Compare March 28, 2025 05:57

k8s-ci-robot added the needs-rebase label Apr 8, 2025

anshuman-agarwala force-pushed the additional-disk branch from 1f3ceed to b58a096 Compare April 8, 2025 02:30

k8s-ci-robot removed the needs-rebase label Apr 8, 2025

k8s-ci-robot requested review from Amulyam24 and Karthik-K-N April 8, 2025 05:38

Karthik-K-N reviewed Apr 8, 2025

View reviewed changes

anshuman-agarwala force-pushed the additional-disk branch 2 times, most recently from 6d1fb3b to b55955a Compare April 8, 2025 17:26

Karthik-K-N reviewed Apr 9, 2025

View reviewed changes

anshuman-agarwala force-pushed the additional-disk branch from b55955a to 75e4915 Compare April 9, 2025 12:07

Karthik-K-N reviewed Apr 10, 2025

View reviewed changes

Amulyam24 reviewed Apr 10, 2025

View reviewed changes

anshuman-agarwala force-pushed the additional-disk branch 3 times, most recently from 334f1b7 to 86d7fab Compare April 10, 2025 07:01

k8s-ci-robot added size/XL and removed size/L labels Apr 10, 2025

Prajyot-Parab suggested changes Apr 10, 2025

View reviewed changes

anshuman-agarwala force-pushed the additional-disk branch 2 times, most recently from 5e9cca6 to e97e31a Compare April 10, 2025 08:58

Amulyam24 reviewed Apr 11, 2025

View reviewed changes

anshuman-agarwala force-pushed the additional-disk branch 2 times, most recently from 98a84a5 to b5d1f7f Compare April 11, 2025 12:50

Added support for additionalVolumes in VPCMachines

dcb4bae

anshuman-agarwala force-pushed the additional-disk branch from b5d1f7f to dcb4bae Compare April 22, 2025 04:21

k8s-ci-robot assigned Amulyam24 Apr 22, 2025

k8s-ci-robot added the lgtm label Apr 22, 2025

Prajyot-Parab approved these changes Apr 22, 2025

View reviewed changes

k8s-ci-robot assigned Prajyot-Parab Apr 22, 2025

k8s-ci-robot added the approved label Apr 22, 2025

k8s-ci-robot merged commit ae6fd3f into kubernetes-sigs:main Apr 22, 2025
13 checks passed

	return nil, err
	return nil, fmt.Errorf("error getting volume attachements: %w, err)

	volumeCreationError := errors.New("Error while creating volume")
	volumeCreationError := errors.New("error while creating volume")

	// AdditionalVolumes is the list of additional volumes attached to the instance
	// additionalVolumes is the list of additional volumes attached to the instance

	createdVolumeList, err := machineScope.GetVolumeAttachments()
	volumeAttachmentList, err := machineScope.GetVolumeAttachments()

	createdVolumeNames := sets.New[string]()
	volumeAttachmentNames := sets.New[string]()

	// GetVolumeAttachments returns the volumeattachments for the instance.
	// GetVolumeAttachments returns the volume attachments for the instance.

Added support for additionalVolumes in VPCMachines #2275

Added support for additionalVolumes in VPCMachines #2275

Conversation

anshuman-agarwala commented Mar 27, 2025

k8s-ci-robot commented Mar 27, 2025

netlify bot commented Mar 27, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

mkumatag commented Mar 27, 2025

Prajyot-Parab commented Apr 8, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Karthik-K-N left a comment

Choose a reason for hiding this comment

Amulyam24 Apr 10, 2025 • edited Loading

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

Amulyam24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshuman-agarwala commented Apr 11, 2025

anshuman-agarwala commented Apr 11, 2025

Amulyam24 commented Apr 14, 2025

Amulyam24 commented Apr 21, 2025

anshuman-agarwala commented Apr 22, 2025

Amulyam24 commented Apr 22, 2025

Prajyot-Parab left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 22, 2025

netlify bot commented Mar 27, 2025 •

edited

Loading

Amulyam24 Apr 10, 2025 •

edited

Loading