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

Add image delete target #8989

Merged
merged 2 commits into from
Oct 5, 2023
Merged

Add image delete target #8989

merged 2 commits into from
Oct 5, 2023

Conversation

dmcgowan
Copy link
Member

Adds atomicity to image delete when deleting from a list.

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dmcgowan
Copy link
Member Author

/test all

@dmcgowan dmcgowan marked this pull request as ready for review August 22, 2023 00:33
@dmcgowan
Copy link
Member Author

Updated and marked as ready for review

djdongjin
djdongjin reviewed Aug 22, 2023
View reviewed changes
image_store.go Outdated Show resolved Hide resolved
metadata/images.go Outdated Show resolved Hide resolved
@dmcgowan
Copy link
Member Author

Updated

@dmcgowan dmcgowan requested a review from fuweid September 5, 2023 18:22
fuweid
fuweid approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

henry118
henry118 reviewed Sep 6, 2023
View reviewed changes
metadata/images_test.go
original: imageBase(),
input: images.Image{
Target: ocispec.Descriptor{
Size: 20,
Copy link
Member

Choose a reason for hiding this comment

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

wonder why do we use 20 here while all other places have 10?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should probably document it better, // Input target size determines target digest, base image uses 10. The digest is generated with the size testcase.original.Target.Digest = digest.FromString(fmt.Sprintf("%s-%d", testcase.name, testcase.original.Target.Size)) so to change the image on update, the size must change

henry118
henry118 approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@henry118 henry118 left a comment

Choose a reason for hiding this comment

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

LGTM

laurazard
laurazard reviewed Sep 7, 2023
View reviewed changes
images/image.go
@@ -72,6 +73,13 @@ func SynchronousDelete() DeleteOpt {
}
}

func DeleteTarget(target *ocispec.Descriptor) DeleteOpt {
Copy link
Member

Choose a reason for hiding this comment

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

Could we maybe add some godoc here (a la

containerd/images/image.go

Lines 66 to 68 in 74e205f

// SynchronousDelete is used to indicate that an image deletion and removal of
// the image resources should occur synchronously before returning a result.
func SynchronousDelete() DeleteOpt {
)

laurazard
laurazard approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@laurazard laurazard left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM <3! (just left a little nit for a comment)

@dmcgowan
api: update image service to support target in delete request
f8fb2da 
Signed-off-by: Derek McGowan <derek@mcg.dev>
@dmcgowan
Add delete target to image remove
2ce971d 
Adds atomicity to image delete when deleting from a list.

Signed-off-by: Derek McGowan <derek@mcg.dev>
@dmcgowan
Copy link
Member Author

Updated

@AkihiroSuda AkihiroSuda merged commit 66aab63 into containerd:main Oct 5, 2023
45 checks passed
@dmcgowan dmcgowan mentioned this pull request Nov 2, 2023
Merged
@dmcgowan dmcgowan mentioned this pull request Apr 19, 2024
Merged
@dmcgowan dmcgowan deleted the add-image-delete-target branch April 20, 2024 00:43
@dmcgowan dmcgowan mentioned this pull request May 3, 2024
Merged
@dmcgowan dmcgowan mentioned this pull request May 17, 2024
Merged
@dmcgowan dmcgowan mentioned this pull request Jun 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djdongjin djdongjin djdongjin left review comments

@henry118 henry118 henry118 approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@fuweid fuweid fuweid approved these changes

@laurazard laurazard laurazard approved these changes

Assignees
No one assigned
Labels
impact/changelog
Projects
Pull Request Review
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dmcgowan @k8s-ci-robot @henry118 @AkihiroSuda @fuweid @djdongjin @laurazard