From 11c1cafdb395950476b9c4b57cd866a47ad97df9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Du=C5=A1an?= Date: Mon, 10 Feb 2025 11:36:19 +0100 Subject: [PATCH] fix(GcpNfsVolume): no longer tries to edit capacity in spec Value in .Spec.Resources.Requests[storage] is no longer edited --- .../modifyPersistentVolumeClaim.go | 9 +--- .../modifyPersistentVolumeClaim_test.go | 53 ++++++++++++++----- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim.go b/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim.go index 122ee9566..13b02cc38 100644 --- a/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim.go +++ b/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim.go @@ -19,7 +19,6 @@ func modifyPersistentVolumeClaim(ctx context.Context, st composed.State) (error, } nfsVolume := state.ObjAsGcpNfsVolume() - capacity := gcpNfsVolumeCapacityToResourceQuantity(nfsVolume) if !meta.IsStatusConditionTrue(nfsVolume.Status.Conditions, v1beta1.ConditionTypeReady) { return nil, nil @@ -29,12 +28,6 @@ func modifyPersistentVolumeClaim(ctx context.Context, st composed.State) (error, return nil, nil } - capacityChanged := !capacity.Equal(state.PVC.Spec.Resources.Requests["storage"]) - if capacityChanged { - state.PVC.Spec.Resources.Requests["storage"] = *capacity - logger.Info("Detected modified PVC capacity") - } - expectedLabels := getVolumeClaimLabels(nfsVolume) labelsChanged := !areLabelsEqual(state.PVC.Labels, expectedLabels) if labelsChanged { @@ -49,7 +42,7 @@ func modifyPersistentVolumeClaim(ctx context.Context, st composed.State) (error, logger.Info("Detected desynced PVC annotations") } - if !(capacityChanged || labelsChanged || annotationsDesynced) { + if !(labelsChanged || annotationsDesynced) { return nil, nil } diff --git a/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim_test.go b/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim_test.go index 8497fe925..35ac56a9a 100644 --- a/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim_test.go +++ b/pkg/skr/gcpnfsvolume/modifyPersistentVolumeClaim_test.go @@ -5,14 +5,12 @@ import ( "testing" "time" - "github.com/kyma-project/cloud-manager/api/cloud-resources/v1beta1" cloudresourcesv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-resources/v1beta1" composed "github.com/kyma-project/cloud-manager/pkg/composed" spy "github.com/kyma-project/cloud-manager/pkg/testinfra/clientspy" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -26,6 +24,7 @@ func TestModifyPersistentVolumeClaim(t *testing.T) { t.Run("modifyPersistentVolumeClaim", func(t *testing.T) { var gcpNfsVolume *cloudresourcesv1beta1.GcpNfsVolume + var actualPVC *corev1.PersistentVolumeClaim var state *State var k8sClient client.WithWatch @@ -38,7 +37,7 @@ func TestModifyPersistentVolumeClaim(t *testing.T) { setupTest := func() { gcpNfsVolume = &cloudresourcesv1beta1.GcpNfsVolume{ - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Name: "test-gcpnfsvol", Namespace: "test-ns", }, @@ -54,19 +53,19 @@ func TestModifyPersistentVolumeClaim(t *testing.T) { }, }, Status: cloudresourcesv1beta1.GcpNfsVolumeStatus{ - Conditions: []v1.Condition{ + Conditions: []metav1.Condition{ { - Type: v1beta1.ConditionTypeReady, + Type: cloudresourcesv1beta1.ConditionTypeReady, Status: metav1.ConditionTrue, - Reason: v1beta1.ConditionReasonReady, + Reason: cloudresourcesv1beta1.ConditionReasonReady, Message: "Volume is ready", }, }, }, } - actualPVC := &corev1.PersistentVolumeClaim{ - ObjectMeta: v1.ObjectMeta{ + actualPVC = &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ Name: "test-gcpnfsvol", Namespace: "test-ns", Labels: getVolumeClaimLabels(gcpNfsVolume), @@ -96,7 +95,7 @@ func TestModifyPersistentVolumeClaim(t *testing.T) { state.PVC = actualPVC } - t.Run("Should: modify PVC when actual state diverges from desired state", func(t *testing.T) { + t.Run("Should: modify PVC when labels change", func(t *testing.T) { setupTest() ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -105,26 +104,56 @@ func TestModifyPersistentVolumeClaim(t *testing.T) { "foo": "bar-modified", "oof": "rab", }, + } + + err, res := modifyPersistentVolumeClaim(ctx, state) + + assert.NotNil(t, err, "should return not-nil err") // not an actual error, but StopWithRequeueDelay + assert.Nil(t, res, "should return nil res") + assert.EqualValues(t, 1, k8sClient.(spy.ClientSpy).UpdateCallCount(), "update should be called") + }) + + t.Run("Should: modify PVC when annotation change", func(t *testing.T) { + setupTest() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + gcpNfsVolume.Spec.PersistentVolumeClaim = &cloudresourcesv1beta1.GcpNfsVolumePvcSpec{ Annotations: map[string]string{ "baz": "qux-modified", "zab": "xuq", }, } + + err, res := modifyPersistentVolumeClaim(ctx, state) + + assert.NotNil(t, err, "should return not-nil err") // not an actual error, but StopWithRequeueDelay + assert.Nil(t, res, "should return nil res") + assert.EqualValues(t, 1, k8sClient.(spy.ClientSpy).UpdateCallCount(), "update should be called") + }) + + t.Run("Should: modify only PVC label when capacity changes", func(t *testing.T) { + setupTest() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + originalCapacity := actualPVC.Spec.Resources.Requests["storage"].DeepCopy() gcpNfsVolume.Spec.CapacityGb = 2000 err, res := modifyPersistentVolumeClaim(ctx, state) + postModifyCapacity := actualPVC.Spec.Resources.Requests["storage"].DeepCopy() assert.NotNil(t, err, "should return not-nil err") // not an actual error, but StopWithRequeueDelay assert.Nil(t, res, "should return nil res") assert.EqualValues(t, 1, k8sClient.(spy.ClientSpy).UpdateCallCount(), "update should be called") + assert.True(t, originalCapacity.Equal(postModifyCapacity), "capacity should not change") + assert.Equal(t, "2000Gi", actualPVC.Labels[cloudresourcesv1beta1.LabelStorageCapacity], "label value should be adjusted to match desired value") }) t.Run("Should: do nothing if GcpNfsVolume is marked for deletion", func(t *testing.T) { setupTest() ctx, cancel := context.WithCancel(context.Background()) defer cancel() - gcpNfsVolume.ObjectMeta = v1.ObjectMeta{ - DeletionTimestamp: &v1.Time{ + gcpNfsVolume.ObjectMeta = metav1.ObjectMeta{ + DeletionTimestamp: &metav1.Time{ Time: time.Now(), }, } @@ -141,7 +170,7 @@ func TestModifyPersistentVolumeClaim(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() state.PVC = nil - gcpNfsVolume.Status.Conditions = []v1.Condition{} + gcpNfsVolume.Status.Conditions = []metav1.Condition{} err, res := modifyPersistentVolumeClaim(ctx, state)