Add cascadeDelete option to the Image and Build objects
#1742
Conversation
pbusko
changed the title
cascadeDelete option to the Image and Build objectscascadeDelete option to the Image and Build objects
| return err | ||
| } | ||
|
|
||
| if err := c.RegistryClient.Delete(keychain, build.Status.LatestImage); err != nil { |
There was a problem hiding this comment.
We would also need to delete all of the additional tags as well.
There was a problem hiding this comment.
I was under the assumption that a Build object produces exactly one image, while Image can produce multiple Build's. If each Build cleans up it's own image, would there be any leftovers? Or the assumption is incorrect?
There was a problem hiding this comment.
A build can push multiple images (see additionalTags). There is also the signed images and image attestations but that might be too much to try to delete and we probably don’t want to delete the attestations
There was a problem hiding this comment.
According to docs, the status.latestImage field contain the image's digest. It should be enough to delete just this (no necessity to delete all tags individually): https://github.com/opencontainers/distribution-spec/blob/main/spec.md#deleting-manifests.
Once deleted, a GET to /v2//manifests/ and any tag pointing to that digest will return a 404.
pkg/reconciler/build/build.go
Outdated
| } | ||
|
|
||
| if err := c.RegistryClient.Delete(keychain, build.Status.LatestImage); err != nil { | ||
| return err |
There was a problem hiding this comment.
Returning error might mean that builds get stuck if the finalized can't delete the image due to immutability rules in the registry. I think this should be a best effort cleanup
There was a problem hiding this comment.
Isn't it fair to keep the build if the desired outcome is not reached? The solution is not to enable the cleanup when the registry forbids objects deletion is used. If I set the cleanup to true but the image is still present, it should be considered an error, not a warning message
| } | ||
|
|
||
| if err := c.RegistryClient.Delete(keychain, build.Status.LatestImage); err != nil { | ||
| //logger.Printf(errors.Wrapf(err, "Could not delete image %q", build.Status.LatestImage)) |
There was a problem hiding this comment.
How should we handle the error in that case? It seems like error logging is not supposed to happen at the reconciliation stage.
There was a problem hiding this comment.
You can log and it will print to the controller logs
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1742 +/- ##
==========================================
- Coverage 67.34% 66.97% -0.38%
==========================================
Files 140 144 +4
Lines 8886 9104 +218
==========================================
+ Hits 5984 6097 +113
- Misses 2393 2482 +89
- Partials 509 525 +16 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Pavel Busko <[email protected]>
Co-authored-by: Nicolas Bender <[email protected]>
modulo11
force-pushed
the
add-image-delete-flag
branch
from
e4f074b to
6ee3a13
Compare
When
cascadeDeleteis set totrueon aBuildobject, the produced image by the build will be removed from the registry upon object deletion. If thecascadeDeleteis set on theImageobject, it will be propagated toBuilds