feat: GC on oci.Store.Delete

[wangxiaoxuan273]

Part of #472, this PR implements recursive GC for oci.Store. A field AutoGarbageCollection of oci.Store is added, default value is true.

[codecov-commenter]

Codecov Report

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

Comparison is base (1d9ad6c) 75.36% compared to head (b454af9) 75.41%.

Files	Patch %	Lines
content/oci/oci.go	76.74%	7 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
+ Coverage   75.36%   75.41%   +0.05%     
==========================================
  Files          59       59              
  Lines        5570     5606      +36     
==========================================
+ Hits         4198     4228      +30     
- Misses       1011     1015       +4     
- Partials      361      363       +2     

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

[eiffel-fl]

Hi!

I tested your branch on Inspektor Gadget on top of inspektor-gadget/inspektor-gadget#2162 with the following patch applied:

diff --git a/go.mod b/go.mod
index 3c639b3c..78b6e5b6 100644
--- a/go.mod
+++ b/go.mod
@@ -198,4 +198,4 @@ replace sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.13.1
 
 replace github.com/vishvananda/netns => github.com/inspektor-gadget/netns v0.0.5-0.20230524185006-155d84c555d6
 
-replace oras.land/oras-go/v2 => github.com/eiffel-fl/oras-go/v2 v2.0.0-20231121181441-6f7761eb16e9
+replace oras.land/oras-go/v2 => github.com/wangxiaoxuan273/oras-go/v2 v2.0.0-20231129084233-5eff85c1314a
diff --git a/go.sum b/go.sum
index b92ceb87..de51e87a 100644
--- a/go.sum
+++ b/go.sum
@@ -129,8 +129,6 @@ github.com/docker/go-metrics v0.0.1 h1:AgB/0SvBxihN0X8OR4SjsblXkbMvalQ8cjmtKQ2rQ
 github.com/docker/go-metrics v0.0.1/go.mod h1:cG1hvH2utMXtqgqqYE9plW6lDxS3/5ayHzueweSI3Vw=
 github.com/docker/go-units v0.5.0 h1:69rxXcBk27SvSaaxTtLh/8llcHD8vYHT7WSdRZ/jvr4=
 github.com/docker/go-units v0.5.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
-github.com/eiffel-fl/oras-go/v2 v2.0.0-20231121181441-6f7761eb16e9 h1:MAiGi7/PtIIs2quCYywtotr9MI6XWJUl5pK+I2JKxXQ=
-github.com/eiffel-fl/oras-go/v2 v2.0.0-20231121181441-6f7761eb16e9/go.mod h1:W2rj9/rGtsTh9lmU3S0uq3iwvR6wNvUuD8nmOFmWBUY=
 github.com/emicklei/go-restful/v3 v3.10.2 h1:hIovbnmBTLjHXkqEBUz3HGpXZdM7ZrE9fJIZIqlJLqE=
 github.com/emicklei/go-restful/v3 v3.10.2/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc=
 github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
@@ -464,6 +462,8 @@ github.com/vbatts/tar-split v0.11.5 h1:3bHCTIheBm1qFTcgh9oPu+nNBtX+XJIupG/vacinC
 github.com/vbatts/tar-split v0.11.5/go.mod h1:yZbwRsSeGjusneWgA781EKej9HF8vme8okylkAeNKLk=
 github.com/vishvananda/netlink v1.2.1-beta.2 h1:Llsql0lnQEbHj0I1OuKyp8otXp0r3q0mPkuhwHfStVs=
 github.com/vishvananda/netlink v1.2.1-beta.2/go.mod h1:twkDnbuQxJYemMlGd4JFIcuhgX83tXhKS2B/PRMpOho=
