Add nc version in SecondaryIPConfig stucture. #701

csfmomo · 2020-10-23T07:58:44Z

feat: Add NC version in SecondaryIPConfig structure.

Reason for Change:
When CNS reconcile, it needs NC version to determine whether an IP should be stay in pending programming or available.

[X ] adds unit tests

codecov · 2020-10-23T08:04:53Z

Codecov Report

Merging #701 into master will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #701      +/-   ##
==========================================
- Coverage   38.84%   38.61%   -0.24%     
==========================================
  Files          79       79              
  Lines       10471    10479       +8     
==========================================
- Hits         4067     4046      -21     
- Misses       5912     5938      +26     
- Partials      492      495       +3

neaggarwMS · 2020-10-27T06:13:05Z

There is no change in the reconcile funntion? can you also cover it here? That will complete the NC version check for all scenarios .

cns/restserver/util.go

cns/NetworkContainerContract.go

cns/restserver/util.go

csfmomo · 2020-10-27T23:25:50Z

There is no change in the reconcile funntion? can you also cover it here? That will complete the NC version check for all scenarios .

Discussed offline. We'll use a separate PR to address this scenario.

ramiro-gamarra

Some comments

ramiro-gamarra · 2020-10-27T23:35:56Z

cns/requestcontroller/kubecontroller/crdtranslator.go

 		ipSubnet.PrefixLength = uint8(size)
 		ncRequest.IPConfiguration.IPSubnet = ipSubnet
 		ncRequest.IPConfiguration.GatewayIPAddress = nc.DefaultGateway
+		ncVersion, _ := strconv.Atoi(ncRequest.Version)


should probably bubble the error to be safe

ramiro-gamarra · 2020-10-27T23:57:28Z

cns/restserver/internalapi_test.go

+}
+
+func validateSecondaryIPsNCVersion(t *testing.T, req cns.CreateNetworkContainerRequest) {
+	containerStatus := svc.state.ContainerStatus[req.NetworkContainerid]


Couple of things:

Reading state should be mutex guarded

You should also check that the map actually contains an entry for the nc id

This is a validation code in test file. I don't think mutex guard is necessary. No others have access to svc.state.

Not necessary. If it doesn't exist, test will fail.

ramiro-gamarra · 2020-10-28T00:05:45Z

cns/restserver/internalapi_test.go

+		if (secIPConfig.IPAddress == "10.0.0.16" && secIPConfig.NCVersion != 0) ||
+			(secIPConfig.IPAddress == "10.0.0.17" && secIPConfig.NCVersion != 1) {
+			t.Fatalf("nc request version is %d, secondary ip %s nc version is %d, expected nc version is 0",
+				ncVersion, secIPConfig.IPAddress, secIPConfig.NCVersion)


These assertion values (the expected ip addresses and versions) are very far from the test. If you need to create similar tests with different IP addresses, this would not be very reusable. You should define the expected values on the test, and pass them to this function. The check is also very narrow: if there was only a secondary IP of 10.0.0.15 for example, this would pass.

No. This test is designed specifically and is validating specific scenario.
In this specific test, secondary IP address only contains 10.0.0.16 and 10.0.0.17.
10.0.0.16 must be version 0 and 10.0.0.17 must be version 1.

ramiro-gamarra · 2020-10-28T00:13:15Z

cns/restserver/util.go

 	for ipId, ipconfig := range ipconfigs {
+		// New secondary IP configs has new NC version however, CNS don't want to override existing IPs'with new NC version
+		// Set it back to previous NC version if IP already exist.
+		if existingIPConfig, existsInPreviousIPConfig := existingSecondaryIPConfigs[ipId]; existsInPreviousIPConfig && existingIPConfig.IPAddress == ipconfig.IPAddress {


Is the check on ip address equality necessary?

I think it's necessary. If uuid is the same, IPAddress must be same. If not the same, then sth wrong. At least we don't want to set the NC version back.
In other way, I don't see any drawback to double check IPAddress.

neaggarwMS · 2020-10-28T00:23:42Z

cns/restserver/internalapi_test.go

+}
+
+func validateSecondaryIPsNCVersion(t *testing.T, req cns.CreateNetworkContainerRequest) {
+	containerStatus := svc.state.ContainerStatus[req.NetworkContainerid]


This function seems generic but inside it has hardcoded the ipaddress and expected version. Can we pass the expected SecondaryIpConfig as parameter and compare the req with expected list? This will allow to reuse this function in future as well

We can but I don't think it's necessary. I don't think any other test should reuse this function. This validation is designed for TestCreateAndUpdateNCWithSecondaryIPNCVersion specifically.

I moved this validation to test function and avoid confusion.

ramiro-gamarra

Few more comments

ramiro-gamarra · 2020-10-28T16:24:15Z

cns/restserver/internalapi_test.go

+	req := createNCReqInternal(t, secondaryIPConfigs, ncID, strconv.Itoa(ncVersion))
+	containerStatus := svc.state.ContainerStatus[req.NetworkContainerid]
+	// Validate secondary IPs' NC version has been updated by NC request
+	receivedSecondaryIPConfigs = containerStatus.CreateNetworkContainerRequest.SecondaryIPConfigs


no need to make a map in line 59, just use := here

the reason why a check for existence of entry in the map is better is that the test won't fail if there is no match. If you don't get the check for existence, you get the zero value of a struct, including all of it's members recursively, and ranging over a nil map is valid. In case the nc id was not found in the map, the checks below will not happen, and the test will continue as normal. Let me know if that makes sense.

I think what you are trying to say is adding one more check in map length. I can do that though it's not the key point in this test.

cns/restserver/internalapi_test.go

ramiro-gamarra · 2020-10-28T16:28:55Z

cns/restserver/internalapi_test.go

+		// Though "10.0.0.16" IP exists in NC version 1, secodanry IP still keep its original NC version 0
+		if (secIPConfig.IPAddress == "10.0.0.16" && secIPConfig.NCVersion != 0) ||
+			(secIPConfig.IPAddress == "10.0.0.17" && secIPConfig.NCVersion != 1) {
+			t.Fatalf("nc request version is %d, secondary ip %s nc version is %d, expected nc version is 0",


prefer a switch statement here, and messages more specific to each. in case the inconsistent version was with 10.0.0.17, the message still tells you that it expected 0, which is not appropriate:

switch secIPConfig: case "10.0.0.16": // check for 0 case "10.0.0.17": // check for 1 default: // error since this is unexpected

Discussed offline, it's ready to merge.

csfmomo added 3 commits October 23, 2020 00:57

Add nc version in SecondaryIPConfig stucture.

0b3872b

Only update IP version when IP changes.

7df229f

Add unit test to secondary IP NC version update.

3cdb962

Simplify unit test.

8a17ba2

csfmomo mentioned this pull request Oct 23, 2020

Add nc version in SecondaryIPConfig structure. #697

Closed

neaggarwMS requested changes Oct 27, 2020

View reviewed changes

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

cns/NetworkContainerContract.go Show resolved Hide resolved

neaggarwMS reviewed Oct 27, 2020

View reviewed changes

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

neaggarwMS reviewed Oct 27, 2020

View reviewed changes

cns/restserver/util.go Show resolved Hide resolved

neaggarwMS requested changes Oct 27, 2020

View reviewed changes

cns/restserver/util.go Show resolved Hide resolved

csfmomo added 2 commits October 27, 2020 00:09

Add more comments.

b7ae041

Update comments.

8eecb89

csfmomo closed this Oct 27, 2020

csfmomo reopened this Oct 27, 2020

ramiro-gamarra reviewed Oct 28, 2020

View reviewed changes

neaggarwMS previously requested changes Oct 28, 2020

View reviewed changes

csfmomo added 4 commits October 27, 2020 23:55

Add nil check to string to int change.

b905803

Remove unnecessary unit test validation.

108ea56

Add nc version to TestInitRequestController.

e4705be

Add more comments in test.

5567aa5

ramiro-gamarra reviewed Oct 28, 2020

View reviewed changes

Update TestCreateAndUpdateNCWithSecondaryIPNCVersion.

72b1e4b

ramiro-gamarra approved these changes Oct 28, 2020

View reviewed changes

csfmomo merged commit 6fad74d into Azure:master Oct 29, 2020

Add nc version in SecondaryIPConfig stucture. #701

Add nc version in SecondaryIPConfig stucture. #701

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

neaggarwMS commented Oct 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csfmomo commented Oct 27, 2020

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!

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

csfmomo commented Oct 23, 2020 •

edited

Loading

codecov bot commented Oct 23, 2020 •

edited

Loading

csfmomo Oct 28, 2020 •

edited

Loading

csfmomo Oct 28, 2020 •

edited

Loading