[POC] lease lock per IP pool #314
Conversation
This is a POC level code for changing lease/leaderElection usage by whereabouts from a global lease to a per pool lease. Signed-off-by: adrianc <adrianc@nvidia.com>
adrianchiris
force-pushed
the
split-leases
branch
from
1191229
to
79e1f1d
Compare
|
I'd say this makes sense without looking in detail ! Please be patient while we sort this out. If you can, please rebase and ensure the tests are passing. @xagent003 could you review ? |
| } | ||
|
|
||
| defer func() { | ||
| if err != nil { |
There was a problem hiding this comment.
I dont think this will ever be hit? It only happens if err != nil but all previous conditions check for err != nil and continue or return.
Is there a reason for this defer func() at all? It does exactly the same as what was previously done above
| @@ -557,14 +592,6 @@ func IPManagementKubernetesUpdate(ctx context.Context, mode int, ipam *Kubernete | |||
| break RETRYLOOP | |||
| } | |||
|
|
|||
| if ipamConf.OverlappingRanges { | |||
There was a problem hiding this comment.
Can you clarify why this change was made - the code is common but was added back tot he Allocate and Deallocate case statements. Now the same code is duplicated in each case. Did this fix some other issue or optimization?
|
Hi @maiqueb I reviewed. the lease lock change looks good, but I'd like to understand the other change for moving around the overlappingranges code. BTW we tested out this code and it does appear to improve performance. |
@adrianchiris could you split this change to another commit and explain its goal in the commit msg ?
Thank you @xagent003 . I really appreciate your help. |
Hey @maiqueb, ATM i have no bandwidth to work on this unfortunately. |
This is a POC level code for changing lease/leaderElection usage by whereabouts from a global lease to a per pool lease.
What this PR does / why we need it:
This PR is related to #313 and is used as additional context on the potential changes proposed.