feat: implement deleter #470

carabasdaniel · 2023-03-23T09:41:01Z

This should close: #454

codecov-commenter · 2023-03-27T02:21:26Z

Codecov Report

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

Comparison is base (7dd0378) 73.22% compared to head (69a1851) 73.09%.
Report is 47 commits behind head on main.

Files	Patch %	Lines
content/memory/memory.go	33.33%	4 Missing and 2 partials ⚠️
content/oci/oci.go	60.00%	4 Missing and 2 partials ⚠️
content/oci/storage.go	57.14%	2 Missing and 1 partial ⚠️
internal/resolver/memory.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #470      +/-   ##
==========================================
- Coverage   73.22%   73.09%   -0.14%     
==========================================
  Files          49       49              
  Lines        4602     4642      +40     
==========================================
+ Hits         3370     3393      +23     
- Misses        923      935      +12     
- Partials      309      314       +5

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

carabasdaniel · 2023-03-28T15:34:15Z

Hello @shizhMSFT @Wwwsylvia,

This is the start for the delete implementation, I would definitely appreciate a bit of feedback on this PR.

shizhMSFT · 2023-03-28T15:42:26Z

@carabasdaniel Thanks for your contribution. We recognize and are reviewing this PR. Since it is related to design, it takes more time to be reviewed but should finish in 1 day or two.

content/storage.go

content/oci/storage.go

Wwwsylvia · 2023-03-29T10:12:29Z

content/oci/oci.go

@@ -192,6 +192,22 @@ func (s *Store) Resolve(ctx context.Context, reference string) (ocispec.Descript
 	return desc, nil
 }

