fix ipampool scaling - only release IPs we have #1110
Conversation
rbtr
force-pushed
the
fix/pool-scaling
branch
from
b08a8fb to
4e5d92d
Compare
cns/ipampool/monitor.go
Outdated
| unallocatedIPConfigCount := cnsPodIPConfigCount - allocatedPodIPCount | ||
| freeIPConfigCount := requestedIPConfigCount - int64(allocatedPodIPCount) | ||
| currentFree := cnsPodIPConfigCount - allocatedPodIPCount | ||
| futureFree := requestedIPConfigCount - allocatedPodIPCount |
There was a problem hiding this comment.
Can we rename it something else, futureFree is little confusing specially when it is being used for ScaleUP. You will scaleup if you dont have ips left. Cant think of a better name too :)
May be "ipsToBeAssignedToCNS"...I'm sure you will come with a better name
There was a problem hiding this comment.
I actually can't think of a better name...can we move forward with this one and I will follow up with a change to clean up the way this state is gathered and passed around?
There was a problem hiding this comment.
How About "totalIpsToBeAssignedToCNS"
There was a problem hiding this comment.
I think we're using assigned and allocated backwards. It should be:
"IPs are allocated to CNS"
"CNS assigns IPs to Pods"
Then, the total number of IPs that CNS has are the its "allocated" IPs, and the total number if IPs in use are how many that CNS has "assigned" to specific Pods.
"currentFree" becomes unassigned := allocated - assigned
"futureFree" becomes something like requestedUnassigned := requested - assigned
There was a problem hiding this comment.
I like the above analogy.
However, since we have defined allocated => CNS assigned ips to pod
lets rename currentFree = unAllocated
FutureFree = requestedUnAllocated
IMO, FutureFree is really hard to explain to a new pair of eyes.
cns/ipampool/monitor.go
Outdated
| unallocatedIPConfigCount := cnsPodIPConfigCount - allocatedPodIPCount | ||
| freeIPConfigCount := requestedIPConfigCount - int64(allocatedPodIPCount) | ||
| currentFree := cnsPodIPConfigCount - allocatedPodIPCount | ||
| futureFree := requestedIPConfigCount - allocatedPodIPCount |
There was a problem hiding this comment.
How About "totalIpsToBeAssignedToCNS"
cns/ipampool/monitor.go
Outdated
| availableIPConfigCount := len(pm.httpService.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns | ||
| requestedIPConfigCount := pm.spec.RequestedIPCount | ||
| requestedIPConfigCount := int(pm.spec.RequestedIPCount) | ||
| unallocatedIPConfigCount := cnsPodIPConfigCount - allocatedPodIPCount |
There was a problem hiding this comment.
We can avoid this, it is same as CurrentFree
cns/ipampool/monitor.go
Outdated
| unallocatedIPConfigCount := cnsPodIPConfigCount - allocatedPodIPCount | ||
| freeIPConfigCount := requestedIPConfigCount - int64(allocatedPodIPCount) | ||
| currentFree := cnsPodIPConfigCount - allocatedPodIPCount | ||
| futureFree := requestedIPConfigCount - allocatedPodIPCount |
There was a problem hiding this comment.
I like the above analogy.
However, since we have defined allocated => CNS assigned ips to pod
lets rename currentFree = unAllocated
FutureFree = requestedUnAllocated
IMO, FutureFree is really hard to explain to a new pair of eyes.
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
rbtr
force-pushed
the
fix/pool-scaling
branch
from
4e5d92d to
5b9f77c
Compare
2 participants
Signed-off-by: Evan Baker rbtr@users.noreply.github.com
Reason for Change:
When scaling up, we should account for the IPs we have requested when we calculate our free. But when scaling down, we should only account for the IPs that we have when calculating free.
Issue Fixed:
Requirements:
Notes: