Skip to content

Commit c8a60db

Browse files
anshuman-agarwalaanshuman-agarwala
committed
Fixed race condition in additional volumes reconcile
1 parent 341627e commit c8a60db

File tree

10 files changed

+418
-49
lines changed

10 files changed

+418
-49
lines changed

‎api/v1beta2/ibmvpcmachine_types.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,9 @@ type IBMVPCMachineV1Beta2Status struct {
199199
// +listMapKey=type
200200
// +kubebuilder:validation:MaxItems=32
201201
Conditions []metav1.Condition `json:"conditions,omitempty"`
202+
// AdditionalVolumeIDs is a list of Volume ID as per IBMCloud
203+
// +optional
204+
AdditionalVolumeIDs []string `json:"additionalVolumeIDs,omitempty"`
202205
}
203206

204207
// +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
@@ -1175,12 +1175,33 @@ func (m *MachineScope) GetVolumeAttachments() ([]vpcv1.VolumeAttachment, error)
11751175
return result.VolumeAttachments, nil
11761176
}
11771177

1178-
// CreateAndAttachVolume creates a new Volume and attaches it to the instance.
1179-
func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1.VPCVolume) error {
1178+
// GetVolumeState returns the volume's state.
1179+
func (m *MachineScope) GetVolumeState(volumeID string) (string, error) {
1180+
options := vpcv1.GetVolumeOptions{
1181+
ID: &volumeID,
1182+
}
1183+
result, _, err := m.IBMVPCClient.GetVolume(&options)
1184+
if err != nil {
1185+
return "", fmt.Errorf("could not fetch volume status: %w", err)
1186+
}
1187+
return *result.Status, err
1188+
}
1189+
1190+
// CreateVolume creates a new Volume and attaches it to the instance.
1191+
func (m *MachineScope) CreateVolume(vpcVolume *infrav1.VPCVolume) (string, error) {
11801192
volumeOptions := vpcv1.CreateVolumeOptions{}
1193+
var resourceGroupID string
1194+
if m.IBMVPCCluster.Status.ResourceGroup != nil {
1195+
resourceGroupID = m.IBMVPCCluster.Status.ResourceGroup.ID
1196+
} else {
1197+
resourceGroupID = m.IBMVPCCluster.Spec.ResourceGroup
1198+
}
11811199
// TODO: EncryptionKeyCRN is not supported for now, the field is omitted from the manifest
11821200
if vpcVolume.Profile != "custom" {
11831201
volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
1202+
ResourceGroup: &vpcv1.ResourceGroupIdentityByID{
1203+
ID: &resourceGroupID,
1204+
},
11841205
Profile: &vpcv1.VolumeProfileIdentityByName{
11851206
Name: &vpcVolume.Profile,
11861207
},
@@ -1191,6 +1212,9 @@ func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1.VPCVolume) error
11911212
}
11921213
} else {
11931214
volumeOptions.VolumePrototype = &vpcv1.VolumePrototype{
1215+
ResourceGroup: &vpcv1.ResourceGroupIdentityByID{
1216+
ID: &resourceGroupID,
1217+
},
11941218
Iops: &vpcVolume.Iops,
11951219
Profile: &vpcv1.VolumeProfileIdentityByName{
11961220
Name: &vpcVolume.Profile,
@@ -1204,19 +1228,24 @@ func (m *MachineScope) CreateAndAttachVolume(vpcVolume *infrav1.VPCVolume) error
12041228

12051229
volumeResult, _, err := m.IBMVPCClient.CreateVolume(&volumeOptions)
12061230
if err != nil {
1207-
return fmt.Errorf("error while creating volume: %w", err)
1231+
return "", fmt.Errorf("error while creating volume: %w", err)
12081232
}
12091233

1234+
return *volumeResult.ID, nil
1235+
}
1236+
1237+
// AttachVolume attaches the given volume to the instance.
1238+
func (m *MachineScope) AttachVolume(deleteOnInstanceDelete bool, volumeID, volumeName string) error {
12101239
attachmentOptions := vpcv1.CreateInstanceVolumeAttachmentOptions{
12111240
InstanceID: &m.IBMVPCMachine.Status.InstanceID,
12121241
Volume: &vpcv1.VolumeAttachmentPrototypeVolume{
1213-
ID: volumeResult.ID,
1242+
ID: &volumeID,
12141243
},
1215-
DeleteVolumeOnInstanceDelete: &vpcVolume.DeleteVolumeOnInstanceDelete,
1216-
Name: &vpcVolume.Name,
1244+
DeleteVolumeOnInstanceDelete: &deleteOnInstanceDelete,
1245+
Name: &volumeName,
12171246
}
12181247

1219-
_, _, err = m.IBMVPCClient.AttachVolumeToInstance(&attachmentOptions)
1248+
_, _, err := m.IBMVPCClient.AttachVolumeToInstance(&attachmentOptions)
12201249
if err != nil {
12211250
err = fmt.Errorf("error while attaching volume to instance: %w", err)
12221251
}

‎cloud/scope/machine_test.go‎

Lines changed: 91 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ import (
4242
. "github.com/onsi/gomega"
4343
)
4444

45+
var (
46+
volumeName = "foo-volume"
47+
volumeID = "foo-volume-id"
48+
)
49+
4550
func newVPCMachine(clusterName, machineName string) *infrav1.IBMVPCMachine {
4651
return &infrav1.IBMVPCMachine{
4752
ObjectMeta: metav1.ObjectMeta{
@@ -1107,7 +1112,6 @@ func TestGetVolumeAttachments(t *testing.T) {
11071112
},
11081113
}
11091114
volumeAttachmentName := "foo-volume-attachment"
1110-
volumeName := "foo-volume"
11111115

11121116
testVolumeAttachments := vpcv1.VolumeAttachmentCollection{
11131117
VolumeAttachments: []vpcv1.VolumeAttachment{{
@@ -1143,14 +1147,57 @@ func TestGetVolumeAttachments(t *testing.T) {
11431147
})
11441148
}
11451149

1146-
func TestCreateAndAttachVolume(t *testing.T) {
1150+
func TestGetVolumeState(t *testing.T) {
11471151
setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
11481152
t.Helper()
11491153
return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
11501154
}
11511155

1152-
volumeName := "foo-volume"
1153-
volumeID := "foo-volume-id"
1156+
volumeStatus := vpcv1.VolumeStatusPendingConst
1157+
1158+
vpcMachine := infrav1.IBMVPCMachine{
1159+
Status: infrav1.IBMVPCMachineStatus{
1160+
InstanceID: "foo-instance-id",
1161+
},
1162+
}
1163+
1164+
vpcVolume := vpcv1.Volume{
1165+
Name: &volumeName,
1166+
ID: &volumeID,
1167+
Status: &volumeStatus,
1168+
}
1169+
volumeFetchError := errors.New("error while fetching volume")
1170+
1171+
t.Run("Return correct volume state", func(t *testing.T) {
1172+
g := NewWithT(t)
1173+
mockController, mockVPC := setup(t)
1174+
t.Cleanup(mockController.Finish)
1175+
scope := setupMachineScope(clusterName, machineName, mockVPC)
1176+
scope.IBMVPCMachine.Status = vpcMachine.Status
1177+
mockVPC.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(&vpcVolume, nil, nil)
1178+
state, err := scope.GetVolumeState(volumeID)
1179+
g.Expect(err).To(BeNil())
1180+
g.Expect(state).To(Equal(volumeStatus))
1181+
})
1182+
1183+
t.Run("Return error when GetVolumeState returns error", func(t *testing.T) {
1184+
g := NewWithT(t)
1185+
mockController, mockVPC := setup(t)
1186+
t.Cleanup(mockController.Finish)
1187+
scope := setupMachineScope(clusterName, machineName, mockVPC)
1188+
scope.IBMVPCMachine.Status = vpcMachine.Status
1189+
mockVPC.EXPECT().GetVolume(gomock.AssignableToTypeOf(&vpcv1.GetVolumeOptions{})).Return(nil, nil, volumeFetchError)
1190+
state, err := scope.GetVolumeState(volumeID)
1191+
g.Expect(state).To(BeZero())
1192+
g.Expect(errors.Is(err, volumeFetchError)).To(BeTrue())
1193+
})
1194+
}
1195+
1196+
func TestCreateVolume(t *testing.T) {
1197+
setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
1198+
t.Helper()
1199+
return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
1200+
}
11541201

11551202
vpcMachine := infrav1.IBMVPCMachine{
11561203
Status: infrav1.IBMVPCMachineStatus{
@@ -1159,54 +1206,75 @@ func TestCreateAndAttachVolume(t *testing.T) {
11591206
}
11601207

11611208
infraVolume := infrav1.VPCVolume{
1162-
Name: volumeName,
1209+
Name: volumeName,
1210+
Profile: "custom",
1211+
Iops: 100,
1212+
SizeGiB: 50,
11631213
}
1214+
pendingState := vpcv1.VolumeStatusPendingConst
11641215

11651216
vpcVolume := vpcv1.Volume{
1166-
Name: &volumeName,
1167-
ID: &volumeID,
1217+
Name: &volumeName,
1218+
ID: &volumeID,
1219+
Status: &pendingState,
11681220
}
11691221

11701222
volumeCreationError := errors.New("error while creating volume")
1171-
1172-
volumeAttachmentError := errors.New("error while attaching volume")
1173-
1174-
t.Run("Volume creation and attachment is successful", func(t *testing.T) {
1223+
t.Run("Volume creation is successful", func(t *testing.T) {
11751224
g := NewWithT(t)
11761225
mockController, mockVPC := setup(t)
11771226
t.Cleanup(mockController.Finish)
11781227
scope := setupMachineScope(clusterName, machineName, mockVPC)
1179-
scope.IBMVPCMachine.Status = vpcMachine.Status
11801228
mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(&vpcVolume, nil, nil)
1181-
mockVPC.EXPECT().AttachVolumeToInstance(gomock.AssignableToTypeOf(&vpcv1.CreateInstanceVolumeAttachmentOptions{})).Return(nil, nil, nil)
1182-
1183-
err := scope.CreateAndAttachVolume(&infraVolume)
1229+
id, err := scope.CreateVolume(&infraVolume)
11841230
g.Expect(err).Should(Succeed())
1231+
g.Expect(id).Should(Equal(volumeID))
11851232
})
1186-
1187-
t.Run("Volume Creation Fails", func(t *testing.T) {
1233+
t.Run("Volume creation fails", func(t *testing.T) {
11881234
g := NewWithT(t)
11891235
mockController, mockVPC := setup(t)
11901236
t.Cleanup(mockController.Finish)
11911237
scope := setupMachineScope(clusterName, machineName, mockVPC)
11921238
scope.IBMVPCMachine.Status = vpcMachine.Status
11931239
mockVPC.EXPECT().CreateVolume(gomock.AssignableToTypeOf(&vpcv1.CreateVolumeOptions{})).Return(nil, nil, volumeCreationError)
1194-
1195-
err := scope.CreateAndAttachVolume(&infraVolume)
1240+
id, err := scope.CreateVolume(&infraVolume)
11961241
g.Expect(err).ShouldNot(Succeed())
11971242
g.Expect(errors.Is(err, volumeCreationError)).To(BeTrue())
1243+
g.Expect(id).To(BeZero())
11981244
})
1245+
}
1246+
1247+
func TestAttachVolume(t *testing.T) {
1248+
setup := func(t *testing.T) (*gomock.Controller, *mock.MockVpc) {
1249+
t.Helper()
1250+
return gomock.NewController(t), mock.NewMockVpc(gomock.NewController(t))
1251+
}
11991252

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

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -690,6 +690,12 @@ spec:
690690
description: V1beta2 groups all the fields that will be added or modified
691691
in IBMVPCMachine's status with the V1Beta2 version.
692692
properties:
693+
additionalVolumeIDs:
694+
description: AdditionalVolumeIDs is a list of Volume ID as per
695+
IBMCloud
696+
items:
697+
type: string
698+
type: array
693699
conditions:
694700
description: Conditions represents the observations of a IBMVPCMachine's
695701
current state.

‎controllers/ibmvpcmachine_controller.go‎

Lines changed: 55 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
apierrors "k8s.io/apimachinery/pkg/api/errors"
3030
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3131
"k8s.io/apimachinery/pkg/runtime"
32+
"k8s.io/apimachinery/pkg/types"
3233
kerrors "k8s.io/apimachinery/pkg/util/errors"
3334
"k8s.io/apimachinery/pkg/util/sets"
3435
"k8s.io/client-go/tools/record"
@@ -325,7 +326,8 @@ func (r *IBMVPCMachineReconciler) reconcileNormal(ctx context.Context, machineSc
325326
}
326327

327328
// Handle Additional Volumes
328-
err = r.reconcileAdditionalVolumes(machineScope)
329+
var result ctrl.Result
330+
result, err = r.reconcileAdditionalVolumes(ctx, machineScope)
329331
if err != nil {
330332
return ctrl.Result{}, fmt.Errorf("error reconciling additional volumes: %w", err)
331333
}
@@ -338,7 +340,8 @@ func (r *IBMVPCMachineReconciler) reconcileNormal(ctx context.Context, machineSc
338340
Status: metav1.ConditionTrue,
339341
Reason: infrav1.IBMVPCMachineInstanceReadyV1Beta2Reason,
340342
})
341-
return ctrl.Result{}, nil
343+
log.Info("Reconcile complete", "result", result)
344+
return result, nil
342345
}
343346

344347
func (r *IBMVPCMachineReconciler) getOrCreate(ctx context.Context, scope *scope.MachineScope) (*vpcv1.Instance, error) {
@@ -423,14 +426,28 @@ func patchIBMVPCMachine(ctx context.Context, patchHelper *v1beta1patch.Helper, i
423426
clusterv1beta1.PausedV1Beta2Condition,
424427
}})
425428
}
426-
func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope.MachineScope) error {
429+
func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(ctx context.Context, machineScope *scope.MachineScope) (ctrl.Result, error) {
430+
log := ctrl.LoggerFrom(ctx)
431+
// Return immediately if no additional volumes exist
432+
if len(machineScope.IBMVPCMachine.Spec.AdditionalVolumes) == 0 {
433+
return ctrl.Result{}, nil
434+
}
435+
err := r.Get(ctx, types.NamespacedName{
436+
Namespace: machineScope.IBMVPCMachine.Namespace,
437+
Name: machineScope.IBMVPCMachine.Name,
438+
}, machineScope.IBMVPCMachine)
439+
if err != nil {
440+
log.Error(err, "Could not fetch machine status")
441+
return ctrl.Result{}, err
442+
}
427443
machineVolumes := machineScope.IBMVPCMachine.Spec.AdditionalVolumes
428-
if len(machineVolumes) == 0 {
429-
return nil
444+
result := ctrl.Result{}
445+
if machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs == nil {
446+
machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs = make([]string, len(machineScope.IBMVPCMachine.Spec.AdditionalVolumes))
430447
}
431448
volumeAttachmentList, err := machineScope.GetVolumeAttachments()
432449
if err != nil {
433-
return err
450+
return result, err
434451
}
435452
volumeAttachmentNames := sets.New[string]()
436453
for i := range volumeAttachmentList {
@@ -440,15 +457,40 @@ func (r *IBMVPCMachineReconciler) reconcileAdditionalVolumes(machineScope *scope
440457
// Read through the list, checking if volume exists and create volume if it does not
441458
for v := range machineVolumes {
442459
if volumeAttachmentNames.Has(machineVolumes[v].Name) {
443-
// volume attachment has been created so volume will eventually be attached
460+
// volume attachment has been created so volume is already attached
444461
continue
445462
}
446-
447-
err = machineScope.CreateAndAttachVolume(machineVolumes[v])
448-
if err != nil {
449-
errList = append(errList, err)
450-
continue
463+
if machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs[v] != "" {
464+
// volume was already created, fetch volume status and attach if possible
465+
state, err := machineScope.GetVolumeState(machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs[v])
466+
if err != nil {
467+
errList = append(errList, err)
468+
}
469+
switch state {
470+
case vpcv1.VolumeStatusPendingConst, vpcv1.VolumeStatusUpdatingConst:
471+
result = ctrl.Result{RequeueAfter: 10 * time.Second}
472+
case vpcv1.VolumeStatusFailedConst, vpcv1.VolumeStatusUnusableConst:
473+
errList = append(errList, fmt.Errorf("volume in unexpected state: %s", state))
474+
case vpcv1.VolumeStatusAvailableConst:
475+
log.Info("Volume is in available state, trying to attach it", "VolumeID", machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs[v])
476+
err = machineScope.AttachVolume(machineVolumes[v].DeleteVolumeOnInstanceDelete, machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs[v], machineVolumes[v].Name)
477+
if err != nil {
478+
log.Error(err, "Error while attaching volume", "VolumeID", machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs[v])
479+
errList = append(errList, err)
480+
}
481+
log.Info("Successfully attached volume", "VolumeID", machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs[v])
482+
}
483+
} else {
484+
// volume does not exist, create it and requeue so that it becomes available
485+
volumeID, err := machineScope.CreateVolume(machineVolumes[v])
486+
machineScope.IBMVPCMachine.Status.V1Beta2.AdditionalVolumeIDs[v] = volumeID
487+
if err != nil {
488+
log.Error(err, "Could not update Machine status. Created Volume needs to be cleaned up manually", "VolumeID", volumeID)
489+
errList = append(errList, err)
490+
}
491+
log.Info("Created new volume", "name", machineVolumes[v].Name, "VolumeID", volumeID)
492+
result = ctrl.Result{RequeueAfter: 10 * time.Second}
451493
}
452494
}
453-
return errors.Join(errList...)
495+
return result, errors.Join(errList...)
454496
}

0 commit comments

Comments
 (0)