Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement deleter #470

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion content/memory/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

// Store represents a memory based store, which implements `oras.Target`.
type Store struct {
storage content.Storage
storage content.DeletableStorage
resolver content.TagResolver
graph *graph.Memory
}
Expand Down Expand Up @@ -72,6 +72,18 @@
return s.resolver.Resolve(ctx, reference)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s.resolver and s.graph are also required to be updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct in saying that s.resolver needs to be updated (possibly by Deleteing the relevant key in the Store) for all use cases, but s.graph being updated is only necessary for use cases where Successor/Predecessor functionality will be used?
Of course the final implementation here needs to do both, I'm just trying to better understand the behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

exists, err := s.storage.Exists(ctx, target)
if err != nil {
return err
}

Check warning on line 80 in content/memory/memory.go

View check run for this annotation

Codecov / codecov/patch

content/memory/memory.go#L79-L80

Added lines #L79 - L80 were not covered by tests
if !exists {
return errdef.ErrNotFound
}

Check warning on line 83 in content/memory/memory.go

View check run for this annotation

Codecov / codecov/patch

content/memory/memory.go#L82-L83

Added lines #L82 - L83 were not covered by tests
return s.storage.Delete(ctx, target)
}

// Tag tags a descriptor with a reference string.
// Returns ErrNotFound if the tagged content does not exist.
func (s *Store) Tag(ctx context.Context, desc ocispec.Descriptor, reference string) error {
Expand Down
45 changes: 45 additions & 0 deletions content/memory/memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,51 @@ func TestStorePredecessors(t *testing.T) {
}
}

func TestStoreDelete(t *testing.T) {
content := []byte("hello world")
desc := ocispec.Descriptor{
MediaType: "test",
Digest: digest.FromBytes(content),
Size: int64(len(content)),
}

s := New()
ctx := context.Background()

err := s.Push(ctx, desc, bytes.NewReader(content))
if err != nil {
t.Fatal("Store.Push() error =", err)
}
ref := "foobar"
err = s.Tag(ctx, desc, ref)
if err != nil {
t.Fatal("Store.Tag() error =", err)
}

internalResolver := s.resolver.(*resolver.Memory)
if got := len(internalResolver.Map()); got != 1 {
t.Errorf("resolver.Map() = %v, want %v", got, 1)
}

exists, err := s.Exists(ctx, desc)
if err != nil {
t.Fatal("Store.Exists() error =", err)
}
if !exists {
t.Errorf("Store.Exists() = %v, want %v", exists, true)
}

err = s.Delete(ctx, desc)
if err != nil {
t.Fatal("Store.Delete() error =", err)
}

internalStorage := s.storage.(*cas.Memory)
if got := len(internalStorage.Map()); got != 0 {
t.Errorf("storage.Map() = %v, want %v", got, 0)
}
}

func equalDescriptorSet(actual []ocispec.Descriptor, expected []ocispec.Descriptor) bool {
if len(actual) != len(expected) {
return false
Expand Down
24 changes: 23 additions & 1 deletion content/oci/oci.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
index *ocispec.Index
indexLock sync.Mutex

storage content.Storage
storage content.DeletableStorage
tagResolver *resolver.Memory
graph *graph.Memory
}
Expand All @@ -76,6 +76,7 @@
if err != nil {
return nil, fmt.Errorf("failed to resolve absolute path for %s: %w", root, err)
}

storage, err := NewStorage(rootAbs)
if err != nil {
return nil, fmt.Errorf("failed to create storage: %w", err)
Expand Down Expand Up @@ -192,6 +193,27 @@
return desc, nil
}


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

@Wwwsylvia Wwwsylvia Mar 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

carabasdaniel marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +197 to +198
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resolvers := s.tagResolver.Map()
for reference, desc := range resolvers {
if content.Equal(desc, target) {
s.tagResolver.Delete(reference)
}
}
if err := s.graph.RemoveFromIndex(ctx, s.storage, target); err != nil {
return err
}

Check warning on line 207 in content/oci/oci.go

View check run for this annotation

Codecov / codecov/patch

content/oci/oci.go#L206-L207

Added lines #L206 - L207 were not covered by tests
if s.AutoSaveIndex {
err := s.SaveIndex()
if err != nil {
return err
}

Check warning on line 212 in content/oci/oci.go

View check run for this annotation

Codecov / codecov/patch

content/oci/oci.go#L211-L212

Added lines #L211 - L212 were not covered by tests
}
return s.storage.Delete(ctx, target)
}

// Predecessors returns the nodes directly pointing to the current node.
// Predecessors returns nil without error if the node does not exists in the
// store.
Expand Down
111 changes: 111 additions & 0 deletions content/oci/oci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1994,3 +1994,114 @@ func equalDescriptorSet(actual []ocispec.Descriptor, expected []ocispec.Descript
}
return true
}

func TestStore_Delete(t *testing.T) {
Copy link
Contributor

@sparr sparr May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

content := []byte("hello world")
ref := "hello-world:0.0.1"
desc := ocispec.Descriptor{
MediaType: "test",
Digest: digest.FromBytes(content),
Size: int64(len(content)),
}

tempDir := t.TempDir()
s, err := New(tempDir)
if err != nil {
t.Fatal("New() error =", err)
}
ctx := context.Background()

err = s.Push(ctx, desc, bytes.NewReader(content))
if err != nil {
t.Errorf("Store.Push() error = %v, wantErr %v", err, false)
}

err = s.Tag(ctx, desc, ref)
if err != nil {
t.Errorf("error tagging descriptor error = %v, wantErr %v", err, false)
}

resolvedDescr, err := s.Resolve(ctx, ref)
if err != nil {
t.Errorf("error resolving descriptor error = %v, wantErr %v", err, false)
}

if !reflect.DeepEqual(resolvedDescr, desc) {
t.Errorf("Store.Resolve() = %v, want %v", resolvedDescr, desc)
}

err = s.Delete(ctx, resolvedDescr)
if err != nil {
t.Errorf("Store.Delete() = %v, wantErr %v", err, true)
}

_, err = s.Resolve(ctx, ref)
if !errors.Is(err, errdef.ErrNotFound) {
t.Errorf("descriptor should no longer exist in store = %v, wantErr %v", err, errdef.ErrNotFound)
}
}

func TestStore_DeleteDescriptoMultipleRefs(t *testing.T) {
content := []byte("hello world")
ref1 := "hello-world:0.0.1"
ref2 := "hello-world:0.0.2"
desc := ocispec.Descriptor{
MediaType: "test",
Digest: digest.FromBytes(content),
Size: int64(len(content)),
}

tempDir := t.TempDir()
s, err := New(tempDir)
s.AutoSaveIndex = true
if err != nil {
t.Fatal("New() error =", err)
}
ctx := context.Background()

err = s.Push(ctx, desc, bytes.NewReader(content))
if err != nil {
t.Errorf("Store.Push() error = %v, wantErr %v", err, false)
}

if len(s.index.Manifests) != 0 {
t.Errorf("manifest should be empty but has %d elements", len(s.index.Manifests))
}

err = s.Tag(ctx, desc, ref1)
if err != nil {
t.Errorf("error tagging descriptor error = %v, wantErr %v", err, false)
}

err = s.Tag(ctx, desc, ref2)
if err != nil {
t.Errorf("error tagging descriptor error = %v, wantErr %v", err, false)
}

if len(s.index.Manifests) != 2 {
t.Errorf("manifest should have %d, but has %d", len(s.index.Manifests), 0)
}

resolvedDescr, err := s.Resolve(ctx, ref1)
if err != nil {
t.Errorf("error resolving descriptor error = %v, wantErr %v", err, false)
}

if !reflect.DeepEqual(resolvedDescr, desc) {
t.Errorf("Store.Resolve() = %v, want %v", resolvedDescr, desc)
}

err = s.Delete(ctx, resolvedDescr)
if err != nil {
t.Errorf("Store.Delete() = %v, wantErr %v", err, true)
}

if len(s.index.Manifests) != 0 {
t.Errorf("manifest should be empty after delete but has %d", len(s.index.Manifests))
}

_, err = s.Resolve(ctx, ref2)
if !errors.Is(err, errdef.ErrNotFound) {
t.Errorf("descriptor should no longer exist in store = %v, wantErr %v", err, errdef.ErrNotFound)
}
}
10 changes: 10 additions & 0 deletions content/oci/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,16 @@
return nil
}

// Delete removes the target blob.
func (s *Storage) Delete(ctx context.Context, target ocispec.Descriptor) error {
path, err := blobPath(target.Digest)
if err != nil {
return fmt.Errorf("%s: %s: %w", target.Digest, target.MediaType, errdef.ErrInvalidDigest)
}

Check warning on line 114 in content/oci/storage.go

View check run for this annotation

Codecov / codecov/patch

content/oci/storage.go#L113-L114

Added lines #L113 - L114 were not covered by tests
targetPath := filepath.Join(s.root, path)
return os.Remove(targetPath)
}

