Skip to content

support SCTP#2298

Merged
nishanttotla merged 1 commit intomoby:masterfrom
AkihiroSuda:sctp
Feb 7, 2018
Merged

support SCTP#2298
nishanttotla merged 1 commit intomoby:masterfrom
AkihiroSuda:sctp

Conversation

@ishidawataru
Copy link
Copy Markdown

@ishidawataru ishidawataru commented Jul 3, 2017

please see moby/moby#33922

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

@ishidawataru ishidawataru mentioned this pull request Jul 3, 2017
Merged
4 tasks
@aaronlehmann
Copy link
Copy Markdown
Collaborator

aaronlehmann commented Jul 6, 2017

LGTM

Could you please rebase on the latest master? I think that will fix the CI problem.

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 6, 2017 •
edited
Loading

Codecov Report

Merging #2298 into master will increase coverage by 0.17%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #2298      +/-   ##
==========================================
+ Coverage   61.46%   61.64%   +0.17%     
==========================================
  Files          49      129      +80     
  Lines        6898    21327   +14429     
==========================================
+ Hits         4240    13146    +8906     
- Misses       2220     6772    +4552     
- Partials      438     1409     +971

@ishidawataru
Copy link
Copy Markdown
Author

ishidawataru commented Jul 6, 2017

@aaronlehmann Thanks for your review. Rebased on the latest master.
Please note that all PRs for other repos to support SCTP are not merged yet.
I'm now waiting for a feedback for them.

@AkihiroSuda AkihiroSuda mentioned this pull request Jul 6, 2017
Merged
@diogomonica
Copy link
Copy Markdown
Contributor

diogomonica commented Jul 24, 2017 •
edited
Loading

LGTM. Rebase needed.

@nishanttotla
Copy link
Copy Markdown
Contributor

nishanttotla commented Feb 1, 2018

@ishidawataru following up on this PR. There has been no movement for a while, but could you rebase once more?

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 1, 2018

I will help @ishidawataru rebase this tomorrow.

@nishanttotla LGTY after rebase?

@nishanttotla
Copy link
Copy Markdown
Contributor

nishanttotla commented Feb 1, 2018

@AkihiroSuda I think we'll need to do a fresh review since it's been so long, but we'll process it fast this time since the changes are small. But overall LGTM.

Also ping @thaJeztah @mavenugo @abhi who may have more context on this.
cc @dperny @anshulpundir for SwarmKit review.

anshulpundir
anshulpundir approved these changes Feb 1, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@anshulpundir anshulpundir left a comment

Choose a reason for hiding this comment

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

Looks good.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 2, 2018

rebased, PTAL

AkihiroSuda
AkihiroSuda reviewed Feb 2, 2018
View reviewed changes
api/api.pb.txt
field {
name: "php_generic_services"
number: 19
number: 42
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Feb 2, 2018

Choose a reason for hiding this comment

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

I'm unfamiliar with this change.

I used golang:1.9.3 docker image with protoc-3.5.0-linux-x86_64.zip, and did make setup and make protos.

Copy link
Copy Markdown
Contributor

@nishanttotla nishanttotla Feb 5, 2018

Choose a reason for hiding this comment

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

@stevvooe is this supposed to happen?

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Feb 5, 2018

Choose a reason for hiding this comment

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

Possibly the PR was opened before the Bump to 3.5? See #2451

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Feb 5, 2018

Choose a reason for hiding this comment

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

Ah, looks like they were re-generated after that (likely with an outdated version of protoc; 58e42ec

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Feb 5, 2018

Choose a reason for hiding this comment

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

Opened #2503 to have just the proto changes

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 2, 2018

Flaky test seems unrelated

time="2018-02-02T08:00:59Z" level=error msg="task allocation failure" error="failed to retrieve network testID3 while allocating task testTaskID3" 
time="2018-02-02T08:00:59Z" level=error msg="Failed allocation for network testID5" error="failed while allocating driver state for network testID5: could not obtain vxlan id for pool 10.0.4.0/24: requested bit is already allocated" 
time="2018-02-02T08:00:59Z" level=error msg="task allocation failure" error="network testID5 attached to task testTaskID6 not allocated yet" 
time="2018-02-02T08:00:59Z" level=error msg="Failed allocation for service testServiceID4" error="requested bit is already allocated" 
time="2018-02-02T08:00:59Z" level=error msg="task allocation failure" error="service testServiceID4 to which this task testTaskID7 belongs has pending allocations" 
--- FAIL: TestNodeAllocator (0.02s)
	allocator_test.go:1047: timed out before watchNode found expected node state

@AkihiroSuda
support SCTP
4b4ee4f 
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
nishanttotla
nishanttotla approved these changes Feb 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@nishanttotla nishanttotla left a comment

Choose a reason for hiding this comment

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

LGTM after #2503 was merged.

@nishanttotla nishanttotla merged commit bbe24e0 into moby:master Feb 7, 2018
@thaJeztah thaJeztah mentioned this pull request Feb 10, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request Mar 6, 2018
Closed
This was referenced Mar 12, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

+3 more reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@nishanttotla nishanttotla nishanttotla approved these changes

@anshulpundir anshulpundir anshulpundir approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ishidawataru @aaronlehmann @diogomonica @nishanttotla @AkihiroSuda @thaJeztah @anshulpundir