CNS Pool Monitor - Init and basic resize logic #663
Conversation
Codecov Report
@@ Coverage Diff @@
## master #663 +/- ##
==========================================
- Coverage 42.06% 41.94% -0.12%
==========================================
Files 71 72 +1
Lines 10271 10354 +83
==========================================
+ Hits 4320 4343 +23
- Misses 5474 5537 +63
+ Partials 477 474 -3 |
cns/cnsclient/apiclient.go
Outdated
| // APIClient interface to update cns state | ||
| type APIClient interface { | ||
| ReconcileNCState(*cns.CreateNetworkContainerRequest, map[string]cns.KubernetesPodInfo) error | ||
| ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.KubernetesPodInfo, batchSize int64, requestThreshold, releaseThreshold float64) error |
There was a problem hiding this comment.
Can we pass all these in some struct, (say ScalerUnits), You also need to initialize the Spec (at least the UpdatedCount) durining init.
cns/cnsclient/apiclient.go
Outdated
| type APIClient interface { | ||
| ReconcileNCState(*cns.CreateNetworkContainerRequest, map[string]cns.KubernetesPodInfo) error | ||
| ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.KubernetesPodInfo, batchSize int64, requestThreshold, releaseThreshold float64) error | ||
| CreateOrUpdateNC(cns.CreateNetworkContainerRequest) error |
There was a problem hiding this comment.
We need to pass at least scaler units in this createorUpdateNC call as well, as the scaler units can change at runtime. You can add that later in a a different PR too if you want
cns/restserver/internalapi.go
Outdated
| func (service *HTTPRestService) StartCNSIPAMPoolMonitor(cnsService *HTTPRestService, requestController requestcontroller.RequestController) { | ||
|
|
||
| // TODO, start pool monitor as well | ||
| service.poolMonitor = NewCNSIPAMPoolMonitor(cnsService, requestController) |
There was a problem hiding this comment.
Do we need to pass cnsService? Is cnsService different from service.
cns/restserver/internalapi.go
Outdated
| } | ||
| } | ||
|
|
||
| service.poolMonitor.UpdatePoolLimitsTransacted(int(batchSize), requestThreshold, releaseThreshold) |
There was a problem hiding this comment.
Can you define a ScalerUnit struct and pass that instead as separate parameters
cns/restserver/ipampoolmonitor.go
Outdated
| @@ -0,0 +1,138 @@ | |||
| package restserver | |||
There was a problem hiding this comment.
Add // Copyright 2017 Microsoft. All rights reserved.
// MIT License
cns/restserver/ipampoolmonitor.go
Outdated
| decreaseIPCount := len(pm.cns.PodIPConfigState) - pm.batchSize | ||
|
|
||
| // mark n number of IP's as pending | ||
| pendingIPAddresses, err := pm.cns.MarkIPsAsPendingTransacted(decreaseIPCount) |
There was a problem hiding this comment.
Can you expose a method in internalapi.go than calling a TransactedAPI directly? If this poolMonitor is outside the RestServer package, then we cant call Transacted APIs directl. Also it will help to expose the same as debug API from client.
There was a problem hiding this comment.
it should be pm.cns.MarkIPsAsPendingTransacted(pm.batchsize)
| func (pm *CNSIPAMPoolMonitor) Start() error { | ||
|
|
||
| if pm.initialized { | ||
| availableIPConfigs := pm.cns.GetAvailableIPConfigs() |
There was a problem hiding this comment.
To be optmized later: We need a more optimized solution to find available IPs, then looping the range everytime. may be keep a counter.
cns/restserver/ipampoolmonitor.go
Outdated
| func (pm *CNSIPAMPoolMonitor) checkForResize(freeIPConfigCount int) int { | ||
| switch { | ||
| // pod count is increasing | ||
| case freeIPConfigCount < pm.minimumFreeIps: |
There was a problem hiding this comment.
You also need to account existing UpdatedCount, scenario we discussed in the meeting where SpecCount is already updated (so now freeIp = AvailableIPs + (If UpdatedSpecCount > TotalIPCount then (UpdatedSpec - TotalIpCount), else 0))
cns/restserver/ipampoolmonitor.go
Outdated
| func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { | ||
|
|
||
| // TODO: Better handling here, negatives? | ||
| decreaseIPCount := len(pm.cns.PodIPConfigState) - pm.batchSize |
There was a problem hiding this comment.
Also need to handle if there are some ips got allocated in the interim
There was a problem hiding this comment.
I dont think above logic is correct. say I have 40 Ips in PodIPConfigState, and available free ips is 20. Now 20 > 15 (maxFreeIps), decreaseIpCount = 40-10 = 30.
DecreaseIPCount is always 10. But you need to handle if some allocations happened in the interim.
cns/restserver/ipampoolmonitor.go
Outdated
| } | ||
|
|
||
| func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { | ||
| increaseIPCount := len(pm.cns.PodIPConfigState) + pm.batchSize |
There was a problem hiding this comment.
You also need to cache this increaseCount to account for next calculation
cns/restserver/internalapi.go
Outdated
| return | ||
| } | ||
|
|
||
| func (service *HTTPRestService) StartCNSIPAMPoolMonitor(cnsService *HTTPRestService, requestController requestcontroller.RequestController) { |
There was a problem hiding this comment.
Shouldnt cnsService be HTTPService?
cns/api.go
Outdated
| SyncNodeStatus(string, string, string, json.RawMessage) (int, string) | ||
| GetAvailableIPConfigs() []IPConfigurationStatus | ||
| GetPodIPConfigState() map[string]IPConfigurationStatus | ||
| MarkIPsAsPendingTransacted(numberToMark int) (map[string]SecondaryIPConfig, error) |
There was a problem hiding this comment.
Can we remove Transacted? All the APIs exposed from this interface are expected to be transacted.
matmerr
force-pushed
the
cnspoolmonitor
branch
from
319db2c to
cc1d65e
Compare
cns/fakes/cnsfake.go
Outdated
| return make(map[string]cns.IPConfigurationStatus) | ||
| } | ||
|
|
||
| func (fake *HTTPServiceFake) MarkIPsAsPendingTransacted(numberToMark int) (map[string]cns.SecondaryIPConfig, error) { |
There was a problem hiding this comment.
This needs to be updated too
* init CNS IPAM pool monitor * update tests * scalar struct, separate package, cns interface * addressing feedback
2 participants
Reason for Change:
Lay the groundwork for the monitor that will make calls to CNS and RequestController to adjust CNS pool size
Issue Fixed:
Requirements:
Notes: