Skip to content

Commit 4e203a6

Browse files
anshuman-agarwalaanshuman-agarwala
committed
Fixed race condition in additional volumes reconcile
1 parent 336a471 commit 4e203a6

File tree

11 files changed

+305
-56
lines changed

11 files changed

+305
-56
lines changed

‎api/v1beta1/zz_generated.conversion.go‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/v1beta2/ibmvpcmachine_types.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,10 @@ type IBMVPCMachineStatus struct {
184184
// LoadBalancerPoolMembers is the status of IBM Cloud VPC Load Balancer Backend Pools the machine is a member.
185185
// +optional
186186
LoadBalancerPoolMembers []VPCLoadBalancerBackendPoolMember `json:"loadBalancerPoolMembers,omitempty"`
187+
188+
// AdditionalVolumeIDs is a list of Volume ID as per IBMCloud
189+
// +optional
190+
AdditionalVolumeIDs []string `json:"additionalVolumeIDs,omitempty"`
187191
}
188192

189193
// +kubebuilder:object:root=true

‎api/v1beta2/zz_generated.deepcopy.go‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎cloud/scope/machine.go‎

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1168,12 +1168,33 @@ func (m *MachineScope) GetVolumeAttachments() ([]vpcv1.VolumeAttachment, error)
11681168
return result.VolumeAttachments, nil
11691169
}
11701170

1171-
// CreateAndAttachVolume creates a new Volume and attaches it to the instance.
1172-
func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1beta2.VPCVolume) error {
1171+
// GetVolumeState returns the volume's state.
1172+
func (m *MachineScope) GetVolumeState(volumeID string) (string, error) {
1173+
options := vpcv1.GetVolumeOptions{
1174+
ID: &volumeID,
1175+
}
1176+
result, _, err := m.IBMVPCClient.GetVolume(&options)
1177+
if err != nil {
1178+
return "", fmt.Errorf("could not fetch volume status: %w", err)
1179+
}
1180+
return *result.Status, err
1181+
}
1182+
1183+
// CreateVolume creates a new Volume and attaches it to the instance.
1184+
func (m *MachineScope) CreateVolume(vpcVolume *infrav1.VPCVolume) (string, error) {
11731185
volumeOptions := vpcv1.CreateVolumeOptions{}
1186+
var resourceGroupID string
1187+
if m.IBMVPCCluster.Status.ResourceGroup != nil {
1188+
resourceGroupID = m.IBMVPCCluster.Status.ResourceGroup.ID
1189+
} else {
1190+
resourceGroupID = m.IBMVPCCluster.Spec.ResourceGroup
1191+
}
11741192
// TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest
11751193
if vpcVolume.Profile != "custom" {
11761194
volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
1195+
ResourceGroup: &vpcv1.ResourceGroupIdentityByID{
1196+
ID: &resourceGroupID,
1197+
},
11771198
Profile: &vpcv1.VolumeProfileIdentityByName{
11781199
Name: &vpcVolume.Profile,
11791200
},
@@ -1184,6 +1205,9 @@ func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1beta2.VPCVolume)
11841205
}
11851206
} else {
11861207
volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
1208+
ResourceGroup: &vpcv1.ResourceGroupIdentityByID{
1209+
ID: &resourceGroupID,
1210+
},
11871211
Iops: &vpcVolume.Iops,
11881212
Profile: &vpcv1.VolumeProfileIdentityByName{
11891213
Name: &vpcVolume.Profile,
@@ -1197,19 +1221,24 @@ func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1beta2.VPCVolume)
11971221

11981222
volumeResult, _, err := m.IBMVPCClient.CreateVolume(&volumeOptions)
11991223
if err != nil {
1200-
return fmt.Errorf("error while creating volume: %w", err)
1224+
return "", fmt.Errorf("error while creating volume: %w", err)
12011225
}
12021226

1227+
return *volumeResult.ID, nil
1228+
}
1229+
1230+
// AttachVolume attaches the given volume to the instance.
1231+
func (m *MachineScope) AttachVolume(deleteOnInstanceDelete bool, volumeID, volumeName string) error {
12031232
attachmentOptions := vpcv1.CreateInstanceVolumeAttachmentOptions{
12041233
InstanceID: &m.IBMVPCMachine.Status.InstanceID,
12051234
Volume: &vpcv1.VolumeAttachmentPrototypeVolume{
1206-
ID: volumeResult.ID,
1235+
ID: &volumeID,
12071236
},
1208-
DeleteVolumeOnInstanceDelete: &vpcVolume.DeleteVolumeOnInstanceDelete,
1209-
Name: &vpcVolume.Name,
1237+
DeleteVolumeOnInstanceDelete: &deleteOnInstanceDelete,
1238+
Name: &volumeName,
12101239
}
12111240

1212-
_, _, err = m.IBMVPCClient.AttachVolumeToInstance(&attachmentOptions)
1241+
_, _, err := m.IBMVPCClient.AttachVolumeToInstance(&attachmentOptions)
12131242
if err != nil {
12141243
err = fmt.Errorf("error while attaching volume to instance: %w", err)
12151244
}

‎cloud/scope/machine_test.go‎

Lines changed: 96 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ import (
4343
. "github.com/onsi/gomega"
4444
)
4545

46+
var (
47+
volumeName = "foo-volume"
48+
volumeID = "foo-volume-id"
49+
)
50+
4651
func newVPCMachine(clusterName, machineName string) *infrav1.IBMVPCMachine {
4752
return &infrav1.IBMVPCMachine{
4853
ObjectMeta: metav1.ObjectMeta{
@@ -1103,13 +1108,12 @@ func TestGetVolumeAttachments(t *testing.T) {
11031108
return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
11041109
}
11051110

1106-
vpcMachine := infrav1beta2.IBMVPCMachine{
1107-
Status: infrav1beta2.IBMVPCMachineStatus{
1111+
vpcMachine := infrav1.IBMVPCMachine{
1112+
Status: infrav1.IBMVPCMachineStatus{
11081113
InstanceID: "foo-instance-id",
11091114
},
11101115
}
11111116
volumeAttachmentName := "foo-volume-attachment"
1112-
volumeName := "foo-volume"
11131117

11141118
testVolumeAttachments := vpcv1.VolumeAttachmentCollection{
11151119
VolumeAttachments: []vpcv1.VolumeAttachment{{
@@ -1145,70 +1149,134 @@ func TestGetVolumeAttachments(t *testing.T) {
11451149
})
11461150
}
11471151

1148-
func TestCreateAndAttachVolume(t *testing.T) {
1152+
func TestGetVolumeState(t *testing.T) {
11491153
setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
11501154
t.Helper()
11511155
return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
11521156
}
11531157

1154-
volumeName := "foo-volume"
1155-
volumeID := "foo-volume-id"
1158+
volumeStatus := vpcv1.VolumeStatusPendingConst
11561159

1157-
vpcMachine := infrav1beta2.IBMVPCMachine{
1158-
Status: infrav1beta2.IBMVPCMachineStatus{
1160+
vpcMachine := infrav1.IBMVPCMachine{
1161+
Status: infrav1.IBMVPCMachineStatus{
11591162
InstanceID: "foo-instance-id",
11601163
},
11611164
}
11621165

1163-
infraVolume := infrav1beta2.VPCVolume{
1164-
Name: volumeName,
1166+
vpcVolume := vpcv1.Volume{
1167+
Name: &volumeName,
1168+
ID: &volumeID,
1169+
Status: &volumeStatus,
11651170
}
1171+
volumeFetchError := errors.New("error while fetching volume")
11661172

1167-
vpcVolume := vpcv1.Volume{
1168-
Name: &volumeName,
1169-
ID: &volumeID,
1173+
t.Run("Return correct volume state", func(t *testing.T) {
1174+
g := NewWithT(t)
1175+
mockController, mockVPC := setup(t)
1176+
t.Cleanup(mockController.Finish)
1177+
scope := setupMachineScope(clusterName, machineName, mockVPC)
1178+
scope.IBMVPCMachine.Status = vpcMachine.Status
1179+
mockVPC.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil)
1180+
state, err := scope.GetVolumeState(volumeID)
1181+
g.Expect(err).To(BeNil())
1182+
g.Expect(state).To(Equal(volumeStatus))
1183+
})
1184+
1185+
t.Run("Return error when GetVolumeState returns error", func(t *testing.T) {
1186+
g := NewWithT(t)
1187+
mockController, mockVPC := setup(t)
1188+
t.Cleanup(mockController.Finish)
1189+
scope := setupMachineScope(clusterName, machineName, mockVPC)
1190+
scope.IBMVPCMachine.Status = vpcMachine.Status
1191+
mockVPC.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(nil, nil, volumeFetchError)
1192+
state, err := scope.GetVolumeState(volumeID)
1193+
g.Expect(state).To(BeZero())
1194+
g.Expect(errors.Is(err, volumeFetchError)).To(BeTrue())
1195+
})
1196+
}
1197+
1198+
func TestCreateVolume(t *testing.T) {
1199+
setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
1200+
t.Helper()
1201+
return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
11701202
}
11711203

1172-
volumeCreationError := errors.New("error while creating volume")
1204+
vpcMachine := infrav1.IBMVPCMachine{
1205+
Status: infrav1.IBMVPCMachineStatus{
1206+
InstanceID: "foo-instance-id",
1207+
},
1208+
}
11731209

1174-
volumeAttachmentError := errors.New("error while attaching volume")
1210+
infraVolume := infrav1.VPCVolume{
1211+
Name: volumeName,
1212+
Profile: "custom",
1213+
Iops: 100,
1214+
SizeGiB: 50,
1215+
}
1216+
pendingState := vpcv1.VolumeStatusPendingConst
1217+
1218+
vpcVolume := vpcv1.Volume{
1219+
Name: &volumeName,
1220+
ID: &volumeID,
1221+
Status: &pendingState,
1222+
}
11751223

1176-
t.Run("Volume creation and attachment is successful", func(t *testing.T) {
1224+
volumeCreationError := errors.New("error while creating volume")
1225+
t.Run("Volume creation is successful", func(t *testing.T) {
11771226
g := NewWithT(t)
11781227
mockController, mockVPC := setup(t)
11791228
t.Cleanup(mockController.Finish)
11801229
scope := setupMachineScope(clusterName, machineName, mockVPC)
1181-
scope.IBMVPCMachine.Status = vpcMachine.Status
11821230
mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil)
1183-
mockVPC.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, nil)
1184-
1185-
err := scope.CreateAndAttachVolume(&infraVolume)
1231+
id, err := scope.CreateVolume(&infraVolume)
11861232
g.Expect(err).Should(Succeed())
1233+
g.Expect(id).Should(Equal(volumeID))
11871234
})
1188-
1189-
t.Run("Volume Creation Fails", func(t *testing.T) {
1235+
t.Run("Volume creation fails", func(t *testing.T) {
11901236
g := NewWithT(t)
11911237
mockController, mockVPC := setup(t)
11921238
t.Cleanup(mockController.Finish)
11931239
scope := setupMachineScope(clusterName, machineName, mockVPC)
11941240
scope.IBMVPCMachine.Status = vpcMachine.Status
11951241
mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(nil, nil, volumeCreationError)
1196-
1197-
err := scope.CreateAndAttachVolume(&infraVolume)
1242+
id, err := scope.CreateVolume(&infraVolume)
11981243
g.Expect(err).ShouldNot(Succeed())
11991244
g.Expect(errors.Is(err, volumeCreationError)).To(BeTrue())
1245+
g.Expect(id).To(BeZero())
12001246
})
1247+
}
1248+
1249+
func TestAttachVolume(t *testing.T) {
1250+
setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
1251+
t.Helper()
1252+
return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
1253+
}
12011254

1202-
t.Run("Volume Attachment Fails", func(t *testing.T) {
1255+
deleteOnInstanceDelete := true
1256+
vpcMachine := infrav1.IBMVPCMachine{
1257+
Status: infrav1.IBMVPCMachineStatus{
1258+
InstanceID: "foo-instance-id",
1259+
},
1260+
}
1261+
volumeAttachmentError := errors.New("error while attaching volume")
1262+
t.Run("Volume attachment is successful", func(t *testing.T) {
1263+
g := NewWithT(t)
1264+
mockController, mockVPC := setup(t)
1265+
t.Cleanup(mockController.Finish)
1266+
scope := setupMachineScope(clusterName, machineName, mockVPC)
1267+
scope.IBMVPCMachine.Status = vpcMachine.Status
1268+
mockVPC.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, nil)
1269+
err := scope.AttachVolume(deleteOnInstanceDelete, volumeID, volumeName)
1270+
g.Expect(err).Should(Succeed())
1271+
})
1272+
t.Run("Volume attachment fails", func(t *testing.T) {
12031273
g := NewWithT(t)
12041274
mockController, mockVPC := setup(t)
12051275
t.Cleanup(mockController.Finish)
12061276
scope := setupMachineScope(clusterName, machineName, mockVPC)
12071277
scope.IBMVPCMachine.Status = vpcMachine.Status
1208-
mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil)
12091278
mockVPC.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, volumeAttachmentError)
1210-
1211-
err := scope.CreateAndAttachVolume(&infraVolume)
1279+
err := scope.AttachVolume(deleteOnInstanceDelete, volumeID, volumeName)
12121280
g.Expect(err).ShouldNot(Succeed())
12131281
g.Expect(errors.Is(err, volumeAttachmentError)).To(BeTrue())
12141282
})

‎config/crd/bases/infrastructure.cluster.x-k8s.io_ibmvpcmachines.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,11 @@ spec:
539539
status:
540540
description: IBMVPCMachineStatus defines the observed state of IBMVPCMachine.
541541
properties:
542+
additionalVolumeIDs:
543+
description: AdditionalVolumeIDs is a list of Volume ID as per IBMCloud
544+
items:
545+
type: string
546+
type: array
542547
addresses:
543548
description: Addresses contains the IBM Cloud instance associated
544549
addresses.

‎controllers/ibmvpcmachine_controller.go‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -259,15 +259,16 @@ func (r *IBMVPCMachineReconciler) reconcileNormal(machineScope *scope.MachineSco
259259
}
260260

261261
// Handle Additional Volumes
262-
err = r.reconcileAdditionalVolumes(machineScope)
262+
var result ctrl.Result
263+
result, err = r.reconcileAdditionalVolumes(machineScope)
263264
if err != nil {
264265
return ctrl.Result{}, fmt.Errorf("error reconciling additional volumes: %w", err)
265266
}
266267

267268
// With a running machine and all Load Balancer Pool Members reconciled, mark machine as ready.
268269
machineScope.SetReady()
269270
v1beta1conditions.MarkTrue(machineScope.IBMVPCMachine, infrav1.InstanceReadyCondition)
270-
return ctrl.Result{}, nil
271+
return result, nil
271272
}
272273

273274
func (r *IBMVPCMachineReconciler) getOrCreate(scope *scope.MachineScope) (*vpcv1.Instance, error) {
@@ -299,14 +300,18 @@ func (r *IBMVPCMachineReconciler) reconcileDelete(scope *scope.MachineScope) (_
299300
return ctrl.Result{}, nil
300301
}
301302

302-
func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope.MachineScope) error {
303+
func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope.MachineScope) (ctrl.Result, error) {
304+
if machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs == nil {
305+
machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs = make([]string, len(machineScope.IBMVPCMachine.Spec.AdditionalVolumes))
306+
}
303307
machineVolumes := machineScope.IBMVPCMachine.Spec.AdditionalVolumes
308+
result := ctrl.Result{}
304309
if len(machineVolumes) == 0 {
305-
return nil
310+
return result, nil
306311
}
307312
volumeAttachmentList, err := machineScope.GetVolumeAttachments()
308313
if err != nil {
309-
return err
314+
return result, err
310315
}
311316
volumeAttachmentNames := sets.New[string]()
312317
for i := range volumeAttachmentList {
@@ -316,15 +321,35 @@ func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope
316321
// Read through the list, checking if volume exists and create volume if it does not
317322
for v := range machineVolumes {
318323
if volumeAttachmentNames.Has(machineVolumes[v].Name) {
319-
// volume attachment has been created so volume will eventually be attached
324+
// volume attachment has been created so volume is already attached
320325
continue
321326
}
322-
323-
err = machineScope.CreateAndAttachVolume(machineVolumes[v])
324-
if err != nil {
325-
errList = append(errList, err)
326-
continue
327+
if machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v] != "" {
328+
// volume was already created, fetch volume status and attach if possible
329+
state, err := machineScope.GetVolumeState(machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v])
330+
if err != nil {
331+
errList = append(errList, err)
332+
}
333+
switch state {
334+
case vpcv1.VolumeStatusPendingConst, vpcv1.VolumeStatusUpdatingConst:
335+
result = ctrl.Result{RequeueAfter: 10 * time.Second}
336+
case vpcv1.VolumeStatusFailedConst, vpcv1.VolumeStatusUnusableConst:
337+
errList = append(errList, fmt.Errorf("volume in unexpected state: %s", state))
338+
case vpcv1.VolumeStatusAvailableConst:
339+
err = machineScope.AttachVolume(machineVolumes[v].DeleteVolumeOnInstanceDelete, machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v], machineVolumes[v].Name)
340+
if err != nil {
341+
errList = append(errList, err)
342+
}
343+
}
344+
} else {
345+
// volume does not exist, create it and requeue so that it becomes available
346+
volumeID, err := machineScope.CreateVolume(machineVolumes[v])
347+
machineScope.IBMVPCMachine.Status.AdditionalVolumeIDs[v] = volumeID
348+
if err != nil {
349+
errList = append(errList, err)
350+
}
351+
result = ctrl.Result{RequeueAfter: 10 * time.Second}
327352
}
328353
}
329-
return errors.Join(errList...)
354+
return result, errors.Join(errList...)
330355
}

0 commit comments

Comments
 (0)