Add ig image remove command

[eiffel-fl]

Hi.

This PR adds ig image remove command.
It depends on work in progress in oras-project/oras-go#614.

Best regards.

[mqasimsarfraz]

Thanks for looking into this, I have been doing rm -rf /var/lib/ig/oci-store ( 🙈 ) to clean up all images so this would be much better. I am wondering if it should be image delete vs image rm ?

[eiffel-fl]

Thanks for looking into this, I have been doing rm -rf /var/lib/ig/oci-store ( 🙈 ) to clean up all images so this would be much better. I am wondering if it should be image delete vs image rm ?

It is named remove, I wrote everything too quickly as for now it is on hold.
I nonetheless still have questions as I still have some empty directories inside /var/lib/ig/oci-store despite having removed every images.
Everything is detailled upstream.

[eiffel-fl]

I was able to come up with some code to remove the whole graph based on upstream work:

$ sudo ls -1 /var/lib/ig/oci-store/blobs/sha256 | wc -l            francis/rmi % u+2-2
12
$ sudo -E ./ig image list                                          francis/rmi % u+2-2
INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST      
ghcr.io/inspektor-gadget/gadget/trace_exec                     latest                                                        30d2db19d0af
ghcr.io/inspektor-gadget/gadget/trace_open                     latest                                                        842e69c79177
$ sudo -E ./ig image remove ghcr.io/inspektor-gadget/gadget/trace_open     
INFO[0000] Experimental features enabled                
$ sudo -E ./ig image remove ghcr.io/inspektor-gadget/gadget/trace_exec
INFO[0000] Experimental features enabled                
$ sudo -E ./ig image list                                          francis/rmi % u+2-2
INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST      
$ sudo ls -1 /var/lib/ig/oci-store/blobs/sha256 | wc -l            francis/rmi % u+2-2
0

I will remove the hold tag.

[mqasimsarfraz]

This is helpful for #2171 as well. Let me test it!

I will remove the hold tag.

Does that mean we can merge it or we still have to wait till upstream PRs are merged?

[eiffel-fl]

I will remove the hold tag.

Does that mean we can merge it or we still have to wait till upstream PRs are merged?

It depends.
If we want to have the remove command in the next release, then better to merge it now.
Otherwise, we can wait.
By the way the upstream PR was rebased and should be merged soon.
On another hand, merging it soon here we can still modify the go.* to use upstream rather than fork once it is merged.

[burak-ok]

I will remove the hold tag.

Does that mean we can merge it or we still have to wait till upstream PRs are merged?

It depends. If we want to have the remove command in the next release, then better to merge it now. Otherwise, we can wait. By the way the upstream PR was rebased and should be merged soon. On another hand, merging it soon here we can still modify the go.* to use upstream rather than fork once it is merged.

There was no activity since 2 weeks on the upstream PR. Should we merge this now?

[eiffel-fl]

I will remove the hold tag.

Does that mean we can merge it or we still have to wait till upstream PRs are merged?

It depends. If we want to have the remove command in the next release, then better to merge it now. Otherwise, we can wait. By the way the upstream PR was rebased and should be merged soon. On another hand, merging it soon here we can still modify the go.* to use upstream rather than fork once it is merged.

There was no activity since 2 weeks on the upstream PR. Should we merge this now?

I updated the commit to use latest modifications brought to this PR.
I am OK with merging it, but as there are no emergency there I would rather wait a bit more, particularly because there is one good point still opened upstream.
Let's see if there is some movement upstream and we can still merge it for this release even if this is not the case.

[mauriciovasquezbernal]

I think there is something off, if there are two tags pointing to the same digest, deleting one of them causes the other one to be removed:

# there are not images at the beginning 
$ sudo -E ig image list
INFO[0000] Experimental features enabled                
REPOSITORY                                                               TAG                                                                      DIGEST      

