Skip to content

Support SCTP port mapping (bump up API to v1.37)#33922

Merged
thaJeztah merged 1 commit intomoby:masterfrom
ishidawataru:sctp
Feb 20, 2018
Merged

Support SCTP port mapping (bump up API to v1.37)#33922
thaJeztah merged 1 commit intomoby:masterfrom
ishidawataru:sctp

Conversation

@ishidawataru
Copy link
Copy Markdown

@ishidawataru ishidawataru commented Jul 3, 2017 •
edited by thaJeztah
Loading

(EDIT by @AkihiroSuda: add list for tracking vendor PRs)

Needs-vendoring:

Vendored changes:

image

Signed-off-by: Wataru Ishida ishida.wataru@lab.ntt.co.jp

- What I did

This PR adds support for SCTP(Stream Control Transmission Protocol) port mapping.
SCTP is widely used in cellular networks as a transport protocol. One of the popular application of SCTP is Diameter. This PR enables running these applications inside docker containers.

Most of the work is done in libnetwork repo and I also opened a PR for it.

I'll shortly open a PR for docker/go-connections repo too.

vendor.conf is temporally filled with my forked repos.

Related issue: #9689

- How I did it

- How to verify it

use https://github.com/ishidawataru/cli/tree/sctp for the cli to try this out.

$ docker run -p 10000:20000/sctp mgoelzer/alpine-socat socat sctp-listen:20000 system:cat

$ docker run -it --net=host mgoelzer/alpine-socat socat stdin sctp4-connect:127.0.0.1:10000,bind=127.0.0.1
hello
hello

- Description for the changelog

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

This was referenced Jul 3, 2017
Merged
Merged
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jul 3, 2017 

diff --git a/hack/dockerfile/binaries-commits b/hack/dockerfile/binaries-commits
index 98344bc2f..ae6324541 100644
--- a/hack/dockerfile/binaries-commits
+++ b/hack/dockerfile/binaries-commits
@@ -6,9 +6,12 @@ TOMLV_COMMIT=9baf8a8a9f2ed20a8e54160840c492f937eeaf9a
 RUNC_COMMIT=2d41c047c83e09a6d61d464906feb2a2f3c52aa4
 CONTAINERD_COMMIT=3addd840653146c90a254301d6c3a663c7fd6429
 TINI_COMMIT=949e6facb77383876aeff8a6944dde66b3089574
-LIBNETWORK_COMMIT=7b2b1feb1de4817d522cc372af149ff48d25028e
+# TODO: remove LIBNETWORK_REPO when libnetwork#1825 gets merged to upstream
+LIBNETWORK_REPO=https://github.com/ishidawataru/libnetwork.git
+LIBNETWORK_COMMIT=c04444a9ef8bd272e9c7c971e6235f3a79e7b11c
 VNDR_COMMIT=c56e082291115e369f77601f9c071dd0b87c7120

 # CLI
