Fixed race condition in additional volumes reconcile

Author: anshuman-agarwala

api/v1beta1/zz_generated.conversion.go

@@ -184,6 +184,10 @@ type IBMVPCMachineStatus struct {
 	// LoadBalancerPoolMembers is the status of IBM Cloud VPC Load Balancer Backend Pools the machine is a member.
 	// +optional
 	LoadBalancerPoolMembers []VPCLoadBalancerBackendPoolMember `json:"loadBalancerPoolMembers,omitempty"`
+
+	// AdditionalVolumeIDs is a list of Volume ID as per IBMCloud
+	// +optional
+	AdditionalVolumeIDs []string `json:"additionalVolumeIDs,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1beta2/ibmvpcmachine_types.go

@@ -1168,8 +1168,20 @@ func (m *MachineScope) GetVolumeAttachments() ([]vpcv1.VolumeAttachment, error)
 	return result.VolumeAttachments, nil
 }
 
-// CreateAndAttachVolume creates a new Volume and attaches it to the instance.
-func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1beta2.VPCVolume) error {
+// 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 *infrav1beta2.VPCVolume) (string, error) {
 	volumeOptions := vpcv1.CreateVolumeOptions{}
 	// TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest
 	if vpcVolume.Profile != "custom" {
@@ -1203,19 +1215,28 @@ func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1beta2.VPCVolume)
 
 	volumeResult, _, err := m.IBMVPCClient.CreateVolume(&volumeOptions)
 	if err != nil {
-		return fmt.Errorf("error while creating volume: %w", err)
+		return "", fmt.Errorf("error while creating volume: %w", err)
 	}
 
+	if *volumeResult.Status != vpcv1.VolumeStatusAvailableConst {
+		return *volumeResult.ID, fmt.Errorf("volume is not in available state")
+	}
+
+	return *volumeResult.ID, m.AttachVolume(vpcVolume.DeleteVolumeOnInstanceDelete, *volumeResult.ID, vpcVolume.Name)
+}
+
+// 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: volumeResult.ID,
+			ID: &volumeID,
 		},
-		DeleteVolumeOnInstanceDelete: &vpcVolume.DeleteVolumeOnInstanceDelete,
-		Name:                         &vpcVolume.Name,
+		DeleteVolumeOnInstanceDelete: &deleteOnInstanceDelete,
+		Name:                         &volumeName,
 	}
 
-	_, _, err = m.IBMVPCClient.AttachVolumeToInstance(&attachmentOptions)
+	_, _, err := m.IBMVPCClient.AttachVolumeToInstance(&attachmentOptions)
 	if err != nil {
 		err = fmt.Errorf("error while attaching volume to instance: %w", err)
 	}

api/v1beta2/zz_generated.deepcopy.go

@@ -43,6 +43,11 @@ import (
 	. "github.com/onsi/gomega"
 )
 
+var (
+	volumeName = "foo-volume"
+	volumeID   = "foo-volume-id"
+)
+
 func newVPCMachine(clusterName, machineName string) *infrav1beta2.IBMVPCMachine {
 	return &infrav1beta2.IBMVPCMachine{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1109,7 +1114,6 @@ func TestGetVolumeAttachments(t *testing.T) {
 		},
 	}
 	volumeAttachmentName := "foo-volume-attachment"
-	volumeName := "foo-volume"
 
 	testVolumeAttachments := vpcv1.VolumeAttachmentCollection{
 		VolumeAttachments: []vpcv1.VolumeAttachment{{
@@ -1145,14 +1149,57 @@ func TestGetVolumeAttachments(t *testing.T) {
 	})
 }
 
-func TestCreateAndAttachVolume(t *testing.T) {
+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))
 	}
 
-	volumeName := "foo-volume"
-	volumeID := "foo-volume-id"
+	volumeStatus := vpcv1.VolumeStatusPendingConst
+
+	vpcMachine := infrav1beta2.IBMVPCMachine{
+		Status: infrav1beta2.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 := infrav1beta2.IBMVPCMachine{
 		Status: infrav1beta2.IBMVPCMachineStatus{
@@ -1161,54 +1208,79 @@ func TestCreateAndAttachVolume(t *testing.T) {
 	}
 
 	infraVolume := infrav1beta2.VPCVolume{
-		Name: volumeName,
+		Name:    volumeName,
+		Profile: "custom",
+		Iops:    100,
+		SizeGiB: 50,
 	}
+	pendingState := vpcv1.VolumeStatusPendingConst
 
 	vpcVolume := vpcv1.Volume{
-		Name: &volumeName,
-		ID:   &volumeID,
+		Name:   &volumeName,
+		ID:     &volumeID,
+		Status: &pendingState,
 	}
 
 	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) {
+	volumeNotAvailableError := errors.New("volume is not in available state")
+	t.Run("Volume creation is successful, volume is not yet in available 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().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())
+		id, err := scope.CreateVolume(&infraVolume)
+		g.Expect(err).ShouldNot(Succeed())
+		g.Expect(err).To(Equal(volumeNotAvailableError))
+		g.Expect(id).Should(Equal(volumeID))
 	})
-
-	t.Run("Volume Creation Fails", func(t *testing.T) {
+	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)
+		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))
+	}
 
-	t.Run("Volume Attachment Fails", func(t *testing.T) {
+	deleteOnInstanceDelete := true
+	vpcMachine := infrav1beta2.IBMVPCMachine{
+		Status: infrav1beta2.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().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil)
 		mockVPC.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, volumeAttachmentError)
-
-		err := scope.CreateAndAttachVolume(&infraVolume)
+		err := scope.AttachVolume(deleteOnInstanceDelete, volumeID, volumeName)
 		g.Expect(err).ShouldNot(Succeed())
 		g.Expect(errors.Is(err, volumeAttachmentError)).To(BeTrue())
 	})

cloud/scope/machine.go

@@ -539,6 +539,11 @@ spec:
           status:
             description: IBMVPCMachineStatus defines the observed state of IBMVPCMachine.
             properties:
+              additionalVolumeIDs:
+                description: AdditionalVolumeIDs is a list of Volume ID as per IBMCloud
+                items:
+                  type: string
+                type: array
               addresses:
                 description: Addresses contains the IBM Cloud instance associated
                   addresses.

cloud/scope/machine_test.go

@@ -258,15 +258,16 @@ func (r *IBMVPCMachineReconciler) reconcileNormal(machineScope *scope.MachineSco
 	}
 
 	// Handle Additional Volumes
-	err = r.reconcileAdditionalVolumes(machineScope)
+	var result ctrl.Result
+	result, err = r.reconcileAdditionalVolumes(machineScope)
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("error reconciling additional volumes: %w", err)
 	}
 
 	// With a running machine and all Load Balancer Pool Members reconciled, mark machine as ready.
 	machineScope.SetReady()
 	conditions.MarkTrue(machineScope.IBMVPCMachine, infrav1beta2.InstanceReadyCondition)
-	return ctrl.Result{}, nil
+	return result, nil
 }
 
 func (r *IBMVPCMachineReconciler) getOrCreate(scope *scope.MachineScope) (*vpcv1.Instance, error) {
@@ -298,14 +299,18 @@ func (r *IBMVPCMachineReconciler) reconcileDelete(scope *scope.MachineScope) (_
 	return ctrl.Result{}, nil
 }
 
-func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope.MachineScope) error {
+func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope.MachineScope) (ctrl.Result, error) {
+	if machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs == nil {
+		machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs = make([]string, len(machineScope.IBMVPCMachine.Spec.AdditionalVolumes))
+	}
 	machineVolumes := machineScope.IBMVPCMachine.Spec.AdditionalVolumes
+	result := ctrl.Result{}
 	if len(machineVolumes) == 0 {
-		return nil
+		return result, nil
 	}
 	volumeAttachmentList, err := machineScope.GetVolumeAttachments()
 	if err != nil {
-		return err
+		return result, err
 	}
 	volumeAttachmentNames := sets.New[string]()
 	for i := range volumeAttachmentList {
@@ -315,15 +320,33 @@ func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope
 	// Read through the list, checking if volume exists and create volume if it does not
 	for v := range machineVolumes {
 		if volumeAttachmentNames.Has(machineVolumes[v].Name) {
-			// volume attachment has been created so volume will eventually be attached
+			// volume attachment has been created so volume is already attached
 			continue
 		}
-
-		err = machineScope.CreateAndAttachVolume(machineVolumes[v])
-		if err != nil {
-			errList = append(errList, err)
-			continue
+		if machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v] != "" {
+			// volume was already created, fetch volume status and attach if possible
+			state, err := machineScope.GetVolumeState(machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v])
+			if err != nil {
+				errList = append(errList, err)
+			}
+			if state != vpcv1.VolumeStatusAvailableConst {
+				// volume not ready, requeue
+				result = ctrl.Result{RequeueAfter: 30 * time.Second}
+				continue
+			}
+			err = machineScope.AttachVolume(machineVolumes[v].DeleteVolumeOnInstanceDelete, machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v], machineVolumes[v].Name)
+			if err != nil {
+				errList = append(errList, err)
+			}
+		} else {
+			volumeID, err := machineScope.CreateVolume(machineVolumes[v])
+			machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v] = volumeID
+			if err != nil && err.Error() == "volume is not in available state" {
+				result = ctrl.Result{RequeueAfter: 30 * time.Second}
+			} else if err != nil {
+				errList = append(errList, err)
+			}
 		}
 	}
-	return errors.Join(errList...)
+	return result, errors.Join(errList...)
 }