CNS Scaling Integration (3/3) #668

matmerr · 2020-09-04T02:47:22Z

Reason for Change:

Integrates the IPAM Pool Monitor with existing request controller and CNS.

DO NOT MERGE BEFORE #667, this PR is continually rebased off that one.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

codecov · 2020-09-04T02:51:53Z

Codecov Report

Merging #668 into master will increase coverage by 0.39%.
The diff coverage is 71.69%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
+ Coverage   42.26%   42.65%   +0.39%     
==========================================
  Files          72       72              
  Lines       10350    10392      +42     
==========================================
+ Hits         4374     4433      +59     
+ Misses       5508     5483      -25     
- Partials      468      476       +8

ramiro-gamarra

Need to review further but leaving some initial feedback for ipampoolmonitor

ramiro-gamarra · 2020-09-11T15:07:27Z

cns/ipampoolmonitor/ipampoolmonitor.go

-	doNothing        = 0
+	increasePoolSize = int64(1)
+	decreasePoolSize = int64(-1)
+	doNothing        = int64(0)


these seem unused

ramiro-gamarra · 2020-09-11T15:20:56Z

cns/ipampoolmonitor/ipampoolmonitor.go

+func (pm *CNSIPAMPoolMonitor) Start(poolMonitorRefreshMilliseconds int, exitChan <-chan struct{}) error {
+	logger.Printf("[ipam-pool-monitor] Starting CNS IPAM Pool Monitor")
+	for {
+		if stopReconcile(exitChan) {


I gather the intent is to have a cancellable loop that does something on an interval. The pattern for that is to use a context and a ticker. Then you can do something like

for { select { case <-ctx.Done(): return fmt.Errorf("CNS IPAM Pool Monitor received cancellation signal") case <- ticker.C: // do things } }

The reason that is preferred to time.Sleeping is that there's no way to cancel the goroutine while it sleeps. If Reconcile is also a potentially blocking method, it should probably also accept a context.

ramiro-gamarra · 2020-09-11T15:23:29Z

cns/ipampoolmonitor/ipampoolmonitor.go

-	pm.MaximumFreeIps = int(pm.scalarUnits.BatchSize * (pm.scalarUnits.ReleaseThresholdPercent / 100))
+		err := pm.Reconcile()
+		if err != nil {
+			logger.Printf("[ipam-pool-monitor] CRITICAL %v", err)


This is probably for offline discussion, but are there any cases where an error from reconciliation should prompt the app to crash and be restarted?

ramiro-gamarra · 2020-09-11T15:34:45Z

cns/ipampoolmonitor/ipampoolmonitor.go


 type CNSIPAMPoolMonitor struct {
-	initialized bool
+	initialized    bool


I'm curious about the reason we need an initialized flag. Is there a way that the pool monitor can be hydrated on construction? That simplifies the rest of the methods, where we can assume that it's been initialized properly and that dependencies are there.

ramiro-gamarra · 2020-09-11T15:37:11Z

cns/ipampoolmonitor/ipampoolmonitor.go

-		return decreasePoolSize
+func (pm *CNSIPAMPoolMonitor) Reconcile() error {
+	if pm.initialized {
+		cnsPodIPConfigCount := len(pm.cns.GetPodIPConfigState())


Happy path is usually best unindented, so I would flip the condition and return early. Even cleaner if we can skip the check and assume things have been initialized :)

ramiro-gamarra · 2020-09-11T15:40:45Z

cns/ipampoolmonitor/ipampoolmonitor.go

+		// no pods scheduled
+		case allocatedPodIPCount == 0:
+			logger.Printf("[ipam-pool-monitor] No pods scheduled")
+			return fmt.Errorf("No pods scheduled")


shouldn't this just be a no-op?

ramiro-gamarra · 2020-09-11T15:45:59Z

cns/ipampoolmonitor/ipampoolmonitor.go

-	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
+func (pm *CNSIPAMPoolMonitor) Reconcile() error {


This should probably take a lock as well. While the loop above might only run this in a single goroutine, the internals of the pool monitor can also be changed using UpdatePoolMonitor which could be executed at the same time.

ramiro-gamarra · 2020-09-11T15:47:31Z

cns/ipampoolmonitor/ipampoolmonitor.go

+	MinimumFreeIps int64
+	MaximumFreeIps int64

 	sync.RWMutex


Consider naming this mutex instead of embedding it. Embedding it makes the lock/unlock methods exported as well, which is something calling code should not be aware of.

ramiro-gamarra · 2020-09-11T15:51:58Z

cns/ipampoolmonitor/ipampoolmonitor.go

+	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")


Bit of a defensive check to be done at runtime. It's probably a programming error if we don't have a cns reference at this point.

ramiro-gamarra · 2020-09-11T15:53:26Z

cns/logger/log.go

 	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)


Are these changes intentional?

neaggarwMS · 2020-09-11T22:13:35Z

cns/ipampoolmonitor/ipampoolmonitor.go

-	pm.initialized = true
-}
+	// 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


It is okay to updateCRD and request for more from CNS. We wont be able to update the request anyways allocatedCount wont increase until the next batch is ready. We dont need this check

neaggarwMS · 2020-09-11T22:17:56Z

cns/ipampoolmonitor/ipampoolmonitor.go

+	if pm.cns == nil {
+		return fmt.Errorf("Error Updating Pool Limits, reference to CNS is nil")
+	}
+


Initial RequestedIpCount should be same as what is specified in the spec. Say you updated the CRP spec RequestedCount to 20 and then CNS died. On reconcile we want to make sure we cache the CRD spec. (Both RequestedCount + IPs which are marked as pending release)

neaggarwMS · 2020-09-11T22:20:27Z

cns/restserver/internalapi.go

 	"net/http"
 	"net/http/httptest"
 	"reflect"



This should also pass the CRD spec details. And we should cache the complete spec.

Also as part of reconcile, ips which are listed in ToBeDeleted list must be marked as PendingRelease if they still exists in PodIpConfigState.

neaggarwMS · 2020-09-11T22:37:35Z

cns/ipampoolmonitor/ipampoolmonitor.go

-	pm.initialized = true
-}
+	// 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


Remove this check, not required.

neaggarwMS · 2020-09-11T22:42:52Z

cns/ipampoolmonitor/ipampoolmonitor.go

+	if pm.cns == nil {
+		return fmt.Errorf("Error Updating Pool Limits, reference to CNS is nil")
+	}
+


RequestedCount should be picked from the crd spec.
Cachedspec should match the CRD spec.

Created NC, SecIpCount = len(podipConfigState) = 10, Cachedspec.RequestedCount = 10

Updated CRD.spec.RequestedCount = 20

CNS crashed

on restart, CND wil recreate NC, SecIpCount = len(podIpConfigstate)=10, Cachedspec.RequestedCount = 10

Also we are missing the ToBeDeletedIPs

neaggarwMS · 2020-09-11T22:50:07Z

cns/ipampoolmonitor/ipampoolmonitor.go

 type CNSIPAMPoolMonitor struct {
-	initialized bool
+	pendingRelease bool
+


Save the whole spec.

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

* init with test framework * handle multiple batch size pool request * test verification * handle batch size multiples * address feedback * fix test * remove test struct from fakes * fix nil test * fix tests * feedback * scale down * 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 * start feedback * update request controller * fixed tests * test fix * ipampoolmonitor in cns not request controller * init fake ipam pool monitor

matmerr added do-not-merge work-in-progress labels Sep 4, 2020

matmerr force-pushed the cnspoolintegration branch 4 times, most recently from f0a7b34 to 6991854 Compare September 9, 2020 00:49

ramiro-gamarra reviewed Sep 11, 2020

View reviewed changes

neaggarwMS reviewed Sep 11, 2020

View reviewed changes

cns/ipampoolmonitor/ipampoolmonitor.go

type CNSIPAMPoolMonitor struct {

initialized bool

pendingRelease bool

Copy link

Member

neaggarwMS Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save the whole spec.

matmerr added 16 commits September 15, 2020 13:18

init with test framework

5d7ea79

handle multiple batch size pool request

259cdd6

test verification

c981fe9

handle batch size multiples

7bb99aa

address feedback

df1832e

fix test

85a935d

remove test struct from fakes

3d41f38

fix nil test

c54449e

fix tests

d1bd3be

feedback

bd72b57

scale down

e3d1719

init scale down

8d258f1

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

start feedback

4737f00

update request controller

b79c375

fixed tests

711411e

test fix

96758b1

matmerr force-pushed the cnspoolintegration branch from 2564b22 to 96758b1 Compare September 15, 2020 20:18

ipampoolmonitor in cns not request controller

4d67241

neaggarwMS previously approved these changes Sep 16, 2020

View reviewed changes

init fake ipam pool monitor

99a1e35

matmerr dismissed neaggarwMS’s stale review via 99a1e35 September 16, 2020 16:05

neaggarwMS approved these changes Sep 16, 2020

View reviewed changes

matmerr merged commit 51a8884 into Azure:master Sep 16, 2020

CNS Scaling Integration (3/3) #668

CNS Scaling Integration (3/3) #668

Uh oh!

Conversation

matmerr commented Sep 4, 2020

Uh oh!

codecov bot commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ramiro-gamarra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 4, 2020 •

edited

Loading