Enhance Nodegroup documentation, logging and e2e tests (#89)

Issue N/A

Description of changes:
- Fix a crucial "typo" in the `Nodegroup` annotations docs
- Add some more logging to help debugging the controller behaviour when
  dealing with the `desired-size-managed-by` annotation
- Add some e2e tests to simulate the "existence" of an external autoscaler

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: a-hilaly

README.md

@@ -29,12 +29,12 @@ For some resources, the `eks-controller` supports annotations to customize the
 behavior of the controller. The following annotations are supported:
 
 - **Nodegroup**
-    - `eks.service.k8s.aws/desired-size-managed-by`: used to control whether the
+    - `eks.services.k8s.aws/desired-size-managed-by`: used to control whether the
       controller should manage the desiredSize of the Nodegroup `spec.ScalingConfig.DesiredSize`.
       It supports the following values:
-        - `ack-eks-controller`: If set the controller will be responsible for
+        - `ack-eks-controller`: If set, the controller will be responsible for
           managing the desired size of the nodegroup.
-        - `external-autoscaler`: If set will ignore any changes to the
+        - `external-autoscaler`: If set, will ignore any changes to the
           `spec.ScalingConfig.DesiredSize` and will not manage the desired size
           of the nodegroup.
 

pkg/resource/nodegroup/hook.go

@@ -19,13 +19,14 @@ import (
 	"reflect"
 	"time"
 
-	svcapitypes "github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition"
 	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
 	svcsdk "github.com/aws/aws-sdk-go/service/eks"
 	corev1 "k8s.io/api/core/v1"
+
+	svcapitypes "github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
 )
 
 // Taken from the list of nodegroup statuses on the boto3 documentation
@@ -390,7 +391,19 @@ func (rm *resourceManager) newUpdateScalingConfigPayload(
 	sc := rm.newNodegroupScalingConfig(desired)
 	// We need to default the desiredSize to the current observed
 	// value in the case where the desiredSize is managed externally.
-	if isManagedByExternalAutoscaler(desired.ko) && latest.ko.Spec.ScalingConfig != nil {
+	isManagedExternally := isManagedByExternalAutoscaler(desired.ko)
+	if isManagedExternally {
+		rm.log.Info(
+			"detected that the desiredSize is managed by an external entity.",
+			"annotation", fmt.Sprintf("%s: '%s'", svcapitypes.DesiredSizeManagedByAnnotation, svcapitypes.DesiredSizeManagedByExternalAutoscaler),
+		)
+	}
+	if isManagedExternally && latest.ko.Spec.ScalingConfig != nil {
+		rm.log.Info(
+			"ignoring the difference in desiredSize as it is managed by an external entity.",
+			"external_desired_size", latest.ko.Spec.ScalingConfig.DesiredSize,
+			"ack_desired_size", desired.ko.Spec.ScalingConfig.DesiredSize,
+		)
 		sc.DesiredSize = latest.ko.Spec.ScalingConfig.DesiredSize
 	}
 	return sc

pkg/resource/nodegroup/hook_test.go

@@ -15,6 +15,7 @@ package nodegroup
 
 import (
 	"fmt"
+	"io"
 	"reflect"
 	"testing"
 
@@ -23,6 +24,7 @@ import (
 	svcsdk "github.com/aws/aws-sdk-go/service/eks"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	"github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
 )
@@ -354,7 +356,11 @@ func Test_resourceManager_newUpdateScalingConfigPayload_ManagedByExternalAutosca
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			rm := resourceManager{}
+			rm := resourceManager{
+				log: zap.New(zap.UseFlagOptions(&zap.Options{
+					DestWriter: io.Discard,
+				})),
+			}
 			if got := rm.newUpdateScalingConfigPayload(&resource{tt.args.desired}, &resource{tt.args.latest}); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("resourceManager.newUpdateScalingConfigPayload() = %v, want %v", got, tt.want)
 			}

test/e2e/tests/test_nodegroup.py

@@ -237,4 +237,86 @@ def test_create_update_delete_nodegroup(self, simple_nodegroup, eks_client):
             nodegroupName=nodegroup_name
         )
 
-        assert "taints" not in aws_res["nodegroup"]
+        assert "taints" not in aws_res["nodegroup"]
+
+    def test_nodegroup_custom_annotations(self, simple_nodegroup, eks_client):
+        (ref, cr) = simple_nodegroup
+
+        cluster_name = cr["spec"]["clusterName"]
+        cr_name = ref.name
+
+        nodegroup_name = cr["spec"]["name"]
+
+        try:
+            aws_res = eks_client.describe_nodegroup(
+                clusterName=cluster_name,
+                nodegroupName=nodegroup_name
+            )
+            assert aws_res is not None
+
+            assert aws_res["nodegroup"]["nodegroupName"] == nodegroup_name
+            assert aws_res["nodegroup"]["nodegroupArn"] is not None
+        except eks_client.exceptions.ResourceNotFoundException:
+            pytest.fail(f"Could not find Nodegroup '{cr_name}' in EKS")
+
+        wait_for_nodegroup_active(eks_client, cluster_name, nodegroup_name)
+
+        # Set the eks.services.k8s.aws/desired-size-managed-by annotations and
+        # update the desiredSize along with the maxSize
+        updates = {
+            "metadata": {
+                "annotations": {
+                    "eks.services.k8s.aws/desired-size-managed-by": "external-autoscaler",
+                }
+            },
+            "spec": {
+                "scalingConfig": {
+                    "minSize": 1,
+                    "maxSize": 2,
+                    "desiredSize": 3, # Controller should not apply this value
+                }
+            }
+        }
+
+        k8s.patch_custom_resource(ref, updates)
+        time.sleep(MODIFY_WAIT_AFTER_SECONDS)
+
+        wait_for_nodegroup_active(eks_client, cluster_name, nodegroup_name)
+
+        aws_res = eks_client.describe_nodegroup(
+            clusterName=cluster_name,
+            nodegroupName=nodegroup_name
+        )
+        assert aws_res["nodegroup"]["scalingConfig"]["minSize"] == 1
+        assert aws_res["nodegroup"]["scalingConfig"]["maxSize"] == 2
+        assert aws_res["nodegroup"]["scalingConfig"]["desiredSize"] == 1 # Should be 1, not 3
+
+        # Set the eks.services.k8s.aws/desired-size-managed-by annotations and
+        # update the desiredSize only
+        updates = {
+            "metadata": {
+                "annotations": {
+                    "eks.services.k8s.aws/desired-size-managed-by": "external-autoscaler",
+                }
+            },
+            "spec": {
+                "scalingConfig": {
+                    "minSize": 1,
+                    "maxSize": 2,
+                    "desiredSize": 10, # Controller should not apply this value
+                }
+            }
+        }
+
+        k8s.patch_custom_resource(ref, updates)
+        time.sleep(MODIFY_WAIT_AFTER_SECONDS)
+
+        wait_for_nodegroup_active(eks_client, cluster_name, nodegroup_name)
+
+        aws_res = eks_client.describe_nodegroup(
+            clusterName=cluster_name,
+            nodegroupName=nodegroup_name
+        )
+        assert aws_res["nodegroup"]["scalingConfig"]["minSize"] == 1
+        assert aws_res["nodegroup"]["scalingConfig"]["maxSize"] == 2
+        assert aws_res["nodegroup"]["scalingConfig"]["desiredSize"] == 1 # Should be 1, not 10