New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ipam options to ipam driver requests #2324

Merged
merged 2 commits into from Oct 4, 2017

Conversation

Projects
None yet
7 participants
@abhi
Member

abhi commented Jul 20, 2017

  1. 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.

  2. 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

This comment has been minimized.

Show comment
Hide comment
@yuexiao-wang

yuexiao-wang Jul 21, 2017

Contributor

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

Contributor

yuexiao-wang commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jul 21, 2017

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

codecov bot commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jul 21, 2017

Collaborator

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.

Collaborator

aaronlehmann commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@abhi
Member

abhi commented Aug 9, 2017

Adding ipam options to ipam driver requests
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

This comment has been minimized.

Show comment
Hide comment
@abhi

abhi Oct 3, 2017

Member

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

Member

abhi commented Oct 3, 2017

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

@marcusmartins

This comment has been minimized.

Show comment
Hide comment
@marcusmartins
Member

marcusmartins commented Oct 3, 2017

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Oct 3, 2017

Contributor

Vendoring issue seems fixed now.

Contributor

nishanttotla commented Oct 3, 2017

Vendoring issue seems fixed now.

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Oct 3, 2017

Contributor

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.

Contributor

nishanttotla commented Oct 3, 2017

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.

@@ -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 {

This comment has been minimized.

@anshulpundir

anshulpundir Oct 4, 2017

Contributor

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.

@anshulpundir

anshulpundir Oct 4, 2017

Contributor

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.

This comment has been minimized.

@abhi

abhi Oct 4, 2017

Member

sure

@abhi

abhi Oct 4, 2017

Member

sure

Serializing IP allocation
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

This comment has been minimized.

Show comment
Hide comment
@abhi
Member

abhi commented Oct 4, 2017

@mavenugo

LGTM

@nishanttotla

LGTM

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

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 60.45% (target 0%)
Details
dco-signed All commits are signed

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

Backport swarmkit fix docker#2324
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 docker#2324
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 docker#2324
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
Backport Adding ipam options to ipam driver requests #2324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment