[FIX] delete upload leases after tagging#64
[FIX] delete upload leases after tagging#64yeetypete wants to merge 1 commit intopsviderski:mainfrom
Conversation
psviderski
left a comment
There was a problem hiding this comment.
Nice one! 👍 Please see one comment about label conflicts when uploading multiple tags concurrently.
The following e2e tests fail on your PR:
=== CONT TestUnregistryPushPull/push/pull_multi-platform_OCI_image_with_regclient
unregistry_test.go:485:
Error Trace: /Users/spy/Workspace/uncloud/unregistry/test/e2e/unregistry_test.go:485
Error: Should be true
Test: TestUnregistryPushPull/push/pull_multi-platform_OCI_image_with_regclient
Messages: Manifest sha256:46300c35aa557736b6fffd302205498b90b71afc0fd6ce4587cd8be0dda7b1b3 should be available
--- FAIL: TestUnregistryPushPull/push/pull_multi-platform_OCI_image_with_regclient (0.42s)
=== RUN TestUnregistryPushPull/push/pull_multi-platform_Docker_image_with_regclient
=== PAUSE TestUnregistryPushPull/push/pull_multi-platform_Docker_image_with_regclient
=== CONT TestUnregistryPushPull/push/pull_multi-platform_Docker_image_with_regclient
unregistry_test.go:485:
Error Trace: /Users/spy/Workspace/uncloud/unregistry/test/e2e/unregistry_test.go:485
Error: Should be true
Test: TestUnregistryPushPull/push/pull_multi-platform_Docker_image_with_regclient
Messages: Manifest sha256:51240564e556984b9f7af480dfafaea2561721b6adfafbdc7352b98804cd8c4f should be available
--- FAIL: TestUnregistryPushPull/push/pull_multi-platform_Docker_image_with_regclient (0.37s)
Please also consider updating one of the tests to check that the space is reclaimed after deleting an image, for example, here:
unregistry/test/e2e/unregistry_test.go
Lines 155 to 157 in 0c685ca
| opts := []leases.Opt{ | ||
| leases.WithRandomID(), | ||
| leases.WithExpiration(leaseExpiration), | ||
| leases.WithLabels(map[string]string{leaseLabel: repo.Name()}), |
There was a problem hiding this comment.
There is a problem with using such non-specific lease label. If there are two or more concurrent uploads for images ubuntu:tag1 and ubuntu:tag2, they both will use the same label ubuntu and risk to remove each other's leases.
We need to make this label more specific to ideally identify only one particular upload session.
Or maybe using the full canonical image name with a tag would be even more beneficial. For example, if the upload session is aborted in the middle, the leases will stay in place, right? Then the next run for the same image name will complete the upload and clean up the leases from the failed session.
Currently if you run
docker image rmon an image uploaded by unregistry it will untag it but not remove the content in containerd. This is because the image content is kept around until the leases expire (1 hr).In this PR we add a lease label and then remove the leases tagged with the lease label after a sucessful upload in
tagService. Nowdocker image rm,docker image prune, etc. immediately work as expected without needing to manually usectrto remove the leases.Test Instructions
Note that its easiest to observe the difference on a clean remote that has no docker images on it yet (i.e. pruned with
sudo ctr -n moby leases rm $(sudo ctr -n moby leases ls -q)anddocker image prune -a).Reproduce Current Behavior
Observe Behavior After Change