CNS IPAM for CNI #584

matmerr · 2020-06-17T21:55:28Z

What this PR does / why we need it:

Adds two new API's in CNS which will enable it to act as an IPAM.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

codecov · 2020-06-19T23:01:43Z

Codecov Report

Merging #584 into master will decrease coverage by 2.57%.
The diff coverage is 23.61%.

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage   39.88%   37.30%   -2.58%     
==========================================
  Files          42       47       +5     
  Lines        4586     5203     +617     
==========================================
+ Hits         1829     1941     +112     
- Misses       2502     3000     +498     
- Partials      255      262       +7

cns/restserver/restserver_test.go

Github Templates

cns/cnsclient/cnsclient_test.go

cns/cnsclient/cnsclient.go

cns/restserver/restserver.go

cns/NetworkContainerContract.go

cns/restserver/restserver_test.go

cns/restserver/ipam_test.go

tamilmani1989

minor comments. One concern is that changed introduced in getnetworkcontainerrequest struct should not regress multitenancy scenario.

cns/NetworkContainerContract.go

cns/cnsclient/cnsclient.go

cns/restserver/ipam.go

cns/restserver/ipam_test.go

pjohnst5 · 2020-07-13T15:50:36Z

cns/restserver/ipam.go

-
 // If IPConfig is already allocated for pod, it returns that else it returns one of the available ipconfigs.
-func requestIPConfigHelper(service *HTTPRestService, req cns.GetNetworkContainerRequest) (*cns.ContainerIPConfigState, error) {
+func requestIPConfigHelper(service *HTTPRestService, req cns.GetIPConfigRequest) (*cns.ContainerIPConfigState, error) {


In this method, we check for an existing ipConfig, when would there already exist an ipConfig for the particular pod?

If the pod dies and is restarted, or if the node reboots and all pods are restarted

pjohnst5 · 2020-07-13T15:56:02Z

cns/restserver/ipam.go

 // If IPConfig is already allocated for pod, it returns that else it returns one of the available ipconfigs.
-func requestIPConfigHelper(service *HTTPRestService, req cns.GetNetworkContainerRequest) (*cns.ContainerIPConfigState, error) {
+func requestIPConfigHelper(service *HTTPRestService, req cns.GetIPConfigRequest) (*cns.ContainerIPConfigState, error) {
 	var (


Later on in this method, it checks that the ipconfig already exists for the pod, and returns the ipstate if err!= nil || isExist. Wouldn't we want to return the ipstate if err == nil && isExist?

Also, inside the implementation of "GetExistingIPConfig", the "err" var goes unused basically, the only time it is used is to return nil, so maybe switch that last return to return nil like you did inside the if statement and get rid of the err var.

Another thing in "GetExistingIPConfig", the returned error message is "Pod->IPIP exists..." but I think you meant "Pod->IPID exists..."

Later on in this method, it checks that the ipconfig already exists for the pod, and returns the ipstate if err!= nil || isExist. Wouldn't we want to return the ipstate if err == nil && isExist?

good catch, fixed

Also, inside the implementation of "GetExistingIPConfig", the "err" var goes unused basically, the only time it is used is to return nil, so maybe switch that last return to return nil like you did inside the if statement and get rid of the err var.

true, reused skeleton, fixed.

Another thing in "GetExistingIPConfig", the returned error message is "Pod->IPIP exists..." but I think you meant "Pod->IPID exists..."

Updated the comment

cns/restserver/ipam.go

cns/cnsclient/cnsclient.go

cns/restserver/ipam.go

tamilmani1989

couple of nits..rest lgtm

cns/cnsclient/cnsclient_test.go

cns/restserver/ipam.go

neaggarwMS · 2020-06-26T16:30:52Z

cni/network/network.go

 	plugin.report.ContainerName = k8sPodName + ":" + k8sNamespace

-	if nwCfg.MultiTenancy {
+	if nwCfg.MultiTenancy || nwCfg.Ipam.Type == azureCNSIPAM {


we should refactor CNI to implement interfaces and and have specific implementation according to the scenario. Not required to do in this change only, but we should take care of as it will get difficult to parse the code with more scenarios coming in.

Yes CNI work is being done in a separate PR to do just that

neaggarwMS · 2020-06-26T16:36:03Z

cni/network/network.go

+					return plugin.Errorf(err.Error())
+				}
+
+				result, resultV6, err = cnsClient.RequestIPAddress(orchestratorContext)


Shouldnt this be part of plugin.IpamAdd call?

cns/cnsclient/cnsclient.go

cns/restserver/restserver.go

neaggarwMS · 2020-07-10T17:33:52Z

cns/restserver/ipam.go

+		if ipState, isExist = service.PodIPConfigState[ipID]; isExist {
+			return ipState, isExist, nil
+		}
+		return ipState, isExist, fmt.Errorf("Pod->IPIP exists but IPID to IPConfig doesn't exist")


This means our CNS state is corrupted right? We should log this error into kusto and raise alert to see if we ever reach this state. Can be handled in a separate PR though

cns/restserver/ipam.go

tamilmani1989

lgtm

cns ipam

3426829

matmerr added the work-in-progress label Jun 17, 2020

matmerr added 4 commits June 17, 2020 19:21

cnsclient requestipaddress in cni

0f5dc91

cnsclient release

69a5c34

state update, tbd

f90fea2

add windows specific vendor

3b3649b

matmerr added 4 commits June 20, 2020 08:25

update windows e2e's

e8d0ce3

further refactoring

b8f10f6

cnsclient integration tests

638c4ac

cns release integration test

20c51fd

matmerr force-pushed the cnsipamcni branch from 82cc129 to 3576c62 Compare June 25, 2020 02:13

fix nested lock

c982f4c

matmerr force-pushed the cnsipamcni branch from 3576c62 to c982f4c Compare June 25, 2020 02:14

Merge remote-tracking branch 'upstream/master' into cnsipamcni

a475c7c

matmerr requested review from pjohnst5 and tamilmani1989 June 25, 2020 02:16

matmerr added enhancement cni Related to CNI. cns Related to CNS. and removed work-in-progress labels Jun 25, 2020

matmerr requested a review from neaggarwMS June 25, 2020 18:16

matmerr commented Jun 25, 2020

View reviewed changes

cns/restserver/restserver_test.go Show resolved Hide resolved

Qingchuan Hao and others added 5 commits June 25, 2020 16:16

Simply issue template

c0c47ba

Use issue template builder

c766a57

Small improvements

069c596

template revision

931abab

Merge pull request #13 from matmerr/githubtemplates

da39b2a

Github Templates

pjohnst5 reviewed Jun 26, 2020

View reviewed changes

move cnsclient to ipamdelegate calls

0c7f746

address feedback

21b2ae0

matmerr force-pushed the cnsipamcni branch 2 times, most recently from 260fc97 to 5c56f56 Compare July 9, 2020 16:54

fix tests

35bda47

matmerr force-pushed the cnsipamcni branch from 5c56f56 to 35bda47 Compare July 9, 2020 17:38

matmerr added 2 commits July 9, 2020 10:48

update comments

4ec459c

ipam refactor with clearer locking

125485f

matmerr requested a review from tamilmani1989 July 9, 2020 23:09

tamilmani1989 requested changes Jul 10, 2020

View reviewed changes

cns/NetworkContainerContract.go Outdated Show resolved Hide resolved

cns/cnsclient/cnsclient.go Outdated Show resolved Hide resolved

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

matmerr and others added 5 commits July 10, 2020 08:51

fix nits

1b42027

Merge branch 'master' into cnsipamcni

c8bd6c4

Merge remote-tracking branch 'upstream/master' into cnsipamcni

0f3863e

Merge branch 'master' into cnsipamcni

af41c5a

add separate structs for new apis

dbcb17c

weng271190436 reviewed Jul 13, 2020

View reviewed changes

cns/restserver/ipam_test.go Show resolved Hide resolved

update tests based on feedback

e2622e8

pjohnst5 reviewed Jul 13, 2020

View reviewed changes

remove nested lock on fail and address comments

69ad3aa

tamilmani1989 requested changes Jul 13, 2020

View reviewed changes

cns/cnsclient/cnsclient.go Outdated Show resolved Hide resolved

cns/cnsclient/cnsclient.go Outdated Show resolved Hide resolved

cns/cnsclient/cnsclient.go Outdated Show resolved Hide resolved

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

matmerr added 2 commits July 13, 2020 13:44

address feedback and add validation

61fa082

update test

1fd805c

matmerr force-pushed the cnsipamcni branch from 3f7044a to 1fd805c Compare July 13, 2020 21:08

tamilmani1989 requested changes Jul 13, 2020

View reviewed changes

cns/cnsclient/cnsclient_test.go Outdated Show resolved Hide resolved

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

neaggarwMS reviewed Jul 13, 2020

View reviewed changes

matmerr added 3 commits July 13, 2020 14:56

rename api's for allocation, nits, readlock for get api's

e965d7d

make release idempotent, and add tests

fe791f8

update getNetworkContainerResponse to take RLock

0556409

matmerr requested a review from tamilmani1989 July 13, 2020 22:54

tamilmani1989 approved these changes Jul 13, 2020

View reviewed changes

matmerr merged commit 644642b into Azure:master Jul 14, 2020

CNS IPAM for CNI #584

CNS IPAM for CNI #584

Uh oh!

Conversation

matmerr commented Jun 17, 2020

Uh oh!

codecov bot commented Jun 19, 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!

tamilmani1989 left a comment

Choose a reason for hiding this comment

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!

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!

tamilmani1989 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tamilmani1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jun 19, 2020 •

edited

Loading