From 85a8dc69b487279787dd5a11139568e8d2ec80b1 Mon Sep 17 00:00:00 2001 From: MICHAEL FRUCHTMAN Date: Wed, 12 Aug 2026 15:30:18 -0700 Subject: [PATCH 1/4] PodResources must be complete for Velero parser Signed-off-by: MICHAEL FRUCHTMAN --- internal/controller/defaults.go | 39 +++++++++++++++++++ internal/controller/nodeagent.go | 6 +++ internal/controller/nodeagent_test.go | 4 +- internal/controller/repository_maintenance.go | 4 ++ .../controller/repository_maintenance_test.go | 4 +- 5 files changed, 54 insertions(+), 3 deletions(-) create mode 100644 internal/controller/defaults.go diff --git a/internal/controller/defaults.go b/internal/controller/defaults.go new file mode 100644 index 00000000000..8e84b2ccca7 --- /dev/null +++ b/internal/controller/defaults.go @@ -0,0 +1,39 @@ +package controller + +import ( + "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. +func setPodResourcesDefaults(pr *kube.PodResources) { + if pr == nil { + return + } + + if pr.CPURequest == "" { + pr.CPURequest = unbounded + } + if pr.CPULimit == "" { + pr.CPULimit = unbounded + } + if pr.MemoryRequest == "" { + pr.MemoryRequest = unbounded + } + if pr.MemoryLimit == "" { + pr.MemoryLimit = unbounded + } + if pr.EphemeralStorageRequest == "" { + pr.EphemeralStorageRequest = unbounded + } + if pr.EphemeralStorageLimit == "" { + pr.EphemeralStorageLimit = unbounded + } +} diff --git a/internal/controller/nodeagent.go b/internal/controller/nodeagent.go index a8bc91f7402..6d0d3cd65a1 100644 --- a/internal/controller/nodeagent.go +++ b/internal/controller/nodeagent.go @@ -182,6 +182,12 @@ 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. + if configWithPrivileged.PodResources != nil { + setPodResourcesDefaults(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 baa58cb4c70..7ce5e994d14 100644 --- a/internal/controller/nodeagent_test.go +++ b/internal/controller/nodeagent_test.go @@ -2036,7 +2036,9 @@ func TestDPAReconciler_updateNodeAgentCM(t *testing.T) { "cpuRequest": "100m", "memoryRequest": "100Mi", "cpuLimit": "200m", - "memoryLimit": "200Mi" + "memoryLimit": "200Mi", + "ephemeralStorageRequest": "0", + "ephemeralStorageLimit": "0" }, "restorePVC": { "ignoreDelayBinding": true diff --git a/internal/controller/repository_maintenance.go b/internal/controller/repository_maintenance.go index 273721d3f1b..d779d0e01f2 100644 --- a/internal/controller/repository_maintenance.go +++ b/internal/controller/repository_maintenance.go @@ -47,6 +47,10 @@ 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 + if config.PodResources != nil { + setPodResourcesDefaults(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 92875a44be2..953e540458d 100644 --- a/internal/controller/repository_maintenance_test.go +++ b/internal/controller/repository_maintenance_test.go @@ -84,7 +84,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 +192,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"}}`, }, }, }, From f4108cf7f9cbf94cd6d081aa46913eaae85ca708 Mon Sep 17 00:00:00 2001 From: MICHAEL FRUCHTMAN Date: Thu, 13 Aug 2026 11:38:02 -0700 Subject: [PATCH 2/4] Revise to prevent DPA Spec mutation Return a new PodResources Add validation that DPA Spec is unchanged by the functions handling the Reconcile of NodeAgentConfig and MaintenanceConfig. --- internal/controller/defaults.go | 34 ++++++++++++++----- internal/controller/nodeagent.go | 4 +-- internal/controller/nodeagent_test.go | 11 ++++++ internal/controller/repository_maintenance.go | 4 +-- .../controller/repository_maintenance_test.go | 18 ++++++++++ 5 files changed, 57 insertions(+), 14 deletions(-) diff --git a/internal/controller/defaults.go b/internal/controller/defaults.go index 8e84b2ccca7..96e84a8948f 100644 --- a/internal/controller/defaults.go +++ b/internal/controller/defaults.go @@ -13,27 +13,45 @@ const unbounded = "0" // PodResources with emptystring will trigger parsing errors in Velero. // Replace empty string with unbounded so partial resource setting is accepted. -func setPodResourcesDefaults(pr *kube.PodResources) { +// +// Returns a new PodResources objects with any default values set to "0". +// If nil, returns the existing nil pointer. +func newPodResourcesWithUnboundedDefaults(pr *kube.PodResources) *kube.PodResources { if pr == nil { - return + return pr } + prWithUnboundedDefaults := &kube.PodResources{} if pr.CPURequest == "" { - pr.CPURequest = unbounded + prWithUnboundedDefaults.CPURequest = unbounded + } else { + prWithUnboundedDefaults.CPURequest = pr.CPURequest } if pr.CPULimit == "" { - pr.CPULimit = unbounded + prWithUnboundedDefaults.CPULimit = unbounded + } else { + prWithUnboundedDefaults.CPULimit = pr.CPULimit } if pr.MemoryRequest == "" { - pr.MemoryRequest = unbounded + prWithUnboundedDefaults.MemoryRequest = unbounded + } else { + prWithUnboundedDefaults.MemoryRequest = pr.MemoryRequest } if pr.MemoryLimit == "" { - pr.MemoryLimit = unbounded + prWithUnboundedDefaults.MemoryLimit = unbounded + } else { + prWithUnboundedDefaults.MemoryLimit = pr.MemoryLimit } if pr.EphemeralStorageRequest == "" { - pr.EphemeralStorageRequest = unbounded + prWithUnboundedDefaults.EphemeralStorageRequest = unbounded + } else { + prWithUnboundedDefaults.EphemeralStorageRequest = pr.EphemeralStorageRequest } if pr.EphemeralStorageLimit == "" { - pr.EphemeralStorageLimit = unbounded + prWithUnboundedDefaults.EphemeralStorageLimit = unbounded + } else { + prWithUnboundedDefaults.EphemeralStorageLimit = pr.EphemeralStorageLimit } + + return prWithUnboundedDefaults } diff --git a/internal/controller/nodeagent.go b/internal/controller/nodeagent.go index 6d0d3cd65a1..e4b382fc3a5 100644 --- a/internal/controller/nodeagent.go +++ b/internal/controller/nodeagent.go @@ -184,9 +184,7 @@ 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. - if configWithPrivileged.PodResources != nil { - setPodResourcesDefaults(configWithPrivileged.PodResources) - } + configWithPrivileged.PodResources = newPodResourcesWithUnboundedDefaults(configWithPrivileged.PodResources) // Convert NodeAgentConfigMapSettings to a generic map configNodeAgentJSON, err := json.Marshal(configWithPrivileged) diff --git a/internal/controller/nodeagent_test.go b/internal/controller/nodeagent_test.go index 7ce5e994d14..40b7a1f64bf 100644 --- a/internal/controller/nodeagent_test.go +++ b/internal/controller/nodeagent_test.go @@ -2271,8 +2271,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{ @@ -2309,6 +2313,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 d779d0e01f2..579f67098bd 100644 --- a/internal/controller/repository_maintenance.go +++ b/internal/controller/repository_maintenance.go @@ -48,9 +48,7 @@ func (r *DataProtectionApplicationReconciler) updateRepositoryMaintenanceCM(cm * // 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 - if config.PodResources != nil { - setPodResourcesDefaults(config.PodResources) - } + 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 953e540458d..f22f86ee2eb 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" @@ -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)) + } }) } } From d29e2c57bc3cd8af15453b3352a20713aea4516a Mon Sep 17 00:00:00 2001 From: MICHAEL FRUCHTMAN Date: Thu, 13 Aug 2026 12:14:58 -0700 Subject: [PATCH 3/4] Add test of defaults due to more complex logic returning new object --- internal/controller/defaults_test.go | 81 ++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 internal/controller/defaults_test.go diff --git a/internal/controller/defaults_test.go b/internal/controller/defaults_test.go new file mode 100644 index 00000000000..9b1dd2b517b --- /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) + } + }) + } +} From 69167fb21931f983639185ec4b61639a4a6f24cc Mon Sep 17 00:00:00 2001 From: Michael Fruchtman Date: Mon, 17 Aug 2026 09:42:45 -0700 Subject: [PATCH 4/4] revise to handle older and newer versions of PodResources with strings, unit test changes requested Signed-off-by: Michael Fruchtman --- internal/controller/defaults.go | 51 ++++++--------- internal/controller/nodeagent_test.go | 92 +++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 33 deletions(-) diff --git a/internal/controller/defaults.go b/internal/controller/defaults.go index 96e84a8948f..72d07a38b6e 100644 --- a/internal/controller/defaults.go +++ b/internal/controller/defaults.go @@ -1,6 +1,8 @@ package controller import ( + "reflect" + "github.com/vmware-tanzu/velero/pkg/util/kube" ) @@ -14,44 +16,27 @@ const unbounded = "0" // PodResources with emptystring will trigger parsing errors in Velero. // Replace empty string with unbounded so partial resource setting is accepted. // -// Returns a new PodResources objects with any default values set to "0". +// 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 := &kube.PodResources{} - if pr.CPURequest == "" { - prWithUnboundedDefaults.CPURequest = unbounded - } else { - prWithUnboundedDefaults.CPURequest = pr.CPURequest - } - if pr.CPULimit == "" { - prWithUnboundedDefaults.CPULimit = unbounded - } else { - prWithUnboundedDefaults.CPULimit = pr.CPULimit - } - if pr.MemoryRequest == "" { - prWithUnboundedDefaults.MemoryRequest = unbounded - } else { - prWithUnboundedDefaults.MemoryRequest = pr.MemoryRequest - } - if pr.MemoryLimit == "" { - prWithUnboundedDefaults.MemoryLimit = unbounded - } else { - prWithUnboundedDefaults.MemoryLimit = pr.MemoryLimit + 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) + } } - if pr.EphemeralStorageRequest == "" { - prWithUnboundedDefaults.EphemeralStorageRequest = unbounded - } else { - prWithUnboundedDefaults.EphemeralStorageRequest = pr.EphemeralStorageRequest - } - if pr.EphemeralStorageLimit == "" { - prWithUnboundedDefaults.EphemeralStorageLimit = unbounded - } else { - prWithUnboundedDefaults.EphemeralStorageLimit = pr.EphemeralStorageLimit - } - - return prWithUnboundedDefaults + return &prWithUnboundedDefaults } diff --git a/internal/controller/nodeagent_test.go b/internal/controller/nodeagent_test.go index 02ae88ebc9d..51c10da73c5 100644 --- a/internal/controller/nodeagent_test.go +++ b/internal/controller/nodeagent_test.go @@ -2274,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 {