# build an image
$ sudo -E ig image build -t mygadget .  --local
INFO[0000] Experimental features enabled                
Successfully built docker.io/library/mygadget:latest@sha256:0b34e37d953f64b800ccbb547481e47f5f517cf9bf0aa290ab4f4760c9bb92fa

$ sudo -E ig image list
INFO[0000] Experimental features enabled                
REPOSITORY                                                               TAG                                                                      DIGEST      
docker.io/library/mygadget                                               latest                                                                   0b34e37d953f

# tag an image with the above 
$ sudo -E ig image tag mygadget foo
INFO[0000] Experimental features enabled                
Successfully tagged with docker.io/library/foo:latest@sha256:0b34e37d953f64b800ccbb547481e47f5f517cf9bf0aa290ab4f4760c9bb92fa

$ sudo -E ig image list
INFO[0000] Experimental features enabled                
REPOSITORY                                                               TAG                                                                      DIGEST      
docker.io/library/foo                                                    latest                                                                   0b34e37d953f
docker.io/library/mygadget                                               latest                                                                   0b34e37d953f

# remove the first image 
$ sudo -E ig image remove mygadget:latest
INFO[0000] Experimental features enabled                

# second one is gone as well
$ sudo -E ig image list
INFO[0000] Experimental features enabled                
REPOSITORY                                                               TAG                                                                      DIGEST    

[eiffel-fl]

I am marking this PR as hold, as I would like to get the upstream one merged, feel free to review it:
oras-project/oras-go#645

[mauriciovasquezbernal]

@eiffel-fl have you seem my comment in #2162 (review)? I'm getting the same behavior with oras-project/oras-go#645.

[eiffel-fl]

@eiffel-fl have you seem my comment in #2162 (review)? I'm getting the same behavior with oras-project/oras-go#645.

I am not Lucky Luke, so I do not code faster than my shadow.
I need to think more about the above problem, as the whole thing is above getting the references in reverse order which, up to my knowledge of oras-land related packages, is not simply doable.

[eiffel-fl]

oras-project/oras-go#647 solves the above problem.

[mauriciovasquezbernal]

I tested and it works fine now.

I'm wondering if we should follow an garbage-collection approach for this too as suggested in oras-project/oras-go#645 (comment)?, deleting an image could be something like

untag()
collectGarbage()

Would you mind adding an entry to https://github.com/inspektor-gadget/inspektor-gadget/blob/main/docs/images.md?

I'm approving it now as does its job. It's up to you to merge or wait for the upstream PRs to be ready.

We're missing the tests for this (and other commands), we can do them in #2171.

[eiffel-fl]

I tested and it works fine now.

I'm wondering if we should follow an garbage-collection approach for this too as suggested in oras-project/oras-go#645 (comment)?, deleting an image could be something like

While the comment from oras-go maintainer totally makes sens, this applies less in our case.
Indeed, oras-go is a generic library to deal with OCI related components, so having GC would be perfect for registries.
In our case, the same tool acts as a registry and a CLI, so having GC makes less sense.
If one day we totally decouple the registry part from the CLI, then it would definitely be welcomed.
For now, I will stick with that we have as it does the trick. But I will take a look on upstream development.

untag()
collectGarbage()

Would you mind adding an entry to https://github.com/inspektor-gadget/inspektor-gadget/blob/main/docs/images.md?

I'm approving it now as does its job. It's up to you to merge or wait for the upstream PRs to be ready.

I will first wait to get some feedback upstream before merging this one.

We're missing the tests for this (and other commands), we can do them in #2171.

[eiffel-fl]

As oras-project/oras-go#647 was merged, we can now use upstream at a specific commit.
So, let's merge it as we have everything needed here to remove image.
Everything should be ready to merge, but I would like to have at least one last review before clicking the merge button.

We will remove the deleteTree() later once oras-project/oras-go#656 and oras-project/oras-go#653 would be merged but no strong dependencies on it.

[mauriciovasquezbernal]

Besides two minor comments, LGTM!

[eiffel-fl]

Thank you for the reviews!