Skip to content

VXLAN UDP Port configuration support#38102

Merged
thaJeztah merged 2 commits intomoby:masterfrom
selansen:master
Nov 24, 2018
Merged

VXLAN UDP Port configuration support#38102
thaJeztah merged 2 commits intomoby:masterfrom
selansen:master

Conversation

@selansen
Copy link
Copy Markdown
Contributor

@selansen selansen commented Oct 29, 2018 

This PR chnages allow user to configure VxLAN UDP
port number. By default we use 4789 port number. But this commit
will allow user to configure port number during swarm init.
VxLAN port can't be modified after swarm init.

Signed-off-by: selansen elango.siva@docker.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@selansen selansen force-pushed the master branch 3 times, most recently from b0bacf0 to 15e8444 Compare October 29, 2018 23:24
AkihiroSuda
AkihiroSuda reviewed Oct 30, 2018
View reviewed changes
Comment thread docs/api/version-history.md Outdated
@selansen selansen force-pushed the master branch 2 times, most recently from 612afd7 to a0bf6c2 Compare October 31, 2018 17:13
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 31, 2018 •
edited
Loading

Codecov Report

Merging #38102 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #38102      +/-   ##
==========================================
+ Coverage    36.1%    36.1%   +<.01%     
==========================================
  Files         610      610              
  Lines       45234    45247      +13     
==========================================
+ Hits        16333    16338       +5     
- Misses      26661    26673      +12     
+ Partials     2240     2236       -4

@selansen selansen changed the title [WIP] VXLAN UDP Port configuration support VXLAN UDP Port configuration support Nov 9, 2018
@selansen selansen force-pushed the master branch 2 times, most recently from 5ddd374 to 4139842 Compare November 9, 2018 12:58
thaJeztah
thaJeztah requested changes Nov 9, 2018
View reviewed changes
Comment thread api/swagger.yaml Outdated
@selansen selansen requested a review from vdemeester as a code owner November 11, 2018 06:14
@selansen
Copy link
Copy Markdown
Contributor Author

selansen commented Nov 11, 2018

@thaJeztah PTAL.
I bumped up API version to 1.41 as per our discussion.

@selansen selansen force-pushed the master branch 2 times, most recently from 237b73c to 828d8b8 Compare November 12, 2018 01:32
@selansen
Copy link
Copy Markdown
Contributor Author

selansen commented Nov 13, 2018

Below Failures doesn't seem related to my PR
Janky:

03:53:17 curl: (18) transfer closed with 5073312 bytes remaining to read
03:53:17 The command '/bin/sh -c contrib/download-frozen-image-v2.sh /output/docker-frozen-images buildpack-deps:jessie@sha256:dd86dced7c9cd2a724e779730f0a53f93b7ef42228d4344b25ce9a42a1486251 busybox:latest@sha256:bbc3a03235220b170ba48a157dd097dd1379299370e1ed99ce976df0355d24f0 busybox:glibc@sha256:0b55a30394294ab23b9afd58fab94e61a923f5834fba7ddbae7f8e0c11ba85e6 debian:jessie@sha256:287a20c5f73087ab406e6b364833e3fb7b3ae63ca0eb3486555dc27ed32c6e60 hello-world:latest@sha256:be0cd392e45be79ffeffa6b05338b98ebb16c87b255f48e297ec7f98e123905c' returned a non-zero code: 18
03:53:17 Build step 'Execute shell' marked build as failure

Experimental:
FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
02:32:25
02:32:25 [dfa594f5b831f] waiting for daemon to start
02:32:25 [dfa594f5b831f] daemon started
02:32:25
02:32:25 [d60fd8f3fbb10] waiting for daemon to start
02:32:25 [d60fd8f3fbb10] daemon started
02:32:25
02:32:25 [d3ad07dfb509b] waiting for daemon to start
02:32:25 [d3ad07dfb509b] daemon started
02:32:25
02:32:25 [dfa594f5b831f] exiting daemon
02:32:25 Waiting for election to occur...
02:32:25 assertion failed: error is not nil: Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded: [d60fd8f3fbb10] (*Daemon).GetNode: NodeInspectWithRaw("xzog4myfhk45ctzni4jo9yquz") failed
02:32:25 waited for 20.001393468s (out of 30s)
02:32:25 [d60fd8f3fbb10] exiting daemon
02:32:25 [d3ad07dfb509b] exiting daemon
02:32:49

