Fixup eni allocation with warm targets #1512

jayanthvn · 2021-06-16T01:22:07Z

What type of PR is this?
bug

Which issue does this PR fix:
ENI has max 14 prefixes and WARM_IP_TARGET is 240. This would need 2 ENIs. Say 1st ENI is attached with 14 prefixes and no prefixes are used. Then we would need one more, so reconciler will try attach more prefixes for ENI1 and the call would come herehttps://github.com/aws/amazon-vpc-cni-k8s/blob/prefix-delegation-preview/pkg/ipamd/ipamd.go#L794-L812 but here we will compute toAllocate = 1 [https://github.com/aws/amazon-vpc-cni-k8s/blob/prefix-delegation-preview/pkg/ipamd/ipamd.go#L800] and toAllocate <= freePrefixesInStore [https://github.com/aws/amazon-vpc-cni-k8s/blob/prefix-delegation-preview/pkg/ipamd/ipamd.go#L807] hence return will be true and we won't allocate another ENI -

amazon-vpc-cni-k8s/pkg/ipamd/ipamd.go

Line 663 in 03f2375

if increasedPool {

.

Also isRequiredForWarmIPTarget and isRequiredForMinimumIPTarget should not take Len of availableIP4Cidrs since we can have a mix of /28 and /32s, so need to walk and compute the correct value.

What does this PR do / Why do we need it:
tryAssignPrefixes similar to tryAssignIPs should always allocate prefixes based on the number of CIDRS short computed in terms of WARM_IP_TARGET/MINIMUM_IP_TARGET or WARM_PREFIX_TARGET. Instead tryAssignCidrs should not call tryAssignPrefix if the number of prefixes is sufficient to reach the WARM_IP_TARGET or WARM_PREFIX_TARGET.

getPrefixesNeeded returns either 1 prefix or number of prefixes needed to reach the warm target.
datastorePrefixTargetState returns the number of prefixes short to reach WARM_PREFIX_TARGET.

Post GA, I will see how to combine datastorePrefixTargetState and datastoreTargetState since already there is lot of logic in datastoreTargetState and it will be a disruptive change to achieve in the current timeline.

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

Testing done on this change:

WARM_IP_TARGET = 240
MINIMUM_IP_TARGET = 10

Node 1 
5, short(prefixes): 0, over(prefixes): 0"}
{"level":"debug","ts":"2021-06-15T23:25:43.796Z","caller":"ipamd/ipamd.go:1047","msg":"Current warm IP stats : target: 240, total: 256, assigned: 1, available: 255, short(prefixes): 0, over(prefixes): 0"}
{"level":"debug","ts":"2021-06-15T23:25:43.796Z","caller":"datastore/data_store.go:852","msg":"ENI eni-0feab4a86670d407c cannot be deleted because it is primary"}
{"level":"debug","ts":"2021-06-15T23:25:43.796Z","caller":"datastore/data_store.go:774","msg":"Found 13 free cidrs on ENI eni-05eac5de44a62637b"}
{"level":"debug","ts":"2021-06-15T23:25:43.796Z","caller":"datastore/data_store.go:774","msg":"warm target 15"}
{"level":"debug","ts":"2021-06-15T23:25:43.796Z","caller":"datastore/data_store.go:852","msg":"ENI eni-05eac5de44a62637b cannot be deleted because it is required for WARM_IP_TARGET: 240"}

Node 2
{"level":"debug","ts":"2021-06-15T23:37:14.366Z","caller":"ipamd/ipamd.go:1047","msg":"Current warm IP stats : target: 240, total: 240, assigned: 0, available: 240, short(prefixes): 0, over(prefixes): 0"}
{"level":"debug","ts":"2021-06-15T23:37:14.366Z","caller":"datastore/data_store.go:774","msg":"Found 14 free cidrs on ENI eni-06ef2989c38dc3add"}
{"level":"debug","ts":"2021-06-15T23:37:14.366Z","caller":"datastore/data_store.go:774","msg":"warm target 15"}
{"level":"debug","ts":"2021-06-15T23:37:14.366Z","caller":"datastore/data_store.go:852","msg":"ENI eni-06ef2989c38dc3add cannot be deleted because it is required for WARM_IP_TARGET: 240"}
{"level":"debug","ts":"2021-06-15T23:37:14.366Z","caller":"datastore/data_store.go:852","msg":"ENI eni-0283a74052cb94d2f cannot be deleted because it is primary"}

MINIMUM_IP_TARGET=240

Node 1
{"level":"debug","ts":"2021-06-15T23:52:56.925Z","caller":"ipamd/ipamd.go:1047","msg":"Current warm IP stats : target: 0, total: 240, assigned: 0, available: 240, short(prefixes): 0, over(prefixes): 0"}
{"level":"debug","ts":"2021-06-15T23:52:56.925Z","caller":"datastore/data_store.go:852","msg":"ENI eni-0283a74052cb94d2f cannot be deleted because it is primary"}
{"level":"debug","ts":"2021-06-15T23:52:56.925Z","caller":"datastore/data_store.go:852","msg":"ENI eni-06ef2989c38dc3add cannot be deleted because it is required for MINIMUM_IP_TARGET: 240"}

Node 2
"level":"debug","ts":"2021-06-15T23:55:39.239Z","caller":"ipamd/ipamd.go:1047","msg":"Current warm IP stats : target: 0, total: 240, assigned: 1, available: 239, short(prefixes): 0, over(prefixes): 0"}
{"level":"debug","ts":"2021-06-15T23:55:39.239Z","caller":"datastore/data_store.go:852","msg":"ENI eni-05eac5de44a62637b cannot be deleted because it is required for MINIMUM_IP_TARGET: 240"}
{"level":"debug","ts":"2021-06-15T23:55:39.239Z","caller":"datastore/data_store.go:852","msg":"ENI eni-0feab4a86670d407c cannot be deleted because it is primary"}

