Stop deleting cache images #1136

dlion · 2023-06-29T12:29:21Z

With this PR the cache will just override the original image with the new image without attempting to delete it first.

Original Issue: [RFC 0116] Stop deleting cache images for newer Platform API versions #803
Readable RFC: https://github.com/buildpacks/rfcs/blob/5c1d82583b0baf1e1fbb6cd2d6eaa201aabaa276/text/0000-stop-deleting-cache-images.md
Tracking RFC issue: [RFC #0116] - Stop deleting cache images rfcs#268

Resolves #803 and it partially resolves buildpacks/rfcs#268

jabrown85

This looks great! Did we say we didn't need to guard this behind an API version @natalieparellano? I don't have a strong opinion. The delete is not in spec.

jjbustamante · 2023-06-30T14:12:01Z

@jabrown85 @natalieparellano

One doubt about this based on the motivation section in the RFC.

How does it impact pack users? Do we need to advertise to users they need to delete or take care of the cache images saved in the registry? or do we need to create an issue on pack to think about this and if we want to offer some feature to manage the cache?

No related directly to this conversation on slack but just wanted to keep track of it

jabrown85 · 2023-06-30T14:50:14Z

How does it impact pack users? Do we need to advertise to users they need to delete or take care of the cache images saved in the registry? or do we need to create an issue on pack to think about this and if we want to offer some feature to manage the cache?

I think that is up to pack on how to handle that. If pack wants to continue the existing experience then it can coded directly into pack to handle the registry operations required. If we aren't worried about this change for pack users, a note in the subsequent release maybe? At least, that is my thinking.

I would be 👍 holding this until pack team decides a path though as pack is our number one platform 😄

dlion · 2023-07-03T10:24:17Z

@jabrown85
I agree on let pack decides how to handle it, is there anything I should do to speed up the decision process?
Of course I'm happy to open a PR to pack if the team decides to go in that direction

dlion · 2023-07-04T08:36:46Z

UPDATE: I requested to talk about it in the next working group
(https://cloud-native.slack.com/archives/C0331B61A1Y/p1688459224497819)

cache/image_cache.go

jabrown85 · 2023-07-06T14:29:28Z

As @natalieparellano pointed out in working group - we previously decided to gate this new behavior on platform api version @dlion. Let's do that for our platform and builder operators.

dlion · 2023-07-06T14:49:42Z

As @natalieparellano pointed out in working group - we previously decided to gate this new behavior on platform api version @dlion. Let's do that for our platform and builder operators.

Sure, I'll comment this PR whenever I open the spec PR so we can be in the same page

cache/cache_deleter.go

dlion · 2023-07-10T15:02:31Z

All right, after a bit of refactoring now the PR should be ready to be reviewed 😄
What I changed:

Now we just need to pass the ApiPlatform evaluated condition a the very first level and not pass the whole object all the way down.
The boolean flag helps us to abstract the ApiPlatform concept from the technical implementation of the deleter allowing us in the future to activate/deactivate the deletion using other factors like for example an external conf or test it without the need of using mocks.
The check is now done in an isolated and specialized component

Thoughts are welcomed 😃

cache/image_cache.go

natalieparellano

@dlion nice work! I made a few suggestions - LMK your thoughts :)

analyzer.go

cmd/lifecycle/main.go

cache/image_deleter_test.go

cache/image_deleter.go

cache/image_cache_test.go

natalieparellano

@dlion thanks for your work on this! It looks good to me.

@jabrown85 did you want to have another look?

natalieparellano · 2023-07-11T21:39:17Z

@dlion are you able to fix the DCO?

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

…est" This reverts commit 17e646fc39602777a37977dd9416e59aa62f6d04. Signed-off-by: Domenico Luciani <dluciani@vmware.com>

…the deletion of the cache images Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

…ways set has enabled Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

```go func Benchmark(b *testing.B) { mockController := gomock.NewController(b) fakeImageComparer := cacheMock.NewMockImageComparer(mockController) testLogger := cmd.DefaultLogger imageDeleter := NewImageDeleter(fakeImageComparer, testLogger, true) for i := 0; i < b.N; i++ { fakeOrigImage := fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeImage"}) fakeNewImage := fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeNewImage"}) fakeImageComparer.EXPECT().ImagesEq(fakeOrigImage, fakeNewImage).AnyTimes().Return(false, nil) imageDeleter.DeleteOrigImageIfDifferentFromNewImage(fakeOrigImage, fakeNewImage) } } ``` The code above produced this result: * without the go-subroutine ``` goos: darwin goarch: amd64 pkg: github.com/buildpacks/lifecycle/cache cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Benchmark Benchmark-12 3501 8995839 ns/op PASS ``` * with the go-subroutine ``` goos: darwin goarch: amd64 pkg: github.com/buildpacks/lifecycle/cache cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Benchmark Benchmark-12 3560 9133704 ns/op PASS ``` Speed increased by 1.53% ns/op Signed-off-by: Domenico Luciani <dluciani@vmware.com>

dlion · 2023-07-12T08:16:07Z

@natalieparellano there was a commit not signed-off, fixed it 😄 I think then we can merge 🎉

dlion · 2023-07-12T08:42:16Z

@natalieparellano Codecov failed again 🫠 we should reproduce what we have done with pack (buildpacks/pack#1809).
Can you re-run the Github Actions?

* Remove deleteOrigImage function from the cache and relative test Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Revert "Remove deleteOrigImage function from the cache and relative test" This reverts commit 17e646fc39602777a37977dd9416e59aa62f6d04. Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Implemented a new component called cache deleter which takes care of the deletion of the cache images Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Adjusted the name of the struct field Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Move the imade deleter instatiation up to the main Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Add parameter to enable/diable the deletion functionality, for now always set has enabled Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Add feature guard based on the platformAPI version Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Fixing some test titles Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Introduce ImageComparer component Signed-off-by: Domenico Luciani <dluciani@vmware.com> * Add async go-subroutine to the delete call to speed up the process ```go func Benchmark(b *testing.B) { mockController := gomock.NewController(b) fakeImageComparer := cacheMock.NewMockImageComparer(mockController) testLogger := cmd.DefaultLogger imageDeleter := NewImageDeleter(fakeImageComparer, testLogger, true) for i := 0; i < b.N; i++ { fakeOrigImage := fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeImage"}) fakeNewImage := fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeNewImage"}) fakeImageComparer.EXPECT().ImagesEq(fakeOrigImage, fakeNewImage).AnyTimes().Return(false, nil) imageDeleter.DeleteOrigImageIfDifferentFromNewImage(fakeOrigImage, fakeNewImage) } } ``` The code above produced this result: * without the go-subroutine ``` goos: darwin goarch: amd64 pkg: github.com/buildpacks/lifecycle/cache cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Benchmark Benchmark-12 3501 8995839 ns/op PASS ``` * with the go-subroutine ``` goos: darwin goarch: amd64 pkg: github.com/buildpacks/lifecycle/cache cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Benchmark Benchmark-12 3560 9133704 ns/op PASS ``` Speed increased by 1.53% ns/op Signed-off-by: Domenico Luciani <dluciani@vmware.com> --------- Signed-off-by: Domenico Luciani <dluciani@vmware.com>

dlion requested a review from a team as a code owner June 29, 2023 12:29

This was referenced Jun 29, 2023

The platform won't delete cache images buildpacks/docs#594

Merged

[RFC #0116] - Stop deleting cache images buildpacks/rfcs#268

Closed

jabrown85 approved these changes Jun 30, 2023

View reviewed changes

natalieparellano reviewed Jul 6, 2023

View reviewed changes

cache/image_cache.go Show resolved Hide resolved

dlion marked this pull request as draft July 7, 2023 14:59

dlion force-pushed the 268-stop-deleting-cache-images branch from 94d0bbe to 37ebe13 Compare July 7, 2023 15:04

joe-kimmel-vmw reviewed Jul 7, 2023

View reviewed changes

cache/cache_deleter.go Outdated Show resolved Hide resolved

dlion marked this pull request as ready for review July 10, 2023 14:59

dlion commented Jul 10, 2023

View reviewed changes

cache/image_cache.go Outdated Show resolved Hide resolved

natalieparellano reviewed Jul 10, 2023

View reviewed changes

dlion force-pushed the 268-stop-deleting-cache-images branch 4 times, most recently from 1396fc8 to dc75380 Compare July 11, 2023 16:06

dlion requested a review from natalieparellano July 11, 2023 18:06

natalieparellano approved these changes Jul 11, 2023

View reviewed changes

jabrown85 approved these changes Jul 11, 2023

View reviewed changes

dlion and others added 4 commits July 12, 2023 10:14

Remove deleteOrigImage function from the cache and relative test

865396b

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Revert "Remove deleteOrigImage function from the cache and relative t…

ead75df

…est" This reverts commit 17e646fc39602777a37977dd9416e59aa62f6d04. Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Implemented a new component called cache deleter which takes care of …

e4b828d

…the deletion of the cache images Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Adjusted the name of the struct field

d31abe3

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Domenico Luciani added 6 commits July 12, 2023 10:14

Move the imade deleter instatiation up to the main

76fd691

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Add parameter to enable/diable the deletion functionality, for now al…

c0d50d7

…ways set has enabled Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Add feature guard based on the platformAPI version

be6913a

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Fixing some test titles

1d13b52

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

Introduce ImageComparer component

dab3e74

Signed-off-by: Domenico Luciani <dluciani@vmware.com>

dlion force-pushed the 268-stop-deleting-cache-images branch from dc75380 to 0fb6d92 Compare July 12, 2023 08:14

natalieparellano merged commit 9c455eb into buildpacks:main Jul 12, 2023
7 checks passed

dlion deleted the 268-stop-deleting-cache-images branch July 13, 2023 08:48

dlion mentioned this pull request Jul 13, 2023

Remove async go-subroutine delete keeping it sync #1152

Merged

natalieparellano mentioned this pull request Aug 18, 2023

Attempt to delete previous cache image fails buildpacks/pack#1132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop deleting cache images #1136

Stop deleting cache images #1136

dlion commented Jun 29, 2023 •

edited

Loading

jabrown85 left a comment

jjbustamante commented Jun 30, 2023

jabrown85 commented Jun 30, 2023

dlion commented Jul 3, 2023

dlion commented Jul 4, 2023

jabrown85 commented Jul 6, 2023

dlion commented Jul 6, 2023

dlion commented Jul 10, 2023 •

edited

Loading

natalieparellano left a comment

natalieparellano left a comment

natalieparellano commented Jul 11, 2023

dlion commented Jul 12, 2023

dlion commented Jul 12, 2023

Stop deleting cache images #1136

Stop deleting cache images #1136

Conversation

dlion commented Jun 29, 2023 • edited Loading

jabrown85 left a comment

Choose a reason for hiding this comment

jjbustamante commented Jun 30, 2023

jabrown85 commented Jun 30, 2023

dlion commented Jul 3, 2023

dlion commented Jul 4, 2023

jabrown85 commented Jul 6, 2023

dlion commented Jul 6, 2023

dlion commented Jul 10, 2023 • edited Loading

natalieparellano left a comment

Choose a reason for hiding this comment

natalieparellano left a comment

Choose a reason for hiding this comment

natalieparellano commented Jul 11, 2023

dlion commented Jul 12, 2023

dlion commented Jul 12, 2023

dlion commented Jun 29, 2023 •

edited

Loading

dlion commented Jul 10, 2023 •

edited

Loading