Default WARM_PREFIX_TARGET to 1

[jayanthvn]

What type of PR is this?
enhancement

Which issue does this PR fix:
WARM_PREFIX_TARGET is implemented similar to WARM_ENI_TARGET. Today we do support WARM_ENI_TARGET to 0 but the problem with this [1] with custom networking we allocate secondary ENI since primary ENI is not used and if there are no pods then IPAMD will determine this as an extra ENI since the target is 0 and free the ENI. Later IPAMD will see "available is 0 and WARM_ENI_TARGET is 0" so allocates a new ENI. This happens continuously by the reconciler adding to more EC2 calls on the account. This is tracked with this issue - #1451 [2] Without custom networking, scaling will be very slow - Initially on nodeInit and ENI will allocated Max secondary IPs and once those IPs are used, the next ENI will be allocated after the next reconciler starts and enters here - https://github.com/aws/amazon-vpc-cni-k8s/blob/master/pkg/ipamd/ipamd.go#L911 and then few more seconds for IPs to be available in IMDS so until then pods will be stuck in container creating. Similar fix done in this PR should be done even for WARM_ENI_TARGET but that will break upgrades if someone is already using this so it would need reach out and documentation update.

Similar issue will happen with WARM_PREFIX TARGET, prefixes will be attached and removed in a loop by the reconciler and when more prefixes are needed we need to wait for the reconciler.

<Sorry @anguslees copying your explanation below since it will help our documentation :D :D >
[1]"I am IP constrained" (allocate/free to maintain a small warm pool -> WARM_IP_TARGET=$small; WARM_ENI/PREFIX=$small or 0; possibly choose SIP-mode)
[2]"I am AWS-API-calls constrained" (allocate/free in large batches -> WARM_ENI/PREFIX=$small or $big)
[3]"I want to burst as quickly as I can" (maintain a large warm pool -> WARM_ENI/PREFIX=$big)

What does this PR do / Why do we need it:
[1] With prefix delegation, we won't support WARM_{PREFIX,IP}_TARGET = 0 and MINIMUM_IP_TARGET=0.
[2] default WARM_PREFIX_TARGET to 1.

If WARM_{PREFIX,IP}_TARGET = 0/unset and MINIMUM_IP_TARGET=0/unset, then aws-node will fail to start with an error.

WARM_PREFIX_TARGET = 0 but WARM_IP_TARGET or MINIMUM_IP_TARGET is non zero will work since WARM_IP_TARGET /MINIMUM_IP_TARGET overrides WARM_PREFIX_TARGET

One combination which this PR won't handle is WARM_PREFIX_TARGET = non-zero but WARM_IP_TARGET or MINIMUM_IP_TARGET is zero, since this is supported with secondary mode and this will be evaluated along with WARM_ENI_TARGET.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
n/a

Testing done on this change:

kubectl logs aws-node-ppwcj -n kube-system
{"level":"info","ts":"2021-06-17T19:43:57.967Z","caller":"entrypoint.sh","msg":"Validating env variables ..."}
{"level":"error","ts":"2021-06-17T19:43:57.968Z","caller":"entrypoint.sh","msg":"Setting WARM_PREFIX_TARGET = 0 is not supported while WARM_IP_TARGET/MINIMUM_IP_TARGET is not set. Please configure either one of the WARM_{PREFIX/IP}_TARGET or MINIMUM_IP_TARGET env variables"}

Automation added to e2e:

No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No

Does this change require updates to the CNI daemonset config files to work?:

No

Does this PR introduce any user-facing change?:

WARM_{PREFIX,IP}_TARGET = 0/unset and MINIMUM_IP_TARGET=0/unset is not supported with PD.

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

[M00nF1sh]

will we ever change defaultWarmPrefixTarget to a value than 0?
If we change it to be same thing like 2, then this read as c.warmPrefixTarget >= 2. I feel this should be something like c.warmPrefixTarget >= 0

[jayanthvn]

defaultWarmPrefixTarget is 1 in this PR and also setting just WARM_PREFIX_TARGET = 0 won't be supported. If someone sets WARM_IP_TARGET = num and WARM_PREFIX_TARGET = 0 or any num then WARM_IP_TARGET will take precedence.

[M00nF1sh]

High level comment:

1. we should fix the reconcile logic. It's clearly a bug if it's allocate/deallocate ENI in loops under specific condition. From the code i read, it seems the purpose of poolTooLow := available < c.maxIPsPerENI*c.warmENITarget || (c.warmENITarget == 0 && available == 0) is when warmENITarget is 0, we only allocate one more ENI when available is 0. However, the ENIDeallocation logic is not honer above, which should be fixed.
2. Ideally the fix can be as simple as normalize warmENITarget and warmIPTarget at startup, so that if warmENITarget==0, warmIPTarget==0, we normalize it to be as "warmIPTarget==1", and remove this hacky logic of c.warmENITarget == 0 && available == 0 above.
3. Ideally warmPrefixTarget should be handled similar to warmENITarget as above. Setting it to 0 should give customer the same effect of warmIPTarget==1.

[M00nF1sh]

I'm ok to have this restriction at beginning and relax it later with a better fix.

[jayanthvn]

High level comment:

1. we should fix the reconcile logic. It's clearly a bug if it's allocate/deallocate ENI in loops under specific condition. From the code i read, it seems the purpose of poolTooLow := available < c.maxIPsPerENI*c.warmENITarget || (c.warmENITarget == 0 && available == 0) is when warmENITarget is 0, we only allocate one more ENI when available is 0. However, the ENIDeallocation logic is not honer above, which should be fixed.
2. Ideally the fix can be as simple as normalize warmENITarget and warmIPTarget at startup, so that if warmENITarget==0, warmIPTarget==0, we normalize it to be as "warmIPTarget==1", and remove this hacky logic of c.warmENITarget == 0 && available == 0 above.
3. Ideally warmPrefixTarget should be handled similar to warmENITarget as above. Setting it to 0 should give customer the same effect of warmIPTarget==1.

Yes I agree, that's why would want to not support WARM_PREFIX_TARGET = 0 now. Later fix up WARM_{ENI,PREFIX}_TARGET which will need rewriting warm pool high and low logic. My proposal to fix this was also [2] which you have suggested. That will be correct since now, reconciler has to start, ENI/IP should be synced to IMDS and then IP will be available for the pods which will take few minutes. In the meantime if old IPs are free then those won't be available for next 30 seconds.

[M00nF1sh]

/lgtm