From 77dc7dda0efe5034df8c42fda47cd3b0d3c003d1 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Fri, 1 Sep 2023 08:15:13 +0000 Subject: [PATCH 01/26] implemented draft version 0, need to analyze deadlock Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 331 ++++++++++++++++++++++++++++++++++++ content/oci/storage.go | 38 ++++- content/storage.go | 6 + internal/graph/memory.go | 19 +++ internal/resolver/memory.go | 5 + 5 files changed, 395 insertions(+), 4 deletions(-) create mode 100644 content/oci/deletableOci.go diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go new file mode 100644 index 00000000..5b739449 --- /dev/null +++ b/content/oci/deletableOci.go @@ -0,0 +1,331 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package oci provides access to an OCI content store. +// Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md +package oci + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "sync" + + "github.com/opencontainers/go-digest" + specs "github.com/opencontainers/image-spec/specs-go" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "oras.land/oras-go/v2/content" + "oras.land/oras-go/v2/errdef" + "oras.land/oras-go/v2/internal/container/set" + "oras.land/oras-go/v2/internal/descriptor" + "oras.land/oras-go/v2/internal/graph" + "oras.land/oras-go/v2/internal/resolver" +) + +// DeletableStore implements `oras.Target`, and represents a content store +// extended with the delete operation. +// Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md +type DeletableStore struct { + // AutoSaveIndex controls if the OCI store will automatically save the index + // file on each Tag() call. + // - If AutoSaveIndex is set to true, the OCI store will automatically call + // this method on each Tag() call. + // - If AutoSaveIndex is set to false, it's the caller's responsibility + // to manually call SaveIndex() when needed. + // - Default value: true. + AutoSaveIndex bool + root string + indexPath string + index *ocispec.Index + indexLock sync.Mutex + + storage content.DeletableStorage + tagResolver *resolver.Memory + graph *graph.Memory +} + +// NewDeletableStore returns a new DeletableStore. +func NewDeletableStore(root string) (*DeletableStore, error) { + return NewDeletableStoreWithContext(context.Background(), root) +} + +// NewDeletableStoreWithContext creates a new DeletableStore. +func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableStore, error) { + rootAbs, err := filepath.Abs(root) + 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) + } + + store := &DeletableStore{ + AutoSaveIndex: true, + root: rootAbs, + indexPath: filepath.Join(rootAbs, ociImageIndexFile), + storage: storage, + tagResolver: resolver.NewMemory(), + graph: graph.NewMemory(), + } + + if err := ensureDir(filepath.Join(rootAbs, ociBlobsDir)); err != nil { + return nil, err + } + if err := store.ensureOCILayoutFile(); err != nil { + return nil, fmt.Errorf("invalid OCI Image Layout: %w", err) + } + if err := store.loadIndexFile(ctx); err != nil { + return nil, fmt.Errorf("invalid OCI Image Index: %w", err) + } + + return store, nil +} + +// Fetch fetches the content identified by the descriptor. +func (s *DeletableStore) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { + return s.storage.Fetch(ctx, target) +} + +// Push pushes the content, matching the expected descriptor. +func (s *DeletableStore) Push(ctx context.Context, expected ocispec.Descriptor, reader io.Reader) error { + if err := s.storage.Push(ctx, expected, reader); err != nil { + return err + } + if err := s.graph.Index(ctx, s.storage, expected); err != nil { + return err + } + if descriptor.IsManifest(expected) { + // tag by digest + return s.tag(ctx, expected, expected.Digest.String()) + } + return nil +} + +// Delete removes the content matching the descriptor from the store. +func (s *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) error { + resolvers := s.tagResolver.Map() + for reference, desc := range resolvers { + if content.Equal(desc, target) { + s.tagResolver.Delete(reference) + } + } + if err := s.graph.RemoveFromIndex(ctx, target); err != nil { + return err + } + if s.AutoSaveIndex { + err := s.SaveIndex() + if err != nil { + return err + } + } + return s.storage.Delete(ctx, target) +} + +// Exists returns true if the described content exists. +func (s *DeletableStore) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { + return s.storage.Exists(ctx, target) +} + +// Tag tags a descriptor with a reference string. +// reference should be a valid tag (e.g. "latest"). +// Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md#indexjson-file +func (s *DeletableStore) Tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { + if err := validateReference(reference); err != nil { + return err + } + + exists, err := s.storage.Exists(ctx, desc) + if err != nil { + return err + } + if !exists { + return fmt.Errorf("%s: %s: %w", desc.Digest, desc.MediaType, errdef.ErrNotFound) + } + + return s.tag(ctx, desc, reference) +} + +// tag tags a descriptor with a reference string. +func (s *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { + dgst := desc.Digest.String() + if reference != dgst { + // also tag desc by its digest + if err := s.tagResolver.Tag(ctx, desc, dgst); err != nil { + return err + } + } + if err := s.tagResolver.Tag(ctx, desc, reference); err != nil { + return err + } + if s.AutoSaveIndex { + return s.SaveIndex() + } + return nil +} + +// Resolve resolves a reference to a descriptor. If the reference to be resolved +// is a tag, the returned descriptor will be a full descriptor declared by +// github.com/opencontainers/image-spec/specs-go/v1. If the reference is a +// digest the returned descriptor will be a plain descriptor (containing only +// the digest, media type and size). +func (s *DeletableStore) Resolve(ctx context.Context, reference string) (ocispec.Descriptor, error) { + if reference == "" { + return ocispec.Descriptor{}, errdef.ErrMissingReference + } + + // attempt resolving manifest + desc, err := s.tagResolver.Resolve(ctx, reference) + if err != nil { + if errors.Is(err, errdef.ErrNotFound) { + // attempt resolving blob + return resolveBlob(os.DirFS(s.root), reference) + } + return ocispec.Descriptor{}, err + } + + if reference == desc.Digest.String() { + return descriptor.Plain(desc), nil + } + + return desc, nil +} + +// Predecessors returns the nodes directly pointing to the current node. +// Predecessors returns nil without error if the node does not exists in the +// store. +func (s *DeletableStore) Predecessors(ctx context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { + return s.graph.Predecessors(ctx, node) +} + +// Tags lists the tags presented in the `index.json` file of the OCI layout, +// returned in ascending order. +// If `last` is NOT empty, the entries in the response start after the tag +// specified by `last`. Otherwise, the response starts from the top of the tags +// list. +// +// See also `Tags()` in the package `registry`. +func (s *DeletableStore) Tags(ctx context.Context, last string, fn func(tags []string) error) error { + return listTags(ctx, s.tagResolver, last, fn) +} + +// ensureOCILayoutFile ensures the `oci-layout` file. +func (s *DeletableStore) ensureOCILayoutFile() error { + layoutFilePath := filepath.Join(s.root, ocispec.ImageLayoutFile) + layoutFile, err := os.Open(layoutFilePath) + if err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("failed to open OCI layout file: %w", err) + } + + layout := ocispec.ImageLayout{ + Version: ocispec.ImageLayoutVersion, + } + layoutJSON, err := json.Marshal(layout) + if err != nil { + return fmt.Errorf("failed to marshal OCI layout file: %w", err) + } + return os.WriteFile(layoutFilePath, layoutJSON, 0666) + } + defer layoutFile.Close() + + var layout ocispec.ImageLayout + err = json.NewDecoder(layoutFile).Decode(&layout) + if err != nil { + return fmt.Errorf("failed to decode OCI layout file: %w", err) + } + return validateOCILayout(&layout) +} + +// loadIndexFile reads index.json from the file system. +// Create index.json if it does not exist. +func (s *DeletableStore) loadIndexFile(ctx context.Context) error { + indexFile, err := os.Open(s.indexPath) + if err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("failed to open index file: %w", err) + } + + // write index.json if it does not exist + s.index = &ocispec.Index{ + Versioned: specs.Versioned{ + SchemaVersion: 2, // historical value + }, + Manifests: []ocispec.Descriptor{}, + } + return s.writeIndexFile() + } + defer indexFile.Close() + + var index ocispec.Index + if err := json.NewDecoder(indexFile).Decode(&index); err != nil { + return fmt.Errorf("failed to decode index file: %w", err) + } + s.index = &index + return loadIndex(ctx, s.index, s.storage, s.tagResolver, s.graph) +} + +// SaveIndex writes the `index.json` file to the file system. +// - If AutoSaveIndex is set to true (default value), +// the OCI store will automatically call this method on each Tag() call. +// - If AutoSaveIndex is set to false, it's the caller's responsibility +// to manually call this method when needed. +func (s *DeletableStore) SaveIndex() error { + s.indexLock.Lock() + defer s.indexLock.Unlock() + + var manifests []ocispec.Descriptor + tagged := set.New[digest.Digest]() + refMap := s.tagResolver.Map() + + // 1. Add descriptors that are associated with tags + // Note: One descriptor can be associated with multiple tags. + for ref, desc := range refMap { + if ref != desc.Digest.String() { + annotations := make(map[string]string, len(desc.Annotations)+1) + for k, v := range desc.Annotations { + annotations[k] = v + } + annotations[ocispec.AnnotationRefName] = ref + desc.Annotations = annotations + manifests = append(manifests, desc) + // mark the digest as tagged for deduplication in step 2 + tagged.Add(desc.Digest) + } + } + // 2. Add descriptors that are not associated with any tag + for ref, desc := range refMap { + if ref == desc.Digest.String() && !tagged.Contains(desc.Digest) { + // skip tagged ones since they have been added in step 1 + manifests = append(manifests, deleteAnnotationRefName(desc)) + } + } + + s.index.Manifests = manifests + return s.writeIndexFile() +} + +// writeIndexFile writes the `index.json` file. +func (s *DeletableStore) writeIndexFile() error { + indexJSON, err := json.Marshal(s.index) + if err != nil { + return fmt.Errorf("failed to marshal index file: %w", err) + } + return os.WriteFile(s.indexPath, indexJSON, 0666) +} diff --git a/content/oci/storage.go b/content/oci/storage.go index 9d4d03b3..40a29856 100644 --- a/content/oci/storage.go +++ b/content/oci/storage.go @@ -44,7 +44,8 @@ var bufPool = sync.Pool{ // Storage is a CAS based on file system with the OCI-Image layout. // Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md type Storage struct { - *ReadOnlyStorage + rs *ReadOnlyStorage + storageLock sync.RWMutex // root is the root directory of the OCI layout. root string // ingestRoot is the root directory of the temporary ingest files. @@ -59,12 +60,24 @@ func NewStorage(root string) (*Storage, error) { } return &Storage{ - ReadOnlyStorage: NewStorageFromFS(os.DirFS(rootAbs)), - root: rootAbs, - ingestRoot: filepath.Join(rootAbs, "ingest"), + rs: NewStorageFromFS(os.DirFS(rootAbs)), + root: rootAbs, + ingestRoot: filepath.Join(rootAbs, "ingest"), }, nil } +func (s *Storage) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { + s.storageLock.RLock() + defer s.storageLock.RUnlock() + return s.rs.Fetch(ctx, target) +} + +func (s *Storage) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { + s.storageLock.RLock() + defer s.storageLock.RUnlock() + return s.rs.Exists(ctx, target) +} + // Push pushes the content, matching the expected descriptor. func (s *Storage) Push(_ context.Context, expected ocispec.Descriptor, content io.Reader) error { path, err := blobPath(expected.Digest) @@ -73,6 +86,9 @@ func (s *Storage) Push(_ context.Context, expected ocispec.Descriptor, content i } target := filepath.Join(s.root, path) + s.storageLock.Lock() + defer s.storageLock.Unlock() + // check if the target content already exists in the blob directory. if _, err := os.Stat(target); err == nil { return fmt.Errorf("%s: %s: %w", expected.Digest, expected.MediaType, errdef.ErrAlreadyExists) @@ -106,6 +122,20 @@ func (s *Storage) Push(_ context.Context, expected ocispec.Descriptor, content i return nil } +// Delete removes the target from the system. With Delete, Storage implements the +// DeletableStorage interface. +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) + } + targetPath := filepath.Join(s.root, path) + + s.storageLock.Lock() + defer s.storageLock.Unlock() + 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 { diff --git a/content/storage.go b/content/storage.go index 47c95d87..f5ad45ac 100644 --- a/content/storage.go +++ b/content/storage.go @@ -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 +} + // ReadOnlyStorage represents a read-only Storage. type ReadOnlyStorage interface { Fetcher diff --git a/internal/graph/memory.go b/internal/graph/memory.go index 0aa25aee..d867ed27 100644 --- a/internal/graph/memory.go +++ b/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, node ocispec.Descriptor) error { + nodeKey := descriptor.FromOCI(node) + _, loaded := m.indexed.LoadAndDelete(nodeKey) + if loaded { + predecessors, err := m.Predecessors(ctx, node) + if err != nil { + return err + } + for _, predecessor := range predecessors { + if err = m.RemoveFromIndex(ctx, predecessor); err != nil { + return err + } + m.predecessors.Delete(predecessor) + } + } + return nil +} + // index indexes predecessors for each direct successor of the given node. // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. diff --git a/internal/resolver/memory.go b/internal/resolver/memory.go index 6fac5e2d..27d6a59d 100644 --- a/internal/resolver/memory.go +++ b/internal/resolver/memory.go @@ -48,6 +48,11 @@ func (m *Memory) Tag(_ context.Context, desc ocispec.Descriptor, reference strin return nil } +// Delete removes a reference from index map. +func (m *Memory) Delete(reference string) { + m.index.Delete(reference) +} + // Map dumps the memory into a built-in map structure. // Like other operations, calling Map() is go-routine safe. However, it does not // necessarily correspond to any consistent snapshot of the storage contents. From d4949d7f05818092597254eafc456f4ba3f28029 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 4 Sep 2023 06:26:41 +0000 Subject: [PATCH 02/26] moved the lock up to the DeletableStore level Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 68 +++++-------------------------------- content/oci/storage.go | 12 +------ 2 files changed, 10 insertions(+), 70 deletions(-) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index 5b739449..844362f0 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -20,7 +20,6 @@ package oci import ( "context" "encoding/json" - "errors" "fmt" "io" "os" @@ -31,7 +30,6 @@ import ( specs "github.com/opencontainers/image-spec/specs-go" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2/content" - "oras.land/oras-go/v2/errdef" "oras.land/oras-go/v2/internal/container/set" "oras.land/oras-go/v2/internal/descriptor" "oras.land/oras-go/v2/internal/graph" @@ -54,6 +52,7 @@ type DeletableStore struct { indexPath string index *ocispec.Index indexLock sync.Mutex + operationLock sync.RWMutex storage content.DeletableStorage tagResolver *resolver.Memory @@ -100,11 +99,15 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS // Fetch fetches the content identified by the descriptor. func (s *DeletableStore) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { + s.operationLock.RLock() + defer s.operationLock.RUnlock() return s.storage.Fetch(ctx, target) } // Push pushes the content, matching the expected descriptor. func (s *DeletableStore) Push(ctx context.Context, expected ocispec.Descriptor, reader io.Reader) error { + s.operationLock.Lock() + defer s.operationLock.Unlock() if err := s.storage.Push(ctx, expected, reader); err != nil { return err } @@ -120,6 +123,8 @@ func (s *DeletableStore) Push(ctx context.Context, expected ocispec.Descriptor, // Delete removes the content matching the descriptor from the store. func (s *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) error { + s.operationLock.Lock() + defer s.operationLock.Unlock() resolvers := s.tagResolver.Map() for reference, desc := range resolvers { if content.Equal(desc, target) { @@ -140,28 +145,11 @@ func (s *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) // Exists returns true if the described content exists. func (s *DeletableStore) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { + s.operationLock.RLock() + defer s.operationLock.RUnlock() return s.storage.Exists(ctx, target) } -// Tag tags a descriptor with a reference string. -// reference should be a valid tag (e.g. "latest"). -// Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md#indexjson-file -func (s *DeletableStore) Tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { - if err := validateReference(reference); err != nil { - return err - } - - exists, err := s.storage.Exists(ctx, desc) - if err != nil { - return err - } - if !exists { - return fmt.Errorf("%s: %s: %w", desc.Digest, desc.MediaType, errdef.ErrNotFound) - } - - return s.tag(ctx, desc, reference) -} - // tag tags a descriptor with a reference string. func (s *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { dgst := desc.Digest.String() @@ -180,33 +168,6 @@ func (s *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, refer return nil } -// Resolve resolves a reference to a descriptor. If the reference to be resolved -// is a tag, the returned descriptor will be a full descriptor declared by -// github.com/opencontainers/image-spec/specs-go/v1. If the reference is a -// digest the returned descriptor will be a plain descriptor (containing only -// the digest, media type and size). -func (s *DeletableStore) Resolve(ctx context.Context, reference string) (ocispec.Descriptor, error) { - if reference == "" { - return ocispec.Descriptor{}, errdef.ErrMissingReference - } - - // attempt resolving manifest - desc, err := s.tagResolver.Resolve(ctx, reference) - if err != nil { - if errors.Is(err, errdef.ErrNotFound) { - // attempt resolving blob - return resolveBlob(os.DirFS(s.root), reference) - } - return ocispec.Descriptor{}, err - } - - if reference == desc.Digest.String() { - return descriptor.Plain(desc), nil - } - - return desc, nil -} - // Predecessors returns the nodes directly pointing to the current node. // Predecessors returns nil without error if the node does not exists in the // store. @@ -214,17 +175,6 @@ func (s *DeletableStore) Predecessors(ctx context.Context, node ocispec.Descript return s.graph.Predecessors(ctx, node) } -// Tags lists the tags presented in the `index.json` file of the OCI layout, -// returned in ascending order. -// If `last` is NOT empty, the entries in the response start after the tag -// specified by `last`. Otherwise, the response starts from the top of the tags -// list. -// -// See also `Tags()` in the package `registry`. -func (s *DeletableStore) Tags(ctx context.Context, last string, fn func(tags []string) error) error { - return listTags(ctx, s.tagResolver, last, fn) -} - // ensureOCILayoutFile ensures the `oci-layout` file. func (s *DeletableStore) ensureOCILayoutFile() error { layoutFilePath := filepath.Join(s.root, ocispec.ImageLayoutFile) diff --git a/content/oci/storage.go b/content/oci/storage.go index 40a29856..9a07a2ec 100644 --- a/content/oci/storage.go +++ b/content/oci/storage.go @@ -44,8 +44,7 @@ var bufPool = sync.Pool{ // Storage is a CAS based on file system with the OCI-Image layout. // Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md type Storage struct { - rs *ReadOnlyStorage - storageLock sync.RWMutex + rs *ReadOnlyStorage // root is the root directory of the OCI layout. root string // ingestRoot is the root directory of the temporary ingest files. @@ -67,14 +66,10 @@ func NewStorage(root string) (*Storage, error) { } func (s *Storage) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { - s.storageLock.RLock() - defer s.storageLock.RUnlock() return s.rs.Fetch(ctx, target) } func (s *Storage) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { - s.storageLock.RLock() - defer s.storageLock.RUnlock() return s.rs.Exists(ctx, target) } @@ -86,9 +81,6 @@ func (s *Storage) Push(_ context.Context, expected ocispec.Descriptor, content i } target := filepath.Join(s.root, path) - s.storageLock.Lock() - defer s.storageLock.Unlock() - // check if the target content already exists in the blob directory. if _, err := os.Stat(target); err == nil { return fmt.Errorf("%s: %s: %w", expected.Digest, expected.MediaType, errdef.ErrAlreadyExists) @@ -131,8 +123,6 @@ func (s *Storage) Delete(ctx context.Context, target ocispec.Descriptor) error { } targetPath := filepath.Join(s.root, path) - s.storageLock.Lock() - defer s.storageLock.Unlock() return os.Remove(targetPath) } From 5cd6043c5b130cb09a3c5280e1694e649490af0a Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 4 Sep 2023 06:33:50 +0000 Subject: [PATCH 03/26] removed the DeletableStorage interface Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 2 +- content/storage.go | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index 844362f0..35e930ff 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -54,7 +54,7 @@ type DeletableStore struct { indexLock sync.Mutex operationLock sync.RWMutex - storage content.DeletableStorage + storage *Storage tagResolver *resolver.Memory graph *graph.Memory } diff --git a/content/storage.go b/content/storage.go index f5ad45ac..47c95d87 100644 --- a/content/storage.go +++ b/content/storage.go @@ -45,12 +45,6 @@ type Storage interface { Pusher } -// DeletableStorage represents an extension of the Storage interface that includes the Deleter. -type DeletableStorage interface { - Storage - Deleter -} - // ReadOnlyStorage represents a read-only Storage. type ReadOnlyStorage interface { Fetcher From 4a0bf32c3051348758f1cfa7c6686b79a5e3f4f6 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 4 Sep 2023 06:41:56 +0000 Subject: [PATCH 04/26] added unit tests Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci_test.go | 69 ++++++++++++++++++++++++++++++++ content/oci/storage_test.go | 40 ++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 content/oci/deletableOci_test.go diff --git a/content/oci/deletableOci_test.go b/content/oci/deletableOci_test.go new file mode 100644 index 00000000..15ce1c31 --- /dev/null +++ b/content/oci/deletableOci_test.go @@ -0,0 +1,69 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package oci provides access to an OCI content store. +// Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md +package oci + +import ( + "bytes" + "context" + "testing" + + "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" +) + +func TestDeletableStore_Delete(t *testing.T) { + content := []byte("test delete") + desc := ocispec.Descriptor{ + MediaType: "test-delete", + Digest: digest.FromBytes(content), + Size: int64(len(content)), + } + + tempDir := t.TempDir() + s, err := NewDeletableStore(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) + } + + 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.Errorf("Store.Delete() = %v, wantErr %v", err, true) + } + + 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, false) + } +} diff --git a/content/oci/storage_test.go b/content/oci/storage_test.go index ac6ae8dd..c4f8aa95 100644 --- a/content/oci/storage_test.go +++ b/content/oci/storage_test.go @@ -377,3 +377,43 @@ func TestStorage_Fetch_Concurrent(t *testing.T) { t.Fatal(err) } } + +func TestStorage_Delete(t *testing.T) { + content := []byte("test delete") + 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) + 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) + } +} From 110fb3eda14917725908d2e0f7ba082c488ca235 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 4 Sep 2023 06:50:23 +0000 Subject: [PATCH 05/26] added Resolve Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 125 +++++++++++++++++++------------ content/oci/deletableOci_test.go | 18 ++++- 2 files changed, 95 insertions(+), 48 deletions(-) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index 35e930ff..4adb2b38 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -20,6 +20,7 @@ package oci import ( "context" "encoding/json" + "errors" "fmt" "io" "os" @@ -30,6 +31,7 @@ import ( specs "github.com/opencontainers/image-spec/specs-go" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2/content" + "oras.land/oras-go/v2/errdef" "oras.land/oras-go/v2/internal/container/set" "oras.land/oras-go/v2/internal/descriptor" "oras.land/oras-go/v2/internal/graph" @@ -98,86 +100,115 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS } // Fetch fetches the content identified by the descriptor. -func (s *DeletableStore) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { - s.operationLock.RLock() - defer s.operationLock.RUnlock() - return s.storage.Fetch(ctx, target) +func (ds *DeletableStore) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { + ds.operationLock.RLock() + defer ds.operationLock.RUnlock() + return ds.storage.Fetch(ctx, target) } // Push pushes the content, matching the expected descriptor. -func (s *DeletableStore) Push(ctx context.Context, expected ocispec.Descriptor, reader io.Reader) error { - s.operationLock.Lock() - defer s.operationLock.Unlock() - if err := s.storage.Push(ctx, expected, reader); err != nil { +func (ds *DeletableStore) Push(ctx context.Context, expected ocispec.Descriptor, reader io.Reader) error { + ds.operationLock.Lock() + defer ds.operationLock.Unlock() + if err := ds.storage.Push(ctx, expected, reader); err != nil { return err } - if err := s.graph.Index(ctx, s.storage, expected); err != nil { + if err := ds.graph.Index(ctx, ds.storage, expected); err != nil { return err } if descriptor.IsManifest(expected) { // tag by digest - return s.tag(ctx, expected, expected.Digest.String()) + return ds.tag(ctx, expected, expected.Digest.String()) } return nil } // Delete removes the content matching the descriptor from the store. -func (s *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) error { - s.operationLock.Lock() - defer s.operationLock.Unlock() - resolvers := s.tagResolver.Map() +func (ds *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) error { + ds.operationLock.Lock() + defer ds.operationLock.Unlock() + resolvers := ds.tagResolver.Map() for reference, desc := range resolvers { if content.Equal(desc, target) { - s.tagResolver.Delete(reference) + ds.tagResolver.Delete(reference) } } - if err := s.graph.RemoveFromIndex(ctx, target); err != nil { + if err := ds.graph.RemoveFromIndex(ctx, target); err != nil { return err } - if s.AutoSaveIndex { - err := s.SaveIndex() + if ds.AutoSaveIndex { + err := ds.SaveIndex() if err != nil { return err } } - return s.storage.Delete(ctx, target) + return ds.storage.Delete(ctx, target) } // Exists returns true if the described content exists. -func (s *DeletableStore) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { - s.operationLock.RLock() - defer s.operationLock.RUnlock() - return s.storage.Exists(ctx, target) +func (ds *DeletableStore) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { + ds.operationLock.RLock() + defer ds.operationLock.RUnlock() + return ds.storage.Exists(ctx, target) } // tag tags a descriptor with a reference string. -func (s *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { +func (ds *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { dgst := desc.Digest.String() if reference != dgst { // also tag desc by its digest - if err := s.tagResolver.Tag(ctx, desc, dgst); err != nil { + if err := ds.tagResolver.Tag(ctx, desc, dgst); err != nil { return err } } - if err := s.tagResolver.Tag(ctx, desc, reference); err != nil { + if err := ds.tagResolver.Tag(ctx, desc, reference); err != nil { return err } - if s.AutoSaveIndex { - return s.SaveIndex() + if ds.AutoSaveIndex { + return ds.SaveIndex() } return nil } +// Resolve resolves a reference to a descriptor. If the reference to be resolved +// is a tag, the returned descriptor will be a full descriptor declared by +// github.com/opencontainers/image-spec/specs-go/v1. If the reference is a +// digest the returned descriptor will be a plain descriptor (containing only +// the digest, media type and size). +func (ds *DeletableStore) Resolve(ctx context.Context, reference string) (ocispec.Descriptor, error) { + ds.operationLock.RLock() + defer ds.operationLock.RUnlock() + if reference == "" { + return ocispec.Descriptor{}, errdef.ErrMissingReference + } + + // attempt resolving manifest + desc, err := ds.tagResolver.Resolve(ctx, reference) + if err != nil { + if errors.Is(err, errdef.ErrNotFound) { + // attempt resolving blob + return resolveBlob(os.DirFS(ds.root), reference) + } + return ocispec.Descriptor{}, err + } + + if reference == desc.Digest.String() { + return descriptor.Plain(desc), nil + } + + return desc, nil +} + // Predecessors returns the nodes directly pointing to the current node. // Predecessors returns nil without error if the node does not exists in the // store. -func (s *DeletableStore) Predecessors(ctx context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { - return s.graph.Predecessors(ctx, node) +func (ds *DeletableStore) Predecessors(ctx context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { + return ds.graph.Predecessors(ctx, node) } // ensureOCILayoutFile ensures the `oci-layout` file. -func (s *DeletableStore) ensureOCILayoutFile() error { - layoutFilePath := filepath.Join(s.root, ocispec.ImageLayoutFile) +func (ds *DeletableStore) ensureOCILayoutFile() error { + layoutFilePath := filepath.Join(ds.root, ocispec.ImageLayoutFile) layoutFile, err := os.Open(layoutFilePath) if err != nil { if !os.IsNotExist(err) { @@ -205,21 +236,21 @@ func (s *DeletableStore) ensureOCILayoutFile() error { // loadIndexFile reads index.json from the file system. // Create index.json if it does not exist. -func (s *DeletableStore) loadIndexFile(ctx context.Context) error { - indexFile, err := os.Open(s.indexPath) +func (ds *DeletableStore) loadIndexFile(ctx context.Context) error { + indexFile, err := os.Open(ds.indexPath) if err != nil { if !os.IsNotExist(err) { return fmt.Errorf("failed to open index file: %w", err) } // write index.json if it does not exist - s.index = &ocispec.Index{ + ds.index = &ocispec.Index{ Versioned: specs.Versioned{ SchemaVersion: 2, // historical value }, Manifests: []ocispec.Descriptor{}, } - return s.writeIndexFile() + return ds.writeIndexFile() } defer indexFile.Close() @@ -227,8 +258,8 @@ func (s *DeletableStore) loadIndexFile(ctx context.Context) error { if err := json.NewDecoder(indexFile).Decode(&index); err != nil { return fmt.Errorf("failed to decode index file: %w", err) } - s.index = &index - return loadIndex(ctx, s.index, s.storage, s.tagResolver, s.graph) + ds.index = &index + return loadIndex(ctx, ds.index, ds.storage, ds.tagResolver, ds.graph) } // SaveIndex writes the `index.json` file to the file system. @@ -236,13 +267,13 @@ func (s *DeletableStore) loadIndexFile(ctx context.Context) error { // the OCI store will automatically call this method on each Tag() call. // - If AutoSaveIndex is set to false, it's the caller's responsibility // to manually call this method when needed. -func (s *DeletableStore) SaveIndex() error { - s.indexLock.Lock() - defer s.indexLock.Unlock() +func (ds *DeletableStore) SaveIndex() error { + ds.indexLock.Lock() + defer ds.indexLock.Unlock() var manifests []ocispec.Descriptor tagged := set.New[digest.Digest]() - refMap := s.tagResolver.Map() + refMap := ds.tagResolver.Map() // 1. Add descriptors that are associated with tags // Note: One descriptor can be associated with multiple tags. @@ -267,15 +298,15 @@ func (s *DeletableStore) SaveIndex() error { } } - s.index.Manifests = manifests - return s.writeIndexFile() + ds.index.Manifests = manifests + return ds.writeIndexFile() } // writeIndexFile writes the `index.json` file. -func (s *DeletableStore) writeIndexFile() error { - indexJSON, err := json.Marshal(s.index) +func (ds *DeletableStore) writeIndexFile() error { + indexJSON, err := json.Marshal(ds.index) if err != nil { return fmt.Errorf("failed to marshal index file: %w", err) } - return os.WriteFile(s.indexPath, indexJSON, 0666) + return os.WriteFile(ds.indexPath, indexJSON, 0666) } diff --git a/content/oci/deletableOci_test.go b/content/oci/deletableOci_test.go index 15ce1c31..f569248d 100644 --- a/content/oci/deletableOci_test.go +++ b/content/oci/deletableOci_test.go @@ -20,19 +20,21 @@ package oci import ( "bytes" "context" + "reflect" "testing" "github.com/opencontainers/go-digest" ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) -func TestDeletableStore_Delete(t *testing.T) { +func TestDeletableStore(t *testing.T) { content := []byte("test delete") desc := ocispec.Descriptor{ MediaType: "test-delete", Digest: digest.FromBytes(content), Size: int64(len(content)), } + ref := "latest" tempDir := t.TempDir() s, err := NewDeletableStore(tempDir) @@ -54,6 +56,20 @@ func TestDeletableStore_Delete(t *testing.T) { t.Errorf("Store.Exists() = %v, want %v", exists, true) } + 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, desc) if err != nil { t.Errorf("Store.Delete() = %v, wantErr %v", err, true) From daa8dcdb982c19e0deec40be8b5f48ec701e7c1e Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 4 Sep 2023 06:57:28 +0000 Subject: [PATCH 06/26] removed the RS variable name Signed-off-by: Xiaoxuan Wang --- content/oci/storage.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/content/oci/storage.go b/content/oci/storage.go index 9a07a2ec..a1803e9a 100644 --- a/content/oci/storage.go +++ b/content/oci/storage.go @@ -44,7 +44,7 @@ var bufPool = sync.Pool{ // Storage is a CAS based on file system with the OCI-Image layout. // Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md type Storage struct { - rs *ReadOnlyStorage + *ReadOnlyStorage // root is the root directory of the OCI layout. root string // ingestRoot is the root directory of the temporary ingest files. @@ -59,20 +59,12 @@ func NewStorage(root string) (*Storage, error) { } return &Storage{ - rs: NewStorageFromFS(os.DirFS(rootAbs)), - root: rootAbs, - ingestRoot: filepath.Join(rootAbs, "ingest"), + ReadOnlyStorage: NewStorageFromFS(os.DirFS(rootAbs)), + root: rootAbs, + ingestRoot: filepath.Join(rootAbs, "ingest"), }, nil } -func (s *Storage) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { - return s.rs.Fetch(ctx, target) -} - -func (s *Storage) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { - return s.rs.Exists(ctx, target) -} - // Push pushes the content, matching the expected descriptor. func (s *Storage) Push(_ context.Context, expected ocispec.Descriptor, content io.Reader) error { path, err := blobPath(expected.Digest) From 536664b9ffed5e825d1a770350fc53ca34af0210 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 4 Sep 2023 08:07:11 +0000 Subject: [PATCH 07/26] added back Tag and Tags Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 58 ++++++++++++++++++++++++++------ content/oci/deletableOci_test.go | 10 +++--- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index 4adb2b38..ba47432e 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -54,7 +54,7 @@ type DeletableStore struct { indexPath string index *ocispec.Index indexLock sync.Mutex - operationLock sync.RWMutex + lock sync.RWMutex storage *Storage tagResolver *resolver.Memory @@ -101,15 +101,15 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS // Fetch fetches the content identified by the descriptor. func (ds *DeletableStore) Fetch(ctx context.Context, target ocispec.Descriptor) (io.ReadCloser, error) { - ds.operationLock.RLock() - defer ds.operationLock.RUnlock() + ds.lock.RLock() + defer ds.lock.RUnlock() return ds.storage.Fetch(ctx, target) } // Push pushes the content, matching the expected descriptor. func (ds *DeletableStore) Push(ctx context.Context, expected ocispec.Descriptor, reader io.Reader) error { - ds.operationLock.Lock() - defer ds.operationLock.Unlock() + ds.lock.Lock() + defer ds.lock.Unlock() if err := ds.storage.Push(ctx, expected, reader); err != nil { return err } @@ -125,8 +125,8 @@ func (ds *DeletableStore) Push(ctx context.Context, expected ocispec.Descriptor, // Delete removes the content matching the descriptor from the store. func (ds *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) error { - ds.operationLock.Lock() - defer ds.operationLock.Unlock() + ds.lock.Lock() + defer ds.lock.Unlock() resolvers := ds.tagResolver.Map() for reference, desc := range resolvers { if content.Equal(desc, target) { @@ -147,11 +147,32 @@ func (ds *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) // Exists returns true if the described content exists. func (ds *DeletableStore) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) { - ds.operationLock.RLock() - defer ds.operationLock.RUnlock() + ds.lock.RLock() + defer ds.lock.RUnlock() return ds.storage.Exists(ctx, target) } +// Tag tags a descriptor with a reference string. +// reference should be a valid tag (e.g. "latest"). +// Reference: https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/image-layout.md#indexjson-file +func (ds *DeletableStore) Tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { + ds.lock.Lock() + defer ds.lock.Unlock() + if err := validateReference(reference); err != nil { + return err + } + + exists, err := ds.storage.Exists(ctx, desc) + if err != nil { + return err + } + if !exists { + return fmt.Errorf("%s: %s: %w", desc.Digest, desc.MediaType, errdef.ErrNotFound) + } + + return ds.tag(ctx, desc, reference) +} + // tag tags a descriptor with a reference string. func (ds *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, reference string) error { dgst := desc.Digest.String() @@ -176,8 +197,8 @@ func (ds *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, refe // digest the returned descriptor will be a plain descriptor (containing only // the digest, media type and size). func (ds *DeletableStore) Resolve(ctx context.Context, reference string) (ocispec.Descriptor, error) { - ds.operationLock.RLock() - defer ds.operationLock.RUnlock() + ds.lock.RLock() + defer ds.lock.RUnlock() if reference == "" { return ocispec.Descriptor{}, errdef.ErrMissingReference } @@ -203,9 +224,24 @@ func (ds *DeletableStore) Resolve(ctx context.Context, reference string) (ocispe // Predecessors returns nil without error if the node does not exists in the // store. func (ds *DeletableStore) Predecessors(ctx context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { + ds.lock.RLock() + defer ds.lock.RUnlock() return ds.graph.Predecessors(ctx, node) } +// Tags lists the tags presented in the `index.json` file of the OCI layout, +// returned in ascending order. +// If `last` is NOT empty, the entries in the response start after the tag +// specified by `last`. Otherwise, the response starts from the top of the tags +// list. +// +// See also `Tags()` in the package `registry`. +func (ds *DeletableStore) Tags(ctx context.Context, last string, fn func(tags []string) error) error { + ds.lock.RLock() + defer ds.lock.RUnlock() + return listTags(ctx, ds.tagResolver, last, fn) +} + // ensureOCILayoutFile ensures the `oci-layout` file. func (ds *DeletableStore) ensureOCILayoutFile() error { layoutFilePath := filepath.Join(ds.root, ocispec.ImageLayoutFile) diff --git a/content/oci/deletableOci_test.go b/content/oci/deletableOci_test.go index f569248d..2f00f137 100644 --- a/content/oci/deletableOci_test.go +++ b/content/oci/deletableOci_test.go @@ -48,6 +48,11 @@ func TestDeletableStore(t *testing.T) { 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) + } + exists, err := s.Exists(ctx, desc) if err != nil { t.Fatal("Store.Exists() error =", err) @@ -56,11 +61,6 @@ func TestDeletableStore(t *testing.T) { t.Errorf("Store.Exists() = %v, want %v", exists, true) } - 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) From 5a8ad61bb7a11b74183db47cdb27cedb2300a93e Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 4 Sep 2023 23:23:12 +0000 Subject: [PATCH 08/26] merged the locks and refactored saveIndex Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index ba47432e..316b8ee6 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -53,7 +53,6 @@ type DeletableStore struct { root string indexPath string index *ocispec.Index - indexLock sync.Mutex lock sync.RWMutex storage *Storage @@ -137,7 +136,7 @@ func (ds *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) return err } if ds.AutoSaveIndex { - err := ds.SaveIndex() + err := ds.saveIndex() if err != nil { return err } @@ -186,7 +185,7 @@ func (ds *DeletableStore) tag(ctx context.Context, desc ocispec.Descriptor, refe return err } if ds.AutoSaveIndex { - return ds.SaveIndex() + return ds.saveIndex() } return nil } @@ -300,13 +299,16 @@ func (ds *DeletableStore) loadIndexFile(ctx context.Context) error { // SaveIndex writes the `index.json` file to the file system. // - If AutoSaveIndex is set to true (default value), -// the OCI store will automatically call this method on each Tag() call. +// the OCI store will automatically save the index on each Tag() call. // - If AutoSaveIndex is set to false, it's the caller's responsibility // to manually call this method when needed. func (ds *DeletableStore) SaveIndex() error { - ds.indexLock.Lock() - defer ds.indexLock.Unlock() + ds.lock.Lock() + defer ds.lock.Unlock() + return ds.saveIndex() +} +func (ds *DeletableStore) saveIndex() error { var manifests []ocispec.Descriptor tagged := set.New[digest.Digest]() refMap := ds.tagResolver.Map() From b1c621c210c2c6f4c51f6ed103f452967b11d439 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Tue, 5 Sep 2023 08:43:07 +0000 Subject: [PATCH 09/26] implemented the delete. Need to add extensive tests and refine Signed-off-by: Xiaoxuan Wang --- internal/graph/memory.go | 55 ++++++++++++++++++++++++----------- internal/graph/memory_test.go | 41 ++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 17 deletions(-) create mode 100644 internal/graph/memory_test.go diff --git a/internal/graph/memory.go b/internal/graph/memory.go index d867ed27..446cdfde 100644 --- a/internal/graph/memory.go +++ b/internal/graph/memory.go @@ -18,6 +18,7 @@ package graph import ( "context" "errors" + "fmt" "sync" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -31,6 +32,7 @@ import ( // Memory is a memory based PredecessorFinder. type Memory struct { predecessors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor + successors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor indexed sync.Map // map[descriptor.Descriptor]any } @@ -116,22 +118,37 @@ 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. +// RemoveFromIndex removes the node from its predecessors and successors. func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) error { nodeKey := descriptor.FromOCI(node) - _, loaded := m.indexed.LoadAndDelete(nodeKey) - if loaded { - predecessors, err := m.Predecessors(ctx, node) - if err != nil { - return err - } - for _, predecessor := range predecessors { - if err = m.RemoveFromIndex(ctx, predecessor); err != nil { - return err - } - m.predecessors.Delete(predecessor) - } + // remove the node from its predecessors' successor list + preds, exists := m.predecessors.Load(nodeKey) + if !exists { + return fmt.Errorf("predecessors of the node is not found") + } + predecessors := preds.(*sync.Map) + predecessors.Range(func(key, value interface{}) bool { + succs, _ := m.successors.Load(key) + successors := succs.(*sync.Map) + successors.Delete(nodeKey) + return true + }) + // remove the node from its successors' predecessor list + succs, exists := m.successors.Load(nodeKey) + if !exists { + return fmt.Errorf("successors of the node is not found") } + successors := succs.(*sync.Map) + successors.Range(func(key, value interface{}) bool { + preds, _ := m.predecessors.Load(key) + predecessors := preds.(*sync.Map) + predecessors.Delete(nodeKey) + return true + }) + // remove the node's entries in the maps + m.predecessors.Delete(nodeKey) + m.successors.Delete(nodeKey) + m.indexed.Delete(nodeKey) return nil } @@ -142,12 +159,16 @@ func (m *Memory) index(ctx context.Context, node ocispec.Descriptor, successors if len(successors) == 0 { return } - predecessorKey := descriptor.FromOCI(node) for _, successor := range successors { successorKey := descriptor.FromOCI(successor) - value, _ := m.predecessors.LoadOrStore(successorKey, &sync.Map{}) - predecessors := value.(*sync.Map) - predecessors.Store(predecessorKey, node) + // store in m.predecessors + pred, _ := m.predecessors.LoadOrStore(successorKey, &sync.Map{}) + predecessorsMap := pred.(*sync.Map) + predecessorsMap.Store(predecessorKey, node) + // store in m.successors + succ, _ := m.successors.LoadOrStore(predecessorKey, &sync.Map{}) + successorsMap := succ.(*sync.Map) + successorsMap.Store(successorKey, successor) } } diff --git a/internal/graph/memory_test.go b/internal/graph/memory_test.go new file mode 100644 index 00000000..285c101a --- /dev/null +++ b/internal/graph/memory_test.go @@ -0,0 +1,41 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package graph + +// func validateMemory(m *Memory, node ocispec.Descriptor, successors []ocispec.Descriptor) error { + +// return nil +// } + +// func TestMemory_index(t *testing.T) { +// tests := []struct { +// name string +// node ocispec.Descriptor +// successors []ocispec.Descriptor +// }{ +// {"A->B", +// ocispec.Descriptor{MediaType: "A"}, +// []ocispec.Descriptor{ +// {MediaType: "B"}, +// }}, +// } +// for _, tt := range tests { +// t.Run(tt.name, func(t *testing.T) { +// m := NewMemory() +// m.index(context.Background(), tt.node, tt.successors) +// }) +// } +// } From 116334552ae0df1eb85e0f2f839c1adf870d5e99 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 6 Sep 2023 03:20:17 +0000 Subject: [PATCH 10/26] refined delete and removed the draft test Signed-off-by: Xiaoxuan Wang --- internal/graph/memory.go | 26 ++++++---------------- internal/graph/memory_test.go | 41 ----------------------------------- 2 files changed, 7 insertions(+), 60 deletions(-) delete mode 100644 internal/graph/memory_test.go diff --git a/internal/graph/memory.go b/internal/graph/memory.go index 446cdfde..46acdc31 100644 --- a/internal/graph/memory.go +++ b/internal/graph/memory.go @@ -121,27 +121,15 @@ func (m *Memory) Predecessors(_ context.Context, node ocispec.Descriptor) ([]oci // RemoveFromIndex removes the node from its predecessors and successors. func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) error { nodeKey := descriptor.FromOCI(node) - // remove the node from its predecessors' successor list - preds, exists := m.predecessors.Load(nodeKey) - if !exists { - return fmt.Errorf("predecessors of the node is not found") - } - predecessors := preds.(*sync.Map) - predecessors.Range(func(key, value interface{}) bool { - succs, _ := m.successors.Load(key) - successors := succs.(*sync.Map) - successors.Delete(nodeKey) - return true - }) // remove the node from its successors' predecessor list - succs, exists := m.successors.Load(nodeKey) + value, exists := m.successors.Load(nodeKey) if !exists { return fmt.Errorf("successors of the node is not found") } - successors := succs.(*sync.Map) - successors.Range(func(key, value interface{}) bool { - preds, _ := m.predecessors.Load(key) - predecessors := preds.(*sync.Map) + successors := value.(*sync.Map) + successors.Range(func(key, _ interface{}) bool { + value, _ = m.predecessors.Load(key) + predecessors := value.(*sync.Map) predecessors.Delete(nodeKey) return true }) @@ -162,11 +150,11 @@ func (m *Memory) index(ctx context.Context, node ocispec.Descriptor, successors predecessorKey := descriptor.FromOCI(node) for _, successor := range successors { successorKey := descriptor.FromOCI(successor) - // store in m.predecessors + // store in m.predecessors, memory.predecessors[successorKey].Store(node) pred, _ := m.predecessors.LoadOrStore(successorKey, &sync.Map{}) predecessorsMap := pred.(*sync.Map) predecessorsMap.Store(predecessorKey, node) - // store in m.successors + // store in m.successors, memory.successors[predecessorKey].Store(successor) succ, _ := m.successors.LoadOrStore(predecessorKey, &sync.Map{}) successorsMap := succ.(*sync.Map) successorsMap.Store(successorKey, successor) diff --git a/internal/graph/memory_test.go b/internal/graph/memory_test.go deleted file mode 100644 index 285c101a..00000000 --- a/internal/graph/memory_test.go +++ /dev/null @@ -1,41 +0,0 @@ -/* -Copyright The ORAS Authors. -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - -http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package graph - -// func validateMemory(m *Memory, node ocispec.Descriptor, successors []ocispec.Descriptor) error { - -// return nil -// } - -// func TestMemory_index(t *testing.T) { -// tests := []struct { -// name string -// node ocispec.Descriptor -// successors []ocispec.Descriptor -// }{ -// {"A->B", -// ocispec.Descriptor{MediaType: "A"}, -// []ocispec.Descriptor{ -// {MediaType: "B"}, -// }}, -// } -// for _, tt := range tests { -// t.Run(tt.name, func(t *testing.T) { -// m := NewMemory() -// m.index(context.Background(), tt.node, tt.successors) -// }) -// } -// } From b27a59cd84e2f3cd9c9fbb7b08895093ecbb2bbb Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 6 Sep 2023 03:33:28 +0000 Subject: [PATCH 11/26] fixed failed unit test Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci_test.go | 2 +- internal/graph/memory.go | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/content/oci/deletableOci_test.go b/content/oci/deletableOci_test.go index 2f00f137..dca14a92 100644 --- a/content/oci/deletableOci_test.go +++ b/content/oci/deletableOci_test.go @@ -72,7 +72,7 @@ func TestDeletableStore(t *testing.T) { err = s.Delete(ctx, desc) if err != nil { - t.Errorf("Store.Delete() = %v, wantErr %v", err, true) + t.Errorf("Store.Delete() = %v, wantErr %v", err, nil) } exists, err = s.Exists(ctx, desc) diff --git a/internal/graph/memory.go b/internal/graph/memory.go index 46acdc31..e5155552 100644 --- a/internal/graph/memory.go +++ b/internal/graph/memory.go @@ -124,7 +124,7 @@ func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) e // remove the node from its successors' predecessor list value, exists := m.successors.Load(nodeKey) if !exists { - return fmt.Errorf("successors of the node is not found") + return fmt.Errorf("successors of the node %v is not found", node) } successors := value.(*sync.Map) successors.Range(func(key, _ interface{}) bool { @@ -144,10 +144,12 @@ func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) e // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. func (m *Memory) index(ctx context.Context, node ocispec.Descriptor, successors []ocispec.Descriptor) { + predecessorKey := descriptor.FromOCI(node) if len(successors) == 0 { + // still create the entry for the node + m.successors.LoadOrStore(predecessorKey, &sync.Map{}) return } - predecessorKey := descriptor.FromOCI(node) for _, successor := range successors { successorKey := descriptor.FromOCI(successor) // store in m.predecessors, memory.predecessors[successorKey].Store(node) From 7db91b6deddf235fa924c2d29be99b41627b5bdd Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 6 Sep 2023 06:49:43 +0000 Subject: [PATCH 12/26] added test Signed-off-by: Xiaoxuan Wang --- internal/graph/memory.go | 32 +++-- internal/graph/memory_test.go | 244 ++++++++++++++++++++++++++++++++++ 2 files changed, 263 insertions(+), 13 deletions(-) create mode 100644 internal/graph/memory_test.go diff --git a/internal/graph/memory.go b/internal/graph/memory.go index e5155552..f15015b3 100644 --- a/internal/graph/memory.go +++ b/internal/graph/memory.go @@ -18,7 +18,6 @@ package graph import ( "context" "errors" - "fmt" "sync" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -122,10 +121,7 @@ func (m *Memory) Predecessors(_ context.Context, node ocispec.Descriptor) ([]oci func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) error { nodeKey := descriptor.FromOCI(node) // remove the node from its successors' predecessor list - value, exists := m.successors.Load(nodeKey) - if !exists { - return fmt.Errorf("successors of the node %v is not found", node) - } + value, _ := m.successors.Load(nodeKey) successors := value.(*sync.Map) successors.Range(func(key, _ interface{}) bool { value, _ = m.predecessors.Load(key) @@ -133,10 +129,7 @@ func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) e predecessors.Delete(nodeKey) return true }) - // remove the node's entries in the maps - m.predecessors.Delete(nodeKey) - m.successors.Delete(nodeKey) - m.indexed.Delete(nodeKey) + m.removeFromMemory(ctx, node) return nil } @@ -144,12 +137,11 @@ func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) e // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. func (m *Memory) index(ctx context.Context, node ocispec.Descriptor, successors []ocispec.Descriptor) { - predecessorKey := descriptor.FromOCI(node) + m.indexIntoMemory(ctx, node) if len(successors) == 0 { - // still create the entry for the node - m.successors.LoadOrStore(predecessorKey, &sync.Map{}) return } + predecessorKey := descriptor.FromOCI(node) for _, successor := range successors { successorKey := descriptor.FromOCI(successor) // store in m.predecessors, memory.predecessors[successorKey].Store(node) @@ -157,8 +149,22 @@ func (m *Memory) index(ctx context.Context, node ocispec.Descriptor, successors predecessorsMap := pred.(*sync.Map) predecessorsMap.Store(predecessorKey, node) // store in m.successors, memory.successors[predecessorKey].Store(successor) - succ, _ := m.successors.LoadOrStore(predecessorKey, &sync.Map{}) + succ, _ := m.successors.Load(predecessorKey) successorsMap := succ.(*sync.Map) successorsMap.Store(successorKey, successor) } } + +func (m *Memory) indexIntoMemory(ctx context.Context, node ocispec.Descriptor) { + key := descriptor.FromOCI(node) + m.predecessors.LoadOrStore(key, &sync.Map{}) + m.successors.LoadOrStore(key, &sync.Map{}) + m.indexed.LoadOrStore(key, &sync.Map{}) +} + +func (m *Memory) removeFromMemory(ctx context.Context, node ocispec.Descriptor) { + key := descriptor.FromOCI(node) + m.predecessors.Delete(key) + m.successors.Delete(key) + m.indexed.Delete(key) +} diff --git a/internal/graph/memory_test.go b/internal/graph/memory_test.go new file mode 100644 index 00000000..a35e0e5b --- /dev/null +++ b/internal/graph/memory_test.go @@ -0,0 +1,244 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package graph + +import ( + "context" + "sync" + "testing" + + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "oras.land/oras-go/v2/internal/descriptor" +) + +var ( + A = ocispec.Descriptor{MediaType: "A"} + AKey = descriptor.FromOCI(A) + B = ocispec.Descriptor{MediaType: "B"} + BKey = descriptor.FromOCI(B) + C = ocispec.Descriptor{MediaType: "C"} + CKey = descriptor.FromOCI(C) + D = ocispec.Descriptor{MediaType: "D"} + DKey = descriptor.FromOCI(D) +) + +// 条件： +// 1. 只要node被index（作为函数的第二个输入），node就在predecessors，successors，indexed中有entry +// 2. 只要node被Removed from index，node就在predecessors，successors，indexed中都没有entry +// 3. 只有node被index（作为函数的第二个输入），successors中才有其entry +// 4. successors中的内容与node中的内容一致，只要entry还在，其内容就不会改变 + +// GC情况： +// predecessors中可能有空的map，如下图中B被删除后，C还在predecessors中有entry但内容为空 +/* +A--->B--->C +| | +| +--->D +| ^ +| | ++---------+ +*/ +func TestMemory_index(t *testing.T) { + ctx := context.Background() + testMemory := NewMemory() + + // test 1: index "A -> B" + testMemory.index(ctx, A, []ocispec.Descriptor{B}) + + // check the memory: A exists in testMemory.predecessors, successors and indexed, + // B ONLY exists in predecessors + _, exists := testMemory.predecessors.Load(AKey) + if !exists { + t.Errorf("could not find the entry of %v in predecessors", A) + } + _, exists = testMemory.successors.Load(AKey) + if !exists { + t.Errorf("could not find the entry of %v in successors", A) + } + _, exists = testMemory.indexed.Load(AKey) + if !exists { + t.Errorf("could not find the entry of %v in indexed", A) + } + _, exists = testMemory.predecessors.Load(BKey) + if !exists { + t.Errorf("could not find the entry of %v in predecessors", B) + } + _, exists = testMemory.successors.Load(BKey) + if exists { + t.Errorf("%v should not exist in successors", B) + } + _, exists = testMemory.indexed.Load(BKey) + if exists { + t.Errorf("%v should not exist in indexed", B) + } + + // predecessors[B] contains A, successors[A] contains B + value, _ := testMemory.predecessors.Load(BKey) + BPreds := value.(*sync.Map) + _, exists = BPreds.Load(AKey) + if !exists { + t.Errorf("could not find %v in predecessors of %v", A, B) + } + value, _ = testMemory.successors.Load(AKey) + ASuccs := value.(*sync.Map) + _, exists = ASuccs.Load(BKey) + if !exists { + t.Errorf("could not find %v in successors of %v", B, A) + } + + // test 2: index "B -> C, B -> D" + testMemory.index(ctx, B, []ocispec.Descriptor{C, D}) + + // check the memory: B exists in testMemory.predecessors, successors and indexed, + // C ONLY exists in predecessors + _, exists = testMemory.predecessors.Load(BKey) + if !exists { + t.Errorf("could not find the entry of %v in predecessors", B) + } + _, exists = testMemory.successors.Load(BKey) + if !exists { + t.Errorf("could not find the entry of %v in successors", B) + } + _, exists = testMemory.indexed.Load(BKey) + if !exists { + t.Errorf("could not find the entry of %v in indexed", B) + } + _, exists = testMemory.predecessors.Load(CKey) + if !exists { + t.Errorf("could not find the entry of %v in predecessors", C) + } + _, exists = testMemory.successors.Load(CKey) + if exists { + t.Errorf("%v should not exist in successors", C) + } + _, exists = testMemory.indexed.Load(CKey) + if exists { + t.Errorf("%v should not exist in indexed", C) + } + + // predecessors[C] contains B, predecessors[D] contains B, + // successors[B] contains C and D + value, _ = testMemory.predecessors.Load(CKey) + CPreds := value.(*sync.Map) + _, exists = CPreds.Load(BKey) + if !exists { + t.Errorf("could not find %v in predecessors of %v", B, C) + } + value, _ = testMemory.predecessors.Load(DKey) + DPreds := value.(*sync.Map) + _, exists = DPreds.Load(BKey) + if !exists { + t.Errorf("could not find %v in predecessors of %v", B, D) + } + value, _ = testMemory.successors.Load(BKey) + BSuccs := value.(*sync.Map) + _, exists = BSuccs.Load(CKey) + if !exists { + t.Errorf("could not find %v in successors of %v", C, B) + } + _, exists = BSuccs.Load(DKey) + if !exists { + t.Errorf("could not find %v in successors of %v", D, B) + } + + // test 3: index "A -> D" + testMemory.index(ctx, A, []ocispec.Descriptor{D}) + + // predecessors[D] contains A and B, successors[A] contains D + value, _ = testMemory.predecessors.Load(DKey) + DPreds = value.(*sync.Map) + _, exists = DPreds.Load(AKey) + if !exists { + t.Errorf("could not find %v in predecessors of %v", A, D) + } + _, exists = DPreds.Load(BKey) + if !exists { + t.Errorf("could not find %v in predecessors of %v", B, D) + } + value, _ = testMemory.successors.Load(AKey) + ASuccs = value.(*sync.Map) + _, exists = ASuccs.Load(DKey) + if !exists { + t.Errorf("could not find %v in successors of %v", D, A) + } + + // check the memory: C and D have not been indexed, so C, D should not + // exist in indexed and successors + _, exists = testMemory.successors.Load(CKey) + if exists { + t.Errorf("%v should not exist in successors", C) + } + _, exists = testMemory.indexed.Load(CKey) + if exists { + t.Errorf("%v should not exist in indexed", C) + } + _, exists = testMemory.successors.Load(DKey) + if exists { + t.Errorf("%v should not exist in successors", D) + } + _, exists = testMemory.indexed.Load(DKey) + if exists { + t.Errorf("%v should not exist in indexed", D) + } + + // test 4: delete B + err := testMemory.RemoveFromIndex(ctx, B) + if err != nil { + t.Errorf("got error when removing %v from index: %v", B, err) + } + + // check the memory: B should NOT exist in predecessors, successors and indexed + _, exists = testMemory.predecessors.Load(BKey) + if exists { + t.Errorf("%v should not exist in predecessors", B) + } + _, exists = testMemory.successors.Load(BKey) + if exists { + t.Errorf("%v should not exist in successors", B) + } + _, exists = testMemory.indexed.Load(BKey) + if exists { + t.Errorf("%v should not exist in indexed", B) + } + + // B should STILL exist in successors[A] + value, _ = testMemory.successors.Load(AKey) + ASuccs = value.(*sync.Map) + _, exists = ASuccs.Load(BKey) + if !exists { + t.Errorf("could not find %v in successors of %v", B, A) + } + + // B should NOT exist in predecessors[C], predecessors[D] + value, _ = testMemory.predecessors.Load(CKey) + CPreds = value.(*sync.Map) + _, exists = CPreds.Load(BKey) + if exists { + t.Errorf("should not find %v in predecessors of %v", B, C) + } + value, _ = testMemory.predecessors.Load(DKey) + DPreds = value.(*sync.Map) + _, exists = DPreds.Load(BKey) + if exists { + t.Errorf("should not find %v in predecessors of %v", B, D) + } + + // A should STILL exist in predecessors[D] + _, exists = DPreds.Load(AKey) + if !exists { + t.Errorf("could not find %v in predecessors of %v", A, D) + } +} From 8bb46b68c629dcab1c1dc0928a9f70be17bcf6eb Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 6 Sep 2023 07:07:26 +0000 Subject: [PATCH 13/26] added another smaller check Signed-off-by: Xiaoxuan Wang --- internal/graph/memory_test.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/internal/graph/memory_test.go b/internal/graph/memory_test.go index a35e0e5b..b88a2dd0 100644 --- a/internal/graph/memory_test.go +++ b/internal/graph/memory_test.go @@ -103,7 +103,7 @@ func TestMemory_index(t *testing.T) { testMemory.index(ctx, B, []ocispec.Descriptor{C, D}) // check the memory: B exists in testMemory.predecessors, successors and indexed, - // C ONLY exists in predecessors + // C, D ONLY exists in predecessors _, exists = testMemory.predecessors.Load(BKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", B) @@ -128,7 +128,18 @@ func TestMemory_index(t *testing.T) { if exists { t.Errorf("%v should not exist in indexed", C) } - + _, exists = testMemory.predecessors.Load(DKey) + if !exists { + t.Errorf("could not find the entry of %v in predecessors", D) + } + _, exists = testMemory.successors.Load(DKey) + if exists { + t.Errorf("%v should not exist in successors", D) + } + _, exists = testMemory.indexed.Load(DKey) + if exists { + t.Errorf("%v should not exist in indexed", D) + } // predecessors[C] contains B, predecessors[D] contains B, // successors[B] contains C and D value, _ = testMemory.predecessors.Load(CKey) @@ -157,7 +168,7 @@ func TestMemory_index(t *testing.T) { // test 3: index "A -> D" testMemory.index(ctx, A, []ocispec.Descriptor{D}) - // predecessors[D] contains A and B, successors[A] contains D + // predecessors[D] contains A and B, successors[A] contains B and D value, _ = testMemory.predecessors.Load(DKey) DPreds = value.(*sync.Map) _, exists = DPreds.Load(AKey) @@ -170,6 +181,10 @@ func TestMemory_index(t *testing.T) { } value, _ = testMemory.successors.Load(AKey) ASuccs = value.(*sync.Map) + _, exists = ASuccs.Load(BKey) + if !exists { + t.Errorf("could not find %v in successors of %v", B, A) + } _, exists = ASuccs.Load(DKey) if !exists { t.Errorf("could not find %v in successors of %v", D, A) From 7381e9b8a3b36ced649f418dd6c49f9a5b56e9b9 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 6 Sep 2023 09:36:44 +0000 Subject: [PATCH 14/26] created memory with delete Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 6 +- content/oci/readonlyoci.go | 19 ++ internal/graph/memory.go | 43 +---- internal/graph/memoryWithDelete.go | 170 ++++++++++++++++++ ...emory_test.go => memoryWithDelete_test.go} | 84 ++++----- 5 files changed, 237 insertions(+), 85 deletions(-) create mode 100644 internal/graph/memoryWithDelete.go rename internal/graph/{memory_test.go => memoryWithDelete_test.go} (69%) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index 316b8ee6..d50a2f54 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -57,7 +57,7 @@ type DeletableStore struct { storage *Storage tagResolver *resolver.Memory - graph *graph.Memory + graph *graph.MemoryWithDelete } // NewDeletableStore returns a new DeletableStore. @@ -82,7 +82,7 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS indexPath: filepath.Join(rootAbs, ociImageIndexFile), storage: storage, tagResolver: resolver.NewMemory(), - graph: graph.NewMemory(), + graph: graph.NewMemoryWithDelete(), } if err := ensureDir(filepath.Join(rootAbs, ociBlobsDir)); err != nil { @@ -294,7 +294,7 @@ func (ds *DeletableStore) loadIndexFile(ctx context.Context) error { return fmt.Errorf("failed to decode index file: %w", err) } ds.index = &index - return loadIndex(ctx, ds.index, ds.storage, ds.tagResolver, ds.graph) + return loadIndexWithMemoryWithDelete(ctx, ds.index, ds.storage, ds.tagResolver, ds.graph) } // SaveIndex writes the `index.json` file to the file system. diff --git a/content/oci/readonlyoci.go b/content/oci/readonlyoci.go index eb94f61c..f9944410 100644 --- a/content/oci/readonlyoci.go +++ b/content/oci/readonlyoci.go @@ -186,6 +186,25 @@ func loadIndex(ctx context.Context, index *ocispec.Index, fetcher content.Fetche return nil } +// loadIndex loads index into memory. +func loadIndexWithMemoryWithDelete(ctx context.Context, index *ocispec.Index, fetcher content.Fetcher, tagger content.Tagger, graph *graph.MemoryWithDelete) error { + for _, desc := range index.Manifests { + if err := tagger.Tag(ctx, deleteAnnotationRefName(desc), desc.Digest.String()); err != nil { + return err + } + if ref := desc.Annotations[ocispec.AnnotationRefName]; ref != "" { + if err := tagger.Tag(ctx, desc, ref); err != nil { + return err + } + } + plain := descriptor.Plain(desc) + if err := graph.IndexAll(ctx, fetcher, plain); err != nil { + return err + } + } + return nil +} + // resolveBlob returns a descriptor describing the blob identified by dgst. func resolveBlob(fsys fs.FS, dgst string) (ocispec.Descriptor, error) { path, err := blobPath(digest.Digest(dgst)) diff --git a/internal/graph/memory.go b/internal/graph/memory.go index f15015b3..fd5a013a 100644 --- a/internal/graph/memory.go +++ b/internal/graph/memory.go @@ -31,7 +31,6 @@ import ( // Memory is a memory based PredecessorFinder. type Memory struct { predecessors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor - successors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor indexed sync.Map // map[descriptor.Descriptor]any } @@ -117,54 +116,18 @@ func (m *Memory) Predecessors(_ context.Context, node ocispec.Descriptor) ([]oci return res, nil } -// RemoveFromIndex removes the node from its predecessors and successors. -func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) error { - nodeKey := descriptor.FromOCI(node) - // remove the node from its successors' predecessor list - value, _ := m.successors.Load(nodeKey) - successors := value.(*sync.Map) - successors.Range(func(key, _ interface{}) bool { - value, _ = m.predecessors.Load(key) - predecessors := value.(*sync.Map) - predecessors.Delete(nodeKey) - return true - }) - m.removeFromMemory(ctx, node) - return nil -} - // index indexes predecessors for each direct successor of the given node. // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. func (m *Memory) index(ctx context.Context, node ocispec.Descriptor, successors []ocispec.Descriptor) { - m.indexIntoMemory(ctx, node) if len(successors) == 0 { return } predecessorKey := descriptor.FromOCI(node) for _, successor := range successors { successorKey := descriptor.FromOCI(successor) - // store in m.predecessors, memory.predecessors[successorKey].Store(node) - pred, _ := m.predecessors.LoadOrStore(successorKey, &sync.Map{}) - predecessorsMap := pred.(*sync.Map) - predecessorsMap.Store(predecessorKey, node) - // store in m.successors, memory.successors[predecessorKey].Store(successor) - succ, _ := m.successors.Load(predecessorKey) - successorsMap := succ.(*sync.Map) - successorsMap.Store(successorKey, successor) + value, _ := m.predecessors.LoadOrStore(successorKey, &sync.Map{}) + predecessors := value.(*sync.Map) + predecessors.Store(predecessorKey, node) } } - -func (m *Memory) indexIntoMemory(ctx context.Context, node ocispec.Descriptor) { - key := descriptor.FromOCI(node) - m.predecessors.LoadOrStore(key, &sync.Map{}) - m.successors.LoadOrStore(key, &sync.Map{}) - m.indexed.LoadOrStore(key, &sync.Map{}) -} - -func (m *Memory) removeFromMemory(ctx context.Context, node ocispec.Descriptor) { - key := descriptor.FromOCI(node) - m.predecessors.Delete(key) - m.successors.Delete(key) - m.indexed.Delete(key) -} diff --git a/internal/graph/memoryWithDelete.go b/internal/graph/memoryWithDelete.go new file mode 100644 index 00000000..b4413bb2 --- /dev/null +++ b/internal/graph/memoryWithDelete.go @@ -0,0 +1,170 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package graph + +import ( + "context" + "errors" + "sync" + + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "oras.land/oras-go/v2/content" + "oras.land/oras-go/v2/errdef" + "oras.land/oras-go/v2/internal/descriptor" + "oras.land/oras-go/v2/internal/status" + "oras.land/oras-go/v2/internal/syncutil" +) + +// MemoryWithDelete is a MemoryWithDelete based PredecessorFinder. +type MemoryWithDelete struct { + predecessors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor + successors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor + indexed sync.Map // map[descriptor.Descriptor]any +} + +// NewMemoryWithDelete creates a new MemoryWithDelete PredecessorFinder. +func NewMemoryWithDelete() *MemoryWithDelete { + return &MemoryWithDelete{} +} + +// Index indexes predecessors for each direct successor of the given node. +// There is no data consistency issue as long as deletion is not implemented +// for the underlying storage. +func (m *MemoryWithDelete) Index(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { + successors, err := content.Successors(ctx, fetcher, node) + if err != nil { + return err + } + + m.index(ctx, node, successors) + return nil +} + +// Index indexes predecessors for all the successors of the given node. +// There is no data consistency issue as long as deletion is not implemented +// for the underlying storage. +func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { + // track content status + tracker := status.NewTracker() + + var fn syncutil.GoFunc[ocispec.Descriptor] + fn = func(ctx context.Context, region *syncutil.LimitedRegion, desc ocispec.Descriptor) error { + // skip the node if other go routine is working on it + _, committed := tracker.TryCommit(desc) + if !committed { + return nil + } + + // skip the node if it has been indexed + key := descriptor.FromOCI(desc) + _, exists := m.indexed.Load(key) + if exists { + return nil + } + + successors, err := content.Successors(ctx, fetcher, desc) + if err != nil { + if errors.Is(err, errdef.ErrNotFound) { + // skip the node if it does not exist + return nil + } + return err + } + m.index(ctx, desc, successors) + m.indexed.Store(key, nil) + + if len(successors) > 0 { + // traverse and index successors + return syncutil.Go(ctx, nil, fn, successors...) + } + return nil + } + return syncutil.Go(ctx, nil, fn, node) +} + +// Predecessors returns the nodes directly pointing to the current node. +// Predecessors returns nil without error if the node does not exists in the +// store. +// Like other operations, calling Predecessors() is go-routine safe. However, +// it does not necessarily correspond to any consistent snapshot of the stored +// contents. +func (m *MemoryWithDelete) Predecessors(_ context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { + key := descriptor.FromOCI(node) + value, exists := m.predecessors.Load(key) + if !exists { + return nil, nil + } + predecessors := value.(*sync.Map) + + var res []ocispec.Descriptor + predecessors.Range(func(key, value interface{}) bool { + res = append(res, value.(ocispec.Descriptor)) + return true + }) + return res, nil +} + +// RemoveFromIndex removes the node from its predecessors and successors. +func (m *MemoryWithDelete) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) error { + nodeKey := descriptor.FromOCI(node) + // remove the node from its successors' predecessor list + value, _ := m.successors.Load(nodeKey) + successors := value.(*sync.Map) + successors.Range(func(key, _ interface{}) bool { + value, _ = m.predecessors.Load(key) + predecessors := value.(*sync.Map) + predecessors.Delete(nodeKey) + return true + }) + m.removeFromMemoryWithDelete(ctx, node) + return nil +} + +// index indexes predecessors for each direct successor of the given node. +// There is no data consistency issue as long as deletion is not implemented +// for the underlying storage. +func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, successors []ocispec.Descriptor) { + m.indexIntoMemoryWithDelete(ctx, node) + if len(successors) == 0 { + return + } + predecessorKey := descriptor.FromOCI(node) + for _, successor := range successors { + successorKey := descriptor.FromOCI(successor) + // store in m.predecessors, MemoryWithDelete.predecessors[successorKey].Store(node) + pred, _ := m.predecessors.LoadOrStore(successorKey, &sync.Map{}) + predecessorsMap := pred.(*sync.Map) + predecessorsMap.Store(predecessorKey, node) + // store in m.successors, MemoryWithDelete.successors[predecessorKey].Store(successor) + succ, _ := m.successors.Load(predecessorKey) + successorsMap := succ.(*sync.Map) + successorsMap.Store(successorKey, successor) + } +} + +func (m *MemoryWithDelete) indexIntoMemoryWithDelete(ctx context.Context, node ocispec.Descriptor) { + key := descriptor.FromOCI(node) + m.predecessors.LoadOrStore(key, &sync.Map{}) + m.successors.LoadOrStore(key, &sync.Map{}) + m.indexed.LoadOrStore(key, &sync.Map{}) +} + +func (m *MemoryWithDelete) removeFromMemoryWithDelete(ctx context.Context, node ocispec.Descriptor) { + key := descriptor.FromOCI(node) + m.predecessors.Delete(key) + m.successors.Delete(key) + m.indexed.Delete(key) +} diff --git a/internal/graph/memory_test.go b/internal/graph/memoryWithDelete_test.go similarity index 69% rename from internal/graph/memory_test.go rename to internal/graph/memoryWithDelete_test.go index b88a2dd0..1097f746 100644 --- a/internal/graph/memory_test.go +++ b/internal/graph/memoryWithDelete_test.go @@ -51,48 +51,48 @@ A--->B--->C | | +---------+ */ -func TestMemory_index(t *testing.T) { +func TestMemoryWithDelete_index(t *testing.T) { ctx := context.Background() - testMemory := NewMemory() + testMemoryWithDelete := NewMemoryWithDelete() // test 1: index "A -> B" - testMemory.index(ctx, A, []ocispec.Descriptor{B}) + testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) - // check the memory: A exists in testMemory.predecessors, successors and indexed, + // check the MemoryWithDelete: A exists in testMemoryWithDelete.predecessors, successors and indexed, // B ONLY exists in predecessors - _, exists := testMemory.predecessors.Load(AKey) + _, exists := testMemoryWithDelete.predecessors.Load(AKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", A) } - _, exists = testMemory.successors.Load(AKey) + _, exists = testMemoryWithDelete.successors.Load(AKey) if !exists { t.Errorf("could not find the entry of %v in successors", A) } - _, exists = testMemory.indexed.Load(AKey) + _, exists = testMemoryWithDelete.indexed.Load(AKey) if !exists { t.Errorf("could not find the entry of %v in indexed", A) } - _, exists = testMemory.predecessors.Load(BKey) + _, exists = testMemoryWithDelete.predecessors.Load(BKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", B) } - _, exists = testMemory.successors.Load(BKey) + _, exists = testMemoryWithDelete.successors.Load(BKey) if exists { t.Errorf("%v should not exist in successors", B) } - _, exists = testMemory.indexed.Load(BKey) + _, exists = testMemoryWithDelete.indexed.Load(BKey) if exists { t.Errorf("%v should not exist in indexed", B) } // predecessors[B] contains A, successors[A] contains B - value, _ := testMemory.predecessors.Load(BKey) + value, _ := testMemoryWithDelete.predecessors.Load(BKey) BPreds := value.(*sync.Map) _, exists = BPreds.Load(AKey) if !exists { t.Errorf("could not find %v in predecessors of %v", A, B) } - value, _ = testMemory.successors.Load(AKey) + value, _ = testMemoryWithDelete.successors.Load(AKey) ASuccs := value.(*sync.Map) _, exists = ASuccs.Load(BKey) if !exists { @@ -100,61 +100,61 @@ func TestMemory_index(t *testing.T) { } // test 2: index "B -> C, B -> D" - testMemory.index(ctx, B, []ocispec.Descriptor{C, D}) + testMemoryWithDelete.index(ctx, B, []ocispec.Descriptor{C, D}) - // check the memory: B exists in testMemory.predecessors, successors and indexed, + // check the MemoryWithDelete: B exists in testMemoryWithDelete.predecessors, successors and indexed, // C, D ONLY exists in predecessors - _, exists = testMemory.predecessors.Load(BKey) + _, exists = testMemoryWithDelete.predecessors.Load(BKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", B) } - _, exists = testMemory.successors.Load(BKey) + _, exists = testMemoryWithDelete.successors.Load(BKey) if !exists { t.Errorf("could not find the entry of %v in successors", B) } - _, exists = testMemory.indexed.Load(BKey) + _, exists = testMemoryWithDelete.indexed.Load(BKey) if !exists { t.Errorf("could not find the entry of %v in indexed", B) } - _, exists = testMemory.predecessors.Load(CKey) + _, exists = testMemoryWithDelete.predecessors.Load(CKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", C) } - _, exists = testMemory.successors.Load(CKey) + _, exists = testMemoryWithDelete.successors.Load(CKey) if exists { t.Errorf("%v should not exist in successors", C) } - _, exists = testMemory.indexed.Load(CKey) + _, exists = testMemoryWithDelete.indexed.Load(CKey) if exists { t.Errorf("%v should not exist in indexed", C) } - _, exists = testMemory.predecessors.Load(DKey) + _, exists = testMemoryWithDelete.predecessors.Load(DKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", D) } - _, exists = testMemory.successors.Load(DKey) + _, exists = testMemoryWithDelete.successors.Load(DKey) if exists { t.Errorf("%v should not exist in successors", D) } - _, exists = testMemory.indexed.Load(DKey) + _, exists = testMemoryWithDelete.indexed.Load(DKey) if exists { t.Errorf("%v should not exist in indexed", D) } // predecessors[C] contains B, predecessors[D] contains B, // successors[B] contains C and D - value, _ = testMemory.predecessors.Load(CKey) + value, _ = testMemoryWithDelete.predecessors.Load(CKey) CPreds := value.(*sync.Map) _, exists = CPreds.Load(BKey) if !exists { t.Errorf("could not find %v in predecessors of %v", B, C) } - value, _ = testMemory.predecessors.Load(DKey) + value, _ = testMemoryWithDelete.predecessors.Load(DKey) DPreds := value.(*sync.Map) _, exists = DPreds.Load(BKey) if !exists { t.Errorf("could not find %v in predecessors of %v", B, D) } - value, _ = testMemory.successors.Load(BKey) + value, _ = testMemoryWithDelete.successors.Load(BKey) BSuccs := value.(*sync.Map) _, exists = BSuccs.Load(CKey) if !exists { @@ -166,10 +166,10 @@ func TestMemory_index(t *testing.T) { } // test 3: index "A -> D" - testMemory.index(ctx, A, []ocispec.Descriptor{D}) + testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{D}) // predecessors[D] contains A and B, successors[A] contains B and D - value, _ = testMemory.predecessors.Load(DKey) + value, _ = testMemoryWithDelete.predecessors.Load(DKey) DPreds = value.(*sync.Map) _, exists = DPreds.Load(AKey) if !exists { @@ -179,7 +179,7 @@ func TestMemory_index(t *testing.T) { if !exists { t.Errorf("could not find %v in predecessors of %v", B, D) } - value, _ = testMemory.successors.Load(AKey) + value, _ = testMemoryWithDelete.successors.Load(AKey) ASuccs = value.(*sync.Map) _, exists = ASuccs.Load(BKey) if !exists { @@ -190,47 +190,47 @@ func TestMemory_index(t *testing.T) { t.Errorf("could not find %v in successors of %v", D, A) } - // check the memory: C and D have not been indexed, so C, D should not + // check the MemoryWithDelete: C and D have not been indexed, so C, D should not // exist in indexed and successors - _, exists = testMemory.successors.Load(CKey) + _, exists = testMemoryWithDelete.successors.Load(CKey) if exists { t.Errorf("%v should not exist in successors", C) } - _, exists = testMemory.indexed.Load(CKey) + _, exists = testMemoryWithDelete.indexed.Load(CKey) if exists { t.Errorf("%v should not exist in indexed", C) } - _, exists = testMemory.successors.Load(DKey) + _, exists = testMemoryWithDelete.successors.Load(DKey) if exists { t.Errorf("%v should not exist in successors", D) } - _, exists = testMemory.indexed.Load(DKey) + _, exists = testMemoryWithDelete.indexed.Load(DKey) if exists { t.Errorf("%v should not exist in indexed", D) } // test 4: delete B - err := testMemory.RemoveFromIndex(ctx, B) + err := testMemoryWithDelete.RemoveFromIndex(ctx, B) if err != nil { t.Errorf("got error when removing %v from index: %v", B, err) } - // check the memory: B should NOT exist in predecessors, successors and indexed - _, exists = testMemory.predecessors.Load(BKey) + // check the MemoryWithDelete: B should NOT exist in predecessors, successors and indexed + _, exists = testMemoryWithDelete.predecessors.Load(BKey) if exists { t.Errorf("%v should not exist in predecessors", B) } - _, exists = testMemory.successors.Load(BKey) + _, exists = testMemoryWithDelete.successors.Load(BKey) if exists { t.Errorf("%v should not exist in successors", B) } - _, exists = testMemory.indexed.Load(BKey) + _, exists = testMemoryWithDelete.indexed.Load(BKey) if exists { t.Errorf("%v should not exist in indexed", B) } // B should STILL exist in successors[A] - value, _ = testMemory.successors.Load(AKey) + value, _ = testMemoryWithDelete.successors.Load(AKey) ASuccs = value.(*sync.Map) _, exists = ASuccs.Load(BKey) if !exists { @@ -238,13 +238,13 @@ func TestMemory_index(t *testing.T) { } // B should NOT exist in predecessors[C], predecessors[D] - value, _ = testMemory.predecessors.Load(CKey) + value, _ = testMemoryWithDelete.predecessors.Load(CKey) CPreds = value.(*sync.Map) _, exists = CPreds.Load(BKey) if exists { t.Errorf("should not find %v in predecessors of %v", B, C) } - value, _ = testMemory.predecessors.Load(DKey) + value, _ = testMemoryWithDelete.predecessors.Load(DKey) DPreds = value.(*sync.Map) _, exists = DPreds.Load(BKey) if exists { From 589934f8956776b386d9c72467dd65477b06100d Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 7 Sep 2023 05:35:40 +0000 Subject: [PATCH 15/26] added lock, need to change sync map to regular map Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 2 +- internal/graph/memoryWithDelete.go | 20 ++++++----- internal/graph/memoryWithDelete_test.go | 48 +++++-------------------- 3 files changed, 22 insertions(+), 48 deletions(-) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index d50a2f54..d6641ea5 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -132,7 +132,7 @@ func (ds *DeletableStore) Delete(ctx context.Context, target ocispec.Descriptor) ds.tagResolver.Delete(reference) } } - if err := ds.graph.RemoveFromIndex(ctx, target); err != nil { + if err := ds.graph.Remove(ctx, target); err != nil { return err } if ds.AutoSaveIndex { diff --git a/internal/graph/memoryWithDelete.go b/internal/graph/memoryWithDelete.go index b4413bb2..911ae7d3 100644 --- a/internal/graph/memoryWithDelete.go +++ b/internal/graph/memoryWithDelete.go @@ -30,9 +30,10 @@ import ( // MemoryWithDelete is a MemoryWithDelete based PredecessorFinder. type MemoryWithDelete struct { + indexed sync.Map // map[descriptor.Descriptor]any predecessors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor successors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor - indexed sync.Map // map[descriptor.Descriptor]any + lock sync.Mutex } // NewMemoryWithDelete creates a new MemoryWithDelete PredecessorFinder. @@ -117,8 +118,10 @@ func (m *MemoryWithDelete) Predecessors(_ context.Context, node ocispec.Descript return res, nil } -// RemoveFromIndex removes the node from its predecessors and successors. -func (m *MemoryWithDelete) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) error { +// Remove removes the node from its predecessors and successors. +func (m *MemoryWithDelete) Remove(ctx context.Context, node ocispec.Descriptor) error { + m.lock.Lock() + defer m.lock.Unlock() nodeKey := descriptor.FromOCI(node) // remove the node from its successors' predecessor list value, _ := m.successors.Load(nodeKey) @@ -129,7 +132,7 @@ func (m *MemoryWithDelete) RemoveFromIndex(ctx context.Context, node ocispec.Des predecessors.Delete(nodeKey) return true }) - m.removeFromMemoryWithDelete(ctx, node) + m.removeEntriesFromMaps(ctx, node) return nil } @@ -137,7 +140,9 @@ func (m *MemoryWithDelete) RemoveFromIndex(ctx context.Context, node ocispec.Des // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, successors []ocispec.Descriptor) { - m.indexIntoMemoryWithDelete(ctx, node) + m.lock.Lock() + defer m.lock.Unlock() + m.createEntriesInMaps(ctx, node) if len(successors) == 0 { return } @@ -155,14 +160,13 @@ func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, s } } -func (m *MemoryWithDelete) indexIntoMemoryWithDelete(ctx context.Context, node ocispec.Descriptor) { +func (m *MemoryWithDelete) createEntriesInMaps(ctx context.Context, node ocispec.Descriptor) { key := descriptor.FromOCI(node) m.predecessors.LoadOrStore(key, &sync.Map{}) m.successors.LoadOrStore(key, &sync.Map{}) - m.indexed.LoadOrStore(key, &sync.Map{}) } -func (m *MemoryWithDelete) removeFromMemoryWithDelete(ctx context.Context, node ocispec.Descriptor) { +func (m *MemoryWithDelete) removeEntriesFromMaps(ctx context.Context, node ocispec.Descriptor) { key := descriptor.FromOCI(node) m.predecessors.Delete(key) m.successors.Delete(key) diff --git a/internal/graph/memoryWithDelete_test.go b/internal/graph/memoryWithDelete_test.go index 1097f746..5195c58c 100644 --- a/internal/graph/memoryWithDelete_test.go +++ b/internal/graph/memoryWithDelete_test.go @@ -35,11 +35,13 @@ var ( DKey = descriptor.FromOCI(D) ) -// 条件： -// 1. 只要node被index（作为函数的第二个输入），node就在predecessors，successors，indexed中有entry -// 2. 只要node被Removed from index，node就在predecessors，successors，indexed中都没有entry +// 当前条件（后续可能改动）： +// 1. 只要node被index（作为函数的第二个输入），node就在predecessors，successors中都有entry +// 2. 只要node被Removed from index，node就在predecessors，successors中都没有entry // 3. 只有node被index（作为函数的第二个输入），successors中才有其entry // 4. successors中的内容与node中的内容一致，只要entry还在，其内容就不会改变 +// 5. 变量indexed的作用和行为和原始版本一样，没有加以变动 +// 6. 删除一个node的时候，indexed里面的entry也会删除。 // GC情况： // predecessors中可能有空的map，如下图中B被删除后，C还在predecessors中有entry但内容为空 @@ -51,14 +53,14 @@ A--->B--->C | | +---------+ */ -func TestMemoryWithDelete_index(t *testing.T) { +func TestMemoryWithDelete_indexAndDelete(t *testing.T) { ctx := context.Background() testMemoryWithDelete := NewMemoryWithDelete() // test 1: index "A -> B" testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) - // check the MemoryWithDelete: A exists in testMemoryWithDelete.predecessors, successors and indexed, + // check the MemoryWithDelete: A exists in testMemoryWithDelete.predecessors and successors // B ONLY exists in predecessors _, exists := testMemoryWithDelete.predecessors.Load(AKey) if !exists { @@ -68,10 +70,6 @@ func TestMemoryWithDelete_index(t *testing.T) { if !exists { t.Errorf("could not find the entry of %v in successors", A) } - _, exists = testMemoryWithDelete.indexed.Load(AKey) - if !exists { - t.Errorf("could not find the entry of %v in indexed", A) - } _, exists = testMemoryWithDelete.predecessors.Load(BKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", B) @@ -80,10 +78,6 @@ func TestMemoryWithDelete_index(t *testing.T) { if exists { t.Errorf("%v should not exist in successors", B) } - _, exists = testMemoryWithDelete.indexed.Load(BKey) - if exists { - t.Errorf("%v should not exist in indexed", B) - } // predecessors[B] contains A, successors[A] contains B value, _ := testMemoryWithDelete.predecessors.Load(BKey) @@ -112,10 +106,6 @@ func TestMemoryWithDelete_index(t *testing.T) { if !exists { t.Errorf("could not find the entry of %v in successors", B) } - _, exists = testMemoryWithDelete.indexed.Load(BKey) - if !exists { - t.Errorf("could not find the entry of %v in indexed", B) - } _, exists = testMemoryWithDelete.predecessors.Load(CKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", C) @@ -124,10 +114,6 @@ func TestMemoryWithDelete_index(t *testing.T) { if exists { t.Errorf("%v should not exist in successors", C) } - _, exists = testMemoryWithDelete.indexed.Load(CKey) - if exists { - t.Errorf("%v should not exist in indexed", C) - } _, exists = testMemoryWithDelete.predecessors.Load(DKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", D) @@ -136,10 +122,6 @@ func TestMemoryWithDelete_index(t *testing.T) { if exists { t.Errorf("%v should not exist in successors", D) } - _, exists = testMemoryWithDelete.indexed.Load(DKey) - if exists { - t.Errorf("%v should not exist in indexed", D) - } // predecessors[C] contains B, predecessors[D] contains B, // successors[B] contains C and D value, _ = testMemoryWithDelete.predecessors.Load(CKey) @@ -191,26 +173,18 @@ func TestMemoryWithDelete_index(t *testing.T) { } // check the MemoryWithDelete: C and D have not been indexed, so C, D should not - // exist in indexed and successors + // exist in successors _, exists = testMemoryWithDelete.successors.Load(CKey) if exists { t.Errorf("%v should not exist in successors", C) } - _, exists = testMemoryWithDelete.indexed.Load(CKey) - if exists { - t.Errorf("%v should not exist in indexed", C) - } _, exists = testMemoryWithDelete.successors.Load(DKey) if exists { t.Errorf("%v should not exist in successors", D) } - _, exists = testMemoryWithDelete.indexed.Load(DKey) - if exists { - t.Errorf("%v should not exist in indexed", D) - } // test 4: delete B - err := testMemoryWithDelete.RemoveFromIndex(ctx, B) + err := testMemoryWithDelete.Remove(ctx, B) if err != nil { t.Errorf("got error when removing %v from index: %v", B, err) } @@ -224,10 +198,6 @@ func TestMemoryWithDelete_index(t *testing.T) { if exists { t.Errorf("%v should not exist in successors", B) } - _, exists = testMemoryWithDelete.indexed.Load(BKey) - if exists { - t.Errorf("%v should not exist in indexed", B) - } // B should STILL exist in successors[A] value, _ = testMemoryWithDelete.successors.Load(AKey) From ed49d71fb7cd2fb9410949d0310e20593b64967a Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 7 Sep 2023 07:47:45 +0000 Subject: [PATCH 16/26] changed successors to regular maps, updated tests Signed-off-by: Xiaoxuan Wang --- internal/graph/memoryWithDelete.go | 30 +++++++-------- internal/graph/memoryWithDelete_test.go | 49 +++++++++++-------------- 2 files changed, 35 insertions(+), 44 deletions(-) diff --git a/internal/graph/memoryWithDelete.go b/internal/graph/memoryWithDelete.go index 911ae7d3..22d24561 100644 --- a/internal/graph/memoryWithDelete.go +++ b/internal/graph/memoryWithDelete.go @@ -30,15 +30,17 @@ import ( // MemoryWithDelete is a MemoryWithDelete based PredecessorFinder. type MemoryWithDelete struct { - indexed sync.Map // map[descriptor.Descriptor]any + indexed sync.Map // map[descriptor.Descriptor]any, this variable is only used by IndexAll predecessors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor - successors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor + successors map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor lock sync.Mutex } // NewMemoryWithDelete creates a new MemoryWithDelete PredecessorFinder. func NewMemoryWithDelete() *MemoryWithDelete { - return &MemoryWithDelete{} + return &MemoryWithDelete{ + successors: make(map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor), + } } // Index indexes predecessors for each direct successor of the given node. @@ -124,14 +126,11 @@ func (m *MemoryWithDelete) Remove(ctx context.Context, node ocispec.Descriptor) defer m.lock.Unlock() nodeKey := descriptor.FromOCI(node) // remove the node from its successors' predecessor list - value, _ := m.successors.Load(nodeKey) - successors := value.(*sync.Map) - successors.Range(func(key, _ interface{}) bool { - value, _ = m.predecessors.Load(key) + for successorKey, _ := range m.successors[nodeKey] { + value, _ := m.predecessors.Load(successorKey) predecessors := value.(*sync.Map) predecessors.Delete(nodeKey) - return true - }) + } m.removeEntriesFromMaps(ctx, node) return nil } @@ -154,21 +153,20 @@ func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, s predecessorsMap := pred.(*sync.Map) predecessorsMap.Store(predecessorKey, node) // store in m.successors, MemoryWithDelete.successors[predecessorKey].Store(successor) - succ, _ := m.successors.Load(predecessorKey) - successorsMap := succ.(*sync.Map) - successorsMap.Store(successorKey, successor) + m.successors[predecessorKey][successorKey] = successor } } func (m *MemoryWithDelete) createEntriesInMaps(ctx context.Context, node ocispec.Descriptor) { key := descriptor.FromOCI(node) m.predecessors.LoadOrStore(key, &sync.Map{}) - m.successors.LoadOrStore(key, &sync.Map{}) + if _, hasEntry := m.successors[key]; !hasEntry { + m.successors[key] = make(map[descriptor.Descriptor]ocispec.Descriptor) + } } func (m *MemoryWithDelete) removeEntriesFromMaps(ctx context.Context, node ocispec.Descriptor) { key := descriptor.FromOCI(node) - m.predecessors.Delete(key) - m.successors.Delete(key) - m.indexed.Delete(key) + delete(m.successors, key) + m.indexed.Delete(key) // pending } diff --git a/internal/graph/memoryWithDelete_test.go b/internal/graph/memoryWithDelete_test.go index 5195c58c..c0096a4a 100644 --- a/internal/graph/memoryWithDelete_test.go +++ b/internal/graph/memoryWithDelete_test.go @@ -60,13 +60,13 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { // test 1: index "A -> B" testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) - // check the MemoryWithDelete: A exists in testMemoryWithDelete.predecessors and successors + // check the memory: A exists in predecessors and successors // B ONLY exists in predecessors _, exists := testMemoryWithDelete.predecessors.Load(AKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", A) } - _, exists = testMemoryWithDelete.successors.Load(AKey) + _, exists = testMemoryWithDelete.successors[AKey] if !exists { t.Errorf("could not find the entry of %v in successors", A) } @@ -74,7 +74,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find the entry of %v in predecessors", B) } - _, exists = testMemoryWithDelete.successors.Load(BKey) + _, exists = testMemoryWithDelete.successors[BKey] if exists { t.Errorf("%v should not exist in successors", B) } @@ -86,9 +86,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find %v in predecessors of %v", A, B) } - value, _ = testMemoryWithDelete.successors.Load(AKey) - ASuccs := value.(*sync.Map) - _, exists = ASuccs.Load(BKey) + _, exists = testMemoryWithDelete.successors[AKey][BKey] if !exists { t.Errorf("could not find %v in successors of %v", B, A) } @@ -96,13 +94,13 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { // test 2: index "B -> C, B -> D" testMemoryWithDelete.index(ctx, B, []ocispec.Descriptor{C, D}) - // check the MemoryWithDelete: B exists in testMemoryWithDelete.predecessors, successors and indexed, + // check the memory: B exists in predecessors and successors, // C, D ONLY exists in predecessors _, exists = testMemoryWithDelete.predecessors.Load(BKey) if !exists { t.Errorf("could not find the entry of %v in predecessors", B) } - _, exists = testMemoryWithDelete.successors.Load(BKey) + _, exists = testMemoryWithDelete.successors[BKey] if !exists { t.Errorf("could not find the entry of %v in successors", B) } @@ -110,7 +108,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find the entry of %v in predecessors", C) } - _, exists = testMemoryWithDelete.successors.Load(CKey) + _, exists = testMemoryWithDelete.successors[CKey] if exists { t.Errorf("%v should not exist in successors", C) } @@ -118,7 +116,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find the entry of %v in predecessors", D) } - _, exists = testMemoryWithDelete.successors.Load(DKey) + _, exists = testMemoryWithDelete.successors[DKey] if exists { t.Errorf("%v should not exist in successors", D) } @@ -136,13 +134,11 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find %v in predecessors of %v", B, D) } - value, _ = testMemoryWithDelete.successors.Load(BKey) - BSuccs := value.(*sync.Map) - _, exists = BSuccs.Load(CKey) + _, exists = testMemoryWithDelete.successors[BKey][CKey] if !exists { t.Errorf("could not find %v in successors of %v", C, B) } - _, exists = BSuccs.Load(DKey) + _, exists = testMemoryWithDelete.successors[BKey][DKey] if !exists { t.Errorf("could not find %v in successors of %v", D, B) } @@ -161,24 +157,22 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find %v in predecessors of %v", B, D) } - value, _ = testMemoryWithDelete.successors.Load(AKey) - ASuccs = value.(*sync.Map) - _, exists = ASuccs.Load(BKey) + _, exists = testMemoryWithDelete.successors[AKey][BKey] if !exists { t.Errorf("could not find %v in successors of %v", B, A) } - _, exists = ASuccs.Load(DKey) + _, exists = testMemoryWithDelete.successors[AKey][DKey] if !exists { t.Errorf("could not find %v in successors of %v", D, A) } - // check the MemoryWithDelete: C and D have not been indexed, so C, D should not + // check the memory: C and D have not been indexed, so C, D should not // exist in successors - _, exists = testMemoryWithDelete.successors.Load(CKey) + _, exists = testMemoryWithDelete.successors[CKey] if exists { t.Errorf("%v should not exist in successors", C) } - _, exists = testMemoryWithDelete.successors.Load(DKey) + _, exists = testMemoryWithDelete.successors[DKey] if exists { t.Errorf("%v should not exist in successors", D) } @@ -189,20 +183,19 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { t.Errorf("got error when removing %v from index: %v", B, err) } - // check the MemoryWithDelete: B should NOT exist in predecessors, successors and indexed + // check the memory: B should NOT exist in successors, should still exist + // in predecessors _, exists = testMemoryWithDelete.predecessors.Load(BKey) - if exists { - t.Errorf("%v should not exist in predecessors", B) + if !exists { + t.Errorf("%v should exist in predecessors", B) } - _, exists = testMemoryWithDelete.successors.Load(BKey) + _, exists = testMemoryWithDelete.successors[BKey] if exists { t.Errorf("%v should not exist in successors", B) } // B should STILL exist in successors[A] - value, _ = testMemoryWithDelete.successors.Load(AKey) - ASuccs = value.(*sync.Map) - _, exists = ASuccs.Load(BKey) + _, exists = testMemoryWithDelete.successors[AKey][BKey] if !exists { t.Errorf("could not find %v in successors of %v", B, A) } From 1044252b29461f7c45535986d989571068edfca3 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 7 Sep 2023 09:22:11 +0000 Subject: [PATCH 17/26] changed predecessors to regular map Signed-off-by: Xiaoxuan Wang --- internal/graph/memoryWithDelete.go | 33 +++++----- internal/graph/memoryWithDelete_test.go | 86 +++++++++++++++---------- 2 files changed, 69 insertions(+), 50 deletions(-) diff --git a/internal/graph/memoryWithDelete.go b/internal/graph/memoryWithDelete.go index 22d24561..84b69428 100644 --- a/internal/graph/memoryWithDelete.go +++ b/internal/graph/memoryWithDelete.go @@ -31,7 +31,7 @@ import ( // MemoryWithDelete is a MemoryWithDelete based PredecessorFinder. type MemoryWithDelete struct { indexed sync.Map // map[descriptor.Descriptor]any, this variable is only used by IndexAll - predecessors sync.Map // map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor + predecessors map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor successors map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor lock sync.Mutex } @@ -39,7 +39,8 @@ type MemoryWithDelete struct { // NewMemoryWithDelete creates a new MemoryWithDelete PredecessorFinder. func NewMemoryWithDelete() *MemoryWithDelete { return &MemoryWithDelete{ - successors: make(map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor), + predecessors: make(map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor), + successors: make(map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor), } } @@ -106,17 +107,14 @@ func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher // contents. func (m *MemoryWithDelete) Predecessors(_ context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { key := descriptor.FromOCI(node) - value, exists := m.predecessors.Load(key) + _, exists := m.predecessors[key] if !exists { return nil, nil } - predecessors := value.(*sync.Map) - var res []ocispec.Descriptor - predecessors.Range(func(key, value interface{}) bool { - res = append(res, value.(ocispec.Descriptor)) - return true - }) + for _, v := range m.predecessors[key] { + res = append(res, v) + } return res, nil } @@ -126,10 +124,8 @@ func (m *MemoryWithDelete) Remove(ctx context.Context, node ocispec.Descriptor) defer m.lock.Unlock() nodeKey := descriptor.FromOCI(node) // remove the node from its successors' predecessor list - for successorKey, _ := range m.successors[nodeKey] { - value, _ := m.predecessors.Load(successorKey) - predecessors := value.(*sync.Map) - predecessors.Delete(nodeKey) + for successorKey := range m.successors[nodeKey] { + delete(m.predecessors[successorKey], nodeKey) } m.removeEntriesFromMaps(ctx, node) return nil @@ -149,9 +145,10 @@ func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, s for _, successor := range successors { successorKey := descriptor.FromOCI(successor) // store in m.predecessors, MemoryWithDelete.predecessors[successorKey].Store(node) - pred, _ := m.predecessors.LoadOrStore(successorKey, &sync.Map{}) - predecessorsMap := pred.(*sync.Map) - predecessorsMap.Store(predecessorKey, node) + if _, exists := m.predecessors[successorKey]; !exists { + m.predecessors[successorKey] = make(map[descriptor.Descriptor]ocispec.Descriptor) + } + m.predecessors[successorKey][predecessorKey] = node // store in m.successors, MemoryWithDelete.successors[predecessorKey].Store(successor) m.successors[predecessorKey][successorKey] = successor } @@ -159,7 +156,9 @@ func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, s func (m *MemoryWithDelete) createEntriesInMaps(ctx context.Context, node ocispec.Descriptor) { key := descriptor.FromOCI(node) - m.predecessors.LoadOrStore(key, &sync.Map{}) + if _, hasEntry := m.predecessors[key]; !hasEntry { + m.predecessors[key] = make(map[descriptor.Descriptor]ocispec.Descriptor) + } if _, hasEntry := m.successors[key]; !hasEntry { m.successors[key] = make(map[descriptor.Descriptor]ocispec.Descriptor) } diff --git a/internal/graph/memoryWithDelete_test.go b/internal/graph/memoryWithDelete_test.go index c0096a4a..cf2595b3 100644 --- a/internal/graph/memoryWithDelete_test.go +++ b/internal/graph/memoryWithDelete_test.go @@ -17,7 +17,6 @@ package graph import ( "context" - "sync" "testing" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -35,17 +34,27 @@ var ( DKey = descriptor.FromOCI(D) ) +// TODO: +// IndexAll和Delete，会不会出现race condition？ +// IndexAll和Index同时调用，会不会出现race condition？ + // 当前条件（后续可能改动）： -// 1. 只要node被index（作为函数的第二个输入），node就在predecessors，successors中都有entry -// 2. 只要node被Removed from index，node就在predecessors，successors中都没有entry -// 3. 只有node被index（作为函数的第二个输入），successors中才有其entry -// 4. successors中的内容与node中的内容一致，只要entry还在，其内容就不会改变 -// 5. 变量indexed的作用和行为和原始版本一样，没有加以变动 -// 6. 删除一个node的时候，indexed里面的entry也会删除。 +// * 变量indexed的作用和行为和原始版本一样，没有加以变动 +// * 只要node被index（作为函数的第二个输入），node就在predecessors，successors中都有entry +// * 只有node被index（作为函数的第二个输入），successors中才有其entry +// * successors中的内容与node中的内容一致，只要entry还在，其内容就不会改变 +// * 删除一个node的时候，indexed和successors里面的entry也会删除。predecessors中的entry不会被删除 // GC情况： // predecessors中可能有空的map，如下图中B被删除后，C还在predecessors中有entry但内容为空 /* + +test 1: index "A -> B" +test 2: index "B -> C, B -> D" +test 3: index "A -> D" +test 4: delete B +test 5: delete A + A--->B--->C | | | +--->D @@ -62,7 +71,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { // check the memory: A exists in predecessors and successors // B ONLY exists in predecessors - _, exists := testMemoryWithDelete.predecessors.Load(AKey) + _, exists := testMemoryWithDelete.predecessors[AKey] if !exists { t.Errorf("could not find the entry of %v in predecessors", A) } @@ -70,7 +79,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find the entry of %v in successors", A) } - _, exists = testMemoryWithDelete.predecessors.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[BKey] if !exists { t.Errorf("could not find the entry of %v in predecessors", B) } @@ -80,9 +89,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { } // predecessors[B] contains A, successors[A] contains B - value, _ := testMemoryWithDelete.predecessors.Load(BKey) - BPreds := value.(*sync.Map) - _, exists = BPreds.Load(AKey) + _, exists = testMemoryWithDelete.predecessors[BKey][AKey] if !exists { t.Errorf("could not find %v in predecessors of %v", A, B) } @@ -96,7 +103,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { // check the memory: B exists in predecessors and successors, // C, D ONLY exists in predecessors - _, exists = testMemoryWithDelete.predecessors.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[BKey] if !exists { t.Errorf("could not find the entry of %v in predecessors", B) } @@ -104,7 +111,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if !exists { t.Errorf("could not find the entry of %v in successors", B) } - _, exists = testMemoryWithDelete.predecessors.Load(CKey) + _, exists = testMemoryWithDelete.predecessors[CKey] if !exists { t.Errorf("could not find the entry of %v in predecessors", C) } @@ -112,7 +119,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { if exists { t.Errorf("%v should not exist in successors", C) } - _, exists = testMemoryWithDelete.predecessors.Load(DKey) + _, exists = testMemoryWithDelete.predecessors[DKey] if !exists { t.Errorf("could not find the entry of %v in predecessors", D) } @@ -122,15 +129,11 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { } // predecessors[C] contains B, predecessors[D] contains B, // successors[B] contains C and D - value, _ = testMemoryWithDelete.predecessors.Load(CKey) - CPreds := value.(*sync.Map) - _, exists = CPreds.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[CKey][BKey] if !exists { t.Errorf("could not find %v in predecessors of %v", B, C) } - value, _ = testMemoryWithDelete.predecessors.Load(DKey) - DPreds := value.(*sync.Map) - _, exists = DPreds.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[DKey][BKey] if !exists { t.Errorf("could not find %v in predecessors of %v", B, D) } @@ -147,13 +150,11 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{D}) // predecessors[D] contains A and B, successors[A] contains B and D - value, _ = testMemoryWithDelete.predecessors.Load(DKey) - DPreds = value.(*sync.Map) - _, exists = DPreds.Load(AKey) + _, exists = testMemoryWithDelete.predecessors[DKey][AKey] if !exists { t.Errorf("could not find %v in predecessors of %v", A, D) } - _, exists = DPreds.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[DKey][BKey] if !exists { t.Errorf("could not find %v in predecessors of %v", B, D) } @@ -185,7 +186,7 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { // check the memory: B should NOT exist in successors, should still exist // in predecessors - _, exists = testMemoryWithDelete.predecessors.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[BKey] if !exists { t.Errorf("%v should exist in predecessors", B) } @@ -201,22 +202,41 @@ func TestMemoryWithDelete_indexAndDelete(t *testing.T) { } // B should NOT exist in predecessors[C], predecessors[D] - value, _ = testMemoryWithDelete.predecessors.Load(CKey) - CPreds = value.(*sync.Map) - _, exists = CPreds.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[CKey][BKey] if exists { t.Errorf("should not find %v in predecessors of %v", B, C) } - value, _ = testMemoryWithDelete.predecessors.Load(DKey) - DPreds = value.(*sync.Map) - _, exists = DPreds.Load(BKey) + _, exists = testMemoryWithDelete.predecessors[DKey][BKey] if exists { t.Errorf("should not find %v in predecessors of %v", B, D) } // A should STILL exist in predecessors[D] - _, exists = DPreds.Load(AKey) + _, exists = testMemoryWithDelete.predecessors[DKey][AKey] if !exists { t.Errorf("could not find %v in predecessors of %v", A, D) } + + // test 5: delete A + err = testMemoryWithDelete.Remove(ctx, A) + if err != nil { + t.Errorf("got error when removing %v from index: %v", A, err) + } + + // check the memory: A should NOT exist in successors, should still exist + // in predecessors + _, exists = testMemoryWithDelete.predecessors[AKey] + if !exists { + t.Errorf("%v should exist in predecessors", A) + } + _, exists = testMemoryWithDelete.successors[AKey] + if exists { + t.Errorf("%v should not exist in successors", A) + } + + // A should NOT exist in predecessors[D] + _, exists = testMemoryWithDelete.predecessors[DKey][AKey] + if exists { + t.Errorf("should not find %v in predecessors of %v", A, D) + } } From 4c5457d20e479b7a4efe95e1e020c97216f36822 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 7 Sep 2023 09:51:04 +0000 Subject: [PATCH 18/26] changed the places of lock operations Signed-off-by: Xiaoxuan Wang --- internal/graph/memory.go | 1 + internal/graph/memoryWithDelete.go | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/graph/memory.go b/internal/graph/memory.go index fd5a013a..0aa25aee 100644 --- a/internal/graph/memory.go +++ b/internal/graph/memory.go @@ -123,6 +123,7 @@ func (m *Memory) index(ctx context.Context, node ocispec.Descriptor, successors if len(successors) == 0 { return } + predecessorKey := descriptor.FromOCI(node) for _, successor := range successors { successorKey := descriptor.FromOCI(successor) diff --git a/internal/graph/memoryWithDelete.go b/internal/graph/memoryWithDelete.go index 84b69428..02c190cd 100644 --- a/internal/graph/memoryWithDelete.go +++ b/internal/graph/memoryWithDelete.go @@ -53,6 +53,8 @@ func (m *MemoryWithDelete) Index(ctx context.Context, fetcher content.Fetcher, n return err } + m.lock.Lock() + defer m.lock.Unlock() m.index(ctx, node, successors) return nil } @@ -96,6 +98,8 @@ func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher } return nil } + m.lock.Lock() + defer m.lock.Unlock() return syncutil.Go(ctx, nil, fn, node) } @@ -120,9 +124,9 @@ func (m *MemoryWithDelete) Predecessors(_ context.Context, node ocispec.Descript // Remove removes the node from its predecessors and successors. func (m *MemoryWithDelete) Remove(ctx context.Context, node ocispec.Descriptor) error { + nodeKey := descriptor.FromOCI(node) m.lock.Lock() defer m.lock.Unlock() - nodeKey := descriptor.FromOCI(node) // remove the node from its successors' predecessor list for successorKey := range m.successors[nodeKey] { delete(m.predecessors[successorKey], nodeKey) @@ -135,8 +139,6 @@ func (m *MemoryWithDelete) Remove(ctx context.Context, node ocispec.Descriptor) // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, successors []ocispec.Descriptor) { - m.lock.Lock() - defer m.lock.Unlock() m.createEntriesInMaps(ctx, node) if len(successors) == 0 { return From ed0e652895643c79527e810e355be099d11dc076 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Tue, 12 Sep 2023 07:49:46 +0000 Subject: [PATCH 19/26] per discussion of yesterday Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 4 +- content/oci/readonlyoci.go | 2 +- internal/container/set/set.go | 5 + ...memoryWithDelete.go => DeletableMemory.go} | 118 ++++----- internal/graph/DeletableMemory_test.go | 239 +++++++++++++++++ internal/graph/memoryWithDelete_test.go | 242 ------------------ 6 files changed, 297 insertions(+), 313 deletions(-) rename internal/graph/{memoryWithDelete.go => DeletableMemory.go} (50%) create mode 100644 internal/graph/DeletableMemory_test.go delete mode 100644 internal/graph/memoryWithDelete_test.go diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index d6641ea5..375103bc 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -57,7 +57,7 @@ type DeletableStore struct { storage *Storage tagResolver *resolver.Memory - graph *graph.MemoryWithDelete + graph *graph.DeletableMemory } // NewDeletableStore returns a new DeletableStore. @@ -82,7 +82,7 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS indexPath: filepath.Join(rootAbs, ociImageIndexFile), storage: storage, tagResolver: resolver.NewMemory(), - graph: graph.NewMemoryWithDelete(), + graph: graph.NewDeletableMemory(), } if err := ensureDir(filepath.Join(rootAbs, ociBlobsDir)); err != nil { diff --git a/content/oci/readonlyoci.go b/content/oci/readonlyoci.go index f9944410..8dd33b3b 100644 --- a/content/oci/readonlyoci.go +++ b/content/oci/readonlyoci.go @@ -187,7 +187,7 @@ func loadIndex(ctx context.Context, index *ocispec.Index, fetcher content.Fetche } // loadIndex loads index into memory. -func loadIndexWithMemoryWithDelete(ctx context.Context, index *ocispec.Index, fetcher content.Fetcher, tagger content.Tagger, graph *graph.MemoryWithDelete) error { +func loadIndexWithMemoryWithDelete(ctx context.Context, index *ocispec.Index, fetcher content.Fetcher, tagger content.Tagger, graph *graph.DeletableMemory) error { for _, desc := range index.Manifests { if err := tagger.Tag(ctx, deleteAnnotationRefName(desc), desc.Digest.String()); err != nil { return err diff --git a/internal/container/set/set.go b/internal/container/set/set.go index a084e288..1a5f9016 100644 --- a/internal/container/set/set.go +++ b/internal/container/set/set.go @@ -33,3 +33,8 @@ func (s Set[T]) Contains(item T) bool { _, ok := s[item] return ok } + +// Delete deletes an item from the set +func (s Set[T]) Delete(item T) { + delete(s, item) +} diff --git a/internal/graph/memoryWithDelete.go b/internal/graph/DeletableMemory.go similarity index 50% rename from internal/graph/memoryWithDelete.go rename to internal/graph/DeletableMemory.go index 02c190cd..6ef42dad 100644 --- a/internal/graph/memoryWithDelete.go +++ b/internal/graph/DeletableMemory.go @@ -23,46 +23,41 @@ import ( ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2/content" "oras.land/oras-go/v2/errdef" + "oras.land/oras-go/v2/internal/container/set" "oras.land/oras-go/v2/internal/descriptor" "oras.land/oras-go/v2/internal/status" "oras.land/oras-go/v2/internal/syncutil" ) -// MemoryWithDelete is a MemoryWithDelete based PredecessorFinder. -type MemoryWithDelete struct { - indexed sync.Map // map[descriptor.Descriptor]any, this variable is only used by IndexAll - predecessors map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor - successors map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor - lock sync.Mutex +// DeletableMemory is a DeletableMemory based PredecessorFinder. +type DeletableMemory struct { + nodes map[descriptor.Descriptor]ocispec.Descriptor // nodes saves the map keys of ocispec.Descriptor + predecessors map[descriptor.Descriptor]set.Set[descriptor.Descriptor] + successors map[descriptor.Descriptor]set.Set[descriptor.Descriptor] + lock sync.RWMutex } -// NewMemoryWithDelete creates a new MemoryWithDelete PredecessorFinder. -func NewMemoryWithDelete() *MemoryWithDelete { - return &MemoryWithDelete{ - predecessors: make(map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor), - successors: make(map[descriptor.Descriptor]map[descriptor.Descriptor]ocispec.Descriptor), +// NewDeletableMemory creates a new DeletableMemory. +func NewDeletableMemory() *DeletableMemory { + return &DeletableMemory{ + nodes: make(map[descriptor.Descriptor]ocispec.Descriptor), + predecessors: make(map[descriptor.Descriptor]set.Set[descriptor.Descriptor]), + successors: make(map[descriptor.Descriptor]set.Set[descriptor.Descriptor]), } } // Index indexes predecessors for each direct successor of the given node. // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. -func (m *MemoryWithDelete) Index(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { - successors, err := content.Successors(ctx, fetcher, node) - if err != nil { - return err - } - - m.lock.Lock() - defer m.lock.Unlock() - m.index(ctx, node, successors) - return nil +func (m *DeletableMemory) Index(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { + _, err := m.index(ctx, fetcher, node) + return err } // Index indexes predecessors for all the successors of the given node. // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. -func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { +func (m *DeletableMemory) IndexAll(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { // track content status tracker := status.NewTracker() @@ -74,14 +69,7 @@ func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher return nil } - // skip the node if it has been indexed - key := descriptor.FromOCI(desc) - _, exists := m.indexed.Load(key) - if exists { - return nil - } - - successors, err := content.Successors(ctx, fetcher, desc) + successors, err := m.index(ctx, fetcher, desc) if err != nil { if errors.Is(err, errdef.ErrNotFound) { // skip the node if it does not exist @@ -89,8 +77,6 @@ func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher } return err } - m.index(ctx, desc, successors) - m.indexed.Store(key, nil) if len(successors) > 0 { // traverse and index successors @@ -98,8 +84,6 @@ func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher } return nil } - m.lock.Lock() - defer m.lock.Unlock() return syncutil.Go(ctx, nil, fn, node) } @@ -109,65 +93,63 @@ func (m *MemoryWithDelete) IndexAll(ctx context.Context, fetcher content.Fetcher // Like other operations, calling Predecessors() is go-routine safe. However, // it does not necessarily correspond to any consistent snapshot of the stored // contents. -func (m *MemoryWithDelete) Predecessors(_ context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { +func (m *DeletableMemory) Predecessors(_ context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { + m.lock.RLock() + defer m.lock.RUnlock() + key := descriptor.FromOCI(node) - _, exists := m.predecessors[key] + set, exists := m.predecessors[key] if !exists { return nil, nil } var res []ocispec.Descriptor - for _, v := range m.predecessors[key] { - res = append(res, v) + for k := range set { + res = append(res, m.nodes[k]) } return res, nil } // Remove removes the node from its predecessors and successors. -func (m *MemoryWithDelete) Remove(ctx context.Context, node ocispec.Descriptor) error { +func (m *DeletableMemory) Remove(ctx context.Context, node ocispec.Descriptor) error { nodeKey := descriptor.FromOCI(node) m.lock.Lock() defer m.lock.Unlock() // remove the node from its successors' predecessor list for successorKey := range m.successors[nodeKey] { - delete(m.predecessors[successorKey], nodeKey) + m.predecessors[successorKey].Delete(successorKey) } - m.removeEntriesFromMaps(ctx, node) + delete(m.successors, nodeKey) return nil } // index indexes predecessors for each direct successor of the given node. // There is no data consistency issue as long as deletion is not implemented // for the underlying storage. -func (m *MemoryWithDelete) index(ctx context.Context, node ocispec.Descriptor, successors []ocispec.Descriptor) { - m.createEntriesInMaps(ctx, node) - if len(successors) == 0 { - return +func (m *DeletableMemory) index(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { + successors, err := content.Successors(ctx, fetcher, node) + if err != nil { + return nil, err } - predecessorKey := descriptor.FromOCI(node) + m.lock.Lock() + defer m.lock.Unlock() + + // index the node + nodeKey := descriptor.FromOCI(node) + m.nodes[nodeKey] = node + + // index the successors and predecessors + successorSet := set.New[descriptor.Descriptor]() + m.successors[nodeKey] = successorSet + for _, successor := range successors { successorKey := descriptor.FromOCI(successor) - // store in m.predecessors, MemoryWithDelete.predecessors[successorKey].Store(node) - if _, exists := m.predecessors[successorKey]; !exists { - m.predecessors[successorKey] = make(map[descriptor.Descriptor]ocispec.Descriptor) + successorSet.Add(successorKey) + predecessorSet, exists := m.predecessors[nodeKey] + if !exists { + predecessorSet = set.New[descriptor.Descriptor]() + m.predecessors[successorKey] = predecessorSet } - m.predecessors[successorKey][predecessorKey] = node - // store in m.successors, MemoryWithDelete.successors[predecessorKey].Store(successor) - m.successors[predecessorKey][successorKey] = successor - } -} - -func (m *MemoryWithDelete) createEntriesInMaps(ctx context.Context, node ocispec.Descriptor) { - key := descriptor.FromOCI(node) - if _, hasEntry := m.predecessors[key]; !hasEntry { - m.predecessors[key] = make(map[descriptor.Descriptor]ocispec.Descriptor) - } - if _, hasEntry := m.successors[key]; !hasEntry { - m.successors[key] = make(map[descriptor.Descriptor]ocispec.Descriptor) + predecessorSet.Add(nodeKey) } -} - -func (m *MemoryWithDelete) removeEntriesFromMaps(ctx context.Context, node ocispec.Descriptor) { - key := descriptor.FromOCI(node) - delete(m.successors, key) - m.indexed.Delete(key) // pending + return successors, nil } diff --git a/internal/graph/DeletableMemory_test.go b/internal/graph/DeletableMemory_test.go new file mode 100644 index 00000000..de336aa6 --- /dev/null +++ b/internal/graph/DeletableMemory_test.go @@ -0,0 +1,239 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package graph + +import ( + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "oras.land/oras-go/v2/internal/descriptor" +) + +var ( + A = ocispec.Descriptor{MediaType: "A"} + AKey = descriptor.FromOCI(A) + B = ocispec.Descriptor{MediaType: "B"} + BKey = descriptor.FromOCI(B) + C = ocispec.Descriptor{MediaType: "C"} + CKey = descriptor.FromOCI(C) + D = ocispec.Descriptor{MediaType: "D"} + DKey = descriptor.FromOCI(D) +) + +// TODO: +// IndexAll和Delete，会不会出现race condition？ +// IndexAll和Index同时调用，会不会出现race condition？ + +// 当前条件（后续可能改动）： +// * 变量indexed的作用和行为和原始版本一样，没有加以变动 +// * 只要node被index（作为函数的第二个输入），node就在predecessors，successors中都有entry +// * 只有node被index（作为函数的第二个输入），successors中才有其entry +// * successors中的内容与node中的内容一致，只要entry还在，其内容就不会改变 +// * 删除一个node的时候，indexed和successors里面的entry也会删除。predecessors中的entry不会被删除 + +// GC情况： +// predecessors中可能有空的map，如下图中B被删除后，C还在predecessors中有entry但内容为空 +/* + +test 1: index "A -> B" +test 2: index "B -> C, B -> D" +test 3: index "A -> D" +test 4: delete B +test 5: delete A + +A--->B--->C +| | +| +--->D +| ^ +| | ++---------+ +*/ +// func TestMemoryWithDelete_indexAndDelete(t *testing.T) { +// ctx := context.Background() +// testMemoryWithDelete := NewMemoryWithDelete() + +// // test 1: index "A -> B" +// testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) + +// // check the memory: A exists in predecessors and successors +// // B ONLY exists in predecessors +// _, exists := testMemoryWithDelete.predecessors[AKey] +// if !exists { +// t.Errorf("could not find the entry of %v in predecessors", A) +// } +// _, exists = testMemoryWithDelete.successors[AKey] +// if !exists { +// t.Errorf("could not find the entry of %v in successors", A) +// } +// _, exists = testMemoryWithDelete.predecessors[BKey] +// if !exists { +// t.Errorf("could not find the entry of %v in predecessors", B) +// } +// _, exists = testMemoryWithDelete.successors[BKey] +// if exists { +// t.Errorf("%v should not exist in successors", B) +// } + +// // predecessors[B] contains A, successors[A] contains B +// _, exists = testMemoryWithDelete.predecessors[BKey][AKey] +// if !exists { +// t.Errorf("could not find %v in predecessors of %v", A, B) +// } +// _, exists = testMemoryWithDelete.successors[AKey][BKey] +// if !exists { +// t.Errorf("could not find %v in successors of %v", B, A) +// } + +// // test 2: index "B -> C, B -> D" +// testMemoryWithDelete.index(ctx, B, []ocispec.Descriptor{C, D}) + +// // check the memory: B exists in predecessors and successors, +// // C, D ONLY exists in predecessors +// _, exists = testMemoryWithDelete.predecessors[BKey] +// if !exists { +// t.Errorf("could not find the entry of %v in predecessors", B) +// } +// _, exists = testMemoryWithDelete.successors[BKey] +// if !exists { +// t.Errorf("could not find the entry of %v in successors", B) +// } +// _, exists = testMemoryWithDelete.predecessors[CKey] +// if !exists { +// t.Errorf("could not find the entry of %v in predecessors", C) +// } +// _, exists = testMemoryWithDelete.successors[CKey] +// if exists { +// t.Errorf("%v should not exist in successors", C) +// } +// _, exists = testMemoryWithDelete.predecessors[DKey] +// if !exists { +// t.Errorf("could not find the entry of %v in predecessors", D) +// } +// _, exists = testMemoryWithDelete.successors[DKey] +// if exists { +// t.Errorf("%v should not exist in successors", D) +// } +// // predecessors[C] contains B, predecessors[D] contains B, +// // successors[B] contains C and D +// _, exists = testMemoryWithDelete.predecessors[CKey][BKey] +// if !exists { +// t.Errorf("could not find %v in predecessors of %v", B, C) +// } +// _, exists = testMemoryWithDelete.predecessors[DKey][BKey] +// if !exists { +// t.Errorf("could not find %v in predecessors of %v", B, D) +// } +// _, exists = testMemoryWithDelete.successors[BKey][CKey] +// if !exists { +// t.Errorf("could not find %v in successors of %v", C, B) +// } +// _, exists = testMemoryWithDelete.successors[BKey][DKey] +// if !exists { +// t.Errorf("could not find %v in successors of %v", D, B) +// } + +// // test 3: index "A -> D" +// testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{D}) + +// // predecessors[D] contains A and B, successors[A] contains B and D +// _, exists = testMemoryWithDelete.predecessors[DKey][AKey] +// if !exists { +// t.Errorf("could not find %v in predecessors of %v", A, D) +// } +// _, exists = testMemoryWithDelete.predecessors[DKey][BKey] +// if !exists { +// t.Errorf("could not find %v in predecessors of %v", B, D) +// } +// _, exists = testMemoryWithDelete.successors[AKey][BKey] +// if !exists { +// t.Errorf("could not find %v in successors of %v", B, A) +// } +// _, exists = testMemoryWithDelete.successors[AKey][DKey] +// if !exists { +// t.Errorf("could not find %v in successors of %v", D, A) +// } + +// // check the memory: C and D have not been indexed, so C, D should not +// // exist in successors +// _, exists = testMemoryWithDelete.successors[CKey] +// if exists { +// t.Errorf("%v should not exist in successors", C) +// } +// _, exists = testMemoryWithDelete.successors[DKey] +// if exists { +// t.Errorf("%v should not exist in successors", D) +// } + +// // test 4: delete B +// err := testMemoryWithDelete.Remove(ctx, B) +// if err != nil { +// t.Errorf("got error when removing %v from index: %v", B, err) +// } + +// // check the memory: B should NOT exist in successors, should still exist +// // in predecessors +// _, exists = testMemoryWithDelete.predecessors[BKey] +// if !exists { +// t.Errorf("%v should exist in predecessors", B) +// } +// _, exists = testMemoryWithDelete.successors[BKey] +// if exists { +// t.Errorf("%v should not exist in successors", B) +// } + +// // B should STILL exist in successors[A] +// _, exists = testMemoryWithDelete.successors[AKey][BKey] +// if !exists { +// t.Errorf("could not find %v in successors of %v", B, A) +// } + +// // B should NOT exist in predecessors[C], predecessors[D] +// _, exists = testMemoryWithDelete.predecessors[CKey][BKey] +// if exists { +// t.Errorf("should not find %v in predecessors of %v", B, C) +// } +// _, exists = testMemoryWithDelete.predecessors[DKey][BKey] +// if exists { +// t.Errorf("should not find %v in predecessors of %v", B, D) +// } + +// // A should STILL exist in predecessors[D] +// _, exists = testMemoryWithDelete.predecessors[DKey][AKey] +// if !exists { +// t.Errorf("could not find %v in predecessors of %v", A, D) +// } + +// // test 5: delete A +// err = testMemoryWithDelete.Remove(ctx, A) +// if err != nil { +// t.Errorf("got error when removing %v from index: %v", A, err) +// } + +// // check the memory: A should NOT exist in successors, should still exist +// // in predecessors +// _, exists = testMemoryWithDelete.predecessors[AKey] +// if !exists { +// t.Errorf("%v should exist in predecessors", A) +// } +// _, exists = testMemoryWithDelete.successors[AKey] +// if exists { +// t.Errorf("%v should not exist in successors", A) +// } + +// // A should NOT exist in predecessors[D] +// _, exists = testMemoryWithDelete.predecessors[DKey][AKey] +// if exists { +// t.Errorf("should not find %v in predecessors of %v", A, D) +// } +// } diff --git a/internal/graph/memoryWithDelete_test.go b/internal/graph/memoryWithDelete_test.go deleted file mode 100644 index cf2595b3..00000000 --- a/internal/graph/memoryWithDelete_test.go +++ /dev/null @@ -1,242 +0,0 @@ -/* -Copyright The ORAS Authors. -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - -http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package graph - -import ( - "context" - "testing" - - ocispec "github.com/opencontainers/image-spec/specs-go/v1" - "oras.land/oras-go/v2/internal/descriptor" -) - -var ( - A = ocispec.Descriptor{MediaType: "A"} - AKey = descriptor.FromOCI(A) - B = ocispec.Descriptor{MediaType: "B"} - BKey = descriptor.FromOCI(B) - C = ocispec.Descriptor{MediaType: "C"} - CKey = descriptor.FromOCI(C) - D = ocispec.Descriptor{MediaType: "D"} - DKey = descriptor.FromOCI(D) -) - -// TODO: -// IndexAll和Delete，会不会出现race condition？ -// IndexAll和Index同时调用，会不会出现race condition？ - -// 当前条件（后续可能改动）： -// * 变量indexed的作用和行为和原始版本一样，没有加以变动 -// * 只要node被index（作为函数的第二个输入），node就在predecessors，successors中都有entry -// * 只有node被index（作为函数的第二个输入），successors中才有其entry -// * successors中的内容与node中的内容一致，只要entry还在，其内容就不会改变 -// * 删除一个node的时候，indexed和successors里面的entry也会删除。predecessors中的entry不会被删除 - -// GC情况： -// predecessors中可能有空的map，如下图中B被删除后，C还在predecessors中有entry但内容为空 -/* - -test 1: index "A -> B" -test 2: index "B -> C, B -> D" -test 3: index "A -> D" -test 4: delete B -test 5: delete A - -A--->B--->C -| | -| +--->D -| ^ -| | -+---------+ -*/ -func TestMemoryWithDelete_indexAndDelete(t *testing.T) { - ctx := context.Background() - testMemoryWithDelete := NewMemoryWithDelete() - - // test 1: index "A -> B" - testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) - - // check the memory: A exists in predecessors and successors - // B ONLY exists in predecessors - _, exists := testMemoryWithDelete.predecessors[AKey] - if !exists { - t.Errorf("could not find the entry of %v in predecessors", A) - } - _, exists = testMemoryWithDelete.successors[AKey] - if !exists { - t.Errorf("could not find the entry of %v in successors", A) - } - _, exists = testMemoryWithDelete.predecessors[BKey] - if !exists { - t.Errorf("could not find the entry of %v in predecessors", B) - } - _, exists = testMemoryWithDelete.successors[BKey] - if exists { - t.Errorf("%v should not exist in successors", B) - } - - // predecessors[B] contains A, successors[A] contains B - _, exists = testMemoryWithDelete.predecessors[BKey][AKey] - if !exists { - t.Errorf("could not find %v in predecessors of %v", A, B) - } - _, exists = testMemoryWithDelete.successors[AKey][BKey] - if !exists { - t.Errorf("could not find %v in successors of %v", B, A) - } - - // test 2: index "B -> C, B -> D" - testMemoryWithDelete.index(ctx, B, []ocispec.Descriptor{C, D}) - - // check the memory: B exists in predecessors and successors, - // C, D ONLY exists in predecessors - _, exists = testMemoryWithDelete.predecessors[BKey] - if !exists { - t.Errorf("could not find the entry of %v in predecessors", B) - } - _, exists = testMemoryWithDelete.successors[BKey] - if !exists { - t.Errorf("could not find the entry of %v in successors", B) - } - _, exists = testMemoryWithDelete.predecessors[CKey] - if !exists { - t.Errorf("could not find the entry of %v in predecessors", C) - } - _, exists = testMemoryWithDelete.successors[CKey] - if exists { - t.Errorf("%v should not exist in successors", C) - } - _, exists = testMemoryWithDelete.predecessors[DKey] - if !exists { - t.Errorf("could not find the entry of %v in predecessors", D) - } - _, exists = testMemoryWithDelete.successors[DKey] - if exists { - t.Errorf("%v should not exist in successors", D) - } - // predecessors[C] contains B, predecessors[D] contains B, - // successors[B] contains C and D - _, exists = testMemoryWithDelete.predecessors[CKey][BKey] - if !exists { - t.Errorf("could not find %v in predecessors of %v", B, C) - } - _, exists = testMemoryWithDelete.predecessors[DKey][BKey] - if !exists { - t.Errorf("could not find %v in predecessors of %v", B, D) - } - _, exists = testMemoryWithDelete.successors[BKey][CKey] - if !exists { - t.Errorf("could not find %v in successors of %v", C, B) - } - _, exists = testMemoryWithDelete.successors[BKey][DKey] - if !exists { - t.Errorf("could not find %v in successors of %v", D, B) - } - - // test 3: index "A -> D" - testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{D}) - - // predecessors[D] contains A and B, successors[A] contains B and D - _, exists = testMemoryWithDelete.predecessors[DKey][AKey] - if !exists { - t.Errorf("could not find %v in predecessors of %v", A, D) - } - _, exists = testMemoryWithDelete.predecessors[DKey][BKey] - if !exists { - t.Errorf("could not find %v in predecessors of %v", B, D) - } - _, exists = testMemoryWithDelete.successors[AKey][BKey] - if !exists { - t.Errorf("could not find %v in successors of %v", B, A) - } - _, exists = testMemoryWithDelete.successors[AKey][DKey] - if !exists { - t.Errorf("could not find %v in successors of %v", D, A) - } - - // check the memory: C and D have not been indexed, so C, D should not - // exist in successors - _, exists = testMemoryWithDelete.successors[CKey] - if exists { - t.Errorf("%v should not exist in successors", C) - } - _, exists = testMemoryWithDelete.successors[DKey] - if exists { - t.Errorf("%v should not exist in successors", D) - } - - // test 4: delete B - err := testMemoryWithDelete.Remove(ctx, B) - if err != nil { - t.Errorf("got error when removing %v from index: %v", B, err) - } - - // check the memory: B should NOT exist in successors, should still exist - // in predecessors - _, exists = testMemoryWithDelete.predecessors[BKey] - if !exists { - t.Errorf("%v should exist in predecessors", B) - } - _, exists = testMemoryWithDelete.successors[BKey] - if exists { - t.Errorf("%v should not exist in successors", B) - } - - // B should STILL exist in successors[A] - _, exists = testMemoryWithDelete.successors[AKey][BKey] - if !exists { - t.Errorf("could not find %v in successors of %v", B, A) - } - - // B should NOT exist in predecessors[C], predecessors[D] - _, exists = testMemoryWithDelete.predecessors[CKey][BKey] - if exists { - t.Errorf("should not find %v in predecessors of %v", B, C) - } - _, exists = testMemoryWithDelete.predecessors[DKey][BKey] - if exists { - t.Errorf("should not find %v in predecessors of %v", B, D) - } - - // A should STILL exist in predecessors[D] - _, exists = testMemoryWithDelete.predecessors[DKey][AKey] - if !exists { - t.Errorf("could not find %v in predecessors of %v", A, D) - } - - // test 5: delete A - err = testMemoryWithDelete.Remove(ctx, A) - if err != nil { - t.Errorf("got error when removing %v from index: %v", A, err) - } - - // check the memory: A should NOT exist in successors, should still exist - // in predecessors - _, exists = testMemoryWithDelete.predecessors[AKey] - if !exists { - t.Errorf("%v should exist in predecessors", A) - } - _, exists = testMemoryWithDelete.successors[AKey] - if exists { - t.Errorf("%v should not exist in successors", A) - } - - // A should NOT exist in predecessors[D] - _, exists = testMemoryWithDelete.predecessors[DKey][AKey] - if exists { - t.Errorf("should not find %v in predecessors of %v", A, D) - } -} From 76c33d4a054d73a375c2abd236b372eddadb0916 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Tue, 12 Sep 2023 07:59:01 +0000 Subject: [PATCH 20/26] moved loadFile function Signed-off-by: Xiaoxuan Wang --- content/oci/deletableOci.go | 21 ++++++++++++++++++++- content/oci/readonlyoci.go | 19 ------------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/content/oci/deletableOci.go b/content/oci/deletableOci.go index 375103bc..5bb0a799 100644 --- a/content/oci/deletableOci.go +++ b/content/oci/deletableOci.go @@ -294,7 +294,7 @@ func (ds *DeletableStore) loadIndexFile(ctx context.Context) error { return fmt.Errorf("failed to decode index file: %w", err) } ds.index = &index - return loadIndexWithMemoryWithDelete(ctx, ds.index, ds.storage, ds.tagResolver, ds.graph) + return loadIndexInDeletableMemory(ctx, ds.index, ds.storage, ds.tagResolver, ds.graph) } // SaveIndex writes the `index.json` file to the file system. @@ -348,3 +348,22 @@ func (ds *DeletableStore) writeIndexFile() error { } return os.WriteFile(ds.indexPath, indexJSON, 0666) } + +// loadIndexInDeletableMemory loads index into memory. +func loadIndexInDeletableMemory(ctx context.Context, index *ocispec.Index, fetcher content.Fetcher, tagger content.Tagger, graph *graph.DeletableMemory) error { + for _, desc := range index.Manifests { + if err := tagger.Tag(ctx, deleteAnnotationRefName(desc), desc.Digest.String()); err != nil { + return err + } + if ref := desc.Annotations[ocispec.AnnotationRefName]; ref != "" { + if err := tagger.Tag(ctx, desc, ref); err != nil { + return err + } + } + plain := descriptor.Plain(desc) + if err := graph.IndexAll(ctx, fetcher, plain); err != nil { + return err + } + } + return nil +} diff --git a/content/oci/readonlyoci.go b/content/oci/readonlyoci.go index 8dd33b3b..eb94f61c 100644 --- a/content/oci/readonlyoci.go +++ b/content/oci/readonlyoci.go @@ -186,25 +186,6 @@ func loadIndex(ctx context.Context, index *ocispec.Index, fetcher content.Fetche return nil } -// loadIndex loads index into memory. -func loadIndexWithMemoryWithDelete(ctx context.Context, index *ocispec.Index, fetcher content.Fetcher, tagger content.Tagger, graph *graph.DeletableMemory) error { - for _, desc := range index.Manifests { - if err := tagger.Tag(ctx, deleteAnnotationRefName(desc), desc.Digest.String()); err != nil { - return err - } - if ref := desc.Annotations[ocispec.AnnotationRefName]; ref != "" { - if err := tagger.Tag(ctx, desc, ref); err != nil { - return err - } - } - plain := descriptor.Plain(desc) - if err := graph.IndexAll(ctx, fetcher, plain); err != nil { - return err - } - } - return nil -} - // resolveBlob returns a descriptor describing the blob identified by dgst. func resolveBlob(fsys fs.FS, dgst string) (ocispec.Descriptor, error) { path, err := blobPath(digest.Digest(dgst)) From 5bc32d2efe53c58e2565211251d78b92ca6a815f Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Tue, 12 Sep 2023 08:23:42 +0000 Subject: [PATCH 21/26] updated file names Signed-off-by: Xiaoxuan Wang --- content/oci/{deletableOci.go => deletableoci.go} | 0 content/oci/{deletableOci_test.go => deletableoci_test.go} | 0 internal/graph/{DeletableMemory.go => deletablememory.go} | 0 .../graph/{DeletableMemory_test.go => deletablememory_test.go} | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename content/oci/{deletableOci.go => deletableoci.go} (100%) rename content/oci/{deletableOci_test.go => deletableoci_test.go} (100%) rename internal/graph/{DeletableMemory.go => deletablememory.go} (100%) rename internal/graph/{DeletableMemory_test.go => deletablememory_test.go} (100%) diff --git a/content/oci/deletableOci.go b/content/oci/deletableoci.go similarity index 100% rename from content/oci/deletableOci.go rename to content/oci/deletableoci.go diff --git a/content/oci/deletableOci_test.go b/content/oci/deletableoci_test.go similarity index 100% rename from content/oci/deletableOci_test.go rename to content/oci/deletableoci_test.go diff --git a/internal/graph/DeletableMemory.go b/internal/graph/deletablememory.go similarity index 100% rename from internal/graph/DeletableMemory.go rename to internal/graph/deletablememory.go diff --git a/internal/graph/DeletableMemory_test.go b/internal/graph/deletablememory_test.go similarity index 100% rename from internal/graph/DeletableMemory_test.go rename to internal/graph/deletablememory_test.go From 466ffd18158e0fdbc7e4deb7f59d7a6f50b90b18 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Thu, 14 Sep 2023 07:11:53 +0000 Subject: [PATCH 22/26] resolved the comments and refined code, need unit tests Signed-off-by: Xiaoxuan Wang --- internal/container/set/set.go | 2 +- internal/graph/deletablememory.go | 8 +- internal/graph/deletablememory_test.go | 372 ++++++++++++------------- 3 files changed, 183 insertions(+), 199 deletions(-) diff --git a/internal/container/set/set.go b/internal/container/set/set.go index 1a5f9016..07c96d47 100644 --- a/internal/container/set/set.go +++ b/internal/container/set/set.go @@ -34,7 +34,7 @@ func (s Set[T]) Contains(item T) bool { return ok } -// Delete deletes an item from the set +// Delete deletes an item from the set. func (s Set[T]) Delete(item T) { delete(s, item) } diff --git a/internal/graph/deletablememory.go b/internal/graph/deletablememory.go index 6ef42dad..1a093edf 100644 --- a/internal/graph/deletablememory.go +++ b/internal/graph/deletablememory.go @@ -29,7 +29,7 @@ import ( "oras.land/oras-go/v2/internal/syncutil" ) -// DeletableMemory is a DeletableMemory based PredecessorFinder. +// DeletableMemory is a memory based PredecessorFinder. type DeletableMemory struct { nodes map[descriptor.Descriptor]ocispec.Descriptor // nodes saves the map keys of ocispec.Descriptor predecessors map[descriptor.Descriptor]set.Set[descriptor.Descriptor] @@ -47,16 +47,12 @@ func NewDeletableMemory() *DeletableMemory { } // Index indexes predecessors for each direct successor of the given node. -// There is no data consistency issue as long as deletion is not implemented -// for the underlying storage. func (m *DeletableMemory) Index(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { _, err := m.index(ctx, fetcher, node) return err } // Index indexes predecessors for all the successors of the given node. -// There is no data consistency issue as long as deletion is not implemented -// for the underlying storage. func (m *DeletableMemory) IndexAll(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { // track content status tracker := status.NewTracker() @@ -123,8 +119,6 @@ func (m *DeletableMemory) Remove(ctx context.Context, node ocispec.Descriptor) e } // index indexes predecessors for each direct successor of the given node. -// There is no data consistency issue as long as deletion is not implemented -// for the underlying storage. func (m *DeletableMemory) index(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { successors, err := content.Successors(ctx, fetcher, node) if err != nil { diff --git a/internal/graph/deletablememory_test.go b/internal/graph/deletablememory_test.go index de336aa6..4b7187b1 100644 --- a/internal/graph/deletablememory_test.go +++ b/internal/graph/deletablememory_test.go @@ -16,6 +16,8 @@ limitations under the License. package graph import ( + "testing" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2/internal/descriptor" ) @@ -31,19 +33,6 @@ var ( DKey = descriptor.FromOCI(D) ) -// TODO: -// IndexAll和Delete，会不会出现race condition？ -// IndexAll和Index同时调用，会不会出现race condition？ - -// 当前条件（后续可能改动）： -// * 变量indexed的作用和行为和原始版本一样，没有加以变动 -// * 只要node被index（作为函数的第二个输入），node就在predecessors，successors中都有entry -// * 只有node被index（作为函数的第二个输入），successors中才有其entry -// * successors中的内容与node中的内容一致，只要entry还在，其内容就不会改变 -// * 删除一个node的时候，indexed和successors里面的entry也会删除。predecessors中的entry不会被删除 - -// GC情况： -// predecessors中可能有空的map，如下图中B被删除后，C还在predecessors中有entry但内容为空 /* test 1: index "A -> B" @@ -59,181 +48,182 @@ A--->B--->C | | +---------+ */ -// func TestMemoryWithDelete_indexAndDelete(t *testing.T) { -// ctx := context.Background() -// testMemoryWithDelete := NewMemoryWithDelete() - -// // test 1: index "A -> B" -// testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) - -// // check the memory: A exists in predecessors and successors -// // B ONLY exists in predecessors -// _, exists := testMemoryWithDelete.predecessors[AKey] -// if !exists { -// t.Errorf("could not find the entry of %v in predecessors", A) -// } -// _, exists = testMemoryWithDelete.successors[AKey] -// if !exists { -// t.Errorf("could not find the entry of %v in successors", A) -// } -// _, exists = testMemoryWithDelete.predecessors[BKey] -// if !exists { -// t.Errorf("could not find the entry of %v in predecessors", B) -// } -// _, exists = testMemoryWithDelete.successors[BKey] -// if exists { -// t.Errorf("%v should not exist in successors", B) -// } - -// // predecessors[B] contains A, successors[A] contains B -// _, exists = testMemoryWithDelete.predecessors[BKey][AKey] -// if !exists { -// t.Errorf("could not find %v in predecessors of %v", A, B) -// } -// _, exists = testMemoryWithDelete.successors[AKey][BKey] -// if !exists { -// t.Errorf("could not find %v in successors of %v", B, A) -// } - -// // test 2: index "B -> C, B -> D" -// testMemoryWithDelete.index(ctx, B, []ocispec.Descriptor{C, D}) - -// // check the memory: B exists in predecessors and successors, -// // C, D ONLY exists in predecessors -// _, exists = testMemoryWithDelete.predecessors[BKey] -// if !exists { -// t.Errorf("could not find the entry of %v in predecessors", B) -// } -// _, exists = testMemoryWithDelete.successors[BKey] -// if !exists { -// t.Errorf("could not find the entry of %v in successors", B) -// } -// _, exists = testMemoryWithDelete.predecessors[CKey] -// if !exists { -// t.Errorf("could not find the entry of %v in predecessors", C) -// } -// _, exists = testMemoryWithDelete.successors[CKey] -// if exists { -// t.Errorf("%v should not exist in successors", C) -// } -// _, exists = testMemoryWithDelete.predecessors[DKey] -// if !exists { -// t.Errorf("could not find the entry of %v in predecessors", D) -// } -// _, exists = testMemoryWithDelete.successors[DKey] -// if exists { -// t.Errorf("%v should not exist in successors", D) -// } -// // predecessors[C] contains B, predecessors[D] contains B, -// // successors[B] contains C and D -// _, exists = testMemoryWithDelete.predecessors[CKey][BKey] -// if !exists { -// t.Errorf("could not find %v in predecessors of %v", B, C) -// } -// _, exists = testMemoryWithDelete.predecessors[DKey][BKey] -// if !exists { -// t.Errorf("could not find %v in predecessors of %v", B, D) -// } -// _, exists = testMemoryWithDelete.successors[BKey][CKey] -// if !exists { -// t.Errorf("could not find %v in successors of %v", C, B) -// } -// _, exists = testMemoryWithDelete.successors[BKey][DKey] -// if !exists { -// t.Errorf("could not find %v in successors of %v", D, B) -// } - -// // test 3: index "A -> D" -// testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{D}) - -// // predecessors[D] contains A and B, successors[A] contains B and D -// _, exists = testMemoryWithDelete.predecessors[DKey][AKey] -// if !exists { -// t.Errorf("could not find %v in predecessors of %v", A, D) -// } -// _, exists = testMemoryWithDelete.predecessors[DKey][BKey] -// if !exists { -// t.Errorf("could not find %v in predecessors of %v", B, D) -// } -// _, exists = testMemoryWithDelete.successors[AKey][BKey] -// if !exists { -// t.Errorf("could not find %v in successors of %v", B, A) -// } -// _, exists = testMemoryWithDelete.successors[AKey][DKey] -// if !exists { -// t.Errorf("could not find %v in successors of %v", D, A) -// } - -// // check the memory: C and D have not been indexed, so C, D should not -// // exist in successors -// _, exists = testMemoryWithDelete.successors[CKey] -// if exists { -// t.Errorf("%v should not exist in successors", C) -// } -// _, exists = testMemoryWithDelete.successors[DKey] -// if exists { -// t.Errorf("%v should not exist in successors", D) -// } - -// // test 4: delete B -// err := testMemoryWithDelete.Remove(ctx, B) -// if err != nil { -// t.Errorf("got error when removing %v from index: %v", B, err) -// } - -// // check the memory: B should NOT exist in successors, should still exist -// // in predecessors -// _, exists = testMemoryWithDelete.predecessors[BKey] -// if !exists { -// t.Errorf("%v should exist in predecessors", B) -// } -// _, exists = testMemoryWithDelete.successors[BKey] -// if exists { -// t.Errorf("%v should not exist in successors", B) -// } - -// // B should STILL exist in successors[A] -// _, exists = testMemoryWithDelete.successors[AKey][BKey] -// if !exists { -// t.Errorf("could not find %v in successors of %v", B, A) -// } - -// // B should NOT exist in predecessors[C], predecessors[D] -// _, exists = testMemoryWithDelete.predecessors[CKey][BKey] -// if exists { -// t.Errorf("should not find %v in predecessors of %v", B, C) -// } -// _, exists = testMemoryWithDelete.predecessors[DKey][BKey] -// if exists { -// t.Errorf("should not find %v in predecessors of %v", B, D) -// } - -// // A should STILL exist in predecessors[D] -// _, exists = testMemoryWithDelete.predecessors[DKey][AKey] -// if !exists { -// t.Errorf("could not find %v in predecessors of %v", A, D) -// } - -// // test 5: delete A -// err = testMemoryWithDelete.Remove(ctx, A) -// if err != nil { -// t.Errorf("got error when removing %v from index: %v", A, err) -// } - -// // check the memory: A should NOT exist in successors, should still exist -// // in predecessors -// _, exists = testMemoryWithDelete.predecessors[AKey] -// if !exists { -// t.Errorf("%v should exist in predecessors", A) -// } -// _, exists = testMemoryWithDelete.successors[AKey] -// if exists { -// t.Errorf("%v should not exist in successors", A) -// } - -// // A should NOT exist in predecessors[D] -// _, exists = testMemoryWithDelete.predecessors[DKey][AKey] -// if exists { -// t.Errorf("should not find %v in predecessors of %v", A, D) -// } -// } + +func TestMemoryWithDelete_indexAndDelete(t *testing.T) { + // ctx := context.Background() + // testDeletableMemory := NewDeletableMemory() + + // // test 1: index "A -> B" + // testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) + + // // check the memory: A exists in predecessors and successors + // // B ONLY exists in predecessors + // _, exists := testMemoryWithDelete.predecessors[AKey] + // if !exists { + // t.Errorf("could not find the entry of %v in predecessors", A) + // } + // _, exists = testMemoryWithDelete.successors[AKey] + // if !exists { + // t.Errorf("could not find the entry of %v in successors", A) + // } + // _, exists = testMemoryWithDelete.predecessors[BKey] + // if !exists { + // t.Errorf("could not find the entry of %v in predecessors", B) + // } + // _, exists = testMemoryWithDelete.successors[BKey] + // if exists { + // t.Errorf("%v should not exist in successors", B) + // } + + // // predecessors[B] contains A, successors[A] contains B + // _, exists = testMemoryWithDelete.predecessors[BKey][AKey] + // if !exists { + // t.Errorf("could not find %v in predecessors of %v", A, B) + // } + // _, exists = testMemoryWithDelete.successors[AKey][BKey] + // if !exists { + // t.Errorf("could not find %v in successors of %v", B, A) + // } + + // // test 2: index "B -> C, B -> D" + // testMemoryWithDelete.index(ctx, B, []ocispec.Descriptor{C, D}) + + // // check the memory: B exists in predecessors and successors, + // // C, D ONLY exists in predecessors + // _, exists = testMemoryWithDelete.predecessors[BKey] + // if !exists { + // t.Errorf("could not find the entry of %v in predecessors", B) + // } + // _, exists = testMemoryWithDelete.successors[BKey] + // if !exists { + // t.Errorf("could not find the entry of %v in successors", B) + // } + // _, exists = testMemoryWithDelete.predecessors[CKey] + // if !exists { + // t.Errorf("could not find the entry of %v in predecessors", C) + // } + // _, exists = testMemoryWithDelete.successors[CKey] + // if exists { + // t.Errorf("%v should not exist in successors", C) + // } + // _, exists = testMemoryWithDelete.predecessors[DKey] + // if !exists { + // t.Errorf("could not find the entry of %v in predecessors", D) + // } + // _, exists = testMemoryWithDelete.successors[DKey] + // if exists { + // t.Errorf("%v should not exist in successors", D) + // } + // // predecessors[C] contains B, predecessors[D] contains B, + // // successors[B] contains C and D + // _, exists = testMemoryWithDelete.predecessors[CKey][BKey] + // if !exists { + // t.Errorf("could not find %v in predecessors of %v", B, C) + // } + // _, exists = testMemoryWithDelete.predecessors[DKey][BKey] + // if !exists { + // t.Errorf("could not find %v in predecessors of %v", B, D) + // } + // _, exists = testMemoryWithDelete.successors[BKey][CKey] + // if !exists { + // t.Errorf("could not find %v in successors of %v", C, B) + // } + // _, exists = testMemoryWithDelete.successors[BKey][DKey] + // if !exists { + // t.Errorf("could not find %v in successors of %v", D, B) + // } + + // // test 3: index "A -> D" + // testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{D}) + + // // predecessors[D] contains A and B, successors[A] contains B and D + // _, exists = testMemoryWithDelete.predecessors[DKey][AKey] + // if !exists { + // t.Errorf("could not find %v in predecessors of %v", A, D) + // } + // _, exists = testMemoryWithDelete.predecessors[DKey][BKey] + // if !exists { + // t.Errorf("could not find %v in predecessors of %v", B, D) + // } + // _, exists = testMemoryWithDelete.successors[AKey][BKey] + // if !exists { + // t.Errorf("could not find %v in successors of %v", B, A) + // } + // _, exists = testMemoryWithDelete.successors[AKey][DKey] + // if !exists { + // t.Errorf("could not find %v in successors of %v", D, A) + // } + + // // check the memory: C and D have not been indexed, so C, D should not + // // exist in successors + // _, exists = testMemoryWithDelete.successors[CKey] + // if exists { + // t.Errorf("%v should not exist in successors", C) + // } + // _, exists = testMemoryWithDelete.successors[DKey] + // if exists { + // t.Errorf("%v should not exist in successors", D) + // } + + // // test 4: delete B + // err := testMemoryWithDelete.Remove(ctx, B) + // if err != nil { + // t.Errorf("got error when removing %v from index: %v", B, err) + // } + + // // check the memory: B should NOT exist in successors, should still exist + // // in predecessors + // _, exists = testMemoryWithDelete.predecessors[BKey] + // if !exists { + // t.Errorf("%v should exist in predecessors", B) + // } + // _, exists = testMemoryWithDelete.successors[BKey] + // if exists { + // t.Errorf("%v should not exist in successors", B) + // } + + // // B should STILL exist in successors[A] + // _, exists = testMemoryWithDelete.successors[AKey][BKey] + // if !exists { + // t.Errorf("could not find %v in successors of %v", B, A) + // } + + // // B should NOT exist in predecessors[C], predecessors[D] + // _, exists = testMemoryWithDelete.predecessors[CKey][BKey] + // if exists { + // t.Errorf("should not find %v in predecessors of %v", B, C) + // } + // _, exists = testMemoryWithDelete.predecessors[DKey][BKey] + // if exists { + // t.Errorf("should not find %v in predecessors of %v", B, D) + // } + + // // A should STILL exist in predecessors[D] + // _, exists = testMemoryWithDelete.predecessors[DKey][AKey] + // if !exists { + // t.Errorf("could not find %v in predecessors of %v", A, D) + // } + + // // test 5: delete A + // err = testMemoryWithDelete.Remove(ctx, A) + // if err != nil { + // t.Errorf("got error when removing %v from index: %v", A, err) + // } + + // // check the memory: A should NOT exist in successors, should still exist + // // in predecessors + // _, exists = testMemoryWithDelete.predecessors[AKey] + // if !exists { + // t.Errorf("%v should exist in predecessors", A) + // } + // _, exists = testMemoryWithDelete.successors[AKey] + // if exists { + // t.Errorf("%v should not exist in successors", A) + // } + + // // A should NOT exist in predecessors[D] + // _, exists = testMemoryWithDelete.predecessors[DKey][AKey] + // if exists { + // t.Errorf("should not find %v in predecessors of %v", A, D) + // } +} From 2117a8d444011aeb351330b55af022ea76a21e2e Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 18 Sep 2023 02:09:44 +0000 Subject: [PATCH 23/26] removed the unit test it's too hard to write Signed-off-by: Xiaoxuan Wang --- internal/graph/deletablememory_test.go | 229 ------------------------- 1 file changed, 229 deletions(-) delete mode 100644 internal/graph/deletablememory_test.go diff --git a/internal/graph/deletablememory_test.go b/internal/graph/deletablememory_test.go deleted file mode 100644 index 4b7187b1..00000000 --- a/internal/graph/deletablememory_test.go +++ /dev/null @@ -1,229 +0,0 @@ -/* -Copyright The ORAS Authors. -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - -http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package graph - -import ( - "testing" - - ocispec "github.com/opencontainers/image-spec/specs-go/v1" - "oras.land/oras-go/v2/internal/descriptor" -) - -var ( - A = ocispec.Descriptor{MediaType: "A"} - AKey = descriptor.FromOCI(A) - B = ocispec.Descriptor{MediaType: "B"} - BKey = descriptor.FromOCI(B) - C = ocispec.Descriptor{MediaType: "C"} - CKey = descriptor.FromOCI(C) - D = ocispec.Descriptor{MediaType: "D"} - DKey = descriptor.FromOCI(D) -) - -/* - -test 1: index "A -> B" -test 2: index "B -> C, B -> D" -test 3: index "A -> D" -test 4: delete B -test 5: delete A - -A--->B--->C -| | -| +--->D -| ^ -| | -+---------+ -*/ - -func TestMemoryWithDelete_indexAndDelete(t *testing.T) { - // ctx := context.Background() - // testDeletableMemory := NewDeletableMemory() - - // // test 1: index "A -> B" - // testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{B}) - - // // check the memory: A exists in predecessors and successors - // // B ONLY exists in predecessors - // _, exists := testMemoryWithDelete.predecessors[AKey] - // if !exists { - // t.Errorf("could not find the entry of %v in predecessors", A) - // } - // _, exists = testMemoryWithDelete.successors[AKey] - // if !exists { - // t.Errorf("could not find the entry of %v in successors", A) - // } - // _, exists = testMemoryWithDelete.predecessors[BKey] - // if !exists { - // t.Errorf("could not find the entry of %v in predecessors", B) - // } - // _, exists = testMemoryWithDelete.successors[BKey] - // if exists { - // t.Errorf("%v should not exist in successors", B) - // } - - // // predecessors[B] contains A, successors[A] contains B - // _, exists = testMemoryWithDelete.predecessors[BKey][AKey] - // if !exists { - // t.Errorf("could not find %v in predecessors of %v", A, B) - // } - // _, exists = testMemoryWithDelete.successors[AKey][BKey] - // if !exists { - // t.Errorf("could not find %v in successors of %v", B, A) - // } - - // // test 2: index "B -> C, B -> D" - // testMemoryWithDelete.index(ctx, B, []ocispec.Descriptor{C, D}) - - // // check the memory: B exists in predecessors and successors, - // // C, D ONLY exists in predecessors - // _, exists = testMemoryWithDelete.predecessors[BKey] - // if !exists { - // t.Errorf("could not find the entry of %v in predecessors", B) - // } - // _, exists = testMemoryWithDelete.successors[BKey] - // if !exists { - // t.Errorf("could not find the entry of %v in successors", B) - // } - // _, exists = testMemoryWithDelete.predecessors[CKey] - // if !exists { - // t.Errorf("could not find the entry of %v in predecessors", C) - // } - // _, exists = testMemoryWithDelete.successors[CKey] - // if exists { - // t.Errorf("%v should not exist in successors", C) - // } - // _, exists = testMemoryWithDelete.predecessors[DKey] - // if !exists { - // t.Errorf("could not find the entry of %v in predecessors", D) - // } - // _, exists = testMemoryWithDelete.successors[DKey] - // if exists { - // t.Errorf("%v should not exist in successors", D) - // } - // // predecessors[C] contains B, predecessors[D] contains B, - // // successors[B] contains C and D - // _, exists = testMemoryWithDelete.predecessors[CKey][BKey] - // if !exists { - // t.Errorf("could not find %v in predecessors of %v", B, C) - // } - // _, exists = testMemoryWithDelete.predecessors[DKey][BKey] - // if !exists { - // t.Errorf("could not find %v in predecessors of %v", B, D) - // } - // _, exists = testMemoryWithDelete.successors[BKey][CKey] - // if !exists { - // t.Errorf("could not find %v in successors of %v", C, B) - // } - // _, exists = testMemoryWithDelete.successors[BKey][DKey] - // if !exists { - // t.Errorf("could not find %v in successors of %v", D, B) - // } - - // // test 3: index "A -> D" - // testMemoryWithDelete.index(ctx, A, []ocispec.Descriptor{D}) - - // // predecessors[D] contains A and B, successors[A] contains B and D - // _, exists = testMemoryWithDelete.predecessors[DKey][AKey] - // if !exists { - // t.Errorf("could not find %v in predecessors of %v", A, D) - // } - // _, exists = testMemoryWithDelete.predecessors[DKey][BKey] - // if !exists { - // t.Errorf("could not find %v in predecessors of %v", B, D) - // } - // _, exists = testMemoryWithDelete.successors[AKey][BKey] - // if !exists { - // t.Errorf("could not find %v in successors of %v", B, A) - // } - // _, exists = testMemoryWithDelete.successors[AKey][DKey] - // if !exists { - // t.Errorf("could not find %v in successors of %v", D, A) - // } - - // // check the memory: C and D have not been indexed, so C, D should not - // // exist in successors - // _, exists = testMemoryWithDelete.successors[CKey] - // if exists { - // t.Errorf("%v should not exist in successors", C) - // } - // _, exists = testMemoryWithDelete.successors[DKey] - // if exists { - // t.Errorf("%v should not exist in successors", D) - // } - - // // test 4: delete B - // err := testMemoryWithDelete.Remove(ctx, B) - // if err != nil { - // t.Errorf("got error when removing %v from index: %v", B, err) - // } - - // // check the memory: B should NOT exist in successors, should still exist - // // in predecessors - // _, exists = testMemoryWithDelete.predecessors[BKey] - // if !exists { - // t.Errorf("%v should exist in predecessors", B) - // } - // _, exists = testMemoryWithDelete.successors[BKey] - // if exists { - // t.Errorf("%v should not exist in successors", B) - // } - - // // B should STILL exist in successors[A] - // _, exists = testMemoryWithDelete.successors[AKey][BKey] - // if !exists { - // t.Errorf("could not find %v in successors of %v", B, A) - // } - - // // B should NOT exist in predecessors[C], predecessors[D] - // _, exists = testMemoryWithDelete.predecessors[CKey][BKey] - // if exists { - // t.Errorf("should not find %v in predecessors of %v", B, C) - // } - // _, exists = testMemoryWithDelete.predecessors[DKey][BKey] - // if exists { - // t.Errorf("should not find %v in predecessors of %v", B, D) - // } - - // // A should STILL exist in predecessors[D] - // _, exists = testMemoryWithDelete.predecessors[DKey][AKey] - // if !exists { - // t.Errorf("could not find %v in predecessors of %v", A, D) - // } - - // // test 5: delete A - // err = testMemoryWithDelete.Remove(ctx, A) - // if err != nil { - // t.Errorf("got error when removing %v from index: %v", A, err) - // } - - // // check the memory: A should NOT exist in successors, should still exist - // // in predecessors - // _, exists = testMemoryWithDelete.predecessors[AKey] - // if !exists { - // t.Errorf("%v should exist in predecessors", A) - // } - // _, exists = testMemoryWithDelete.successors[AKey] - // if exists { - // t.Errorf("%v should not exist in successors", A) - // } - - // // A should NOT exist in predecessors[D] - // _, exists = testMemoryWithDelete.predecessors[DKey][AKey] - // if exists { - // t.Errorf("should not find %v in predecessors of %v", A, D) - // } -} From 5d6bede4fb3b93f614b3e9a8649199ae1574f9d8 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 18 Sep 2023 02:23:43 +0000 Subject: [PATCH 24/26] refined the code Signed-off-by: Xiaoxuan Wang --- content/oci/deletableoci.go | 14 +------------- content/oci/storage.go | 1 - content/oci/storage_test.go | 4 ---- internal/graph/deletablememory.go | 5 ----- 4 files changed, 1 insertion(+), 23 deletions(-) diff --git a/content/oci/deletableoci.go b/content/oci/deletableoci.go index 5bb0a799..0333693a 100644 --- a/content/oci/deletableoci.go +++ b/content/oci/deletableoci.go @@ -75,7 +75,6 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS if err != nil { return nil, fmt.Errorf("failed to create storage: %w", err) } - store := &DeletableStore{ AutoSaveIndex: true, root: rootAbs, @@ -84,7 +83,6 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS tagResolver: resolver.NewMemory(), graph: graph.NewDeletableMemory(), } - if err := ensureDir(filepath.Join(rootAbs, ociBlobsDir)); err != nil { return nil, err } @@ -94,7 +92,6 @@ func NewDeletableStoreWithContext(ctx context.Context, root string) (*DeletableS if err := store.loadIndexFile(ctx); err != nil { return nil, fmt.Errorf("invalid OCI Image Index: %w", err) } - return store, nil } @@ -160,7 +157,6 @@ func (ds *DeletableStore) Tag(ctx context.Context, desc ocispec.Descriptor, refe if err := validateReference(reference); err != nil { return err } - exists, err := ds.storage.Exists(ctx, desc) if err != nil { return err @@ -168,7 +164,6 @@ func (ds *DeletableStore) Tag(ctx context.Context, desc ocispec.Descriptor, refe if !exists { return fmt.Errorf("%s: %s: %w", desc.Digest, desc.MediaType, errdef.ErrNotFound) } - return ds.tag(ctx, desc, reference) } @@ -201,7 +196,6 @@ func (ds *DeletableStore) Resolve(ctx context.Context, reference string) (ocispe if reference == "" { return ocispec.Descriptor{}, errdef.ErrMissingReference } - // attempt resolving manifest desc, err := ds.tagResolver.Resolve(ctx, reference) if err != nil { @@ -211,11 +205,9 @@ func (ds *DeletableStore) Resolve(ctx context.Context, reference string) (ocispe } return ocispec.Descriptor{}, err } - if reference == desc.Digest.String() { return descriptor.Plain(desc), nil } - return desc, nil } @@ -249,7 +241,6 @@ func (ds *DeletableStore) ensureOCILayoutFile() error { if !os.IsNotExist(err) { return fmt.Errorf("failed to open OCI layout file: %w", err) } - layout := ocispec.ImageLayout{ Version: ocispec.ImageLayoutVersion, } @@ -260,7 +251,6 @@ func (ds *DeletableStore) ensureOCILayoutFile() error { return os.WriteFile(layoutFilePath, layoutJSON, 0666) } defer layoutFile.Close() - var layout ocispec.ImageLayout err = json.NewDecoder(layoutFile).Decode(&layout) if err != nil { @@ -277,7 +267,6 @@ func (ds *DeletableStore) loadIndexFile(ctx context.Context) error { if !os.IsNotExist(err) { return fmt.Errorf("failed to open index file: %w", err) } - // write index.json if it does not exist ds.index = &ocispec.Index{ Versioned: specs.Versioned{ @@ -288,7 +277,6 @@ func (ds *DeletableStore) loadIndexFile(ctx context.Context) error { return ds.writeIndexFile() } defer indexFile.Close() - var index ocispec.Index if err := json.NewDecoder(indexFile).Decode(&index); err != nil { return fmt.Errorf("failed to decode index file: %w", err) @@ -349,7 +337,7 @@ func (ds *DeletableStore) writeIndexFile() error { return os.WriteFile(ds.indexPath, indexJSON, 0666) } -// loadIndexInDeletableMemory loads index into memory. +// loadIndexInDeletableMemory loads index into the memory. func loadIndexInDeletableMemory(ctx context.Context, index *ocispec.Index, fetcher content.Fetcher, tagger content.Tagger, graph *graph.DeletableMemory) error { for _, desc := range index.Manifests { if err := tagger.Tag(ctx, deleteAnnotationRefName(desc), desc.Digest.String()); err != nil { diff --git a/content/oci/storage.go b/content/oci/storage.go index a1803e9a..98df6428 100644 --- a/content/oci/storage.go +++ b/content/oci/storage.go @@ -114,7 +114,6 @@ func (s *Storage) Delete(ctx context.Context, target ocispec.Descriptor) error { return fmt.Errorf("%s: %s: %w", target.Digest, target.MediaType, errdef.ErrInvalidDigest) } targetPath := filepath.Join(s.root, path) - return os.Remove(targetPath) } diff --git a/content/oci/storage_test.go b/content/oci/storage_test.go index c4f8aa95..6c30cba2 100644 --- a/content/oci/storage_test.go +++ b/content/oci/storage_test.go @@ -385,18 +385,15 @@ func TestStorage_Delete(t *testing.T) { 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) @@ -404,7 +401,6 @@ func TestStorage_Delete(t *testing.T) { if !exists { t.Errorf("Storage.Exists() = %v, want %v", exists, true) } - err = s.Delete(ctx, desc) if err != nil { t.Fatal("Storage.Delete() error =", err) diff --git a/internal/graph/deletablememory.go b/internal/graph/deletablememory.go index 1a093edf..49ef64dc 100644 --- a/internal/graph/deletablememory.go +++ b/internal/graph/deletablememory.go @@ -56,7 +56,6 @@ func (m *DeletableMemory) Index(ctx context.Context, fetcher content.Fetcher, no func (m *DeletableMemory) IndexAll(ctx context.Context, fetcher content.Fetcher, node ocispec.Descriptor) error { // track content status tracker := status.NewTracker() - var fn syncutil.GoFunc[ocispec.Descriptor] fn = func(ctx context.Context, region *syncutil.LimitedRegion, desc ocispec.Descriptor) error { // skip the node if other go routine is working on it @@ -64,7 +63,6 @@ func (m *DeletableMemory) IndexAll(ctx context.Context, fetcher content.Fetcher, if !committed { return nil } - successors, err := m.index(ctx, fetcher, desc) if err != nil { if errors.Is(err, errdef.ErrNotFound) { @@ -73,7 +71,6 @@ func (m *DeletableMemory) IndexAll(ctx context.Context, fetcher content.Fetcher, } return err } - if len(successors) > 0 { // traverse and index successors return syncutil.Go(ctx, nil, fn, successors...) @@ -92,7 +89,6 @@ func (m *DeletableMemory) IndexAll(ctx context.Context, fetcher content.Fetcher, func (m *DeletableMemory) Predecessors(_ context.Context, node ocispec.Descriptor) ([]ocispec.Descriptor, error) { m.lock.RLock() defer m.lock.RUnlock() - key := descriptor.FromOCI(node) set, exists := m.predecessors[key] if !exists { @@ -134,7 +130,6 @@ func (m *DeletableMemory) index(ctx context.Context, fetcher content.Fetcher, no // index the successors and predecessors successorSet := set.New[descriptor.Descriptor]() m.successors[nodeKey] = successorSet - for _, successor := range successors { successorKey := descriptor.FromOCI(successor) successorSet.Add(successorKey) From cb8749ea2b83627a23cbecc8afdd45886573b14a Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Wed, 20 Sep 2023 11:09:56 +0000 Subject: [PATCH 25/26] bug fix and basic unit tests Signed-off-by: Xiaoxuan Wang --- internal/graph/deletablememory.go | 4 +- internal/graph/deletablememory_test.go | 379 +++++++++++++++++++++++++ 2 files changed, 381 insertions(+), 2 deletions(-) create mode 100644 internal/graph/deletablememory_test.go diff --git a/internal/graph/deletablememory.go b/internal/graph/deletablememory.go index 49ef64dc..54aa02a5 100644 --- a/internal/graph/deletablememory.go +++ b/internal/graph/deletablememory.go @@ -108,7 +108,7 @@ func (m *DeletableMemory) Remove(ctx context.Context, node ocispec.Descriptor) e defer m.lock.Unlock() // remove the node from its successors' predecessor list for successorKey := range m.successors[nodeKey] { - m.predecessors[successorKey].Delete(successorKey) + m.predecessors[successorKey].Delete(nodeKey) } delete(m.successors, nodeKey) return nil @@ -133,7 +133,7 @@ func (m *DeletableMemory) index(ctx context.Context, fetcher content.Fetcher, no for _, successor := range successors { successorKey := descriptor.FromOCI(successor) successorSet.Add(successorKey) - predecessorSet, exists := m.predecessors[nodeKey] + predecessorSet, exists := m.predecessors[successorKey] if !exists { predecessorSet = set.New[descriptor.Descriptor]() m.predecessors[successorKey] = predecessorSet diff --git a/internal/graph/deletablememory_test.go b/internal/graph/deletablememory_test.go new file mode 100644 index 00000000..ef2ee164 --- /dev/null +++ b/internal/graph/deletablememory_test.go @@ -0,0 +1,379 @@ +/* +Copyright The ORAS Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package graph + +import ( + "bytes" + "context" + "encoding/json" + "io" + "reflect" + "testing" + + "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "oras.land/oras-go/v2/internal/cas" + "oras.land/oras-go/v2/internal/descriptor" +) + +/* +A(manifest)-------------------+ + + | | + | | + | | + v | + +B(manifest)-----------------+ | + + | | | + | | | + v v v + +C(layer) D(layer) +*/ +func TestDeletableMemory_IndexAndRemove(t *testing.T) { + testFetcher := cas.NewMemory() + testDeletableMemory := NewDeletableMemory() + ctx := context.Background() + + // generate test content + var blobs [][]byte + var descs []ocispec.Descriptor + appendBlob := func(mediaType string, blob []byte) ocispec.Descriptor { + blobs = append(blobs, blob) + descs = append(descs, ocispec.Descriptor{ + MediaType: mediaType, + Digest: digest.FromBytes(blob), + Size: int64(len(blob)), + }) + return descs[len(descs)-1] + } + generateManifest := func(layers ...ocispec.Descriptor) ocispec.Descriptor { + manifest := ocispec.Manifest{ + Layers: layers, + } + manifestJSON, err := json.Marshal(manifest) + if err != nil { + t.Fatal(err) + } + return appendBlob(ocispec.MediaTypeImageManifest, manifestJSON) + } + descC := appendBlob("layer node C", []byte("Node C is a layer")) // blobs[0], layer "C" + descD := appendBlob("layer node D", []byte("Node D is a layer")) // blobs[1], layer "D" + descB := generateManifest(descs[0:2]...) // blobs[2], manifest "B" + descA := generateManifest(descs[1:3]...) // blobs[3], manifest "A" + testFetcher.Push(ctx, descA, bytes.NewReader(blobs[3])) + testFetcher.Push(ctx, descB, bytes.NewReader(blobs[2])) + testFetcher.Push(ctx, descC, bytes.NewReader(blobs[0])) + testFetcher.Push(ctx, descD, bytes.NewReader(blobs[1])) + + // make sure that testFetcher works + rc, err := testFetcher.Fetch(ctx, descA) + if err != nil { + t.Errorf("testFetcher.Fetch() error = %v", err) + } + got, err := io.ReadAll(rc) + if err != nil { + t.Errorf("testFetcher.Fetch().Read() error = %v", err) + } + err = rc.Close() + if err != nil { + t.Errorf("testFetcher.Fetch().Close() error = %v", err) + } + if !bytes.Equal(got, blobs[3]) { + t.Errorf("testFetcher.Fetch() = %v, want %v", got, blobs[4]) + } + + // index test content into graph.DeletableMemory + testDeletableMemory.Index(ctx, testFetcher, descA) + testDeletableMemory.Index(ctx, testFetcher, descB) + testDeletableMemory.Index(ctx, testFetcher, descC) + testDeletableMemory.Index(ctx, testFetcher, descD) + + // check that the content of testDeletableMemory.predecessors and successors + // are correct + nodeKeyA := descriptor.FromOCI(descA) + nodeKeyB := descriptor.FromOCI(descB) + nodeKeyC := descriptor.FromOCI(descC) + nodeKeyD := descriptor.FromOCI(descD) + + // check the information of node A + predecessorsA := testDeletableMemory.predecessors[nodeKeyA] + successorsA := testDeletableMemory.successors[nodeKeyA] + for range predecessorsA { + t.Errorf("predecessors of %s should be empty", "A") + } + if !successorsA.Contains(nodeKeyB) { + t.Errorf("successors of %s should contain %s", "A", "B") + } + if !successorsA.Contains(nodeKeyD) { + t.Errorf("successors of %s should contain %s", "A", "D") + } + + // check the information of node B + predecessorsB := testDeletableMemory.predecessors[nodeKeyB] + successorsB := testDeletableMemory.successors[nodeKeyB] + if !predecessorsB.Contains(nodeKeyA) { + t.Errorf("predecessors of %s should contain %s", "B", "A") + } + if !successorsB.Contains(nodeKeyC) { + t.Errorf("successors of %s should contain %s", "B", "C") + } + if !successorsB.Contains(nodeKeyD) { + t.Errorf("successors of %s should contain %s", "B", "D") + } + + // check the information of node C + predecessorsC := testDeletableMemory.predecessors[nodeKeyC] + successorsC := testDeletableMemory.successors[nodeKeyC] + if !predecessorsC.Contains(nodeKeyB) { + t.Errorf("predecessors of %s should contain %s", "C", "B") + } + for range successorsC { + t.Errorf("successors of %s should be empty", "C") + } + + // check the information of node D + predecessorsD := testDeletableMemory.predecessors[nodeKeyD] + successorsD := testDeletableMemory.successors[nodeKeyD] + if !predecessorsD.Contains(nodeKeyB) { + t.Errorf("predecessors of %s should contain %s", "D", "B") + } + if !predecessorsD.Contains(nodeKeyA) { + t.Errorf("predecessors of %s should contain %s", "D", "A") + } + for range successorsD { + t.Errorf("successors of %s should be empty", "C") + } + + // remove node B and check the stored information + testDeletableMemory.Remove(ctx, descB) + if predecessorsC.Contains(nodeKeyB) { + t.Errorf("predecessors of %s should not contain %s", "C", "B") + } + if predecessorsD.Contains(nodeKeyB) { + t.Errorf("predecessors of %s should not contain %s", "D", "B") + } + if !successorsA.Contains(nodeKeyB) { + t.Errorf("successors of %s should still contain %s", "A", "B") + } + if _, exists := testDeletableMemory.successors[nodeKeyB]; exists { + t.Errorf("testDeletableMemory.successors should not contain the entry of %v", "B") + } + + // remove node A and check the stored information + testDeletableMemory.Remove(ctx, descA) + if predecessorsD.Contains(nodeKeyA) { + t.Errorf("predecessors of %s should not contain %s", "D", "A") + } + if _, exists := testDeletableMemory.successors[nodeKeyA]; exists { + t.Errorf("testDeletableMemory.successors should not contain the entry of %v", "A") + } +} + +func TestDeletableMemory_IndexAllAndPredecessors(t *testing.T) { + testFetcher := cas.NewMemory() + testDeletableMemory := NewDeletableMemory() + ctx := context.Background() + + // generate test content + var blobs [][]byte + var descriptors []ocispec.Descriptor + appendBlob := func(mediaType string, blob []byte) ocispec.Descriptor { + blobs = append(blobs, blob) + descriptors = append(descriptors, ocispec.Descriptor{ + MediaType: mediaType, + Digest: digest.FromBytes(blob), + Size: int64(len(blob)), + }) + return descriptors[len(descriptors)-1] + } + generateManifest := func(layers ...ocispec.Descriptor) ocispec.Descriptor { + manifest := ocispec.Manifest{ + Layers: layers, + } + manifestJSON, err := json.Marshal(manifest) + if err != nil { + t.Fatal(err) + } + return appendBlob(ocispec.MediaTypeImageManifest, manifestJSON) + } + generateIndex := func(manifests ...ocispec.Descriptor) ocispec.Descriptor { + index := ocispec.Index{ + Manifests: manifests, + } + indexJSON, err := json.Marshal(index) + if err != nil { + t.Fatal(err) + } + return appendBlob(ocispec.MediaTypeImageIndex, indexJSON) + } + descE := appendBlob("layer node E", []byte("Node E is a layer")) // blobs[0], layer "E" + descF := appendBlob("layer node F", []byte("Node F is a layer")) // blobs[1], layer "F" + descB := generateManifest(descriptors[0:1]...) // blobs[2], manifest "B" + descC := generateManifest(descriptors[0:2]...) // blobs[3], manifest "C" + descD := generateManifest(descriptors[1:2]...) // blobs[4], manifest "D" + descA := generateIndex(descriptors[2:5]...) // blobs[5], index "A" + testFetcher.Push(ctx, descA, bytes.NewReader(blobs[5])) + testFetcher.Push(ctx, descB, bytes.NewReader(blobs[2])) + testFetcher.Push(ctx, descC, bytes.NewReader(blobs[3])) + testFetcher.Push(ctx, descD, bytes.NewReader(blobs[4])) + testFetcher.Push(ctx, descE, bytes.NewReader(blobs[0])) + testFetcher.Push(ctx, descF, bytes.NewReader(blobs[1])) + + // make sure that testFetcher works + rc, err := testFetcher.Fetch(ctx, descA) + if err != nil { + t.Errorf("testFetcher.Fetch() error = %v", err) + } + got, err := io.ReadAll(rc) + if err != nil { + t.Errorf("testFetcher.Fetch().Read() error = %v", err) + } + err = rc.Close() + if err != nil { + t.Errorf("testFetcher.Fetch().Close() error = %v", err) + } + if !bytes.Equal(got, blobs[5]) { + t.Errorf("testFetcher.Fetch() = %v, want %v", got, blobs[4]) + } + + // index node A into graph.DeletableMemory using IndexAll + testDeletableMemory.IndexAll(ctx, testFetcher, descA) + + // check that the content of testDeletableMemory.predecessors and successors + // are correct + nodeKeyA := descriptor.FromOCI(descA) + nodeKeyB := descriptor.FromOCI(descB) + nodeKeyC := descriptor.FromOCI(descC) + nodeKeyD := descriptor.FromOCI(descD) + nodeKeyE := descriptor.FromOCI(descE) + nodeKeyF := descriptor.FromOCI(descF) + + // check the information of node A + predecessorsA := testDeletableMemory.predecessors[nodeKeyA] + successorsA := testDeletableMemory.successors[nodeKeyA] + for range predecessorsA { + t.Errorf("predecessors of %s should be empty", "A") + } + if !successorsA.Contains(nodeKeyB) { + t.Errorf("successors of %s should contain %s", "A", "B") + } + if !successorsA.Contains(nodeKeyC) { + t.Errorf("successors of %s should contain %s", "A", "C") + } + if !successorsA.Contains(nodeKeyD) { + t.Errorf("successors of %s should contain %s", "A", "D") + } + + // check the information of node B + predecessorsB := testDeletableMemory.predecessors[nodeKeyB] + successorsB := testDeletableMemory.successors[nodeKeyB] + if !predecessorsB.Contains(nodeKeyA) { + t.Errorf("predecessors of %s should contain %s", "B", "A") + } + if !successorsB.Contains(nodeKeyE) { + t.Errorf("successors of %s should contain %s", "B", "E") + } + + // check the information of node C + predecessorsC := testDeletableMemory.predecessors[nodeKeyC] + successorsC := testDeletableMemory.successors[nodeKeyC] + if !predecessorsC.Contains(nodeKeyA) { + t.Errorf("predecessors of %s should contain %s", "C", "A") + } + if !successorsC.Contains(nodeKeyE) { + t.Errorf("successors of %s should contain %s", "C", "E") + } + if !successorsC.Contains(nodeKeyF) { + t.Errorf("successors of %s should contain %s", "C", "F") + } + + // check the information of node D + predecessorsD := testDeletableMemory.predecessors[nodeKeyD] + successorsD := testDeletableMemory.successors[nodeKeyD] + if !predecessorsD.Contains(nodeKeyA) { + t.Errorf("predecessors of %s should contain %s", "D", "A") + } + if !successorsD.Contains(nodeKeyF) { + t.Errorf("successors of %s should contain %s", "D", "F") + } + + // check the information of node E + predecessorsE := testDeletableMemory.predecessors[nodeKeyE] + successorsE := testDeletableMemory.successors[nodeKeyE] + if !predecessorsE.Contains(nodeKeyB) { + t.Errorf("predecessors of %s should contain %s", "E", "B") + } + if !predecessorsE.Contains(nodeKeyC) { + t.Errorf("predecessors of %s should contain %s", "E", "C") + } + for range successorsE { + t.Errorf("successors of %s should be empty", "E") + } + + // check the information of node F + predecessorsF := testDeletableMemory.predecessors[nodeKeyF] + successorsF := testDeletableMemory.successors[nodeKeyF] + if !predecessorsF.Contains(nodeKeyC) { + t.Errorf("predecessors of %s should contain %s", "F", "C") + } + if !predecessorsF.Contains(nodeKeyD) { + t.Errorf("predecessors of %s should contain %s", "F", "D") + } + for range successorsF { + t.Errorf("successors of %s should be empty", "E") + } + + // check the Predecessors + predsC, err := testDeletableMemory.Predecessors(ctx, descC) + if err != nil { + t.Errorf("testFetcher.Predecessors() error = %v", err) + } + if len(predsC) != 1 { + t.Errorf("%s should have length %d", "predsC", 1) + } + if !reflect.DeepEqual(predsC[0], descA) { + t.Errorf("incorrect predecessor result") + } + + // TODO: change it to G + + // // remove node B and check the stored information + // testDeletableMemory.Remove(ctx, descB) + // if predecessorsC.Contains(nodeKeyB) { + // t.Errorf("predecessors of %s should not contain %s", "C", "B") + // } + // if predecessorsD.Contains(nodeKeyB) { + // t.Errorf("predecessors of %s should not contain %s", "D", "B") + // } + // if !successorsA.Contains(nodeKeyB) { + // t.Errorf("successors of %s should still contain %s", "A", "B") + // } + // if _, exists := testDeletableMemory.successors[nodeKeyB]; exists { + // t.Errorf("testDeletableMemory.successors should not contain the entry of %v", "B") + // } + + // // remove node A and check the stored information + // testDeletableMemory.Remove(ctx, descA) + // if predecessorsD.Contains(nodeKeyA) { + // t.Errorf("predecessors of %s should not contain %s", "D", "A") + // } + // if _, exists := testDeletableMemory.successors[nodeKeyA]; exists { + // t.Errorf("testDeletableMemory.successors should not contain the entry of %v", "A") + // } +} From 6a4ade05483fc6737fd3d7a17630445d2ca01c82 Mon Sep 17 00:00:00 2001 From: Xiaoxuan Wang Date: Mon, 25 Sep 2023 08:47:05 +0000 Subject: [PATCH 26/26] removed an outdated comment Signed-off-by: Xiaoxuan Wang --- content/oci/storage.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/content/oci/storage.go b/content/oci/storage.go index 98df6428..6acf5b34 100644 --- a/content/oci/storage.go +++ b/content/oci/storage.go @@ -106,8 +106,7 @@ func (s *Storage) Push(_ context.Context, expected ocispec.Descriptor, content i return nil } -// Delete removes the target from the system. With Delete, Storage implements the -// DeletableStorage interface. +// Delete removes the target from the system. func (s *Storage) Delete(ctx context.Context, target ocispec.Descriptor) error { path, err := blobPath(target.Digest) if err != nil {