// ingest write the content into a temporary ingest file.
func (s *Storage) ingest(expected ocispec.Descriptor, content io.Reader) (path string, ingestErr error) {
if err := ensureDir(s.ingestRoot); err != nil {
Expand Down
40 changes: 40 additions & 0 deletions content/oci/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,3 +377,43 @@ func TestStorage_Fetch_Concurrent(t *testing.T) {
t.Fatal(err)
}
}

func TestStorage_Delete(t *testing.T) {
content := []byte("hello world")
desc := ocispec.Descriptor{
MediaType: "test",
Digest: digest.FromBytes(content),
Size: int64(len(content)),
}

tempDir := t.TempDir()
s, err := NewStorage(tempDir)
if err != nil {
t.Fatal("New() error =", err)
}
ctx := context.Background()

if err := s.Push(ctx, desc, bytes.NewReader(content)); err != nil {
t.Fatal("Storage.Push() error =", err)
}

exists, err := s.Exists(ctx, desc)
if err != nil {
t.Fatal("Storage.Exists() error =", err)
}
if !exists {
t.Errorf("Storage.Exists() = %v, want %v", exists, true)
}

err = s.Delete(ctx, desc)
carabasdaniel marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
t.Fatal("Storage.Delete() error =", err)
}
exists, err = s.Exists(ctx, desc)
if err != nil {
t.Fatal("Storage.Exists() error =", err)
}
if exists {
t.Errorf("Storage.Exists() = %v, want %v", exists, false)
}
}
6 changes: 6 additions & 0 deletions content/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ type Storage interface {
Pusher
}

// DeletableStorage represents an extension of the Storage interface that includes the Deleter.
type DeletableStorage interface {
Storage
Deleter
carabasdaniel marked this conversation as resolved.
Show resolved Hide resolved
carabasdaniel marked this conversation as resolved.
Show resolved Hide resolved
}

// ReadOnlyStorage represents a read-only Storage.
type ReadOnlyStorage interface {
Fetcher
Expand Down
10 changes: 10 additions & 0 deletions internal/cas/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,13 @@ func (m *Memory) Map() map[descriptor.Descriptor][]byte {
})
return res
}

// Delete removes a target descriptor from content map.
func (m *Memory) Delete(ctx context.Context, target ocispec.Descriptor) error {
key := descriptor.FromOCI(target)
_, deleted := m.content.LoadAndDelete(key)
if !deleted {
return errdef.ErrNotFound
}
return nil
}
63 changes: 63 additions & 0 deletions internal/cas/memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,66 @@ func TestMemoryBadPush(t *testing.T) {
t.Errorf("Memory.Push() error = %v, wantErr %v", err, true)
}
}

func TestMemoryDelete(t *testing.T) {
content := []byte("hello world")
desc := ocispec.Descriptor{
MediaType: "test",
Digest: digest.FromBytes(content),
Size: int64(len(content)),
}

s := NewMemory()
ctx := context.Background()

err := s.Push(ctx, desc, bytes.NewReader(content))
if err != nil {
t.Fatal("Memory.Push() error =", err)
}

exists, err := s.Exists(ctx, desc)
if err != nil {
t.Fatal("Memory.Exists() error =", err)
}
if !exists {
t.Errorf("Memory.Exists() = %v, want %v", exists, true)
}
if got := len(s.Map()); got != 1 {
t.Errorf("Memory.Map() = %v, want %v", got, 1)
}

err = s.Delete(ctx, desc)
if err != nil {
t.Fatal("Memory.Delete() error =", err)
}
if got := len(s.Map()); got != 0 {
carabasdaniel marked this conversation as resolved.
Show resolved Hide resolved
t.Errorf("Memory.Map() = %v, want %v", got, 0)
}
exists, err = s.Exists(ctx, desc)
if err != nil {
t.Fatal("Memory.Exists() error =", err)
}
if exists {
t.Errorf("Memory.Exists() = %v, want %v", exists, false)
}
}

func TestMemoryBadDelete(t *testing.T) {
content := []byte("hello world")
desc := ocispec.Descriptor{
MediaType: "test",
Digest: digest.FromBytes(content),
Size: int64(len(content)),
}

s := NewMemory()
ctx := context.Background()

err := s.Delete(ctx, desc)
if err == nil {
carabasdaniel marked this conversation as resolved.
Show resolved Hide resolved
t.Errorf("Memory.Delete() error = %v, wantErr %v", err, true)
}
if !errors.Is(err, errdef.ErrNotFound) {
t.Errorf("Memory.Delete() error = %v, want %v", err, errdef.ErrNotFound)
}
}
Loading