+github.com/wangxiaoxuan273/oras-go/v2 v2.0.0-20231129084233-5eff85c1314a h1:fwagKv0yhY62dRzQJZXSwc11g5phnWep4IMYl1ei3cg=
+github.com/wangxiaoxuan273/oras-go/v2 v2.0.0-20231129084233-5eff85c1314a/go.mod h1:W2rj9/rGtsTh9lmU3S0uq3iwvR6wNvUuD8nmOFmWBUY=
 github.com/xlab/treeprint v1.2.0 h1:HzHnuAF1plUN2zGlAFHbSQP2qJ0ZAD3XF5XD7OesXRQ=
 github.com/xlab/treeprint v1.2.0/go.mod h1:gj5Gd3gPdKtR1ikdDK6fnFLdmIS0X30kTTuNd/WEJu0=
 github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
diff --git a/pkg/oci/oci.go b/pkg/oci/oci.go
index 515351b6..bdc3cc6d 100644
--- a/pkg/oci/oci.go
+++ b/pkg/oci/oci.go
@@ -346,28 +346,28 @@ func DeleteGadgetImage(ctx context.Context, image string) error {
                return fmt.Errorf("resolving image: %w", err)
        }
 
-       images, err := listGadgetImages(ctx, ociStore)
-       if err != nil {
-               return fmt.Errorf("listing images: %w", err)
-       }
-
-       digest := descriptor.Digest.String()
-       for _, img := range images {
-               imgFullName := fmt.Sprintf("%s:%s", img.Repository, img.Tag)
-               if img.Digest == digest && imgFullName != fullName {
-                       // We cannot blindly delete a whole image tree.
-                       // Indeed, it is possible for several image names to point to the same
-                       // underlying image, like:
-                       // REPOSITORY            TAG    DIGEST
-                       // docker.io/library/bar latest f959f580ba01
-                       // docker.io/library/foo latest f959f580ba01
-                       // Where foo and bar are different names referencing the same image, as
-                       // the digest shows.
-                       // In this case, we just untag the image name given by the user.
-                       return ociStore.Untag(ctx, fullName)
-               }
-       }
-       return deleteTree(ctx, ociStore, descriptor)
+//     images, err := listGadgetImages(ctx, ociStore)
+//     if err != nil {
+//             return fmt.Errorf("listing images: %w", err)
+//     }
+
+//     digest := descriptor.Digest.String()
+//     for _, img := range images {
+//             imgFullName := fmt.Sprintf("%s:%s", img.Repository, img.Tag)
+//             if img.Digest == digest && imgFullName != fullName {
+//                     // We cannot blindly delete a whole image tree.
+//                     // Indeed, it is possible for several image names to point to the same
+//                     // underlying image, like:
+//                     // REPOSITORY            TAG    DIGEST
+//                     // docker.io/library/bar latest f959f580ba01
+//                     // docker.io/library/foo latest f959f580ba01
+//                     // Where foo and bar are different names referencing the same image, as
+//                     // the digest shows.
+//                     // In this case, we just untag the image name given by the user.
+//                     return ociStore.Untag(ctx, fullName)
+//             }
+//     }
+       return ociStore.Delete(ctx, descriptor)
 }
 
 func getTagFromImage(image string) (string, error) {

And everything works as expected:

$ sudo -E ./ig image list                                            
INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                          DIGEST      
docker.io/library/sockets                                      latest                                       f8a8d91b99c6
$ sudo ls /var/lib/ig/oci-store/blobs/sha256                         
2e199d2fa02dfffcffea861b778d4dffefb7f0796662f076562c9c8f435fdb4b  e050fbbe7dd90d4e5a8563780ee0da294edd37cb6111fd7
98c5df1f30bd9eb2b892b2cb4dab504fe16799be59a6d3a86d3160dfad6c758f  f13a89b07cdc61366a2f1bc9a33dfb111258e10b065a193
ac032b892ec07e214ca18978a6c41b415f1a4ba7d15fd753ed8336ed7fcefe4f  f8ad3217ef61a29fd6ae902091a5722f6c2bfa22aa5b061
$ sudo -E ./ig image remove docker.io/library/sockets                
INFO[0000] Experimental features enabled                
$ sudo ls /var/lib/ig/oci-store/blobs/sha256                         
$ sudo -E ./ig image list                                            
INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST

I took a slight look at the code, but did not find something to say (I found one nit in the other PR).
I will take a deeper look later though.

Best regards.

[shizhMSFT]

I just noticed that this PR does not consider the GC on referrers.

For example, consider the following graph:

graph TD;
  A[Manifest List A]
  B[Manifest B]
  C[Manifest C]
  D[Layer D]
  E[Layer E]
  F[Layer F]
  G[Artifact Manifest G]
  H[Layer H]
  A --> B
  A --> C
  B --> D
  B --> E
  C --> E
  C --> F
  G -- subject --> C
  G --> H

Loading

Deleting Manifest C should also delete G, H, F.

@Wwwsylvia What do you think?

[wangxiaoxuan273]

I just noticed that this PR does not consider the GC on referrers.

For example, consider the following graph:

graph TD;
  A[Manifest List A]
  B[Manifest B]
  C[Manifest C]
  D[Layer D]
  E[Layer E]
  F[Layer F]
  G[Artifact Manifest G]
  H[Layer H]
  A --> B
  A --> C
  B --> D
  B --> E
  C --> E
  C --> F
  G -- subject --> C
  G --> H

Loading

Deleting Manifest C should also delete G, H, F.

@Wwwsylvia What do you think?

Do we need to delete the referrer? According to the distribution spec, the referrer can exist without the existence of the referenced manifest.

[Wwwsylvia]

I just noticed that this PR does not consider the GC on referrers.
For example, consider the following graph:

graph TD;
A[Manifest List A]
B[Manifest B]
C[Manifest C]
D[Layer D]
E[Layer E]
F[Layer F]
G[Artifact Manifest G]
H[Layer H]
A --> B
A --> C
B --> D
B --> E
C --> E
C --> F
G -- subject --> C
G --> H

Loading

  graph TD;

A[Manifest List A]
B[Manifest B]
C[Manifest C]
D[Layer D]
E[Layer E]
F[Layer F]
G[Artifact Manifest G]
H[Layer H]
A --> B
A --> C
B --> D
B --> E
C --> E
C --> F
G -- subject --> C
G --> H

Deleting Manifest C should also delete G, H, F.
@Wwwsylvia What do you think?

Do we need to delete the referrer? According to the distribution spec, the referrer can exist without the existence of the referenced manifest.

Referrers can exist without its subject. But when a user is deleting a manifest, they probably want to delete the referrers as well.
I think we can have an option to control whether to delete referrers or not.

[eiffel-fl]

Hi!

Thank you for polishing this code!
I tested it and got a segfault, I have an idea of the fix, can you please take a look and share your opinion?

Best regards.

[eiffel-fl]

Hi!

I took another look at your contribution.
While I do not have any comments on the code, as it perfectly does its jobs, I am wondering if I am not missing something.

Indeed, in my use case, I want to delete the whole image, but testing with your code I still have some dangling blobs:

$ sudo ls /var/lib/ig/oci-store/blobs/sha256                   francis/rmi-upstream *%
225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7  b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313  c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1  f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb
$ sudo -E ./ig image remove execs                              francis/rmi-upstream *%
INFO[0000] Experimental features enabled                
Successfully removed execs
# Expected ls output is empty.
$ sudo ls /var/lib/ig/oci-store/blobs/sha256                   francis/rmi-upstream *%
225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7  c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313  f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb
b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37

I understand this difference as your code use Referrers() and then delete all the referrers to the descriptor being deleted, while the one I used previously deleted the successors.
Taking a deeper look at Referrers(), this function acts on predecessors.
But, if B is a successor of A, then A is a predecessor of B?
I suspect I am missing OCI-related knowledge, but shouldn't a successor also be considered as a referrer?

Best regards.

[wangxiaoxuan273]

Hi!

I took another look at your contribution. While I do not have any comments on the code, as it perfectly does its jobs, I am wondering if I am not missing something.

Indeed, in my use case, I want to delete the whole image, but testing with your code I still have some dangling blobs:

$ sudo ls /var/lib/ig/oci-store/blobs/sha256                   francis/rmi-upstream *%
225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7  b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313  c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1  f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb
$ sudo -E ./ig image remove execs                              francis/rmi-upstream *%
INFO[0000] Experimental features enabled                
Successfully removed execs
# Expected ls output is empty.
$ sudo ls /var/lib/ig/oci-store/blobs/sha256                   francis/rmi-upstream *%
225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7  c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313  f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb
b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37

I understand this difference as your code use Referrers() and then delete all the referrers to the descriptor being deleted, while the one I used previously deleted the successors. Taking a deeper look at Referrers(), this function acts on predecessors. But, if B is a successor of A, then A is a predecessor of B? I suspect I am missing OCI-related knowledge, but shouldn't a successor also be considered as a referrer?

Best regards.

Hi @eiffel-fl,

Thanks for the message. Referrer is a concept independent from successor. Referrer is related to the subject field of a manifest. To be precise, consider the following manifest A and manifest B:
Manifest A { layers: [LayerA], subject: nil}
Manifest B { layers: [LayerB], subject: Manifest A}

We say that Manifest B is a referrer of Manifest A.
A referrer is considered a predecessor. The referenced manifest is considered a successor (child) of its referrer. In this case, Manifest A is a successor of Manifest B, just like Layer B is a successor of Manifest B.

But, if B is a successor of A, then A is a predecessor of B?

This is correct. In the above case, Layer B and Manifest A are successors of Manifest B, and Manifest B is a predecessor of Layer B and Manifest A.

[shizhMSFT]

I suspect I am missing OCI-related knowledge, but shouldn't a successor also be considered as a referrer?

@eiffel-fl The concept of referrers is from the distribution-spec while the concepts of successors and predecessors are from the graph thoery (precisely, directed acyclic graph or DAG). In other words, referrers of a certain node are predecessors, of which subject is that node.

[shizhMSFT]

LGTM

[shizhMSFT]

LGTM

[Wwwsylvia]

LGTM

[eiffel-fl]

Hi!

/* SNIP */

Hi @eiffel-fl,

Thanks for the message. Referrer is a concept independent from successor. Referrer is related to the subject field of a manifest. To be precise, consider the following manifest A and manifest B: Manifest A { layers: [LayerA], subject: nil} Manifest B { layers: [LayerB], subject: Manifest A}

First, thank you for the merge of this cool work 🎉!
I think I understand, but if we remove Manifest B, manifest A may be unreachable?
So, we should also take a look at successors (this is more to get a a better understanding and if the problem I shared is actually a real one or only mine).

I suspect I am missing OCI-related knowledge, but shouldn't a successor also be considered as a referrer?

@eiffel-fl The concept of referrers is from the distribution-spec while the concepts of successors and predecessors are from the graph thoery (precisely, directed acyclic graph or DAG). In other words, referrers of a certain node are predecessors, of which subject is that node.

It makes sense, hence the fact I am more familiar with predecessors and successors words rather than OCI one. THank you for shedding some light here.

Best regards and have an happy end of the year!

[Wwwsylvia]

I think I understand, but if we remove Manifest B, manifest A may be unreachable?
So, we should also take a look at successors (this is more to get a a better understanding and if the problem I shared is actually a real one or only mine).

Hi @eiffel-fl , Happy New Year! 🎆

In this particular case,

• A is a successor (subject) of B. For example, it can be an image manifest
• B is a referrer and a predecessor of A. For example, it can be the manifest of a signature of the image
• A can live without its referrer B, while B does not make sense without its subject A

So we shouldn't delete A when we delete B.
However, we should delete B when we delete A. And this is what we do in PR #656 .

[eiffel-fl]

Hi!

Hi @eiffel-fl , Happy New Year! 🎆

In this particular case,

* `A` is a successor (subject) of `B`. For example, it can be an image manifest

* `B` is a referrer and a predecessor of `A`. For example, it can be the manifest of a signature of the image

* `A` can live without its referrer `B`, while `B` does not make sense without its subject `A`

So we shouldn't delete A when we delete B. However, we should delete B when we delete A. And this is what we do in PR #656 .

OK, thank you for the clarification!
I will definitely take a look at #656 as I guess a call to GC() would perfectly suit my use case :D!

Best regards!