Removed MINIMUM_IP_TARGET=240

Node 1
{"level":"info","ts":"2021-06-15T23:57:27.993Z","caller":"ipamd/ipamd.go:1902","msg":"Trying to unassign the following Prefixes [192.168.30.144/28 192.168.11.0/28 192.168.6.144/28 192.168.0.160/28 192.168.27.0/28 192.168.12.144/28 192.168.19.144/28 192.168.18.16/28 192.168.18.144/28 192.168.4.0/28 192.168.16.0/28 192.168.26.16/28 192.168.10.224/28] from ENI eni-0feab4a86670d407c"}
{"level":"debug","ts":"2021-06-15T23:57:28.493Z","caller":"ipamd/ipamd.go:1902","msg":"Successfully freed Prefixes [192.168.30.144/28 192.168.11.0/28 192.168.6.144/28 192.168.0.160/28 192.168.27.0/28 192.168.12.144/28 192.168.19.144/28 192.168.18.16/28 192.168.18.144/28 192.168.4.0/28 192.168.16.0/28 192.168.26.16/28 192.168.10.224/28] from ENI eni-0feab4a86670d407c"}
{"level":"info","ts":"2021-06-15T23:57:28.493Z","caller":"ipamd/ipamd.go:649","msg":"Deleted ENI(eni-05eac5de44a62637b)'s IP/Prefix 192.168.11.96/28 from datastore"}
{"level":"info","ts":"2021-06-15T23:57:28.493Z","caller":"ipamd/ipamd.go:649","msg":"Deleted ENI(eni-05eac5de44a62637b)'s IP/Prefix 192.168.1.48/28 from datastore"}
{"level":"info","ts":"2021-06-15T23:57:28.493Z","caller":"ipamd/ipamd.go:1902","msg":"Trying to unassign the following Prefixes [192.168.11.96/28 192.168.1.48/28] from ENI eni-05eac5de44a62637b"}
{"level":"debug","ts":"2021-06-15T23:57:28.835Z","caller":"ipamd/ipamd.go:1902","msg":"Successfully freed Prefixes [192.168.11.96/28 192.168.1.48/28] from ENI eni-05eac5de44a62637b"}
{"level":"debug","ts":"2021-06-15T23:57:28.835Z","caller":"ipamd/ipamd.go:564","msg":"Successfully decreased IP pool"}
{"level":"debug","ts":"2021-06-15T23:57:28.835Z","caller":"ipamd/ipamd.go:590","msg":"Prefix pool stats: total = 16, used = 1, c.maxIPsPerENI = 224"}

Node 2
{"level":"debug","ts":"2021-06-15T23:59:05.637Z","caller":"ipamd/ipamd.go:555","msg":"IP/Prefix Address Pool stats: total: 0, assigned: 0, total prefixes: 0"}
{"level":"debug","ts":"2021-06-15T23:59:08.137Z","caller":"ipamd/ipamd.go:1848","msg":"Prefix pool stats: total = 0, used = 0, c.maxIPsPerENI = 224"}

WARM_PREFIX_TARGET=15
Node 1
{"level":"debug","ts":"2021-06-16T00:02:50.231Z","caller":"ipamd/ipamd.go:879","msg":"Datastore Pool stats: total(/32): 240, assigned(/32): 0, total prefixes(/28): 15"}
{"level":"debug","ts":"2021-06-16T00:02:50.231Z","caller":"ipamd/ipamd.go:706","msg":"Successfully increased Prefix pool, total: 15, used: 0"}
{"level":"debug","ts":"2021-06-16T00:02:50.231Z","caller":"ipamd/ipamd.go:733","msg":"Prefix pool stats: total = 240, used = 0, c.maxIPsPerENI = 224"}

