Readded support for additional disks (#2448)

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

This reverts commit 4f7eb65.

* Fixed race condition in additional volumes reconcile

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
@@ -190,6 +199,9 @@ type IBMVPCMachineV1Beta2Status struct {
 	// +listMapKey=type
 	// +kubebuilder:validation:MaxItems=32
 	Conditions []metav1.Condition `json:"conditions,omitempty"`
+	// AdditionalVolumeIDs is a list of Volume IDs as per IBMCloud
+	// +optional
+	AdditionalVolumeIDs []string `json:"additionalVolumeIDs,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1beta2/ibmvpcmachine_types.go

@@ -1162,3 +1162,80 @@ 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
+}
+
+// GetVolumeState returns the volume's state.
+func (m *MachineScope) GetVolumeState(volumeID string) (string, error) {
+	options := vpcv1.GetVolumeOptions{
+		ID: &volumeID,
+	}
+	result, _, err := m.IBMVPCClient.GetVolume(&options)
+	if err != nil {
+		return "", fmt.Errorf("could not fetch volume status: %w", err)
+	}
+	return *result.Status, err
+}
+
+// CreateVolume creates a new Volume and attaches it to the instance.
+func (m *MachineScope) CreateVolume(vpcVolume *infrav1.VPCVolume) (string, error) {
+	volumeOptions := vpcv1.CreateVolumeOptions{}
+	var resourceGroupID string
+	if m.IBMVPCCluster.Status.ResourceGroup != nil {
+		resourceGroupID = m.IBMVPCCluster.Status.ResourceGroup.ID
+	} else {
+		resourceGroupID = m.IBMVPCCluster.Spec.ResourceGroup
+	}
+	// TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest
+	volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
+		ResourceGroup: &vpcv1.ResourceGroupIdentityByID{
+			ID: &resourceGroupID,
+		},
+		Profile: &vpcv1.VolumeProfileIdentityByName{
+			Name: &vpcVolume.Profile,
+		},
+		Zone: &vpcv1.ZoneIdentity{
+			Name: &m.IBMVPCMachine.Spec.Zone,
+		},
+		Capacity: &vpcVolume.SizeGiB,
+	}
+
+	if vpcVolume.Profile == "custom" {
+		volumeOptions.VolumePrototype.(*vpcv1.VolumePrototype).Iops = &vpcVolume.Iops
+	}
+
+	volumeResult, _, err := m.IBMVPCClient.CreateVolume(&volumeOptions)
+	if err != nil {
+		return "", fmt.Errorf("error while creating volume: %w", err)
+	}
+
+	return *volumeResult.ID, nil
+}
+
+// AttachVolume attaches the given volume to the instance.
+func (m *MachineScope) AttachVolume(deleteOnInstanceDelete bool, volumeID, volumeName string) error {
+	attachmentOptions := vpcv1.CreateInstanceVolumeAttachmentOptions{
+		InstanceID: &m.IBMVPCMachine.Status.InstanceID,
+		Volume: &vpcv1.VolumeAttachmentPrototypeVolume{
+			ID: &volumeID,
+		},
+		DeleteVolumeOnInstanceDelete: &deleteOnInstanceDelete,
+		Name:                         &volumeName,
+	}
+
+	_, _, 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

@@ -42,6 +42,11 @@ import (
 	. "github.com/onsi/gomega"
 )
 
+var (
+	volumeName = "foo-volume"
+	volumeID   = "foo-volume-id"
+)
+
 func newVPCMachine(clusterName, machineName string) *infrav1.IBMVPCMachine {
 	return &infrav1.IBMVPCMachine{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1094,3 +1099,183 @@ 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"
+
+	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 TestGetVolumeState(t *testing.T) {
+	setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
+		t.Helper()
+		return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
+	}
+
+	volumeStatus := vpcv1.VolumeStatusPendingConst
+
+	vpcMachine := infrav1.IBMVPCMachine{
+		Status: infrav1.IBMVPCMachineStatus{
+			InstanceID: "foo-instance-id",
+		},
+	}
+
+	vpcVolume := vpcv1.Volume{
+		Name:   &volumeName,
+		ID:     &volumeID,
+		Status: &volumeStatus,
+	}
+	volumeFetchError := errors.New("error while fetching volume")
+
+	t.Run("Return correct volume state", 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().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil)
+		state, err := scope.GetVolumeState(volumeID)
+		g.Expect(err).To(BeNil())
+		g.Expect(state).To(Equal(volumeStatus))
+	})
+
+	t.Run("Return error when GetVolumeState returns error", 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().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(nil, nil, volumeFetchError)
+		state, err := scope.GetVolumeState(volumeID)
+		g.Expect(state).To(BeZero())
+		g.Expect(errors.Is(err, volumeFetchError)).To(BeTrue())
+	})
+}
+
+func TestCreateVolume(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",
+		},
+	}
+
+	infraVolume := infrav1.VPCVolume{
+		Name:    volumeName,
+		Profile: "custom",
+		Iops:    100,
+		SizeGiB: 50,
+	}
+	pendingState := vpcv1.VolumeStatusPendingConst
+
+	vpcVolume := vpcv1.Volume{
+		Name:   &volumeName,
+		ID:     &volumeID,
+		Status: &pendingState,
+	}
+
+	volumeCreationError := errors.New("error while creating volume")
+	t.Run("Volume creation is successful", func(t *testing.T) {
+		g := NewWithT(t)
+		mockController, mockVPC := setup(t)
+		t.Cleanup(mockController.Finish)
+		scope := setupMachineScope(clusterName, machineName, mockVPC)
+		mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil)
+		id, err := scope.CreateVolume(&infraVolume)
+		g.Expect(err).Should(Succeed())
+		g.Expect(id).Should(Equal(volumeID))
+	})
+	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)
+		id, err := scope.CreateVolume(&infraVolume)
+		g.Expect(err).ShouldNot(Succeed())
+		g.Expect(errors.Is(err, volumeCreationError)).To(BeTrue())
+		g.Expect(id).To(BeZero())
+	})
+}
+
+func TestAttachVolume(t *testing.T) {
+	setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
+		t.Helper()
+		return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
+	}
+
+	deleteOnInstanceDelete := true
+	vpcMachine := infrav1.IBMVPCMachine{
+		Status: infrav1.IBMVPCMachineStatus{
+			InstanceID: "foo-instance-id",
+		},
+	}
+	volumeAttachmentError := errors.New("error while attaching volume")
+	t.Run("Volume 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().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, nil)
+		err := scope.AttachVolume(deleteOnInstanceDelete, volumeID, volumeName)
+		g.Expect(err).Should(Succeed())
+	})
+	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().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, volumeAttachmentError)
+		err := scope.AttachVolume(deleteOnInstanceDelete, volumeID, volumeName)
+		g.Expect(err).ShouldNot(Succeed())
+		g.Expect(errors.Is(err, volumeAttachmentError)).To(BeTrue())
+	})
+}