Z
FAIL: docker_api_swarm_service_test.go:96: DockerSwarmSuite.TestAPISwarmServicesMultipleAgents
02:52:51
02:52:51 [dcfa856305118] waiting for daemon to start
02:52:51 [dcfa856305118] daemon started
02:52:51
02:52:51 [d7e8bc2f9c002] waiting for daemon to start
02:52:51 [d7e8bc2f9c002] daemon started
02:52:51
02:52:51 [d80a69416464d] waiting for daemon to start
02:52:51 [d80a69416464d] daemon started
02:52:51
02:52:51 waited for 1.554943134s (out of 30s)
02:52:51 waited for 13.953249ms (out of 30s)
02:52:51 waited for 25.256027ms (out of 30s)
02:52:51 waited for 359.189177ms (out of 30s)
02:52:51 [d7e8bc2f9c002] exiting daemon
02:52:51 waited for 6.51003692s (out of 30s)
02:52:51 docker_api_swarm_service_test.go:120:
02:52:51 waitAndAssert(c, defaultReconciliationTimeout, reducedCheck(sumAsIntegers, d1.CheckActiveContainerCount, d3.CheckActiveContainerCount), checker.Equals, instances)
02:52:51 docker_utils_test.go:441:
02:52:51 c.Assert(v, checker, args...)
02:52:51 ... obtained int = 6
02:52:51 ... expected int = 5
02:52:51 ... output: "21121dfaf7c1\n05353c71c2d8\n", output: "ccf8a00865a1\n693edb3a6155\nc68cf8921705\n0224458b9a26\n"
02:52:51
02:52:51 waited for 30.096178055s (out of 30s)
02:52:51 [dcfa856305118] exiting daemon
02:52:51 [d80a69416464d] exiting daemon

@selansen
Copy link
Copy Markdown
Contributor Author

selansen commented Nov 22, 2018 via email

@selansen
Copy link
Copy Markdown
Contributor Author

selansen commented Nov 22, 2018

@thaJeztah @tiborvass PTAL

thaJeztah
thaJeztah requested changes Nov 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating! One question, but looks good otherwise

