Readded support for additional-volumes, fixed race condition in additional-vol reconciliation

This reverts commit 4f7eb65.

Author: anshuman-agarwala

api/v1beta1/zz_generated.conversion.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
+	// There is a hard limit of 12 volume attachments per instance:
+	// https://cloud.ibm.com/docs/vpc?topic=vpc-attaching-block-storage&interface=api#vol-attach-limits
+	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:MaxItems=12
+	// +kubebuilder:validation:XValidation:rule="oldSelf.all(x, x in self)",message="Values may only be added"
+	AdditionalVolumes []*VPCVolume `json:"additionalVolumes,omitempty"`
 }
 
 // IBMVPCResourceReference is a reference to a specific VPC resource by ID or Name
@@ -95,7 +103,7 @@ type IBMVPCResourceReference struct {
 	Name *string `json:"name,omitempty"`
 }
 
-// VPCVolume defines the volume information for the instance.
+// VPCVolume defines the volume information.
 type VPCVolume struct {
 	// DeleteVolumeOnInstanceDelete If set to true, when deleting the instance the volume will also be deleted.
 	// Default is set as true
@@ -108,14 +116,15 @@ type VPCVolume struct {
 	// +optional
 	Name string `json:"name,omitempty"`
 
-	// SizeGiB is the size of the virtual server's boot disk in GiB.
+	// SizeGiB is the size of the virtual server's disk in GiB.
 	// Default to the size of the image's `minimum_provisioned_size`.
 	// +optional
 	SizeGiB int64 `json:"sizeGiB,omitempty"`
 
-	// Profile is the volume profile for the bootdisk, refer https://cloud.ibm.com/docs/vpc?topic=vpc-block-storage-profiles
+	// Profile is the volume profile for the disk, refer https://cloud.ibm.com/docs/vpc?topic=vpc-block-storage-profiles
 	// for more information.
 	// Default to general-purpose
+	// NOTE: If a profile other than custom is specified, the Iops and SizeGiB fields will be ignored
 	// +kubebuilder:validation:Enum="general-purpose";"5iops-tier";"10iops-tier";"custom"
 	// +kubebuilder:default=general-purpose
 	// +optional

api/v1beta2/ibmvpcmachine_types.go

@@ -1162,3 +1162,63 @@ func (m *MachineScope) APIServerPort() int32 {
 	}
 	return infrav1.DefaultAPIServerPort
 }
+
+// GetVolumeAttachments returns the volume attachments for the instance.
+func (m *MachineScope) GetVolumeAttachments() ([]vpcv1.VolumeAttachment, error) {
+	options := vpcv1.ListInstanceVolumeAttachmentsOptions{
+		InstanceID: &m.IBMVPCMachine.Status.InstanceID,
+	}
+	result, _, err := m.IBMVPCClient.GetVolumeAttachments(&options)
+	if err != nil {
+		return nil, fmt.Errorf("error while getting volume attachments: %w", err)
+	}
+	return result.VolumeAttachments, nil
+}
+
+// CreateAndAttachVolume creates a new Volume and attaches it to the instance.
+func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1.VPCVolume) error {
+	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,
+		}
+	}
+
+	volumeResult, _, err := m.IBMVPCClient.CreateVolume(&volumeOptions)
+	if err != nil {
+		return fmt.Errorf("error while creating volume: %w", err)
+	}
+
+	attachmentOptions := vpcv1.CreateInstanceVolumeAttachmentOptions{
+		InstanceID: &m.IBMVPCMachine.Status.InstanceID,
+		Volume: &vpcv1.VolumeAttachmentPrototypeVolume{
+			ID: volumeResult.ID,
+		},
+		DeleteVolumeOnInstanceDelete: &vpcVolume.DeleteVolumeOnInstanceDelete,
+		Name:                         &vpcVolume.Name,
+	}
+
+	_, _, err = m.IBMVPCClient.AttachVolumeToInstance(&attachmentOptions)
+	if err != nil {
+		err = fmt.Errorf("error while attaching volume to instance: %w", err)
+	}
+	return err
+}

api/v1beta2/zz_generated.deepcopy.go

@@ -1094,3 +1094,120 @@ func TestDeleteVPCLoadBalancerPoolMember(t *testing.T) {
 		})
 	})
 }
+
+func TestGetVolumeAttachments(t *testing.T) {
+	setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
+		t.Helper()
+		return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
+	}
+
+	vpcMachine := infrav1.IBMVPCMachine{
+		Status: infrav1.IBMVPCMachineStatus{
+			InstanceID: "foo-instance-id",
+		},
+	}
+	volumeAttachmentName := "foo-volume-attachment"
+	volumeName := "foo-volume"
+
+	testVolumeAttachments := vpcv1.VolumeAttachmentCollection{
+		VolumeAttachments: []vpcv1.VolumeAttachment{{
+			Name: &volumeAttachmentName,
+		},
+			{
+				Name: &volumeName,
+			}},
+	}
+
+	t.Run("Return List of Volume Attachments for Machine", func(t *testing.T) {
+		g := NewWithT(t)
+		mockController, mockVPC := setup(t)
+		t.Cleanup(mockController.Finish)
+		scope := setupMachineScope(clusterName, machineName, mockVPC)
+		scope.IBMVPCMachine.Status = vpcMachine.Status
+		mockVPC.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&testVolumeAttachments, nil, nil)
+		attachments, err := scope.GetVolumeAttachments()
+		g.Expect(attachments).To(Equal(testVolumeAttachments.VolumeAttachments))
+		g.Expect(err).Should(Succeed())
+	})
+
+	t.Run("Return Error when GetVolumeAttachments fails", func(t *testing.T) {
+		g := NewWithT(t)
+		mockController, mockVPC := setup(t)
+		t.Cleanup(mockController.Finish)
+		scope := setupMachineScope(clusterName, machineName, mockVPC)
+		scope.IBMVPCMachine.Status = vpcMachine.Status
+		mockVPC.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(nil, nil, errors.New("Error when getting volume attachments"))
+		attachments, err := scope.GetVolumeAttachments()
+		g.Expect(attachments).To(BeNil())
+		g.Expect(err).ShouldNot(Succeed())
+	})
+}
+
+func TestCreateAndAttachVolume(t *testing.T) {
+	setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
+		t.Helper()
+		return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
+	}
+
+	volumeName := "foo-volume"
+	volumeID := "foo-volume-id"
+
+	vpcMachine := infrav1.IBMVPCMachine{
+		Status: infrav1.IBMVPCMachineStatus{
+			InstanceID: "foo-instance-id",
+		},
+	}
+
+	infraVolume := infrav1.VPCVolume{
+		Name: volumeName,
+	}
+
+	vpcVolume := vpcv1.Volume{
+		Name: &volumeName,
+		ID:   &volumeID,
+	}
+
+	volumeCreationError := errors.New("error while creating volume")
+
+	volumeAttachmentError := errors.New("error while attaching volume")
+
+	t.Run("Volume creation and attachment is successful", func(t *testing.T) {
+		g := NewWithT(t)
+		mockController, mockVPC := setup(t)
+		t.Cleanup(mockController.Finish)
+		scope := setupMachineScope(clusterName, machineName, mockVPC)
+		scope.IBMVPCMachine.Status = vpcMachine.Status
+		mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil)
+		mockVPC.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, nil)
+
+		err := scope.CreateAndAttachVolume(&infraVolume)
+		g.Expect(err).Should(Succeed())
+	})
+
+	t.Run("Volume Creation Fails", func(t *testing.T) {
+		g := NewWithT(t)
+		mockController, mockVPC := setup(t)
+		t.Cleanup(mockController.Finish)
+		scope := setupMachineScope(clusterName, machineName, mockVPC)
+		scope.IBMVPCMachine.Status = vpcMachine.Status
+		mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(nil, nil, volumeCreationError)
+
+		err := scope.CreateAndAttachVolume(&infraVolume)
+		g.Expect(err).ShouldNot(Succeed())
+		g.Expect(errors.Is(err, volumeCreationError)).To(BeTrue())
+	})
+
+	t.Run("Volume Attachment Fails", func(t *testing.T) {
+		g := NewWithT(t)
+		mockController, mockVPC := setup(t)
+		t.Cleanup(mockController.Finish)
+		scope := setupMachineScope(clusterName, machineName, mockVPC)
+		scope.IBMVPCMachine.Status = vpcMachine.Status
+		mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil)
+		mockVPC.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, volumeAttachmentError)
+
+		err := scope.CreateAndAttachVolume(&infraVolume)
+		g.Expect(err).ShouldNot(Succeed())
+		g.Expect(errors.Is(err, volumeAttachmentError)).To(BeTrue())
+	})
+}

cloud/scope/machine.go

@@ -206,6 +206,65 @@ spec:
           spec:
             description: IBMVPCMachineSpec defines the desired state of IBMVPCMachine.
             properties:
+              additionalVolumes:
+                description: |-
+                  additionalVolumes is the list of additional volumes attached to the instance
+                  There is a hard limit of 12 volume attachments per instance:
+                  https://cloud.ibm.com/docs/vpc?topic=vpc-attaching-block-storage&interface=api#vol-attach-limits
+                items:
+                  description: VPCVolume defines the volume information.
+                  properties:
+                    deleteVolumeOnInstanceDelete:
+                      default: true
+                      description: |-
+                        DeleteVolumeOnInstanceDelete If set to true, when deleting the instance the volume will also be deleted.
+                        Default is set as true
+                      type: boolean
+                    encryptionKeyCRN:
+                      description: |-
+                        EncryptionKey is the root key to use to wrap the data encryption key for the volume and this points to the CRN
+                        and possible values are as follows.
+                        The CRN of the [Key Protect Root
+                        Key](https://cloud.ibm.com/docs/key-protect?topic=key-protect-getting-started-tutorial) or [Hyper Protect Crypto
+                        Service Root Key](https://cloud.ibm.com/docs/hs-crypto?topic=hs-crypto-get-started) for this resource.
+                        If unspecified, the `encryption` type for the volume will be `provider_managed`.
+                      type: string
+                    iops:
+                      description: |-
+                        Iops is the maximum I/O operations per second (IOPS) to use for the volume. Applicable only to volumes using a profile
+                        family of `custom`.
+                      format: int64
+                      type: integer
+                    name:
+                      description: |-
+                        Name is the unique user-defined name for this volume.
+                        Default will be autogenerated
+                      type: string
+                    profile:
+                      default: general-purpose
+                      description: |-
+                        Profile is the volume profile for the disk, refer https://cloud.ibm.com/docs/vpc?topic=vpc-block-storage-profiles
+                        for more information.
+                        Default to general-purpose
+                        NOTE: If a profile other than custom is specified, the Iops and SizeGiB fields will be ignored
+                      enum:
+                      - general-purpose
+                      - 5iops-tier
+                      - 10iops-tier
+                      - custom
+                      type: string
+                    sizeGiB:
+                      description: |-
+                        SizeGiB is the size of the virtual server's disk in GiB.
+                        Default to the size of the image's `minimum_provisioned_size`.
+                      format: int64
+                      type: integer
+                  type: object
+                maxItems: 12
+                type: array
+                x-kubernetes-validations:
+                - message: Values may only be added
+                  rule: oldSelf.all(x, x in self)
               bootVolume:
                 description: BootVolume contains machines's boot volume configurations
                   like size, iops etc..
@@ -239,9 +298,10 @@ spec:
                   profile:
                     default: general-purpose
                     description: |-
-                      Profile is the volume profile for the bootdisk, refer https://cloud.ibm.com/docs/vpc?topic=vpc-block-storage-profiles
+                      Profile is the volume profile for the disk, refer https://cloud.ibm.com/docs/vpc?topic=vpc-block-storage-profiles
                       for more information.
                       Default to general-purpose
+                      NOTE: If a profile other than custom is specified, the Iops and SizeGiB fields will be ignored
                     enum:
                     - general-purpose
                     - 5iops-tier
@@ -250,7 +310,7 @@ spec:
                     type: string
                   sizeGiB:
                     description: |-
-                      SizeGiB is the size of the virtual server's boot disk in GiB.
+                      SizeGiB is the size of the virtual server's disk in GiB.
                       Default to the size of the image's `minimum_provisioned_size`.
                     format: int64
                     type: integer