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

[24.0 backport] c8d: Make sure the content isn't removed while we export #46021

Merged
merged 1 commit into from Jul 19, 2023

Conversation

thaJeztah
Copy link
Member


This change add leases for all the content that will be exported, once the image(s) are exported the lease is removed, thus letting containerd's GC to do its job if needed. This fixes the case where someone would remove an image that is still being exported.

This fixes the TestAPIImagesSaveAndLoad cli integration test.

(cherry picked from commit f3a6b0f)

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

This change add leases for all the content that will be exported, once
the image(s) are exported the lease is removed, thus letting
containerd's GC to do its job if needed. This fixes the case where
someone would remove an image that is still being exported.

This fixes the TestAPIImagesSaveAndLoad cli integration test.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
(cherry picked from commit f3a6b0f)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added status/2-code-review area/images kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Jul 19, 2023
@thaJeztah thaJeztah added this to the 24.0.5 milestone Jul 19, 2023
defer release(ctx)
defer func() {
if err := leasesManager.Delete(ctx, lease); err != nil {
logrus.WithError(err).Warn("cleaning up lease")
Copy link
Member Author

@thaJeztah thaJeztah Jul 19, 2023

Choose a reason for hiding this comment

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

Had a minor conflict in the imports, due to this branch not yet using the containerd log-package; I decided to change this to logrus (instead of the log.G(ctx)

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

I was my PR so yeah, LGTM :D

@thaJeztah thaJeztah merged commit 2b2a72c into moby:24.0 Jul 19, 2023
105 checks passed
@thaJeztah thaJeztah deleted the 24.0_backport_c8d-image-save-lease branch July 19, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/images containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants