diff --git a/internal/controller/defaults.go b/internal/controller/defaults.go new file mode 100644 index 0000000000..72d07a38b6 --- /dev/null +++ b/internal/controller/defaults.go @@ -0,0 +1,42 @@ +package controller + +import ( + "reflect" + + "github.com/vmware-tanzu/velero/pkg/util/kube" +) + +const unbounded = "0" + +// This module is for setting structure defaults where the default golang values for the type are not valid. +// int -> 0 +// float -> 0.0 +// string -> "" + +// PodResources with emptystring will trigger parsing errors in Velero. +// Replace empty string with unbounded so partial resource setting is accepted. +// +// Returns a new PodResources object with any empty string fields set to "0". +// If nil, returns the existing nil pointer. +func newPodResourcesWithUnboundedDefaults(pr *kube.PodResources) *kube.PodResources { + if pr == nil { + return pr + } + + prWithUnboundedDefaults := *pr + // Velero 1.18.1 adds ephemeralStorageRequest and ephemeralStorageLimits not in prior versions. + // Reflection handles new versions provided the new underlying fields are strings. + // Velero 1.18.1 and below are handled due to all fields are strings. + reflectedPr := reflect.ValueOf(pr).Elem() + reflectNewPr := reflect.ValueOf(&prWithUnboundedDefaults).Elem() + for i := range reflectedPr.NumField() { + oldField := reflectedPr.Field(i) + newField := reflectNewPr.Field(i) + if oldField.Kind() == reflect.String && oldField.String() == "" { + newField.SetString(unbounded) + } else { + newField.Set(oldField) + } + } + return &prWithUnboundedDefaults +} diff --git a/internal/controller/defaults_test.go b/internal/controller/defaults_test.go new file mode 100644 index 0000000000..9b1dd2b517 --- /dev/null +++ b/internal/controller/defaults_test.go @@ -0,0 +1,81 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/vmware-tanzu/velero/pkg/util/kube" +) + +func Test_newPodResourcesWithUnboundedDefaults(t *testing.T) { + tests := []struct { + name string + input *kube.PodResources + want *kube.PodResources + }{ + { + name: "nil returns nil", + input: nil, + want: nil, + }, + { + name: "partially set fields get unset values replaced with unbounded", + input: &kube.PodResources{ + CPURequest: "100m", + MemoryRequest: "128Mi", + }, + want: &kube.PodResources{ + CPURequest: "100m", + CPULimit: "0", + MemoryRequest: "128Mi", + MemoryLimit: "0", + EphemeralStorageRequest: "0", + EphemeralStorageLimit: "0", + }, + }, + { + name: "all fields set returns unchanged", + input: &kube.PodResources{ + CPURequest: "100m", + CPULimit: "200m", + MemoryRequest: "128Mi", + MemoryLimit: "256Mi", + EphemeralStorageRequest: "1Gi", + EphemeralStorageLimit: "2Gi", + }, + want: &kube.PodResources{ + CPURequest: "100m", + CPULimit: "200m", + MemoryRequest: "128Mi", + MemoryLimit: "256Mi", + EphemeralStorageRequest: "1Gi", + EphemeralStorageLimit: "2Gi", + }, + }, + { + name: "zero-value struct gets all fields set to unbounded", + input: &kube.PodResources{}, + want: &kube.PodResources{ + CPURequest: "0", + CPULimit: "0", + MemoryRequest: "0", + MemoryLimit: "0", + EphemeralStorageRequest: "0", + EphemeralStorageLimit: "0", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := newPodResourcesWithUnboundedDefaults(tt.input) + require.Equal(t, tt.want, got) + + // require the output object is not a mutation of the existing object + if tt.input != nil { + require.NotSame(t, tt.input, got) + } + }) + } +} diff --git a/internal/controller/nodeagent.go b/internal/controller/nodeagent.go index a8bc91f740..e4b382fc3a 100644 --- a/internal/controller/nodeagent.go +++ b/internal/controller/nodeagent.go @@ -182,6 +182,10 @@ func (r *DataProtectionApplicationReconciler) updateNodeAgentCM(cm *corev1.Confi } } + // If PodResources is set all fields must be filled for the Velero parser or it will be rejected. + // Fill unused fields with "0" as "" will cause parser rejection. + configWithPrivileged.PodResources = newPodResourcesWithUnboundedDefaults(configWithPrivileged.PodResources) + // Convert NodeAgentConfigMapSettings to a generic map configNodeAgentJSON, err := json.Marshal(configWithPrivileged) if err != nil { diff --git a/internal/controller/nodeagent_test.go b/internal/controller/nodeagent_test.go index 0865050f8f..51c10da73c 100644 --- a/internal/controller/nodeagent_test.go +++ b/internal/controller/nodeagent_test.go @@ -2047,7 +2047,9 @@ func TestDPAReconciler_updateNodeAgentCM(t *testing.T) { "cpuRequest": "100m", "memoryRequest": "100Mi", "cpuLimit": "200m", - "memoryLimit": "200Mi" + "memoryLimit": "200Mi", + "ephemeralStorageRequest": "0", + "ephemeralStorageLimit": "0" }, "restorePVC": { "ignoreDelayBinding": true @@ -2272,6 +2274,98 @@ func TestDPAReconciler_updateNodeAgentCM(t *testing.T) { }`, }), }, + { + name: "Given DPA CR instance with only memoryLimit, all other resource quantities should be '0' on output, for memory eviction support", + nodeAgentConfigMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.NodeAgentConfigMapPrefix + testCmName, + Namespace: testCmNs, + }, + }, + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCmName, + Namespace: testCmNs, + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{ + oadpv1alpha1.DefaultPluginAWS, + }, + }, + NodeAgent: &oadpv1alpha1.NodeAgentConfig{ + NodeAgentCommonFields: oadpv1alpha1.NodeAgentCommonFields{}, + NodeAgentConfigMapSettings: oadpv1alpha1.NodeAgentConfigMapSettings{ + PodResources: &kube.PodResources{ + MemoryLimit: "100Mi", + }, + }, + }, + }, + }, + }, + wantErr: false, + wantNodeAgentConfigMap: createTestBuiltNodeAgentCM(map[string]string{ + "node-agent-config": `{ + "podResources": { + "cpuRequest": "0", + "memoryRequest": "0", + "cpuLimit": "0", + "memoryLimit": "100Mi", + "ephemeralStorageRequest": "0", + "ephemeralStorageLimit": "0" + }, + "privilegedFsBackup": true + }`, + }), + }, + { + name: "Given DPA CR instance with only ephemeralStorageLimit, all other resource quantities should be '0' on output, for ephemeral-storage eviction support", + nodeAgentConfigMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.NodeAgentConfigMapPrefix + testCmName, + Namespace: testCmNs, + }, + }, + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCmName, + Namespace: testCmNs, + }, + Spec: oadpv1alpha1.DataProtectionApplicationSpec{ + Configuration: &oadpv1alpha1.ApplicationConfig{ + Velero: &oadpv1alpha1.VeleroConfig{ + DefaultPlugins: []oadpv1alpha1.DefaultPlugin{ + oadpv1alpha1.DefaultPluginAWS, + }, + }, + NodeAgent: &oadpv1alpha1.NodeAgentConfig{ + NodeAgentCommonFields: oadpv1alpha1.NodeAgentCommonFields{}, + NodeAgentConfigMapSettings: oadpv1alpha1.NodeAgentConfigMapSettings{ + PodResources: &kube.PodResources{ + EphemeralStorageLimit: "250Mi", + }, + }, + }, + }, + }, + }, + wantErr: false, + wantNodeAgentConfigMap: createTestBuiltNodeAgentCM(map[string]string{ + "node-agent-config": `{ + "podResources": { + "cpuRequest": "0", + "memoryRequest": "0", + "cpuLimit": "0", + "memoryLimit": "0", + "ephemeralStorageRequest": "0", + "ephemeralStorageLimit": "250Mi" + }, + "privilegedFsBackup": true + }`, + }), + }, } for _, tt := range tests { @@ -2280,8 +2374,12 @@ func TestDPAReconciler_updateNodeAgentCM(t *testing.T) { if err != nil { t.Fatalf("error in creating fake client, likely programmer error") } + var dpaSpecBeforeTest = oadpv1alpha1.DataProtectionApplicationSpec{} if tt.dpa != nil && tt.dpa.Spec.Configuration != nil { tt.dpa.AutoCorrect() + // Snapshot the DPA Spec before calling updateNodeAgentCM, + // Required to test the DPA is unchanged. + dpaSpecBeforeTest = *tt.dpa.Spec.DeepCopy() } r := &DataProtectionApplicationReconciler{ @@ -2318,6 +2416,13 @@ func TestDPAReconciler_updateNodeAgentCM(t *testing.T) { // Compare the unmarshalled maps require.Equal(t, wantMap, gotMap, "ConfigMaps are not equal") + // Require that updateNodeAgentCM did not mutate the DPA Spec. + // PodResource output will not match the original object if not all fields are set. + if tt.dpa != nil { + require.Truef(t, reflect.DeepEqual(tt.dpa.Spec, dpaSpecBeforeTest), + "updateNodeAgentCM must not modify the DPA Spec: diff=%s", + cmp.Diff(dpaSpecBeforeTest, tt.dpa.Spec)) + } }) } } diff --git a/internal/controller/repository_maintenance.go b/internal/controller/repository_maintenance.go index 273721d3f1..579f67098b 100644 --- a/internal/controller/repository_maintenance.go +++ b/internal/controller/repository_maintenance.go @@ -47,6 +47,8 @@ func (r *DataProtectionApplicationReconciler) updateRepositoryMaintenanceCM(cm * // to to match the upstream implementation // https://github.com/vmware-tanzu/velero/issues/9159 for key, config := range r.dpa.Spec.Configuration.RepositoryMaintenance { + // Velero parses a resource string of "" as invalid, replace with unbounded if unset + config.PodResources = newPodResourcesWithUnboundedDefaults(config.PodResources) configJSON, err := json.Marshal(config) if err != nil { return fmt.Errorf("failed to serialize repository maintenance config for key %s: %w", key, err) diff --git a/internal/controller/repository_maintenance_test.go b/internal/controller/repository_maintenance_test.go index 92875a44be..f22f86ee2e 100644 --- a/internal/controller/repository_maintenance_test.go +++ b/internal/controller/repository_maintenance_test.go @@ -3,9 +3,11 @@ package controller import ( "context" "encoding/json" + "reflect" "testing" "github.com/go-logr/logr" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" "github.com/vmware-tanzu/velero/pkg/util/kube" corev1 "k8s.io/api/core/v1" @@ -84,7 +86,7 @@ func TestDataProtectionApplicationReconciler_updateRepositoryMaintenanceCM(t *te }, }, Data: map[string]string{ - "global": `{"loadAffinity":[{"nodeSelector":{"matchLabels":{"app.kubernetes.io/name":"test-dpa"}}}],"podResources":{"cpuRequest":"100m","memoryRequest":"128Mi","cpuLimit":"200m","memoryLimit":"256Mi"}}`, + "global": `{"loadAffinity":[{"nodeSelector":{"matchLabels":{"app.kubernetes.io/name":"test-dpa"}}}],"podResources":{"cpuRequest":"100m","memoryRequest":"128Mi","cpuLimit":"200m","memoryLimit":"256Mi","ephemeralStorageRequest":"0","ephemeralStorageLimit":"0"}}`, "maintenance-job-1": `{"loadAffinity":[{"nodeSelector":{"matchLabels":{"app.kubernetes.io/name":"test-dpa"}}}]}`, }, }, @@ -192,7 +194,7 @@ func TestDataProtectionApplicationReconciler_updateRepositoryMaintenanceCM(t *te }, }, Data: map[string]string{ - "global": `{"podResources":{"cpuRequest":"100m","memoryRequest":"128Mi"},"podAnnotations":{"sidecar.istio.io/inject":"false"},"podLabels":{"network-access":"allowed"}}`, + "global": `{"podResources":{"cpuRequest":"100m","memoryRequest":"128Mi","cpuLimit":"0","memoryLimit":"0","ephemeralStorageRequest":"0","ephemeralStorageLimit":"0"},"podAnnotations":{"sidecar.istio.io/inject":"false"},"podLabels":{"network-access":"allowed"}}`, }, }, }, @@ -204,6 +206,14 @@ func TestDataProtectionApplicationReconciler_updateRepositoryMaintenanceCM(t *te if err != nil { t.Errorf("error in creating fake client, likely programmer error") } + + var dpaSpecBeforeTest = oadpv1alpha1.DataProtectionApplicationSpec{} + if tt.dpa != nil { + // Snapshot the DPA Spec before calling updateRepositoryMaintenanceCM, + // Required to test the DPA is unchanged. + dpaSpecBeforeTest = *tt.dpa.Spec.DeepCopy() + } + r := &DataProtectionApplicationReconciler{ Client: fakeClient, Scheme: fakeClient.Scheme(), @@ -237,6 +247,14 @@ func TestDataProtectionApplicationReconciler_updateRepositoryMaintenanceCM(t *te require.NoError(t, json.Unmarshal([]byte(actualData), &actualMap), "Failed to unmarshal actual Data for key %s", key) require.Equal(t, expectedMap, actualMap, "ConfigMap Data does not match for key %s", key) } + + // Require that updateRepositoryMaintenanceCM did not mutate the DPA Spec. + // PodResource output will not match the original object if not all fields are set. + if tt.dpa != nil { + require.Truef(t, reflect.DeepEqual(tt.dpa.Spec, dpaSpecBeforeTest), + "updateRepositoryMaintenanceCM must not modify the DPA Spec: diff=%s", + cmp.Diff(dpaSpecBeforeTest, tt.dpa.Spec)) + } }) } }