mNAT + LB support for swift containers #680

vakalapa · 2020-09-29T19:49:52Z

Reason for Change:
mNAT + LB support changes in CNS

uses conventional commit messages
includes documentation
adds unit tests

Notes:

codecov · 2020-09-29T19:55:52Z

Codecov Report

Merging #680 (4d3de91) into master (0309922) will decrease coverage by 0.05%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
- Coverage   39.12%   39.06%   -0.06%     
==========================================
  Files          83       83              
  Lines       10697    10743      +46     
==========================================
+ Hits         4185     4197      +12     
- Misses       6010     6039      +29     
- Partials      502      507       +5

cns/nmagentclient/nmagentclient.go

cns/restserver/api.go

cns/restserver/internalapi.go

cns/restserver/api.go

ramiro-gamarra

Some comments/questions

ramiro-gamarra · 2020-09-30T15:55:39Z

cns/NetworkContainerContract.go

 type NodeInfoResponse struct {
 	NetworkContainers  []CreateNetworkContainerRequest
 	GetNCVersionURLFmt string
+	NmAgentApisMissing bool


It's probably more extensible to return the list of available actions than just a flag to say that some are missing. This way, the client can validate if the specific ones they are looking for are missing.

Right now i do not think there are any other requests or actions apart from the one in question. SO kept it simple with a single flag.

I must have been confused before. This is what DNC will be returning?

Correct. CNS will ask for regular syncNodeStatus, as a response, DNC sends back list of NCs, version and the flag for NMAgentApis list. And DNC will set this flag only if the NodeConfig has GreCapableNotSet property. If the Node is not capable or if the Node has been previously updated with this list then DNC will skip asking for this information.

cns/NetworkContainerContract.go

cns/nmagentclient/nmagentclient.go

cns/restserver/internalapi.go

cns/api.go

cns/restserver/api.go

cns/restserver/internalapi.go

aegal · 2020-09-30T22:42:28Z

cns/restserver/internalapi.go

 	service.saveState()
 	service.Unlock()

+	if nodeInfoResponse.NmAgentApisMissing {


This doesn't seem like it is needed. CNS would not get past start up if register node did not succeed in the main, so this information would be there if the node was registered with DNC earlier or not?

This will work as kind of a re-try mechanism. If DNC fails to set its properties due to some error, in the next NodeSyncStatus it can solicit for these details again.

The only thing I don't quite understand is if DNC fails to set this properties, then the register node call should fail, so we would never reach here.

Today, register Node happens only once, at the time of initial CNS bringup, main.go L#385. And yes, we do not want to register if DNC sets this flag as false (same as todays behavior)

I don't follow either. If I'm reading main correctly, if node registration failed during start up, that terminates CNS. Registration must succeed before the sync loop is even started. Once it's registered, why do we need to try to register it again here?

After thinking through and testing, i removed the re-register logic. Now if the Node needs to get the ability to add GreKeys, Cx will need to drain NCs, unregister the Node and register again.

ramiro-gamarra

Few more comments/questions

ramiro-gamarra · 2020-10-05T21:40:15Z

cns/NetworkContainerContract.go

 type NodeInfoResponse struct {
 	NetworkContainers  []CreateNetworkContainerRequest
 	GetNCVersionURLFmt string
+	NmAgentApisMissing bool


I must have been confused before. This is what DNC will be returning?

cns/NetworkContainerContract.go

cns/nmagentclient/nmagentclient.go

cns/restserver/api.go

ramiro-gamarra · 2020-10-05T22:19:58Z

cns/restserver/internalapi.go

 	service.saveState()
 	service.Unlock()

+	if nodeInfoResponse.NmAgentApisMissing {


I don't follow either. If I'm reading main correctly, if node registration failed during start up, that terminates CNS. Registration must succeed before the sync loop is even started. Once it's registered, why do we need to try to register it again here?

cns/restserver/internalapi.go

matmerr · 2020-10-14T16:56:35Z

/azp run

azure-pipelines · 2020-10-14T16:56:53Z

Azure Pipelines successfully started running 1 pipeline(s).

ramiro-gamarra

Few comments

cns/nmagentclient/nmagentclient.go

cns/service/main.go

ramiro-gamarra · 2020-11-17T00:59:07Z

cns/service/main.go

+		case <-nodeRegisterTicker.C:
+			go sendRegisterNodeRequest(httpc, httpRestService, nodeRegisterRequest, url, responseChan)
+		}
+	}


This would wait five seconds before it attempts to send the first request. Not sure if that's the intended behavior. It also seems you may not need goroutines if these operations are sequential. Do we want to retry the operation forever or crash at some point? I would recommend looking at the retry package that got added a couple weeks ago, might come in handy here: https://github.com/Azure/azure-container-networking/blob/master/test/integration/retry/retry.go

Will correct the first 5 sec delay. Existing behavior is to retry forever. You are right, we need a better mechanism here. I can add a work item to improve this logic for exponential back off as a separate effort ?

ramiro-gamarra · 2020-11-17T01:12:59Z

cns/service/main.go

-		}
+		response.Body.Close()
+	} else {
+		logger.Errorf("[Azure CNS] Failed to register node with err: %+v", err)


Is there a reason why this path doesn't send the error? Also, i know this is just adding support of channels, but probably a good opportunity to clean up the function since its kinda hard to follow the code flow. Suggest returning early like:

res, err := post() if err != nil { /* return */ } defer res.body.close() if res.StatusCode != { /* return */ } if err := decode(req); err != nil { /* return */ } setNodeOrch(req)

Okay will refactor. Yes, I am assuming this error is retriable error, like timeout or some other transport path error. (which most probably can be due to delays in setting up of azure services). The other errors are specific from DNC side, which i am attributing as not retriable.

vakalapa · 2020-11-18T18:06:58Z

/azp run

azure-pipelines · 2020-11-18T18:07:09Z

Azure Pipelines successfully started running 1 pipeline(s).

ramiro-gamarra

lgtm!

csfmomo · 2020-11-20T21:06:14Z

.pipelines/e2e-step-template.yaml

      export REGIONS=$(AKS_ENGINE_REGION) 
      export IS_JENKINS=false 
      export DEBUG_CRASHING_PODS=true
+      export


Why we need this one?

To print out all the env variables. For better debuggability. This will mask any secrets like below:

declare -x CLIENT_ID="55990953-d723-433b-a204-01af59561ed8" declare -x CLIENT_SECRET="***" declare -x CLUSTER_DEFINITION="./cniLinux1604.json"

vakalapa added 2 commits September 28, 2020 15:09

Adding greKey allocation support for mNAT and LB

9921109

Covering pull model for DNC-CNS nmAgentSupportedApis list

7ed7e91

matmerr reviewed Sep 29, 2020

View reviewed changes

Addressing comments

3e804a2

ramiro-gamarra reviewed Sep 30, 2020

View reviewed changes

Adding test cases for register Node

4a03264

vakalapa marked this pull request as ready for review September 30, 2020 19:31

vakalapa added 2 commits September 30, 2020 14:41

Adding one more testcase

bc2b728

Adding in export function to checkk all test env variables

e9e0686

aegal reviewed Sep 30, 2020

View reviewed changes

vakalapa added 4 commits September 30, 2020 22:39

addressing comments and adding one test

42cf7d1

changing fail criteria for a test

0084f53

Addressing more comments

6b2acb4

fixing a small typo

1220cda

ramiro-gamarra reviewed Oct 5, 2020

View reviewed changes

vakalapa added 7 commits November 11, 2020 13:51

Adding a ticker logic and cleaning up some comments

5dea9d9

Merge branch 'master' into vakr/cns_lb_mnat

52b030f

resolving Merge conflict issues

84742f8

changing wireserver to Var, as tests are trying to change this value

3eedd84

Correcting a timing of the ticker

b971ce4

Correcting a timing of the ticker

6717e48

Adjusting logic on re-registering the Node in mDNC case

2129a6f

ramiro-gamarra reviewed Nov 17, 2020

View reviewed changes

Addressing comments

aafc041

vakalapa mentioned this pull request Nov 18, 2020

Update CNS to handle updated DNC API in managed mode #725

Closed

vakalapa added 2 commits November 19, 2020 11:48

changing logic around noderegister

cf34795

Addressing comments

4d3de91

ramiro-gamarra approved these changes Nov 19, 2020

View reviewed changes

vakalapa merged commit a22a852 into master Nov 19, 2020

vakalapa deleted the vakr/cns_lb_mnat branch November 19, 2020 22:15

csfmomo reviewed Nov 20, 2020

View reviewed changes

mNAT + LB support for swift containers #680

mNAT + LB support for swift containers #680

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

vakalapa Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

ramiro-gamarra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matmerr commented Oct 14, 2020

Uh oh!

azure-pipelines bot commented Oct 14, 2020

Uh oh!

ramiro-gamarra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vakalapa commented Sep 29, 2020 •

edited

Loading

codecov bot commented Sep 29, 2020 •

edited

Loading

vakalapa Oct 5, 2020 •

edited

Loading