Comment thread daemon/cluster/listen_addr.go Outdated
Comment thread internal/test/daemon/swarm.go Outdated
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't notice this change really, why was this needed? If no port is specified, won't SwarmKit / libnetwork still set the default automatically? (if not, that would be a problem, because older clients won't set this value).

Copy link
Copy Markdown
Contributor Author

@selansen selansen Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for testing new option DataPathPort. You are right, if port is not specified , libnetwork will use default value. if no port is specified , here the value ill be 0. also this test is going to run only on API version 40 and above.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Nov 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is setting the port now by default, so even if no port is specified in the test, a port is send in the request.

Point is that this utility is used by at least four tests (TestAPISwarmJoinToken, TestUpdateSwarmAddExternalCA, TestAPISwarmPromoteDemote, TestAPISwarmForceNewCluster), and because it's called by StartAndSwarmInit (which is called by AddDaemon and NewSwarm) is used by 130+ other tests, so all of those tests will now change, correct?

Copy link
Copy Markdown
Contributor Author

@selansen selansen Nov 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. Sending 0 is more or less equal to setting no value. But I got your point. I will add if check to make sure we add DataPort option only if it is set.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so I think my confusion here is that we're overriding the initRequest with a daemon configuration; looking a bit further;

So, this PR adds a daemon option DataPathPort , so that the port gets automatically set when calling swarm.NewSwarm();

moby/integration/internal/swarm/service.go

Lines 50 to 61 in 5d82d77

// NewSwarm creates a swarm daemon for testing
func NewSwarm(t *testing.T, testEnv *environment.Execution, ops ...func(*daemon.Daemon)) *daemon.Daemon {
t.Helper()
skip.If(t, testEnv.IsRemoteDaemon)
skip.If(t, testEnv.DaemonInfo.OSType == "windows")
if testEnv.DaemonInfo.ExperimentalBuild {
ops = append(ops, daemon.WithExperimental)
}
d := daemon.New(t, ops...)
d.StartAndSwarmInit(t)
return d
}

I think the issue lies there;

  • Unlike (e.g.) storageDriver, DataPathPort is not a daemon configuration
  • The DataPathPort must be given when calling SwarmInit

So the problem is that swarm.NewSwarm() is doing to much for this test; possible resolutions for that;

  • Add a swarmops ...func(*swarm.InitRequest) to swarm.NewSwarm() (so that the options to initialize the swarm can be passed)
  • Don't use swarm.NewSwarm() in this test, and first create the daemon, start it (d.StartNode()), then call `
d := daemon.New(t)
d.StartNode(t)
d.SwarmInit(t, swarm.InitRequest{DataPathPort: 12345})

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we've taken the same approach for DefaultAddrPool, which also is not a daemon option (but something to set on swarm init).

We should fix that, but let's do that in a follow-up

@selansen selansen force-pushed the master branch 3 times, most recently from 8606902 to 9486762 Compare November 22, 2018 16:29
@selansen
Vendor commit for VXLAN UDP Port configuration support
a7ce314 
This commit brings Swarmkit and Libnetwork library changes
Signed-off-by: selansen <elango.siva@docker.com>
@selansen
VXLAN UDP Port configuration support
32180ac 
This commit contains changes to configure DataPathPort
option. By default we use 4789 port number. But this commit
will allow user to configure port number during swarm init.
DataPathPort can't be modified after swarm init.
Signed-off-by: selansen <elango.siva@docker.com>
thaJeztah
thaJeztah approved these changes Nov 22, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if green, thanks!!

@selansen
Copy link
Copy Markdown
Contributor Author

selansen commented Nov 23, 2018

Failures unrelated to this PR:
FAIL: docker_cli_swarm_test.go:1316: DockerSwarmSuite.TestSwarmClusterRotateUnlockKey

FAIL: docker_cli_swarm_test.go:1128: DockerSwarmSuite.TestSwarmLockUnlockCluster

@tiborvass , @crosbymichael PTAL.

tiborvass
tiborvass approved these changes Nov 23, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 24, 2018

All green; ignoring the codecov/patch one

@thaJeztah thaJeztah merged commit 0b7cb16 into moby:master Nov 24, 2018
@selansen
Copy link
Copy Markdown
Contributor Author

selansen commented Nov 24, 2018

Thanks @thaJeztah @tiborvass . Let me update CLI PR n ping you soon.

@olljanat olljanat mentioned this pull request Dec 27, 2018
Closed
This was referenced Jan 24, 2019
Merged
Closed
@sgutermann
Copy link
Copy Markdown

sgutermann commented Apr 4, 2019

@selansen first of all, thank you for your work. I am faced with this problem and I just pulled myself a "nightly" which already shows your work.

One maybe strange question on my part ist, what about the docker swarm join command? I am not able to specify the vxlan port here. Does that bother? Will my vxlan magiacally get the right port?

@selansen
Copy link
Copy Markdown
Contributor Author

selansen commented Apr 4, 2019

@sgutermann you don't need to pass vxlan port during swarm join. swarm init is the only place you will have to pass vxlan port number.

During swarm join event , swarm manager will propagate vxlan information to the joining node. It's all taken care so user doesn't need to remember each time when they invoke swarm join.

@olljanat olljanat mentioned this pull request Jun 1, 2019
Open
@Drewster727 Drewster727 mentioned this pull request Aug 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@tiborvass tiborvass tiborvass approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@duglin duglin

Labels

area/networking Networking area/swarm impact/api impact/changelog impact/documentation status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@selansen @thaJeztah @tiborvass @sgutermann @AkihiroSuda @duglin @GordonTheTurtle