Node 2
{"level":"debug","ts":"2021-06-16T00:02:33.558Z","caller":"ipamd/ipamd.go:879","msg":"Datastore Pool stats: total(/32): 256, assigned(/32): 1, total prefixes(/28): 16"}
{"level":"debug","ts":"2021-06-16T00:02:33.558Z","caller":"ipamd/ipamd.go:706","msg":"Successfully increased Prefix pool, total: 16, used: 1"}

Automation added to e2e:

Yes

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?:

No

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

pkg/ipamd/datastore/data_store.go

jayanthvn · 2021-06-17T05:10:03Z

pkg/ipamd/ipamd.go

+	// no prefixes used. Hence better to handle it. 
+	// [ENI allocates/frees in loop with custom networking and WARM_ENI_TARGET=0 #1451]
+	// But Will sync up internally with the team and see if need to even support WARM_ENI_TARGET = 0 and WARM_PREFIX_TARGET = 0
+	if over > 0 && ((available - (over * numIPsPerPrefix)) <= 0) && c.warmPrefixTarget == 0 {


I have discussed this internally and having WARM_{PREFIX,ENI}TARGET = 0 and WARM_IP_TARGET = 0 will not help adding IPs to datastore. So in the follow up PR, this section will be removed and also (c.warmENITarget == 0 && available == 0) in isDataStoreLow(). Also will be adding documentation on the recommended configuration setting. WARM_IP_TARGET will override WARM{PREFIX,ENI}TARGET as expected but if there is WARM{PREFIX,ENI}_TARGET then it has to be non-zero. This will fix #1451 which will cause unwanted churn with ENI creates and deletes. In code default value of warm_eni_target is 1 so will make warm_prefix_target to 1 by default too.

Since, we're introducing WARM_PREFIX_TARGET for the very first time we can avoid this scenario by not allowing 0 for this env variable. Our default is going to be 1 anyways and we will be able to do away with this additional check. As discussed offline, we can take it up in a follow up PR.

Yes Apurup, For warm_prefix_target, lets default it to 1. But for warm_eni_target there might be some Cx using it so lets discuss more and see how we can decommission the support since it has few issues where IPAMD silently keeps adding and removing the ENI.

achevuru · 2021-06-17T06:29:34Z

pkg/ipamd/ipamd.go

-		return
+	if !warmIPTargetDefined {
+		shortPrefix, warmTargetDefined := c.datastorePrefixTargetState()
+        	if warmTargetDefined && shortPrefix == 0 {


How about when WARM_PREFIX_TARGET is not defined (i.e.,) when warmTargetDefined is false? Shouldn't we return in that scenario as well? It appears that we're going to continue further if that is the case with the current logic.

This is added to give WARM_IP_TARGET precedence over WARM_PREFIX_TARGET. If WARM_PREFIX_TARGET is not defined then tryAssignCidr will allocate just one prefix.

achevuru · 2021-06-17T06:30:02Z

pkg/ipamd/ipamd.go

-		return false, nil
+	if !warmIPTargetDefined {
+		shortPrefix, warmTargetDefined := c.datastorePrefixTargetState()
+       		if warmTargetDefined && shortPrefix == 0 {


Same comment as above

TryAssignCidr is called from nodeInit and reconciler(previous commented function), hence we need to add the check in both places. Also the reasoning is same, we need to give priority to WARM_IP_TARGET and then check WARM_PREFIX_TARGET since in the manifest we can have both defined.

…ter (#1516) * [Preview] Prefix delegation feature development (#1434) * ENABLE_PREFIX_DELEGATION knob WARM_PREFIX_TARGET knob cr https://code.amazon.com/reviews/CR-40610031 * PD changes - dev only * Cooldown prefix IP * minor fixes to support prefix count * Code cleanup * Handle few corner cases * Nitro based check * With custom networking, do not get prefix for primary ENI * Code refactor * Handle graceful upgrade/enable PD from disable PD * code refactoring * Code refactoring * fix computing too low IPs * UT for prefix store * Fix UTs and handle CR comments * Clean up SDK code and fix model code generation * fix format and merge induced error * Merge broke the code * Fix Dockerfile.test * Added IPAMD UTs and fixed removeENI total count * Couple more IPAMD UTs for PD * UTs for awsutils/imds * Handle graceful PD enable to disable knob * get prefix list for non-pd case * Prevent reconcile of prefix IPs in IP datastore * Handle disable scenario * fix formatting * clean up comment * Remove unnecessary debugs * Handle PR comments * formatting fix * Remodelled PD datastore * Fix up UTs and fix Prefix nil * formatting * PR comments - minor cosmetic changes * removed the sdk override from makefile * Internal repo merge added these lines * Update config file * Handle wrapper of DescribeNetworkInterfacesWithContext to take one eni * RemoveUnusedENIFromStore was not accounting for prefixes deleted * Removed hardcoding of 16 * Code refactor - merge ENI's secondary and prefix store into single store of CIDRs (#1471) * Code refactor - merge to single DB * remove few debugs * remove prefix store files * PR comments * Fix up CR comments * formatting * Updated UT cases * UT and formatting * Minor fixes * Minor comments * Updated /32 store term * remove unused code * Multi-prefix and WARM/MIN IP targets support with PD (#1477) * Multi-pd and WARM targets support * cleanup * Updated variable names * Default prefix count to -1 * Get stats should be computed on the fly since CIDR pool can have /32 or /28 * Support for warm prefix 0 * code review comments * PD test cases and readme update (#1478) * Traffic test case and readme update * Added testcases for warm ip/min ip with PD * Testcases for prefix count * Testcase for warm prefix along with warm ip/min ip * Updated traffic test case while PD mode is flipped * Fix minor comments * pr comments * added pods per eni * fix up count * Support mixed instances with PD (#1483) * Support mixed instances with PD * fix up the log * Optimization for prefixes allocation (#1500) * optimization for prefixes Prefix store optimization * pr comment * Fixup eni allocation with warm targets (#1512) * Fixup eni allocation with warm targets * fixup cidr count * code comments and warm prefix 0 * Default WARM_PREFIX_TARGET to 1 (#1515) * Handle prefix target 0 * pr commets * Fix up UTs, was failing because of vendor * No need to commit this * make format * needed for UT workflow * IMDS code refactor * PR comments - v1 Error with merge * PR comments v2 * PR comments - v3 * Update logs * PR comments - v4

jayanthvn requested a review from achevuru June 16, 2021 01:22

Fixup eni allocation with warm targets

90eeb74

jayanthvn force-pushed the pd_warmipfix branch from 404c6c7 to 90eeb74 Compare June 16, 2021 01:30

achevuru reviewed Jun 16, 2021

View reviewed changes

pkg/ipamd/datastore/data_store.go Outdated Show resolved Hide resolved

fixup cidr count

5660664

jayanthvn force-pushed the pd_warmipfix branch from 3d041ad to 5660664 Compare June 16, 2021 12:50

achevuru reviewed Jun 16, 2021

View reviewed changes

pkg/ipamd/datastore/data_store.go Show resolved Hide resolved

achevuru reviewed Jun 16, 2021

View reviewed changes

pkg/ipamd/datastore/data_store.go Show resolved Hide resolved

achevuru reviewed Jun 16, 2021

View reviewed changes

pkg/ipamd/datastore/data_store.go Show resolved Hide resolved

code comments and warm prefix 0

bb5c74a

jayanthvn force-pushed the pd_warmipfix branch from 66942db to bb5c74a Compare June 17, 2021 01:09

jayanthvn commented Jun 17, 2021

View reviewed changes

achevuru reviewed Jun 17, 2021

View reviewed changes

achevuru approved these changes Jun 17, 2021

View reviewed changes

jayanthvn merged commit 9dfc9b2 into aws:prefix-delegation-preview Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup eni allocation with warm targets #1512

Fixup eni allocation with warm targets #1512

jayanthvn commented Jun 16, 2021 •

edited

Loading

jayanthvn Jun 17, 2021 •

edited

Loading

achevuru Jun 17, 2021

jayanthvn Jun 17, 2021

achevuru Jun 17, 2021

jayanthvn Jun 17, 2021

achevuru Jun 17, 2021

jayanthvn Jun 17, 2021

Fixup eni allocation with warm targets #1512

Fixup eni allocation with warm targets #1512

Conversation

jayanthvn commented Jun 16, 2021 • edited Loading

jayanthvn Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

achevuru Jun 17, 2021

Choose a reason for hiding this comment

jayanthvn Jun 17, 2021

Choose a reason for hiding this comment

achevuru Jun 17, 2021

Choose a reason for hiding this comment

jayanthvn Jun 17, 2021

Choose a reason for hiding this comment

achevuru Jun 17, 2021

Choose a reason for hiding this comment

jayanthvn Jun 17, 2021

Choose a reason for hiding this comment

jayanthvn commented Jun 16, 2021 •

edited

Loading

jayanthvn Jun 17, 2021 •

edited

Loading