From 0b7172924ad5e5cfbb7b117e93f16704d097ce4e Mon Sep 17 00:00:00 2001 From: Anshuman Date: Thu, 8 May 2025 15:32:09 +0530 Subject: [PATCH] Fixed race condition in additional volumes reconcile --- api/v1beta1/zz_generated.conversion.go | 1 + api/v1beta2/ibmvpcmachine_types.go | 4 + api/v1beta2/zz_generated.deepcopy.go | 5 + cloud/scope/machine.go | 43 +++++-- cloud/scope/machine_test.go | 114 ++++++++++++++---- ...cture.cluster.x-k8s.io_ibmvpcmachines.yaml | 5 + controllers/ibmvpcmachine_controller.go | 49 ++++++-- controllers/ibmvpcmachine_controller_test.go | 102 +++++++++++++++- pkg/cloud/services/vpc/mock/vpc_generated.go | 16 +++ pkg/cloud/services/vpc/service.go | 5 + pkg/cloud/services/vpc/vpc.go | 1 + 11 files changed, 297 insertions(+), 48 deletions(-) diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 97a3929e4..055cea689 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -1444,6 +1444,7 @@ func autoConvert_v1beta2_IBMVPCMachineStatus_To_v1beta1_IBMVPCMachineStatus(in * // WARNING: in.FailureMessage requires manual conversion: does not exist in peer-type out.InstanceStatus = in.InstanceStatus // WARNING: in.LoadBalancerPoolMembers requires manual conversion: does not exist in peer-type + // WARNING: in.AdditionalVolumeIDs requires manual conversion: does not exist in peer-type return nil } diff --git a/api/v1beta2/ibmvpcmachine_types.go b/api/v1beta2/ibmvpcmachine_types.go index ef98caff7..ee54a7602 100644 --- a/api/v1beta2/ibmvpcmachine_types.go +++ b/api/v1beta2/ibmvpcmachine_types.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 diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index 65b9aac62..327edfb3a 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -1301,6 +1301,11 @@ func (in *IBMVPCMachineStatus) DeepCopyInto(out *IBMVPCMachineStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.AdditionalVolumeIDs != nil { + in, out := &in.AdditionalVolumeIDs, &out.AdditionalVolumeIDs + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new IBMVPCMachineStatus. diff --git a/cloud/scope/machine.go b/cloud/scope/machine.go index 21f91c0fe..697fc6ee2 100644 --- a/cloud/scope/machine.go +++ b/cloud/scope/machine.go @@ -1168,12 +1168,33 @@ 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{} + 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 if vpcVolume.Profile != "custom" { volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{ + ResourceGroup: &vpcv1.ResourceGroupIdentityByID{ + ID: &resourceGroupID, + }, Profile: &vpcv1.VolumeProfileIdentityByName{ Name: &vpcVolume.Profile, }, @@ -1184,6 +1205,9 @@ func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1beta2.VPCVolume) } } else { volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{ + ResourceGroup: &vpcv1.ResourceGroupIdentityByID{ + ID: &resourceGroupID, + }, Iops: &vpcVolume.Iops, Profile: &vpcv1.VolumeProfileIdentityByName{ Name: &vpcVolume.Profile, @@ -1197,19 +1221,24 @@ 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) } + 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: 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) } diff --git a/cloud/scope/machine_test.go b/cloud/scope/machine_test.go index 7eefc8222..1074b24b2 100644 --- a/cloud/scope/machine_test.go +++ b/cloud/scope/machine_test.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,75 @@ 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) { + 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) - 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) + 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) { + 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()) }) diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmvpcmachines.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmvpcmachines.yaml index 0a378fa96..db6df6df4 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmvpcmachines.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_ibmvpcmachines.yaml @@ -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. diff --git a/controllers/ibmvpcmachine_controller.go b/controllers/ibmvpcmachine_controller.go index 31300a375..87d0f32ee 100644 --- a/controllers/ibmvpcmachine_controller.go +++ b/controllers/ibmvpcmachine_controller.go @@ -258,7 +258,8 @@ 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) } @@ -266,7 +267,7 @@ func (r *IBMVPCMachineReconciler) reconcileNormal(machineScope *scope.MachineSco // 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,35 @@ 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) + } + switch state { + case vpcv1.VolumeStatusPendingConst, vpcv1.VolumeStatusUpdatingConst: + result = ctrl.Result{RequeueAfter: 10 * time.Second} + case vpcv1.VolumeStatusFailedConst, vpcv1.VolumeStatusUnusableConst: + errList = append(errList, fmt.Errorf("volume in unexpected state: %s", state)) + case vpcv1.VolumeStatusAvailableConst: + err = machineScope.AttachVolume(machineVolumes[v].DeleteVolumeOnInstanceDelete, machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v], machineVolumes[v].Name) + if err != nil { + errList = append(errList, err) + } + } + } else { + // volume does not exist, create it and requeue so that it becomes available + volumeID, err := machineScope.CreateVolume(machineVolumes[v]) + machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v] = volumeID + if err != nil { + errList = append(errList, err) + } + result = ctrl.Result{RequeueAfter: 10 * time.Second} } } - return errors.Join(errList...) + return result, errors.Join(errList...) } diff --git a/controllers/ibmvpcmachine_controller_test.go b/controllers/ibmvpcmachine_controller_test.go index 1f75a2ed3..2dd8e1abd 100644 --- a/controllers/ibmvpcmachine_controller_test.go +++ b/controllers/ibmvpcmachine_controller_test.go @@ -709,6 +709,7 @@ func TestReconcileAdditionalVolumes(t *testing.T) { volumeName := "foo-volume" volumeID := "foo-volume-id" volumeGeneratedName := "foo-generated-name" + clusterResourceGroup := "foo-resource-group" setup := func(t *testing.T) (*gomock.Controller, *vpcmock.MockVpc, *scope.MachineScope, IBMVPCMachineReconciler) { t.Helper() @@ -719,6 +720,11 @@ func TestReconcileAdditionalVolumes(t *testing.T) { } machineScope := &scope.MachineScope{ Logger: klog.Background(), + IBMVPCCluster: &infrav1beta2.IBMVPCCluster{ + Spec: infrav1beta2.IBMVPCClusterSpec{ + ResourceGroup: clusterResourceGroup, + }, + }, IBMVPCMachine: &infrav1beta2.IBMVPCMachine{ ObjectMeta: metav1.ObjectMeta{ Name: "capi-machine", @@ -755,26 +761,109 @@ func TestReconcileAdditionalVolumes(t *testing.T) { }}, } + testMachineStatus := infrav1beta2.IBMVPCMachineStatus{ + AdditionalVolumeIDs: []string{volumeID}, + } + emptyVolumeAttachments := vpcv1.VolumeAttachmentCollection{} + volumeAvailableState := vpcv1.VolumeStatusAvailableConst + volumePendingState := vpcv1.VolumeStatusPendingConst + volumeUpdatingState := vpcv1.VolumeStatusUpdatingConst + volumeFailedState := vpcv1.VolumeStatusFailedConst + volumeUnusableState := vpcv1.VolumeStatusUnusableConst t.Run("Should successfully return when no additional volumes are present in spec", func(t *testing.T) { g := NewWithT(t) mockController, _, machineScope, reconciler := setup(t) t.Cleanup(mockController.Finish) - err := reconciler.reconcileAdditionalVolumes(machineScope) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) g.Expect(err).Should(BeNil()) + g.Expect(result).To(Equal(ctrl.Result{})) }) - t.Run("Should successfully create and attach additional volumes", func(t *testing.T) { + t.Run("Should successfully attach volume when volume id is defined and volume is in available state", func(t *testing.T) { g := NewWithT(t) mockController, mockvpc, machineScope, reconciler := setup(t) t.Cleanup(mockController.Finish) - machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes mockvpc.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&emptyVolumeAttachments, nil, nil) - mockvpc.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil) + machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes + machineScope.IBMVPCMachine.Status = testMachineStatus mockvpc.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, nil) - err := reconciler.reconcileAdditionalVolumes(machineScope) + vpcVolume.Status = &volumeAvailableState + mockvpc.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) + g.Expect(err).Should(BeNil()) + g.Expect(result).To(Equal(ctrl.Result{})) + }) + + t.Run("Should requeue when volume is successfully created but in pending state", func(t *testing.T) { + g := NewWithT(t) + mockController, mockvpc, machineScope, reconciler := setup(t) + t.Cleanup(mockController.Finish) + mockvpc.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&emptyVolumeAttachments, nil, nil) + machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes + machineScope.IBMVPCMachine.Status = testMachineStatus + vpcVolume.Status = &volumePendingState + mockvpc.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) + g.Expect(err).Should(BeNil()) + g.Expect(result.RequeueAfter).ToNot(BeZero()) + }) + + t.Run("Should requeue when volume is successfully created but in updating state", func(t *testing.T) { + g := NewWithT(t) + mockController, mockvpc, machineScope, reconciler := setup(t) + t.Cleanup(mockController.Finish) + mockvpc.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&emptyVolumeAttachments, nil, nil) + machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes + machineScope.IBMVPCMachine.Status = testMachineStatus + vpcVolume.Status = &volumeUpdatingState + mockvpc.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) + g.Expect(err).Should(BeNil()) + g.Expect(result.RequeueAfter).ToNot(BeZero()) + }) + + t.Run("Should requeue when volume is in updating state", func(t *testing.T) { + g := NewWithT(t) + mockController, mockvpc, machineScope, reconciler := setup(t) + t.Cleanup(mockController.Finish) + mockvpc.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&emptyVolumeAttachments, nil, nil) + machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes + machineScope.IBMVPCMachine.Status = testMachineStatus + vpcVolume.Status = &volumeUpdatingState + mockvpc.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) g.Expect(err).Should(BeNil()) + g.Expect(result.RequeueAfter).ToNot(BeZero()) + }) + + t.Run("Should return error when volume is in failed state", func(t *testing.T) { + g := NewWithT(t) + mockController, mockvpc, machineScope, reconciler := setup(t) + t.Cleanup(mockController.Finish) + mockvpc.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&emptyVolumeAttachments, nil, nil) + machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes + machineScope.IBMVPCMachine.Status = testMachineStatus + vpcVolume.Status = &volumeFailedState + mockvpc.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) + g.Expect(err).ShouldNot(BeNil()) + g.Expect(result).To(BeZero()) + }) + + t.Run("Should return error when volume is in unusable state", func(t *testing.T) { + g := NewWithT(t) + mockController, mockvpc, machineScope, reconciler := setup(t) + t.Cleanup(mockController.Finish) + mockvpc.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&emptyVolumeAttachments, nil, nil) + machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes + machineScope.IBMVPCMachine.Status = testMachineStatus + vpcVolume.Status = &volumeUnusableState + mockvpc.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) + g.Expect(err).ShouldNot(BeNil()) + g.Expect(result).To(BeZero()) }) t.Run("Should not create new volume if it already exists", func(t *testing.T) { @@ -783,7 +872,8 @@ func TestReconcileAdditionalVolumes(t *testing.T) { t.Cleanup(mockController.Finish) machineScope.IBMVPCMachine.Spec.AdditionalVolumes = additionalVolumes mockvpc.EXPECT().GetVolumeAttachments(gomock.AssignableToTypeOf(&vpcv1.ListInstanceVolumeAttachmentsOptions{})).Return(&testVolumeAttachments, nil, nil) - err := reconciler.reconcileAdditionalVolumes(machineScope) + result, err := reconciler.reconcileAdditionalVolumes(machineScope) g.Expect(err).Should(BeNil()) + g.Expect(result).To(Equal(ctrl.Result{})) }) } diff --git a/pkg/cloud/services/vpc/mock/vpc_generated.go b/pkg/cloud/services/vpc/mock/vpc_generated.go index d214dcabb..f48768529 100644 --- a/pkg/cloud/services/vpc/mock/vpc_generated.go +++ b/pkg/cloud/services/vpc/mock/vpc_generated.go @@ -616,6 +616,22 @@ func (mr *MockVpcMockRecorder) GetVPCZonesByRegion(region any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetVPCZonesByRegion", reflect.TypeOf((*MockVpc)(nil).GetVPCZonesByRegion), region) } +// GetVolume mocks base method. +func (m *MockVpc) GetVolume(options *vpcv1.GetVolumeOptions) (*vpcv1.Volume, *core.DetailedResponse, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetVolume", options) + ret0, _ := ret[0].(*vpcv1.Volume) + ret1, _ := ret[1].(*core.DetailedResponse) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// GetVolume indicates an expected call of GetVolume. +func (mr *MockVpcMockRecorder) GetVolume(options any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetVolume", reflect.TypeOf((*MockVpc)(nil).GetVolume), options) +} + // GetVolumeAttachments mocks base method. func (m *MockVpc) GetVolumeAttachments(options *vpcv1.ListInstanceVolumeAttachmentsOptions) (*vpcv1.VolumeAttachmentCollection, *core.DetailedResponse, error) { m.ctrl.T.Helper() diff --git a/pkg/cloud/services/vpc/service.go b/pkg/cloud/services/vpc/service.go index 0b86ae6ed..34122a3c7 100644 --- a/pkg/cloud/services/vpc/service.go +++ b/pkg/cloud/services/vpc/service.go @@ -536,6 +536,11 @@ func (s *Service) AttachVolumeToInstance(options *vpcv1.CreateInstanceVolumeAtta return s.vpcService.CreateInstanceVolumeAttachment(options) } +// GetVolume fetches the given volume's status. +func (s *Service) GetVolume(options *vpcv1.GetVolumeOptions) (result *vpcv1.Volume, response *core.DetailedResponse, err error) { + return s.vpcService.GetVolume(options) +} + // NewService returns a new VPC Service. func NewService(svcEndpoint string) (Vpc, error) { service := &Service{} diff --git a/pkg/cloud/services/vpc/vpc.go b/pkg/cloud/services/vpc/vpc.go index cf8f3b6e6..075809fdc 100644 --- a/pkg/cloud/services/vpc/vpc.go +++ b/pkg/cloud/services/vpc/vpc.go @@ -75,4 +75,5 @@ type Vpc interface { CreateVolume(options *vpcv1.CreateVolumeOptions) (*vpcv1.Volume, *core.DetailedResponse, error) AttachVolumeToInstance(options *vpcv1.CreateInstanceVolumeAttachmentOptions) (*vpcv1.VolumeAttachment, *core.DetailedResponse, error) GetVolumeAttachments(options *vpcv1.ListInstanceVolumeAttachmentsOptions) (result *vpcv1.VolumeAttachmentCollection, response *core.DetailedResponse, err error) + GetVolume(options *vpcv1.GetVolumeOptions) (result *vpcv1.Volume, response *core.DetailedResponse, err error) }