Add pending program status for IPs in CNS. #690

csfmomo · 2020-10-14T02:42:43Z

*Add pending program status for IPs in CNS.
*Add logic structure of how to update program status.

feat:
Add logic structure for CNS udpate NC secondary IP to pending program before available

Reason for Change:
Pending program is a status to track that IP is available in NC but not in NMAgent side yet. IP is available when NMAgent is also ready.

Issue Fixed:
Prevent CNS allocate pending program IP to pod.

Add logic structure of how to update program status.

matmerr · 2020-10-14T16:55:17Z

/azp run

azure-pipelines · 2020-10-14T16:55:39Z

Azure Pipelines successfully started running 1 pipeline(s).

csfmomo · 2020-10-14T23:05:07Z

/azp run

azure-pipelines · 2020-10-14T23:05:22Z

Azure Pipelines successfully started running 1 pipeline(s).

codecov · 2020-10-14T23:09:37Z

Codecov Report

Merging #690 into master will decrease coverage by 0.06%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #690      +/-   ##
==========================================
- Coverage   38.60%   38.54%   -0.07%     
==========================================
  Files          78       79       +1     
  Lines       10433    10471      +38     
==========================================
+ Hits         4028     4036       +8     
- Misses       5913     5943      +30     
  Partials      492      492

csfmomo · 2020-10-15T01:17:21Z

/azp run

azure-pipelines · 2020-10-15T01:17:40Z

Azure Pipelines successfully started running 1 pipeline(s).

… test.

cns/NetworkContainerContract.go

neaggarwMS · 2020-10-15T06:07:31Z

cns/requestcontroller/kubecontroller/crdtranslator.go

 		ncRequest.NetworkContainerid = nc.ID
 		ncRequest.NetworkContainerType = cns.Docker
-		ncRequest.Version = nc.Version
+		ncRequest.NCVersion = nc.Version


Why is this changed? It should be set to ncRequest.Version only

Will change it back to ncRequest.Version.
I didn't reuse the Version since the usage of Version is not clear in CNS previously. There are Version, which compared with VMVersion and HostVersion which record NMA version. After double checking code, resusing Version should be fine.

cns/ipampoolmonitor/ipampoolmonitor.go

neaggarwMS · 2020-10-15T06:16:13Z

cns/restserver/util.go

+func (service *HTTPRestService) addIPConfigStateUntransacted(ncVersion, ncId string, ipconfigs map[string]cns.SecondaryIPConfig) {
+
+	existingNCStatus, _ := service.state.ContainerStatus[ncId]
+	existingNCVersion, _ := strconv.Atoi(existingNCStatus.CreateNetworkContainerRequest.NCVersion)


This wont return the expected value, NCRequest is already overwritten in the caller API (saveNetworkContainerGoalState, line 120). You need to pass existing Version (not NCVersion) from the called like we are passing existingIpconfigs

Will update.
I think we should not overwritten existing NC status unless CNS finish all the updating works in this call. Imagine if call crash in the middle, update is not finished but existing NC status got overwritten. Another plus is we don't need to create new object to carry existing NC status temporarily. I can follow the previous style for now but thinking open another PR which do all the works needed before updating existing NC status.

neaggarwMS · 2020-10-15T06:17:19Z

cns/restserver/util.go

+
+	existingNCStatus, _ := service.state.ContainerStatus[ncId]
+	existingNCVersion, _ := strconv.Atoi(existingNCStatus.CreateNetworkContainerRequest.NCVersion)
+	nmAgentNCVersion := getNCVersionFromNMAgent(ncId)


I actually recommend to move it to background or make a call in a go routing and wait for it.

Actually we SHOULD not make an outbound call while acquiring a lock to the Service. it must be outside the lock

Let's keep it as it for this PR since getNCVersionFromNMAgent didn't have any logic now. This PR is a structure and I'll fill in logic in next PR. It can be a background thread or separate go routine.
No matter whether we query NMAgent in background or not, we should compare NMAgent's version every time when NC is updated. I don't think NC update will be too frequent to bug NMAgent.

neaggarwMS · 2020-10-15T06:17:56Z

cns/restserver/util.go

-		}
+func (service *HTTPRestService) addIPConfigStateUntransacted(ncVersion, ncId string, ipconfigs map[string]cns.SecondaryIPConfig) {
+
+	existingNCStatus, _ := service.state.ContainerStatus[ncId]


Again this is already updated in the caller

Will update.

neaggarwMS · 2020-10-15T06:20:41Z

cns/restserver/util.go

 }

+// To do, complete this logic according to getNetworkContainerStatus
+func getNCVersionFromNMAgent(ncid string) int {


Please add the logic to get NMAgent ver in ImdsClientInterface

getNetworkContainerStatus is using imdsClient.GetNetworkContainerInfoFromHost. I think GetNetworkContainerInfoFromHost covers most of this logic. I can double check. Will update there.

neaggarwMS · 2020-10-15T06:22:48Z

cns/restserver/util.go

-		if _, exists := service.PodIPConfigState[ipId]; exists {
-			continue
-		}
+func (service *HTTPRestService) addIPConfigStateUntransacted(ncVersion, ncId string, ipconfigs map[string]cns.SecondaryIPConfig) {


I think in this function you also need to pass existingIpconfigs so that you can keep those IPs as available and mark the new ones in PendingProgramming State

Yes, existingSecondaryIPConfigs is needed.

neaggarwMS · 2020-10-15T06:23:32Z

cns/NetworkContainerContract.go

 	Available      = "Available"
 	Allocated      = "Allocated"
 	PendingRelease = "PendingRelease"
+	PendingProgram = "PendingProgram"


can we rename it to PendingProgramming

cns/NetworkContainerContract.go

neaggarwMS · 2020-10-15T06:26:18Z

cns/restserver/util.go


 	// Add the newIpConfigs, ignore if ip state is already in the map
-	service.addIPConfigStateUntransacted(req.NetworkContainerid, newIPConfigs)
+	service.addIPConfigStateUntransacted(req.NCVersion, req.NetworkContainerid, newIPConfigs)


Can you add some logs to updateIpConfigsStateUntransacted function, it is missing logs overall. Specially when we are updating the state or if this api is returning error

Double checked, error msg is already logged, reference
https://github.com/Azure/azure-container-networking/blob/master/cns/restserver/api.go#L831

…working into programmedIp

neaggarwMS · 2020-10-15T06:34:39Z

cns/restserver/util.go

+
+	existingNCStatus, _ := service.state.ContainerStatus[ncId]
+	existingNCVersion, _ := strconv.Atoi(existingNCStatus.CreateNetworkContainerRequest.NCVersion)
+	nmAgentNCVersion := getNCVersionFromNMAgent(ncId)


Actually we SHOULD not make an outbound call while acquiring a lock to the Service. it must be outside the lock

neaggarwMS · 2020-10-19T17:32:14Z

cns/restserver/util.go

+		if nmAgentNCVersion >= existingNCVersion {
+			service.addIPConfigStateUntransacted(cns.Available, cns.PendingProgramming, req.NetworkContainerid, newIPConfigs)
+		} else {
+			service.addIPConfigStateUntransacted(cns.PendingProgramming, cns.PendingProgramming, req.NetworkContainerid, newIPConfigs)


Why we need to move existing IPs back to PendingState if they are already available?

We are NOT make an outboud call. getNCVersionFromNMAgent didn't have implementation yet. It could be the function which fetch NMAgent version locally from what background thread got and saved.

We not not moving existing IPs back to Pending State.

cns/restserver/util.go

…to a background thread.

cns/restserver/util.go

neaggarwMS

Left one minor comment.

neaggarwMS · 2020-10-22T18:46:22Z

cns/fakes/imdsclientfake.go

+// GetNMagentVersion - Mock implementation to return host NMAgent NC version
+// Set it as 0 which is the same as default initial NC version for testing purpose
+func (imdsClient *ImdsClientTest) GetNMagentVersion() int {
+	logger.Printf("[Azure CNS] GetNMagentVersionFromNMAgent")


Update the log to get the NC version from NMAgent

neaggarwMS · 2020-10-22T18:47:26Z

cns/imdsclient/api.go

 	GetNetworkContainerInfoFromHost(networkContainerID string, primaryAddress string, authToken string, apiVersion string) (*ContainerVersion, error)
 	GetPrimaryInterfaceInfoFromHost() (*InterfaceInfo, error)
 	GetPrimaryInterfaceInfoFromMemory() (*InterfaceInfo, error)
+	GetNMagentVersion() int


Rename the function name GetNetworkContainerInfoFromHost and pass all the parameters networkContainerID string, primaryAddress string, apiVersion string)

Add pending program status for IPs in CNS.

a9c4915

Add logic structure of how to update program status.

csfmomo requested review from matmerr, neaggarwMS and ramiro-gamarra October 14, 2020 02:45

Add missing NCVersion CreateNetworkContainerRequest in commit.

29ef434

Add missing fake GetPendingProgramIPConfigs to unblock ipam pool unit…

0f24d45

… test.

neaggarwMS reviewed Oct 15, 2020

View reviewed changes

cns/NetworkContainerContract.go Outdated Show resolved Hide resolved

neaggarwMS requested changes Oct 15, 2020

View reviewed changes

csfmomo added 8 commits October 15, 2020 18:33

Address feedbacks.

c48b183

Modify function parameter to meet current unit test requirement.

0c117f1

Add updating secondary IPs to pending programming status logic.

4a79902

Add ip state update when CRD status change.

6cdb6f9

Update NC ID.

a37b5e7

Update NC ID.

1056109

Update comments.

41e0b9c

Merge branch 'programmedIp' of github.com:csfmomo/azure-container-net…

183058b

…working into programmedIp

neaggarwMS requested changes Oct 19, 2020

View reviewed changes

neaggarwMS reviewed Oct 19, 2020

View reviewed changes

cns/restserver/util.go Outdated Show resolved Hide resolved

csfmomo added 2 commits October 19, 2020 15:47

Remove NC version compare and update logic. This logic will be moved …

7e2190c

…to a background thread.

Update comments accordingly.

4b28926

matmerr added the hacktoberfest-accepted label Oct 20, 2020

neaggarwMS reviewed Oct 20, 2020

View reviewed changes

cns/restserver/util.go Outdated Show resolved Hide resolved

Add unit test.

895fbf6

Update nmagent version in test.

8afcf44

csfmomo mentioned this pull request Oct 22, 2020

Add a function to handle updating pending programming IPs. #700

Closed

csfmomo requested a review from thatmattlong October 22, 2020 00:33

neaggarwMS requested changes Oct 22, 2020

View reviewed changes

Update function name.

5cf37b8

neaggarwMS approved these changes Oct 22, 2020

View reviewed changes

csfmomo merged commit ce35cd2 into Azure:master Oct 22, 2020

csfmomo deleted the programmedIp branch October 23, 2020 01:39

Add pending program status for IPs in CNS. #690

Add pending program status for IPs in CNS. #690

Uh oh!

Conversation

csfmomo commented Oct 14, 2020

Uh oh!

matmerr commented Oct 14, 2020

Uh oh!

azure-pipelines bot commented Oct 14, 2020

Uh oh!

csfmomo commented Oct 14, 2020

Uh oh!

azure-pipelines bot commented Oct 14, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

csfmomo commented Oct 15, 2020

Uh oh!

azure-pipelines bot commented Oct 15, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csfmomo Oct 15, 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!

csfmomo Oct 15, 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

Uh oh!

csfmomo Oct 15, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csfmomo Oct 15, 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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 14, 2020 •

edited

Loading

csfmomo Oct 15, 2020 •

edited

Loading

csfmomo Oct 15, 2020 •

edited

Loading

csfmomo Oct 15, 2020 •

edited

Loading

csfmomo Oct 15, 2020 •

edited

Loading