feat: extend oci.Store with DeleteTree(). #645

eiffel-fl · 2023-11-20T10:36:58Z

Hi!

Following the merge of #614, I opened this PR to propose DeleteTree().
This function will delete the content matching the given descriptor but also its children if they do not have any other parent.
We use this code in Inspektor Gadget to delete OCI image:
inspektor-gadget/inspektor-gadget#2162
inspektor-gadget/inspektor-gadget@2488118#diff-570aea5baf5f0e0b7f46b570a48f173e8858df3436f091fb61d2e3c745db6f0eR298
While it is still not merged at the moment, it was nonetheless validated.

If you see any way to improve this PR, feel free to share your feedback.

Best regards and thank you in advance.

content/oci/oci.go

shizhMSFT · 2023-11-21T15:32:39Z

@eiffel-fl Thank you for proposing the DeleteTree().

Instead of deleting a tree, I'm thinking about garbage collection (GC).

There are many benefits of GC over simply deleting a tree.

GC can be performed after several combination of Push() and Delete() operations.
GC can clean up any dangling nodes.

The basic idea is to mimic the garbage collection behavior of a real registry.

Feature Request: Consider supporting size management for OCI Store #472

/cc @wangxiaoxuan273 @Wwwsylvia

eiffel-fl · 2023-11-21T16:07:36Z

Hi!

@eiffel-fl Thank you for proposing the DeleteTree().

Instead of deleting a tree, I'm thinking about garbage collection (GC).

There are many benefits of GC over simply deleting a tree.
* GC can be performed after several combination of `Push()` and `Delete()` operations.

* GC can clean up any dangling nodes.
The basic idea is to mimic the garbage collection behavior of a real registry.

After thinking a bit about it, having GC makes more sense as the GC would be on the store side and the CLI side would not need to handle it.
So, a DeleteTree()-like operation would rather go through all the layers used by the image to decrement their reference counter and call Delete() in case it reaches 0?
Is this what you have in mind?
I can maybe think about something but I also need to see how it can articulate with my use case (which is really similar to docker rmi).

Best regards.

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

codecov-commenter · 2023-11-22T08:31:36Z

Codecov Report

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

Comparison is base (3776676) 75.20% compared to head (d57c2d1) 75.10%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
content/oci/oci.go	40.00%	8 Missing and 4 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
- Coverage   75.20%   75.10%   -0.10%     
==========================================
  Files          59       59              
  Lines        5473     5500      +27     
==========================================
+ Hits         4116     4131      +15     
- Misses       1001     1009       +8     
- Partials      356      360       +4

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

wangxiaoxuan273 · 2023-11-23T08:23:16Z

@eiffel-fl Thanks for opening this PR. Let's make future discussions on the #472 issue page, so that more stakeholders can participate.

Wwwsylvia · 2023-12-29T08:22:38Z

@eiffel-fl Given that #653 has been merged, shall we close this PR? Thanks for bringing this up!

eiffel-fl · 2023-12-29T08:40:18Z

Hi!

@eiffel-fl Given that #653 has been merged, shall we close this PR? Thanks for bringing this up!

#653 handled the referrers, i.e. predecessors, while this PR handles the successors.
When I tested my code with #653, I am getting dangling blobs, which is normal, but does not suit my use case:

$ 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

So, I am wondering if we should not add some code here to remove also the successors?
Indeed, this seems to be the use case to remove and OCI image by its tag.
Of course, adding a dedicated function to do so seems a bad idea and we should rather modify the existing.
So, I would like to get your opinion on this point? Particularly because if we agree I can modify this PR to tackle this point in the proper way.

Best regards.

Wwwsylvia · 2023-12-29T09:01:21Z

Hi @eiffel-fl , can you help us to understand the relationships among those blobs you listed?

$sudo ls /var/lib/ig/oci-store/blobs/sha256  
225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7  b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313  c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1  f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb

A mermaid diagram would be very helpful.

eiffel-fl · 2023-12-29T09:55:32Z

Hi!

Hi @eiffel-fl , can you help us to understand the relationships among those blobs you listed?

$sudo ls /var/lib/ig/oci-store/blobs/sha256  
225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7  b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313  c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1  f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb

A mermaid diagram would be very helpful.

Sure, sorry for the lack of details, here is the diagram:

graph TD;
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1 --> 662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1 --> f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313 --> b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313 --> 225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7
f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb --> b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb --> c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd

Basically, we have the following:

the root is a JSON file containing information over its children.
children of the root are JSON containing information over their children. We have two of them here because we have two architectures (amd64 and arm64).
the leaves are eBPF binaries (one for each architecture) and a YAML config file which is shared by all architectures.

By using DeleteTree(), I give the root ID, i.e. b0eff7f0ec and it will the whole tree as the nodes are only referred by it.

Best regards.

Wwwsylvia · 2023-12-29T10:07:17Z

@eiffel-fl I have some further questions:

What are their media types?
Is any of them a subject of another one?
Which of them are recorded in index.json?

eiffel-fl · 2023-12-29T10:21:32Z

@eiffel-fl I have some further questions:

Sure!

1. What are their media types?

application/vnd.gadget.ebpf.program.v1+binary for eBPF binaries,
application/vnd.gadget.config.v1+yaml for YAML
and application/vnd.oci.image.index.v1+json for JSON.

2. Is any of them a `subject` of another one?

Can you please precise how can I get this information 😅?

3. Which of them are recorded in `index.json`?

Here is its output:

root@pwmachine:/var/lib/ig/oci-store# cat index.json | jq
{
  "schemaVersion": 2,
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.index.v1+json",
      "digest": "sha256:b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1",
      "size": 491,
      "annotations": {
        "org.opencontainers.image.ref.name": "docker.io/library/execs:latest"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313",
      "size": 581,
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    },
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb",
      "size": 581,
      "platform": {
        "architecture": "arm64",
        "os": "linux"
      }
    }
  ]
}

Best regards.

eiffel-fl · 2023-12-29T12:38:53Z

For the record, I tested with #656 and I am still getting some blobs even after calling GC().
I am wondering if something is not wrong on our (Inspektor Gadget) side, particularly the fact to have the JSON for architectures being available in index.json.

Wwwsylvia · 2024-01-02T07:27:15Z

Hi @eiffel-fl , thanks for the clarification. I now understand what's happening.

TL;DR: the behavior you observed is by design. We don't automatically delete the successor manifests along with the deleted index, if they are present in index.json.

Let me put the diagram in another way:

graph TD;
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1[Index:latest] --> 662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313
b0eff7f0ec2a27b8d71fe904cf373fa2002c6d669db54c25a9542cf2cb5f46d1 --> f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313[Manifest:amd64] --> b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313 --> 225a8e697e260684d9451ccf8a622cd5548b0ae5fac6e13c416cd1d85247b7b7
f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb[Manifest:arm64] --> b4be31f07444fb9be6dee8cd1349aab3c780d333b63406be10c5c14c1b39dd37
f61eef5ff800b2e4366c6d2e7311e95ce8dc3cff5c020a3a810429656fd93efb --> c06a86dad7e582686b53bec9c729b92fc56c838b59cc11d0105c2c20f3158dcd

We know that:

Index:latest references two manifests: Manifest:amd64 and Manifest:arm64
Index:latest, Manifest:amd64, and Manifest:arm64 are all recorded in index.json

When one is deleting Index:latest, they may expect:

Manifest:amd64 and Manifest:arm64 also get deleted
Manifest:amd64 and Manifest:arm64 are not deleted (they wants to manually delete Manifest:amd64 later and keep Manifest:arm64)

oras-go could not tell the user's purpose, so it chooses not to automatically delete Manifest:amd64 and Manifest:arm64.
To delete the whole graph, you need to manually delete Manifest:amd64 and Manifest:arm64 after deleting Index:latest.

The idea is that oras-go considers everything in index.json is meaningful.
That is to say, if Manifest:amd64 and Manifest:arm64 were not present in index.json, they would get automatically deleted together with Index:latest.

Does this make sense to you?

Wwwsylvia · 2024-01-02T07:29:53Z

BTW, may I know what tool did you use to generate this OCI layout?

Wwwsylvia · 2024-01-02T07:34:50Z

Is any of them a subject of another one?

Can you please precise how can I get this information 😅?

Oh you can just look into the manifests and see if they have a subject field, like:

cat /var/lib/ig/oci-store/blobs/sha256/662ca58536d5a03864781eb42870f6712927981383f1dab1449d9adf80067313 | jq .

But in this particular case, the answer is that: none of them is a subject of another. 😀

eiffel-fl · 2024-01-02T08:58:14Z

Hi!

Hi @eiffel-fl , thanks for the clarification. I now understand what's happening.

TL;DR: the behavior you observed is by design. We don't automatically delete the successor manifests along with the deleted index, if they are present in index.json.

Thank you for the precision, it makes everything clear.
Now, I am really wondering if we do not do something wrong as I find it odd to have the architecture manifests in index.json, as, from my understanding, the index.json should only contain index which do the abstraction for the architectures.

BTW, may I know what tool did you use to generate this OCI layout?

We do it ourselves using oras-go:
https://github.com/inspektor-gadget/inspektor-gadget/blob/75c4d288defc30e736c5338aae1e63babb9560b2/pkg/oci/build.go#L172
We call createManifestForTarget() for each architecture, so I suspect something in this function is wrong and lead to these manifests to be written in index.json.

Oh you can just look into the manifests and see if they have a subject field, like:

Thank you for sharing, I will for sure remember!

Best regards.

Wwwsylvia · 2024-01-02T10:26:46Z

Now, I am really wondering if we do not do something wrong as I find it odd to have the architecture manifests in index.json, as, from my understanding, the index.json should only contain index which do the abstraction for the architectures.

Actually, it is oras-go to blame!
Currently oras-go will automatically add manifests into index.json, no matter it has a tag or not. This was to make sure referrers manifests without tags were still trackable.
We might need to improve this design.

eiffel-fl · 2024-01-02T10:33:37Z

Now, I am really wondering if we do not do something wrong as I find it odd to have the architecture manifests in index.json, as, from my understanding, the index.json should only contain index which do the abstraction for the architectures.

Actually, it is oras-go to blame! Currently oras-go will automatically add manifests into index.json, no matter it has a tag or not. This was to make sure referrers manifests without tags were still trackable. We might need to improve this design.

Oh OK!
In this case, can you please point which part of the code you think is guilty?
I could then try to write a workaround and open a PR to fix this behavior.

Wwwsylvia · 2024-01-02T11:13:50Z

This is not a bug, and it is not straightforward to change it🤔.
We need to work out a more proper design for this. Would you like to open an issue for discussion?

eiffel-fl · 2024-01-02T11:35:10Z

This is not a bug, and it is not straightforward to change it🤔. We need to work out a more proper design for this. Would you like to open an issue for discussion?

Sure!
I will open an issue, so we can discuss it deeper and once opened I will close this PR as it is not the proper solution.

eiffel-fl · 2024-01-02T11:41:56Z

Closing as we should continue discussion in #664.

Wwwsylvia · 2024-01-02T11:51:09Z

@eiffel-fl Thank you!

eiffel-fl requested review from sajayantony, shizhMSFT, SteveLasker and Wwwsylvia as code owners November 20, 2023 10:36

TerryHowe reviewed Nov 20, 2023

View reviewed changes

content/oci/oci.go Outdated Show resolved Hide resolved

content/oci/oci.go Outdated Show resolved Hide resolved

eiffel-fl force-pushed the francis/delete-tree branch 2 times, most recently from 1b137cc to f77d4b2 Compare November 21, 2023 10:39

eiffel-fl mentioned this pull request Nov 21, 2023

Add ig image remove command inspektor-gadget/inspektor-gadget#2162

Merged

eiffel-fl force-pushed the francis/delete-tree branch from f77d4b2 to 7d8a764 Compare November 21, 2023 11:00

feat: extend oci.Store with DeleteTree().

d57c2d1

Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>

eiffel-fl force-pushed the francis/delete-tree branch from 7d8a764 to d57c2d1 Compare November 21, 2023 17:05

eiffel-fl mentioned this pull request Jan 2, 2024

Manifest should not be added to index.json #664

Closed

eiffel-fl closed this Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extend oci.Store with DeleteTree(). #645

feat: extend oci.Store with DeleteTree(). #645

eiffel-fl commented Nov 20, 2023 •

edited

Loading

shizhMSFT commented Nov 21, 2023

eiffel-fl commented Nov 21, 2023

codecov-commenter commented Nov 22, 2023

wangxiaoxuan273 commented Nov 23, 2023

Wwwsylvia commented Dec 29, 2023

eiffel-fl commented Dec 29, 2023

Wwwsylvia commented Dec 29, 2023 •

edited

Loading

eiffel-fl commented Dec 29, 2023

Wwwsylvia commented Dec 29, 2023 •

edited

Loading

eiffel-fl commented Dec 29, 2023

eiffel-fl commented Dec 29, 2023

Wwwsylvia commented Jan 2, 2024 •

edited

Loading

Wwwsylvia commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

feat: extend oci.Store with DeleteTree(). #645

feat: extend oci.Store with DeleteTree(). #645

Conversation

eiffel-fl commented Nov 20, 2023 • edited Loading

shizhMSFT commented Nov 21, 2023

eiffel-fl commented Nov 21, 2023

codecov-commenter commented Nov 22, 2023

Codecov Report

wangxiaoxuan273 commented Nov 23, 2023

Wwwsylvia commented Dec 29, 2023

eiffel-fl commented Dec 29, 2023

Wwwsylvia commented Dec 29, 2023 • edited Loading

eiffel-fl commented Dec 29, 2023

Wwwsylvia commented Dec 29, 2023 • edited Loading

eiffel-fl commented Dec 29, 2023

eiffel-fl commented Dec 29, 2023

Wwwsylvia commented Jan 2, 2024 • edited Loading

Wwwsylvia commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

eiffel-fl commented Jan 2, 2024

Wwwsylvia commented Jan 2, 2024

eiffel-fl commented Nov 20, 2023 •

edited

Loading

Wwwsylvia commented Dec 29, 2023 •

edited

Loading

Wwwsylvia commented Dec 29, 2023 •

edited

Loading

Wwwsylvia commented Jan 2, 2024 •

edited

Loading