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

Update "opt disabling" patch to remove "opt" plugin entirely #15

Merged
merged 2 commits into from
May 24, 2024

Conversation

tianon
Copy link
Owner

@tianon tianon commented May 24, 2024

Also, update testing to reproduce the original failure and validate that "opt" is successfully gone. 🚀

(As discovered in tianon/dockerfiles#713)

I can't seem to find the upstream change that causes this failure (nor figure out why it only fails if the config gets round-tripped/specified explicitly somehow), but maybe it's been wrong for a while and it's only Docker 23 changes that trigger the failure. 😭

(Tests are updated to explicitly check for the plugin being properly disabled, though!)

@tianon
Copy link
Owner Author

tianon commented May 24, 2024

Oof, this doesn't work in the other direction -- it now does not successfully disable opt by default. So there's a minor upstream bug of some kind here. 🤔

@tianon
Copy link
Owner Author

tianon commented May 24, 2024

Hmm, so I took this approach because I thought it'd be able to be overwritten in configuration, but it turns out that the way the config is parsed that's not even true, so I should reconsider simply patching out opt entirely (which I wanted to avoid, but probably easy enough).

Also, update testing to reproduce the original failure and validate that "opt" is successfully gone. 🚀
@tianon tianon changed the title Update "opt disabling" patch to use the correct plugin ID Update "opt disabling" patch to remove "opt" plugin entirely May 24, 2024
@tianon
Copy link
Owner Author

tianon commented May 24, 2024

Ok, OP description/title/patch updated.

@@ -66,7 +79,9 @@ cat <<-'EOBASH'
docker buildx create --name tianon --node tianon --driver docker-container --driver-opt image=tianon/buildkit --bootstrap
printf 'FROM hello-world\nRUN ["/hello"]' | docker buildx build --builder tianon --tag hello-build:buildx -

docker image inspect --format '.' hello-build:classic hello-build:buildkit hello-build:buildx > /dev/null
docker buildx build --tag hello-build:buildx-git 'https://github.com/docker-library/hello-world.git#3fb6ebca4163bf5b9cc496ac3e8f11cb1e754aee:amd64/hello-world'
Copy link
Owner Author

Choose a reason for hiding this comment

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

Sneaking this test in too -- I thought I broke this as well, but in testing it turns out I'd just had a typo and the error message I got was something obscure like "failed to find username" instead of a classic 404 (:upside_down_face:), but it felt worth keeping anyways because it's a good simple smoke test of the Git integration.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I guess I should do the same for docker build without buildx. 🤔

@tianon
Copy link
Owner Author

tianon commented May 24, 2024

I've tested some of my old builds, and I'm now wondering how this ever worked?????

Even my 1.6.21 (the oldest I have for bookworm) builds fail -- they fail to disable the plugin, but my 25 and 28 builds fail in the same place this build was, so confirmed definitely something to do with Docker 20 -> 23 behavior changes in how it starts containerd, which I think is due to Docker now generating a config file for containerd and just specifying that invokes the "check the config" codepath which then balks at this default-supplied disabled plugin.

@tianon tianon merged commit da9bc19 into tianon:main May 24, 2024
2 checks passed
@tianon tianon deleted the containerd-opt branch May 24, 2024 22:04
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.

1 participant