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

Upgrade moby/moby dependencies to reflect StopOptions change (carry 3083) #3103

Conversation

thaJeztah
Copy link
Member

Update to the latest v23.0.0-dev

full diff: moby/moby@7ea283f...v23.0.0-beta.1

rewrite TestAllocateWithInvalidSubnet as it was no longer valid

This test was added in d5f9e7f / #62 (renamed in b2ad71f / #93). At the time, docker did not support /32 subnets, but starting with
moby/moby@3a938df / moby/moby#42626, docker added support for /31 and /32 subnets, rendering this tests invalid.

- How to test it

- Description for the changelog

Update to the latest v23.0.0-dev

full diff: moby/moby@7ea283f...v23.0.0-beta.1

rewrite TestAllocateWithInvalidSubnet as it was no longer valid

This test was added in d5f9e7f (renamed
in b2ad71f). At the time, docker did not
support /32 subnets, but starting with
moby/moby@3a938df,
docker added support for /31 and /32 subnets, rendering this tests invalid.

Signed-off-by: Martin Braun <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@@ -432,7 +432,8 @@ func (c *containerConfig) volumeCreateRequest(mount *api.Mount) *volume.VolumeCr
labels = mount.VolumeOptions.Labels
}

return &volume.VolumeCreateBody{
// FIXME: do we need the ClusterVolumeSpec here?
Copy link
Contributor

@vvoland vvoland Dec 15, 2022

Choose a reason for hiding this comment

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

Ugh, it looks like we should pass ClusterVolumeSpec for API >=1.42

https://github.com/moby/moby/blob/aac5e5906d31d339fd9ed67fe9198d2444efa7d7/api/server/router/volume/volume_routes.go#L106-L124

20.10.3 seems to be API 1.41, so of we upgrade to 23.0 (API >=1.42) we probably need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this code though 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it’s a new feature, and at this time “not yet supported” on the executor implementation in SwarmKit itself.

The implementation in docker lives in https://github.com/moby/moby/blob/v23.0.0-beta.1/daemon/cluster/executor/container/controller.go

So yes, it should be added at some point (perhaps I should create a tracking ticket for it) for swarmkit running standalone as well, but first priority for me was to make sure it is at least compatible with the upcoming (v23) release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #3104

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for the explanation!

@thaJeztah
Copy link
Member Author

Let me bring this one in, and do a vendor update in docker/docker

@thaJeztah thaJeztah merged commit 0da442b into moby:master Dec 15, 2022
@thaJeztah thaJeztah deleted the carry_3083_implement_stopoptions_for_container_restart branch December 15, 2022 13:22
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
3 participants