feat: extend oci.Store with Delete()

[wangxiaoxuan273]

Part 4/4 of #454

This PR should be reviewed and merged after PR #606 #607 #608

[codecov-commenter]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (9febd7b) 75.19% compared to head (fad8ed4) 75.20%.

Files	Patch %	Lines
content/oci/oci.go	78.72%	8 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #614   +/-   ##
=======================================
  Coverage   75.19%   75.20%           
=======================================
  Files          59       59           
  Lines        5427     5473   +46     
=======================================
+ Hits         4081     4116   +35     
- Misses        992     1001    +9     
- Partials      354      356    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shizhMSFT]

@wangxiaoxuan273 Can you update the branch with the changes introduced in #606 so that we can review your PR more easily?

[Wwwsylvia]

Should we benchmark oci.DeletableStore v.s. oci.Store?

[eiffel-fl]

Hi!

Thank you a lot for this work and your previous! This is really valuable!
I have some comments, particularly regarding the whole organization of the code.
It should be possible to avoid copy/pasting of function between oci.Store and oci.DeletableStore by making the second inherits from the first.
Then, oci.DeletableStore would just implement Delete() as you did.
I will test the code later.

Best regards.

[eiffel-fl]

Hi!

I tested in my project (inspektor-gadget/inspektor-gadget#2162), it seems to work fine but there are still some remaining layers, am I missing something?

$ sudo ls /var/lib/ig/oci-store/blobs/sha256                      
14403318573ad08ef7e2cd426c32afac1365af3600dd8660dc83d9fcfb3852db  70a7726fe322dc6064d849d0f45fb8f3fc1dce2a69a94d79a61e13a2c6a28dd7
30d2db19d0afaa03e1a92549a00fd002d9c67c8e1a8f3a7ba6176bde1bc43c2b  95f570bdf511b42aff18c6568d16039cb1d795f89c77e69fb773fa1b2f19118e
328068b1b19152da4c5516f485cd6d45345aa9e00dc40b12e2b74f6f79bc6cd4  b1b01e47a71bf06f35c903b9b3ff9a3d4220fd03260e0bf637cc26d3f4bd8d2f
42e7e1feed065ef44a59ec1c6679a77bed6ce6fa974540bf919e36e5afdc0d10  db3a22450d530ed62fa06d3b33606cc3d592e8168cb010f4d38f2a39e87ef41b
5aee384ce7fed55309d917c846ad80d8dc979dbb37855a84a694955267e5024d  ea3f5d39490da7a6481939ce592f0670820b2f39e29b1669296a06804d2ed37e
$ sudo -E ./ig image list                                         

INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST      
ghcr.io/inspektor-gadget/gadget/trace_dns                      test                                                          95f570bdf511
ghcr.io/inspektor-gadget/gadget/trace_exec                     test                                                          30d2db19d0af
$ sudo -E ./ig image remove ghcr.io/inspektor-gadget/gadget/trace_dns:test
INFO[0000] Experimental features enabled                
$ sudo -E ./ig image remove ghcr.io/inspektor-gadget/gadget/trace_exec:test
INFO[0000] Experimental features enabled                
$ sudo -E ./ig image list                                         

INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST      
$ sudo ls /var/lib/ig/oci-store/blobs/sha256                      
14403318573ad08ef7e2cd426c32afac1365af3600dd8660dc83d9fcfb3852db  70a7726fe322dc6064d849d0f45fb8f3fc1dce2a69a94d79a61e13a2c6a28dd7
328068b1b19152da4c5516f485cd6d45345aa9e00dc40b12e2b74f6f79bc6cd4  b1b01e47a71bf06f35c903b9b3ff9a3d4220fd03260e0bf637cc26d3f4bd8d2f
42e7e1feed065ef44a59ec1c6679a77bed6ce6fa974540bf919e36e5afdc0d10  db3a22450d530ed62fa06d3b33606cc3d592e8168cb010f4d38f2a39e87ef41b
5aee384ce7fed55309d917c846ad80d8dc979dbb37855a84a694955267e5024d  ea3f5d39490da7a6481939ce592f0670820b2f39e29b1669296a06804d2ed37e

Best regards.

[wangxiaoxuan273]

Hi!

I tested in my project (inspektor-gadget/inspektor-gadget#2162), it seems to work fine but there are still some remaining layers, am I missing something?

$ sudo ls /var/lib/ig/oci-store/blobs/sha256                      
14403318573ad08ef7e2cd426c32afac1365af3600dd8660dc83d9fcfb3852db  70a7726fe322dc6064d849d0f45fb8f3fc1dce2a69a94d79a61e13a2c6a28dd7
30d2db19d0afaa03e1a92549a00fd002d9c67c8e1a8f3a7ba6176bde1bc43c2b  95f570bdf511b42aff18c6568d16039cb1d795f89c77e69fb773fa1b2f19118e
328068b1b19152da4c5516f485cd6d45345aa9e00dc40b12e2b74f6f79bc6cd4  b1b01e47a71bf06f35c903b9b3ff9a3d4220fd03260e0bf637cc26d3f4bd8d2f
42e7e1feed065ef44a59ec1c6679a77bed6ce6fa974540bf919e36e5afdc0d10  db3a22450d530ed62fa06d3b33606cc3d592e8168cb010f4d38f2a39e87ef41b
5aee384ce7fed55309d917c846ad80d8dc979dbb37855a84a694955267e5024d  ea3f5d39490da7a6481939ce592f0670820b2f39e29b1669296a06804d2ed37e
$ sudo -E ./ig image list                                         

INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST      
ghcr.io/inspektor-gadget/gadget/trace_dns                      test                                                          95f570bdf511
ghcr.io/inspektor-gadget/gadget/trace_exec                     test                                                          30d2db19d0af
$ sudo -E ./ig image remove ghcr.io/inspektor-gadget/gadget/trace_dns:test
INFO[0000] Experimental features enabled                
$ sudo -E ./ig image remove ghcr.io/inspektor-gadget/gadget/trace_exec:test
INFO[0000] Experimental features enabled                
$ sudo -E ./ig image list                                         

INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST      
$ sudo ls /var/lib/ig/oci-store/blobs/sha256                      
14403318573ad08ef7e2cd426c32afac1365af3600dd8660dc83d9fcfb3852db  70a7726fe322dc6064d849d0f45fb8f3fc1dce2a69a94d79a61e13a2c6a28dd7
328068b1b19152da4c5516f485cd6d45345aa9e00dc40b12e2b74f6f79bc6cd4  b1b01e47a71bf06f35c903b9b3ff9a3d4220fd03260e0bf637cc26d3f4bd8d2f
42e7e1feed065ef44a59ec1c6679a77bed6ce6fa974540bf919e36e5afdc0d10  db3a22450d530ed62fa06d3b33606cc3d592e8168cb010f4d38f2a39e87ef41b
5aee384ce7fed55309d917c846ad80d8dc979dbb37855a84a694955267e5024d  ea3f5d39490da7a6481939ce592f0670820b2f39e29b1669296a06804d2ed37e

Best regards.

Hi @eiffel-fl, I have looked into the command line output you posted above. From my understanding, you performed delete on the two tags trace_dns:test and trace_exec:test. And the two layers (or let's call them nodes for convenience) associated with these two tags (whose digests are 95f570bdf511 and 30d2db19d0af) are gone, while the other layers remain.

So do you expect the other layers to be deleted as well? The delete function in this pr is limited to delete a single node instead of a whole graph(a.k.a. a full image).

[eiffel-fl]

Hi!

Hi!
I tested in my project (inspektor-gadget/inspektor-gadget#2162), it seems to work fine but there are still some remaining layers, am I missing something?

SNIP

Best regards.

Hi @eiffel-fl, I have looked into the command line output you posted above. From my understanding, you performed delete on the two tags trace_dns:test and trace_exec:test. And the two layers (or let's call them nodes for convenience) associated with these two tags (whose digests are 95f570bdf511 and 30d2db19d0af) are gone, while the other layers remain.

So do you expect the other layers to be deleted as well? The delete function in this pr is limited to delete a single node instead of a whole graph(a.k.a. a full image).

This was my understanding and the potential root cause, which is actually not a problem and this PR does what it aims to do perfectly.
I nonetheless tried using Predecessors() to get the whole graph but it only removed the digests:

$ git diff                                                           francis/rmi *% u=
diff --git a/pkg/oci/oci.go b/pkg/oci/oci.go
index 176f44ca..5dc721f1 100644
--- a/pkg/oci/oci.go
+++ b/pkg/oci/oci.go
@@ -322,9 +322,17 @@ func DeleteGadgetImage(ctx context.Context, image string) error {
                return fmt.Errorf("resolving image: %w", err)
        }
 
-       err = ociDeletableStore.Delete(ctx, descriptor)
+       descriptors, err := ociDeletableStore.Predecessors(ctx, descriptor)
        if err != nil {
-               return fmt.Errorf("deleting image: %w", err)
+               return fmt.Errorf("getting image predecessors: %w", err)
+       }
+
+       descriptors = append([]ocispec.Descriptor{descriptor}, descriptors...)
+       for _, d := range descriptors {
+               err := ociDeletableStore.Delete(ctx, d)
+               if err != nil {
+                       return fmt.Errorf("deleting image: %w", err)
+               }
        }
 
        return nil
$ sudo ls -l /var/lib/ig/oci-store/blobs/sha256 | wc -l              francis/rmi *% u=
44
# Delete some images
$ sudo ls -l /var/lib/ig/oci-store/blobs/sha256 | wc -l              francis/rmi *% u=
39

Is my understanding of Predecessors() wrong? Is there any more work needed? Maybe I can give a hand?

Best regards.

[wangxiaoxuan273]

Hi @eiffel-fl, I just want to confirm if I understand your scenario above correctly.

Given this screenshot, did you expect all the 44 items to be deleted? but instead there are still 39 remaining after the delete, do I understand it right?

And, are the 44 items together a full image (aka. manifest, layers etc. )? I'd like to know the relation among them.

[shizhMSFT]

@eiffel-fl Thanks for reviewing this PR.

This PR aims to delete a single node of the graph in the OCI layout. That is, deleting a manifest will not delete its layers. Since layers are successors of a manifest, you should call content.Successors() to get its layers before deleting it. Predecessors() API is a reverse of Successors(). As you may know that layers in the OCI layout are de-deduplicated, a single layer can be shared by multiple manifests. In this case, deleting its layers when deleting a manifest may cause broken integrity of other images stored in the same OCI layout folder.

We noticed that users want to remove those dangling nodes (layers) when deleting a manifest. We can introduce garbage collection (GC) functionality after this PR being merged to do the job systematically.

[eiffel-fl]

Hi!

Hi @eiffel-fl, I just want to confirm if I understand your scenario above correctly.

Given this screenshot, did you expect all the 44 items to be deleted? but instead there are still 39 remaining after the delete, do I understand it right?

Using Predecessors() I would have image yes.
But it seems my understanding of this function is wrong (see #614 (comment)).

And, are the 44 items together a full image (aka. manifest, layers etc. )? I'd like to know the relation among them.

They are not a full images, but they belong to two different images that I "removed".

Best regards.

[eiffel-fl]

Hi!

@eiffel-fl Thanks for reviewing this PR.

You are welcome!

This PR aims to delete a single node of the graph in the OCI layout. That is, deleting a manifest will not delete its layers. Since layers are successors of a manifest, you should call content.Successors() to get its layers before deleting it. Predecessors() API is a reverse of Successors(). As you may know that layers in the OCI layout are de-deduplicated, a single layer can be shared by multiple manifests. In this case, deleting its layers when deleting a manifest may cause broken integrity of other images stored in the same OCI layout folder.

I perfectly understand the goal of this PR is to remove a single node and this PR is perfect as it is.
I am definitely not asking for this PR to be extended to handle removing all the nodes as it clearly involves more work and more thinking about it.
Moreover, having small and "subject-dedicated" PR is a really good thing as they are quicker to review and then merge, which leads to quicker iteration and innovation.

Regarding layers, we definitely need to be smart about removing them.
Indeed, a layer can be shared by different images and we remove it unconditionally, we will break the others images.
The thing here would be to add referencing counting to layers and remove them when they are referenced by no others images (easy to write, not so much to implement).

We noticed that users want to remove those dangling nodes (layers) when deleting a manifest. We can introduce garbage collection (GC) functionality after this PR being merged to do the job systematically.

We should definitely think about it and let this as future work.
Meanwhile, I will think about how I can be useful for this future work.
In any case, thank you for the present PR.

Best regards.

[eiffel-fl]

Hi.

For your information, I was able to achieve my goal of removing the whole image while ensuring a shared layer is not removed by using the following code:
inspektor-gadget/inspektor-gadget@015dc30#diff-570aea5baf5f0e0b7f46b570a48f173e8858df3436f091fb61d2e3c745db6f0eR309-R329

Once this PR is merged, I will open a PR here to upstream this code, so everyone can benefit from it.

Best regards.

[Wwwsylvia]

LGTM with a few minor suggestions

[shizhMSFT]

LGTM