Added support for removing images/tags #854
Conversation
|
Some screenshots: Removing an image with tags:
After the previous screenshot, we push the same image back:
The UI for removing images and tags:
|
Soft delete support has been supported in Docker Distribution since at least 2.1. This was not enough to implement the removal of images/tags in Portus because there was no support to GC these soft deleted blobs. Since 2.4, it's possible to not just delete the manifest, but also to GC blobs no longer referenced by any image manifest. This means that after being able to track digests, we can now safely provide image/tag removal support. For safety concerns, tags with an empty digest will not be allowed to be removed (this is more likely to be the case of Portus versions that have been running for some time). In previous PRs this has already been addressed, so admins can update their DB filling in the empty gaps (e.g. see PRs SUSE#825 or SUSE#830). The main downside of this change is that there is no way for a client to detect whether a remote registry supports GC. Because of this, a configuration option has been provided, which is disabled by default. The rationale is that administrators that are really sure about the availability of GC on their private registry will have to enable it explicitly. Fixes SUSE#197 Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
| @@ -0,0 +1,14 @@ | |||
| # DeleteEnabeld redirects the user back if delete support is not enabled. A | |||
There was a problem hiding this comment.
Typo, should read DeleteEnabled.
|
LGTM |
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
|
Fixed typos pointed out by @monstermunchkin. I'll squash my commits once reviewing is over 😉 |
| @@ -0,0 +1,14 @@ | |||
| # DeleteEnabled redirects the user back if delete support is not enabled. A | |||
| # `before_action` will be created for the :destroy method. | |||
| module DeleteEnabled | |||
There was a problem hiding this comment.
There is some convention to name concerns like polymorph activerecord relations ending with able, e.g. Deletable
|
Beside my NITPICK LGTM |
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
|
|
||
| # Delete this repository if all tags were successfully deleted. | ||
| if @repository.reload.tags.any? | ||
| redirect_to repository_path(@repository), alert: "Could not remove all the tags" |
There was a problem hiding this comment.
I think it would be useful to dump more information into Rails' log (like which tags could not have been deleted and eventually a reason). Otherwise an admin will have a hard time figuring this out on his own.
There was a problem hiding this comment.
It is logged, because it ends up calling Tag#delete_by_digest!, which logs any errors. Maybe I can provide an extra line of: "Error: tags [...] could not be removed" (the reason on why they have been removed is logged by Tag#delete_by_digest!)
|
It looks good to me. A couple of considerations:
|
Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>
|
@flavio pushed a commit fixing your comments. I'll squash the commits once the review is over.
Yes. My plan is to do the same as I'm doing for removed images/tags: update the recipient and add stuff into the
Yes and no 😄. Yes, administrators are still required to call GC (this will be documented, and there's already a big fat explanation on the config. option about that). About maintenance mode... well, it's recommended but not enforced 😅. That is, you can run a GC operation without being in maintenance mode and the registry won't complain. However, if you do that, then there are race conditions. So, from the point of view of the registry, it's recommended but not enforced. This should also be in the documentation 😉 |
LGTM
Sounds good to me.
I'm fine with that as we discussed the ongoing plan about "remove all the things!" |
Soft delete support has been supported in Docker Distribution since at least
2.1. This was not enough to implement the removal of images/tags in Portus
because there was no support to GC these soft deleted blobs. Since 2.4, it's
possible to not just delete the manifest, but also to GC blobs no longer
referenced by any image manifest. This means that after being able to track
digests, we can now safely provide image/tag removal support. For safety
concerns, tags with an empty digest will not be allowed to be removed (this
is more likely to be the case of Portus versions that have been running for
some time). In previous PRs this has already been addressed, so admins can
update their DB filling in the empty gaps (e.g. see PRs #825 or #830).
The main downside of this change is that there is no way for a client to detect
whether a remote registry supports GC. Because of this, a configuration option
has been provided, which is disabled by default. The rationale is that
administrators that are really sure about the availability of GC on their
private registry will have to enable it explicitly.
Fixes #197
Signed-off-by: Miquel Sabaté Solà msabate@suse.com