Update probe settings

[mogren]

Description of changes:

• Reduce readiness probe startup delay to 1s
• Increase liveness probe startup delay to 60, since it will only fail if the CNI fails to start, or has crashed.
• Reduce shutdown grace period to 10 seconds

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

[anguslees]

I think updateStrategy.rollingUpdate.maxUnavailable=10% still requires the readiness probe to determine "Available"

[mogren]

Oh, you are right! We might have to revisit the time it takes though, rolling updates take a really long time currently.

[anguslees]

We might have to revisit the time it takes though

How are the new values arrived at?
(So we can point back at this PR and our original assumptions, when we next want to change these values)

[mogren]

The default terminationGracePeriodSeconds is 30 seconds. The CNI does not really need any time to shut down, so reducing this to 10s.

[anguslees]

why not zero (sigkill the pod immediately)?

[mogren]

Here I did set it at 10s on purpose to avoid leaking ENIs. There is a shutdown hook and that flag is checked before trying to free or add an ENI. If the shutdown happens just when ipamd has detached an ENI, but before it has deleted it, we would "leak" it. It usually takes around 2 seconds for the detach to complete, but p99 is 10s according to EC2. Same for creating ENIs, we want the create to complete and the ENI to be attached to the node (with the delete on termination flag set) so that it gets cleaned up if the node terminates.

That said, i don't think we have done enough testing on this.

[anguslees]

(:rocket: This is exactly the sort of analysis I was hoping to capture)

[mogren]

Helped a lot! I tested on a cluster with 50 pods on 3 nodes:

kube-system   aws-node-2ph8t              0/1     ContainerCreating   0          1s      
kube-system   aws-node-2ph8t              1/1     Running             0          3s 

[haouc]

Looks good to me. Thanks.

Changes addressed 5 days ago :)

[anguslees]

(:rocket: This is exactly the sort of analysis I was hoping to capture)

[anguslees]

The PR description says "Increase liveness polling period". I think this PR increases the liveness initial delay period however - which one was intended? (I suspect this should have been periodSeconds=60)

(Separately) jsonnet style tip: You can "merge" values with +. Since liveness/readiness probes are very similar, I would do this as:

livenessProbe: self.readinessProbe + {
    initialDelaySeconds: 60, // If desired, but see above comment
    periodSeconds: 60,
},

[mogren]

I'll fix the comment. I think, like you said in #1028 (comment), that we should just increase the initial timeout, but keep the period at the default in order to catch issues that happen after startup faster. That should give the aws-node about 90 seconds to get started, after a 60 s startup delay and three 10 s liveness probes.

[mogren]

We will need to bump this time as well:

amazon-vpc-cni-k8s/scripts/entrypoint.sh

Lines 49 to 61 in a5d340b

	# Checks for IPAM connectivity on localhost port 50051, retrying connectivity
	# check with a timeout of 36 seconds
	wait_for_ipam() {
	local __sleep_time=0
	until [ $__sleep_time -eq 8 ]; do
	sleep $((__sleep_time++))
	if ./grpc-health-probe -addr 127.0.0.1:50051 >/dev/null 2>&1; then
	return 0
	fi
	done
	return 1
	}