Skip to content

Commit

Permalink
test: Fix TestAutopilotReconcilerAdjustment (#882)
Browse files Browse the repository at this point in the history 
Fix Autopilot resource math:
- Set limits to requsts
- Round CPU up to 250m
- Round Memory up to 512Mi
  • Loading branch information
@karlkfi
karlkfi authored Sep 15, 2023
1 parent fd864d9 commit 7e0acda
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 117 deletions.
38 changes: 35 additions & 3 deletions e2e/nomostest/testpredicates/predicates.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -289,14 +291,16 @@ 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]
if !ok {
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
Expand All @@ -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)
Expand Down
180 changes: 66 additions & 114 deletions e2e/testcases/reconciler_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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()
Expand All @@ -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.
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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))
Expand All @@ -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 {
Expand Down

0 comments on commit 7e0acda

Please sign in to comment.