Code refactor - merge ENI's secondary and prefix store into single store of CIDRs #1471
Conversation
|
@anguslees and @achevuru - I have updated the PR based on the review comments. |
jayanthvn
changed the title
pkg/ipamd/datastore/data_store.go
Outdated
| @@ -207,13 +228,13 @@ func (p *ENIPool) AssignedIPv4Addresses() int { | |||
| } | |||
|
|
|||
| // FindAddressForSandbox returns ENI and AddressInfo or (nil, nil) if not found | |||
| func (p *ENIPool) FindAddressForSandbox(ipamKey IPAMKey) (*ENI, *AddressInfo) { | |||
| func (p *ENIPool) FindAddressForSandbox(ipamKey IPAMKey) (*ENI, *AssignedIPv4Addresses, *AddressInfo) { | |||
There was a problem hiding this comment.
nit: map is passed by "reference", i don't think we need pointer receiver here.
There was a problem hiding this comment.
There are quite a places even in the legacy code where we are passing the pointer (AddressInfo*). It would need quite some code changes so will discuss with you and take it up in a followup PR.
There was a problem hiding this comment.
Discussed with Yang, there is lot of legacy code which needs this clean up, so will take up in future PRs.
| return err | ||
| ipv4Addr := net.ParseIP(allocation.IPv4) | ||
| found := false | ||
| eniloop: |
There was a problem hiding this comment.
it feels to me that we can make this more maintainable by abstract this into a method in ENIPool.
something like RecordIPv4AddressAllocation.
There was a problem hiding this comment.
Will address this with the next PR for warm ip target support.
| ds.allocatedPrefix-- | ||
| // Prometheus gauge | ||
| ones, bits := cidr.Mask.Size() | ||
| ds.total -= 1 << (bits - ones) |
There was a problem hiding this comment.
since this logic(1 << (bits - ones)) happens multiple places, it make sense to be a method on cidr.
e.g. CIDRInfo.Size() or CIDRInfo.GetIPv4AddressCount(),
There was a problem hiding this comment.
Will address this in the next PR.
| var err error | ||
|
|
||
| if ds.isPDEnabled && availableCidr.IsPrefix { | ||
| strPrivateIPv4, err = ds.getFreeIPv4AddrfromCidr(availableCidr) |
There was a problem hiding this comment.
why for cidr, we use getFreeIPv4AddrfromCidr(availableCidr)
but for sip, we didn't use it.
the place where we check for assigned/cooldown differs between this two code path (but ideally it can be treated same way)
There was a problem hiding this comment.
With PD, we try to get a free unused IP/32 from /28 hence I abstracted that in a function but whereas for SIP we already have a /32. As you suggested(next comment), in the following PR, I will create a function under CidrInfo to allocate a free IP.
| addr.Prefix = prefix.Prefix.IP | ||
| addr = availableCidr.IPv4Addresses[strPrivateIPv4] | ||
| if addr == nil { | ||
| // addr is nil when we are using a new IP from prefix or SIP pool |
There was a problem hiding this comment.
this is hacky, as it couples the implementation detail of low level data structure in high level business code.
How about a method in CIDR like, so it hides the memory handling behind CIDR structure.
func (i *CIDR) tryAllocateFreeIPAddress() (*AddressInfo, bool) {
if i.IsPrefix {
return i.tryAllocateFreeIPAddressForPrefix()
}
tryAllocateFreeIPAddressForSIP()
}
func (i *CIDR) tryAllocateFreeIPAddressForPrefix() (*AddressInfo, bool) {
...
do the allocation
}
func (i *CIDR) tryAllocateFreeIPAddressForSIP() (*AddressInfo, bool) {
....
do the allocation
}
There was a problem hiding this comment.
Will push this logic into tryAllicateFreeIPAddress*
| } | ||
| ds.log.Debugf("New IP from PD pool- %s", strPrivateIPv4) | ||
| if availableCidr.IPv4Addresses == nil { | ||
| availableCidr.IPv4Addresses = make(map[string]*AddressInfo) |
There was a problem hiding this comment.
nit: when will this IPv4Addresses ever be nil? seem it never happens from the code i reviewed.
I'm ok to have this safety check, but let's add some comment and debug statement here.
also, this is a sign that we are not encapsulation memory handling logic good enough.
There was a problem hiding this comment.
Yes, it is just for safety check, will add a debug
There was a problem hiding this comment.
overall, we should improve the encapsulation here.
ideally, the datastore shouldn't touch the internal members of CIDRs directly(which hides complicity for IPaddress/memory management behind cidr). also, we might need to consider when to use pointers vs when to use structures directly. (e.g. the addressInfo)
I'm open to merge this RP and improve in separate PRs given it's not release candidate and there are a lot legacy code involved.
|
I have merged the PR to unblock the rest of the development. Please feel free drop in comments and I will take it up in the future PRs. |
…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
What type of PR is this?
cleanup
Which issue does this PR fix:
Merges Secondary IP DB and Prefix DB
What does this PR do / Why do we need it:
In the preview release we had 2 DBs, with this we will have a single DB ->
AvailableCIDRs->AssignedIPs->AddressInfo
Eg:
/32 -> /32 - {address info}
or
/28 -> ip1/32 - {address info}
ip2/32 - {address info}
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Testing done on this change:
Yes
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?:
No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.