+func (s *Store) Delete(ctx context.Context, target ocispec.Descriptor) error {


One tricky thing about deleting is that we need to maintain the predecessors relationship as well. See Store.Push and Store.Predecessors.
This also applies to other GraphStorage that implement PredecessorsFinder.

content/file/file.go

content/storage.go

internal/cas/proxy.go

internal/resolver/memory.go

internal/cas/memory.go

content/file/file.go

content/oci/storage.go

shizhMSFT · 2023-03-29T10:25:06Z

content/oci/oci.go

+	resolvers := s.tagResolver.Map()
+	for reference, desc := range resolvers {
+		if content.Equal(desc, target) {
+			s.tagResolver.Remove(reference)
+		}
+	}


What if some is pushing and deleting at the same time? There is a race condition.

The targetResolver.Map() represent an inconsistent snapshot of the storage content as far as I understand, therefore I'm not sure if a delete of a pushing content would be possible as the tagResolver is actually a syncMap.

@carabasdaniel Since it is possible that someone is trying to push and delete at the same time, we have to trade-off some performance for the deletion.

Similar to what I have mentioned in other comments, the current oci.Store is append-only and thus it has the benefits that multiple push and fetch can happen at the same time without race conditions.

However, adding the functionality of Delete() makes the store mutable, and we have to change the underlying implementation from sync maps to a RWMutex to ensure consistency. The drawback is that we no longer can push and delete simultaneously.

Overall, I suggest

Drop Delete() for oci.Store.

Develop oci.MutableStore with Delete() support.

/cc @Wwwsylvia

Overall, I suggest
Drop Delete() for oci.Store.
Develop oci.MutableStore with Delete() support.

It makes sense.

@carabasdaniel I would also like to learn your use cases of deleting content in OCI layout. Is it for GC purpose? Could you share a little bit?

Hi @Wwwsylvia, @shizhMSFT,

We have a tool used for building OCI images from rego policies: policy CLI.

We have been using the previous major version of oras-go to build our containers and interact with upstream OCI compliant registries. In our case we want the user to be able to build and easily remove containers from the local OCI storage, thus the need to have a delete mechanism in place, keeping the same workflow feel as the docker CLI.

At the moment we use a fork that includes these changes in our v0.2.0 release candidate versions of the policy CLI and it seems to work with our current needs.

I think having a different store might make sense.

Got it. Thank you for sharing this!

@Wwwsylvia for additional use case info... I am working on https://github.com/awslabs/soci-snapshotter which uses https://pkg.go.dev/oras.land/oras-go/content to keep content in the local filesystem. Our artifacts are conceptually similar to image and layer metadata; we store one artifact per image we handle, and one artifact per layer in those images, with one-way references (via a list of digests equivalent to image layers) from artifacts of the first type to artifacts of the second type.

carabasdaniel · 2023-03-29T10:48:04Z

Hi @shizhMSFT @Wwwsylvia,

Thanks for the feedback, I'll start working on the required changes.

internal/cas/memory.go

shizhMSFT · 2023-03-30T15:26:56Z

content/storage.go

+// DeleteStorage represents an extension of the Storage interface that includes the Deleter.
+type DeleteStorage interface {


The Storage interface abstracts append-only storages. Adding Deleter makes the storage a MutableStorage.

@Wwwsylvia Do you have a better name suggestion on DeleteStorage?

MutableStorage sounds like that the existing data can be modified or deleted. But in our case, the data can be deleted but cannot be modified.
How about StorageDeleter or DeletableStorage?
BTW, do we really need to define this interface?

type StorageWithDelete interface { Storage Deleter }

This is from my current efforts in the same direction as this PR. If you don't define this interface, folks like me will just have to do it themselves, and our naming divergence will make it harder to find usage examples in the wild.

content/oci/storage.go

content/oci/oci.go

internal/cas/memory_test.go

Wwwsylvia · 2023-03-31T07:22:21Z

content/storage.go

+// DeleteStorage represents an extension of the Storage interface that includes the Deleter.
+type DeleteStorage interface {


MutableStorage sounds like that the existing data can be modified or deleted. But in our case, the data can be deleted but cannot be modified.
How about StorageDeleter or DeletableStorage?
BTW, do we really need to define this interface?

content/oci/storage_test.go

sparr · 2023-05-26T17:58:07Z

I find myself preparing to implement similar functionality as part of https://github.com/awslabs/soci-snapshotter and want to check in on this PR. I would much prefer to contribute upstream and then consume that. Is this still under development? Would additional effort be welcome?

sparr · 2023-05-26T18:13:06Z

content/oci/oci_test.go

@@ -1993,3 +2042,114 @@ func equalDescriptorSet(actual []ocispec.Descriptor, expected []ocispec.Descript
 	}
 	return true
 }
+
+func TestStore_Delete(t *testing.T) {


Can some/most of this be deduplicated with the memory_test.go TestStoreDelete with similar contents? Ditto in storage_test.go

content/oci/storage.go

TerryHowe

Sounds like maybe two other changes needed here as well as the deletes

content/oci/storage.go

content/oci/oci_test.go

content/oci/oci.go

Signed-off-by: carabasdaniel <dani@aserto.com>

Co-authored-by: Terry Howe <terrylhowe@gmail.com> Signed-off-by: carabasdaniel <dani@aserto.com>

carabasdaniel · 2023-06-02T13:26:45Z

Merged suggested changes and rebased on top of latest upstream.
If we merge this I can open another PR that adds the untag feature.

Thanks for the help @TerryHowe.

sparr · 2023-06-02T14:21:51Z

@carabasdaniel There appear to be two outstanding comments referring to needed functionality for updating graph and resolver when content is deleted.

My limited understanding is that graph is used for predecessor/successor (aka parent/child) lookups and traversal of content and is populated in Store.Push where it calls graph.Index and during loadIndex when it loads index.json and then calls graph.IndexAll, and resolver is used for looking up content by tags and is populated by Tag (which may require something like your Untag to undo although I haven't looked far enough to determine if it would meet that need)

Wwwsylvia · 2023-06-05T11:46:23Z

My limited understanding is that graph is used for predecessor/successor (aka parent/child) lookups and traversal of content and is populated in Store.Push where it calls graph.Index and during loadIndex when it loads index.json and then calls graph.IndexAll, and resolver is used for looking up content by tags and is populated by Tag (which may require something like your Untag to undo although I haven't looked far enough to determine if it would meet that need)

Yes it's right.

carabasdaniel · 2023-06-08T07:07:20Z

Hi @sparr @Wwwsylvia,

Currently my bandwidth is very limited so I'm not sure when I'll be able to come back to this. For our current needs we are using a fork that has this change and the UnTag change merged.

In addition to the recent changes I made here I also prepared an untag branch that would build on top of this PR for the resolver.

I would appreciate any help to get this set implemented/merged.

shizhMSFT

As it is technically challenging to add Delete() to the existing *oci.Store, as mentioned in #470 (comment), I would suggest developing a new store named *oci.DeletableStore where the functionality of Predecessors() can be dropped so that the difficulty of the development should be dramatically reduced.

It should also meet the requirements of https://github.com/opcr-io/policy as it does not leverage the functionality of Predecessors().

shizhMSFT · 2023-06-09T07:43:53Z

content/memory/memory.go

@@ -72,6 +72,18 @@ func (s *Store) Resolve(ctx context.Context, reference string) (ocispec.Descript
 	return s.resolver.Resolve(ctx, reference)
 }

+// Delete a target descriptor for storage.
+func (s *Store) Delete(ctx context.Context, target ocispec.Descriptor) error {


s.resolver is for the tagging service, i.e. for Resolve(), as Resolve() should not resolve a reference to a deleted descriptor. Similarly, s.graph is for Predecessors().

Since this PR mainly focuses on OCI store, we can keep the changes to the Memory store in another PR.

shizhMSFT · 2023-06-09T07:51:17Z

internal/graph/memory.go

@@ -116,6 +116,25 @@ func (m *Memory) Predecessors(_ context.Context, node ocispec.Descriptor) ([]oci
 	return res, nil
 }

+// RemoveFromIndex removes the node and all its predecessors from the index and predecessor maps.
+func (m *Memory) RemoveFromIndex(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error {


It seems fetcher is not used anywhere.

shizhMSFT · 2023-06-09T07:54:00Z

content/oci/oci.go

+// Delete removed a target descriptor from index and storage.
+func (s *Store) Delete(ctx context.Context, target ocispec.Descriptor) error {


As mentioned in #470 (comment), the Delete() method might be called concurrently with other methods like Push() and Resolve(). Race condition is a concern here.

sparr · 2023-06-09T13:20:29Z

Speaking only for myself, an implementation of a new Store type with Delete functionality but without Predecessors would not fit my current use case. My current solution is to delete files then recreate index.json then recreate the content store so that the graph will be reindexed from scratch.

shizhMSFT · 2023-06-25T03:16:43Z

The main technical challenge here is that the current *oci.Store implementation is optimized for the concurrent use of append-only store in terms of performance. It is non-trivial to modify it to have the delete functionality.

To unblock this PR attempt, @wangxiaoxuan273 could you prototype a *oci.DeletableStore with both the functionality of Delete() and Predecessors()? We can have performance penalty, but we have to make sure that all public methods of *oci.DeletableStore MUST be go-routine safe since it is by design for all Targets.

done

TerryHowe · 2023-07-07T12:30:14Z

Where is this going. Any progress on the oci.DeletableStore ?

wangxiaoxuan273 · 2023-07-08T07:07:54Z

Where is this going. Any progress on the oci.DeletableStore ?

No progress so far. I'm still in the stage of investigating. But it's likely that this PR will not be continued, and we will open a new one to fix this issue. @TerryHowe

Wwwsylvia · 2023-11-17T12:15:51Z

Where is this going. Any progress on the oci.DeletableStore ?

FYI, here is the PR authored by @wangxiaoxuan273: #614

carabasdaniel force-pushed the delete branch 4 times, most recently from dac5f62 to 008a185 Compare March 23, 2023 14:35

carabasdaniel marked this pull request as ready for review March 24, 2023 13:26

carabasdaniel requested review from sajayantony, shizhMSFT, SteveLasker and Wwwsylvia as code owners March 24, 2023 13:26

shizhMSFT changed the title ~~feature: Implement deleter~~ feat: implement deleter Mar 27, 2023

Wwwsylvia reviewed Mar 29, 2023

View reviewed changes

shizhMSFT requested changes Mar 29, 2023

View reviewed changes

carabasdaniel force-pushed the delete branch 2 times, most recently from 8812296 to 8c25175 Compare March 29, 2023 13:57

shizhMSFT reviewed Mar 30, 2023

View reviewed changes

Wwwsylvia reviewed Mar 31, 2023

View reviewed changes

carabasdaniel force-pushed the delete branch from 8c25175 to d5382ea Compare March 31, 2023 14:03

sparr reviewed May 26, 2023

View reviewed changes

rogpeppe reviewed May 27, 2023

View reviewed changes

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

TerryHowe previously requested changes Jun 1, 2023

View reviewed changes

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

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

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

carabasdaniel and others added 4 commits June 2, 2023 10:26

Add Delete to storages

5ade634

Signed-off-by: carabasdaniel <dani@aserto.com>

Update content/oci/storage.go

1392624

Co-authored-by: Terry Howe <terrylhowe@gmail.com> Signed-off-by: carabasdaniel <dani@aserto.com>

Update content/oci/oci_test.go

8f342d1

Co-authored-by: Terry Howe <terrylhowe@gmail.com> Signed-off-by: carabasdaniel <dani@aserto.com>

Update content/oci/oci.go

69a1851

Co-authored-by: Terry Howe <terrylhowe@gmail.com> Signed-off-by: carabasdaniel <dani@aserto.com>

carabasdaniel force-pushed the delete branch from 0713b75 to 69a1851 Compare June 2, 2023 07:30

qweeah mentioned this pull request Jun 5, 2023

support OCI image layout target in oras manifest delete oras-project/oras#762

Closed

shizhMSFT reviewed Jun 9, 2023

View reviewed changes

sparr mentioned this pull request Jun 20, 2023

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

Open

wangxiaoxuan273 mentioned this pull request Sep 1, 2023

[WIP] Implement Delete for OCI Layout #582

Closed

eiffel-fl mentioned this pull request Oct 18, 2023

[RFC] Add Delete() to oci.Store. #620

Closed

shizhMSFT added the stale Inactive issues or pull requests label Dec 29, 2023

carabasdaniel closed this May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement deleter #470

feat: implement deleter #470

carabasdaniel commented Mar 23, 2023

codecov-commenter commented Mar 27, 2023 •

edited by codecov bot

Loading

carabasdaniel commented Mar 28, 2023

shizhMSFT commented Mar 28, 2023

Wwwsylvia Mar 29, 2023 •

edited

Loading

shizhMSFT Mar 29, 2023

carabasdaniel Mar 29, 2023

shizhMSFT Mar 30, 2023

Wwwsylvia Mar 31, 2023

carabasdaniel Mar 31, 2023

Wwwsylvia Apr 4, 2023

sparr May 30, 2023

carabasdaniel commented Mar 29, 2023

shizhMSFT Mar 30, 2023

Wwwsylvia Mar 31, 2023

sparr May 30, 2023

Wwwsylvia Mar 31, 2023

sparr commented May 26, 2023

sparr May 26, 2023 •

edited

Loading

TerryHowe left a comment

carabasdaniel commented Jun 2, 2023

sparr commented Jun 2, 2023 •

edited

Loading

Wwwsylvia commented Jun 5, 2023

carabasdaniel commented Jun 8, 2023

shizhMSFT left a comment •

edited

Loading

shizhMSFT Jun 9, 2023

shizhMSFT Jun 9, 2023

shizhMSFT Jun 9, 2023

sparr commented Jun 9, 2023

shizhMSFT commented Jun 25, 2023

TerryHowe commented Jul 7, 2023

wangxiaoxuan273 commented Jul 8, 2023 •

edited

Loading

Wwwsylvia commented Nov 17, 2023

		// DeleteStorage represents an extension of the Storage interface that includes the Deleter.
		type DeleteStorage interface {

		// Delete removed a target descriptor from index and storage.
		func (s *Store) Delete(ctx context.Context, target ocispec.Descriptor) error {

feat: implement deleter #470

feat: implement deleter #470

Conversation

carabasdaniel commented Mar 23, 2023

codecov-commenter commented Mar 27, 2023 • edited by codecov bot Loading

Codecov Report

carabasdaniel commented Mar 28, 2023

shizhMSFT commented Mar 28, 2023

Wwwsylvia Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carabasdaniel commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparr commented May 26, 2023

sparr May 26, 2023 • edited Loading

Choose a reason for hiding this comment

TerryHowe left a comment

Choose a reason for hiding this comment

carabasdaniel commented Jun 2, 2023

sparr commented Jun 2, 2023 • edited Loading

Wwwsylvia commented Jun 5, 2023

carabasdaniel commented Jun 8, 2023

shizhMSFT left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparr commented Jun 9, 2023

shizhMSFT commented Jun 25, 2023

TerryHowe commented Jul 7, 2023

wangxiaoxuan273 commented Jul 8, 2023 • edited Loading

Wwwsylvia commented Nov 17, 2023

codecov-commenter commented Mar 27, 2023 •

edited by codecov bot

Loading

Wwwsylvia Mar 29, 2023 •

edited

Loading

sparr May 26, 2023 •

edited

Loading

sparr commented Jun 2, 2023 •

edited

Loading

shizhMSFT left a comment •

edited

Loading

wangxiaoxuan273 commented Jul 8, 2023 •

edited

Loading