[WIP] Implement Delete for OCI Layout #582
Conversation
wangxiaoxuan273
requested review from
sajayantony,
shizhMSFT,
SteveLasker and
Wwwsylvia
as code owners
wangxiaoxuan273
changed the title
internal/graph/memory.go
Outdated
| // There is no data consistency issue as long as deletion is not implemented | ||
| // for the underlying storage. |
There was a problem hiding this comment.
Here's the note to RemoveFromIndex.
There was a problem hiding this comment.
Moved the lock to the DeletableStore level.
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #582 +/- ##
==========================================
- Coverage 74.64% 72.63% -2.02%
==========================================
Files 59 62 +3
Lines 5266 5635 +369
==========================================
+ Hits 3931 4093 +162
- Misses 983 1162 +179
- Partials 352 380 +28
|
wangxiaoxuan273
force-pushed
the
draft-oci-delete
branch
from
c0ccb7d to
dd0282d
Compare
internal/graph/memory.go
Outdated
| @@ -31,6 +31,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 | |||
There was a problem hiding this comment.
Since the successor list of a node is immutable and we don't need random access on the successors, the mapping can be from a descriptor to a successor list (map[descriptor.Descriptor][]ocispec.Descriptor).
There was a problem hiding this comment.
changed both successors and predecessors to the regular map, and added locks in index and Remove.
internal/graph/memory.go
Outdated
| key := descriptor.FromOCI(node) | ||
| m.predecessors.LoadOrStore(key, &sync.Map{}) | ||
| m.successors.LoadOrStore(key, &sync.Map{}) | ||
| m.indexed.LoadOrStore(key, &sync.Map{}) |
There was a problem hiding this comment.
We should mark a node as indexed after it gets indexed.
There was a problem hiding this comment.
I think the node's presence in indexed means it has been indexed? Do we need an extra mark?
There was a problem hiding this comment.
The current implementation marks the node as indexed before it gets indexed.
There was a problem hiding this comment.
Good point. I did not notice that I changed the behavior of indexed from the original behavior. Changed it back now. Currently indexed is updated after the node gets indexed.
There was a problem hiding this comment.
The variable indexed has been removed now.
internal/graph/memory.go
Outdated
| } | ||
| } | ||
|
|
||
| func (m *Memory) indexIntoMemory(ctx context.Context, node ocispec.Descriptor) { |
There was a problem hiding this comment.
Why is it called indexIntoMemory? Everything is in memory in this file. 🤔
There was a problem hiding this comment.
Renamed it as createEntriesInMaps.
internal/graph/memory.go
Outdated
|
|
||
| func (m *Memory) removeFromMemory(ctx context.Context, node ocispec.Descriptor) { | ||
| key := descriptor.FromOCI(node) | ||
| m.predecessors.Delete(key) |
There was a problem hiding this comment.
We might not want to delete the key from predecessors when a node is deleted.
There was a problem hiding this comment.
Why not? Do we have a scenario of still needing it after the node is deleted? The information is stored in the node's predecessors' successors anyway.
There was a problem hiding this comment.
To get the predecessors of a deleted node.
There was a problem hiding this comment.
Added the entry back. Currently we don't delete the entry in predecessors when doing Delete.
content/oci/deletableOci.go
Outdated
|
|
||
| // loadIndexFile reads index.json from the file system. | ||
| // Create index.json if it does not exist. | ||
| func (ds *DeletableStore) loadIndexFile(ctx context.Context) error { |
There was a problem hiding this comment.
Looks like this part was not locked (I can't recall why not, probably a bug😟). Should we lock it?
There was a problem hiding this comment.
Oh it was because loadIndexFile is only called on initialization. We need a comment for this then. (I wrote this code bug I already forget it😅)
There was a problem hiding this comment.
I don't think we need to. This function is only used in New() and I guess it's called only once?
There was a problem hiding this comment.
Yeah but people (like me) might not realize that😂
internal/graph/memory.go
Outdated
| // RemoveFromIndex removes the node from its predecessors and successors. | ||
| func (m *Memory) RemoveFromIndex(ctx context.Context, node ocispec.Descriptor) error { |
There was a problem hiding this comment.
It seems that we just call it Remove.
There was a problem hiding this comment.
Renamed as Remove.
internal/graph/memory.go
Outdated
| @@ -116,19 +117,54 @@ 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 { | |||
There was a problem hiding this comment.
There is a race condition where the same node is indexed and removed at the same time.
There was a problem hiding this comment.
For example, what if another goroutine calls Index() when the current goroutine just about to hit the line 132?
wangxiaoxuan273
force-pushed
the
draft-oci-delete
branch
from
eb3b438 to
7731e12
Compare
| ) | ||
|
|
||
| // 条件： | ||
| // 1. 只要node被index（作为函数的第二个输入），node就在predecessors，successors，indexed中有entry |
There was a problem hiding this comment.
Can you please translate the comments to English? =)
There was a problem hiding this comment.
I will remove it or put it formally in English in later versions. Currently this is a draft PR, and I wrote this comment to remind myself the designs, and the designs may change anytime.
wangxiaoxuan273
force-pushed
the
draft-oci-delete
branch
from
31bc59f to
e69b610
Compare
internal/graph/memoryWithDelete.go
Outdated
| 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 |
There was a problem hiding this comment.
Can this be of type map[descriptor.Descriptor][]ocispec.Descriptor?
See #582 (comment)
internal/graph/memoryWithDelete.go
Outdated
|
|
||
| // MemoryWithDelete is a MemoryWithDelete based PredecessorFinder. | ||
| type MemoryWithDelete struct { | ||
| indexed sync.Map // map[descriptor.Descriptor]any, this variable is only used by IndexAll |
There was a problem hiding this comment.
Looks like this can be moved into IndexAll.
There was a problem hiding this comment.
I would like to avoid making changes to IndexAll, until I'm sure it's safe to do it. I still have questions about the behavior of IndexAll.
internal/graph/memoryWithDelete.go
Outdated
| _, exists := m.predecessors[key] | ||
| if !exists { | ||
| return nil, nil | ||
| } | ||
| var res []ocispec.Descriptor | ||
| for _, v := range m.predecessors[key] { | ||
| res = append(res, v) | ||
| } |
There was a problem hiding this comment.
nit:
| _, exists := m.predecessors[key] | |
| if !exists { | |
| return nil, nil | |
| } | |
| var res []ocispec.Descriptor | |
| for _, v := range m.predecessors[key] { | |
| res = append(res, v) | |
| } | |
| predecessors, exists := m.predecessors[key] | |
| if !exists { | |
| return nil, nil | |
| } | |
| var res []ocispec.Descriptor | |
| for _, v := range predecessors { | |
| res = append(res, v) | |
| } |
?
internal/graph/memoryWithDelete.go
Outdated
|
|
||
| func (m *MemoryWithDelete) createEntriesInMaps(ctx context.Context, node ocispec.Descriptor) { | ||
| key := descriptor.FromOCI(node) | ||
| if _, hasEntry := m.predecessors[key]; !hasEntry { |
There was a problem hiding this comment.
nit: a simple ok should be good enough.
internal/graph/memoryWithDelete.go
Outdated
| // 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) |
There was a problem hiding this comment.
I don't think we need createEntriesInMaps and removeEntriesFromMaps as they are used in only one place. Just expanding them may make the caller function more readable?
There was a problem hiding this comment.
I think it's better to have these two functions for maintainability and modularity. The exact behaviors of predecessors, successors and other variables at indexing and deleting may change later as we continue to improve our design, and these two functions reduce the chance of errors.
I originally did not have these two functions, but as I keep working on this feature, I've found them being functions quite useful.
content/oci/readonlyoci.go
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
This should be defined in deletableOci.go as it has nothing to do with readonlyoci.go.
There was a problem hiding this comment.
Good point, thanks. Moved it to deletableOci.go.
wangxiaoxuan273
force-pushed
the
draft-oci-delete
branch
2 times, most recently
from
fb078a0 to
91b86d9
Compare
internal/graph/DeletableMemory.go
Outdated
There was a problem hiding this comment.
golang file names use underscores like deletable_memory.go
There was a problem hiding this comment.
Sorry. Updated the file names to be deletableoci.go and deletablememory.go, to be consistent with other file names in the repo.
internal/container/set/set.go
Outdated
| @@ -33,3 +33,8 @@ func (s Set[T]) Contains(item T) bool { | |||
| _, ok := s[item] | |||
| return ok | |||
| } | |||
|
|
|||
| // Delete deletes an item from the set | |||
There was a problem hiding this comment.
nit:
| // Delete deletes an item from the set | |
| // Delete deletes item from the set. |
There was a problem hiding this comment.
Added the period.
internal/graph/deletablememory.go
Outdated
| } | ||
|
|
||
| // Index indexes predecessors for each direct successor of the given node. | ||
| // There is no data consistency issue as long as deletion is not implemented |
There was a problem hiding this comment.
This comment is no longer valid.
There was a problem hiding this comment.
Removed the comment.
| m.lock.RLock() | ||
| defer m.lock.RUnlock() | ||
| key := descriptor.FromOCI(node) | ||
| set, exists := m.predecessors[key] |
There was a problem hiding this comment.
nit: would it be better to call it predecessors instead of set?
| } | ||
|
|
||
| // loadIndexInDeletableMemory loads index into the memory. | ||
| func loadIndexInDeletableMemory(ctx context.Context, index *ocispec.Index, fetcher content.Fetcher, tagger content.Tagger, graph *graph.DeletableMemory) error { |
There was a problem hiding this comment.
If we want to maintain DeletableStore separately, this function can be renamed to
func (ds *DeletableStore) loadIndex since it is only used by DeletableStore.
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
wangxiaoxuan273
force-pushed
the
draft-oci-delete
branch
from
93945ab to
cb8749e
Compare
Signed-off-by: Xiaoxuan Wang <[email protected]>
Part 2/4 of #454 Based on draft PR #582 --------- Signed-off-by: Xiaoxuan Wang <[email protected]> Co-authored-by: Terry Howe <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]> Part 3/4 of #454 Based on draft PR #582 --------- Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]> Part 1/4 of #454 Based on draft PR #582 Current behavior regarding `graph.Memory.Remove(node)`: * `node` entry in `m.successors` and `m.nodes` is removed. * `node` is removed from its successors predecessors list. * `node` entry in `m.predecessors` is NOT removed, **unless all its predecessors no longer exist**. * `node` is NOT removed from its predecessors' `m.successors` list. The `m.successors` is always in accordance with the actual content. Signed-off-by: Xiaoxuan Wang <[email protected]>
Attempt to resolve #454
Based on the implementation of #470, but with
RWMutex.DeletableStoreis basically copied fromoci.Storewith an addedRWMutex.DeletableMemoryis basically copied fromgraph.Memorywith an addedRWMutex.