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

Remove support for pulling v2 schema1 #42300

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Apr 16, 2021

Last commit is a quick push / reminder of what needs to be done:

WIP: remove DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE hack

This PR will still fail, as we need to find a different solution to provision a "v2 schema1" registry to test pulling these manifests.

Some solutions;

  • create an image with the registry and images pre-loaded
  • export the registry's storage, and load it for tests
  • look for a suitable image on Docker Hub, and pull that (Docker Hub may currently be converting manifests; need to check)

Manifest v2 schema1 was deprecated in 4866f51 (#39365) and this PR
removes the push code for v2 schema1.

- Description for the changelog

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

@thaJeztah thaJeztah added this to the 21.xx milestone Apr 16, 2021
@thaJeztah thaJeztah force-pushed the carry_39384_remove_v2_schema1_push branch 4 times, most recently from 2a95a46 to 09e5b4d Compare November 18, 2021 15:02
@thaJeztah thaJeztah changed the title [WIP] Remove v2 schema1 push (carry 39384) Remove support for pushing and pulling v2 schema1 Nov 18, 2021
@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 18, 2021

Looks like we have one test depending on an ancient image;

=== RUN   TestDockerNetworkSuite/TestConntrackFlowsLeak
    docker_cli_network_unix_test.go:1751: assertion failed: 
        Command:  /usr/local/cli/docker run -d --name server --net testbind -p 8080:8080/udp appropriate/nc sh -c while true; do echo hello | nc -w 1 -lu 8080; done
        ExitCode: 125
        Error:    exit status 125
        Stdout:   
        Stderr:   Unable to find image 'appropriate/nc:latest' locally
        /usr/local/cli/docker: Error response from daemon: unsupported manifest media type and no default available: application/vnd.docker.distribution.manifest.v1+prettyjws.
        See '/usr/local/cli/docker run --help'.
        
        
        Failures:
        ExitCode was 125 expected 0
        Expected no error
    docker_cli_network_unix_test.go:46: [db3f1e5df27d0] daemon is not started
    --- FAIL: TestDockerNetworkSuite/TestConntrackFlowsLeak (0.32s)

Image is literally just an apk install netcat-openbsd

IMAGE                                                                     CREATED       CREATED BY                                                                                          SIZE      COMMENT
sha256:867ff23d913e68c1c032a4c547d315fd1d2f543cb85e772fff325d4756e37732   6 years ago   /bin/sh -c #(nop) CMD ["nc"]                                                                        0B
<missing>                                                                 6 years ago   /bin/sh -c #(nop) ENTRYPOINT &{["/entrypoint.sh"]}                                                  0B
<missing>                                                                 6 years ago   /bin/sh -c #(nop) COPY file:cf241fd8b9027436b115396345e77913cf86121c3a7ef848845d9ec974280c61 in /   143B
<missing>                                                                 6 years ago   /bin/sh -c apk add --update netcat-openbsd && rm -rf /var/cache/apk/*                               3.02MB
<missing>                                                                 6 years ago   /bin/sh -c #(nop) ADD file:43a95cc218d164ff589cb91519964373d53b607469f5ccce725631916392cd88 in /    5.25MB

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 18, 2021

I just recalled that I already looked at that test / image at some point; #34832. Let me give it another go

Original image was built from https://github.com/appropriate/docker-nc/tree/master/latest

@thaJeztah

This comment has been minimized.

@thaJeztah thaJeztah force-pushed the carry_39384_remove_v2_schema1_push branch 2 times, most recently from 8db2387 to 531034e Compare November 19, 2021 13:40
@thaJeztah thaJeztah marked this pull request as ready for review November 19, 2021 15:03
@thaJeztah
Copy link
Member Author

Whoop; CI is green now, so that's a start

@thaJeztah
Copy link
Member Author

thaJeztah commented Nov 19, 2021

We discussed this PR in the maintainers meeting. In general, everyone seemed to agree that (permitted there's no large use of these images) it should be ok to remove this.

The provisional schema 2 v1 format became deprecated when v2 was introduced in 2015 (distribution/distribution#1068). Originally, support in Docker was meant to be removed in 2018 (#37874), but extended for some time to allow users and registries to migrate (#39365 / docker-archive#284 / docker/cli#1956).

We briefly discussed an issue we encountered some time ago where (due to some bug on Docker Hub?) images were pushed (or converted on pull?) as schema 2 v1 images, but @tianon mentioned that that issue was resolved, and hasn't been seen for quite a while, so probably not a concern.

Some other concerns were raised by @tonistiigi about the actual removal:

  • Even though we have printed a warning, uses tend to ignore warnings, so they might still be caught by surprise (admitted: even our own CI turned out to be using some v1 image: TestConntrackFlowsLeak: use busybox "nc" #43031)
  • The deprecation warning may not be visible to all users. When we deprecated support, we found that some registries only supported the deprecated format (quay.io, some big companies in China, and (I think) Red Hat's registry). Given that there was no option for users to address that, we made a change in distribution: modify warning logic when pulling v2 schema1 manifests #39736 to only print the warning when pulling from Docker Hub. That said; this was more than 2 Years ago now, and those registries now support the current spec.
  • Containerd currently has support for the deprecated schema; this was done to provide a clean migration path for users switching from DockerShim to Containerd CRI.
  • As a consequence of the above, it would still be possible to:
    • use a v2 schema1 image in a Dockerfile (FROM), because BuildKit uses containerd's distribution code (note that pushing is not possible even in that case).
    • use v2 schema1 images in Kubernetes (using containerd CRI), but pulling the same image with Docker would fail.
    • users using the DockerShim for Kubernetes on the other hand, will see these images fail (given: this might be a good incentive to migrate to using containerd CRI, as the DockerShim is being deprecated in upstream kubernetes)

For the above, it was suggested if we could synchronize this with containerd (both deprecating and removing v2 schema 1).

@cpuguy83 started a discussion with the containerd maintainers (copying from the slack conversation);

Discussing removing schema1 support in docker. It came up that "we should synchronize this with containerd". Are there thoughts on removing this in cri and ctr?

From @dmcgowan:

Is there any data on who (if anyone) is still relying on this? Previously it was quay which had a bunch of these images and a few big companies in China seemed to use it (3+ years ago at this point)
in terms of timeline, after 1.6 I am thinking of proposing 1.7 scoped to many of the new APIs we have discussed, then immediately after 1.7 ripping out deprecated stuff and starting a 2.0 beta. This would be an excellent candidate for removal then

From @fuweid:

yeah. 2018-2019,alibaba group was using schema1ed images(10%) internally. I didn't see it in 2020. Agreed to deprecate it in 1.7

So questions from the above:

  • Do we have any insight (from Docker Hub?) into how many pulls are still happening for these deprecated images?
  • (related); what's the proposed deprecation gonna look like for Docker Hub? I think there's some automatic conversion of images (?) will this be disabled? I don't think pulling of legacy will be disabled in other ways (correct?)
  • Are we ok with removing support (for this release), before containerd (possible other tools) have removed support?
  • For the docker build (with BuildKit) case; do we consider this a problem? Do we want an option to disable the v2 schema 1 support in containerd (so that BuildKit can disable it in the vendored code)?

@justincormack ^^ perhaps you have some input on this?

Let me also /cc

@giuseppe
Copy link
Contributor

@vrothberg @mtrmac PTAL

@justincormack
Copy link
Contributor

  • We still need to do the data analysis from Docker Hub. I believe the number of pulls is very low, and we can consider converting images (or asking owners to convert) if necessary, its just a pull and push after all.
  • We won't remove any automatic conversion in the short term although we also want to remove that in the long term. I suspect there is more of this going on, again we need to look at data.
  • Happy to remove before containerd, and even if buildkit doesn't. We will get signal to see if it affects users which will help everyone. If users have niche use cases they can still use old software.
  • buildkit should follow containerd if they use their code.

@thaJeztah
Copy link
Member Author

Happy to remove before containerd, and even if buildkit doesn't. We will get signal to see if it affects users which will help everyone. If users have niche use cases they can still use old software.

I agree, I think it should be fine; we've deprecated it long time ago now, and have been delaying removal multiple times. Let me also check with the containerd maintainers if they would be ok with deprecating (not removing) this. I think it's good to warn users early, to raise awareness of this deprecated version of the spec. (cc @dmcgowan @dims @fuweid)

@vrothberg @mtrmac PTAL

Thanks @giuseppe! I wasn't sure who was the right person to ping 😅
It's of course up to each project, but I thought it would be nice if we could signal deprecation across the whole "container ecosystem"; at least I don't think anyone is a fan of having to maintain too much code to keep backward compatibility with ancient schemas 😂

@vrothberg
Copy link

vrothberg commented Nov 22, 2021

Thank you for pulling us in.

at least I don't think anyone is a fan of having to maintain too much code to keep backward compatibility with ancient schemas

Totally agree 😄 I will reach out to Quay and ask if they have stats on schema 1 images. I am always happy to add things but am terrified of removals in fear of breaking users.

@mtrmac
Copy link
Contributor

mtrmac commented Nov 22, 2021

WRT c/image, it’s quite true that the conversion from schema1 (having to compute diffID values) is a hassle and it’s a bit attractive to drop that code, but the code already exists, and the users affected by these changes need some path forward, so I’m inclined to keep the code (of the generic copy pipeline, at least) as is, including the current schema1 support. (New transports might easily choose not to support schema1, and the generic copy pipeline’s conversion implementation would deal with that.)

Registries are already turning off schema1 (by default?), which seems sufficient to push the ecosystem forward. It might be worth removing schema1 from the default list of formats to try converting to…

My primary wish in this transition is to have distribution/distribution#2925 , to allow clients to report useful error messages. Just dropping the format support from the clients entirely is a way to make that moot, I suppose.

@mtrmac
Copy link
Contributor

mtrmac commented Nov 22, 2021

Thank you for pulling us in.

at least I don't think anyone is a fan of having to maintain too much code to keep backward compatibility with ancient schemas

I will reach out to Quay and ask if they have stats on schema 1 images. I am always happy to add things but am terrified of removals in fear of breaking users.

… and schema1-only on-premise server deployments? The public quay.io transition was relatively recent, I have no idea what that means about the transition schedule of private instances. Private, internet-unknown registries that are unlikely to upgrade are my primary worry WRT the client changes.

@thaJeztah thaJeztah force-pushed the carry_39384_remove_v2_schema1_push branch from 81a06bb to 08ecf2a Compare August 24, 2023 15:02
@thaJeztah thaJeztah changed the title Remove support for pushing and pulling v2 schema1 Remove support for pulling v2 schema1 Aug 24, 2023
@thaJeztah thaJeztah force-pushed the carry_39384_remove_v2_schema1_push branch 2 times, most recently from e7fa171 to 8c77d9a Compare August 25, 2023 00:12
@thaJeztah thaJeztah force-pushed the carry_39384_remove_v2_schema1_push branch from 8c77d9a to cc546af Compare September 4, 2023 09:01
@dmcgowan
Copy link
Member

dmcgowan commented Sep 4, 2023

Change looks good, what is remaining to get this in?

thaJeztah and others added 5 commits May 24, 2024 14:50
Manifest v2 schema1 was deprecated in 4866f5139a1 and this commit
removes the push code for v2 schema1.

This reverts commit f695e98,
adjusted for changes that were made since

daemon: do not mkdir trust directory

Remove push tests and move UUID tests to integration

Partial revert of f23a51a.

Only the schema1 push tests are removed but the schema1 pull tests
are still desired.

The UUID test is moved from integration-cli to integration.

Signed-off-by: Tibor Vass <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Also remove the DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE from Jenkins

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This image format is only used for docker save / docker load.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This format-check was only used for v1 images.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the carry_39384_remove_v2_schema1_push branch from 6f719ac to 8591fed Compare May 24, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants