Adding ipam options to ipam driver requests #2324

abhi · 2017-07-20T22:18:30Z

Currently ipam options doesnt seem to be passed to remote ipam drivers. Remote ipam drivers that depend on any driver specific options will not work if ipam options are not included in the
RequestAddress from the swarm manager to allocate IPs. This is a minor fix to pass the ipam options to ipam drivers.
This also ensures that the IP and port allocation done by the libnetwork allocator is done in serial manner (next available) as opposed to first available. This helps mitigate some of the issue we see due to immediate resource release on the swarm side.

Signed-off-by: Abhinandan Prativadi abhi@docker.com

yuexiao-wang · 2017-07-21T03:02:58Z

manager/allocator/cnmallocator/networkallocator.go

@@ -553,6 +553,8 @@ func (na *cnmNetworkAllocator) releaseEndpoints(networks []*api.NetworkAttachmen

 // allocate virtual IP for a single endpoint attachment of the service.
 func (na *cnmNetworkAllocator) allocateVIP(vip *api.Endpoint_VirtualIP) error {
+


Should be delete the empty line

yuexiao-wang · 2017-07-21T03:27:53Z

CI fails :

inconsistent dependencies! what you have in vendor.conf does not match with what you have in vendor
make: *** [dep-validate] Error 1

.vendor.bak/golang.org/x/text/unicode/norm/maketables.go
👹 please format Go code with 'gofmt -s -w'
make: *** [fmt] Error 1

codecov · 2017-07-21T04:10:54Z

Codecov Report

Merging #2324 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2324      +/-   ##
==========================================
- Coverage   60.56%   60.45%   -0.11%     
==========================================
  Files         128      128              
  Lines       26277    26303      +26     
==========================================
- Hits        15915    15902      -13     
- Misses       8966     8993      +27     
- Partials     1396     1408      +12

aaronlehmann · 2017-07-21T16:23:34Z

CI fails :

inconsistent dependencies! what you have in vendor.conf does not match with what you have in vendor
make: *** [dep-validate] Error 1

Rebasing on the lastest swarmkit master should fix this. It looks like the branch is based on an old version.

abhi · 2017-08-09T14:43:20Z

ping @aluzzardi @mavenugo

Currently ipam options doesnt seem to be passed to remote ipam drivers. Remote ipam drivers that depend on any driver specific options will not work if ipam options are not included in the RequestAddress fromthe swarm manager to allocate IPs. This is minor fix to pass the ipam options to ipam drivers. Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

abhi · 2017-10-03T21:35:52Z

@nishanttotla This change is needed for the release. libnetwork part of the change is merged.

marcusmartins · 2017-10-03T23:18:40Z

cc @nishanttotla @dperny @anshulpundir for review.

nishanttotla · 2017-10-03T23:23:26Z

Vendoring issue seems fixed now.

nishanttotla · 2017-10-03T23:25:23Z

I'm not very well aware of this part of the codebase, but after reading the initial description and going through the code, it looks fine.

anshulpundir · 2017-10-04T00:33:05Z

manager/allocator/cnmallocator/networkallocator.go

@@ -603,9 +604,16 @@ func (na *cnmNetworkAllocator) allocateVIP(vip *api.Endpoint_VirtualIP) error {
 			return err
 		}
 	}
+	if localNet.nw.IPAM != nil && localNet.nw.IPAM.Driver != nil {


Can I request you to please add comments in the source ? Its probably obvious to those familiar with the code, but not so much for first time readers. Same comment for other changes also.

mavenugo · 2017-10-04T00:39:28Z

manager/allocator/cnmallocator/networkallocator.go

+		if opts == nil {
+			opts = make(map[string]string)
+		}
+		opts[ipamapi.AllocSerialPrefix] = "true"


Can you check for the presence of this key and not override it if the user explicitly set it to false ?
Also, can you pls set this flag only for default ipam driver ?

This does apply to default ipam driver because of namespaced labels. I can add a check.

mavenugo · 2017-10-04T00:40:01Z

manager/allocator/cnmallocator/networkallocator.go

+			if opts == nil {
+				opts = make(map[string]string)
+			}
+			opts[ipamapi.AllocSerialPrefix] = "true"


same as above. optimize it into a function to avoid duplicated code ?

mavenugo · 2017-10-04T00:40:25Z

manager/allocator/cnmallocator/networkallocator.go

+			dOptions = make(map[string]string)
+		}
+		dOptions[ipamapi.RequestAddressType] = netlabel.Gateway
+		dOptions[ipamapi.AllocSerialPrefix] = "true"


same comment

aaronlehmann · 2017-10-04T02:08:21Z

manager/allocator/cnmallocator/networkallocator.go

+	if opts == nil {
+		opts = make(map[string]string)
+	}
+	if _, ok := opts[ipamapi.AllocSerialPrefix]; !ok {


What happens if the value in the map is set to "false"? Should we rename the function to make clear that it only sets serial allocation if no setting was already in place?

Also, IPAM in the funtion name should be IPAM.

ideally I should have added a setter api in ipam libnetwork library. I missed that train. I can probably come back and make this better.

This commit contains fix to serialize IPAM and Port allocation. This would fix transient issues seen due to immediate resource release. Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

abhi · 2017-10-04T02:54:10Z

Addressed comments. @nishanttotla @anshulpundir @marcusmartins @mavenugo PTAL.

mavenugo

LGTM

nishanttotla

LGTM

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

Backport Adding ipam options to ipam driver requests #2324

yuexiao-wang reviewed Jul 21, 2017

View reviewed changes

abhi force-pushed the ipam branch 2 times, most recently from 92d66d1 to a5797af Compare July 21, 2017 03:59

cyli mentioned this pull request Aug 3, 2017

Add ipam options(RequestAddressType) #2338

Open

abhi force-pushed the ipam branch 4 times, most recently from 0a6e4f1 to 7d27468 Compare October 3, 2017 21:22

abhi force-pushed the ipam branch 2 times, most recently from ef820cc to 7d27468 Compare October 3, 2017 23:02

abhi force-pushed the ipam branch from f0de6c5 to bfa7a4c Compare October 3, 2017 23:19

nishanttotla added the area/networking label Oct 3, 2017

abhi force-pushed the ipam branch from bfa7a4c to 63f3bd4 Compare October 3, 2017 23:43

anshulpundir reviewed Oct 4, 2017

View reviewed changes

mavenugo reviewed Oct 4, 2017

View reviewed changes

abhi force-pushed the ipam branch 2 times, most recently from d53074a to d922486 Compare October 4, 2017 01:57

aaronlehmann reviewed Oct 4, 2017

View reviewed changes

Serializing IP allocation

fe0d7f6

This commit contains fix to serialize IPAM and Port allocation. This would fix transient issues seen due to immediate resource release. Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

abhi force-pushed the ipam branch from d922486 to fe0d7f6 Compare October 4, 2017 02:21

mavenugo approved these changes Oct 4, 2017

View reviewed changes

nishanttotla approved these changes Oct 4, 2017

View reviewed changes

nishanttotla merged commit 872861d into moby:master Oct 4, 2017

abhi added a commit to abhi/swarmkit that referenced this pull request Oct 4, 2017

Backport swarmkit fix moby#2324

0bb7ee8

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

abhi added a commit to abhi/swarmkit that referenced this pull request Oct 4, 2017

Backport swarmkit fix moby#2324

ecf35f7

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

abhi added a commit to abhi/swarmkit that referenced this pull request Oct 5, 2017

Backport swarmkit fix moby#2324

005d515

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

dperny added a commit that referenced this pull request Oct 5, 2017

Merge pull request #2394 from abhi/pick_2324

8afb0c4

Backport Adding ipam options to ipam driver requests #2324

andrewhsu mentioned this pull request Oct 11, 2017

[17.10] re-vndr libnetwork to 6c51292 and swarmkit to 872861d docker-archive/docker-ce#264

Merged

fcrisciani mentioned this pull request Nov 15, 2017

[Backport 17.03] Adding ipam options to ipam driver requests #2449

Closed

thaJeztah mentioned this pull request Nov 20, 2017

[17.09] Adding ipam options to ipam driver requests #2455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ipam options to ipam driver requests #2324

Adding ipam options to ipam driver requests #2324

abhi commented Jul 20, 2017 •

edited

Loading

yuexiao-wang Jul 21, 2017

yuexiao-wang commented Jul 21, 2017

codecov bot commented Jul 21, 2017 •

edited

Loading

aaronlehmann commented Jul 21, 2017

abhi commented Aug 9, 2017

abhi commented Oct 3, 2017

marcusmartins commented Oct 3, 2017

nishanttotla commented Oct 3, 2017

nishanttotla commented Oct 3, 2017

anshulpundir Oct 4, 2017

abhi Oct 4, 2017

mavenugo Oct 4, 2017

abhi Oct 4, 2017

mavenugo Oct 4, 2017

mavenugo Oct 4, 2017

aaronlehmann Oct 4, 2017

abhi Oct 4, 2017

abhi commented Oct 4, 2017

mavenugo left a comment

nishanttotla left a comment

		@@ -553,6 +553,8 @@ func (na cnmNetworkAllocator) releaseEndpoints(networks []api.NetworkAttachmen

		// allocate virtual IP for a single endpoint attachment of the service.
		func (na cnmNetworkAllocator) allocateVIP(vip api.Endpoint_VirtualIP) error {

Adding ipam options to ipam driver requests #2324

Adding ipam options to ipam driver requests #2324

Conversation

abhi commented Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

yuexiao-wang commented Jul 21, 2017

codecov bot commented Jul 21, 2017 • edited Loading

Codecov Report

aaronlehmann commented Jul 21, 2017

abhi commented Aug 9, 2017

abhi commented Oct 3, 2017

marcusmartins commented Oct 3, 2017

nishanttotla commented Oct 3, 2017

nishanttotla commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhi commented Oct 4, 2017

mavenugo left a comment

Choose a reason for hiding this comment

nishanttotla left a comment

Choose a reason for hiding this comment

abhi commented Jul 20, 2017 •

edited

Loading

codecov bot commented Jul 21, 2017 •

edited

Loading