Fix and improve migrate-nodegroup test

[metral]

Fixes #194.

The changes in this fix resolve a leaked ENI that lead to a dependency violation on tear down. It was resolved by bumping up aws-cni to v1.5.2 that was recently released [1]. We now also:

• Wait 5 min after the migration step to ensure it completes,
• Delete the workload Namespace, and aws-cni DaemonSet to ensure these get explicitly torn down.

This fix was tested across 70+ runs of the migrate-nodegroup test, and no run hit issue #194.

Additionally, logLevel, logFile, and image have been added as options to VpcCniOptions, and defaults for logging have been adjusted to be verbose, and be produced for the aws-cni Pods.

---

[1] - The specific fixes in aws-cni v1.5.2 were:

• Detach ENI before deleting.
• The addition of a healthz endpoint: Adding healthz endpoint to IPamD aws/amazon-vpc-cni-k8s#548 and Update start script to wait for ipamd health check aws/amazon-vpc-cni-k8s#553. The healthz endpoint is used in the readiness & liveness probes of the DaemonSet.

[lukehoban]

Does this now march any released version of this yaml from the source? Would love to keep this identical to some released version if possible.

[metral]

The upstream YAML manifest and ours match. Below is the diff between upstream and ours.
The main differences lie in:

• Removing the image, and AWS_VPC_K8S_CNI_LOGLEVEL as we set those above in cni.ts
• Enabling the use of the readiness & liveness probes since these are omitted upstream as this healthz endpoint is only available on CNI images >= v1.5.2 which we use now.

$ diff -u upstream-aws-k8s-cni.yaml aws-k8s-cni.yaml
--- upstream-aws-k8s-cni.yaml   2019-08-08 11:28:15.838079451 -0700
+++ aws-k8s-cni.yaml    2019-08-07 19:12:21.888614038 -0700
@@ -81,23 +81,20 @@
       tolerations:
         - operator: Exists
       containers:
-        - image: 602401143452.dkr.ecr.us-west-2.amazonaws.com/amazon-k8s-cni:v1.5.2
-          imagePullPolicy: Always
+        - imagePullPolicy: Always
           ports:
             - containerPort: 61678
               name: metrics
           name: aws-node
-          #readinessProbe:
-          #  exec:
-          #    command: ["/app/grpc_health_probe", "-addr=:50051"]
-          #  initialDelaySeconds: 5
-          #livenessProbe:
-          #  exec:
-          #    command: ["/app/grpc_health_probe", "-addr=:50051"]
-          #  initialDelaySeconds: 5
+          readinessProbe:
+            exec:
+              command: ["/app/grpc_health_probe", "-addr=:50051"]
+            initialDelaySeconds: 5
+          livenessProbe:
+            exec:
+              command: ["/app/grpc_health_probe", "-addr=:50051"]
+            initialDelaySeconds: 5
           env:
-            - name: AWS_VPC_K8S_CNI_LOGLEVEL
-              value: DEBUG
             - name: MY_NODE_NAME
               valueFrom:
                 fieldRef:

[metral]

Shouldn’t we just update the image in the yaml to the right default and use similar overrides as above?

I'll make changes to adopt the override approach instead of the current one, and not remove bits from the YAML manifest directly to maintain that with upstream as best we can. Though note that logFile is not a default in the YAML manifest that we have to set to get any sort of meaningful logs out of aws-cni

[metral]

Feedback has been addressed. PTAL @lukehoban.

Updated diff in the YAML manifest. The diffs are:

• Enabling the use of the probes since pulumi-eks has been updated to using image = v1.5.2
• Adding in a missing envvar for AWS_VPC_K8S_CNI_LOG_FILE in the YAML manifest to have meaningful logs, and to allow to be user-configurable via override approach.

$ diff -u upstream-aws-k8s-cni.yaml aws-k8s-cni.yaml
--- upstream-aws-k8s-cni.yaml   2019-08-08 11:52:46.430206543 -0700
+++ aws-k8s-cni.yaml    2019-08-08 11:51:38.085461619 -0700
@@ -87,17 +87,19 @@
             - containerPort: 61678
               name: metrics
           name: aws-node
-          #readinessProbe:
-          #  exec:
-          #    command: ["/app/grpc_health_probe", "-addr=:50051"]
-          #  initialDelaySeconds: 5
-          #livenessProbe:
-          #  exec:
-          #    command: ["/app/grpc_health_probe", "-addr=:50051"]
-          #  initialDelaySeconds: 5
+          readinessProbe:
+            exec:
+              command: ["/app/grpc_health_probe", "-addr=:50051"]
+            initialDelaySeconds: 5
+          livenessProbe:
+            exec:
+              command: ["/app/grpc_health_probe", "-addr=:50051"]
+            initialDelaySeconds: 5
           env:
             - name: AWS_VPC_K8S_CNI_LOGLEVEL
               value: DEBUG
+            - name: AWS_VPC_K8S_CNI_LOG_FILE
+              value: stdout
             - name: MY_NODE_NAME
               valueFrom:
                 fieldRef:

[lukehoban]

LGTM

[lukehoban]

Great that this is ultimately addressed by fixes in 1.5.2 CNI!

[metral]

Great that this is ultimately addressed by fixes in 1.5.2 CNI!

Agreed!

Also, the explicit removal of the NGINX workload and aws-cni also helped to achieve successful cluster tear downs. #215 could help here, and would remove our need to do these manual deletions in the migrate-nodegroup test.