CNS to DNC communication in Managed DNC #574

jaer-tsun · 2020-06-06T00:00:51Z

What this PR does / why we need it:

CNS talks to DNC to get node goalstate
CNS checks with NMA if it has goalstate for NC

Special notes for your reviewer:
re-review after pretty fat rebase

codecov · 2020-06-06T00:06:02Z

Codecov Report

Merging #574 into master will decrease coverage by 0.46%.
The diff coverage is 10.11%.

@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
- Coverage   42.43%   41.97%   -0.47%     
==========================================
  Files          72       72              
  Lines        9993    10151     +158     
==========================================
+ Hits         4241     4261      +20     
- Misses       5280     5421     +141     
+ Partials      472      469       -3

tamilmani1989 · 2020-07-15T18:18:38Z

cns/restserver/restserver.go

+
+		var (
+			context      = podInfo.PodName + podInfo.PodNamespace
+			dncEP        = service.GetOption(acn.OptPrivateEndpoint).(string)
+			infraVnet    = service.GetOption(acn.OptInfrastructureNetwork).(string)
+			nodeID       = service.GetOption(acn.OptNodeID).(string)
+			isManagedDnc = dncEP != "" && infraVnet != "" && nodeID != ""
+		)
+
+		containerID, exists = service.state.ContainerIDByOrchestratorContext[context]
+		if !exists {
+			if isManagedDnc {
+				service.lock.Unlock()
+				getNetworkContainerResponse.Response.ReturnCode, getNetworkContainerResponse.Response.Message = service.SyncNodeStatus(dncEP, infraVnet, nodeID, req.OrchestratorContext)
+				service.lock.Lock()
+				if getNetworkContainerResponse.Response.ReturnCode == NotFound {
+					return getNetworkContainerResponse
+				}
+
+				containerID = service.state.ContainerIDByOrchestratorContext[context]
+			}
+		} else if isManagedDnc {
+			_, getNetworkContainerResponse.Response.ReturnCode, getNetworkContainerResponse.Response.Message = service.isNCWaitingForUpdate(service.state.ContainerStatus[containerID].CreateNetworkContainerRequest.Version, containerID)
+		}


move to separate function

move what to separate function?

github doesnt have good gui...

Comment on lines 1266 to 1289

tamilmani1989 · 2020-07-15T18:19:11Z

cns/restserver/restserver.go

+		containerID, exists = service.state.ContainerIDByOrchestratorContext[context]
+		if !exists {
+			if isManagedDnc {
+				service.lock.Unlock()


why do you unlock?..no lock acquired before this code

it's actually supposed to be RUnlock and RLock

cns/restserver/restserver.go

cns/NetworkContainerContract.go

cns/restserver/restserver.go

cns/service/main.go

ashvindeodhar · 2020-07-15T20:54:30Z

cns/service/main.go

+		go func(ep, vnet, node string) {
+			// Periodically poll (30s) DNC for node updates
+			for {
+				<-time.NewTicker(time.Second * 30).C


move 30 sec to a const

it's only used here

my comment in general is to not hardcode values like 5 seconds, 30 seconds. Instead declare those as variable like syncNodeInterval.

I just finished reviewing the DNC PR. We are retrieving all the NCs every 30 seconds. let's expose this as a configurable param via config file. If we see we are hitting cosmosdb RU limit due to NC processing in DNC we can increase this w/o needing to update the code.

common/config.go

ashvindeodhar · 2020-07-15T21:07:19Z

Can you add the UTs to test register, sync and NMA api? You will need to inject the dependency / mock those components

cns/NetworkContainerContract.go

cns/nmagentclient/nmagentclient.go

tamilmani1989 · 2020-07-17T17:53:10Z

cns/nmagentclient/nmagentclient.go

+	getNetworkContainerVersionURL string) (*http.Response, error) {
+	logger.Printf("[NMAgentClient] GetNetworkContainerVersion NC: %s", networkContainerID)
+
+	response, err := common.GetHttpClient().Get(getNetworkContainerVersionURL)


common.GetHttpClient()- can this function return nil?

it's init at start

tamilmani1989 · 2020-07-17T17:54:09Z

cns/restserver/api.go

+
+// SetNodeOrchestrator :- Set node orchestrator after registering with mDNC
+func (service *HTTPRestService) SetNodeOrchestrator(r *cns.SetOrchestratorTypeRequest) {
+	body, _ := json.Marshal(r)


catching marshal error and logging it?

tamilmani1989 · 2020-07-17T17:54:34Z

cns/restserver/api.go

+// SetNodeOrchestrator :- Set node orchestrator after registering with mDNC
+func (service *HTTPRestService) SetNodeOrchestrator(r *cns.SetOrchestratorTypeRequest) {
+	body, _ := json.Marshal(r)
+	req, _ := http.NewRequest(http.MethodPost, "", bytes.NewBuffer(body))


same here..catching error and logging it

tamilmani1989 · 2020-07-17T17:58:10Z

cns/restserver/api.go

+	)
+
+	// try to retrieve NodeInfoResponse from mDNC
+	response, err = httpc.Get(fmt.Sprintf(common.SyncNodeNetworkContainersURLFmt, dncEP, infraVnet, nodeID, dncApiVersion))


it would be good we move this to separate statement and log full url

tamilmani1989 · 2020-07-17T21:30:54Z

cns/restserver/util.go

+					waitingForUpdate = true
+					returnCode = NetworkContainerPendingStatePropagation
+					message = fmt.Sprintf("[Azure-CNS] Network container %s v%d had not propagated to respective NMA w/ v%d", ncid, programmedVersion, nmaVersion)
+				}


do we need to log if programmedversion < nmaversion?

no, CNS should be receiving latest

cns/service/main.go

cns/nmagentclient/nmagentclient.go

cns/configuration/configuration.go

cns/nmagentclient/nmagentclient.go

cns/restserver/internalapi.go

cns/service/main.go

…working into cnsToDnc

cns/restserver/internalapi.go

tamilmani1989 · 2020-07-22T18:03:08Z

cns/configuration/configuration.go


+type ManagedSettings struct {
+	PrivateEndpoint           string
+	InfrastructureNetwork     string


is this vnetid? if this is id, can we name it as InfrastructureNetworkID.
also can you add a comment what value privateendpoint takes(is it dns or ip?)

added ID, and it takes either IP or DNS, that's why it's just Endpoint

yes but can add a comment there

tamilmani1989 · 2020-07-22T18:08:41Z

cns/restserver/internalapi.go

+
+		if tmpReturnCode == UnexpectedError {
+			continue
+		}


i think i asked earlier...not sure what you replied for that.. what if tmpReturnCode != Success && tmpReturnCode !=unexpectederror && bytes.Compare(nc.OrchestratorContext, contextFromCNI) != 0 ?

then that means it's NetworkContainerPendingStatePropagation; in this case, the err gets returned to CNI and CNS will still save the state

cns/restserver/internalapi.go

tamilmani1989 · 2020-07-23T05:05:43Z

cns/restserver/util.go

+					dncEP     = service.GetOption(acn.OptPrivateEndpoint).(string)
+					infraVnet = service.GetOption(acn.OptInfrastructureNetwork).(string)
+					nodeID    = service.GetOption(acn.OptNodeID).(string)


are we not using config?

we do use config, those values are passed to the service

tamilmani1989 · 2020-07-23T05:08:11Z

cns/restserver/util.go

+				nodeID    = service.GetOption(acn.OptNodeID).(string)
+			)
+
+			returnCode, msg := service.SyncNodeStatus(dncEP, infraVnet, nodeID, json.RawMessage{})


can you add comment why are we passing empty podcontext

only CNI passes non-empty, there's a func comment about this already

tamilmani1989 · 2020-07-23T18:52:40Z

cns/service/main.go

+	privateEndpoint := acn.GetArg(acn.OptPrivateEndpoint).(string)
+	infravnet := acn.GetArg(acn.OptInfrastructureNetworkID).(string)
+	nodeID := acn.GetArg(acn.OptNodeID).(string)


why do we need this? we are going to get these from config anyway

checked with jaeryn..we should not advise customer to use cmdline args..its just for internal testing and it should be removed if not needed

tamilmani1989

lgtm

* initial changes for CNS->DNC support * Adding changes for CNS to be compatible with managed DNC (reverse communication channel) * adding NC version validation with respective NMA * return errors for respective NC based on orchestrator context from CNI * add nc version check via NMA * adding logic to SyncNodeStatus and check if NCWaitingForUpdate for CniADD and CnsAttach calls * addressing most of ashvin's comments * adding managed config * fat rebase * addressing some comments * slight optimizations... * adding channel mode instead of managed bool * set err in register node so that we keep looping * addressing ashvin's comments * fix test * removing swift prefix mods for mdnc * addressing tamanoha's comments Co-authored-by: Jaeryn <tsun.chu@microsoft.com>

jaer-tsun requested a review from ashvindeodhar June 6, 2020 00:03

jaer-tsun force-pushed the cnsToDnc branch 2 times, most recently from 7a4c854 to 603f703 Compare June 9, 2020 16:46

jaer-tsun force-pushed the cnsToDnc branch 2 times, most recently from d4f235a to c7d9479 Compare June 18, 2020 22:50

jaer-tsun added the work-in-progress label Jun 24, 2020

jaer-tsun changed the title ~~[WIP] Cns to dnc~~ Cns to dnc Jun 24, 2020

jaer-tsun force-pushed the cnsToDnc branch 6 times, most recently from 3c80a80 to 510ce73 Compare July 13, 2020 18:50

jaer-tsun removed the work-in-progress label Jul 13, 2020

jaer-tsun force-pushed the cnsToDnc branch from 510ce73 to 4440d63 Compare July 14, 2020 21:01

ashvindeodhar requested a review from tamilmani1989 July 15, 2020 17:09

jaer-tsun force-pushed the cnsToDnc branch 2 times, most recently from a8609ac to db7726b Compare July 15, 2020 17:52

tamilmani1989 changed the title ~~Cns to dnc~~ CNS to DNC communication in Managed DNC Jul 15, 2020

tamilmani1989 requested changes Jul 15, 2020

View reviewed changes

cns/NetworkContainerContract.go Outdated Show resolved Hide resolved

jaer-tsun force-pushed the cnsToDnc branch 2 times, most recently from 4a63970 to cef191e Compare July 15, 2020 18:47

jaer-tsun requested a review from tamilmani1989 July 15, 2020 18:51

ashvindeodhar requested changes Jul 15, 2020

View reviewed changes

jaer-tsun requested a review from ashvindeodhar July 15, 2020 22:45

jaer-tsun force-pushed the cnsToDnc branch 2 times, most recently from 5037681 to 3530953 Compare July 15, 2020 23:13

tamilmani1989 requested changes Jul 17, 2020

View reviewed changes

cns/NetworkContainerContract.go Outdated Show resolved Hide resolved

tamilmani1989 requested changes Jul 17, 2020

View reviewed changes

addressing some comments

5084325

jaer-tsun force-pushed the cnsToDnc branch from fa37d01 to 5084325 Compare July 17, 2020 22:10

Jaeryn added 2 commits July 20, 2020 10:43

slight optimizations...

048c6bd

adding channel mode instead of managed bool

3666fe4

jaer-tsun requested review from neaggarwMS and tamilmani1989 July 21, 2020 00:27

ashvindeodhar requested changes Jul 21, 2020

View reviewed changes

set err in register node so that we keep looping

945410d

jaer-tsun requested a review from ashvindeodhar July 21, 2020 18:29

addressing ashvin's comments

aba2db4

ashvindeodhar previously approved these changes Jul 21, 2020

View reviewed changes

cns/service/main.go Show resolved Hide resolved

Merge branch 'master' of https://github.com/Azure/azure-container-net…

35ff6df

…working into cnsToDnc

jaer-tsun dismissed ashvindeodhar’s stale review via 46d7f83 July 22, 2020 00:01

jaer-tsun force-pushed the cnsToDnc branch 2 times, most recently from b8b1c2f to ebe5faf Compare July 22, 2020 00:20

fix test

e4beffa

jaer-tsun force-pushed the cnsToDnc branch from ebe5faf to e4beffa Compare July 22, 2020 01:05

removing swift prefix mods for mdnc

1827cde

neaggarwMS previously approved these changes Jul 22, 2020

View reviewed changes

cns/restserver/internalapi.go Show resolved Hide resolved

tamilmani1989 requested changes Jul 23, 2020

View reviewed changes

addressing tamanoha's comments

d3c82e8

jaer-tsun dismissed neaggarwMS’s stale review via d3c82e8 July 23, 2020 18:10

synced latest from master

78c07a4

tamilmani1989 requested changes Jul 23, 2020

View reviewed changes

tamilmani1989 approved these changes Jul 23, 2020

View reviewed changes

jaer-tsun merged commit dd7abd1 into Azure:master Jul 23, 2020

jaer-tsun deleted the cnsToDnc branch July 23, 2020 20:03

CNS to DNC communication in Managed DNC #574

CNS to DNC communication in Managed DNC #574

Uh oh!

Conversation

jaer-tsun commented Jun 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

Uh oh!

ashvindeodhar commented Jul 15, 2020

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tamilmani1989 Jul 22, 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!

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

jaer-tsun commented Jun 6, 2020 •

edited

Loading

codecov bot commented Jun 6, 2020 •

edited

Loading

tamilmani1989 Jul 22, 2020 •

edited

Loading