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.

Sign up for GitHub

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

Jump to bottom

kind build node-image: Support Docker v25.0.1 #3487

Merged
merged 1 commit into from
Jan 25, 2024
Merged

kind build node-image: Support Docker v25.0.1 #3487

merged 1 commit into from
Jan 25, 2024

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Jan 21, 2024 •
edited
Loading

docker save in Docker v25 produces Docker/OCI dual-format archives:

  • repositories, manifest.json: for legacy Docker format
  • oci-layout, index.json (and blobs): for OCI format

However, pkg/build/nodeimage/internal/container/docker.EditArchive did not support rewriting OCI Index.

This was resulting in producing broken images with Docker v25:

We can just drop docker.EditArchive and use ctr images tag instead.

NOTE: This is still incompatible with Docker v25.0.0 due to moby/moby#47150. The issue was fixed in v25.0.1.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 21, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 21, 2024
@AkihiroSuda AkihiroSuda mentioned this pull request Jan 21, 2024
Closed
@AkihiroSuda
Copy link
Member Author

@aojea @BenTheElder Can we have a new release after merging this? 🙏

@dims
Copy link
Member

dims commented Jan 21, 2024

/assign @aojea
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 21, 2024
@AkihiroSuda AkihiroSuda force-pushed the docker-v25-avoid-broken-oci branch from c8ffa19 to 11a4351 Compare January 21, 2024 15:06
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 21, 2024
@AkihiroSuda AkihiroSuda changed the title Support Docker v25 kind build node-image: Support Docker v25 Jan 21, 2024
@stmcginnis stmcginnis mentioned this pull request Jan 23, 2024
Closed
@AkihiroSuda AkihiroSuda force-pushed the docker-v25-avoid-broken-oci branch from 11a4351 to 867cbde Compare January 24, 2024 14:42
@AkihiroSuda AkihiroSuda changed the title kind build node-image: Support Docker v25 kind build node-image: Support Docker v25.0.1 Jan 24, 2024
BenTheElder
BenTheElder reviewed Jan 24, 2024
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

We should consider just avoiding the archive edit entirely.
When we wrote this, ctr images tag didn't exist.

@BenTheElder
Copy link
Member

Alternately: Don't use Docker v25 yet, which we're definitely not doing elsewhere given the broken release.

@BenTheElder BenTheElder mentioned this pull request Jan 24, 2024
Closed
BenTheElder
BenTheElder reviewed Jan 24, 2024
View reviewed changes
go.sum Outdated Show resolved Hide resolved
@BenTheElder
Copy link
Member

@aojea @BenTheElder Can we have a new release after merging this? 🙏

Kubernetes @ HEAD typically requires KIND @ HEAD, so any test pipelines shouldn't be blocked on this part.

We need a release anyhow, just clarifying that part.

@AkihiroSuda
kind build node-image: Support Docker v25.0.1
10c59fd 
`docker save` in Docker v25 produces Docker/OCI dual-format archives:
- `repositories`, `manifest.json`: for legacy Docker format
- `oci-layout`, `index.json` (and blobs): for OCI format

However, `pkg/build/nodeimage/internal/container/docker.EditArchive` did not
support rewriting OCI Index.

This was resulting in producing broken images with Docker v25.
We can just drop `docker.EditArchive` and use `ctr images tag` instead.

See kubernetes/kubernetes issue 122894

NOTE: This is still incompatible with Docker v25.0.0 due to
moby/moby issue 47150. The issue was fixed in v25.0.1.

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda AkihiroSuda force-pushed the docker-v25-avoid-broken-oci branch from 867cbde to 10c59fd Compare January 25, 2024 00:07
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 25, 2024
@AkihiroSuda
Copy link
Member Author

We should consider just avoiding the archive edit entirely. When we wrote this, ctr images tag didn't exist.

Updated to use ctr images tag.
This command is available since 2019 (containerd v1.3).

BenTheElder
BenTheElder reviewed Jan 25, 2024
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
thank you!!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AkihiroSuda, BenTheElder, dims

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3ff0e90 into kubernetes-sigs:main Jan 25, 2024
27 checks passed
@AkihiroSuda AkihiroSuda mentioned this pull request Jan 25, 2024
Merged
@BenTheElder BenTheElder mentioned this pull request Feb 8, 2024
Closed
@chrischdi chrischdi mentioned this pull request Feb 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenTheElder BenTheElder BenTheElder left review comments

@aojea aojea Awaiting requested review from aojea

@neolit123 neolit123 Awaiting requested review from neolit123

Assignees

@dims dims

@BenTheElder BenTheElder

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AkihiroSuda @dims @BenTheElder @k8s-ci-robot @aojea