From 315c7907964e55938952852b2035756c9de674a8 Mon Sep 17 00:00:00 2001 From: Katarzyna Kulpa Date: Tue, 10 Dec 2024 12:26:50 +0100 Subject: [PATCH 1/6] issue-1256 fixing drive removal --- pkg/crcontrollers/drive/drivecontroller.go | 12 ++++-- .../drive/drivecontroller_test.go | 37 +++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/pkg/crcontrollers/drive/drivecontroller.go b/pkg/crcontrollers/drive/drivecontroller.go index d915dcb75..a83f0b5c4 100644 --- a/pkg/crcontrollers/drive/drivecontroller.go +++ b/pkg/crcontrollers/drive/drivecontroller.go @@ -181,13 +181,14 @@ func (c *Controller) handleDriveUpdate(ctx context.Context, log *logrus.Entry, d break } + manualRemovalNotReady := c.driveManualRemovalNotReady(drive.Annotations) + value, foundAllDRVolFakeAttach := drive.Annotations[allDRVolumesFakeAttachedAnnotation] fakeAttachDR := !drive.Spec.IsClean && foundAllDRVolFakeAttach && value == allDRVolumesFakeAttachedKey - if drive.Spec.IsClean || fakeAttachDR { + if (drive.Spec.IsClean || fakeAttachDR) && manualRemovalNotReady { log.Infof("Initiating automatic removal of drive: %s", drive.GetName()) } else { - status, found := getDriveAnnotationRemoval(drive.Annotations) - if !found || status != apiV1.DriveAnnotationRemovalReady { + if manualRemovalNotReady { break } } @@ -607,3 +608,8 @@ func (c *Controller) handleDriveLableUpdate(ctx context.Context, log *logrus.Ent log.Infof("Update AC %s labels to related drive %s successful", ac.GetName(), drive.GetName()) return nil } + +func (c *Controller) driveManualRemovalNotReady(annotations map[string]string) bool { + status, found := getDriveAnnotationRemoval(annotations) + return !found || status != apiV1.DriveAnnotationRemovalReady +} \ No newline at end of file diff --git a/pkg/crcontrollers/drive/drivecontroller_test.go b/pkg/crcontrollers/drive/drivecontroller_test.go index f646cc621..40821eeca 100644 --- a/pkg/crcontrollers/drive/drivecontroller_test.go +++ b/pkg/crcontrollers/drive/drivecontroller_test.go @@ -1407,3 +1407,40 @@ func TestCheckLVGVolumeWithoutFakeAttachRemoved(t *testing.T) { }) } } + +func TestDriveManualRemovalNotReady(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + want bool + }{ + { + name: "No annotations", + annotations: map[string]string{}, + want: true, + }, + { + name: "Annotation exists with different value", + annotations: map[string]string{ + apiV1.DriveAnnotationRemoval: "some_value", + }, + want: true, + }, + { + name: "Annotation exists with correct value", + annotations: map[string]string{ + apiV1.DriveAnnotationRemoval: apiV1.DriveAnnotationRemovalReady, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Controller{} + if got := c.driveManualRemovalNotReady(tt.annotations); got != tt.want { + t.Errorf("driveManualRemovalNotReady() = %v, want %v", got, tt.want) + } + }) + } +} \ No newline at end of file From f66df94323ff2f85bc95484eccc75c975be5553a Mon Sep 17 00:00:00 2001 From: Katarzyna Kulpa Date: Tue, 10 Dec 2024 13:01:29 +0100 Subject: [PATCH 2/6] issue-1256 lint --- pkg/crcontrollers/drive/drivecontroller.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/crcontrollers/drive/drivecontroller.go b/pkg/crcontrollers/drive/drivecontroller.go index a83f0b5c4..e46e205d9 100644 --- a/pkg/crcontrollers/drive/drivecontroller.go +++ b/pkg/crcontrollers/drive/drivecontroller.go @@ -187,10 +187,8 @@ func (c *Controller) handleDriveUpdate(ctx context.Context, log *logrus.Entry, d fakeAttachDR := !drive.Spec.IsClean && foundAllDRVolFakeAttach && value == allDRVolumesFakeAttachedKey if (drive.Spec.IsClean || fakeAttachDR) && manualRemovalNotReady { log.Infof("Initiating automatic removal of drive: %s", drive.GetName()) - } else { - if manualRemovalNotReady { + } else if manualRemovalNotReady { break - } } toUpdate = true drive.Spec.Usage = apiV1.DriveUsageRemoving From 469d11f89187ee47ee4137d006e2d6118bb6eb37 Mon Sep 17 00:00:00 2001 From: Katarzyna Kulpa Date: Tue, 10 Dec 2024 13:16:37 +0100 Subject: [PATCH 3/6] issue-1256 fmt --- pkg/crcontrollers/drive/drivecontroller.go | 6 +++--- pkg/crcontrollers/drive/drivecontroller_test.go | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/crcontrollers/drive/drivecontroller.go b/pkg/crcontrollers/drive/drivecontroller.go index e46e205d9..9c715321d 100644 --- a/pkg/crcontrollers/drive/drivecontroller.go +++ b/pkg/crcontrollers/drive/drivecontroller.go @@ -181,14 +181,14 @@ func (c *Controller) handleDriveUpdate(ctx context.Context, log *logrus.Entry, d break } - manualRemovalNotReady := c.driveManualRemovalNotReady(drive.Annotations) + manualRemovalNotReady := c.driveManualRemovalNotReady(drive.Annotations) value, foundAllDRVolFakeAttach := drive.Annotations[allDRVolumesFakeAttachedAnnotation] fakeAttachDR := !drive.Spec.IsClean && foundAllDRVolFakeAttach && value == allDRVolumesFakeAttachedKey if (drive.Spec.IsClean || fakeAttachDR) && manualRemovalNotReady { log.Infof("Initiating automatic removal of drive: %s", drive.GetName()) } else if manualRemovalNotReady { - break + break } toUpdate = true drive.Spec.Usage = apiV1.DriveUsageRemoving @@ -610,4 +610,4 @@ func (c *Controller) handleDriveLableUpdate(ctx context.Context, log *logrus.Ent func (c *Controller) driveManualRemovalNotReady(annotations map[string]string) bool { status, found := getDriveAnnotationRemoval(annotations) return !found || status != apiV1.DriveAnnotationRemovalReady -} \ No newline at end of file +} diff --git a/pkg/crcontrollers/drive/drivecontroller_test.go b/pkg/crcontrollers/drive/drivecontroller_test.go index 40821eeca..becc74f00 100644 --- a/pkg/crcontrollers/drive/drivecontroller_test.go +++ b/pkg/crcontrollers/drive/drivecontroller_test.go @@ -1410,14 +1410,14 @@ func TestCheckLVGVolumeWithoutFakeAttachRemoved(t *testing.T) { func TestDriveManualRemovalNotReady(t *testing.T) { tests := []struct { - name string + name string annotations map[string]string - want bool + want bool }{ { - name: "No annotations", + name: "No annotations", annotations: map[string]string{}, - want: true, + want: true, }, { name: "Annotation exists with different value", @@ -1443,4 +1443,4 @@ func TestDriveManualRemovalNotReady(t *testing.T) { } }) } -} \ No newline at end of file +} From 2a31e73443435a1d97c804fa18273c00c4fbe9c3 Mon Sep 17 00:00:00 2001 From: Katarzyna Kulpa Date: Tue, 17 Dec 2024 07:33:49 +0100 Subject: [PATCH 4/6] [issue-1256] disk eject auto --- api/v1/constants.go | 3 ++ pkg/crcontrollers/drive/drivecontroller.go | 17 +++--- .../drive/drivecontroller_test.go | 52 +++++++------------ 3 files changed, 32 insertions(+), 40 deletions(-) diff --git a/api/v1/constants.go b/api/v1/constants.go index 41e2c1253..12c83f093 100644 --- a/api/v1/constants.go +++ b/api/v1/constants.go @@ -69,6 +69,9 @@ const ( DriveAnnotationVolumeStatusPrefix = "status" // Deprecated annotations DriveAnnotationReplacement = "replacement" + // Disk auto eject + DriveAnnotationEject = "eject" + DriveAnnotationEjectAuto = "auto" // Volume operational status OperationalStatusOperative = "OPERATIVE" diff --git a/pkg/crcontrollers/drive/drivecontroller.go b/pkg/crcontrollers/drive/drivecontroller.go index 9c715321d..22ed87376 100644 --- a/pkg/crcontrollers/drive/drivecontroller.go +++ b/pkg/crcontrollers/drive/drivecontroller.go @@ -181,14 +181,15 @@ func (c *Controller) handleDriveUpdate(ctx context.Context, log *logrus.Entry, d break } - manualRemovalNotReady := c.driveManualRemovalNotReady(drive.Annotations) - value, foundAllDRVolFakeAttach := drive.Annotations[allDRVolumesFakeAttachedAnnotation] fakeAttachDR := !drive.Spec.IsClean && foundAllDRVolFakeAttach && value == allDRVolumesFakeAttachedKey - if (drive.Spec.IsClean || fakeAttachDR) && manualRemovalNotReady { + if (drive.Spec.IsClean || fakeAttachDR) && c.driveEjectAuto(drive.Annotations) { log.Infof("Initiating automatic removal of drive: %s", drive.GetName()) - } else if manualRemovalNotReady { - break + } else { + status, found := getDriveAnnotationRemoval(drive.Annotations) + if !found || status != apiV1.DriveAnnotationRemovalReady { + break + } } toUpdate = true drive.Spec.Usage = apiV1.DriveUsageRemoving @@ -607,7 +608,7 @@ func (c *Controller) handleDriveLableUpdate(ctx context.Context, log *logrus.Ent return nil } -func (c *Controller) driveManualRemovalNotReady(annotations map[string]string) bool { - status, found := getDriveAnnotationRemoval(annotations) - return !found || status != apiV1.DriveAnnotationRemovalReady +func (c *Controller) driveEjectAuto(annotations map[string]string) bool { + status, found := annotations[apiV1.DriveAnnotationEject] + return !found || status != apiV1.DriveAnnotationEjectAuto } diff --git a/pkg/crcontrollers/drive/drivecontroller_test.go b/pkg/crcontrollers/drive/drivecontroller_test.go index becc74f00..5551474ad 100644 --- a/pkg/crcontrollers/drive/drivecontroller_test.go +++ b/pkg/crcontrollers/drive/drivecontroller_test.go @@ -1408,39 +1408,27 @@ func TestCheckLVGVolumeWithoutFakeAttachRemoved(t *testing.T) { } } -func TestDriveManualRemovalNotReady(t *testing.T) { - tests := []struct { - name string - annotations map[string]string - want bool - }{ - { - name: "No annotations", - annotations: map[string]string{}, - want: true, - }, - { - name: "Annotation exists with different value", - annotations: map[string]string{ - apiV1.DriveAnnotationRemoval: "some_value", - }, - want: true, - }, - { - name: "Annotation exists with correct value", - annotations: map[string]string{ - apiV1.DriveAnnotationRemoval: apiV1.DriveAnnotationRemovalReady, - }, - want: false, - }, +func TestDriveEjectAuto(t *testing.T) { + c := &Controller{} + + // Test case when annotation is not found + annotations := make(map[string]string) + result := c.driveEjectAuto(annotations) + if !result { + t.Errorf("Expected true, but got false") } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := &Controller{} - if got := c.driveManualRemovalNotReady(tt.annotations); got != tt.want { - t.Errorf("driveManualRemovalNotReady() = %v, want %v", got, tt.want) - } - }) + // Test case when annotation is found and its value is not 'auto' + annotations[apiV1.DriveAnnotationEject] = "manual" + result = c.driveEjectAuto(annotations) + if !result { + t.Errorf("Expected true, but got false") + } + + // Test case when annotation is found and its value is 'auto' + annotations[apiV1.DriveAnnotationEject] = apiV1.DriveAnnotationEjectAuto + result = c.driveEjectAuto(annotations) + if result { + t.Errorf("Expected false, but got true") } } From f2329d3ae279e7c6788540e1a93f7f44150d6f1e Mon Sep 17 00:00:00 2001 From: Katarzyna Kulpa Date: Tue, 17 Dec 2024 09:48:33 +0100 Subject: [PATCH 5/6] [issue-1256] disk auto eject --- pkg/crcontrollers/drive/drivecontroller.go | 2 +- .../drive/drivecontroller_test.go | 42 +++++++++++++++---- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/pkg/crcontrollers/drive/drivecontroller.go b/pkg/crcontrollers/drive/drivecontroller.go index 22ed87376..60078c698 100644 --- a/pkg/crcontrollers/drive/drivecontroller.go +++ b/pkg/crcontrollers/drive/drivecontroller.go @@ -610,5 +610,5 @@ func (c *Controller) handleDriveLableUpdate(ctx context.Context, log *logrus.Ent func (c *Controller) driveEjectAuto(annotations map[string]string) bool { status, found := annotations[apiV1.DriveAnnotationEject] - return !found || status != apiV1.DriveAnnotationEjectAuto + return found && status == apiV1.DriveAnnotationEjectAuto } diff --git a/pkg/crcontrollers/drive/drivecontroller_test.go b/pkg/crcontrollers/drive/drivecontroller_test.go index 5551474ad..9986221f7 100644 --- a/pkg/crcontrollers/drive/drivecontroller_test.go +++ b/pkg/crcontrollers/drive/drivecontroller_test.go @@ -425,21 +425,45 @@ func TestDriveController_handleDriveUpdate(t *testing.T) { assert.Nil(t, dc.client.DeleteCR(k8s.ListFailCtx, expectedD)) assert.Nil(t, dc.client.DeleteCR(k8s.ListFailCtx, expectedV)) }) - t.Run("ReleasedUsage - transition to Removing state", func(t *testing.T) { + t.Run("ReleasedUsage - auto transition to Removing state", func(t *testing.T) { expectedD := testBadCRDrive.DeepCopy() assert.NotNil(t, expectedD) expectedD.Spec.Usage = apiV1.DriveUsageReleased expectedD.Spec.IsClean = true + expectedD.Annotations = map[string]string{"eject": "auto"} + assert.Nil(t, dc.client.CreateCR(testCtx, expectedD.Name, expectedD)) res, err := dc.handleDriveUpdate(testCtx, dc.log, expectedD) assert.NotNil(t, res) assert.Nil(t, err) assert.Equal(t, res, update) + assert.Equal(t, expectedD.Spec.Usage, apiV1.DriveUsageRemoving) + + assert.Nil(t, dc.client.DeleteCR(testCtx, expectedD)) + }) + t.Run("ReleasedUsage - manual transition to Removing state", func(t *testing.T) { + expectedD := testBadCRDrive.DeepCopy() + assert.NotNil(t, expectedD) + expectedD.Spec.Usage = apiV1.DriveUsageRemoving + expectedD.Spec.IsClean = true + + assert.Nil(t, dc.client.CreateCR(testCtx, expectedD.Name, expectedD)) + + expectedV := failedVolCR.DeepCopy() + assert.NotNil(t, expectedV) + expectedV.Spec.CSIStatus = apiV1.Created + assert.Nil(t, dc.client.CreateCR(testCtx, expectedV.Name, expectedV)) + + res, err := dc.handleDriveUpdate(testCtx, dc.log, expectedD) + assert.NotNil(t, res) + assert.Nil(t, err) + assert.Equal(t, expectedD.Spec.Usage, apiV1.DriveUsageRemoving) assert.Empty(t, expectedD.Annotations) assert.Nil(t, dc.client.DeleteCR(testCtx, expectedD)) + assert.Nil(t, dc.client.DeleteCR(testCtx, expectedV)) }) t.Run("ReleasedUsage - blocked transition to Removing state", func(t *testing.T) { expectedD := testBadCRDrive.DeepCopy() @@ -463,12 +487,12 @@ func TestDriveController_handleDriveUpdate(t *testing.T) { assert.Nil(t, dc.client.DeleteCR(testCtx, expectedD)) assert.Nil(t, dc.client.DeleteCR(testCtx, expectedV)) }) - t.Run("ReleasedUsage - has all-volumes-fake-attached annotation", func(t *testing.T) { + t.Run("ReleasedUsage - auto removal has all-volumes-fake-attached annotation", func(t *testing.T) { expectedD := testBadCRDrive.DeepCopy() assert.NotNil(t, expectedD) expectedD.Spec.Usage = apiV1.DriveUsageReleased expectedD.Spec.IsClean = false - expectedD.Annotations = map[string]string{allDRVolumesFakeAttachedAnnotation: allDRVolumesFakeAttachedKey} + expectedD.Annotations = map[string]string{allDRVolumesFakeAttachedAnnotation: allDRVolumesFakeAttachedKey, "eject": "auto"} assert.Nil(t, dc.client.CreateCR(testCtx, expectedD.Name, expectedD)) expectedV := failedVolCR.DeepCopy() @@ -1414,21 +1438,21 @@ func TestDriveEjectAuto(t *testing.T) { // Test case when annotation is not found annotations := make(map[string]string) result := c.driveEjectAuto(annotations) - if !result { - t.Errorf("Expected true, but got false") + if result { + t.Errorf("Expected false, but got true") } // Test case when annotation is found and its value is not 'auto' annotations[apiV1.DriveAnnotationEject] = "manual" result = c.driveEjectAuto(annotations) - if !result { - t.Errorf("Expected true, but got false") + if result { + t.Errorf("Expected false, but got true") } // Test case when annotation is found and its value is 'auto' annotations[apiV1.DriveAnnotationEject] = apiV1.DriveAnnotationEjectAuto result = c.driveEjectAuto(annotations) - if result { - t.Errorf("Expected false, but got true") + if !result { + t.Errorf("Expected true, but got false") } } From 8bc91eb7f2901e26052fd98580e1eb52054bb1cd Mon Sep 17 00:00:00 2001 From: Katarzyna Kulpa Date: Tue, 17 Dec 2024 13:40:18 +0100 Subject: [PATCH 6/6] [issue-1256] e2e tests update --- tests/e2e-test-framework/framework/const.py | 2 ++ .../tests/test_drive_replacement_multi_volumes.py | 7 +++++++ .../test_drive_replacement_multi_volumes_single_fail.py | 7 +++++++ .../tests/test_drive_replacement_one_volume.py | 7 +++++++ 4 files changed, 23 insertions(+) diff --git a/tests/e2e-test-framework/framework/const.py b/tests/e2e-test-framework/framework/const.py index 9dfcd3f7d..b770300a0 100644 --- a/tests/e2e-test-framework/framework/const.py +++ b/tests/e2e-test-framework/framework/const.py @@ -37,11 +37,13 @@ DRIVE_HEALTH_ANNOTATION = "health" VOLUME_RELEASE_ANNOTATION = "release" FAKE_ATTACH_PVC_ANNOTATION_KEY = "pv.attach.kubernetes.io/ignore-if-inaccessible" +DRIVE_EJECT = "eject" # annotation values DRIVE_HEALTH_BAD_ANNOTATION = "BAD" VOLUME_RELEASE_DONE_VALUE = "done" FAKE_ATTACH_PVC_ANNOTATION_VALUE = "yes" +DRIVE_EJECT_AUTO = "auto" # health HEALTH_GOOD = "GOOD" diff --git a/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes.py b/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes.py index aa2195e82..216667b26 100644 --- a/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes.py +++ b/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes.py @@ -56,6 +56,13 @@ def test_5921_auto_drive_replacement_with_multiple_volumes_per_pod(self): # 2. simulate drive failure. Annotate drive used by pod with health=BAD for drive in drives: drive_name = drive["metadata"]["name"] + # mark drive as auto eject + self.utils.annotate_custom_resource( + resource_name=drive_name, + resource_type="drives", + annotation_key=const.DRIVE_EJECT, + annotation_value=const.DRIVE_EJECT_AUTO, + ) self.utils.annotate_custom_resource( resource_name=drive_name, resource_type="drives", diff --git a/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes_single_fail.py b/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes_single_fail.py index 8574c10af..44cccc2ad 100644 --- a/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes_single_fail.py +++ b/tests/e2e-test-framework/tests/test_drive_replacement_multi_volumes_single_fail.py @@ -60,6 +60,13 @@ def test_5955_auto_drive_replacement_with_multiple_volumes_per_pod_single_failur failed_volume = volumes[0] # 2. simulate drive failure. Annotate drive used by pod with health=BAD failed_drive_name = failed_drive["metadata"]["name"] + # mark drive as auto eject + self.utils.annotate_custom_resource( + resource_name=failed_drive_name, + resource_type="drives", + annotation_key=const.DRIVE_EJECT, + annotation_value=const.DRIVE_EJECT_AUTO, + ) self.utils.annotate_custom_resource( resource_name=failed_drive_name, resource_type="drives", diff --git a/tests/e2e-test-framework/tests/test_drive_replacement_one_volume.py b/tests/e2e-test-framework/tests/test_drive_replacement_one_volume.py index 63ee48986..de7a8ca3b 100644 --- a/tests/e2e-test-framework/tests/test_drive_replacement_one_volume.py +++ b/tests/e2e-test-framework/tests/test_drive_replacement_one_volume.py @@ -55,6 +55,13 @@ def test_5771_auto_drive_replacement_with_one_volume_per_pod(self): # 2.1 simulate drive failure. Annotate drive used by pod with health=BAD drive_name = drive["metadata"]["name"] + # mark drive as auto eject + self.utils.annotate_custom_resource( + resource_name=drive_name, + resource_type="drives", + annotation_key=const.DRIVE_EJECT, + annotation_value=const.DRIVE_EJECT_AUTO, + ) self.utils.annotate_custom_resource( resource_name=drive_name, resource_type="drives",