OADP-8548: PodResources unset fields should be output as "0" to fix parsing errors results in ignored resource settings

internal/controller/defaults.go

@@ -0,0 +1,57 @@
+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.
+//
+// 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 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
+	}
+	if pr.EphemeralStorageRequest == "" {
+		prWithUnboundedDefaults.EphemeralStorageRequest = unbounded
+	} else {
+		prWithUnboundedDefaults.EphemeralStorageRequest = pr.EphemeralStorageRequest
+	}
+	if pr.EphemeralStorageLimit == "" {
+		prWithUnboundedDefaults.EphemeralStorageLimit = unbounded
+	} else {
+		prWithUnboundedDefaults.EphemeralStorageLimit = pr.EphemeralStorageLimit
+	}
+
+	return prWithUnboundedDefaults
+}

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)
+			}
+		})
+	}
+}

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 {

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
@@ -2280,8 +2282,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 +2324,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))
+			}
 		})
 	}
 }

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)

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))
+			}
 		})
 	}
 }