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

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

Merged
merged 1 commit into from Feb 20, 2018

Conversation

@ishidawataru

ishidawataru commented Jul 3, 2017

(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)

@AkihiroSuda

This comment has been minimized.

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

This comment has been minimized.

ishidawataru commented Jul 3, 2017

DOCKERCLI_REPO=https://github.com/docker/cli
DOCKERCLI_COMMIT=3dfb8343b139d6342acfd9975d7f1068b5b1c3d3
DOCKERCLI_REPO=https://github.com/ishidawataru/cli.git
DOCKERCLI_COMMIT=49ee8ec5882240454c12e098ecc0b86fd12922c9

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Jul 3, 2017

Member

the commit seems gone. 72233db2ec641b322c2f49f81645ff5bf6f745b6?

This comment has been minimized.

@ishidawataru

ishidawataru Jul 3, 2017

thanks, I pushed a fix.

@ishidawataru ishidawataru referenced this pull request Jul 3, 2017

Merged

support SCTP #2298

@ishidawataru

This comment has been minimized.

ishidawataru commented Jul 3, 2017

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

@AkihiroSuda

This comment has been minimized.

Member

AkihiroSuda commented Jul 4, 2017

CI failure is weird but seems real

@AkihiroSuda

This comment has been minimized.

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.

@ishidawataru ishidawataru force-pushed the ishidawataru:sctp branch from 2d5f1bd to e534cc7 Jul 4, 2017

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jul 4, 2017

@ishidawataru

This comment has been minimized.

ishidawataru commented Jul 4, 2017

@AkihiroSuda No problem. I reverted the commits.

@orbsfoc

This comment has been minimized.

orbsfoc commented Jan 2, 2018

Any chance of this getting included?

@AkihiroSuda

This comment has been minimized.

Member

AkihiroSuda commented Jan 2, 2018

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

@Peter-eid

This comment has been minimized.

Peter-eid commented Feb 1, 2018

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

@AkihiroSuda

This comment has been minimized.

Member

AkihiroSuda commented Feb 1, 2018

@Peter-eid

docker/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

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 docker/libnetwork#2070 - are local changes needed to use that? Does that change need updates in the documentation or CLI?

@@ -10,7 +10,7 @@ RUNC_COMMIT=6c55f98695e902427906eed2c799e566e3d3dfb5
# fixes or new APIs.
CONTAINERD_COMMIT=cfd04396dc68220d1cecbe686a6cc3aa5ce3667c # v1.0.2
TINI_COMMIT=949e6facb77383876aeff8a6944dde66b3089574
LIBNETWORK_COMMIT=fcf1c3b5e57833aaaa756ae3c4140ea54da00319
LIBNETWORK_COMMIT=ed2130d117c11c542327b4d5216a5db36770bc65

This comment has been minimized.

@thaJeztah

thaJeztah Feb 16, 2018

Member

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

This comment has been minimized.

@nwoodmsft

nwoodmsft Feb 16, 2018

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

@thaJeztah

This comment has been minimized.

Member

thaJeztah commented Feb 16, 2018

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

@AkihiroSuda AkihiroSuda force-pushed the ishidawataru:sctp branch from e593897 to 7df2ffe Feb 19, 2018

@AkihiroSuda

This comment has been minimized.

Member

AkihiroSuda commented Feb 19, 2018

rebased

@thaJeztah

This comment has been minimized.

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

This comment has been minimized.

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

LGTM 🐸

@codecov

This comment has been minimized.

codecov bot commented Feb 19, 2018

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
Support SCTP port mapping (bump up API to v1.37)
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 from Support SCTP port mapping to Support SCTP port mapping (bump up API to v1.37) Feb 20, 2018

@AkihiroSuda AkihiroSuda force-pushed the ishidawataru:sctp branch from 7df2ffe to 8e435b8 Feb 20, 2018

@AkihiroSuda

This comment has been minimized.

Member

AkihiroSuda commented Feb 20, 2018

bumped up API to v1.37

@cpuguy83

LGTM

@thaJeztah

LGTM

@thaJeztah thaJeztah merged commit 079ed01 into moby:master Feb 20, 2018

8 of 9 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 39408 has succeeded
Details
janky Jenkins build Docker-PRs 48155 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 8565 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 4143 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 19682 has succeeded
Details
z Jenkins build Docker-PRs-s390x 8550 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment