-
Notifications
You must be signed in to change notification settings - Fork 615
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
[Backport 17.03] Adding ipam options to ipam driver requests #2449
Conversation
@nishanttotla @stevvooe @abhi for the review. |
vendor.conf
Outdated
@@ -14,18 +14,18 @@ github.com/prometheus/common ebdfc6da46522d58825777cf1f90490a5b1ef1d8 | |||
github.com/prometheus/procfs abf152e5f3e97f2fafac028d2cc06c1feb87ffa5 | |||
|
|||
github.com/docker/distribution 7230e9def796c63a4033211dc5107742d689fc1e | |||
github.com/docker/docker 0fb0d67008157add34f1e11685e23a691db92644 | |||
github.com/docker/docker e4512d264741e83e954a19f9ef5e3cb06c5856b6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What version is this coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the problem is the following:
the original commit does not contain an api that libnetwork uses.
This is the commit id that is used inside libnetwork: https://github.com/docker/libnetwork/blob/release/v0.9/Godeps/Godeps.json#L147
But unfortunately the latter does not have anymore the types: types.ContainersPruneConfig, types.ImagesPruneConfig, types.NetworksPruneConfig, types.VolumesPruneConfig
That got nuked by this commit: moby/moby#28535
Yea, that is kind of a problem. It will break the tests. What version of docker are you bringing in? |
3d2e416
to
4f06009
Compare
Codecov Report
@@ Coverage Diff @@
## bump_v17.03 #2449 +/- ##
==============================================
+ Coverage 55.41% 55.61% +0.2%
==============================================
Files 102 102
Lines 21566 21596 +30
==============================================
+ Hits 11950 12011 +61
+ Misses 8456 8434 -22
+ Partials 1160 1151 -9 |
2119da0
to
134eaac
Compare
@@ -753,6 +753,10 @@ func (a *mockIpam) DiscoverDelete(dType discoverapi.DiscoveryType, data interfac | |||
return nil | |||
} | |||
|
|||
func (a *mockIpam) IsBuiltIn() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming from this PR: 8afb5ec
github.com/docker/go-connections 34b5052da6b11e27f5f2e357b38b571ddddd3928 | ||
github.com/docker/go-events 37d35add5005832485c0225ec870121b78fcff1c | ||
github.com/docker/go-units 954fed01cc617c55d838fa2230073f2cb17386c8 | ||
github.com/docker/libkv 9fd56606e928ff1f309808f5d5a0b7a2ef73f9a8 | ||
github.com/docker/libnetwork 3ab699ea36573d98f481d233c30c742ade737565 | ||
github.com/docker/libnetwork 878043960238db64a7e783199f688211560dd84c https://github.com/fcrisciani/libnetwork |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporary till the libnetwork change gets merged
dOptions = setIPAMSerialAlloc(dOptions) | ||
defer delete(dOptions, ipamapi.RequestAddressType) | ||
|
||
gwIP, _, err := ipam.RequestAddress(poolID, net.ParseIP(ic.Gateway), dOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fcrisciani in the master branch, this statement seems to be inside a conditional:
if ic.Gateway != "" || gwIP == nil {
...
}
Do we need that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, have to check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at it better looks like that logic with the if seems had been introduced by another commit fb74191, that is a major restructure looks like and the logic there extract gwIP.
This change as is does not change the current behavior but simply pass the additional flag to the IPAM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for checking!
@fcrisciani can we add a healthy number of comments here indicating why these changes were done, and a link to this PR? It may turn out to be useful in the future. |
@nishanttotla the PR backported(#2324) is the one that I put in the top description, in the commits there is also the reference to the master commit that got ported. I can copy paste the same description of the original PR here, or is there also some other information that you consider useful? |
@fcrisciani I just meant a comment in networkallocator.go in the code either linking to this PR or a short description of this change, and that it diverges from the master branch. Not super critical though, just nice to have. |
@@ -764,7 +778,15 @@ func (na *NetworkAllocator) allocatePools(n *api.Network) (map[string]string, er | |||
} | |||
pools[poolIP.String()] = poolID | |||
|
|||
gwIP, _, err := ipam.RequestAddress(poolID, net.ParseIP(ic.Gateway), nil) | |||
if dOptions == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This change is a backport that leverage a new allocation policy offered by the libnetwork IPAM
// This will let the ipam driver allocate IP sequentially differently from before that was the leftmost free bit of the pool.
// The objective is to reduce the occurrence of address already in use error that is due to a non synchronous handling
// of resources made by swarmkit, where the state of container is deleted from raft and
// the resources (like IP) is made available for other tasks to use immediately while the previous owner is
// still shutting down
@nishanttotla does it work something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fcrisciani looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k updating the patch now then
Signed-off-by: Flavio Crisciani <[email protected]> (cherry picked from commit ed271d4, fe0d7f6)
Signed-off-by: Flavio Crisciani <[email protected]>
- backport of moby#1876 commit: 7541809 commit: c6aacc7 commit: f250807 To handle the type deletion happened in docker/docker and not updated in swarmkit. The docker vendoring was needed due to a dependency on a method requested by libnetwork vendoring Signed-off-by: Flavio Crisciani <[email protected]>
134eaac
to
b97ab63
Compare
@nishanttotla @stevvooe PTAL |
@fcrisciani close this out ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for this falling by the wayside. LGTM.
Is this even needed at this point ? Its been open since Nov |
@anshulpundir @nishanttotla not worth anymore, 17.03 is going EOL next month |
Backport PR:#2324