Skip to content

[Fix]: image delete in convert (#3510) #4121

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

Merged
merged 1 commit into from
Apr 18, 2025

Conversation

apostasie
Copy link
Contributor

Hopefully, this should be the last one of these (fingers crossed).

This part of convert has the same code pattern as seen in containerd.Convert.

My understanding is this:

  • we ask for deletion of the image known as newI.Name
  • if asynchronous, it may kick in before we then call Create
  • if synchronous, it will happen before, for sure
  • in both cases, if that image already existed, and if it shared any layer with the to-be-created image, these layers will be deleted (either directly by the call to delete, or maybe because of garbage collection)
  • one way or the other, the then newly created image might then miss layers (triggering the content digest not found error)

I believe this understanding of how containerd API works is (at least partly) valid, as the prior series of patch have significantly reduced the symptoms.

As far as I can tell, the last place where we see this is #3510 - which does presumably hit this code path.

I will run the CI a few times to confirm, and then that should fix #3510.

Note: obviously, containerd image store API is wildly confusing and hard to reason about (and possibly has bugs - at least converter.Convert seems broken). This has clearly tripped the most savvy people here. Suggesting we isolate it behind a simpler, tested, nerdctl.ImageStore, and prevent casual code from using the containerd store directly.

@apostasie apostasie changed the title [DEBUGGING] Fix image delete in convert [DEBUGGING] Fix image delete in convert (#3510) Apr 17, 2025
@apostasie
Copy link
Contributor Author

One count for #4068

@apostasie apostasie closed this Apr 17, 2025
@apostasie apostasie reopened this Apr 17, 2025
@apostasie
Copy link
Contributor Author

One count for #4119

@apostasie apostasie closed this Apr 17, 2025
@apostasie apostasie reopened this Apr 17, 2025
@apostasie
Copy link
Contributor Author

Ignoring ubuntu broken servers.

@apostasie apostasie closed this Apr 18, 2025
@apostasie apostasie reopened this Apr 18, 2025
@apostasie
Copy link
Contributor Author

apostasie commented Apr 18, 2025

One count for #3556

@apostasie apostasie closed this Apr 18, 2025
@apostasie apostasie reopened this Apr 18, 2025
@apostasie apostasie closed this Apr 18, 2025
@apostasie apostasie reopened this Apr 18, 2025
@apostasie apostasie closed this Apr 18, 2025
@apostasie apostasie reopened this Apr 18, 2025
@apostasie
Copy link
Contributor Author

One count for #4046

@apostasie apostasie closed this Apr 18, 2025
@apostasie apostasie reopened this Apr 18, 2025
@apostasie apostasie changed the title [DEBUGGING] Fix image delete in convert (#3510) Fix image delete in convert (#3510) Apr 18, 2025
@apostasie
Copy link
Contributor Author

@AkihiroSuda this is ready for review.

I am still going to run the CI a few more times to increase confidence that the bug is gone, but so far I see only the other flakies and no longer this %^&*$ content digest not found.

@apostasie apostasie marked this pull request as ready for review April 18, 2025 16:31
@apostasie apostasie changed the title Fix image delete in convert (#3510) [Fix image delete in convert (#3510) Apr 18, 2025
@apostasie apostasie changed the title [Fix image delete in convert (#3510) [Fix]: image delete in convert (#3510) Apr 18, 2025
@apostasie apostasie closed this Apr 18, 2025
@apostasie apostasie reopened this Apr 18, 2025
@apostasie
Copy link
Contributor Author

One count for #4046 - damn we need to fix this one.

@apostasie apostasie closed this Apr 18, 2025
@apostasie apostasie reopened this Apr 18, 2025
@AkihiroSuda AkihiroSuda added this to the v2.0.5 milestone Apr 18, 2025
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit d2b3963 into containerd:main Apr 18, 2025
118 of 120 checks passed
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.

[CI]: content digest not found: IPFS not completely fixed
2 participants