diff --git a/e2e/nomostest/testpredicates/predicates.go b/e2e/nomostest/testpredicates/predicates.go index 3f0ecf704b..6756cf1b5f 100644 --- a/e2e/nomostest/testpredicates/predicates.go +++ b/e2e/nomostest/testpredicates/predicates.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/pkg/errors" + "go.uber.org/multierr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -30,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "kpt.dev/configsync/e2e/nomostest/retry" "kpt.dev/configsync/e2e/nomostest/testkubeclient" + "kpt.dev/configsync/e2e/nomostest/testlogger" "kpt.dev/configsync/e2e/nomostest/testutils" "kpt.dev/configsync/pkg/api/configsync/v1beta1" "kpt.dev/configsync/pkg/core" @@ -277,7 +279,7 @@ func DeploymentContainerResourcesEqual(expectedSpec v1beta1.ContainerResourcesSp // DeploymentContainerResourcesAllEqual verifies all reconciler deployment // containers have the expected resource requests and limits. -func DeploymentContainerResourcesAllEqual(scheme *runtime.Scheme, expectedByName map[string]v1beta1.ContainerResourcesSpec) Predicate { +func DeploymentContainerResourcesAllEqual(scheme *runtime.Scheme, logger *testlogger.TestLogger, expectedByName map[string]v1beta1.ContainerResourcesSpec) Predicate { return func(o client.Object) error { if o == nil { return ErrObjectNotFound @@ -289,6 +291,7 @@ func DeploymentContainerResourcesAllEqual(scheme *runtime.Scheme, expectedByName } // Validate the container resources exactly match expectations var foundContainers []string + var errs []error for _, container := range d.Spec.Template.Spec.Containers { foundContainers = append(foundContainers, container.Name) expectedSpec, ok := expectedByName[container.Name] @@ -296,7 +299,8 @@ func DeploymentContainerResourcesAllEqual(scheme *runtime.Scheme, expectedByName continue // error later when the list doesn't match } if err := validateContainerResources(&container, expectedSpec); err != nil { - return err + errs = append(errs, err) + continue } } // Validate the containers names exactly match expectations @@ -307,14 +311,42 @@ func DeploymentContainerResourcesAllEqual(scheme *runtime.Scheme, expectedByName sort.Strings(expectedContainers) sort.Strings(foundContainers) if !cmp.Equal(expectedContainers, foundContainers) { - return fmt.Errorf("expected containers [%s], but found [%s]", + err := fmt.Errorf("expected containers [%s], but found [%s]", strings.Join(expectedContainers, ","), strings.Join(foundContainers, ",")) + errs = append(errs, err) + } + // Combine all errors and log some helpful debug info + if len(errs) > 0 { + logger.Info("Container resources expected:") + for containerName, containerSpec := range expectedByName { + logger.Infof("%s: %s", containerName, log.AsJSON(containerResourceSpecToRequirements(containerSpec))) + } + logger.Info("Container resources found:") + for _, container := range d.Spec.Template.Spec.Containers { + logger.Infof("%s: %s", container.Name, log.AsJSON(container.Resources)) + } + return multierr.Combine(errs...) } return nil } } +// containerResourceSpecToRequirements converts ContainerResourcesSpec to +// ResourceRequirements +func containerResourceSpecToRequirements(spec v1beta1.ContainerResourcesSpec) corev1.ResourceRequirements { + return corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: spec.CPURequest, + corev1.ResourceMemory: spec.MemoryRequest, + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: spec.CPULimit, + corev1.ResourceMemory: spec.MemoryLimit, + }, + } +} + func convertToDeployment(obj client.Object, scheme *runtime.Scheme) (*appsv1.Deployment, error) { if uObj, ok := obj.(*unstructured.Unstructured); ok { rObj, err := kinds.ToTypedObject(uObj, scheme) diff --git a/e2e/testcases/reconciler_manager_test.go b/e2e/testcases/reconciler_manager_test.go index 109271e4c7..c721d7ac38 100644 --- a/e2e/testcases/reconciler_manager_test.go +++ b/e2e/testcases/reconciler_manager_test.go @@ -668,42 +668,6 @@ func templateForSSHAuthType() testpredicates.Predicate { } } -func totalContainerMemoryRequestEquals(expected resource.Quantity) testpredicates.Predicate { - return func(o client.Object) error { - if o == nil { - return testpredicates.ErrObjectNotFound - } - d, ok := o.(*appsv1.Deployment) - if !ok { - return testpredicates.WrongTypeErr(d, &appsv1.Deployment{}) - } - total := totalContainerMemoryRequests(d.Spec.Template.Spec.Containers) - if total.Cmp(expected) != 0 { - return fmt.Errorf("expected total Memory request of all containers: %d, got: %d", - expected.Value(), total.Value()) - } - return nil - } -} - -func totalContainerCPURequestEquals(expected resource.Quantity) testpredicates.Predicate { - return func(o client.Object) error { - if o == nil { - return testpredicates.ErrObjectNotFound - } - d, ok := o.(*appsv1.Deployment) - if !ok { - return testpredicates.WrongTypeErr(d, &appsv1.Deployment{}) - } - total := totalContainerCPURequests(d.Spec.Template.Spec.Containers) - if total.Cmp(expected) != 0 { - return fmt.Errorf("expected total CPU request of all containers: %d, got: %d", - expected.MilliValue(), total.MilliValue()) - } - return nil - } -} - func firstContainerNameEquals(expected string) testpredicates.Predicate { return func(o client.Object) error { if o == nil { @@ -722,20 +686,20 @@ func firstContainerNameEquals(expected string) testpredicates.Predicate { } } -func totalContainerCPURequests(containers []corev1.Container) resource.Quantity { - total := resource.MustParse("0m") - for _, container := range containers { - total.Add(*container.Resources.Requests.Cpu()) +func totalExpectedContainerResources(resourceMap map[string]v1beta1.ContainerResourcesSpec) v1beta1.ContainerResourcesSpec { + totals := v1beta1.ContainerResourcesSpec{ + CPURequest: resource.MustParse("0m"), + CPULimit: resource.MustParse("0m"), + MemoryRequest: resource.MustParse("0m"), + MemoryLimit: resource.MustParse("0m"), } - return total -} - -func totalContainerMemoryRequests(containers []corev1.Container) resource.Quantity { - total := resource.MustParse("0Mi") - for _, container := range containers { - total.Add(*container.Resources.Requests.Memory()) + for _, container := range resourceMap { + totals.CPURequest.Add(container.CPURequest) + totals.CPULimit.Add(container.CPULimit) + totals.MemoryRequest.Add(container.MemoryRequest) + totals.MemoryLimit.Add(container.MemoryLimit) } - return total + return totals } func TestAutopilotReconcilerAdjustment(t *testing.T) { @@ -772,22 +736,19 @@ func TestAutopilotReconcilerAdjustment(t *testing.T) { reconcilermanager.Reconciler, reconcilermanager.GitSync, metrics.OtelAgentName) + nt.T.Logf("expectedResources: %s", log.AsJSON(expectedResources)) if _, found := expectedResources[firstContainerName]; !found { nt.T.Fatalf("expected the default resource map to include %q, but it was missing: %+v", firstContainerName, expectedResources) } - // Compute expected totals from initial values. - // This ensures the totals only account for the containers present, - // even if expectedResources includes additional containers. - expectedTotalResources := v1beta1.ContainerResourcesSpec{ - CPURequest: totalContainerCPURequests(reconcilerDeployment.Spec.Template.Spec.Containers), - MemoryRequest: totalContainerMemoryRequests(reconcilerDeployment.Spec.Template.Spec.Containers), - } - - // Autopilot increases the CPU of the first container, - // if the total CPU is less than 250m. if nt.IsGKEAutopilot { + // Compute expected totals + expectedTotalResources := totalExpectedContainerResources(expectedResources) + nt.T.Logf("expectedTotalResources: %s", log.AsJSON(expectedTotalResources)) + + // Autopilot increases the CPU of the first container, + // if the total CPU is less than 250m. minimumTotalCPURequests := resource.MustParse("250m") if expectedTotalResources.CPURequest.Cmp(minimumTotalCPURequests) < 0 { // Compute difference @@ -799,29 +760,38 @@ func TestAutopilotReconcilerAdjustment(t *testing.T) { updated := expectedResources[firstContainerName] updated.CPURequest.Add(diff) expectedResources[firstContainerName] = updated - // Update total - expectedTotalResources.CPURequest = minimumTotalCPURequests } + + // Autopilot increases the Memory of the first container, + // if the total CPU is less than 512Mi. + minimumTotalMemoryRequests := resource.MustParse("512Mi") + if expectedTotalResources.MemoryRequest.Cmp(minimumTotalMemoryRequests) < 0 { + // Compute difference + diff := minimumTotalMemoryRequests.DeepCopy() + diff.Sub(expectedTotalResources.MemoryRequest) + // Add difference to first container + // Go doesn't allow modifying a struct field in a map directly, + // so read, update, and write it back. + updated := expectedResources[firstContainerName] + updated.MemoryRequest.Add(diff) + expectedResources[firstContainerName] = updated + } + + // Autopilot sets Limits to Requests + setLimitsToRequests(expectedResources) + nt.T.Logf("expectedResources (adjusted for autopilot): %s", log.AsJSON(expectedResources)) + expectedTotalResources = totalExpectedContainerResources(expectedResources) + nt.T.Logf("expectedTotalResources (adjusted for autopilot): %s", log.AsJSON(expectedTotalResources)) } nt.T.Log("Validating container resources - 1") reconcilerDeployment = &appsv1.Deployment{} err = nt.Validate(reconcilerNN.Name, reconcilerNN.Namespace, reconcilerDeployment, testpredicates.HasGenerationAtLeast(generation), - totalContainerCPURequestEquals(expectedTotalResources.CPURequest), - totalContainerMemoryRequestEquals(expectedTotalResources.MemoryRequest), + testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, nt.Logger, expectedResources), firstContainerNameEquals(firstContainerName), - testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, expectedResources), ) if err != nil { - nt.T.Log("Reconciler container specs (expected):") - for containerName, containerSpec := range expectedResources { - nt.T.Logf("%s: %s", containerName, log.AsJSON(containerSpec)) - } - nt.T.Log("Reconciler container specs (found):") - for _, container := range reconcilerDeployment.Spec.Template.Spec.Containers { - nt.T.Logf("%s: %s", container.Name, log.AsJSON(container.Resources)) - } nt.T.Fatal(err) } generation = reconcilerDeployment.GetGeneration() @@ -842,14 +812,13 @@ func TestAutopilotReconcilerAdjustment(t *testing.T) { updated.CPURequest.Sub(resource.MustParse("10m")) updated.CPURequest.Add(resource.MustParse("250m")) expectedResources[firstContainerName] = updated - // Increase the expected total resources - expectedTotalResources.CPURequest.Add(resource.MustParse("250m")) - } else { - // Increase the expected total resources - expectedTotalResources.CPURequest.Add(resource.MustParse("10m")) + + // Autopilot sets Limits to Requests + setLimitsToRequests(expectedResources) + nt.T.Logf("expectedResources (adjusted for autopilot): %s", log.AsJSON(expectedResources)) + expectedTotalResources := totalExpectedContainerResources(expectedResources) + nt.T.Logf("expectedTotalResources (adjusted for autopilot): %s", log.AsJSON(expectedTotalResources)) } - // Autopilot doesn't adjust memory - expectedTotalResources.MemoryRequest.Add(resource.MustParse("10Mi")) // Wait for overrides to be applied // Note: This depends on the Syncing condition reflecting the current RSync generation. @@ -870,20 +839,10 @@ func TestAutopilotReconcilerAdjustment(t *testing.T) { reconcilerDeployment = &appsv1.Deployment{} err = nt.Validate(reconcilerNN.Name, reconcilerNN.Namespace, reconcilerDeployment, testpredicates.HasGenerationAtLeast(generation), - totalContainerCPURequestEquals(expectedTotalResources.CPURequest), - totalContainerMemoryRequestEquals(expectedTotalResources.MemoryRequest), + testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, nt.Logger, expectedResources), firstContainerNameEquals(firstContainerName), - testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, expectedResources), ) if err != nil { - nt.T.Log("Reconciler container specs (expected):") - for containerName, containerSpec := range expectedResources { - nt.T.Logf("%s: %s", containerName, log.AsJSON(containerSpec)) - } - nt.T.Log("Reconciler container specs (found):") - for _, container := range reconcilerDeployment.Spec.Template.Spec.Containers { - nt.T.Logf("%s: %s", container.Name, log.AsJSON(container.Resources)) - } nt.T.Fatal(err) } generation = reconcilerDeployment.GetGeneration() @@ -915,20 +874,10 @@ func TestAutopilotReconcilerAdjustment(t *testing.T) { reconcilerDeployment = &appsv1.Deployment{} err = nt.Validate(reconcilerNN.Name, reconcilerNN.Namespace, reconcilerDeployment, testpredicates.HasGenerationAtLeast(generation), - totalContainerCPURequestEquals(expectedTotalResources.CPURequest), - totalContainerMemoryRequestEquals(expectedTotalResources.MemoryRequest), + testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, nt.Logger, expectedResources), firstContainerNameEquals(firstContainerName), - testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, expectedResources), ) if err != nil { - nt.T.Log("Reconciler container specs (expected):") - for containerName, containerSpec := range expectedResources { - nt.T.Logf("%s: %s", containerName, log.AsJSON(containerSpec)) - } - nt.T.Log("Reconciler container specs (found):") - for _, container := range reconcilerDeployment.Spec.Template.Spec.Containers { - nt.T.Logf("%s: %s", container.Name, log.AsJSON(container.Resources)) - } nt.T.Fatal(err) } generation = reconcilerDeployment.GetGeneration() @@ -942,16 +891,20 @@ func TestAutopilotReconcilerAdjustment(t *testing.T) { expected := updated // copy expected.CPURequest.Add(resource.MustParse("10m")) expectedResources[firstContainerName] = expected - // No update to total resources // Expect reconciler-manager NOT to update the reconciler Deployment + + // Autopilot sets Limits to Requests + setLimitsToRequests(expectedResources) + nt.T.Logf("expectedResources (adjusted for autopilot): %s", log.AsJSON(expectedResources)) + expectedTotalResources := totalExpectedContainerResources(expectedResources) + nt.T.Logf("expectedTotalResources (adjusted for autopilot): %s", log.AsJSON(expectedTotalResources)) } else { // Update expectations to match override change expectedResources[firstContainerName] = updated - // Increase the expected total resources - expectedTotalResources.CPURequest.Sub(resource.MustParse("10m")) // Expect reconciler-manager to update the reconciler Deployment generation++ } + nt.MustMergePatch(rootSyncObj, fmt.Sprintf(`{"spec":{"override":{"resources":[{"containerName":%q,"memoryRequest":%q, "cpuRequest":%q}]}}}`, firstContainerName, &updated.MemoryRequest, &updated.CPURequest)) @@ -974,24 +927,23 @@ func TestAutopilotReconcilerAdjustment(t *testing.T) { reconcilerDeployment = &appsv1.Deployment{} err = nt.Validate(reconcilerNN.Name, reconcilerNN.Namespace, reconcilerDeployment, testpredicates.HasGenerationAtLeast(generation), - totalContainerCPURequestEquals(expectedTotalResources.CPURequest), - totalContainerMemoryRequestEquals(expectedTotalResources.MemoryRequest), + testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, nt.Logger, expectedResources), firstContainerNameEquals(firstContainerName), - testpredicates.DeploymentContainerResourcesAllEqual(nt.Scheme, expectedResources), ) if err != nil { - nt.T.Log("Reconciler container specs (expected):") - for containerName, containerSpec := range expectedResources { - nt.T.Logf("%s: %s", containerName, log.AsJSON(containerSpec)) - } - nt.T.Log("Reconciler container specs (found):") - for _, container := range reconcilerDeployment.Spec.Template.Spec.Containers { - nt.T.Logf("%s: %s", container.Name, log.AsJSON(container.Resources)) - } nt.T.Fatal(err) } } +func setLimitsToRequests(resourceMap map[string]v1beta1.ContainerResourcesSpec) { + for containerName := range resourceMap { + updated := resourceMap[containerName] + updated.CPULimit = updated.CPURequest + updated.MemoryLimit = updated.MemoryRequest + resourceMap[containerName] = updated + } +} + func getDeploymentGeneration(nt *nomostest.NT, name, namespace string) int64 { dep := &appsv1.Deployment{} if err := nt.KubeClient.Get(name, namespace, dep); err != nil {