Skip to content
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

3081 implement stopoptions for container restart #3083

Conversation

s4ke
Copy link
Contributor

@s4ke s4ke commented Nov 8, 2022

closes #3081

- What I did

Upgrade moby/moby dependencies and try to fix compiler issues around StopOptions

- How I did it

go get
go mod vendor
Use the most straighforward fixes on each compiler issue

- How to test it

- Description for the changelog

Upgrade moby/moby dependencies to fix compiler issues around StopOptions

@s4ke
Copy link
Contributor Author

s4ke commented Nov 8, 2022

This is currently failing on TestAllocateWithInvalidSubnet as discussed in #3081 on my machine. Could this be a bug in libnetwork?

/cc @dperny

@s4ke
Copy link
Contributor Author

s4ke commented Nov 8, 2022

@thaJeztah PTAL if convenient, I think you are the right person to weigh in here, since the StopOptions commit was authored by you :)

go.sum Show resolved Hide resolved
@s4ke s4ke closed this Nov 8, 2022
@s4ke s4ke reopened this Nov 8, 2022
@s4ke s4ke force-pushed the 3081-implement-stopoptions-for-container-restart branch from a5396a6 to 0277a25 Compare November 8, 2022 17:08
@s4ke
Copy link
Contributor Author

s4ke commented Nov 8, 2022

Seems like TestAllocateWithInvalidSubnet fails due to this change in libnetworkd in insertBitMask of vendor/github.com/docker/docker/libnetwork/ipam/allocator.go

grafik

@s4ke
Copy link
Contributor Author

s4ke commented Nov 8, 2022

Yes, this was caused by the change in libnetworkd. this used to fail in line 574 of vendor/github.com/docker/docker/libnetwork/ipam/allocator.go, now it doesn't:
grafik

@s4ke
Copy link
Contributor Author

s4ke commented Nov 8, 2022

seems to be caused by this commit moby/moby@3a938df

I will do a PR to moby/moby to handle the case of 1.1.1.1/32 which was left out in the PR.

@@ -9,7 +9,7 @@ require (
github.com/cloudflare/cfssl v0.0.0-20180323000720-5d63dbd981b5
github.com/container-storage-interface/spec v1.2.0
github.com/docker/distribution v2.7.1+incompatible
github.com/docker/docker v20.10.3-0.20220408103430-7ea283fd9166+incompatible // v22.04 dev
github.com/docker/docker v20.10.3-0.20221105174132-4f3c5b2568c2+incompatible // v22.04 dev
Copy link
Member

Choose a reason for hiding this comment

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

We should probably pick a commit (head) from the 22.06 branch in moby/moby, as master is currently used to work on the release after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in any case, the PR in moby/moby I linked is blocking this as both have the same code path. Will update once merged.

@s4ke
Copy link
Contributor Author

s4ke commented Dec 13, 2022

Update from the PR in moby/moby: 1.1.1.1/32 is a valid network. The test in swarmkit is actually faulty and should probably be removed / revisited why it existed.

@thaJeztah
Copy link
Member

Thanks @s4ke ! I had this branch locally and updated it to v23.0.0-beta.1, and also included the fix for the failing test (as discussed on the moby ticket); carrying this in #3103 (I kept your commit as starting point).

@s4ke
Copy link
Contributor Author

s4ke commented Dec 15, 2022

so we can close this PR?

@thaJeztah
Copy link
Member

Yup! I marked the other one to close this one automatically; it's been merged 👍

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement StopOptions for ContainerRestart and ContainerStop from moby/moby
2 participants