From 5d7ea79ce0e88449e51aaed7183f8d108ed729a4 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Mon, 31 Aug 2020 21:59:44 -0700 Subject: [PATCH 01/18] init with test framework --- cns/api.go | 4 +- cns/fakes/cnsfake.go | 162 +++++++++++++++++++- cns/fakes/requestcontrollerfake.go | 53 ++++++- cns/ipampoolmonitor/ipampoolmonitor.go | 43 ++++-- cns/ipampoolmonitor/ipampoolmonitor_test.go | 39 ++++- cns/restserver/internalapi.go | 8 +- 6 files changed, 281 insertions(+), 28 deletions(-) diff --git a/cns/api.go b/cns/api.go index 2b719eaf06..f8f757d561 100644 --- a/cns/api.go +++ b/cns/api.go @@ -36,6 +36,7 @@ type HTTPService interface { SetNodeOrchestrator(*SetOrchestratorTypeRequest) SyncNodeStatus(string, string, string, json.RawMessage) (int, string) GetAvailableIPConfigs() []IPConfigurationStatus + GetAllocatedIPConfigs() []IPConfigurationStatus GetPodIPConfigState() map[string]IPConfigurationStatus MarkIPsAsPending(numberToMark int) (map[string]SecondaryIPConfig, error) } @@ -163,13 +164,14 @@ type NodeConfiguration struct { type IPAMPoolMonitor interface { Start() error - UpdatePoolLimitsTransacted(ScalarUnits) + UpdatePoolLimitsTransacted(scalarUnits ScalarUnits) } type ScalarUnits struct { BatchSize int64 RequestThresholdPercent int64 ReleaseThresholdPercent int64 + IPConfigCount int } // Response describes generic response from CNS. diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 0686d66d2d..7bcfac2ed7 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -2,17 +2,144 @@ package fakes import ( "encoding/json" + "errors" + "net" + "sync" "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" ) +const ( + PrivateIPRangeClassA = "10.0.0.1/8" +) + +// available IP's stack +// all IP's map + +type StringStack struct { + lock sync.Mutex // you don't have to do this if you don't want thread safety + items []string +} + +func NewStack() *StringStack { + return &StringStack{sync.Mutex{}, make([]string, 0)} +} + +func (stack *StringStack) Push(v string) { + stack.lock.Lock() + defer stack.lock.Unlock() + + stack.items = append(stack.items, v) +} + +func (stack *StringStack) Pop() (string, error) { + stack.lock.Lock() + defer stack.lock.Unlock() + + length := len(stack.items) + if length == 0 { + return "", errors.New("Empty Stack") + } + + res := stack.items[length-1] + stack.items = stack.items[:length-1] + return res, nil +} + +func incrementIP(ip net.IP) { + for j := len(ip) - 1; j >= 0; j-- { + ip[j]++ + if ip[j] > 0 { + break + } + } +} + +type IPStateManager struct { + AvailableIPConfigState map[string]cns.IPConfigurationStatus + AllocatedIPConfigState map[string]cns.IPConfigurationStatus + PendingReleaseIPConfigState map[string]cns.IPConfigurationStatus + AvailableIPIDStack StringStack + sync.RWMutex +} + +func NewIPStateManager() IPStateManager { + return IPStateManager{ + AvailableIPConfigState: make(map[string]cns.IPConfigurationStatus), + AllocatedIPConfigState: make(map[string]cns.IPConfigurationStatus), + PendingReleaseIPConfigState: make(map[string]cns.IPConfigurationStatus), + AvailableIPIDStack: StringStack{}, + } +} + +func (ipm *IPStateManager) AddIPConfigs(ipconfigs []cns.IPConfigurationStatus) { + ipm.Lock() + defer ipm.Unlock() + + for i := 0; i < len(ipconfigs); i++ { + if ipconfigs[i].State == cns.Available { + ipm.AvailableIPConfigState[ipconfigs[i].ID] = ipconfigs[i] + ipm.AvailableIPIDStack.Push(ipconfigs[i].ID) + } else if ipconfigs[i].State == cns.Allocated { + ipm.AllocatedIPConfigState[ipconfigs[i].ID] = ipconfigs[i] + } else if ipconfigs[i].State == cns.PendingRelease { + ipm.PendingReleaseIPConfigState[ipconfigs[i].ID] = ipconfigs[i] + } + } + + return +} + +func (ipm *IPStateManager) ReserveIPConfig() (cns.IPConfigurationStatus, error) { + ipm.Lock() + defer ipm.Unlock() + id, err := ipm.AvailableIPIDStack.Pop() + if err != nil { + return cns.IPConfigurationStatus{}, err + } + ipm.AllocatedIPConfigState[id] = ipm.AvailableIPConfigState[id] + delete(ipm.AvailableIPConfigState, id) + return ipm.AllocatedIPConfigState[id], nil +} + +func (ipm *IPStateManager) ReleaseIPConfig(ipconfigID string) (cns.IPConfigurationStatus, error) { + ipm.Lock() + defer ipm.Unlock() + ipm.AvailableIPConfigState[ipconfigID] = ipm.AllocatedIPConfigState[ipconfigID] + ipm.AvailableIPIDStack.Push(ipconfigID) + delete(ipm.AllocatedIPConfigState, ipconfigID) + return ipm.AvailableIPConfigState[ipconfigID], nil +} + type HTTPServiceFake struct { - PoolMonitor cns.IPAMPoolMonitor + IPStateManager IPStateManager + PoolMonitor cns.IPAMPoolMonitor } func NewHTTPServiceFake() *HTTPServiceFake { - return &HTTPServiceFake{} + + svc := &HTTPServiceFake{ + IPStateManager: NewIPStateManager(), + } + + return svc +} + +func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int) error { + + currentAllocatedIPCount := len(fake.IPStateManager.AllocatedIPConfigState) + delta := (desiredAllocatedIPCount - currentAllocatedIPCount) + // need to free some IP's + for i := 0; i < delta; i++ { + _, err := fake.IPStateManager.ReserveIPConfig() + if err != nil { + return err + } + } + + // TODO decrease number of IP's + return nil } func (fake *HTTPServiceFake) SendNCSnapShotPeriodically(int, chan bool) { @@ -27,12 +154,39 @@ func (fake *HTTPServiceFake) SyncNodeStatus(string, string, string, json.RawMess return 0, "" } +// this is only returning a slice because of the interface +// TODO: return map instead func (fake *HTTPServiceFake) GetAvailableIPConfigs() []cns.IPConfigurationStatus { - return []cns.IPConfigurationStatus{} + ipconfigs := []cns.IPConfigurationStatus{} + for _, ipconfig := range fake.IPStateManager.AvailableIPConfigState { + ipconfigs = append(ipconfigs, ipconfig) + } + return ipconfigs +} + +// this is only returning a slice because of the interface +// TODO: return map instead +func (fake *HTTPServiceFake) GetAllocatedIPConfigs() []cns.IPConfigurationStatus { + ipconfigs := []cns.IPConfigurationStatus{} + for _, ipconfig := range fake.IPStateManager.AllocatedIPConfigState { + ipconfigs = append(ipconfigs, ipconfig) + } + return ipconfigs } +// TODO: return union of all state maps func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfigurationStatus { - return make(map[string]cns.IPConfigurationStatus) + ipconfigs := make(map[string]cns.IPConfigurationStatus) + for key, val := range fake.IPStateManager.AllocatedIPConfigState { + ipconfigs[key] = val + } + for key, val := range fake.IPStateManager.AvailableIPConfigState { + ipconfigs[key] = val + } + for key, val := range fake.IPStateManager.PendingReleaseIPConfigState { + ipconfigs[key] = val + } + return ipconfigs } func (fake *HTTPServiceFake) MarkIPsAsPending(numberToMark int) (map[string]cns.SecondaryIPConfig, error) { diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 936cd731f3..25037a7f57 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -2,21 +2,66 @@ package fakes import ( "context" + "fmt" + "net" + "github.com/Azure/azure-container-networking/cns" nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" + "github.com/google/uuid" +) + +const ( + PrivateIPRangeClassA = "10.0.0.1/8" +) + +var ( + ip net.IP ) type RequestControllerFake struct { + cnsService *HTTPServiceFake + scalarUnits cns.ScalarUnits + desiredState nnc.NodeNetworkConfigSpec +} + +func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { + + ip, _, _ = net.ParseCIDR(PrivateIPRangeClassA) + + ipconfigs := carveIPs(numberOfIPConfigs) + + cnsService.IPStateManager.AddIPConfigs(ipconfigs[0:numberOfIPConfigs]) + fmt.Println(cnsService.IPStateManager.AvailableIPConfigState) + + return &RequestControllerFake{ + cnsService: cnsService, + scalarUnits: scalarUnits, + } } -func NewRequestControllerFake() *RequestControllerFake { - return &RequestControllerFake{} +func carveIPs(ipCount int) []cns.IPConfigurationStatus { + var ipconfigs []cns.IPConfigurationStatus + for i := 0; i < ipCount; i++ { + ipconfig := cns.IPConfigurationStatus{ + ID: uuid.New().String(), + IPAddress: ip.String(), + State: cns.Available, + } + ipconfigs = append(ipconfigs, ipconfig) + incrementIP(ip) + } + return ipconfigs } -func (rc RequestControllerFake) StartRequestController(exitChan <-chan struct{}) error { +func (rc *RequestControllerFake) StartRequestController(exitChan <-chan struct{}) error { + return nil +} + +func (rc *RequestControllerFake) UpdateCRDSpec(cntxt context.Context, crdSpec nnc.NodeNetworkConfigSpec) error { + return nil } -func (rc RequestControllerFake) UpdateCRDSpec(cntxt context.Context, crdSpec nnc.NodeNetworkConfigSpec) error { +func (rc *RequestControllerFake) UpdateIPCountInCNS(cntxt context.Context, crdSpec nnc.NodeNetworkConfigSpec) error { return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 1e3b897efa..1fba5ec88f 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -19,12 +19,15 @@ var ( type CNSIPAMPoolMonitor struct { initialized bool + cachedSpec nnc.NodeNetworkConfigSpec cns cns.HTTPService rc requestcontroller.RequestController scalarUnits cns.ScalarUnits MinimumFreeIps int MaximumFreeIps int + goalIPCount int + sync.RWMutex } @@ -38,10 +41,18 @@ func NewCNSIPAMPoolMonitor(cnsService cns.HTTPService, requestController request // TODO: add looping and cancellation to this, and add to CNS MAIN func (pm *CNSIPAMPoolMonitor) Start() error { + // run Reconcile in a loop + return nil +} + +func (pm *CNSIPAMPoolMonitor) Reconcile() error { + // get pool size, and if the size is the same size as desired spec size, mark the spec as the current state + // if if pm.initialized { - availableIPConfigs := pm.cns.GetAvailableIPConfigs() - rebatchAction := pm.checkForResize(len(availableIPConfigs)) + //get number of allocated IP configs, and calculate free IP's against the cached spec + + rebatchAction := pm.checkForResize() switch rebatchAction { case increasePoolSize: return pm.increasePoolSize() @@ -59,14 +70,22 @@ func (pm *CNSIPAMPoolMonitor) UpdatePoolLimitsTransacted(scalarUnits cns.ScalarU defer pm.Unlock() pm.scalarUnits = scalarUnits + if !pm.initialized { + pm.goalIPCount = pm.scalarUnits.IPConfigCount + } + // TODO rounding? - pm.MinimumFreeIps = int(pm.scalarUnits.BatchSize * (pm.scalarUnits.RequestThresholdPercent / 100)) - pm.MaximumFreeIps = int(pm.scalarUnits.BatchSize * (pm.scalarUnits.ReleaseThresholdPercent / 100)) + pm.MinimumFreeIps = int(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) + pm.MaximumFreeIps = int(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) pm.initialized = true } -func (pm *CNSIPAMPoolMonitor) checkForResize(freeIPConfigCount int) int { +func (pm *CNSIPAMPoolMonitor) checkForResize() int { + + allocatedIPCount := len(pm.cns.GetAllocatedIPConfigs()) + freeIPConfigCount := pm.goalIPCount - allocatedIPCount + switch { // pod count is increasing case freeIPConfigCount < pm.MinimumFreeIps: @@ -82,22 +101,24 @@ func (pm *CNSIPAMPoolMonitor) checkForResize(freeIPConfigCount int) int { } func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { - increaseIPCount := len(pm.cns.GetPodIPConfigState()) + int(pm.scalarUnits.BatchSize) + var err error + pm.goalIPCount += int(pm.scalarUnits.BatchSize) // pass nil map to CNStoCRDSpec because we don't want to modify the to be deleted ipconfigs - spec, err := CNSToCRDSpec(nil, increaseIPCount) + pm.cachedSpec, err = CNSToCRDSpec(nil, pm.goalIPCount) if err != nil { return err } - return pm.rc.UpdateCRDSpec(context.Background(), spec) + return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { // TODO: Better handling here, negatives // TODO: Maintain desired state to check against if pool size adjustment is already happening - decreaseIPCount := len(pm.cns.GetPodIPConfigState()) - int(pm.scalarUnits.BatchSize) + decreaseIPCount := pm.goalIPCount - int(pm.scalarUnits.BatchSize) + pm.goalIPCount -= int(pm.scalarUnits.BatchSize) // mark n number of IP's as pending pendingIPAddresses, err := pm.cns.MarkIPsAsPending(decreaseIPCount) @@ -106,12 +127,12 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { } // convert the pending IP addresses to a spec - spec, err := CNSToCRDSpec(pendingIPAddresses, decreaseIPCount) + pm.cachedSpec, err = CNSToCRDSpec(pendingIPAddresses, pm.goalIPCount) if err != nil { return err } - return pm.rc.UpdateCRDSpec(context.Background(), spec) + return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } // CNSToCRDSpec translates CNS's map of Ips to be released and requested ip count into a CRD Spec diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 54f5847d7d..769ce73b03 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -5,15 +5,46 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/fakes" + "github.com/Azure/azure-container-networking/cns/logger" ) func TestInterfaces(t *testing.T) { + logger.InitLogger("testlogs", 0, 0, "./") + + scalarUnits := cns.ScalarUnits{ + BatchSize: 10, + IPConfigCount: 10, + RequestThresholdPercent: 30, + ReleaseThresholdPercent: 150, + } + + initialAvailableIPConfigCount := 10 + fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake() + fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, initialAvailableIPConfigCount) + poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc) + fakecns.PoolMonitor = poolmonitor + + poolmonitor.UpdatePoolLimitsTransacted(scalarUnits) + + t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) + t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) + + err := fakecns.SetNumberOfAllocatedIPs(8) + + poolmonitor.Reconcile() + + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err %v", err) + } + + poolmonitor.Reconcile() - fakecns.PoolMonitor = NewCNSIPAMPoolMonitor(fakecns, fakerc) + err = fakecns.SetNumberOfAllocatedIPs(9) + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err %v", err) + } - scalarUnits := cns.ScalarUnits{} + poolmonitor.Reconcile() - fakecns.PoolMonitor.UpdatePoolLimitsTransacted(scalarUnits) } diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index a15915e15e..b68068af9f 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -166,7 +166,7 @@ func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkCon return Success } - returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest, scalarUnits) + returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest) // If the NC was created successfully, then reconcile the allocated pod state if returnCode != Success { @@ -198,11 +198,13 @@ func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkCon } } + service.PoolMonitor.UpdatePoolLimitsTransacted(scalarUnits) + return 0 } // This API will be called by CNS RequestController on CRD update. -func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest, scalarUnits cns.ScalarUnits) int { +func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest) int { if req.NetworkContainerid == "" { logger.Errorf("[Azure CNS] Error. NetworkContainerid is empty") return NetworkContainerNotSpecified @@ -252,7 +254,5 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C logger.Errorf(returnMessage) } - service.PoolMonitor.UpdatePoolLimitsTransacted(scalarUnits) - return returnCode } From 259cdd6c1ef4627b732893f8b8c981fdabbbdf2a Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Tue, 1 Sep 2020 12:55:07 -0700 Subject: [PATCH 02/18] handle multiple batch size pool request --- cns/api.go | 10 +- cns/fakes/cnsfake.go | 24 ++-- cns/fakes/requestcontrollerfake.go | 30 +++-- cns/ipampoolmonitor/ipampoolmonitor.go | 23 ++-- cns/ipampoolmonitor/ipampoolmonitor_test.go | 107 +++++++++++++++--- .../kubecontroller/crdreconciler.go | 6 +- .../kubecontroller/crdrequestcontroller.go | 6 +- cns/restserver/internalapi.go | 8 +- cns/restserver/ipam.go | 5 + 9 files changed, 160 insertions(+), 59 deletions(-) diff --git a/cns/api.go b/cns/api.go index f8f757d561..a12d22ec28 100644 --- a/cns/api.go +++ b/cns/api.go @@ -37,6 +37,7 @@ type HTTPService interface { SyncNodeStatus(string, string, string, json.RawMessage) (int, string) GetAvailableIPConfigs() []IPConfigurationStatus GetAllocatedIPConfigs() []IPConfigurationStatus + GetPendingAllocationIPCount() int GetPodIPConfigState() map[string]IPConfigurationStatus MarkIPsAsPending(numberToMark int) (map[string]SecondaryIPConfig, error) } @@ -164,14 +165,13 @@ type NodeConfiguration struct { type IPAMPoolMonitor interface { Start() error - UpdatePoolLimitsTransacted(scalarUnits ScalarUnits) + UpdatePoolLimits(scalarUnits ScalarUnits) } type ScalarUnits struct { - BatchSize int64 - RequestThresholdPercent int64 - ReleaseThresholdPercent int64 - IPConfigCount int + BatchSize int + RequestThresholdPercent int + ReleaseThresholdPercent int } // Response describes generic response from CNS. diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 7bcfac2ed7..9a86aa5444 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -10,10 +10,6 @@ import ( "github.com/Azure/azure-container-networking/cns/common" ) -const ( - PrivateIPRangeClassA = "10.0.0.1/8" -) - // available IP's stack // all IP's map @@ -61,6 +57,7 @@ type IPStateManager struct { AllocatedIPConfigState map[string]cns.IPConfigurationStatus PendingReleaseIPConfigState map[string]cns.IPConfigurationStatus AvailableIPIDStack StringStack + PendingAllocationIPCount int sync.RWMutex } @@ -70,6 +67,7 @@ func NewIPStateManager() IPStateManager { AllocatedIPConfigState: make(map[string]cns.IPConfigurationStatus), PendingReleaseIPConfigState: make(map[string]cns.IPConfigurationStatus), AvailableIPIDStack: StringStack{}, + PendingAllocationIPCount: 0, } } @@ -81,6 +79,9 @@ func (ipm *IPStateManager) AddIPConfigs(ipconfigs []cns.IPConfigurationStatus) { if ipconfigs[i].State == cns.Available { ipm.AvailableIPConfigState[ipconfigs[i].ID] = ipconfigs[i] ipm.AvailableIPIDStack.Push(ipconfigs[i].ID) + if ipm.PendingAllocationIPCount > 0 { + ipm.PendingAllocationIPCount-- + } } else if ipconfigs[i].State == cns.Allocated { ipm.AllocatedIPConfigState[ipconfigs[i].ID] = ipconfigs[i] } else if ipconfigs[i].State == cns.PendingRelease { @@ -134,7 +135,7 @@ func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int for i := 0; i < delta; i++ { _, err := fake.IPStateManager.ReserveIPConfig() if err != nil { - return err + fake.IPStateManager.PendingAllocationIPCount++ } } @@ -154,8 +155,7 @@ func (fake *HTTPServiceFake) SyncNodeStatus(string, string, string, json.RawMess return 0, "" } -// this is only returning a slice because of the interface -// TODO: return map instead +// TODO: change real CNS return map instead func (fake *HTTPServiceFake) GetAvailableIPConfigs() []cns.IPConfigurationStatus { ipconfigs := []cns.IPConfigurationStatus{} for _, ipconfig := range fake.IPStateManager.AvailableIPConfigState { @@ -164,8 +164,7 @@ func (fake *HTTPServiceFake) GetAvailableIPConfigs() []cns.IPConfigurationStatus return ipconfigs } -// this is only returning a slice because of the interface -// TODO: return map instead +// TODO: change real CNS return map instead func (fake *HTTPServiceFake) GetAllocatedIPConfigs() []cns.IPConfigurationStatus { ipconfigs := []cns.IPConfigurationStatus{} for _, ipconfig := range fake.IPStateManager.AllocatedIPConfigState { @@ -174,7 +173,11 @@ func (fake *HTTPServiceFake) GetAllocatedIPConfigs() []cns.IPConfigurationStatus return ipconfigs } -// TODO: return union of all state maps +func (fake *HTTPServiceFake) GetPendingAllocationIPCount() int { + return fake.IPStateManager.PendingAllocationIPCount +} + +// Return union of all state maps func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfigurationStatus { ipconfigs := make(map[string]cns.IPConfigurationStatus) for key, val := range fake.IPStateManager.AllocatedIPConfigState { @@ -189,6 +192,7 @@ func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfiguratio return ipconfigs } +// TODO: Populate on scale down func (fake *HTTPServiceFake) MarkIPsAsPending(numberToMark int) (map[string]cns.SecondaryIPConfig, error) { return make(map[string]cns.SecondaryIPConfig), nil } diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 25037a7f57..43f11af8bc 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -2,7 +2,6 @@ package fakes import ( "context" - "fmt" "net" "github.com/Azure/azure-container-networking/cns" @@ -19,23 +18,21 @@ var ( ) type RequestControllerFake struct { - cnsService *HTTPServiceFake - scalarUnits cns.ScalarUnits - desiredState nnc.NodeNetworkConfigSpec + fakecns *HTTPServiceFake + testScalarUnits cns.ScalarUnits + desiredState nnc.NodeNetworkConfigSpec } func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { ip, _, _ = net.ParseCIDR(PrivateIPRangeClassA) - ipconfigs := carveIPs(numberOfIPConfigs) cnsService.IPStateManager.AddIPConfigs(ipconfigs[0:numberOfIPConfigs]) - fmt.Println(cnsService.IPStateManager.AvailableIPConfigState) return &RequestControllerFake{ - cnsService: cnsService, - scalarUnits: scalarUnits, + fakecns: cnsService, + testScalarUnits: scalarUnits, } } @@ -58,10 +55,23 @@ func (rc *RequestControllerFake) StartRequestController(exitChan <-chan struct{} } func (rc *RequestControllerFake) UpdateCRDSpec(cntxt context.Context, crdSpec nnc.NodeNetworkConfigSpec) error { - + rc.desiredState = crdSpec return nil } -func (rc *RequestControllerFake) UpdateIPCountInCNS(cntxt context.Context, crdSpec nnc.NodeNetworkConfigSpec) error { +func (rc *RequestControllerFake) Reconcile() error { + + rc.fakecns.GetPodIPConfigState() + diff := int(rc.desiredState.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState()) + + // carve the difference of test IPs + ipconfigs := carveIPs(diff) + + // add IPConfigs to CNS + rc.fakecns.IPStateManager.AddIPConfigs(ipconfigs) + + // update + rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) + return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 1fba5ec88f..8d65354c35 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -65,36 +65,37 @@ func (pm *CNSIPAMPoolMonitor) Reconcile() error { } // UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits -func (pm *CNSIPAMPoolMonitor) UpdatePoolLimitsTransacted(scalarUnits cns.ScalarUnits) { +func (pm *CNSIPAMPoolMonitor) UpdatePoolLimits(scalarUnits cns.ScalarUnits) { pm.Lock() defer pm.Unlock() pm.scalarUnits = scalarUnits - if !pm.initialized { - pm.goalIPCount = pm.scalarUnits.IPConfigCount - } - // TODO rounding? pm.MinimumFreeIps = int(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) pm.MaximumFreeIps = int(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) - pm.initialized = true + if !pm.initialized && len(pm.cns.GetPodIPConfigState()) > 0 { + pm.goalIPCount = len(pm.cns.GetPodIPConfigState()) + pm.initialized = true + } } func (pm *CNSIPAMPoolMonitor) checkForResize() int { - allocatedIPCount := len(pm.cns.GetAllocatedIPConfigs()) - freeIPConfigCount := pm.goalIPCount - allocatedIPCount + podIPCount := len(pm.cns.GetAllocatedIPConfigs()) + pm.cns.GetPendingAllocationIPCount() // TODO: add pending allocation count to real cns + freeIPConfigCount := pm.goalIPCount - podIPCount switch { // pod count is increasing + case podIPCount == 0: + logger.Printf("No pods scheduled") + return doNothing + case freeIPConfigCount < pm.MinimumFreeIps: - logger.Printf("Number of free IP's (%d) < minimum free IPs (%d), request batch increase\n", freeIPConfigCount, pm.MinimumFreeIps) return increasePoolSize // pod count is decreasing case freeIPConfigCount > pm.MaximumFreeIps: - logger.Printf("Number of free IP's (%d) > maximum free IPs (%d), request batch decrease\n", freeIPConfigCount, pm.MaximumFreeIps) return decreasePoolSize } return doNothing @@ -110,6 +111,7 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { return err } + logger.Printf("Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v, Pods waiting for IP's %v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, len(pm.cns.GetAllocatedIPConfigs()), pm.cns.GetPendingAllocationIPCount()) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } @@ -132,6 +134,7 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { return err } + logger.Printf("Decreasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, pm.cns.GetAllocatedIPConfigs()) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 769ce73b03..3a7e9f8488 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -8,43 +8,122 @@ import ( "github.com/Azure/azure-container-networking/cns/logger" ) -func TestInterfaces(t *testing.T) { +func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent int) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { logger.InitLogger("testlogs", 0, 0, "./") scalarUnits := cns.ScalarUnits{ - BatchSize: 10, - IPConfigCount: 10, - RequestThresholdPercent: 30, - ReleaseThresholdPercent: 150, + BatchSize: batchSize, + RequestThresholdPercent: requestThresholdPercent, + ReleaseThresholdPercent: releaseThresholdPercent, } - initialAvailableIPConfigCount := 10 - fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, initialAvailableIPConfigCount) + fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, initialIPConfigCount) poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc) fakecns.PoolMonitor = poolmonitor - poolmonitor.UpdatePoolLimitsTransacted(scalarUnits) + poolmonitor.UpdatePoolLimits(scalarUnits) + return fakecns, fakerc, poolmonitor +} + +func TestPoolSizeIncrease(t *testing.T) { + var ( + batchSize = 10 + initialIPConfigCount = 10 + requestThresholdPercent = 30 + releaseThresholdPercent = 150 + ) + + fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) - err := fakecns.SetNumberOfAllocatedIPs(8) - + // Effectively calling reconcile on start poolmonitor.Reconcile() + // increase number of allocated IP's in CNS + err := fakecns.SetNumberOfAllocatedIPs(8) if err != nil { - t.Fatalf("Failed to allocate test ipconfigs with err %v", err) + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } - poolmonitor.Reconcile() + // When poolmonitor reconcile is called, trigger increase and cache goal state + err = poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) + } + // increase number of allocated IP's in CNS, within allocatable size but still inside trigger threshold, err = fakecns.SetNumberOfAllocatedIPs(9) if err != nil { - t.Fatalf("Failed to allocate test ipconfigs with err %v", err) + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } + // poolmonitor reconciles, but doesn't actually update the CRD, because there is already a pending update + err = poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to reconcile pool monitor after allocation ip increase with err: %v", err) + } + + // request controller reconciles, carves new IP's from the test subnet and adds to CNS state + err = fakerc.Reconcile() + if err != nil { + t.Fatalf("Failed to reconcile fake requestcontroller with err: %v", err) + } + + // when poolmonitor reconciles again here, the IP count will be within the thresholds + // so no CRD update and nothing pending + err = poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to reconcile pool monitor after request controller updates CNS state: %v", err) + } +} + +func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T) { + var ( + batchSize = 10 + initialIPConfigCount = 10 + requestThresholdPercent = 30 + releaseThresholdPercent = 150 + ) + + fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + + t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) + t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) + + // Effectively calling reconcile on start + err := poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed reconcile pool on start %v", err) + } + + // increase number of allocated IP's in CNS, within allocatable size but still inside trigger threshold, + err = fakecns.SetNumberOfAllocatedIPs(8) + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) + } + + // When poolmonitor reconcile is called, trigger increase and cache target pool size poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) + } + + // increase number of allocated IP's in CNS, such that the new IP count won't fit in the pending update + err = fakecns.SetNumberOfAllocatedIPs(25) + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) + } + + // Request controller hasn't reconciled yet, but pool monitor needs to issue a second update to the CRD + // to fit the new IPConfigs + err = poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to issue second CRD update when total IP count exceeds the requested IP count: %v", err) + } + fakerc.Reconcile() + logger.Printf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.goalIPCount, len(fakecns.GetAllocatedIPConfigs())) } diff --git a/cns/requestcontroller/kubecontroller/crdreconciler.go b/cns/requestcontroller/kubecontroller/crdreconciler.go index eb2ab0e9f6..a0df7e24b3 100644 --- a/cns/requestcontroller/kubecontroller/crdreconciler.go +++ b/cns/requestcontroller/kubecontroller/crdreconciler.go @@ -56,9 +56,9 @@ func (r *CrdReconciler) Reconcile(request reconcile.Request) (reconcile.Result, } scalarUnits := cns.ScalarUnits{ - BatchSize: nodeNetConfig.Status.Scaler.BatchSize, - RequestThresholdPercent: nodeNetConfig.Status.Scaler.RequestThresholdPercent, - ReleaseThresholdPercent: nodeNetConfig.Status.Scaler.ReleaseThresholdPercent, + BatchSize: int(nodeNetConfig.Status.Scaler.BatchSize), + RequestThresholdPercent: int(nodeNetConfig.Status.Scaler.RequestThresholdPercent), + ReleaseThresholdPercent: int(nodeNetConfig.Status.Scaler.ReleaseThresholdPercent), } if err = r.CNSClient.CreateOrUpdateNC(ncRequest, scalarUnits); err != nil { diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go index ab17045983..3d419c7e3f 100644 --- a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go +++ b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go @@ -191,9 +191,9 @@ func (crdRC *crdRequestController) initCNS() error { } scalarUnits = cns.ScalarUnits{ - BatchSize: nodeNetConfig.Status.Scaler.BatchSize, - RequestThresholdPercent: nodeNetConfig.Status.Scaler.RequestThresholdPercent, - ReleaseThresholdPercent: nodeNetConfig.Status.Scaler.ReleaseThresholdPercent, + BatchSize: int(nodeNetConfig.Status.Scaler.BatchSize), + RequestThresholdPercent: int(nodeNetConfig.Status.Scaler.RequestThresholdPercent), + ReleaseThresholdPercent: int(nodeNetConfig.Status.Scaler.ReleaseThresholdPercent), } // If instance of crd is not found, pass nil to CNSClient diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index b68068af9f..7de8b33804 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -166,7 +166,7 @@ func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkCon return Success } - returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest) + returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest, scalarUnits) // If the NC was created successfully, then reconcile the allocated pod state if returnCode != Success { @@ -198,13 +198,11 @@ func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkCon } } - service.PoolMonitor.UpdatePoolLimitsTransacted(scalarUnits) - return 0 } // This API will be called by CNS RequestController on CRD update. -func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest) int { +func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest, scalarUnits cns.ScalarUnits) int { if req.NetworkContainerid == "" { logger.Errorf("[Azure CNS] Error. NetworkContainerid is empty") return NetworkContainerNotSpecified @@ -254,5 +252,7 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C logger.Errorf(returnMessage) } + service.PoolMonitor.UpdatePoolLimits(scalarUnits) + return returnCode } diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index c076bc439c..087def59a8 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -132,6 +132,11 @@ func (service *HTTPRestService) GetAvailableIPConfigs() []cns.IPConfigurationSta }) } +// TODO: implement this for pool resizing to know how many IP's have requested above the current pool size +func (service *HTTPRestService) GetPendingAllocationIPCount() int { + return 0 +} + func filterIPConfigMap(toBeAdded map[string]cns.IPConfigurationStatus, f func(cns.IPConfigurationStatus) bool) []cns.IPConfigurationStatus { vsf := make([]cns.IPConfigurationStatus, 0) for _, v := range toBeAdded { From c981fe91aef3ace8e2cdd2c1fe85bd17ca29e461 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Tue, 1 Sep 2020 14:10:28 -0700 Subject: [PATCH 03/18] test verification --- cns/fakes/cnsfake.go | 19 ++++++++--- cns/fakes/requestcontrollerfake.go | 2 ++ cns/ipampoolmonitor/ipampoolmonitor.go | 9 +++--- cns/ipampoolmonitor/ipampoolmonitor_test.go | 36 ++++++++++++++++++++- 4 files changed, 56 insertions(+), 10 deletions(-) diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 9a86aa5444..7aad4b4670 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -3,6 +3,7 @@ package fakes import ( "encoding/json" "errors" + "log" "net" "sync" @@ -79,9 +80,6 @@ func (ipm *IPStateManager) AddIPConfigs(ipconfigs []cns.IPConfigurationStatus) { if ipconfigs[i].State == cns.Available { ipm.AvailableIPConfigState[ipconfigs[i].ID] = ipconfigs[i] ipm.AvailableIPIDStack.Push(ipconfigs[i].ID) - if ipm.PendingAllocationIPCount > 0 { - ipm.PendingAllocationIPCount-- - } } else if ipconfigs[i].State == cns.Allocated { ipm.AllocatedIPConfigState[ipconfigs[i].ID] = ipconfigs[i] } else if ipconfigs[i].State == cns.PendingRelease { @@ -128,7 +126,7 @@ func NewHTTPServiceFake() *HTTPServiceFake { } func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int) error { - + log.Printf("[cns] Setting Allocated IPConfig count to %v", desiredAllocatedIPCount) currentAllocatedIPCount := len(fake.IPStateManager.AllocatedIPConfigState) delta := (desiredAllocatedIPCount - currentAllocatedIPCount) // need to free some IP's @@ -192,6 +190,19 @@ func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfiguratio return ipconfigs } +func (fake *HTTPServiceFake) AllocateTestIPConfigsToPendingPods() error { + for fake.IPStateManager.PendingAllocationIPCount > 0 { + _, err := fake.IPStateManager.ReserveIPConfig() + if err != nil { + return err + } + + fake.IPStateManager.PendingAllocationIPCount-- + } + + return nil +} + // TODO: Populate on scale down func (fake *HTTPServiceFake) MarkIPsAsPending(numberToMark int) (map[string]cns.SecondaryIPConfig, error) { return make(map[string]cns.SecondaryIPConfig), nil diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 43f11af8bc..1486fde450 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -2,6 +2,7 @@ package fakes import ( "context" + "log" "net" "github.com/Azure/azure-container-networking/cns" @@ -69,6 +70,7 @@ func (rc *RequestControllerFake) Reconcile() error { // add IPConfigs to CNS rc.fakecns.IPStateManager.AddIPConfigs(ipconfigs) + log.Printf("[fake-rc] Carved %v IP's to set total IPConfigs in CNS to %v", diff, len(rc.fakecns.GetPodIPConfigState())) // update rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 8d65354c35..f9cd3d959e 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -2,10 +2,10 @@ package ipampoolmonitor import ( "context" + "log" "sync" "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/requestcontroller" nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) @@ -47,7 +47,6 @@ func (pm *CNSIPAMPoolMonitor) Start() error { func (pm *CNSIPAMPoolMonitor) Reconcile() error { // get pool size, and if the size is the same size as desired spec size, mark the spec as the current state - // if if pm.initialized { //get number of allocated IP configs, and calculate free IP's against the cached spec @@ -88,7 +87,7 @@ func (pm *CNSIPAMPoolMonitor) checkForResize() int { switch { // pod count is increasing case podIPCount == 0: - logger.Printf("No pods scheduled") + log.Printf("[ipam-pool-monitor] No pods scheduled") return doNothing case freeIPConfigCount < pm.MinimumFreeIps: @@ -111,7 +110,7 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { return err } - logger.Printf("Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v, Pods waiting for IP's %v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, len(pm.cns.GetAllocatedIPConfigs()), pm.cns.GetPendingAllocationIPCount()) + log.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v, Pods waiting for IP's %v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, len(pm.cns.GetAllocatedIPConfigs()), pm.cns.GetPendingAllocationIPCount()) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } @@ -134,7 +133,7 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { return err } - logger.Printf("Decreasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, pm.cns.GetAllocatedIPConfigs()) + log.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, pm.cns.GetAllocatedIPConfigs()) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 3a7e9f8488..4ad6525303 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -1,6 +1,7 @@ package ipampoolmonitor import ( + "log" "testing" "github.com/Azure/azure-container-networking/cns" @@ -78,6 +79,18 @@ func TestPoolSizeIncrease(t *testing.T) { if err != nil { t.Fatalf("Failed to reconcile pool monitor after request controller updates CNS state: %v", err) } + + // make sure IPConfig state size reflects the new pool size + if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(1*batchSize) { + t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(2*batchSize)) + } + + // ensure pool monitor has reached quorum with cns + if poolmonitor.goalIPCount != initialIPConfigCount+(1*batchSize) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.goalIPCount, len(fakecns.GetPodIPConfigState())) + } + + log.Printf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.goalIPCount, len(fakecns.GetAllocatedIPConfigs())) } func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T) { @@ -124,6 +137,27 @@ func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T t.Fatalf("Failed to issue second CRD update when total IP count exceeds the requested IP count: %v", err) } + // request controller populates CNS state with new ipconfigs fakerc.Reconcile() - logger.Printf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.goalIPCount, len(fakecns.GetAllocatedIPConfigs())) + if err != nil { + t.Fatalf("Fake request controller failed to reconcile state with err: %v", err) + } + + // for test scenario, assign IP's to pods that previously were unable to get IPs before pool resize + err = fakecns.AllocateTestIPConfigsToPendingPods() + if err != nil { + t.Fatalf("Failed to assign ipconfigs to pending pods with err: %v", err) + } + + // make sure IPConfig state size reflects the new pool size + if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(2*batchSize) { + t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(2*batchSize)) + } + + // ensure pool monitor has reached quorum with cns + if poolmonitor.goalIPCount != initialIPConfigCount+(2*batchSize) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.goalIPCount, len(fakecns.GetPodIPConfigState())) + } + + log.Printf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.goalIPCount, len(fakecns.GetAllocatedIPConfigs())) } From 7bb99aa91eaad57d38f1cf38d1973bd5b7e4f7f2 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 2 Sep 2020 18:37:39 -0700 Subject: [PATCH 04/18] handle batch size multiples --- cns/api.go | 6 +- cns/fakes/cnsfake.go | 9 +- cns/fakes/requestcontrollerfake.go | 8 +- cns/ipampoolmonitor/ipampoolmonitor.go | 92 +++++++++---------- cns/ipampoolmonitor/ipampoolmonitor_test.go | 83 +++++++++++++---- .../kubecontroller/crdreconciler.go | 6 +- .../kubecontroller/crdrequestcontroller.go | 6 +- 7 files changed, 127 insertions(+), 83 deletions(-) diff --git a/cns/api.go b/cns/api.go index a12d22ec28..76fb82ea97 100644 --- a/cns/api.go +++ b/cns/api.go @@ -169,9 +169,9 @@ type IPAMPoolMonitor interface { } type ScalarUnits struct { - BatchSize int - RequestThresholdPercent int - ReleaseThresholdPercent int + BatchSize int64 + RequestThresholdPercent int64 + ReleaseThresholdPercent int64 } // Response describes generic response from CNS. diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 7aad4b4670..a3684c939e 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -3,9 +3,9 @@ package fakes import ( "encoding/json" "errors" - "log" "net" "sync" + "testing" "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" @@ -112,21 +112,22 @@ func (ipm *IPStateManager) ReleaseIPConfig(ipconfigID string) (cns.IPConfigurati } type HTTPServiceFake struct { + t *testing.T IPStateManager IPStateManager PoolMonitor cns.IPAMPoolMonitor } -func NewHTTPServiceFake() *HTTPServiceFake { - +func NewHTTPServiceFake(t *testing.T) *HTTPServiceFake { svc := &HTTPServiceFake{ IPStateManager: NewIPStateManager(), + t: t, } return svc } func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int) error { - log.Printf("[cns] Setting Allocated IPConfig count to %v", desiredAllocatedIPCount) + fake.t.Logf("[cns] Setting Allocated IPConfig count to %v", desiredAllocatedIPCount) currentAllocatedIPCount := len(fake.IPStateManager.AllocatedIPConfigState) delta := (desiredAllocatedIPCount - currentAllocatedIPCount) // need to free some IP's diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 1486fde450..e5589c181b 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -2,8 +2,8 @@ package fakes import ( "context" - "log" "net" + "testing" "github.com/Azure/azure-container-networking/cns" nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" @@ -19,12 +19,13 @@ var ( ) type RequestControllerFake struct { + t *testing.T fakecns *HTTPServiceFake testScalarUnits cns.ScalarUnits desiredState nnc.NodeNetworkConfigSpec } -func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { +func NewRequestControllerFake(t *testing.T, cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { ip, _, _ = net.ParseCIDR(PrivateIPRangeClassA) ipconfigs := carveIPs(numberOfIPConfigs) @@ -34,6 +35,7 @@ func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.Scala return &RequestControllerFake{ fakecns: cnsService, testScalarUnits: scalarUnits, + t: t, } } @@ -70,7 +72,7 @@ func (rc *RequestControllerFake) Reconcile() error { // add IPConfigs to CNS rc.fakecns.IPStateManager.AddIPConfigs(ipconfigs) - log.Printf("[fake-rc] Carved %v IP's to set total IPConfigs in CNS to %v", diff, len(rc.fakecns.GetPodIPConfigState())) + rc.t.Logf("[fake-rc] Carved %v IP's to set total IPConfigs in CNS to %v", diff, len(rc.fakecns.GetPodIPConfigState())) // update rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index f9cd3d959e..a586d6964b 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -3,6 +3,7 @@ package ipampoolmonitor import ( "context" "log" + "math" "sync" "github.com/Azure/azure-container-networking/cns" @@ -11,9 +12,9 @@ import ( ) var ( - increasePoolSize = 1 - decreasePoolSize = -1 - doNothing = 0 + increasePoolSize = int64(1) + decreasePoolSize = int64(-1) + doNothing = int64(0) ) type CNSIPAMPoolMonitor struct { @@ -23,10 +24,8 @@ type CNSIPAMPoolMonitor struct { cns cns.HTTPService rc requestcontroller.RequestController scalarUnits cns.ScalarUnits - MinimumFreeIps int - MaximumFreeIps int - - goalIPCount int + MinimumFreeIps int64 + MaximumFreeIps int64 sync.RWMutex } @@ -46,105 +45,87 @@ func (pm *CNSIPAMPoolMonitor) Start() error { } func (pm *CNSIPAMPoolMonitor) Reconcile() error { - // get pool size, and if the size is the same size as desired spec size, mark the spec as the current state - if pm.initialized { //get number of allocated IP configs, and calculate free IP's against the cached spec - - rebatchAction := pm.checkForResize() + rebatchAction, batchSizeMultiplier := pm.checkForResize() switch rebatchAction { case increasePoolSize: - return pm.increasePoolSize() + return pm.increasePoolSize(int64(batchSizeMultiplier)) case decreasePoolSize: - return pm.decreasePoolSize() + return pm.decreasePoolSize(int64(batchSizeMultiplier)) } } return nil } -// UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits -func (pm *CNSIPAMPoolMonitor) UpdatePoolLimits(scalarUnits cns.ScalarUnits) { - pm.Lock() - defer pm.Unlock() - pm.scalarUnits = scalarUnits - - // TODO rounding? - pm.MinimumFreeIps = int(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) - pm.MaximumFreeIps = int(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) - - if !pm.initialized && len(pm.cns.GetPodIPConfigState()) > 0 { - pm.goalIPCount = len(pm.cns.GetPodIPConfigState()) - pm.initialized = true - } -} - -func (pm *CNSIPAMPoolMonitor) checkForResize() int { - +func (pm *CNSIPAMPoolMonitor) checkForResize() (int64, float64) { podIPCount := len(pm.cns.GetAllocatedIPConfigs()) + pm.cns.GetPendingAllocationIPCount() // TODO: add pending allocation count to real cns - freeIPConfigCount := pm.goalIPCount - podIPCount + freeIPConfigCount := pm.cachedSpec.RequestedIPCount - int64(podIPCount) + + batchSizeMultiplier := math.Ceil(float64(podIPCount) / float64(pm.cachedSpec.RequestedIPCount)) switch { // pod count is increasing case podIPCount == 0: log.Printf("[ipam-pool-monitor] No pods scheduled") - return doNothing + return doNothing, batchSizeMultiplier case freeIPConfigCount < pm.MinimumFreeIps: - return increasePoolSize + return increasePoolSize, batchSizeMultiplier // pod count is decreasing case freeIPConfigCount > pm.MaximumFreeIps: - return decreasePoolSize + return decreasePoolSize, batchSizeMultiplier } - return doNothing + return doNothing, batchSizeMultiplier } -func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { +func (pm *CNSIPAMPoolMonitor) increasePoolSize(batchSizeMultiplier int64) error { var err error - pm.goalIPCount += int(pm.scalarUnits.BatchSize) + pm.cachedSpec.RequestedIPCount += pm.scalarUnits.BatchSize * batchSizeMultiplier // pass nil map to CNStoCRDSpec because we don't want to modify the to be deleted ipconfigs - pm.cachedSpec, err = CNSToCRDSpec(nil, pm.goalIPCount) + pm.cachedSpec, err = CNSToCRDSpec(nil, pm.cachedSpec.RequestedIPCount) if err != nil { return err } - log.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v, Pods waiting for IP's %v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, len(pm.cns.GetAllocatedIPConfigs()), pm.cns.GetPendingAllocationIPCount()) + log.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v, Pods waiting for IP's %v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs()), pm.cns.GetPendingAllocationIPCount()) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } -func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { +func (pm *CNSIPAMPoolMonitor) decreasePoolSize(batchSizeMultiplier int64) error { // TODO: Better handling here, negatives // TODO: Maintain desired state to check against if pool size adjustment is already happening - decreaseIPCount := pm.goalIPCount - int(pm.scalarUnits.BatchSize) - pm.goalIPCount -= int(pm.scalarUnits.BatchSize) + decreaseIPCount := pm.cachedSpec.RequestedIPCount - pm.scalarUnits.BatchSize + pm.cachedSpec.RequestedIPCount -= pm.scalarUnits.BatchSize * batchSizeMultiplier // mark n number of IP's as pending - pendingIPAddresses, err := pm.cns.MarkIPsAsPending(decreaseIPCount) + pendingIPAddresses, err := pm.cns.MarkIPsAsPending(int(decreaseIPCount)) if err != nil { return err } // convert the pending IP addresses to a spec - pm.cachedSpec, err = CNSToCRDSpec(pendingIPAddresses, pm.goalIPCount) + pm.cachedSpec, err = CNSToCRDSpec(pendingIPAddresses, pm.cachedSpec.RequestedIPCount) if err != nil { return err } - log.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.goalIPCount, pm.cns.GetAllocatedIPConfigs()) + log.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, pm.cns.GetAllocatedIPConfigs()) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } // CNSToCRDSpec translates CNS's map of Ips to be released and requested ip count into a CRD Spec -func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.SecondaryIPConfig, ipCount int) (nnc.NodeNetworkConfigSpec, error) { +func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.SecondaryIPConfig, ipCount int64) (nnc.NodeNetworkConfigSpec, error) { var ( spec nnc.NodeNetworkConfigSpec uuid string ) - spec.RequestedIPCount = int64(ipCount) + spec.RequestedIPCount = ipCount for uuid = range toBeDeletedSecondaryIPConfigs { spec.IPsNotInUse = append(spec.IPsNotInUse, uuid) @@ -152,3 +133,18 @@ func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.SecondaryIPConfig return spec, nil } + +// UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits +func (pm *CNSIPAMPoolMonitor) UpdatePoolLimits(scalarUnits cns.ScalarUnits) { + pm.Lock() + defer pm.Unlock() + pm.scalarUnits = scalarUnits + + pm.MinimumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) + pm.MaximumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) + + if !pm.initialized && len(pm.cns.GetPodIPConfigState()) > 0 { + pm.cachedSpec.RequestedIPCount = int64(len(pm.cns.GetPodIPConfigState())) + pm.initialized = true + } +} diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 4ad6525303..c4ef841f7a 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -1,7 +1,6 @@ package ipampoolmonitor import ( - "log" "testing" "github.com/Azure/azure-container-networking/cns" @@ -9,17 +8,17 @@ import ( "github.com/Azure/azure-container-networking/cns/logger" ) -func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent int) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { +func initFakes(t *testing.T, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent int) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { logger.InitLogger("testlogs", 0, 0, "./") scalarUnits := cns.ScalarUnits{ - BatchSize: batchSize, - RequestThresholdPercent: requestThresholdPercent, - ReleaseThresholdPercent: releaseThresholdPercent, + BatchSize: int64(batchSize), + RequestThresholdPercent: int64(requestThresholdPercent), + ReleaseThresholdPercent: int64(releaseThresholdPercent), } - fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, initialIPConfigCount) + fakecns := fakes.NewHTTPServiceFake(t) + fakerc := fakes.NewRequestControllerFake(t, fakecns, scalarUnits, initialIPConfigCount) poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc) fakecns.PoolMonitor = poolmonitor @@ -35,7 +34,7 @@ func TestPoolSizeIncrease(t *testing.T) { releaseThresholdPercent = 150 ) - fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + fakecns, fakerc, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -86,11 +85,11 @@ func TestPoolSizeIncrease(t *testing.T) { } // ensure pool monitor has reached quorum with cns - if poolmonitor.goalIPCount != initialIPConfigCount+(1*batchSize) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.goalIPCount, len(fakecns.GetPodIPConfigState())) + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } - log.Printf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.goalIPCount, len(fakecns.GetAllocatedIPConfigs())) + t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) } func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T) { @@ -101,7 +100,7 @@ func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T releaseThresholdPercent = 150 ) - fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + fakecns, fakerc, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -124,8 +123,9 @@ func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } - // increase number of allocated IP's in CNS, such that the new IP count won't fit in the pending update - err = fakecns.SetNumberOfAllocatedIPs(25) + // at this point pool size is 20, increase number of allocated IP's to 35, + // pool will need to be resized to 40, which is 2x the batch size + err = fakecns.SetNumberOfAllocatedIPs(35) if err != nil { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } @@ -150,14 +150,59 @@ func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T } // make sure IPConfig state size reflects the new pool size - if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(2*batchSize) { - t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(2*batchSize)) + if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(3*batchSize) { + t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(3*batchSize)) } // ensure pool monitor has reached quorum with cns - if poolmonitor.goalIPCount != initialIPConfigCount+(2*batchSize) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.goalIPCount, len(fakecns.GetPodIPConfigState())) + if int(poolmonitor.cachedSpec.RequestedIPCount) != initialIPConfigCount+(3*batchSize) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, initialIPConfigCount+(3*batchSize)) } - log.Printf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.goalIPCount, len(fakecns.GetAllocatedIPConfigs())) + t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedSpec.RequestedIPCount, initialIPConfigCount+(3*batchSize)) +} + +func TestPoolSizeIncreaseIdempotency(t *testing.T) { + var ( + batchSize = 10 + initialIPConfigCount = 10 + requestThresholdPercent = 30 + releaseThresholdPercent = 150 + ) + + fakecns, _, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + + t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) + t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) + + // Effectively calling reconcile on start + poolmonitor.Reconcile() + + // increase number of allocated IP's in CNS + err := fakecns.SetNumberOfAllocatedIPs(8) + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) + } + + // When poolmonitor reconcile is called, trigger increase and cache goal state + err = poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) + } + + // ensure pool monitor has increased batch size + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + } + + // reconcile pool monitor a second time, then verify requested ip count is still the same + err = poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) + } + + // ensure pool monitor requested pool size is unchanged as request controller hasn't reconciled yet + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + } } diff --git a/cns/requestcontroller/kubecontroller/crdreconciler.go b/cns/requestcontroller/kubecontroller/crdreconciler.go index a0df7e24b3..eb2ab0e9f6 100644 --- a/cns/requestcontroller/kubecontroller/crdreconciler.go +++ b/cns/requestcontroller/kubecontroller/crdreconciler.go @@ -56,9 +56,9 @@ func (r *CrdReconciler) Reconcile(request reconcile.Request) (reconcile.Result, } scalarUnits := cns.ScalarUnits{ - BatchSize: int(nodeNetConfig.Status.Scaler.BatchSize), - RequestThresholdPercent: int(nodeNetConfig.Status.Scaler.RequestThresholdPercent), - ReleaseThresholdPercent: int(nodeNetConfig.Status.Scaler.ReleaseThresholdPercent), + BatchSize: nodeNetConfig.Status.Scaler.BatchSize, + RequestThresholdPercent: nodeNetConfig.Status.Scaler.RequestThresholdPercent, + ReleaseThresholdPercent: nodeNetConfig.Status.Scaler.ReleaseThresholdPercent, } if err = r.CNSClient.CreateOrUpdateNC(ncRequest, scalarUnits); err != nil { diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go index 3d419c7e3f..ab17045983 100644 --- a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go +++ b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go @@ -191,9 +191,9 @@ func (crdRC *crdRequestController) initCNS() error { } scalarUnits = cns.ScalarUnits{ - BatchSize: int(nodeNetConfig.Status.Scaler.BatchSize), - RequestThresholdPercent: int(nodeNetConfig.Status.Scaler.RequestThresholdPercent), - ReleaseThresholdPercent: int(nodeNetConfig.Status.Scaler.ReleaseThresholdPercent), + BatchSize: nodeNetConfig.Status.Scaler.BatchSize, + RequestThresholdPercent: nodeNetConfig.Status.Scaler.RequestThresholdPercent, + ReleaseThresholdPercent: nodeNetConfig.Status.Scaler.ReleaseThresholdPercent, } // If instance of crd is not found, pass nil to CNSClient From df1832e0943c97dbe413367e3fd350de6bc0e86a Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Thu, 3 Sep 2020 12:37:09 -0700 Subject: [PATCH 05/18] address feedback --- cns/api.go | 2 +- cns/fakes/cnsfake.go | 25 ++---- cns/fakes/requestcontrollerfake.go | 27 +++---- cns/ipampoolmonitor/ipampoolmonitor.go | 42 +++++----- cns/ipampoolmonitor/ipampoolmonitor_test.go | 88 ++++++++++++--------- cns/restserver/ipam.go | 9 ++- 6 files changed, 100 insertions(+), 93 deletions(-) diff --git a/cns/api.go b/cns/api.go index 76fb82ea97..7aa70ade6a 100644 --- a/cns/api.go +++ b/cns/api.go @@ -37,7 +37,7 @@ type HTTPService interface { SyncNodeStatus(string, string, string, json.RawMessage) (int, string) GetAvailableIPConfigs() []IPConfigurationStatus GetAllocatedIPConfigs() []IPConfigurationStatus - GetPendingAllocationIPCount() int + GetPendingReleaseIPConfigs() []IPConfigurationStatus GetPodIPConfigState() map[string]IPConfigurationStatus MarkIPsAsPending(numberToMark int) (map[string]SecondaryIPConfig, error) } diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index a3684c939e..982048d0b0 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -58,7 +58,6 @@ type IPStateManager struct { AllocatedIPConfigState map[string]cns.IPConfigurationStatus PendingReleaseIPConfigState map[string]cns.IPConfigurationStatus AvailableIPIDStack StringStack - PendingAllocationIPCount int sync.RWMutex } @@ -68,7 +67,6 @@ func NewIPStateManager() IPStateManager { AllocatedIPConfigState: make(map[string]cns.IPConfigurationStatus), PendingReleaseIPConfigState: make(map[string]cns.IPConfigurationStatus), AvailableIPIDStack: StringStack{}, - PendingAllocationIPCount: 0, } } @@ -134,7 +132,7 @@ func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int for i := 0; i < delta; i++ { _, err := fake.IPStateManager.ReserveIPConfig() if err != nil { - fake.IPStateManager.PendingAllocationIPCount++ + return err } } @@ -172,8 +170,12 @@ func (fake *HTTPServiceFake) GetAllocatedIPConfigs() []cns.IPConfigurationStatus return ipconfigs } -func (fake *HTTPServiceFake) GetPendingAllocationIPCount() int { - return fake.IPStateManager.PendingAllocationIPCount +func (fake *HTTPServiceFake) GetPendingReleaseIPConfigs() []cns.IPConfigurationStatus { + ipconfigs := []cns.IPConfigurationStatus{} + for _, ipconfig := range fake.IPStateManager.PendingReleaseIPConfigState { + ipconfigs = append(ipconfigs, ipconfig) + } + return ipconfigs } // Return union of all state maps @@ -191,19 +193,6 @@ func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfiguratio return ipconfigs } -func (fake *HTTPServiceFake) AllocateTestIPConfigsToPendingPods() error { - for fake.IPStateManager.PendingAllocationIPCount > 0 { - _, err := fake.IPStateManager.ReserveIPConfig() - if err != nil { - return err - } - - fake.IPStateManager.PendingAllocationIPCount-- - } - - return nil -} - // TODO: Populate on scale down func (fake *HTTPServiceFake) MarkIPsAsPending(numberToMark int) (map[string]cns.SecondaryIPConfig, error) { return make(map[string]cns.SecondaryIPConfig), nil diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index e5589c181b..8cea93dae2 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -14,41 +14,40 @@ const ( PrivateIPRangeClassA = "10.0.0.1/8" ) -var ( - ip net.IP -) - type RequestControllerFake struct { t *testing.T fakecns *HTTPServiceFake testScalarUnits cns.ScalarUnits desiredState nnc.NodeNetworkConfigSpec + ip net.IP } func NewRequestControllerFake(t *testing.T, cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { - ip, _, _ = net.ParseCIDR(PrivateIPRangeClassA) - ipconfigs := carveIPs(numberOfIPConfigs) - - cnsService.IPStateManager.AddIPConfigs(ipconfigs[0:numberOfIPConfigs]) - - return &RequestControllerFake{ + rc := &RequestControllerFake{ fakecns: cnsService, testScalarUnits: scalarUnits, t: t, } + + rc.ip, _, _ = net.ParseCIDR(PrivateIPRangeClassA) + ipconfigs := rc.carveIPs(numberOfIPConfigs) + + cnsService.IPStateManager.AddIPConfigs(ipconfigs[0:numberOfIPConfigs]) + + return rc } -func carveIPs(ipCount int) []cns.IPConfigurationStatus { +func (rc *RequestControllerFake) carveIPs(ipCount int) []cns.IPConfigurationStatus { var ipconfigs []cns.IPConfigurationStatus for i := 0; i < ipCount; i++ { ipconfig := cns.IPConfigurationStatus{ ID: uuid.New().String(), - IPAddress: ip.String(), + IPAddress: rc.ip.String(), State: cns.Available, } ipconfigs = append(ipconfigs, ipconfig) - incrementIP(ip) + incrementIP(rc.ip) } return ipconfigs } @@ -68,7 +67,7 @@ func (rc *RequestControllerFake) Reconcile() error { diff := int(rc.desiredState.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState()) // carve the difference of test IPs - ipconfigs := carveIPs(diff) + ipconfigs := rc.carveIPs(diff) // add IPConfigs to CNS rc.fakecns.IPStateManager.AddIPConfigs(ipconfigs) diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index a586d6964b..60e1bb8e07 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -3,7 +3,6 @@ package ipampoolmonitor import ( "context" "log" - "math" "sync" "github.com/Azure/azure-container-networking/cns" @@ -47,43 +46,50 @@ func (pm *CNSIPAMPoolMonitor) Start() error { func (pm *CNSIPAMPoolMonitor) Reconcile() error { if pm.initialized { //get number of allocated IP configs, and calculate free IP's against the cached spec - rebatchAction, batchSizeMultiplier := pm.checkForResize() + rebatchAction := pm.checkForResize() switch rebatchAction { case increasePoolSize: - return pm.increasePoolSize(int64(batchSizeMultiplier)) + return pm.increasePoolSize() case decreasePoolSize: - return pm.decreasePoolSize(int64(batchSizeMultiplier)) + return pm.decreasePoolSize() } } return nil } -func (pm *CNSIPAMPoolMonitor) checkForResize() (int64, float64) { - podIPCount := len(pm.cns.GetAllocatedIPConfigs()) + pm.cns.GetPendingAllocationIPCount() // TODO: add pending allocation count to real cns - freeIPConfigCount := pm.cachedSpec.RequestedIPCount - int64(podIPCount) +func (pm *CNSIPAMPoolMonitor) checkForResize() int64 { - batchSizeMultiplier := math.Ceil(float64(podIPCount) / float64(pm.cachedSpec.RequestedIPCount)) + // if there's a pending change to the spec count, and the pending release state is nonzero, + // skip so we don't thrash the UpdateCRD + if pm.cachedSpec.RequestedIPCount != int64(len(pm.cns.GetPodIPConfigState())) && len(pm.cns.GetPendingReleaseIPConfigs()) > 0 { + return doNothing + } + + cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState()) + allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) + availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns + freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) switch { // pod count is increasing - case podIPCount == 0: + case allocatedPodIPCount == 0: log.Printf("[ipam-pool-monitor] No pods scheduled") - return doNothing, batchSizeMultiplier + return doNothing case freeIPConfigCount < pm.MinimumFreeIps: - return increasePoolSize, batchSizeMultiplier + return increasePoolSize // pod count is decreasing case freeIPConfigCount > pm.MaximumFreeIps: - return decreasePoolSize, batchSizeMultiplier + return decreasePoolSize } - return doNothing, batchSizeMultiplier + return doNothing } -func (pm *CNSIPAMPoolMonitor) increasePoolSize(batchSizeMultiplier int64) error { +func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { var err error - pm.cachedSpec.RequestedIPCount += pm.scalarUnits.BatchSize * batchSizeMultiplier + pm.cachedSpec.RequestedIPCount += pm.scalarUnits.BatchSize // pass nil map to CNStoCRDSpec because we don't want to modify the to be deleted ipconfigs pm.cachedSpec, err = CNSToCRDSpec(nil, pm.cachedSpec.RequestedIPCount) @@ -91,16 +97,16 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize(batchSizeMultiplier int64) error return err } - log.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v, Pods waiting for IP's %v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs()), pm.cns.GetPendingAllocationIPCount()) + log.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } -func (pm *CNSIPAMPoolMonitor) decreasePoolSize(batchSizeMultiplier int64) error { +func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { // TODO: Better handling here, negatives // TODO: Maintain desired state to check against if pool size adjustment is already happening decreaseIPCount := pm.cachedSpec.RequestedIPCount - pm.scalarUnits.BatchSize - pm.cachedSpec.RequestedIPCount -= pm.scalarUnits.BatchSize * batchSizeMultiplier + pm.cachedSpec.RequestedIPCount -= pm.scalarUnits.BatchSize // mark n number of IP's as pending pendingIPAddresses, err := pm.cns.MarkIPsAsPending(int(decreaseIPCount)) diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index c4ef841f7a..3f8d11ef59 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -40,10 +40,18 @@ func TestPoolSizeIncrease(t *testing.T) { t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) // Effectively calling reconcile on start - poolmonitor.Reconcile() + err := poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed to initialize poolmonitor on start with err: %v", err) + } + + // ensure pool monitor has reached quorum with cns + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + } // increase number of allocated IP's in CNS - err := fakecns.SetNumberOfAllocatedIPs(8) + err = fakecns.SetNumberOfAllocatedIPs(8) if err != nil { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } @@ -54,16 +62,9 @@ func TestPoolSizeIncrease(t *testing.T) { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } - // increase number of allocated IP's in CNS, within allocatable size but still inside trigger threshold, - err = fakecns.SetNumberOfAllocatedIPs(9) - if err != nil { - t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) - } - - // poolmonitor reconciles, but doesn't actually update the CRD, because there is already a pending update - err = poolmonitor.Reconcile() - if err != nil { - t.Fatalf("Failed to reconcile pool monitor after allocation ip increase with err: %v", err) + // ensure pool monitor has reached quorum with cns + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // request controller reconciles, carves new IP's from the test subnet and adds to CNS state @@ -79,20 +80,20 @@ func TestPoolSizeIncrease(t *testing.T) { t.Fatalf("Failed to reconcile pool monitor after request controller updates CNS state: %v", err) } - // make sure IPConfig state size reflects the new pool size - if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(1*batchSize) { - t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(2*batchSize)) - } - // ensure pool monitor has reached quorum with cns if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } + // make sure IPConfig state size reflects the new pool size + if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(1*batchSize) { + t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(1*batchSize)) + } + t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) } -func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T) { +func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { var ( batchSize = 10 initialIPConfigCount = 10 @@ -108,58 +109,67 @@ func TestPoolSizeIncreaseWhenAllocationCountExceedsRequestedIPCount(t *testing.T // Effectively calling reconcile on start err := poolmonitor.Reconcile() if err != nil { - t.Fatalf("Failed reconcile pool on start %v", err) + t.Fatalf("Failed to initialize poolmonitor on start with err: %v", err) } - // increase number of allocated IP's in CNS, within allocatable size but still inside trigger threshold, + // ensure pool monitor has reached quorum with cns + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + } + + // increase number of allocated IP's in CNS err = fakecns.SetNumberOfAllocatedIPs(8) if err != nil { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } - // When poolmonitor reconcile is called, trigger increase and cache target pool size - poolmonitor.Reconcile() + // When poolmonitor reconcile is called, trigger increase and cache goal state + err = poolmonitor.Reconcile() if err != nil { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } - // at this point pool size is 20, increase number of allocated IP's to 35, - // pool will need to be resized to 40, which is 2x the batch size - err = fakecns.SetNumberOfAllocatedIPs(35) + // increase number of allocated IP's in CNS, within allocatable size but still inside trigger threshold, + err = fakecns.SetNumberOfAllocatedIPs(9) if err != nil { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } - // Request controller hasn't reconciled yet, but pool monitor needs to issue a second update to the CRD - // to fit the new IPConfigs + // poolmonitor reconciles, but doesn't actually update the CRD, because there is already a pending update err = poolmonitor.Reconcile() if err != nil { - t.Fatalf("Failed to issue second CRD update when total IP count exceeds the requested IP count: %v", err) + t.Fatalf("Failed to reconcile pool monitor after allocation ip increase with err: %v", err) + } + + // ensure pool monitor has reached quorum with cns + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } - // request controller populates CNS state with new ipconfigs - fakerc.Reconcile() + // request controller reconciles, carves new IP's from the test subnet and adds to CNS state + err = fakerc.Reconcile() if err != nil { - t.Fatalf("Fake request controller failed to reconcile state with err: %v", err) + t.Fatalf("Failed to reconcile fake requestcontroller with err: %v", err) } - // for test scenario, assign IP's to pods that previously were unable to get IPs before pool resize - err = fakecns.AllocateTestIPConfigsToPendingPods() + // when poolmonitor reconciles again here, the IP count will be within the thresholds + // so no CRD update and nothing pending + err = poolmonitor.Reconcile() if err != nil { - t.Fatalf("Failed to assign ipconfigs to pending pods with err: %v", err) + t.Fatalf("Failed to reconcile pool monitor after request controller updates CNS state: %v", err) } // make sure IPConfig state size reflects the new pool size - if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(3*batchSize) { - t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(3*batchSize)) + if len(fakecns.GetPodIPConfigState()) != initialIPConfigCount+(1*batchSize) { + t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(1*batchSize)) } // ensure pool monitor has reached quorum with cns - if int(poolmonitor.cachedSpec.RequestedIPCount) != initialIPConfigCount+(3*batchSize) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, initialIPConfigCount+(3*batchSize)) + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } - t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedSpec.RequestedIPCount, initialIPConfigCount+(3*batchSize)) + t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) } func TestPoolSizeIncreaseIdempotency(t *testing.T) { diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 087def59a8..a2028a1217 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -132,9 +132,12 @@ func (service *HTTPRestService) GetAvailableIPConfigs() []cns.IPConfigurationSta }) } -// TODO: implement this for pool resizing to know how many IP's have requested above the current pool size -func (service *HTTPRestService) GetPendingAllocationIPCount() int { - return 0 +func (service *HTTPRestService) GetPendingReleaseIPConfigs() []cns.IPConfigurationStatus { + service.RLock() + defer service.RUnlock() + return filterIPConfigMap(service.PodIPConfigState, func(ipconfig cns.IPConfigurationStatus) bool { + return ipconfig.State == cns.PendingRelease + }) } func filterIPConfigMap(toBeAdded map[string]cns.IPConfigurationStatus, f func(cns.IPConfigurationStatus) bool) []cns.IPConfigurationStatus { From 85a935d168fb19fd5cda9b55e8df0e941e6e65bb Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Thu, 3 Sep 2020 19:57:11 -0700 Subject: [PATCH 06/18] fix test --- cns/cnsclient/cnsclient_test.go | 5 ++++- cns/fakes/cnsfake.go | 6 +----- cns/fakes/requestcontrollerfake.go | 4 +--- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/cns/cnsclient/cnsclient_test.go b/cns/cnsclient/cnsclient_test.go index e96a39565c..70b0049e86 100644 --- a/cns/cnsclient/cnsclient_test.go +++ b/cns/cnsclient/cnsclient_test.go @@ -123,7 +123,10 @@ func TestMain(m *testing.M) { httpRestService, err := restserver.NewHTTPRestService(&config, fakes.NewFakeImdsClient()) svc = httpRestService.(*restserver.HTTPRestService) svc.Name = "cns-test-server" - svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(nil, nil) + + fakecns := fakes.NewHTTPServiceFake() + fakerc := fakes.NewRequestControllerFake(fakecns, cns.ScalarUnits{BatchSize: 10, RequestThresholdPercent: 100, ReleaseThresholdPercent: 30}, 16) + svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(fakecns, fakerc) if err != nil { logger.Errorf("Failed to create CNS object, err:%v.\n", err) diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 982048d0b0..a1de1ecf50 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -5,7 +5,6 @@ import ( "errors" "net" "sync" - "testing" "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" @@ -110,22 +109,19 @@ func (ipm *IPStateManager) ReleaseIPConfig(ipconfigID string) (cns.IPConfigurati } type HTTPServiceFake struct { - t *testing.T IPStateManager IPStateManager PoolMonitor cns.IPAMPoolMonitor } -func NewHTTPServiceFake(t *testing.T) *HTTPServiceFake { +func NewHTTPServiceFake() *HTTPServiceFake { svc := &HTTPServiceFake{ IPStateManager: NewIPStateManager(), - t: t, } return svc } func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int) error { - fake.t.Logf("[cns] Setting Allocated IPConfig count to %v", desiredAllocatedIPCount) currentAllocatedIPCount := len(fake.IPStateManager.AllocatedIPConfigState) delta := (desiredAllocatedIPCount - currentAllocatedIPCount) // need to free some IP's diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 8cea93dae2..4f771438b1 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -22,12 +22,11 @@ type RequestControllerFake struct { ip net.IP } -func NewRequestControllerFake(t *testing.T, cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { +func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { rc := &RequestControllerFake{ fakecns: cnsService, testScalarUnits: scalarUnits, - t: t, } rc.ip, _, _ = net.ParseCIDR(PrivateIPRangeClassA) @@ -71,7 +70,6 @@ func (rc *RequestControllerFake) Reconcile() error { // add IPConfigs to CNS rc.fakecns.IPStateManager.AddIPConfigs(ipconfigs) - rc.t.Logf("[fake-rc] Carved %v IP's to set total IPConfigs in CNS to %v", diff, len(rc.fakecns.GetPodIPConfigState())) // update rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) From 3d41f38beecd4de7f6eebadaddbbf6904681337e Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Thu, 3 Sep 2020 20:03:51 -0700 Subject: [PATCH 07/18] remove test struct from fakes --- cns/fakes/requestcontrollerfake.go | 2 -- cns/ipampoolmonitor/ipampoolmonitor_test.go | 12 ++++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 4f771438b1..06fa9ebc0f 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -3,7 +3,6 @@ package fakes import ( "context" "net" - "testing" "github.com/Azure/azure-container-networking/cns" nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" @@ -15,7 +14,6 @@ const ( ) type RequestControllerFake struct { - t *testing.T fakecns *HTTPServiceFake testScalarUnits cns.ScalarUnits desiredState nnc.NodeNetworkConfigSpec diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 3f8d11ef59..0db4a3c4ff 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -8,7 +8,7 @@ import ( "github.com/Azure/azure-container-networking/cns/logger" ) -func initFakes(t *testing.T, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent int) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { +func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent int) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { logger.InitLogger("testlogs", 0, 0, "./") scalarUnits := cns.ScalarUnits{ @@ -17,8 +17,8 @@ func initFakes(t *testing.T, batchSize, initialIPConfigCount, requestThresholdPe ReleaseThresholdPercent: int64(releaseThresholdPercent), } - fakecns := fakes.NewHTTPServiceFake(t) - fakerc := fakes.NewRequestControllerFake(t, fakecns, scalarUnits, initialIPConfigCount) + fakecns := fakes.NewHTTPServiceFake() + fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, initialIPConfigCount) poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc) fakecns.PoolMonitor = poolmonitor @@ -34,7 +34,7 @@ func TestPoolSizeIncrease(t *testing.T) { releaseThresholdPercent = 150 ) - fakecns, fakerc, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -101,7 +101,7 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { releaseThresholdPercent = 150 ) - fakecns, fakerc, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) @@ -180,7 +180,7 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { releaseThresholdPercent = 150 ) - fakecns, _, poolmonitor := initFakes(t, batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + fakecns, _, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) From c54449ea1a0d9100879e23287eb6c7a95e7d0efa Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Thu, 3 Sep 2020 20:35:24 -0700 Subject: [PATCH 08/18] fix nil test --- cns/restserver/api_test.go | 5 +++++ cns/restserver/internalapi_test.go | 1 - 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 875d32d1ee..74bb501036 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -18,6 +18,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" "github.com/Azure/azure-container-networking/cns/fakes" + "github.com/Azure/azure-container-networking/cns/ipampoolmonitor" "github.com/Azure/azure-container-networking/cns/logger" acncommon "github.com/Azure/azure-container-networking/common" ) @@ -677,6 +678,10 @@ func startService() { } } + fakecns := fakes.NewHTTPServiceFake() + fakerc := fakes.NewRequestControllerFake(fakecns, cns.ScalarUnits{BatchSize: 10, RequestThresholdPercent: 30, ReleaseThresholdPercent: 100}, 10) + service.(*HTTPRestService).PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(fakecns, fakerc) + // Get the internal http mux as test hook. mux = service.(*HTTPRestService).Listener.GetMux() } diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index 25dc1d77c8..d772ed119f 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -189,7 +189,6 @@ func validateCreateOrUpdateNCInternal(t *testing.T, secondaryIpCount int) { func createAndValidateNCRequest(t *testing.T, secondaryIPConfigs map[string]cns.SecondaryIPConfig, ncId string) { req := generateNetworkContainerRequest(secondaryIPConfigs, ncId) var scalarUnits cns.ScalarUnits - svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(nil, nil) returnCode := svc.CreateOrUpdateNetworkContainerInternal(req, scalarUnits) if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) From d1bd3be29a8834885d4813c868189b3dbd22dae5 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Thu, 3 Sep 2020 20:48:18 -0700 Subject: [PATCH 09/18] fix tests --- cns/api.go | 2 +- cns/fakes/requestcontrollerfake.go | 5 +---- cns/ipampoolmonitor/ipampoolmonitor.go | 9 ++++++++- cns/restserver/internalapi.go | 6 +++++- cns/restserver/internalapi_test.go | 3 --- cns/restserver/ipam_test.go | 6 ++++++ 6 files changed, 21 insertions(+), 10 deletions(-) diff --git a/cns/api.go b/cns/api.go index 7aa70ade6a..3b48b763b0 100644 --- a/cns/api.go +++ b/cns/api.go @@ -165,7 +165,7 @@ type NodeConfiguration struct { type IPAMPoolMonitor interface { Start() error - UpdatePoolLimits(scalarUnits ScalarUnits) + UpdatePoolLimits(scalarUnits ScalarUnits) error } type ScalarUnits struct { diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 06fa9ebc0f..03b2f2be3a 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -69,8 +69,5 @@ func (rc *RequestControllerFake) Reconcile() error { // add IPConfigs to CNS rc.fakecns.IPStateManager.AddIPConfigs(ipconfigs) - // update - rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) - - return nil + return rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 60e1bb8e07..2af7f6ff62 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -2,6 +2,7 @@ package ipampoolmonitor import ( "context" + "fmt" "log" "sync" @@ -141,7 +142,7 @@ func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.SecondaryIPConfig } // UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits -func (pm *CNSIPAMPoolMonitor) UpdatePoolLimits(scalarUnits cns.ScalarUnits) { +func (pm *CNSIPAMPoolMonitor) UpdatePoolLimits(scalarUnits cns.ScalarUnits) error { pm.Lock() defer pm.Unlock() pm.scalarUnits = scalarUnits @@ -149,8 +150,14 @@ func (pm *CNSIPAMPoolMonitor) UpdatePoolLimits(scalarUnits cns.ScalarUnits) { pm.MinimumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) pm.MaximumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) + if pm.cns == nil { + return fmt.Errorf("Error Updating Pool Limits, reference to CNS is nil") + } + if !pm.initialized && len(pm.cns.GetPodIPConfigState()) > 0 { pm.cachedSpec.RequestedIPCount = int64(len(pm.cns.GetPodIPConfigState())) pm.initialized = true } + + return nil } diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index 7de8b33804..6c88019894 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -252,7 +252,11 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C logger.Errorf(returnMessage) } - service.PoolMonitor.UpdatePoolLimits(scalarUnits) + err = service.PoolMonitor.UpdatePoolLimits(scalarUnits) + if err != nil { + logger.Errorf("[Azure CNS] Error. Reference to CNS not set in IPAM Pool Monitor: %v", req) + return UnexpectedError + } return returnCode } diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index d772ed119f..e565c1ed60 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -11,7 +11,6 @@ import ( "testing" "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/ipampoolmonitor" "github.com/google/uuid" ) @@ -54,7 +53,6 @@ func TestReconcileNCWithEmptyState(t *testing.T) { func TestReconcileNCWithExistingState(t *testing.T) { restartService() var scalarUnits cns.ScalarUnits - svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(nil, nil) setEnv(t) setOrchestratorTypeInternal(cns.KubernetesCRD) @@ -92,7 +90,6 @@ func TestReconcileNCWithExistingState(t *testing.T) { func TestReconcileNCWithSystemPods(t *testing.T) { restartService() - svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(nil, nil) var scalarUnits cns.ScalarUnits setEnv(t) setOrchestratorTypeInternal(cns.KubernetesCRD) diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 3418a65e48..7b7e5bb7f9 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -12,6 +12,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" "github.com/Azure/azure-container-networking/cns/fakes" + "github.com/Azure/azure-container-networking/cns/ipampoolmonitor" ) var ( @@ -43,6 +44,11 @@ func getTestService() *HTTPRestService { var config common.ServiceConfig httpsvc, _ := NewHTTPRestService(&config, fakes.NewFakeImdsClient()) svc = httpsvc.(*HTTPRestService) + + fakecns := fakes.NewHTTPServiceFake() + fakerc := fakes.NewRequestControllerFake(fakecns, cns.ScalarUnits{BatchSize: 10, RequestThresholdPercent: 30, ReleaseThresholdPercent: 150}, 10) + svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(fakecns, fakerc) + setOrchestratorTypeInternal(cns.KubernetesCRD) return svc From bd72b570fbd7410a32307fbe493632d3cc75633d Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Tue, 8 Sep 2020 17:23:32 -0700 Subject: [PATCH 10/18] feedback --- cns/fakes/cnsfake.go | 37 +++++++++++------------------- cns/fakes/requestcontrollerfake.go | 9 ++++++++ 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index a1de1ecf50..5570ec0113 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -3,7 +3,6 @@ package fakes import ( "encoding/json" "errors" - "net" "sync" "github.com/Azure/azure-container-networking/cns" @@ -43,15 +42,6 @@ func (stack *StringStack) Pop() (string, error) { return res, nil } -func incrementIP(ip net.IP) { - for j := len(ip) - 1; j >= 0; j-- { - ip[j]++ - if ip[j] > 0 { - break - } - } -} - type IPStateManager struct { AvailableIPConfigState map[string]cns.IPConfigurationStatus AllocatedIPConfigState map[string]cns.IPConfigurationStatus @@ -74,12 +64,14 @@ func (ipm *IPStateManager) AddIPConfigs(ipconfigs []cns.IPConfigurationStatus) { defer ipm.Unlock() for i := 0; i < len(ipconfigs); i++ { - if ipconfigs[i].State == cns.Available { + + switch { + case ipconfigs[i].State == cns.Available: ipm.AvailableIPConfigState[ipconfigs[i].ID] = ipconfigs[i] ipm.AvailableIPIDStack.Push(ipconfigs[i].ID) - } else if ipconfigs[i].State == cns.Allocated { + case ipconfigs[i].State == cns.Allocated: ipm.AllocatedIPConfigState[ipconfigs[i].ID] = ipconfigs[i] - } else if ipconfigs[i].State == cns.PendingRelease { + case ipconfigs[i].State == cns.PendingRelease: ipm.PendingReleaseIPConfigState[ipconfigs[i].ID] = ipconfigs[i] } } @@ -124,7 +116,7 @@ func NewHTTPServiceFake() *HTTPServiceFake { func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int) error { currentAllocatedIPCount := len(fake.IPStateManager.AllocatedIPConfigState) delta := (desiredAllocatedIPCount - currentAllocatedIPCount) - // need to free some IP's + // reserver IP configs for i := 0; i < delta; i++ { _, err := fake.IPStateManager.ReserveIPConfig() if err != nil { @@ -132,7 +124,6 @@ func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int } } - // TODO decrease number of IP's return nil } @@ -148,21 +139,21 @@ func (fake *HTTPServiceFake) SyncNodeStatus(string, string, string, json.RawMess return 0, "" } -// TODO: change real CNS return map instead func (fake *HTTPServiceFake) GetAvailableIPConfigs() []cns.IPConfigurationStatus { ipconfigs := []cns.IPConfigurationStatus{} for _, ipconfig := range fake.IPStateManager.AvailableIPConfigState { ipconfigs = append(ipconfigs, ipconfig) } + return ipconfigs } -// TODO: change real CNS return map instead func (fake *HTTPServiceFake) GetAllocatedIPConfigs() []cns.IPConfigurationStatus { ipconfigs := []cns.IPConfigurationStatus{} for _, ipconfig := range fake.IPStateManager.AllocatedIPConfigState { ipconfigs = append(ipconfigs, ipconfig) } + return ipconfigs } @@ -171,6 +162,7 @@ func (fake *HTTPServiceFake) GetPendingReleaseIPConfigs() []cns.IPConfigurationS for _, ipconfig := range fake.IPStateManager.PendingReleaseIPConfigState { ipconfigs = append(ipconfigs, ipconfig) } + return ipconfigs } @@ -180,12 +172,15 @@ func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfiguratio for key, val := range fake.IPStateManager.AllocatedIPConfigState { ipconfigs[key] = val } + for key, val := range fake.IPStateManager.AvailableIPConfigState { ipconfigs[key] = val } + for key, val := range fake.IPStateManager.PendingReleaseIPConfigState { ipconfigs[key] = val } + return ipconfigs } @@ -198,14 +193,10 @@ func (fake *HTTPServiceFake) GetOption(string) interface{} { return nil } -func (fake *HTTPServiceFake) SetOption(string, interface{}) { - -} +func (fake *HTTPServiceFake) SetOption(string, interface{}) {} func (fake *HTTPServiceFake) Start(*common.ServiceConfig) error { return nil } -func (fake *HTTPServiceFake) Stop() { - -} +func (fake *HTTPServiceFake) Stop() {} diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 03b2f2be3a..450175efe0 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -71,3 +71,12 @@ func (rc *RequestControllerFake) Reconcile() error { return rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) } + +func incrementIP(ip net.IP) { + for j := len(ip) - 1; j >= 0; j-- { + ip[j]++ + if ip[j] > 0 { + break + } + } +} From e3d17195b1db60bc37777679a2e7dde30cd34a05 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 2 Sep 2020 11:08:18 -0700 Subject: [PATCH 11/18] scale down --- cns/api.go | 2 +- cns/fakes/cnsfake.go | 81 ++++++++++- cns/fakes/requestcontrollerfake.go | 100 ++++++++++---- cns/ipampoolmonitor/ipampoolmonitor.go | 21 ++- cns/ipampoolmonitor/ipampoolmonitor_test.go | 142 +++++++++++++++++++- cns/restserver/ipam.go | 8 +- 6 files changed, 302 insertions(+), 52 deletions(-) diff --git a/cns/api.go b/cns/api.go index 3b48b763b0..a7a8458930 100644 --- a/cns/api.go +++ b/cns/api.go @@ -39,7 +39,7 @@ type HTTPService interface { GetAllocatedIPConfigs() []IPConfigurationStatus GetPendingReleaseIPConfigs() []IPConfigurationStatus GetPodIPConfigState() map[string]IPConfigurationStatus - MarkIPsAsPending(numberToMark int) (map[string]SecondaryIPConfig, error) + MarkIPsAsPending(numberToMark int) (map[string]IPConfigurationStatus, error) } // This is used for KubernetesCRD orchastrator Type where NC has multiple ips. diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 5570ec0113..367c52660f 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -79,6 +79,17 @@ func (ipm *IPStateManager) AddIPConfigs(ipconfigs []cns.IPConfigurationStatus) { return } +func (ipm *IPStateManager) RemovePendingReleaseIPConfigs(ipconfigNames []string) { + ipm.Lock() + defer ipm.Unlock() + + for i := 0; i < len(ipconfigNames); i++ { + delete(ipm.PendingReleaseIPConfigState, ipconfigNames[i]) + } + + return +} + func (ipm *IPStateManager) ReserveIPConfig() (cns.IPConfigurationStatus, error) { ipm.Lock() defer ipm.Unlock() @@ -100,6 +111,45 @@ func (ipm *IPStateManager) ReleaseIPConfig(ipconfigID string) (cns.IPConfigurati return ipm.AvailableIPConfigState[ipconfigID], nil } +func (ipm *IPStateManager) MarkIPsAsPending(numberOfIPsToMark int) (map[string]cns.IPConfigurationStatus, error) { + ipm.Lock() + defer ipm.Unlock() + + var ( + err error + pendingRelease []cns.IPConfigurationStatus + ) + + defer func() { + // if there was an error, and not all ip's have been freed, restore state + if err != nil && len(pendingRelease) != numberOfIPsToMark { + for i := range pendingRelease { + ipm.AvailableIPIDStack.Push(pendingRelease[i].ID) + ipm.AvailableIPConfigState[pendingRelease[i].ID] = pendingRelease[i] + delete(ipm.PendingReleaseIPConfigState, pendingRelease[i].ID) + } + } + }() + + for i := 0; i < numberOfIPsToMark; i++ { + id, err := ipm.AvailableIPIDStack.Pop() + if err != nil { + return ipm.PendingReleaseIPConfigState, err + } + + // add all pending release to a slice + pendingRelease = append(pendingRelease, ipm.AvailableIPConfigState[id]) + delete(ipm.AvailableIPConfigState, id) + } + + // if no errors at this point, add the pending ips to the Pending state + for i := range pendingRelease { + ipm.PendingReleaseIPConfigState[pendingRelease[i].ID] = pendingRelease[i] + } + + return ipm.PendingReleaseIPConfigState, nil +} + type HTTPServiceFake struct { IPStateManager IPStateManager PoolMonitor cns.IPAMPoolMonitor @@ -116,11 +166,28 @@ func NewHTTPServiceFake() *HTTPServiceFake { func (fake *HTTPServiceFake) SetNumberOfAllocatedIPs(desiredAllocatedIPCount int) error { currentAllocatedIPCount := len(fake.IPStateManager.AllocatedIPConfigState) delta := (desiredAllocatedIPCount - currentAllocatedIPCount) - // reserver IP configs - for i := 0; i < delta; i++ { - _, err := fake.IPStateManager.ReserveIPConfig() - if err != nil { - return err + + if delta > 0 { + for i := 0; i < delta; i++ { + if _, err := fake.IPStateManager.ReserveIPConfig(); err != nil { + return err + } + } + } else if delta < 0 { + + // deallocate IP's + delta *= -1 + i := 0 + for id := range fake.IPStateManager.AllocatedIPConfigState { + if i < delta { + if _, err := fake.IPStateManager.ReleaseIPConfig(id); err != nil { + return err + } + + } else { + break + } + i++ } } @@ -185,8 +252,8 @@ func (fake *HTTPServiceFake) GetPodIPConfigState() map[string]cns.IPConfiguratio } // TODO: Populate on scale down -func (fake *HTTPServiceFake) MarkIPsAsPending(numberToMark int) (map[string]cns.SecondaryIPConfig, error) { - return make(map[string]cns.SecondaryIPConfig), nil +func (fake *HTTPServiceFake) MarkIPsAsPending(numberToMark int) (map[string]cns.IPConfigurationStatus, error) { + return fake.IPStateManager.MarkIPsAsPending(numberToMark) } func (fake *HTTPServiceFake) GetOption(string) interface{} { diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 450175efe0..d113e22c7b 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -9,14 +9,11 @@ import ( "github.com/google/uuid" ) -const ( - PrivateIPRangeClassA = "10.0.0.1/8" -) - type RequestControllerFake struct { fakecns *HTTPServiceFake testScalarUnits cns.ScalarUnits - desiredState nnc.NodeNetworkConfigSpec + desiredSpec nnc.NodeNetworkConfigSpec + currentStatus nnc.NodeNetworkConfigStatus ip net.IP } @@ -25,51 +22,102 @@ func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.Scala rc := &RequestControllerFake{ fakecns: cnsService, testScalarUnits: scalarUnits, + currentStatus: nnc.NodeNetworkConfigStatus{Scaler: nnc.Scaler{ + BatchSize: scalarUnits.BatchSize, + RequestThresholdPercent: scalarUnits.RequestThresholdPercent, + ReleaseThresholdPercent: scalarUnits.ReleaseThresholdPercent, + }, + NetworkContainers: []nnc.NetworkContainer{ + nnc.NetworkContainer{ + IPAssignments: []nnc.IPAssignment{}, + SubnetAddressSpace: "10.0.0.0/8", + }, + }, + }, } - rc.ip, _, _ = net.ParseCIDR(PrivateIPRangeClassA) - ipconfigs := rc.carveIPs(numberOfIPConfigs) - - cnsService.IPStateManager.AddIPConfigs(ipconfigs[0:numberOfIPConfigs]) + rc.ip, _, _ = net.ParseCIDR(rc.currentStatus.NetworkContainers[0].SubnetAddressSpace) + rc.CarveIPConfigsAndAddToStatusAndCNS(numberOfIPConfigs) return rc } -func (rc *RequestControllerFake) carveIPs(ipCount int) []cns.IPConfigurationStatus { - var ipconfigs []cns.IPConfigurationStatus - for i := 0; i < ipCount; i++ { - ipconfig := cns.IPConfigurationStatus{ - ID: uuid.New().String(), - IPAddress: rc.ip.String(), +func (rc *RequestControllerFake) CarveIPConfigsAndAddToStatusAndCNS(numberOfIPConfigs int) []cns.IPConfigurationStatus { + var cnsIPConfigs []cns.IPConfigurationStatus + for i := 0; i < numberOfIPConfigs; i++ { + + ipconfigCRD := nnc.IPAssignment{ + Name: uuid.New().String(), + IP: rc.ip.String(), + } + rc.currentStatus.NetworkContainers[0].IPAssignments = append(rc.currentStatus.NetworkContainers[0].IPAssignments, ipconfigCRD) + + ipconfigCNS := cns.IPConfigurationStatus{ + ID: ipconfigCRD.Name, + IPAddress: ipconfigCRD.IP, State: cns.Available, } - ipconfigs = append(ipconfigs, ipconfig) + cnsIPConfigs = append(cnsIPConfigs, ipconfigCNS) + incrementIP(rc.ip) } - return ipconfigs + + rc.fakecns.IPStateManager.AddIPConfigs(cnsIPConfigs) + + return cnsIPConfigs } func (rc *RequestControllerFake) StartRequestController(exitChan <-chan struct{}) error { return nil } -func (rc *RequestControllerFake) UpdateCRDSpec(cntxt context.Context, crdSpec nnc.NodeNetworkConfigSpec) error { - rc.desiredState = crdSpec +func (rc *RequestControllerFake) UpdateCRDSpec(cntxt context.Context, desiredSpec nnc.NodeNetworkConfigSpec) error { + rc.desiredSpec = desiredSpec + return nil } +func remove(slice []nnc.IPAssignment, s int) []nnc.IPAssignment { + return append(slice[:s], slice[s+1:]...) +} + func (rc *RequestControllerFake) Reconcile() error { - rc.fakecns.GetPodIPConfigState() - diff := int(rc.desiredState.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState()) + diff := int(rc.desiredSpec.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState()) + + if diff > 0 { + // carve the difference of test IPs and add them to CNS, assume dnc has populated the CRD status + rc.CarveIPConfigsAndAddToStatusAndCNS(diff) + + } else if diff < 0 { + + // Assume DNC has removed the IPConfigs from the status + + // mimic DNC removing IPConfigs from the CRD + for _, notInUseIPConfigName := range rc.desiredSpec.IPsNotInUse { - // carve the difference of test IPs - ipconfigs := rc.carveIPs(diff) + // remove ipconfig from status + index := 0 + for _, ipconfig := range rc.currentStatus.NetworkContainers[0].IPAssignments { + if notInUseIPConfigName == ipconfig.Name { + break + } + index++ + } + rc.currentStatus.NetworkContainers[0].IPAssignments = remove(rc.currentStatus.NetworkContainers[0].IPAssignments, index) - // add IPConfigs to CNS - rc.fakecns.IPStateManager.AddIPConfigs(ipconfigs) + } + + // remove ipconfig from CNS + rc.fakecns.IPStateManager.RemovePendingReleaseIPConfigs(rc.desiredSpec.IPsNotInUse) - return rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) + // empty the not in use ip's from the spec + rc.desiredSpec.IPsNotInUse = []string{} + } + // update + rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) + + return nil } func incrementIP(ip net.IP) { diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 2af7f6ff62..7c512b833b 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -61,17 +61,24 @@ func (pm *CNSIPAMPoolMonitor) Reconcile() error { func (pm *CNSIPAMPoolMonitor) checkForResize() int64 { + cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState()) + allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) + pendingReleaseIPCount := len(pm.cns.GetPendingReleaseIPConfigs()) + availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns + freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) + + // if cns pending pending ip release map is empty, request controller has already reconciled the CNS state, + // so we can remove it from our cache + if pendingReleaseIPCount == 0 { + pm.cachedSpec.IPsNotInUse = []string{} + } + // if there's a pending change to the spec count, and the pending release state is nonzero, // skip so we don't thrash the UpdateCRD if pm.cachedSpec.RequestedIPCount != int64(len(pm.cns.GetPodIPConfigState())) && len(pm.cns.GetPendingReleaseIPConfigs()) > 0 { return doNothing } - cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState()) - allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) - availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns - freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) - switch { // pod count is increasing case allocatedPodIPCount == 0: @@ -121,12 +128,12 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { return err } - log.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, pm.cns.GetAllocatedIPConfigs()) + log.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Goal IP Count: %v, Pods with IP's: %v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } // CNSToCRDSpec translates CNS's map of Ips to be released and requested ip count into a CRD Spec -func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.SecondaryIPConfig, ipCount int64) (nnc.NodeNetworkConfigSpec, error) { +func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationStatus, ipCount int64) (nnc.NodeNetworkConfigSpec, error) { var ( spec nnc.NodeNetworkConfigSpec uuid string diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 0db4a3c4ff..28812675f6 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -1,6 +1,7 @@ package ipampoolmonitor import ( + "log" "testing" "github.com/Azure/azure-container-networking/cns" @@ -36,9 +37,6 @@ func TestPoolSizeIncrease(t *testing.T) { fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) - t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) - t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) - // Effectively calling reconcile on start err := poolmonitor.Reconcile() if err != nil { @@ -103,9 +101,6 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) - t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) - t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) - // Effectively calling reconcile on start err := poolmonitor.Reconcile() if err != nil { @@ -216,3 +211,138 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } } + +func TestPoolDecrease(t *testing.T) { + var ( + batchSize = 10 + initialIPConfigCount = 20 + requestThresholdPercent = 30 + releaseThresholdPercent = 150 + ) + + fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + + log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) + log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) + + // Effectively calling reconcile on start, likely when no pods are scheduled + err := poolmonitor.Reconcile() + if err != nil { + t.Errorf("%v", err) + } + + // initial pool count is 20, set 15 of them to be allocated + err = fakecns.SetNumberOfAllocatedIPs(15) + if err != nil { + t.Error(err) + } + + // Pool monitor does nothing, as the current number of IP's falls in the threshold + err = poolmonitor.Reconcile() + if err != nil { + t.Error(err) + } + + // Decrease the number of allocated IP's down to 5. This should trigger a scale down + err = fakecns.SetNumberOfAllocatedIPs(4) + if err != nil { + t.Error(err) + } + + // Pool monitor will adjust the spec so the pool size will be 1 batch size smaller + err = poolmonitor.Reconcile() + if err != nil { + t.Error(err) + } + + // ensure that the adjusted spec is smaller than the initial pool size + if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + } + + // reconcile the fake request controller + err = fakerc.Reconcile() + if err != nil { + t.Error(err) + } + + // Ensure the size of the requested spec is still the same + if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + } + + return +} + +func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { + var ( + batchSize = 10 + initialIPConfigCount = 20 + requestThresholdPercent = 30 + releaseThresholdPercent = 100 + ) + + fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) + + log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) + log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) + + // Effectively calling reconcile on start + err := poolmonitor.Reconcile() + if err != nil { + t.Fatalf("Failed reconcile pool on start %v", err) + } + + // initial pool count is 30, set 25 of them to be allocated + err = fakecns.SetNumberOfAllocatedIPs(5) + if err != nil { + t.Error(err) + } + + // Pool monitor does nothing, as the current number of IP's falls in the threshold + err = poolmonitor.Reconcile() + if err != nil { + t.Errorf("Expected pool monitor to not fail after CNS set number of allocated IP's %v", err) + } + + // Ensure the size of the requested spec is still the same + if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected IP's not in use be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + } + + // Ensure the request ipcount is now one batch size smaller than the inital IP count + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + } + + // Update pods with IP count, ensure pool monitor stays the same until request controller reconciles + err = fakecns.SetNumberOfAllocatedIPs(6) + if err != nil { + t.Error(err) + } + + // Ensure the size of the requested spec is still the same + if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected IP's not in use to be one batch size smaller after reconcile, and not change after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + } + + // Ensure the request ipcount is now one batch size smaller than the inital IP count + if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, and not change after existing call, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + } + + err = fakerc.Reconcile() + if err != nil { + t.Error(err) + } + + err = poolmonitor.Reconcile() + if err != nil { + t.Errorf("Expected no pool monitor failure after request controller reconcile: %v", err) + } + + // Ensure the spec doesn't have any IPsNotInUse after request controller has reconciled + if len(poolmonitor.cachedSpec.IPsNotInUse) != 0 { + t.Fatalf("Expected IP's not in use to be 0 after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + } +} diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index a2028a1217..4fef4b7cae 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -90,8 +90,8 @@ func (service *HTTPRestService) releaseIPConfigHandler(w http.ResponseWriter, r return } -func (service *HTTPRestService) MarkIPsAsPending(numberToMark int) (map[string]cns.SecondaryIPConfig, error) { - pendingReleaseIPs := make(map[string]cns.SecondaryIPConfig) +func (service *HTTPRestService) MarkIPsAsPending(numberToMark int) (map[string]cns.IPConfigurationStatus, error) { + pendingReleaseIPs := make(map[string]cns.IPConfigurationStatus) markedIPCount := 0 service.Lock() @@ -99,9 +99,7 @@ func (service *HTTPRestService) MarkIPsAsPending(numberToMark int) (map[string]c for uuid, ipconfig := range service.PodIPConfigState { if ipconfig.State == cns.Available { ipconfig.State = cns.PendingRelease - pendingReleaseIPs[uuid] = cns.SecondaryIPConfig{ - IPAddress: ipconfig.IPAddress, - } + pendingReleaseIPs[uuid] = service.PodIPConfigState[uuid] markedIPCount++ if markedIPCount == numberToMark { return pendingReleaseIPs, nil From 8d258f18e4ea2f27109eb9c76fae5bbfab2ec1cf Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 2 Sep 2020 11:08:18 -0700 Subject: [PATCH 12/18] init scale down scale down testing test work reconcile fake requestcontroller test revision check for cached nonreleased ipconfigs remove test struct from decrease fakes initial integration fix interface signature test get pending release futher integration tseting test fixes --- cns/api.go | 4 +- cns/fakes/ipampoolmonitorfake.go | 21 +++ cns/fakes/requestcontrollerfake.go | 11 +- cns/ipampoolmonitor/ipampoolmonitor.go | 151 +++++++++++------- cns/ipampoolmonitor/ipampoolmonitor_test.go | 58 ++----- cns/logger/log.go | 6 +- .../kubecontroller/crdrequestcontroller.go | 4 + cns/restserver/api_test.go | 5 +- cns/restserver/internalapi.go | 10 +- cns/restserver/ipam.go | 12 +- cns/restserver/ipam_test.go | 46 +++++- cns/service/main.go | 23 ++- 12 files changed, 208 insertions(+), 143 deletions(-) create mode 100644 cns/fakes/ipampoolmonitorfake.go diff --git a/cns/api.go b/cns/api.go index a7a8458930..2e98d43d76 100644 --- a/cns/api.go +++ b/cns/api.go @@ -164,8 +164,8 @@ type NodeConfiguration struct { } type IPAMPoolMonitor interface { - Start() error - UpdatePoolLimits(scalarUnits ScalarUnits) error + Start(poolMonitorRefreshMilliseconds int, exitChan <-chan struct{}) error + UpdatePoolMonitor(scalarUnits ScalarUnits) error } type ScalarUnits struct { diff --git a/cns/fakes/ipampoolmonitorfake.go b/cns/fakes/ipampoolmonitorfake.go new file mode 100644 index 0000000000..7ccf72dd75 --- /dev/null +++ b/cns/fakes/ipampoolmonitorfake.go @@ -0,0 +1,21 @@ +package fakes + +import "github.com/Azure/azure-container-networking/cns" + +type IPAMPoolMonitorFake struct{} + +func NewIPAMPoolMonitorFake() *IPAMPoolMonitorFake { + return &IPAMPoolMonitorFake{} +} + +func (ipm *IPAMPoolMonitorFake) Start(poolMonitorRefreshMilliseconds int, exitChan <-chan struct{}) error { + return nil +} + +func (ipm *IPAMPoolMonitorFake) UpdatePoolMonitor(scalarUnits cns.ScalarUnits) error { + return nil +} + +func (ipm *IPAMPoolMonitorFake) Reconcile() error { + return nil +} diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index d113e22c7b..0d293dd8d6 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -18,7 +18,6 @@ type RequestControllerFake struct { } func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { - rc := &RequestControllerFake{ fakecns: cnsService, testScalarUnits: scalarUnits, @@ -28,16 +27,18 @@ func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.Scala ReleaseThresholdPercent: scalarUnits.ReleaseThresholdPercent, }, NetworkContainers: []nnc.NetworkContainer{ - nnc.NetworkContainer{ + { IPAssignments: []nnc.IPAssignment{}, SubnetAddressSpace: "10.0.0.0/8", }, }, }, + desiredSpec: nnc.NodeNetworkConfigSpec{ + RequestedIPCount: int64(numberOfIPConfigs), + }, } rc.ip, _, _ = net.ParseCIDR(rc.currentStatus.NetworkContainers[0].SubnetAddressSpace) - rc.CarveIPConfigsAndAddToStatusAndCNS(numberOfIPConfigs) return rc } @@ -63,6 +64,7 @@ func (rc *RequestControllerFake) CarveIPConfigsAndAddToStatusAndCNS(numberOfIPCo } rc.fakecns.IPStateManager.AddIPConfigs(cnsIPConfigs) + rc.desiredSpec.RequestedIPCount = int64(len(cnsIPConfigs)) return cnsIPConfigs } @@ -114,8 +116,9 @@ func (rc *RequestControllerFake) Reconcile() error { // empty the not in use ip's from the spec rc.desiredSpec.IPsNotInUse = []string{} } + // update - rc.fakecns.PoolMonitor.UpdatePoolLimits(rc.testScalarUnits) + rc.fakecns.PoolMonitor.UpdatePoolMonitor(rc.testScalarUnits) return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 7c512b833b..4e7e0b8195 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -3,10 +3,11 @@ package ipampoolmonitor import ( "context" "fmt" - "log" "sync" + "time" "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/requestcontroller" nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) @@ -18,9 +19,12 @@ var ( ) type CNSIPAMPoolMonitor struct { - initialized bool + initialized bool + pendingRelease bool + + cachedSpec nnc.NodeNetworkConfigSpec + cachedSecondaryIPConfigs map[string]cns.SecondaryIPConfig - cachedSpec nnc.NodeNetworkConfigSpec cns cns.HTTPService rc requestcontroller.RequestController scalarUnits cns.ScalarUnits @@ -32,67 +36,83 @@ type CNSIPAMPoolMonitor struct { func NewCNSIPAMPoolMonitor(cnsService cns.HTTPService, requestController requestcontroller.RequestController) *CNSIPAMPoolMonitor { return &CNSIPAMPoolMonitor{ - initialized: false, - cns: cnsService, - rc: requestController, + initialized: false, + pendingRelease: false, + cns: cnsService, + rc: requestController, } } -// TODO: add looping and cancellation to this, and add to CNS MAIN -func (pm *CNSIPAMPoolMonitor) Start() error { - // run Reconcile in a loop - return nil -} - -func (pm *CNSIPAMPoolMonitor) Reconcile() error { - if pm.initialized { - //get number of allocated IP configs, and calculate free IP's against the cached spec - rebatchAction := pm.checkForResize() - switch rebatchAction { - case increasePoolSize: - return pm.increasePoolSize() - case decreasePoolSize: - return pm.decreasePoolSize() - } +func stopReconcile(ch <-chan struct{}) bool { + select { + case <-ch: + return true + default: } - return nil + return false } -func (pm *CNSIPAMPoolMonitor) checkForResize() int64 { +func (pm *CNSIPAMPoolMonitor) Start(poolMonitorRefreshMilliseconds int, exitChan <-chan struct{}) error { + logger.Printf("[ipam-pool-monitor] Starting CNS IPAM Pool Monitor") + for { + if stopReconcile(exitChan) { + return fmt.Errorf("CNS IPAM Pool Monitor received cancellation signal") + } - cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState()) - allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) - pendingReleaseIPCount := len(pm.cns.GetPendingReleaseIPConfigs()) - availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns - freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) + err := pm.Reconcile() + if err != nil { + logger.Printf("[ipam-pool-monitor] CRITICAL %v", err) + } - // if cns pending pending ip release map is empty, request controller has already reconciled the CNS state, - // so we can remove it from our cache - if pendingReleaseIPCount == 0 { - pm.cachedSpec.IPsNotInUse = []string{} + time.Sleep(time.Duration(poolMonitorRefreshMilliseconds) * time.Millisecond) } +} - // if there's a pending change to the spec count, and the pending release state is nonzero, - // skip so we don't thrash the UpdateCRD - if pm.cachedSpec.RequestedIPCount != int64(len(pm.cns.GetPodIPConfigState())) && len(pm.cns.GetPendingReleaseIPConfigs()) > 0 { - return doNothing - } +func (pm *CNSIPAMPoolMonitor) Reconcile() error { + if pm.initialized { + cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState()) + allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) + pendingReleaseIPCount := len(pm.cns.GetPendingReleaseIPConfigs()) + availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns + freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) + + logger.Printf("[ipam-pool-monitor] Checking pool for resize, Pool Size: %v, Goal Size: %v, BatchSize: %v, MinFree: %v, MaxFree:%v, Allocated: %v, Available: %v, Pending Release: %v, Free: %v", cnsPodIPConfigCount, pm.cachedSpec.RequestedIPCount, pm.scalarUnits.BatchSize, pm.MinimumFreeIps, pm.MaximumFreeIps, allocatedPodIPCount, availableIPConfigCount, pendingReleaseIPCount, freeIPConfigCount) + + // if there's a pending change to the spec count, and the pending release state is nonzero, + // skip so we don't thrash the UpdateCRD + if pm.cachedSpec.RequestedIPCount != int64(len(pm.cns.GetPodIPConfigState())) && len(pm.cns.GetPendingReleaseIPConfigs()) > 0 { + return nil + } - switch { - // pod count is increasing - case allocatedPodIPCount == 0: - log.Printf("[ipam-pool-monitor] No pods scheduled") - return doNothing + switch { + // pod count is increasing + case freeIPConfigCount < pm.MinimumFreeIps: + logger.Printf("[ipam-pool-monitor] Increasing pool size...") + return pm.increasePoolSize() + + // pod count is decreasing + case freeIPConfigCount > pm.MaximumFreeIps: + logger.Printf("[ipam-pool-monitor] Decreasing pool size...") + return pm.decreasePoolSize() - case freeIPConfigCount < pm.MinimumFreeIps: - return increasePoolSize + // CRD has reconciled CNS state, and target spec is now the same size as the state + // free to remove the IP's from the CRD + case pm.pendingRelease && int(pm.cachedSpec.RequestedIPCount) == cnsPodIPConfigCount: + logger.Printf("[ipam-pool-monitor] Removing Pending Release IP's from CRD...") + return pm.cleanPendingRelease() + + // no pods scheduled + case allocatedPodIPCount == 0: + logger.Printf("[ipam-pool-monitor] No pods scheduled") + return fmt.Errorf("No pods scheduled") + } + } else if !pm.initialized { + return fmt.Errorf("CNS Pool monitor not initialized") - // pod count is decreasing - case freeIPConfigCount > pm.MaximumFreeIps: - return decreasePoolSize } - return doNothing + + return nil } func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { @@ -105,19 +125,16 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { return err } - log.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Existing Goal IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) + logger.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Requested IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { - // TODO: Better handling here, negatives - // TODO: Maintain desired state to check against if pool size adjustment is already happening - decreaseIPCount := pm.cachedSpec.RequestedIPCount - pm.scalarUnits.BatchSize pm.cachedSpec.RequestedIPCount -= pm.scalarUnits.BatchSize // mark n number of IP's as pending - pendingIPAddresses, err := pm.cns.MarkIPsAsPending(int(decreaseIPCount)) + pendingIPAddresses, err := pm.cns.MarkIPsAsPending(int(pm.scalarUnits.BatchSize)) if err != nil { return err } @@ -127,13 +144,27 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { if err != nil { return err } + pm.pendingRelease = true + logger.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Requested IP Count: %v, Pods with IP's: %v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) + return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) +} + +// if cns pending ip release map is empty, request controller has already reconciled the CNS state, +// so we can remove it from our cache and remove the IP's from the CRD +func (pm *CNSIPAMPoolMonitor) cleanPendingRelease() error { + var err error + pm.cachedSpec, err = CNSToCRDSpec(nil, pm.cachedSpec.RequestedIPCount) + if err != nil { + logger.Printf("[ipam-pool-monitor] Failed to translate ") + } - log.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Goal IP Count: %v, Pods with IP's: %v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) + pm.pendingRelease = false return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) } // CNSToCRDSpec translates CNS's map of Ips to be released and requested ip count into a CRD Spec func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationStatus, ipCount int64) (nnc.NodeNetworkConfigSpec, error) { + var ( spec nnc.NodeNetworkConfigSpec uuid string @@ -141,15 +172,19 @@ func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationSt spec.RequestedIPCount = ipCount - for uuid = range toBeDeletedSecondaryIPConfigs { - spec.IPsNotInUse = append(spec.IPsNotInUse, uuid) + if toBeDeletedSecondaryIPConfigs == nil { + spec.IPsNotInUse = make([]string, 0) + } else { + for uuid = range toBeDeletedSecondaryIPConfigs { + spec.IPsNotInUse = append(spec.IPsNotInUse, uuid) + } } return spec, nil } // UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits -func (pm *CNSIPAMPoolMonitor) UpdatePoolLimits(scalarUnits cns.ScalarUnits) error { +func (pm *CNSIPAMPoolMonitor) UpdatePoolMonitor(scalarUnits cns.ScalarUnits) error { pm.Lock() defer pm.Unlock() pm.scalarUnits = scalarUnits diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 28812675f6..01aae1ffe8 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -23,7 +23,8 @@ func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, release poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc) fakecns.PoolMonitor = poolmonitor - poolmonitor.UpdatePoolLimits(scalarUnits) + fakerc.Reconcile() + return fakecns, fakerc, poolmonitor } @@ -37,19 +38,8 @@ func TestPoolSizeIncrease(t *testing.T) { fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) - // Effectively calling reconcile on start - err := poolmonitor.Reconcile() - if err != nil { - t.Fatalf("Failed to initialize poolmonitor on start with err: %v", err) - } - - // ensure pool monitor has reached quorum with cns - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) - } - // increase number of allocated IP's in CNS - err = fakecns.SetNumberOfAllocatedIPs(8) + err := fakecns.SetNumberOfAllocatedIPs(8) if err != nil { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } @@ -101,19 +91,8 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { fakecns, fakerc, poolmonitor := initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent) - // Effectively calling reconcile on start - err := poolmonitor.Reconcile() - if err != nil { - t.Fatalf("Failed to initialize poolmonitor on start with err: %v", err) - } - - // ensure pool monitor has reached quorum with cns - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) - } - // increase number of allocated IP's in CNS - err = fakecns.SetNumberOfAllocatedIPs(8) + err := fakecns.SetNumberOfAllocatedIPs(8) if err != nil { t.Fatalf("Failed to allocate test ipconfigs with err: %v", err) } @@ -180,9 +159,6 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { t.Logf("Minimum free IPs to request: %v", poolmonitor.MinimumFreeIps) t.Logf("Maximum free IPs to release: %v", poolmonitor.MaximumFreeIps) - // Effectively calling reconcile on start - poolmonitor.Reconcile() - // increase number of allocated IP's in CNS err := fakecns.SetNumberOfAllocatedIPs(8) if err != nil { @@ -225,34 +201,28 @@ func TestPoolDecrease(t *testing.T) { log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) - // Effectively calling reconcile on start, likely when no pods are scheduled - err := poolmonitor.Reconcile() - if err != nil { - t.Errorf("%v", err) - } - // initial pool count is 20, set 15 of them to be allocated - err = fakecns.SetNumberOfAllocatedIPs(15) + err := fakecns.SetNumberOfAllocatedIPs(15) if err != nil { - t.Error(err) + t.Fatal(err) } // Pool monitor does nothing, as the current number of IP's falls in the threshold err = poolmonitor.Reconcile() if err != nil { - t.Error(err) + t.Fatal(err) } // Decrease the number of allocated IP's down to 5. This should trigger a scale down err = fakecns.SetNumberOfAllocatedIPs(4) if err != nil { - t.Error(err) + t.Fatal(err) } // Pool monitor will adjust the spec so the pool size will be 1 batch size smaller err = poolmonitor.Reconcile() if err != nil { - t.Error(err) + t.Fatal(err) } // ensure that the adjusted spec is smaller than the initial pool size @@ -263,7 +233,7 @@ func TestPoolDecrease(t *testing.T) { // reconcile the fake request controller err = fakerc.Reconcile() if err != nil { - t.Error(err) + t.Fatal(err) } // Ensure the size of the requested spec is still the same @@ -287,14 +257,8 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { log.Printf("Min free IP's %v", poolmonitor.MinimumFreeIps) log.Printf("Max free IP %v", poolmonitor.MaximumFreeIps) - // Effectively calling reconcile on start - err := poolmonitor.Reconcile() - if err != nil { - t.Fatalf("Failed reconcile pool on start %v", err) - } - // initial pool count is 30, set 25 of them to be allocated - err = fakecns.SetNumberOfAllocatedIPs(5) + err := fakecns.SetNumberOfAllocatedIPs(5) if err != nil { t.Error(err) } diff --git a/cns/logger/log.go b/cns/logger/log.go index bfccd9b3d5..a3ae8e3fab 100644 --- a/cns/logger/log.go +++ b/cns/logger/log.go @@ -137,9 +137,9 @@ func Request(tag string, request interface{}, err error) { var msg string if err == nil { - msg = fmt.Sprintf("[%s] Received %T %+v.", tag, request, request) + //msg = fmt.Sprintf("[%s] Received %T %+v.", tag, request, request) } else { - msg = fmt.Sprintf("[%s] Failed to decode %T %+v %s.", tag, request, request, err.Error()) + //msg = fmt.Sprintf("[%s] Failed to decode %T %+v %s.", tag, request, request, err.Error()) } sendTraceInternal(msg) @@ -154,7 +154,7 @@ func Response(tag string, response interface{}, returnCode int, returnStr string var msg string if err == nil && returnCode == 0 { - msg = fmt.Sprintf("[%s] Sent %T %+v.", tag, response, response) + //msg = fmt.Sprintf("[%s] Sent %T %+v.", tag, response, response) } else if err != nil { msg = fmt.Sprintf("[%s] Code:%s, %+v %s.", tag, returnStr, response, err.Error()) } else { diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go index ab17045983..bf2d3a7cae 100644 --- a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go +++ b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go @@ -251,9 +251,13 @@ func (crdRC *crdRequestController) UpdateCRDSpec(cntxt context.Context, crdSpec return err } + logger.Printf("[cns-rc] Received update for IP count %+v", crdSpec) + //Update the CRD spec crdSpec.DeepCopyInto(&nodeNetworkConfig.Spec) + logger.Printf("[cns-rc] After deep copy %+v", nodeNetworkConfig.Spec) + //Send update to API server if err := crdRC.updateNodeNetConfig(cntxt, nodeNetworkConfig); err != nil { logger.Errorf("[cns-rc] Error updating CRD spec %v", err) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 74bb501036..2f2b3430ad 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -18,7 +18,6 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" "github.com/Azure/azure-container-networking/cns/fakes" - "github.com/Azure/azure-container-networking/cns/ipampoolmonitor" "github.com/Azure/azure-container-networking/cns/logger" acncommon "github.com/Azure/azure-container-networking/common" ) @@ -678,9 +677,7 @@ func startService() { } } - fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake(fakecns, cns.ScalarUnits{BatchSize: 10, RequestThresholdPercent: 30, ReleaseThresholdPercent: 100}, 10) - service.(*HTTPRestService).PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(fakecns, fakerc) + service.(*HTTPRestService).PoolMonitor = fakes.NewIPAMPoolMonitorFake() // Get the internal http mux as test hook. mux = service.(*HTTPRestService).Listener.GetMux() diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index 6c88019894..f90c314fe9 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -12,10 +12,8 @@ import ( "reflect" "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/ipampoolmonitor" "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/nmagentclient" - "github.com/Azure/azure-container-networking/cns/requestcontroller" "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/log" ) @@ -152,12 +150,6 @@ func (service *HTTPRestService) SyncNodeStatus(dncEP, infraVnet, nodeID string, return } -func (service *HTTPRestService) StartCNSIPAMPoolMonitor(cnsService cns.HTTPService, requestController requestcontroller.RequestController) { - - // TODO, start pool monitor as well - service.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(cnsService, requestController) -} - // This API will be called by CNS RequestController on CRD update. func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIp map[string]cns.KubernetesPodInfo, scalarUnits cns.ScalarUnits) int { // check if ncRequest is null, then return as there is no CRD state yet @@ -252,7 +244,7 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C logger.Errorf(returnMessage) } - err = service.PoolMonitor.UpdatePoolLimits(scalarUnits) + err = service.PoolMonitor.UpdatePoolMonitor(scalarUnits) if err != nil { logger.Errorf("[Azure CNS] Error. Reference to CNS not set in IPAM Pool Monitor: %v", req) return UnexpectedError diff --git a/cns/restserver/ipam.go b/cns/restserver/ipam.go index 4fef4b7cae..cf4e9c7201 100644 --- a/cns/restserver/ipam.go +++ b/cns/restserver/ipam.go @@ -96,10 +96,12 @@ func (service *HTTPRestService) MarkIPsAsPending(numberToMark int) (map[string]c service.Lock() defer service.Unlock() - for uuid, ipconfig := range service.PodIPConfigState { - if ipconfig.State == cns.Available { - ipconfig.State = cns.PendingRelease - pendingReleaseIPs[uuid] = service.PodIPConfigState[uuid] + for uuid, _ := range service.PodIPConfigState { + mutableIPConfig := service.PodIPConfigState[uuid] + if mutableIPConfig.State == cns.Available { + mutableIPConfig.State = cns.PendingRelease + service.PodIPConfigState[uuid] = mutableIPConfig + pendingReleaseIPs[uuid] = mutableIPConfig markedIPCount++ if markedIPCount == numberToMark { return pendingReleaseIPs, nil @@ -111,6 +113,8 @@ func (service *HTTPRestService) MarkIPsAsPending(numberToMark int) (map[string]c } func (service *HTTPRestService) GetPodIPConfigState() map[string]cns.IPConfigurationStatus { + service.RLock() + defer service.RUnlock() return service.PodIPConfigState } diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 7b7e5bb7f9..5b26e8f86a 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -12,7 +12,6 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" "github.com/Azure/azure-container-networking/cns/fakes" - "github.com/Azure/azure-container-networking/cns/ipampoolmonitor" ) var ( @@ -45,9 +44,7 @@ func getTestService() *HTTPRestService { httpsvc, _ := NewHTTPRestService(&config, fakes.NewFakeImdsClient()) svc = httpsvc.(*HTTPRestService) - fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake(fakecns, cns.ScalarUnits{BatchSize: 10, RequestThresholdPercent: 30, ReleaseThresholdPercent: 150}, 10) - svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(fakecns, fakerc) + svc.PoolMonitor = fakes.NewIPAMPoolMonitorFake() setOrchestratorTypeInternal(cns.KubernetesCRD) @@ -538,3 +535,44 @@ func validateIpState(t *testing.T, actualIps []cns.IPConfigurationStatus, expect } } } + +func TestIPAMMarkIPConfigAsPending(t *testing.T) { + svc := getTestService() + // set state as already allocated + state1, _ := NewPodStateWithOrchestratorContext(testIP1, 24, testPod1GUID, testNCID, cns.Available, testPod1Info) + ipconfigs := map[string]cns.IPConfigurationStatus{ + state1.ID: state1, + } + + err := UpdatePodIpConfigState(t, svc, ipconfigs) + if err != nil { + t.Fatalf("Expected to not fail adding IP's to state: %+v", err) + } + + // Release Test Pod 1 + ips, err := svc.MarkIPsAsPending(1) + if err != nil { + t.Fatalf("Unexpected failure releasing IP: %+v", err) + } + + if _, exists := ips[testPod1GUID]; !exists { + t.Fatalf("Expected ID not marked as pending: %+v", err) + } + + // Release Test Pod 1 + pendingrelease := svc.GetPendingReleaseIPConfigs() + if len(pendingrelease) != 1 { + t.Fatal("Expected pending release slice to be nonzero after pending release") + } + + available := svc.GetAvailableIPConfigs() + if len(available) != 0 { + t.Fatal("Expected available ips to be zero after marked as pending") + } + + // Call release again, should be fine + err = svc.releaseIPConfig(testPod1Info) + if err != nil { + t.Fatalf("Unexpected failure releasing IP: %+v", err) + } +} diff --git a/cns/service/main.go b/cns/service/main.go index 533a2edbd5..d0c529c435 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -38,11 +38,12 @@ import ( const ( // Service name. - name = "azure-cns" - pluginName = "azure-vnet" - defaultCNINetworkConfigFileName = "10-azure.conflist" - configFileName = "config.json" - dncApiVersion = "?api-version=2018-03-01" + name = "azure-cns" + pluginName = "azure-vnet" + defaultCNINetworkConfigFileName = "10-azure.conflist" + configFileName = "config.json" + dncApiVersion = "?api-version=2018-03-01" + poolIPAMRefreshRateInMilliseconds = 1000 ) // Version is populated by make during build. @@ -472,9 +473,15 @@ func main() { } }() - poolMonitor := ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestService, requestController) - - httpRestServiceImplementation.PoolMonitor = poolMonitor + ipamPoolMonitorControllerStopChannel := make(chan struct{}) + defer close(ipamPoolMonitorControllerStopChannel) + go func() { + poolMonitor := ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestService, requestController) + httpRestServiceImplementation.PoolMonitor = poolMonitor + if err := poolMonitor.Start(poolIPAMRefreshRateInMilliseconds, ipamPoolMonitorControllerStopChannel); err != nil { + logger.Errorf("[Azure CNS] Failed to start pool monitor with err: %v", err) + } + }() } var netPlugin network.NetPlugin From 4737f00792dde8a460b9d396145c8e72d5cf1565 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Fri, 11 Sep 2020 10:00:19 -0700 Subject: [PATCH 13/18] start feedback --- cns/api.go | 3 +- cns/fakes/ipampoolmonitorfake.go | 8 +- cns/ipampoolmonitor/ipampoolmonitor.go | 121 ++++++++++++------------- cns/service/main.go | 7 +- 4 files changed, 69 insertions(+), 70 deletions(-) diff --git a/cns/api.go b/cns/api.go index 2e98d43d76..90fbb3b90c 100644 --- a/cns/api.go +++ b/cns/api.go @@ -4,6 +4,7 @@ package cns import ( + "context" "encoding/json" "github.com/Azure/azure-container-networking/cns/common" @@ -164,7 +165,7 @@ type NodeConfiguration struct { } type IPAMPoolMonitor interface { - Start(poolMonitorRefreshMilliseconds int, exitChan <-chan struct{}) error + Start(ctx context.Context, poolMonitorRefreshMilliseconds int) error UpdatePoolMonitor(scalarUnits ScalarUnits) error } diff --git a/cns/fakes/ipampoolmonitorfake.go b/cns/fakes/ipampoolmonitorfake.go index 7ccf72dd75..e864611776 100644 --- a/cns/fakes/ipampoolmonitorfake.go +++ b/cns/fakes/ipampoolmonitorfake.go @@ -1,6 +1,10 @@ package fakes -import "github.com/Azure/azure-container-networking/cns" +import ( + "context" + + "github.com/Azure/azure-container-networking/cns" +) type IPAMPoolMonitorFake struct{} @@ -8,7 +12,7 @@ func NewIPAMPoolMonitorFake() *IPAMPoolMonitorFake { return &IPAMPoolMonitorFake{} } -func (ipm *IPAMPoolMonitorFake) Start(poolMonitorRefreshMilliseconds int, exitChan <-chan struct{}) error { +func (ipm *IPAMPoolMonitorFake) Start(ctx context.Context, poolMonitorRefreshMilliseconds int) error { return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index 4e7e0b8195..f2e04df8f5 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -12,14 +12,7 @@ import ( nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) -var ( - increasePoolSize = int64(1) - decreasePoolSize = int64(-1) - doNothing = int64(0) -) - type CNSIPAMPoolMonitor struct { - initialized bool pendingRelease bool cachedSpec nnc.NodeNetworkConfigSpec @@ -31,12 +24,11 @@ type CNSIPAMPoolMonitor struct { MinimumFreeIps int64 MaximumFreeIps int64 - sync.RWMutex + mu sync.RWMutex } func NewCNSIPAMPoolMonitor(cnsService cns.HTTPService, requestController requestcontroller.RequestController) *CNSIPAMPoolMonitor { return &CNSIPAMPoolMonitor{ - initialized: false, pendingRelease: false, cns: cnsService, rc: requestController, @@ -53,69 +45,68 @@ func stopReconcile(ch <-chan struct{}) bool { return false } -func (pm *CNSIPAMPoolMonitor) Start(poolMonitorRefreshMilliseconds int, exitChan <-chan struct{}) error { +func (pm *CNSIPAMPoolMonitor) Start(ctx context.Context, poolMonitorRefreshMilliseconds int) error { logger.Printf("[ipam-pool-monitor] Starting CNS IPAM Pool Monitor") - for { - if stopReconcile(exitChan) { - return fmt.Errorf("CNS IPAM Pool Monitor received cancellation signal") - } + ticker := time.NewTicker(time.Duration(poolMonitorRefreshMilliseconds) * time.Millisecond) + select { + case <-ctx.Done(): + return fmt.Errorf("CNS IPAM Pool Monitor received cancellation signal") + case <-ticker.C: err := pm.Reconcile() if err != nil { - logger.Printf("[ipam-pool-monitor] CRITICAL %v", err) + logger.Printf("[ipam-pool-monitor] Reconcile failed with err %v", err) } - - time.Sleep(time.Duration(poolMonitorRefreshMilliseconds) * time.Millisecond) } + + return nil } func (pm *CNSIPAMPoolMonitor) Reconcile() error { - if pm.initialized { - cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState()) - allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) - pendingReleaseIPCount := len(pm.cns.GetPendingReleaseIPConfigs()) - availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns - freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) - - logger.Printf("[ipam-pool-monitor] Checking pool for resize, Pool Size: %v, Goal Size: %v, BatchSize: %v, MinFree: %v, MaxFree:%v, Allocated: %v, Available: %v, Pending Release: %v, Free: %v", cnsPodIPConfigCount, pm.cachedSpec.RequestedIPCount, pm.scalarUnits.BatchSize, pm.MinimumFreeIps, pm.MaximumFreeIps, allocatedPodIPCount, availableIPConfigCount, pendingReleaseIPCount, freeIPConfigCount) - - // if there's a pending change to the spec count, and the pending release state is nonzero, - // skip so we don't thrash the UpdateCRD - if pm.cachedSpec.RequestedIPCount != int64(len(pm.cns.GetPodIPConfigState())) && len(pm.cns.GetPendingReleaseIPConfigs()) > 0 { - return nil - } - - switch { - // pod count is increasing - case freeIPConfigCount < pm.MinimumFreeIps: - logger.Printf("[ipam-pool-monitor] Increasing pool size...") - return pm.increasePoolSize() - - // pod count is decreasing - case freeIPConfigCount > pm.MaximumFreeIps: - logger.Printf("[ipam-pool-monitor] Decreasing pool size...") - return pm.decreasePoolSize() - - // CRD has reconciled CNS state, and target spec is now the same size as the state - // free to remove the IP's from the CRD - case pm.pendingRelease && int(pm.cachedSpec.RequestedIPCount) == cnsPodIPConfigCount: - logger.Printf("[ipam-pool-monitor] Removing Pending Release IP's from CRD...") - return pm.cleanPendingRelease() - - // no pods scheduled - case allocatedPodIPCount == 0: - logger.Printf("[ipam-pool-monitor] No pods scheduled") - return fmt.Errorf("No pods scheduled") - } - } else if !pm.initialized { - return fmt.Errorf("CNS Pool monitor not initialized") + cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState()) + allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) + pendingReleaseIPCount := len(pm.cns.GetPendingReleaseIPConfigs()) + availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns + freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) + + logger.Printf("[ipam-pool-monitor] Checking pool for resize, Pool Size: %v, Goal Size: %v, BatchSize: %v, MinFree: %v, MaxFree:%v, Allocated: %v, Available: %v, Pending Release: %v, Free: %v", cnsPodIPConfigCount, pm.cachedSpec.RequestedIPCount, pm.scalarUnits.BatchSize, pm.MinimumFreeIps, pm.MaximumFreeIps, allocatedPodIPCount, availableIPConfigCount, pendingReleaseIPCount, freeIPConfigCount) + + // if there's a pending change to the spec count, and the pending release state is nonzero, + // skip so we don't thrash the UpdateCRD + if pm.cachedSpec.RequestedIPCount != int64(len(pm.cns.GetPodIPConfigState())) && len(pm.cns.GetPendingReleaseIPConfigs()) > 0 { + return nil + } + switch { + // pod count is increasing + case freeIPConfigCount < pm.MinimumFreeIps: + logger.Printf("[ipam-pool-monitor] Increasing pool size...") + return pm.increasePoolSize() + + // pod count is decreasing + case freeIPConfigCount > pm.MaximumFreeIps: + logger.Printf("[ipam-pool-monitor] Decreasing pool size...") + return pm.decreasePoolSize() + + // CRD has reconciled CNS state, and target spec is now the same size as the state + // free to remove the IP's from the CRD + case pm.pendingRelease && int(pm.cachedSpec.RequestedIPCount) == cnsPodIPConfigCount: + logger.Printf("[ipam-pool-monitor] Removing Pending Release IP's from CRD...") + return pm.cleanPendingRelease() + + // no pods scheduled + case allocatedPodIPCount == 0: + logger.Printf("[ipam-pool-monitor] No pods scheduled") + return nil } return nil } func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { + pm.mu.Lock() + defer pm.mu.Unlock() + var err error pm.cachedSpec.RequestedIPCount += pm.scalarUnits.BatchSize @@ -130,7 +121,10 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { } func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { - // TODO: Better handling here, negatives + pm.mu.Lock() + defer pm.mu.Unlock() + + // TODO: Better handling here for negatives pm.cachedSpec.RequestedIPCount -= pm.scalarUnits.BatchSize // mark n number of IP's as pending @@ -152,6 +146,9 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { // if cns pending ip release map is empty, request controller has already reconciled the CNS state, // so we can remove it from our cache and remove the IP's from the CRD func (pm *CNSIPAMPoolMonitor) cleanPendingRelease() error { + pm.mu.Lock() + defer pm.mu.Unlock() + var err error pm.cachedSpec, err = CNSToCRDSpec(nil, pm.cachedSpec.RequestedIPCount) if err != nil { @@ -164,7 +161,6 @@ func (pm *CNSIPAMPoolMonitor) cleanPendingRelease() error { // CNSToCRDSpec translates CNS's map of Ips to be released and requested ip count into a CRD Spec func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationStatus, ipCount int64) (nnc.NodeNetworkConfigSpec, error) { - var ( spec nnc.NodeNetworkConfigSpec uuid string @@ -185,8 +181,8 @@ func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationSt // UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits func (pm *CNSIPAMPoolMonitor) UpdatePoolMonitor(scalarUnits cns.ScalarUnits) error { - pm.Lock() - defer pm.Unlock() + pm.mu.Lock() + defer pm.mu.Unlock() pm.scalarUnits = scalarUnits pm.MinimumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) @@ -196,10 +192,7 @@ func (pm *CNSIPAMPoolMonitor) UpdatePoolMonitor(scalarUnits cns.ScalarUnits) err return fmt.Errorf("Error Updating Pool Limits, reference to CNS is nil") } - if !pm.initialized && len(pm.cns.GetPodIPConfigState()) > 0 { - pm.cachedSpec.RequestedIPCount = int64(len(pm.cns.GetPodIPConfigState())) - pm.initialized = true - } + pm.cachedSpec.RequestedIPCount = int64(len(pm.cns.GetPodIPConfigState())) return nil } diff --git a/cns/service/main.go b/cns/service/main.go index d0c529c435..5a882cc6e9 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -5,6 +5,7 @@ package main import ( "bytes" + "context" "encoding/json" "fmt" "net/http" @@ -473,12 +474,12 @@ func main() { } }() - ipamPoolMonitorControllerStopChannel := make(chan struct{}) - defer close(ipamPoolMonitorControllerStopChannel) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() go func() { poolMonitor := ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestService, requestController) httpRestServiceImplementation.PoolMonitor = poolMonitor - if err := poolMonitor.Start(poolIPAMRefreshRateInMilliseconds, ipamPoolMonitorControllerStopChannel); err != nil { + if err := poolMonitor.Start(ctx, poolIPAMRefreshRateInMilliseconds); err != nil { logger.Errorf("[Azure CNS] Failed to start pool monitor with err: %v", err) } }() From b79c37591f287fdaedd1fce2bc32f3588496e19b Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Mon, 14 Sep 2020 17:14:13 -0700 Subject: [PATCH 14/18] update request controller --- cns/api.go | 10 +-- cns/cnsclient/apiclient.go | 4 +- cns/cnsclient/httpapi/client.go | 8 +- cns/ipampoolmonitor/ipampoolmonitor.go | 79 +++++++++---------- cns/logger/log.go | 6 +- .../kubecontroller/crdreconciler.go | 19 ++--- .../kubecontroller/crdrequestcontroller.go | 22 ++---- cns/restserver/internalapi.go | 12 +-- cns/service/main.go | 7 +- 9 files changed, 74 insertions(+), 93 deletions(-) diff --git a/cns/api.go b/cns/api.go index 90fbb3b90c..84af52e082 100644 --- a/cns/api.go +++ b/cns/api.go @@ -8,6 +8,7 @@ import ( "encoding/json" "github.com/Azure/azure-container-networking/cns/common" + nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) // Container Network Service remote API Contract @@ -163,16 +164,9 @@ type NodeConfiguration struct { NodeID string NodeSubnet Subnet } - type IPAMPoolMonitor interface { Start(ctx context.Context, poolMonitorRefreshMilliseconds int) error - UpdatePoolMonitor(scalarUnits ScalarUnits) error -} - -type ScalarUnits struct { - BatchSize int64 - RequestThresholdPercent int64 - ReleaseThresholdPercent int64 + Update(nodeConfig nnc.NodeNetworkConfig) error } // Response describes generic response from CNS. diff --git a/cns/cnsclient/apiclient.go b/cns/cnsclient/apiclient.go index 3fc0493840..c7ce324d71 100644 --- a/cns/cnsclient/apiclient.go +++ b/cns/cnsclient/apiclient.go @@ -4,6 +4,6 @@ import "github.com/Azure/azure-container-networking/cns" // APIClient interface to update cns state type APIClient interface { - ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.KubernetesPodInfo, scalarUnits cns.ScalarUnits) error - CreateOrUpdateNC(nc cns.CreateNetworkContainerRequest, scalarUnits cns.ScalarUnits) error + ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.KubernetesPodInfo) error + CreateOrUpdateNC(nc cns.CreateNetworkContainerRequest) error } diff --git a/cns/cnsclient/httpapi/client.go b/cns/cnsclient/httpapi/client.go index a7821b98f9..04a49f9740 100644 --- a/cns/cnsclient/httpapi/client.go +++ b/cns/cnsclient/httpapi/client.go @@ -13,8 +13,8 @@ type Client struct { } // CreateOrUpdateNC updates cns state -func (client *Client) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest, scalarUnits cns.ScalarUnits) error { - returnCode := client.RestService.CreateOrUpdateNetworkContainerInternal(ncRequest, scalarUnits) +func (client *Client) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error { + returnCode := client.RestService.CreateOrUpdateNetworkContainerInternal(ncRequest) if returnCode != 0 { return fmt.Errorf("Failed to Create NC request: %+v, errorCode: %d", ncRequest, returnCode) @@ -24,8 +24,8 @@ func (client *Client) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerReque } // ReconcileNCState initializes cns state -func (client *Client) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo, scalarUnits cns.ScalarUnits) error { - returnCode := client.RestService.ReconcileNCState(ncRequest, podInfoByIP, scalarUnits) +func (client *Client) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo) error { + returnCode := client.RestService.ReconcileNCState(ncRequest, podInfoByIP) if returnCode != 0 { return fmt.Errorf("Failed to Reconcile ncState: ncRequest %+v, podInfoMap: %+v, errorCode: %d", *ncRequest, podInfoByIP, returnCode) diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index f2e04df8f5..b3bd90342d 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -15,23 +15,27 @@ import ( type CNSIPAMPoolMonitor struct { pendingRelease bool - cachedSpec nnc.NodeNetworkConfigSpec - cachedSecondaryIPConfigs map[string]cns.SecondaryIPConfig + cachedNNC nnc.NodeNetworkConfig + scalarUnits nnc.Scaler cns cns.HTTPService rc requestcontroller.RequestController - scalarUnits cns.ScalarUnits MinimumFreeIps int64 MaximumFreeIps int64 mu sync.RWMutex } -func NewCNSIPAMPoolMonitor(cnsService cns.HTTPService, requestController requestcontroller.RequestController) *CNSIPAMPoolMonitor { +func (pm *CNSIPAMPoolMonitor) UpdateReferences(cnsService cns.HTTPService, requestController requestcontroller.RequestController) { + pm.mu.Lock() + defer pm.mu.Unlock() + pm.cns = cnsService + pm.rc = requestController +} + +func NewCNSIPAMPoolMonitor() *CNSIPAMPoolMonitor { return &CNSIPAMPoolMonitor{ pendingRelease: false, - cns: cnsService, - rc: requestController, } } @@ -49,17 +53,18 @@ func (pm *CNSIPAMPoolMonitor) Start(ctx context.Context, poolMonitorRefreshMilli logger.Printf("[ipam-pool-monitor] Starting CNS IPAM Pool Monitor") ticker := time.NewTicker(time.Duration(poolMonitorRefreshMilliseconds) * time.Millisecond) - select { - case <-ctx.Done(): - return fmt.Errorf("CNS IPAM Pool Monitor received cancellation signal") - case <-ticker.C: - err := pm.Reconcile() - if err != nil { - logger.Printf("[ipam-pool-monitor] Reconcile failed with err %v", err) + + for { + select { + case <-ctx.Done(): + return fmt.Errorf("CNS IPAM Pool Monitor received cancellation signal") + case <-ticker.C: + err := pm.Reconcile() + if err != nil { + logger.Printf("[ipam-pool-monitor] Reconcile failed with err %v", err) + } } } - - return nil } func (pm *CNSIPAMPoolMonitor) Reconcile() error { @@ -67,15 +72,9 @@ func (pm *CNSIPAMPoolMonitor) Reconcile() error { allocatedPodIPCount := len(pm.cns.GetAllocatedIPConfigs()) pendingReleaseIPCount := len(pm.cns.GetPendingReleaseIPConfigs()) availableIPConfigCount := len(pm.cns.GetAvailableIPConfigs()) // TODO: add pending allocation count to real cns - freeIPConfigCount := int64(availableIPConfigCount + (int(pm.cachedSpec.RequestedIPCount) - cnsPodIPConfigCount)) - - logger.Printf("[ipam-pool-monitor] Checking pool for resize, Pool Size: %v, Goal Size: %v, BatchSize: %v, MinFree: %v, MaxFree:%v, Allocated: %v, Available: %v, Pending Release: %v, Free: %v", cnsPodIPConfigCount, pm.cachedSpec.RequestedIPCount, pm.scalarUnits.BatchSize, pm.MinimumFreeIps, pm.MaximumFreeIps, allocatedPodIPCount, availableIPConfigCount, pendingReleaseIPCount, freeIPConfigCount) + freeIPConfigCount := pm.cachedNNC.Spec.RequestedIPCount - int64(allocatedPodIPCount) - // if there's a pending change to the spec count, and the pending release state is nonzero, - // skip so we don't thrash the UpdateCRD - if pm.cachedSpec.RequestedIPCount != int64(len(pm.cns.GetPodIPConfigState())) && len(pm.cns.GetPendingReleaseIPConfigs()) > 0 { - return nil - } + logger.Printf("[ipam-pool-monitor] Checking pool for resize, Pool Size: %v, Goal Size: %v, BatchSize: %v, MinFree: %v, MaxFree:%v, Allocated: %v, Available: %v, Pending Release: %v, Free: %v", cnsPodIPConfigCount, pm.cachedNNC.Spec.RequestedIPCount, pm.scalarUnits.BatchSize, pm.MinimumFreeIps, pm.MaximumFreeIps, allocatedPodIPCount, availableIPConfigCount, pendingReleaseIPCount, freeIPConfigCount) switch { // pod count is increasing @@ -90,7 +89,7 @@ func (pm *CNSIPAMPoolMonitor) Reconcile() error { // CRD has reconciled CNS state, and target spec is now the same size as the state // free to remove the IP's from the CRD - case pm.pendingRelease && int(pm.cachedSpec.RequestedIPCount) == cnsPodIPConfigCount: + case pm.pendingRelease && int(pm.cachedNNC.Spec.RequestedIPCount) == cnsPodIPConfigCount: logger.Printf("[ipam-pool-monitor] Removing Pending Release IP's from CRD...") return pm.cleanPendingRelease() @@ -108,16 +107,16 @@ func (pm *CNSIPAMPoolMonitor) increasePoolSize() error { defer pm.mu.Unlock() var err error - pm.cachedSpec.RequestedIPCount += pm.scalarUnits.BatchSize + pm.cachedNNC.Spec.RequestedIPCount += pm.scalarUnits.BatchSize // pass nil map to CNStoCRDSpec because we don't want to modify the to be deleted ipconfigs - pm.cachedSpec, err = CNSToCRDSpec(nil, pm.cachedSpec.RequestedIPCount) + pm.cachedNNC.Spec, err = MarkIPsAsPendingInCRD(nil, pm.cachedNNC.Spec.RequestedIPCount) if err != nil { return err } - logger.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Requested IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) - return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) + logger.Printf("[ipam-pool-monitor] Increasing pool size, Current Pool Size: %v, Requested IP Count: %v, Pods with IP's:%v", len(pm.cns.GetPodIPConfigState()), pm.cachedNNC.Spec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) + return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedNNC.Spec) } func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { @@ -125,7 +124,7 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { defer pm.mu.Unlock() // TODO: Better handling here for negatives - pm.cachedSpec.RequestedIPCount -= pm.scalarUnits.BatchSize + pm.cachedNNC.Spec.RequestedIPCount -= pm.scalarUnits.BatchSize // mark n number of IP's as pending pendingIPAddresses, err := pm.cns.MarkIPsAsPending(int(pm.scalarUnits.BatchSize)) @@ -134,13 +133,13 @@ func (pm *CNSIPAMPoolMonitor) decreasePoolSize() error { } // convert the pending IP addresses to a spec - pm.cachedSpec, err = CNSToCRDSpec(pendingIPAddresses, pm.cachedSpec.RequestedIPCount) + pm.cachedNNC.Spec, err = MarkIPsAsPendingInCRD(pendingIPAddresses, pm.cachedNNC.Spec.RequestedIPCount) if err != nil { return err } pm.pendingRelease = true - logger.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Requested IP Count: %v, Pods with IP's: %v", len(pm.cns.GetPodIPConfigState()), pm.cachedSpec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) - return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) + logger.Printf("[ipam-pool-monitor] Decreasing pool size, Current Pool Size: %v, Requested IP Count: %v, Pods with IP's: %v", len(pm.cns.GetPodIPConfigState()), pm.cachedNNC.Spec.RequestedIPCount, len(pm.cns.GetAllocatedIPConfigs())) + return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedNNC.Spec) } // if cns pending ip release map is empty, request controller has already reconciled the CNS state, @@ -150,17 +149,17 @@ func (pm *CNSIPAMPoolMonitor) cleanPendingRelease() error { defer pm.mu.Unlock() var err error - pm.cachedSpec, err = CNSToCRDSpec(nil, pm.cachedSpec.RequestedIPCount) + pm.cachedNNC.Spec, err = MarkIPsAsPendingInCRD(nil, pm.cachedNNC.Spec.RequestedIPCount) if err != nil { logger.Printf("[ipam-pool-monitor] Failed to translate ") } pm.pendingRelease = false - return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedSpec) + return pm.rc.UpdateCRDSpec(context.Background(), pm.cachedNNC.Spec) } // CNSToCRDSpec translates CNS's map of Ips to be released and requested ip count into a CRD Spec -func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationStatus, ipCount int64) (nnc.NodeNetworkConfigSpec, error) { +func MarkIPsAsPendingInCRD(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationStatus, ipCount int64) (nnc.NodeNetworkConfigSpec, error) { var ( spec nnc.NodeNetworkConfigSpec uuid string @@ -180,19 +179,15 @@ func CNSToCRDSpec(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfigurationSt } // UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits -func (pm *CNSIPAMPoolMonitor) UpdatePoolMonitor(scalarUnits cns.ScalarUnits) error { +func (pm *CNSIPAMPoolMonitor) Update(nodeConfig nnc.NodeNetworkConfig) error { pm.mu.Lock() defer pm.mu.Unlock() - pm.scalarUnits = scalarUnits + pm.scalarUnits = nodeConfig.Status.Scaler pm.MinimumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) pm.MaximumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) - if pm.cns == nil { - return fmt.Errorf("Error Updating Pool Limits, reference to CNS is nil") - } - - pm.cachedSpec.RequestedIPCount = int64(len(pm.cns.GetPodIPConfigState())) + pm.cachedNNC.Spec.RequestedIPCount = int64(len(pm.cns.GetPodIPConfigState())) return nil } diff --git a/cns/logger/log.go b/cns/logger/log.go index a3ae8e3fab..bfccd9b3d5 100644 --- a/cns/logger/log.go +++ b/cns/logger/log.go @@ -137,9 +137,9 @@ func Request(tag string, request interface{}, err error) { var msg string if err == nil { - //msg = fmt.Sprintf("[%s] Received %T %+v.", tag, request, request) + msg = fmt.Sprintf("[%s] Received %T %+v.", tag, request, request) } else { - //msg = fmt.Sprintf("[%s] Failed to decode %T %+v %s.", tag, request, request, err.Error()) + msg = fmt.Sprintf("[%s] Failed to decode %T %+v %s.", tag, request, request, err.Error()) } sendTraceInternal(msg) @@ -154,7 +154,7 @@ func Response(tag string, response interface{}, returnCode int, returnStr string var msg string if err == nil && returnCode == 0 { - //msg = fmt.Sprintf("[%s] Sent %T %+v.", tag, response, response) + msg = fmt.Sprintf("[%s] Sent %T %+v.", tag, response, response) } else if err != nil { msg = fmt.Sprintf("[%s] Code:%s, %+v %s.", tag, returnStr, response, err.Error()) } else { diff --git a/cns/requestcontroller/kubecontroller/crdreconciler.go b/cns/requestcontroller/kubecontroller/crdreconciler.go index eb2ab0e9f6..79b37cbc59 100644 --- a/cns/requestcontroller/kubecontroller/crdreconciler.go +++ b/cns/requestcontroller/kubecontroller/crdreconciler.go @@ -15,9 +15,10 @@ import ( // CrdReconciler watches for CRD status changes type CrdReconciler struct { - KubeClient KubeClient - NodeName string - CNSClient cnsclient.APIClient + KubeClient KubeClient + NodeName string + CNSClient cnsclient.APIClient + IPAMPoolMonitor cns.IPAMPoolMonitor } // Reconcile is called on CRD status changes @@ -55,14 +56,14 @@ func (r *CrdReconciler) Reconcile(request reconcile.Request) (reconcile.Result, return reconcile.Result{}, err } - scalarUnits := cns.ScalarUnits{ - BatchSize: nodeNetConfig.Status.Scaler.BatchSize, - RequestThresholdPercent: nodeNetConfig.Status.Scaler.RequestThresholdPercent, - ReleaseThresholdPercent: nodeNetConfig.Status.Scaler.ReleaseThresholdPercent, + if err = r.CNSClient.CreateOrUpdateNC(ncRequest); err != nil { + logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) + // requeue + return reconcile.Result{}, err } - if err = r.CNSClient.CreateOrUpdateNC(ncRequest, scalarUnits); err != nil { - logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) + if err = r.IPAMPoolMonitor.Update(nodeNetConfig); err != nil { + logger.Errorf("[cns-rc] Error creating or updating IPAM Pool Monitor: %v", err) // requeue return reconcile.Result{}, err } diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go index bf2d3a7cae..8aae840b99 100644 --- a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go +++ b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go @@ -58,7 +58,7 @@ func GetKubeConfig() (*rest.Config, error) { } //NewCrdRequestController given a reference to CNS's HTTPRestService state, returns a crdRequestController struct -func NewCrdRequestController(restService *restserver.HTTPRestService, kubeconfig *rest.Config) (*crdRequestController, error) { +func NewCrdRequestController(restService *restserver.HTTPRestService, ipamPoolMonitor cns.IPAMPoolMonitor, kubeconfig *rest.Config) (*crdRequestController, error) { //Check that logger package has been intialized if logger.Log == nil { @@ -114,9 +114,10 @@ func NewCrdRequestController(restService *restserver.HTTPRestService, kubeconfig //Create reconciler crdreconciler := &CrdReconciler{ - KubeClient: mgr.GetClient(), - NodeName: nodeName, - CNSClient: httpClient, + KubeClient: mgr.GetClient(), + NodeName: nodeName, + CNSClient: httpClient, + IPAMPoolMonitor: ipamPoolMonitor, } // Setup manager with reconciler @@ -177,7 +178,6 @@ func (crdRC *crdRequestController) initCNS() error { cntxt context.Context ncRequest cns.CreateNetworkContainerRequest err error - scalarUnits cns.ScalarUnits ) cntxt = context.Background() @@ -190,15 +190,9 @@ func (crdRC *crdRequestController) initCNS() error { os.Exit(1) } - scalarUnits = cns.ScalarUnits{ - BatchSize: nodeNetConfig.Status.Scaler.BatchSize, - RequestThresholdPercent: nodeNetConfig.Status.Scaler.RequestThresholdPercent, - ReleaseThresholdPercent: nodeNetConfig.Status.Scaler.ReleaseThresholdPercent, - } - // If instance of crd is not found, pass nil to CNSClient if client.IgnoreNotFound(err) == nil { - return crdRC.CNSClient.ReconcileNCState(nil, nil, scalarUnits) + return crdRC.CNSClient.ReconcileNCState(nil, nil) } // If it's any other error, log it and return @@ -208,7 +202,7 @@ func (crdRC *crdRequestController) initCNS() error { // If there are no NCs, pass nil to CNSClient if len(nodeNetConfig.Status.NetworkContainers) == 0 { - return crdRC.CNSClient.ReconcileNCState(nil, nil, scalarUnits) + return crdRC.CNSClient.ReconcileNCState(nil, nil) } // Convert to CreateNetworkContainerRequest @@ -239,7 +233,7 @@ func (crdRC *crdRequestController) initCNS() error { } // Call cnsclient init cns passing those two things - return crdRC.CNSClient.ReconcileNCState(&ncRequest, podInfoByIP, scalarUnits) + return crdRC.CNSClient.ReconcileNCState(&ncRequest, podInfoByIP) } diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index f90c314fe9..03dfac4885 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -151,14 +151,14 @@ func (service *HTTPRestService) SyncNodeStatus(dncEP, infraVnet, nodeID string, } // This API will be called by CNS RequestController on CRD update. -func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIp map[string]cns.KubernetesPodInfo, scalarUnits cns.ScalarUnits) int { +func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIp map[string]cns.KubernetesPodInfo) int { // check if ncRequest is null, then return as there is no CRD state yet if ncRequest == nil { log.Logf("CNS starting with no NC state, podInfoMap count %d", len(podInfoByIp)) return Success } - returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest, scalarUnits) + returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest) // If the NC was created successfully, then reconcile the allocated pod state if returnCode != Success { @@ -194,7 +194,7 @@ func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkCon } // This API will be called by CNS RequestController on CRD update. -func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest, scalarUnits cns.ScalarUnits) int { +func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest) int { if req.NetworkContainerid == "" { logger.Errorf("[Azure CNS] Error. NetworkContainerid is empty") return NetworkContainerNotSpecified @@ -244,11 +244,5 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C logger.Errorf(returnMessage) } - err = service.PoolMonitor.UpdatePoolMonitor(scalarUnits) - if err != nil { - logger.Errorf("[Azure CNS] Error. Reference to CNS not set in IPAM Pool Monitor: %v", req) - return UnexpectedError - } - return returnCode } diff --git a/cns/service/main.go b/cns/service/main.go index 5a882cc6e9..40f05e3732 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -451,6 +451,8 @@ func main() { return } + poolMonitor := ipampoolmonitor.NewCNSIPAMPoolMonitor() + // Set orchestrator type orchestrator := cns.SetOrchestratorTypeRequest{ OrchestratorType: cns.KubernetesCRD, @@ -458,12 +460,14 @@ func main() { httpRestServiceImplementation.SetNodeOrchestrator(&orchestrator) // Get crd implementation of request controller - requestController, err = kubecontroller.NewCrdRequestController(httpRestServiceImplementation, kubeConfig) + requestController, err = kubecontroller.NewCrdRequestController(httpRestServiceImplementation, poolMonitor, kubeConfig) if err != nil { logger.Errorf("[Azure CNS] Failed to make crd request controller :%v", err) return } + poolMonitor.UpdateReferences(httpRestService, requestController) + //Start the RequestController which starts the reconcile loop requestControllerStopChannel := make(chan struct{}) defer close(requestControllerStopChannel) @@ -477,7 +481,6 @@ func main() { ctx, cancel := context.WithCancel(context.Background()) defer cancel() go func() { - poolMonitor := ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestService, requestController) httpRestServiceImplementation.PoolMonitor = poolMonitor if err := poolMonitor.Start(ctx, poolIPAMRefreshRateInMilliseconds); err != nil { logger.Errorf("[Azure CNS] Failed to start pool monitor with err: %v", err) From 711411ea81edec9fe176f67d2c7555b4e71dfae1 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Tue, 15 Sep 2020 11:34:11 -0700 Subject: [PATCH 15/18] fixed tests --- cns/cnsclient/cnsclient_test.go | 8 +-- cns/fakes/ipampoolmonitorfake.go | 4 +- cns/fakes/requestcontrollerfake.go | 55 +++++++-------- cns/ipampoolmonitor/ipampoolmonitor_test.go | 67 ++++++++++--------- .../crdrequestcontroller_test.go | 9 +-- cns/restserver/api_test.go | 2 - cns/restserver/internalapi_test.go | 12 ++-- cns/restserver/ipam_test.go | 2 - cns/restserver/restserver.go | 1 - cns/service/main.go | 1 - 10 files changed, 70 insertions(+), 91 deletions(-) diff --git a/cns/cnsclient/cnsclient_test.go b/cns/cnsclient/cnsclient_test.go index 70b0049e86..d5274ae9c6 100644 --- a/cns/cnsclient/cnsclient_test.go +++ b/cns/cnsclient/cnsclient_test.go @@ -15,7 +15,6 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" "github.com/Azure/azure-container-networking/cns/fakes" - "github.com/Azure/azure-container-networking/cns/ipampoolmonitor" "github.com/Azure/azure-container-networking/cns/logger" "github.com/Azure/azure-container-networking/cns/restserver" "github.com/Azure/azure-container-networking/log" @@ -39,7 +38,6 @@ var ( func addTestStateToRestServer(t *testing.T, secondaryIps []string) { var ipConfig cns.IPConfiguration - var scalarUnits cns.ScalarUnits ipConfig.DNSServers = dnsservers ipConfig.GatewayIPAddress = gatewayIp var ipSubnet cns.IPSubnet @@ -63,7 +61,7 @@ func addTestStateToRestServer(t *testing.T, secondaryIps []string) { SecondaryIPConfigs: secondaryIPConfigs, } - returnCode := svc.CreateOrUpdateNetworkContainerInternal(req, scalarUnits) + returnCode := svc.CreateOrUpdateNetworkContainerInternal(req) if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } @@ -124,10 +122,6 @@ func TestMain(m *testing.M) { svc = httpRestService.(*restserver.HTTPRestService) svc.Name = "cns-test-server" - fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake(fakecns, cns.ScalarUnits{BatchSize: 10, RequestThresholdPercent: 100, ReleaseThresholdPercent: 30}, 16) - svc.PoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(fakecns, fakerc) - if err != nil { logger.Errorf("Failed to create CNS object, err:%v.\n", err) return diff --git a/cns/fakes/ipampoolmonitorfake.go b/cns/fakes/ipampoolmonitorfake.go index e864611776..cafc903f23 100644 --- a/cns/fakes/ipampoolmonitorfake.go +++ b/cns/fakes/ipampoolmonitorfake.go @@ -3,7 +3,7 @@ package fakes import ( "context" - "github.com/Azure/azure-container-networking/cns" + nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) type IPAMPoolMonitorFake struct{} @@ -16,7 +16,7 @@ func (ipm *IPAMPoolMonitorFake) Start(ctx context.Context, poolMonitorRefreshMil return nil } -func (ipm *IPAMPoolMonitorFake) UpdatePoolMonitor(scalarUnits cns.ScalarUnits) error { +func (ipm *IPAMPoolMonitorFake) Update(nnc nnc.NodeNetworkConfig) error { return nil } diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index 0d293dd8d6..dba1ec79a1 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -10,35 +10,26 @@ import ( ) type RequestControllerFake struct { - fakecns *HTTPServiceFake - testScalarUnits cns.ScalarUnits - desiredSpec nnc.NodeNetworkConfigSpec - currentStatus nnc.NodeNetworkConfigStatus - ip net.IP + fakecns *HTTPServiceFake + cachedCRD nnc.NodeNetworkConfig + ip net.IP } -func NewRequestControllerFake(cnsService *HTTPServiceFake, scalarUnits cns.ScalarUnits, numberOfIPConfigs int) *RequestControllerFake { +func NewRequestControllerFake(cnsService *HTTPServiceFake, ipamPoolMonitor cns.IPAMPoolMonitor, scalar nnc.Scaler, subnetAddressSpace string, numberOfIPConfigs int) *RequestControllerFake { rc := &RequestControllerFake{ - fakecns: cnsService, - testScalarUnits: scalarUnits, - currentStatus: nnc.NodeNetworkConfigStatus{Scaler: nnc.Scaler{ - BatchSize: scalarUnits.BatchSize, - RequestThresholdPercent: scalarUnits.RequestThresholdPercent, - ReleaseThresholdPercent: scalarUnits.ReleaseThresholdPercent, - }, - NetworkContainers: []nnc.NetworkContainer{ - { - IPAssignments: []nnc.IPAssignment{}, - SubnetAddressSpace: "10.0.0.0/8", - }, + fakecns: cnsService, + cachedCRD: nnc.NodeNetworkConfig{ + Spec: nnc.NodeNetworkConfigSpec{}, + Status: nnc.NodeNetworkConfigStatus{ + Scaler: scalar, + NetworkContainers: []nnc.NetworkContainer{nnc.NetworkContainer{ + SubnetAddressSpace: subnetAddressSpace, + }}, }, }, - desiredSpec: nnc.NodeNetworkConfigSpec{ - RequestedIPCount: int64(numberOfIPConfigs), - }, } - rc.ip, _, _ = net.ParseCIDR(rc.currentStatus.NetworkContainers[0].SubnetAddressSpace) + rc.ip, _, _ = net.ParseCIDR(subnetAddressSpace) return rc } @@ -51,7 +42,7 @@ func (rc *RequestControllerFake) CarveIPConfigsAndAddToStatusAndCNS(numberOfIPCo Name: uuid.New().String(), IP: rc.ip.String(), } - rc.currentStatus.NetworkContainers[0].IPAssignments = append(rc.currentStatus.NetworkContainers[0].IPAssignments, ipconfigCRD) + rc.cachedCRD.Status.NetworkContainers[0].IPAssignments = append(rc.cachedCRD.Status.NetworkContainers[0].IPAssignments, ipconfigCRD) ipconfigCNS := cns.IPConfigurationStatus{ ID: ipconfigCRD.Name, @@ -64,7 +55,7 @@ func (rc *RequestControllerFake) CarveIPConfigsAndAddToStatusAndCNS(numberOfIPCo } rc.fakecns.IPStateManager.AddIPConfigs(cnsIPConfigs) - rc.desiredSpec.RequestedIPCount = int64(len(cnsIPConfigs)) + rc.cachedCRD.Spec.RequestedIPCount = int64(len(cnsIPConfigs)) return cnsIPConfigs } @@ -74,7 +65,7 @@ func (rc *RequestControllerFake) StartRequestController(exitChan <-chan struct{} } func (rc *RequestControllerFake) UpdateCRDSpec(cntxt context.Context, desiredSpec nnc.NodeNetworkConfigSpec) error { - rc.desiredSpec = desiredSpec + rc.cachedCRD.Spec = desiredSpec return nil } @@ -85,7 +76,7 @@ func remove(slice []nnc.IPAssignment, s int) []nnc.IPAssignment { func (rc *RequestControllerFake) Reconcile() error { - diff := int(rc.desiredSpec.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState()) + diff := int(rc.cachedCRD.Spec.RequestedIPCount) - len(rc.fakecns.GetPodIPConfigState()) if diff > 0 { // carve the difference of test IPs and add them to CNS, assume dnc has populated the CRD status @@ -96,29 +87,29 @@ func (rc *RequestControllerFake) Reconcile() error { // Assume DNC has removed the IPConfigs from the status // mimic DNC removing IPConfigs from the CRD - for _, notInUseIPConfigName := range rc.desiredSpec.IPsNotInUse { + for _, notInUseIPConfigName := range rc.cachedCRD.Spec.IPsNotInUse { // remove ipconfig from status index := 0 - for _, ipconfig := range rc.currentStatus.NetworkContainers[0].IPAssignments { + for _, ipconfig := range rc.cachedCRD.Status.NetworkContainers[0].IPAssignments { if notInUseIPConfigName == ipconfig.Name { break } index++ } - rc.currentStatus.NetworkContainers[0].IPAssignments = remove(rc.currentStatus.NetworkContainers[0].IPAssignments, index) + rc.cachedCRD.Status.NetworkContainers[0].IPAssignments = remove(rc.cachedCRD.Status.NetworkContainers[0].IPAssignments, index) } // remove ipconfig from CNS - rc.fakecns.IPStateManager.RemovePendingReleaseIPConfigs(rc.desiredSpec.IPsNotInUse) + rc.fakecns.IPStateManager.RemovePendingReleaseIPConfigs(rc.cachedCRD.Spec.IPsNotInUse) // empty the not in use ip's from the spec - rc.desiredSpec.IPsNotInUse = []string{} + rc.cachedCRD.Spec.IPsNotInUse = []string{} } // update - rc.fakecns.PoolMonitor.UpdatePoolMonitor(rc.testScalarUnits) + rc.fakecns.PoolMonitor.Update(rc.cachedCRD) return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 01aae1ffe8..2321ef6fec 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -4,23 +4,26 @@ import ( "log" "testing" - "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/fakes" "github.com/Azure/azure-container-networking/cns/logger" + nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, releaseThresholdPercent int) (*fakes.HTTPServiceFake, *fakes.RequestControllerFake, *CNSIPAMPoolMonitor) { logger.InitLogger("testlogs", 0, 0, "./") - scalarUnits := cns.ScalarUnits{ + scalarUnits := nnc.Scaler{ BatchSize: int64(batchSize), RequestThresholdPercent: int64(requestThresholdPercent), ReleaseThresholdPercent: int64(releaseThresholdPercent), } + subnetaddresspace := "10.0.0.0/8" + poolmonitor := NewCNSIPAMPoolMonitor() fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, initialIPConfigCount) - poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc) + fakerc := fakes.NewRequestControllerFake(fakecns, poolmonitor, scalarUnits, subnetaddresspace, initialIPConfigCount) + poolmonitor.UpdateReferences(fakecns, fakerc) + fakecns.PoolMonitor = poolmonitor fakerc.Reconcile() @@ -51,8 +54,8 @@ func TestPoolSizeIncrease(t *testing.T) { } // ensure pool monitor has reached quorum with cns - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // request controller reconciles, carves new IP's from the test subnet and adds to CNS state @@ -69,8 +72,8 @@ func TestPoolSizeIncrease(t *testing.T) { } // ensure pool monitor has reached quorum with cns - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // make sure IPConfig state size reflects the new pool size @@ -78,7 +81,7 @@ func TestPoolSizeIncrease(t *testing.T) { t.Fatalf("CNS Pod IPConfig state count doesn't match, expected: %v, actual %v", len(fakecns.GetPodIPConfigState()), initialIPConfigCount+(1*batchSize)) } - t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) + t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) } func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { @@ -116,8 +119,8 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { } // ensure pool monitor has reached quorum with cns - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // request controller reconciles, carves new IP's from the test subnet and adds to CNS state @@ -139,11 +142,11 @@ func TestPoolIncreaseDoesntChangeWhenIncreaseIsAlreadyInProgress(t *testing.T) { } // ensure pool monitor has reached quorum with cns - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } - t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) + t.Logf("Pool size %v, Target pool size %v, Allocated IP's %v, ", len(fakecns.GetPodIPConfigState()), poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetAllocatedIPConfigs())) } func TestPoolSizeIncreaseIdempotency(t *testing.T) { @@ -172,8 +175,8 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { } // ensure pool monitor has increased batch size - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } // reconcile pool monitor a second time, then verify requested ip count is still the same @@ -183,8 +186,8 @@ func TestPoolSizeIncreaseIdempotency(t *testing.T) { } // ensure pool monitor requested pool size is unchanged as request controller hasn't reconciled yet - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { - t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedSpec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount+(1*batchSize)) { + t.Fatalf("Pool monitor target IP count doesn't match CNS pool state after reconcile: %v, actual %v", poolmonitor.cachedNNC.Spec.RequestedIPCount, len(fakecns.GetPodIPConfigState())) } } @@ -226,8 +229,8 @@ func TestPoolDecrease(t *testing.T) { } // ensure that the adjusted spec is smaller than the initial pool size - if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // reconcile the fake request controller @@ -237,8 +240,8 @@ func TestPoolDecrease(t *testing.T) { } // Ensure the size of the requested spec is still the same - if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } return @@ -270,13 +273,13 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { } // Ensure the size of the requested spec is still the same - if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected IP's not in use be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected IP's not in use be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Ensure the request ipcount is now one batch size smaller than the inital IP count - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Update pods with IP count, ensure pool monitor stays the same until request controller reconciles @@ -286,13 +289,13 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { } // Ensure the size of the requested spec is still the same - if len(poolmonitor.cachedSpec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected IP's not in use to be one batch size smaller after reconcile, and not change after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { + t.Fatalf("Expected IP's not in use to be one batch size smaller after reconcile, and not change after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } // Ensure the request ipcount is now one batch size smaller than the inital IP count - if poolmonitor.cachedSpec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, and not change after existing call, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + if poolmonitor.cachedNNC.Spec.RequestedIPCount != int64(initialIPConfigCount-batchSize) { + t.Fatalf("Expected pool size to be one batch size smaller after reconcile, and not change after existing call, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } err = fakerc.Reconcile() @@ -306,7 +309,7 @@ func TestPoolSizeDecreaseWhenDecreaseHasAlreadyBeenRequested(t *testing.T) { } // Ensure the spec doesn't have any IPsNotInUse after request controller has reconciled - if len(poolmonitor.cachedSpec.IPsNotInUse) != 0 { - t.Fatalf("Expected IP's not in use to be 0 after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedSpec.IPsNotInUse)) + if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != 0 { + t.Fatalf("Expected IP's not in use to be 0 after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) } } diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go index 57113fcc46..30ccf0bac3 100644 --- a/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go +++ b/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/cns/fakes" "github.com/Azure/azure-container-networking/cns/logger" nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" corev1 "k8s.io/api/core/v1" @@ -99,12 +100,12 @@ type MockCNSClient struct { } // we're just testing that reconciler interacts with CNS on Reconcile(). -func (mi *MockCNSClient) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest, scalarUnits cns.ScalarUnits) error { +func (mi *MockCNSClient) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error { mi.MockCNSUpdated = true return nil } -func (mi *MockCNSClient) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo, scalarUnits cns.ScalarUnits) error { +func (mi *MockCNSClient) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo) error { mi.MockCNSInitialized = true mi.Pods = podInfoByIP mi.NCRequest = ncRequest @@ -167,7 +168,7 @@ func (mc *MockDirectAPIClient) ListPods(cntxt context.Context, namespace, node s } func TestNewCrdRequestController(t *testing.T) { //Test making request controller without logger initialized, should fail - _, err := NewCrdRequestController(nil, nil) + _, err := NewCrdRequestController(nil, fakes.NewIPAMPoolMonitorFake(), nil) if err == nil { t.Fatalf("Expected error when making NewCrdRequestController without initializing logger, got nil error") } else if !strings.Contains(err.Error(), "logger") { @@ -187,7 +188,7 @@ func TestNewCrdRequestController(t *testing.T) { } }() - _, err = NewCrdRequestController(nil, nil) + _, err = NewCrdRequestController(nil, fakes.NewIPAMPoolMonitorFake(), nil) if err == nil { t.Fatalf("Expected error when making NewCrdRequestController without setting " + nodeNameEnvVar + " env var, got nil error") } else if !strings.Contains(err.Error(), nodeNameEnvVar) { diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 2f2b3430ad..875d32d1ee 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -677,8 +677,6 @@ func startService() { } } - service.(*HTTPRestService).PoolMonitor = fakes.NewIPAMPoolMonitorFake() - // Get the internal http mux as test hook. mux = service.(*HTTPRestService).Listener.GetMux() } diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index e565c1ed60..c8c3631de1 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -36,13 +36,12 @@ func TestCreateOrUpdateNetworkContainerInternal(t *testing.T) { func TestReconcileNCWithEmptyState(t *testing.T) { restartService() - var scalarUnits cns.ScalarUnits setEnv(t) setOrchestratorTypeInternal(cns.KubernetesCRD) expectedNcCount := len(svc.state.ContainerStatus) expectedAllocatedPods := make(map[string]cns.KubernetesPodInfo) - returnCode := svc.ReconcileNCState(nil, expectedAllocatedPods, scalarUnits) + returnCode := svc.ReconcileNCState(nil, expectedAllocatedPods) if returnCode != Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } @@ -52,7 +51,6 @@ func TestReconcileNCWithEmptyState(t *testing.T) { func TestReconcileNCWithExistingState(t *testing.T) { restartService() - var scalarUnits cns.ScalarUnits setEnv(t) setOrchestratorTypeInternal(cns.KubernetesCRD) @@ -80,7 +78,7 @@ func TestReconcileNCWithExistingState(t *testing.T) { } expectedNcCount := len(svc.state.ContainerStatus) - returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods, scalarUnits) + returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods) if returnCode != Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } @@ -90,7 +88,6 @@ func TestReconcileNCWithExistingState(t *testing.T) { func TestReconcileNCWithSystemPods(t *testing.T) { restartService() - var scalarUnits cns.ScalarUnits setEnv(t) setOrchestratorTypeInternal(cns.KubernetesCRD) @@ -119,7 +116,7 @@ func TestReconcileNCWithSystemPods(t *testing.T) { } expectedNcCount := len(svc.state.ContainerStatus) - returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods, scalarUnits) + returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods) if returnCode != Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } @@ -185,8 +182,7 @@ func validateCreateOrUpdateNCInternal(t *testing.T, secondaryIpCount int) { func createAndValidateNCRequest(t *testing.T, secondaryIPConfigs map[string]cns.SecondaryIPConfig, ncId string) { req := generateNetworkContainerRequest(secondaryIPConfigs, ncId) - var scalarUnits cns.ScalarUnits - returnCode := svc.CreateOrUpdateNetworkContainerInternal(req, scalarUnits) + returnCode := svc.CreateOrUpdateNetworkContainerInternal(req) if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 5b26e8f86a..94fea1993e 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -44,8 +44,6 @@ func getTestService() *HTTPRestService { httpsvc, _ := NewHTTPRestService(&config, fakes.NewFakeImdsClient()) svc = httpsvc.(*HTTPRestService) - svc.PoolMonitor = fakes.NewIPAMPoolMonitorFake() - setOrchestratorTypeInternal(cns.KubernetesCRD) return svc diff --git a/cns/restserver/restserver.go b/cns/restserver/restserver.go index 17e2d050aa..a76ba8948e 100644 --- a/cns/restserver/restserver.go +++ b/cns/restserver/restserver.go @@ -43,7 +43,6 @@ type HTTPRestService struct { PodIPConfigState map[string]cns.IPConfigurationStatus // seondaryipid(uuid) is key AllocatedIPCount map[string]allocatedIPCount // key - ncid routingTable *routes.RoutingTable - PoolMonitor cns.IPAMPoolMonitor store store.KeyValueStore state *httpRestServiceState sync.RWMutex diff --git a/cns/service/main.go b/cns/service/main.go index 40f05e3732..556ac07047 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -481,7 +481,6 @@ func main() { ctx, cancel := context.WithCancel(context.Background()) defer cancel() go func() { - httpRestServiceImplementation.PoolMonitor = poolMonitor if err := poolMonitor.Start(ctx, poolIPAMRefreshRateInMilliseconds); err != nil { logger.Errorf("[Azure CNS] Failed to start pool monitor with err: %v", err) } From 96758b17a5645b91cacd0200317d4cdeda340f00 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Tue, 15 Sep 2020 12:35:44 -0700 Subject: [PATCH 16/18] test fix --- cns/fakes/requestcontrollerfake.go | 2 ++ cns/ipampoolmonitor/ipampoolmonitor.go | 2 +- cns/ipampoolmonitor/ipampoolmonitor_test.go | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index dba1ec79a1..e9ca9f7ca1 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -31,6 +31,8 @@ func NewRequestControllerFake(cnsService *HTTPServiceFake, ipamPoolMonitor cns.I rc.ip, _, _ = net.ParseCIDR(subnetAddressSpace) + rc.CarveIPConfigsAndAddToStatusAndCNS(numberOfIPConfigs) + return rc } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index b3bd90342d..be56c050dd 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -187,7 +187,7 @@ func (pm *CNSIPAMPoolMonitor) Update(nodeConfig nnc.NodeNetworkConfig) error { pm.MinimumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) pm.MaximumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) - pm.cachedNNC.Spec.RequestedIPCount = int64(len(pm.cns.GetPodIPConfigState())) + pm.cachedNNC = nodeConfig return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 2321ef6fec..0b465622c7 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -240,8 +240,8 @@ func TestPoolDecrease(t *testing.T) { } // Ensure the size of the requested spec is still the same - if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != (initialIPConfigCount - batchSize) { - t.Fatalf("Expected pool size to be one batch size smaller after reconcile, expected %v, actual %v", (initialIPConfigCount - batchSize), len(poolmonitor.cachedNNC.Spec.IPsNotInUse)) + if len(poolmonitor.cachedNNC.Spec.IPsNotInUse) != 0 { + t.Fatalf("Expected IPsNotInUse to be 0 after request controller reconcile, actual %v", poolmonitor.cachedNNC.Spec.IPsNotInUse) } return From 4d67241184e1b0e3bff79d64cdd6899c22d8c974 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Tue, 15 Sep 2020 16:38:30 -0700 Subject: [PATCH 17/18] ipampoolmonitor in cns not request controller --- cns/api.go | 2 +- cns/cnsclient/apiclient.go | 9 ++++++--- cns/cnsclient/cnsclient_test.go | 6 +++++- cns/cnsclient/httpapi/client.go | 9 +++++---- cns/fakes/cnsfake.go | 15 +++++++++++++++ cns/fakes/ipampoolmonitorfake.go | 2 +- cns/fakes/requestcontrollerfake.go | 4 ++-- cns/ipampoolmonitor/ipampoolmonitor.go | 17 ++++++----------- cns/ipampoolmonitor/ipampoolmonitor_test.go | 6 +++--- .../kubecontroller/crdreconciler.go | 8 +------- .../kubecontroller/crdrequestcontroller.go | 15 +++++++-------- .../kubecontroller/crdrequestcontroller_test.go | 9 ++++----- cns/restserver/api_test.go | 2 ++ cns/restserver/internalapi.go | 14 +++++++++++--- cns/restserver/internalapi_test.go | 13 +++++++++---- cns/restserver/ipam_test.go | 2 +- cns/restserver/restserver.go | 1 + cns/service/main.go | 9 ++++----- 18 files changed, 84 insertions(+), 59 deletions(-) diff --git a/cns/api.go b/cns/api.go index 84af52e082..0c06500e28 100644 --- a/cns/api.go +++ b/cns/api.go @@ -166,7 +166,7 @@ type NodeConfiguration struct { } type IPAMPoolMonitor interface { Start(ctx context.Context, poolMonitorRefreshMilliseconds int) error - Update(nodeConfig nnc.NodeNetworkConfig) error + Update(scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error } // Response describes generic response from CNS. diff --git a/cns/cnsclient/apiclient.go b/cns/cnsclient/apiclient.go index c7ce324d71..6982926790 100644 --- a/cns/cnsclient/apiclient.go +++ b/cns/cnsclient/apiclient.go @@ -1,9 +1,12 @@ package cnsclient -import "github.com/Azure/azure-container-networking/cns" +import ( + "github.com/Azure/azure-container-networking/cns" + nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" +) // APIClient interface to update cns state type APIClient interface { - ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.KubernetesPodInfo) error - CreateOrUpdateNC(nc cns.CreateNetworkContainerRequest) error + ReconcileNCState(nc *cns.CreateNetworkContainerRequest, pods map[string]cns.KubernetesPodInfo, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error + CreateOrUpdateNC(nc cns.CreateNetworkContainerRequest, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error } diff --git a/cns/cnsclient/cnsclient_test.go b/cns/cnsclient/cnsclient_test.go index d5274ae9c6..79858da075 100644 --- a/cns/cnsclient/cnsclient_test.go +++ b/cns/cnsclient/cnsclient_test.go @@ -30,6 +30,10 @@ const ( gatewayIp = "10.0.0.1" subnetPrfixLength = 24 dockerContainerType = cns.Docker + releasePercent = 50 + requestPercent = 100 + batchSize = 10 + initPoolSize = 10 ) var ( @@ -61,7 +65,7 @@ func addTestStateToRestServer(t *testing.T, secondaryIps []string) { SecondaryIPConfigs: secondaryIPConfigs, } - returnCode := svc.CreateOrUpdateNetworkContainerInternal(req) + returnCode := svc.CreateOrUpdateNetworkContainerInternal(req, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } diff --git a/cns/cnsclient/httpapi/client.go b/cns/cnsclient/httpapi/client.go index 04a49f9740..d3add84ce8 100644 --- a/cns/cnsclient/httpapi/client.go +++ b/cns/cnsclient/httpapi/client.go @@ -5,6 +5,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/restserver" + nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) // Client implements APIClient interface. Used to update CNS state @@ -13,8 +14,8 @@ type Client struct { } // CreateOrUpdateNC updates cns state -func (client *Client) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error { - returnCode := client.RestService.CreateOrUpdateNetworkContainerInternal(ncRequest) +func (client *Client) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error { + returnCode := client.RestService.CreateOrUpdateNetworkContainerInternal(ncRequest, scalar, spec) if returnCode != 0 { return fmt.Errorf("Failed to Create NC request: %+v, errorCode: %d", ncRequest, returnCode) @@ -24,8 +25,8 @@ func (client *Client) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerReque } // ReconcileNCState initializes cns state -func (client *Client) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo) error { - returnCode := client.RestService.ReconcileNCState(ncRequest, podInfoByIP) +func (client *Client) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error { + returnCode := client.RestService.ReconcileNCState(ncRequest, podInfoByIP, scalar, spec) if returnCode != 0 { return fmt.Errorf("Failed to Reconcile ncState: ncRequest %+v, podInfoMap: %+v, errorCode: %d", *ncRequest, podInfoByIP, returnCode) diff --git a/cns/fakes/cnsfake.go b/cns/fakes/cnsfake.go index 367c52660f..80afdad04d 100644 --- a/cns/fakes/cnsfake.go +++ b/cns/fakes/cnsfake.go @@ -7,6 +7,7 @@ import ( "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/common" + nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) // available IP's stack @@ -17,6 +18,20 @@ type StringStack struct { items []string } +func NewFakeScalar(releaseThreshold, requestThreshold, batchSize int) nnc.Scaler { + return nnc.Scaler{ + BatchSize: int64(batchSize), + ReleaseThresholdPercent: int64(releaseThreshold), + RequestThresholdPercent: int64(requestThreshold), + } +} + +func NewFakeNodeNetworkConfigSpec(requestedIPCount int) nnc.NodeNetworkConfigSpec { + return nnc.NodeNetworkConfigSpec{ + RequestedIPCount: int64(requestedIPCount), + } +} + func NewStack() *StringStack { return &StringStack{sync.Mutex{}, make([]string, 0)} } diff --git a/cns/fakes/ipampoolmonitorfake.go b/cns/fakes/ipampoolmonitorfake.go index cafc903f23..03c707d6c7 100644 --- a/cns/fakes/ipampoolmonitorfake.go +++ b/cns/fakes/ipampoolmonitorfake.go @@ -16,7 +16,7 @@ func (ipm *IPAMPoolMonitorFake) Start(ctx context.Context, poolMonitorRefreshMil return nil } -func (ipm *IPAMPoolMonitorFake) Update(nnc nnc.NodeNetworkConfig) error { +func (ipm *IPAMPoolMonitorFake) Update(scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error { return nil } diff --git a/cns/fakes/requestcontrollerfake.go b/cns/fakes/requestcontrollerfake.go index e9ca9f7ca1..6b46c6f2c0 100644 --- a/cns/fakes/requestcontrollerfake.go +++ b/cns/fakes/requestcontrollerfake.go @@ -15,7 +15,7 @@ type RequestControllerFake struct { ip net.IP } -func NewRequestControllerFake(cnsService *HTTPServiceFake, ipamPoolMonitor cns.IPAMPoolMonitor, scalar nnc.Scaler, subnetAddressSpace string, numberOfIPConfigs int) *RequestControllerFake { +func NewRequestControllerFake(cnsService *HTTPServiceFake, scalar nnc.Scaler, subnetAddressSpace string, numberOfIPConfigs int) *RequestControllerFake { rc := &RequestControllerFake{ fakecns: cnsService, cachedCRD: nnc.NodeNetworkConfig{ @@ -111,7 +111,7 @@ func (rc *RequestControllerFake) Reconcile() error { } // update - rc.fakecns.PoolMonitor.Update(rc.cachedCRD) + rc.fakecns.PoolMonitor.Update(rc.cachedCRD.Status.Scaler, rc.cachedCRD.Spec) return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor.go b/cns/ipampoolmonitor/ipampoolmonitor.go index be56c050dd..716ea3ce84 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor.go +++ b/cns/ipampoolmonitor/ipampoolmonitor.go @@ -26,16 +26,11 @@ type CNSIPAMPoolMonitor struct { mu sync.RWMutex } -func (pm *CNSIPAMPoolMonitor) UpdateReferences(cnsService cns.HTTPService, requestController requestcontroller.RequestController) { - pm.mu.Lock() - defer pm.mu.Unlock() - pm.cns = cnsService - pm.rc = requestController -} - -func NewCNSIPAMPoolMonitor() *CNSIPAMPoolMonitor { +func NewCNSIPAMPoolMonitor(cns cns.HTTPService, rc requestcontroller.RequestController) *CNSIPAMPoolMonitor { return &CNSIPAMPoolMonitor{ pendingRelease: false, + cns: cns, + rc: rc, } } @@ -179,15 +174,15 @@ func MarkIPsAsPendingInCRD(toBeDeletedSecondaryIPConfigs map[string]cns.IPConfig } // UpdatePoolLimitsTransacted called by request controller on reconcile to set the batch size limits -func (pm *CNSIPAMPoolMonitor) Update(nodeConfig nnc.NodeNetworkConfig) error { +func (pm *CNSIPAMPoolMonitor) Update(scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error { pm.mu.Lock() defer pm.mu.Unlock() - pm.scalarUnits = nodeConfig.Status.Scaler + pm.scalarUnits = scalar pm.MinimumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.RequestThresholdPercent) / 100)) pm.MaximumFreeIps = int64(float64(pm.scalarUnits.BatchSize) * (float64(pm.scalarUnits.ReleaseThresholdPercent) / 100)) - pm.cachedNNC = nodeConfig + pm.cachedNNC.Spec = spec return nil } diff --git a/cns/ipampoolmonitor/ipampoolmonitor_test.go b/cns/ipampoolmonitor/ipampoolmonitor_test.go index 0b465622c7..934019bb34 100644 --- a/cns/ipampoolmonitor/ipampoolmonitor_test.go +++ b/cns/ipampoolmonitor/ipampoolmonitor_test.go @@ -19,10 +19,10 @@ func initFakes(batchSize, initialIPConfigCount, requestThresholdPercent, release } subnetaddresspace := "10.0.0.0/8" - poolmonitor := NewCNSIPAMPoolMonitor() fakecns := fakes.NewHTTPServiceFake() - fakerc := fakes.NewRequestControllerFake(fakecns, poolmonitor, scalarUnits, subnetaddresspace, initialIPConfigCount) - poolmonitor.UpdateReferences(fakecns, fakerc) + fakerc := fakes.NewRequestControllerFake(fakecns, scalarUnits, subnetaddresspace, initialIPConfigCount) + + poolmonitor := NewCNSIPAMPoolMonitor(fakecns, fakerc) fakecns.PoolMonitor = poolmonitor diff --git a/cns/requestcontroller/kubecontroller/crdreconciler.go b/cns/requestcontroller/kubecontroller/crdreconciler.go index 79b37cbc59..a4ecc8281e 100644 --- a/cns/requestcontroller/kubecontroller/crdreconciler.go +++ b/cns/requestcontroller/kubecontroller/crdreconciler.go @@ -56,18 +56,12 @@ func (r *CrdReconciler) Reconcile(request reconcile.Request) (reconcile.Result, return reconcile.Result{}, err } - if err = r.CNSClient.CreateOrUpdateNC(ncRequest); err != nil { + if err = r.CNSClient.CreateOrUpdateNC(ncRequest, nodeNetConfig.Status.Scaler, nodeNetConfig.Spec); err != nil { logger.Errorf("[cns-rc] Error creating or updating NC in reconcile: %v", err) // requeue return reconcile.Result{}, err } - if err = r.IPAMPoolMonitor.Update(nodeNetConfig); err != nil { - logger.Errorf("[cns-rc] Error creating or updating IPAM Pool Monitor: %v", err) - // requeue - return reconcile.Result{}, err - } - return reconcile.Result{}, err } diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go index 8aae840b99..79a5386566 100644 --- a/cns/requestcontroller/kubecontroller/crdrequestcontroller.go +++ b/cns/requestcontroller/kubecontroller/crdrequestcontroller.go @@ -58,7 +58,7 @@ func GetKubeConfig() (*rest.Config, error) { } //NewCrdRequestController given a reference to CNS's HTTPRestService state, returns a crdRequestController struct -func NewCrdRequestController(restService *restserver.HTTPRestService, ipamPoolMonitor cns.IPAMPoolMonitor, kubeconfig *rest.Config) (*crdRequestController, error) { +func NewCrdRequestController(restService *restserver.HTTPRestService, kubeconfig *rest.Config) (*crdRequestController, error) { //Check that logger package has been intialized if logger.Log == nil { @@ -114,10 +114,9 @@ func NewCrdRequestController(restService *restserver.HTTPRestService, ipamPoolMo //Create reconciler crdreconciler := &CrdReconciler{ - KubeClient: mgr.GetClient(), - NodeName: nodeName, - CNSClient: httpClient, - IPAMPoolMonitor: ipamPoolMonitor, + KubeClient: mgr.GetClient(), + NodeName: nodeName, + CNSClient: httpClient, } // Setup manager with reconciler @@ -192,7 +191,7 @@ func (crdRC *crdRequestController) initCNS() error { // If instance of crd is not found, pass nil to CNSClient if client.IgnoreNotFound(err) == nil { - return crdRC.CNSClient.ReconcileNCState(nil, nil) + return crdRC.CNSClient.ReconcileNCState(nil, nil, nodeNetConfig.Status.Scaler, nodeNetConfig.Spec) } // If it's any other error, log it and return @@ -202,7 +201,7 @@ func (crdRC *crdRequestController) initCNS() error { // If there are no NCs, pass nil to CNSClient if len(nodeNetConfig.Status.NetworkContainers) == 0 { - return crdRC.CNSClient.ReconcileNCState(nil, nil) + return crdRC.CNSClient.ReconcileNCState(nil, nil, nodeNetConfig.Status.Scaler, nodeNetConfig.Spec) } // Convert to CreateNetworkContainerRequest @@ -233,7 +232,7 @@ func (crdRC *crdRequestController) initCNS() error { } // Call cnsclient init cns passing those two things - return crdRC.CNSClient.ReconcileNCState(&ncRequest, podInfoByIP) + return crdRC.CNSClient.ReconcileNCState(&ncRequest, podInfoByIP, nodeNetConfig.Status.Scaler, nodeNetConfig.Spec) } diff --git a/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go b/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go index 30ccf0bac3..7f280a7907 100644 --- a/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go +++ b/cns/requestcontroller/kubecontroller/crdrequestcontroller_test.go @@ -10,7 +10,6 @@ import ( "testing" "github.com/Azure/azure-container-networking/cns" - "github.com/Azure/azure-container-networking/cns/fakes" "github.com/Azure/azure-container-networking/cns/logger" nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" corev1 "k8s.io/api/core/v1" @@ -100,12 +99,12 @@ type MockCNSClient struct { } // we're just testing that reconciler interacts with CNS on Reconcile(). -func (mi *MockCNSClient) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest) error { +func (mi *MockCNSClient) CreateOrUpdateNC(ncRequest cns.CreateNetworkContainerRequest, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error { mi.MockCNSUpdated = true return nil } -func (mi *MockCNSClient) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo) error { +func (mi *MockCNSClient) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIP map[string]cns.KubernetesPodInfo, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) error { mi.MockCNSInitialized = true mi.Pods = podInfoByIP mi.NCRequest = ncRequest @@ -168,7 +167,7 @@ func (mc *MockDirectAPIClient) ListPods(cntxt context.Context, namespace, node s } func TestNewCrdRequestController(t *testing.T) { //Test making request controller without logger initialized, should fail - _, err := NewCrdRequestController(nil, fakes.NewIPAMPoolMonitorFake(), nil) + _, err := NewCrdRequestController(nil, nil) if err == nil { t.Fatalf("Expected error when making NewCrdRequestController without initializing logger, got nil error") } else if !strings.Contains(err.Error(), "logger") { @@ -188,7 +187,7 @@ func TestNewCrdRequestController(t *testing.T) { } }() - _, err = NewCrdRequestController(nil, fakes.NewIPAMPoolMonitorFake(), nil) + _, err = NewCrdRequestController(nil, nil) if err == nil { t.Fatalf("Expected error when making NewCrdRequestController without setting " + nodeNameEnvVar + " env var, got nil error") } else if !strings.Contains(err.Error(), nodeNameEnvVar) { diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 875d32d1ee..3cc1cc0559 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -669,6 +669,8 @@ func startService() { return } + svc.IPAMPoolMonitor = fakes.NewIPAMPoolMonitorFake() + if service != nil { err = service.Start(&config) if err != nil { diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index 03dfac4885..f82046cbfb 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -16,6 +16,7 @@ import ( "github.com/Azure/azure-container-networking/cns/nmagentclient" "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/log" + nnc "github.com/Azure/azure-container-networking/nodenetworkconfig/api/v1alpha" ) // This file contains the internal functions called by either HTTP APIs (api.go) or @@ -151,14 +152,14 @@ func (service *HTTPRestService) SyncNodeStatus(dncEP, infraVnet, nodeID string, } // This API will be called by CNS RequestController on CRD update. -func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIp map[string]cns.KubernetesPodInfo) int { +func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkContainerRequest, podInfoByIp map[string]cns.KubernetesPodInfo, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) int { // check if ncRequest is null, then return as there is no CRD state yet if ncRequest == nil { log.Logf("CNS starting with no NC state, podInfoMap count %d", len(podInfoByIp)) return Success } - returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest) + returnCode := service.CreateOrUpdateNetworkContainerInternal(*ncRequest, scalar, spec) // If the NC was created successfully, then reconcile the allocated pod state if returnCode != Success { @@ -194,7 +195,7 @@ func (service *HTTPRestService) ReconcileNCState(ncRequest *cns.CreateNetworkCon } // This API will be called by CNS RequestController on CRD update. -func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest) int { +func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.CreateNetworkContainerRequest, scalar nnc.Scaler, spec nnc.NodeNetworkConfigSpec) int { if req.NetworkContainerid == "" { logger.Errorf("[Azure CNS] Error. NetworkContainerid is empty") return NetworkContainerNotSpecified @@ -244,5 +245,12 @@ func (service *HTTPRestService) CreateOrUpdateNetworkContainerInternal(req cns.C logger.Errorf(returnMessage) } + if err = service.IPAMPoolMonitor.Update(scalar, spec); err != nil { + logger.Errorf("[cns-rc] Error creating or updating IPAM Pool Monitor: %v", err) + // requeue + return UnexpectedError + } + return returnCode + } diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index c8c3631de1..b6721bf8d9 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/cns/fakes" "github.com/google/uuid" ) @@ -19,6 +20,10 @@ const ( gatewayIp = "10.0.0.1" subnetPrfixLength = 24 dockerContainerType = cns.Docker + releasePercent = 50 + requestPercent = 100 + batchSize = 10 + initPoolSize = 10 ) var ( @@ -41,7 +46,7 @@ func TestReconcileNCWithEmptyState(t *testing.T) { expectedNcCount := len(svc.state.ContainerStatus) expectedAllocatedPods := make(map[string]cns.KubernetesPodInfo) - returnCode := svc.ReconcileNCState(nil, expectedAllocatedPods) + returnCode := svc.ReconcileNCState(nil, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } @@ -78,7 +83,7 @@ func TestReconcileNCWithExistingState(t *testing.T) { } expectedNcCount := len(svc.state.ContainerStatus) - returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods) + returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } @@ -116,7 +121,7 @@ func TestReconcileNCWithSystemPods(t *testing.T) { } expectedNcCount := len(svc.state.ContainerStatus) - returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods) + returnCode := svc.ReconcileNCState(&req, expectedAllocatedPods, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != Success { t.Errorf("Unexpected failure on reconcile with no state %d", returnCode) } @@ -182,7 +187,7 @@ func validateCreateOrUpdateNCInternal(t *testing.T, secondaryIpCount int) { func createAndValidateNCRequest(t *testing.T, secondaryIPConfigs map[string]cns.SecondaryIPConfig, ncId string) { req := generateNetworkContainerRequest(secondaryIPConfigs, ncId) - returnCode := svc.CreateOrUpdateNetworkContainerInternal(req) + returnCode := svc.CreateOrUpdateNetworkContainerInternal(req, fakes.NewFakeScalar(releasePercent, requestPercent, batchSize), fakes.NewFakeNodeNetworkConfigSpec(initPoolSize)) if returnCode != 0 { t.Fatalf("Failed to createNetworkContainerRequest, req: %+v, err: %d", req, returnCode) } diff --git a/cns/restserver/ipam_test.go b/cns/restserver/ipam_test.go index 94fea1993e..1f72b55aa7 100644 --- a/cns/restserver/ipam_test.go +++ b/cns/restserver/ipam_test.go @@ -43,7 +43,7 @@ func getTestService() *HTTPRestService { var config common.ServiceConfig httpsvc, _ := NewHTTPRestService(&config, fakes.NewFakeImdsClient()) svc = httpsvc.(*HTTPRestService) - + svc.IPAMPoolMonitor = fakes.NewIPAMPoolMonitorFake() setOrchestratorTypeInternal(cns.KubernetesCRD) return svc diff --git a/cns/restserver/restserver.go b/cns/restserver/restserver.go index a76ba8948e..cce2494743 100644 --- a/cns/restserver/restserver.go +++ b/cns/restserver/restserver.go @@ -42,6 +42,7 @@ type HTTPRestService struct { PodIPIDByOrchestratorContext map[string]string // OrchestratorContext is key and value is Pod IP uuid. PodIPConfigState map[string]cns.IPConfigurationStatus // seondaryipid(uuid) is key AllocatedIPCount map[string]allocatedIPCount // key - ncid + IPAMPoolMonitor cns.IPAMPoolMonitor routingTable *routes.RoutingTable store store.KeyValueStore state *httpRestServiceState diff --git a/cns/service/main.go b/cns/service/main.go index 556ac07047..e6b256479a 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -451,8 +451,6 @@ func main() { return } - poolMonitor := ipampoolmonitor.NewCNSIPAMPoolMonitor() - // Set orchestrator type orchestrator := cns.SetOrchestratorTypeRequest{ OrchestratorType: cns.KubernetesCRD, @@ -460,13 +458,14 @@ func main() { httpRestServiceImplementation.SetNodeOrchestrator(&orchestrator) // Get crd implementation of request controller - requestController, err = kubecontroller.NewCrdRequestController(httpRestServiceImplementation, poolMonitor, kubeConfig) + requestController, err = kubecontroller.NewCrdRequestController(httpRestServiceImplementation, kubeConfig) if err != nil { logger.Errorf("[Azure CNS] Failed to make crd request controller :%v", err) return } - poolMonitor.UpdateReferences(httpRestService, requestController) + // initialize the ipam pool monitor + httpRestServiceImplementation.IPAMPoolMonitor = ipampoolmonitor.NewCNSIPAMPoolMonitor(httpRestServiceImplementation, requestController) //Start the RequestController which starts the reconcile loop requestControllerStopChannel := make(chan struct{}) @@ -481,7 +480,7 @@ func main() { ctx, cancel := context.WithCancel(context.Background()) defer cancel() go func() { - if err := poolMonitor.Start(ctx, poolIPAMRefreshRateInMilliseconds); err != nil { + if err := httpRestServiceImplementation.IPAMPoolMonitor.Start(ctx, poolIPAMRefreshRateInMilliseconds); err != nil { logger.Errorf("[Azure CNS] Failed to start pool monitor with err: %v", err) } }() From 99a1e3546ed9c135fdeb0b25b31e6cddd0d0c948 Mon Sep 17 00:00:00 2001 From: Mathew Merrick Date: Wed, 16 Sep 2020 09:05:16 -0700 Subject: [PATCH 18/18] init fake ipam pool monitor --- cns/cnsclient/cnsclient_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cns/cnsclient/cnsclient_test.go b/cns/cnsclient/cnsclient_test.go index 79858da075..372f134e48 100644 --- a/cns/cnsclient/cnsclient_test.go +++ b/cns/cnsclient/cnsclient_test.go @@ -125,6 +125,7 @@ func TestMain(m *testing.M) { httpRestService, err := restserver.NewHTTPRestService(&config, fakes.NewFakeImdsClient()) svc = httpRestService.(*restserver.HTTPRestService) svc.Name = "cns-test-server" + svc.IPAMPoolMonitor = fakes.NewIPAMPoolMonitorFake() if err != nil { logger.Errorf("Failed to create CNS object, err:%v.\n", err)