-DOCKERCLI_REPO=https://github.com/docker/cli
-DOCKERCLI_COMMIT=3dfb8343b139d6342acfd9975d7f1068b5b1c3d3
+DOCKERCLI_REPO=https://github.com/ishidawataru/cli.git
+DOCKERCLI_COMMIT=49ee8ec5882240454c12e098ecc0b86fd12922c9
+
diff --git a/hack/dockerfile/install-binaries.sh b/hack/dockerfile/install-binaries.sh
index 370ec7ce4..9747478c7 100755
--- a/hack/dockerfile/install-binaries.sh
+++ b/hack/dockerfile/install-binaries.sh
@@ -40,7 +40,7 @@ install_containerd() {

 install_proxy() {
        echo "Install docker-proxy version $LIBNETWORK_COMMIT"
-       git clone https://github.com/docker/libnetwork.git "$GOPATH/src/github.com/docker/libnetwork"
+       git clone "$LIBNETWORK_REPO" "$GOPATH/src/github.com/docker/libnetwork"
        cd "$GOPATH/src/github.com/docker/libnetwork"
        git checkout -q "$LIBNETWORK_COMMIT"
        go build -ldflags="$PROXY_LDFLAGS" -o /usr/local/bin/docker-proxy github.com/docker/libnetwork/cmd/proxy

Please update hack/dockerfile temporarily for ease of testing and reviewing

@ishidawataru
Copy link
Copy Markdown
Author

ishidawataru commented Jul 3, 2017

@AkihiroSuda done

AkihiroSuda
AkihiroSuda reviewed Jul 3, 2017
View reviewed changes
hack/dockerfile/binaries-commits Outdated
DOCKERCLI_REPO=https://github.com/docker/cli
DOCKERCLI_COMMIT=3dfb8343b139d6342acfd9975d7f1068b5b1c3d3
DOCKERCLI_REPO=https://github.com/ishidawataru/cli.git
DOCKERCLI_COMMIT=49ee8ec5882240454c12e098ecc0b86fd12922c9
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Jul 3, 2017

Choose a reason for hiding this comment

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

the commit seems gone. 72233db2ec641b322c2f49f81645ff5bf6f745b6?

Copy link
Copy Markdown
Author

@ishidawataru ishidawataru Jul 3, 2017

Choose a reason for hiding this comment

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

thanks, I pushed a fix.

@AkihiroSuda AkihiroSuda mentioned this pull request Jul 3, 2017
Closed
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jul 3, 2017

added code for swarm; please cherry-pick:

@ishidawataru ishidawataru mentioned this pull request Jul 3, 2017
Merged
@ishidawataru
Copy link
Copy Markdown
Author

ishidawataru commented Jul 3, 2017 •
edited
Loading

@AkihiroSuda Thanks a lot for your support! Cherry-picked your commits on my branches.
Also, updated the list of related PRs.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jul 4, 2017

CI failure is weird but seems real

@AkihiroSuda AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 4, 2017
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jul 4, 2017

Test failure seems because this PR vendors CLI that seems recently got broken (docker/cli#293)

@ishidawataru Please revert change to DOCKERCLI_COMMIT to see if CI still fails?
Sorry for going back and forth.

@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 4, 2017
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jul 4, 2017
@ishidawataru
Copy link
Copy Markdown
Author

ishidawataru commented Jul 4, 2017

@AkihiroSuda No problem. I reverted the commits.

@AkihiroSuda AkihiroSuda removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 4, 2017
@orbsfoc
Copy link
Copy Markdown

orbsfoc commented Jan 2, 2018

Any chance of this getting included?

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jan 2, 2018

@orbsfoc moby/libnetwork#1825 needs to be merged first. Review is welcome.

@Peter-eid
Copy link
Copy Markdown

Peter-eid commented Feb 1, 2018

@AkihiroSuda @ishidawataru what is the status of this feature ?

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 1, 2018

@Peter-eid

moby/libnetwork#1825 needs to be reviewed and merged.

I guess not all libnetwork maintainers are familiar with SCTP, so inputs from SCTP community would be helpful to move this forward.

thaJeztah
thaJeztah approved these changes Feb 16, 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.

Changes LGTM

@AkihiroSuda I opened #36335 to keep unrelated bumps out of this PR; we can rebase this PR after that's merged

@fcrisciani @nwoodmsft this vendor bump also brings in moby/libnetwork#2070 - are local changes needed to use that? Does that change need updates in the documentation or CLI?

hack/dockerfile/binaries-commits Outdated
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Feb 16, 2018

Choose a reason for hiding this comment

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

doh! looks like this commit was out of sync with vendor.conf; I'll open a PR to add a comment here, and in vendor.conf to keep these in sync.

Perhaps we should have some code to parse vendor.conf and automatically sync them (I think I saw some code in another repository doing this)

Let me open a separate PR to straighten this separate

Copy link
Copy Markdown
Contributor

@nwoodmsft nwoodmsft Feb 16, 2018

Choose a reason for hiding this comment

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

No local changes needed. From my perspective we are good to bring it in.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Feb 16, 2018

#36335 was merged; can you rebase, @AkihiroSuda / @ishidawataru?

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 19, 2018

rebased

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Feb 19, 2018

z failure doesn't look related; https://jenkins.dockerproject.org/job/Docker-PRs-s390x/8542/console

restarting

05:45:58 ----------------------------------------------------------------------
05:45:58 FAIL: check_test.go:366: DockerSwarmSuite.TearDownTest
05:45:58 
05:45:58 unmount of /tmp/docker-execroot/d90a4b96ddada/netns failed: invalid argument
05:45:58 unmount of /tmp/docker-execroot/d90a4b96ddada/netns failed: no such file or directory
05:45:58 check_test.go:371:
05:45:58     d.Stop(c)
05:45:58 daemon/daemon.go:391:
05:45:58     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
05:45:58 ... Error: Error while stopping the daemon d29a6fde43de3 : exit status 130
05:45:58 
05:45:58 
05:45:58 ----------------------------------------------------------------------
05:45:58 PANIC: docker_api_swarm_test.go:461: DockerSwarmSuite.TestAPISwarmManagerRestore
05:45:58 
05:45:58 [d90a4b96ddada] waiting for daemon to start
05:45:58 [d90a4b96ddada] daemon started
05:45:58 
05:45:58 [d90a4b96ddada] exiting daemon
05:45:58 [d90a4b96ddada] waiting for daemon to start
05:45:58 [d90a4b96ddada] daemon started
05:45:58 
05:45:58 [d29a6fde43de3] waiting for daemon to start
05:45:58 [d29a6fde43de3] daemon started
05:45:58 
05:45:58 [d29a6fde43de3] exiting daemon
05:45:58 [d29a6fde43de3] waiting for daemon to start
05:45:58 [d29a6fde43de3] daemon started
05:45:58 
05:45:58 [dd4cd730508ff] waiting for daemon to start
05:45:58 [dd4cd730508ff] daemon started
05:45:58 
05:45:58 [dd4cd730508ff] exiting daemon
05:45:58 [dd4cd730508ff] waiting for daemon to start
05:45:58 [dd4cd730508ff] daemon started
05:45:58 
05:45:58 [dd4cd730508ff] waiting for daemon to start
05:45:58 [dd4cd730508ff] daemon started
05:45:58 
05:45:58 [d90a4b96ddada] exiting daemon
05:45:58 [d29a6fde43de3] daemon started
05:45:58 Attempt #2: daemon is still running with pid 9141
05:45:58 Attempt #3: daemon is still running with pid 9141
05:45:58 Attempt #4: daemon is still running with pid 9141
05:45:58 [d29a6fde43de3] exiting daemon
05:45:58 ... Panic: Fixture has panicked (see related PANIC)
05:45:58

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Feb 19, 2018

oh, does this require an API version bump? (At least a mention in the API history: https://github.com/moby/moby/blob/master/docs/api/version-history.md)

vdemeester
vdemeester approved these changes Feb 19, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 19, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@733ed2d). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #33922   +/-   ##
=========================================
  Coverage          ?    34.3%           
=========================================
  Files             ?      609           
  Lines             ?    45299           
  Branches          ?        0           
=========================================
  Hits              ?    15542           
  Misses            ?    27750           
  Partials          ?     2007

@AkihiroSuda
Support SCTP port mapping (bump up API to v1.37)
8e435b8 
Signed-off-by: Wataru Ishida <ishida.wataru@lab.ntt.co.jp>
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda AkihiroSuda changed the title Support SCTP port mapping Support SCTP port mapping (bump up API to v1.37) Feb 20, 2018
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 20, 2018

bumped up API to v1.37

cpuguy83
cpuguy83 approved these changes Feb 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 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
thaJeztah approved these changes Feb 20, 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

@thaJeztah thaJeztah merged commit 079ed01 into moby:master Feb 20, 2018
This was referenced Feb 20, 2018
Merged
Closed
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

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@dnephin dnephin Awaiting requested review from dnephin

@tianon tianon Awaiting requested review from tianon tianon is a code owner

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

+2 more reviewers

@nwoodmsft nwoodmsft nwoodmsft left review comments

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vdemeester vdemeester

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ishidawataru @AkihiroSuda @orbsfoc @Peter-eid @thaJeztah @vdemeester @cpuguy83 @nwoodmsft @GordonTheTurtle