Add the metadata store interface and an in-memory implementation. #9

Random-Liu · 2017-04-18T05:01:26Z

For #6.

This PR added:

The metadata store interface;
An in-memory implementation;
A sandbox metadata store based on the metadata store.

In the future, we need to have an on-disk implementation. The TODOs in memory_store.go describes how to implement a file-based checkpoint.

The metadata store interface is defined as follows:

// All byte arrays are expected to be read-only. User MUST NOT modify byte
// array element directly!!

// UpdateFunc is function used to update a specific metadata. The value
// passed in is the current value. The updated value should be returned.
// If there is an error, the update will be rolled back.
type UpdateFunc func([]byte) ([]byte, error)

// Metadata is a metadata in the store. It supports transactional update.
type Metadata interface {
	// Get a snapshot of the metadata.
	Get() ([]byte, error)
	// Update the value.
	Update(UpdateFunc) error
}

// MetadataStore is the interface for storing metadata.
type MetadataStore interface {
	// Create the metadata with the specified id.
	// Note:
	// * Create MUST return error if the id already exists.
	// * The id and metadata MUST be added in one transaction.
	Create(string, []byte) (Metadata, error)
	// Get the metadata by id.
	Get(string) (Metadata, error)
	// List all the metadatas.
	List() ([]Metadata, error)
	// Delete the metadata by id.
	// Note:
	// * Delete MUST not return error if the id doesn't exist.
	// * The id and metadata MUST be deleted in one transaction.
	Delete(string) error
}

/cc @kubernetes-incubator/maintainers-cri-containerd @feiskyer @resouer

Random-Liu · 2017-04-19T07:03:31Z

Change to WIP, I'll firstly try to use the metadata store in the sandbox lifecycle management to see whether the interface is sufficient.

Random-Liu · 2017-04-21T00:02:48Z

@yujuhong @dchen1107

The metadata store will be very important for the stability of cri-containerd;
And this is the first time we will do checkpoint.

Please take a look at this PR to make sure the design makes sense to you.

I'll add unit test after the design is reviewed.

mikebrow

Just nits.. would need to see it in action though since there's so many spinning wheels I might've missed something in my read only review.

mikebrow · 2017-04-24T23:45:40Z

pkg/metadata/sandbox.go

+// version and versionedSandboxMetadata is not used now, but should be used
+// in the future:
+// 1) Only versioned metadata should be written into the store;
+// 2) Only unversioned metadata should be returned to the user;


why hide the version info?

Because I don't think the caller want to care about the version.

All version stuff should be handled inside the metadata store.

mikebrow · 2017-04-24T23:58:14Z

pkg/metadata/store/store.go

+	List() ([]Metadata, error)
+	// Delete the metadata by id.
+	// Note:
+	// * Delete MUST not return error if the id doesn't exist.


I just mean Delete should be idempotent, it should not return error when corresponding metadata doesn't exist.

I see so:
// Delete(id) returns error = nil when there is no metadata for the id or when the metadata has been successfully deleted. Otherwise Delete(id) returns an error !=nil when there is metadata but the delete operation has failed...

mikebrow · 2017-04-24T23:59:44Z

pkg/metadata/store/store.go

+	Create(string, []byte) (Metadata, error)
+	// Get the metadata by id.
+	Get(string) (Metadata, error)
+	// List all the metadatas.


return entire array of Metadata from MetadataStore

mikebrow · 2017-04-25T00:02:16Z

pkg/metadata/store/store.go

+	Get(string) (Metadata, error)
+	// List all the metadatas.
+	List() ([]Metadata, error)
+	// Delete the metadata by id.


/s/metadata/Metadata/

mikebrow · 2017-04-25T00:03:37Z

pkg/metadata/store/store.go

+	// * Create MUST return error if the id already exists.
+	// * The id and metadata MUST be added in one transaction.
+	Create(string, []byte) (Metadata, error)
+	// Get the metadata by id.


/s/metadata/Metadata/

mikebrow · 2017-04-25T00:05:07Z

pkg/metadata/store/store.go

+// []byte with interface{}, so as to avoid extra marshal/unmarshal on the
+// user side.
+type MetadataStore interface {
+	// Create the metadata with the specified id.


/s/the metadata with the specified id/an instance of Metadata with the specified id containing the array of bytes passed./

mikebrow · 2017-04-25T00:11:14Z

pkg/metadata/store/store.go

+	// Create the metadata with the specified id.
+	// Note:
+	// * Create MUST return error if the id already exists.
+	// * The id and metadata MUST be added in one transaction.


/s/metadata/the instance of Metadata/

/s/./to the MetadataStore./

mikebrow · 2017-04-25T00:19:10Z

pkg/metadata/store/store.go

+// array element directly!!
+
+// UpdateFunc is function used to update a specific metadata. The value
+// passed in is the current value. The updated value should be returned.


/s/current/new/

Nope, it's the old value. I'll explicitly say "old" here. :)

Random-Liu · 2017-04-26T01:30:07Z

Added unit test, PR ready for review.

mikebrow · 2017-04-26T19:38:39Z

pkg/metadata/sandbox_test.go

+	sbs, err = s.List()
+	assert.NoError(err)
+	assert.Len(sbs, 2)
+	meta, err := s.Get(testID)


t.Logf("after deleting metadata for id Get for that id should return nil for metadata and with no error")

mikebrow · 2017-04-26T19:42:03Z

pkg/metadata/sandbox_test.go

+	meta, err := s.Get(testID)
+	assert.NoError(err)
+	assert.True(meta == nil)
+}


I'd add another test that creates the metadata again for id.. is that the expected pattern? or are we only deleting the metadata as we delete a sandbox?

Will only add the case into the basic metadata store.

We'll have SandboxStore, ContainerStore and ImageStore. They will be sharing the same underlying basic metadata store. If we add full test coverage for all these 3 stores, there will be a lot of duplicated code. (Golang doesn't support something like C++ template... :()

So let's add more test coverage in the basic metadata store, and only basic and necessary case at this level.

mikebrow

question on the delete ...

mikebrow · 2017-04-26T19:50:48Z

pkg/metadata/store/metadata_store.go

+
+// delete deletes the data on disk atomically.
+func (m *memoryMeta) delete() error {
+	// TODO(random-liu): Hold write lock, rename the data on the disk.


m.data = nil?

Option 1: If we set m.data=nil, other operation (e.g. ListPodSandbox, PodSandboxStatus) which has already got the metadata reference will fail, and return error.

Option 2: If we don't set m.data=nil, other operation which has got the reference will success:

If that operation is read only, it will return staled data.

If that operation updates the metadata, it will only change the in-memory state.

Any future operations won't be able to get the metadata because it is removed from the list already.

Either option 1 or 2 is acceptable to me, because this is unavoidable race unless we lock the whole store for each single metadata operation, which is not what we want. :)

I prefer option 2. WDYT?

Is there a door number 3? I suppose we should talk about this. Maybe defer calls to delete?

What do you mean by "defer calls to delete"?

If deleting metadata can't be done due to dangling references one could treat the request as a deferred request to delete.. or perhaps direct that delete should only be called like this "defer meta.delete()" and in such a manner as the dangling references will have been removed first.

guess I don't get the part about the call is only for deleting data on disk.. while leaving it around for further use.. Maybe a quick phone call.

guess I don't get the part about the call is only for deleting data on disk.. while leaving it around for further use.. Maybe a quick phone call.

Actually the data is already removed from the store, that is where the deletion happens. It's already not accessible by future operations.
The only problem is for the operations which have already got the reference, they could either fail or return/update staled data, because there is no order guarantee for 2 concurrent operations.

Maybe I should change the name to deleteCheckpoint or something to make it more clear.

right by further I meant.. continued use by existing references..

yes deleteCheckpoint works! Then just some comments to explain it and we're good

@mikebrow OK. Will update the PR. :)

mikebrow · 2017-04-26T19:52:37Z

pkg/metadata/store/metadata_store_test.go

+	assert.Equal(testData[0], old)
+
+	// TODO(random-liu): Test delete and cleanup after disk based
+	// implementation is added.


TODO for delete and cleanup?

The TODO is just for this.

Not sure how I missed the TODO text.. odd :-)

mikebrow · 2017-04-26T20:07:53Z

pkg/metadata/store/metadata_store_test.go

+	existingGot, err := existingMeta.Get()
+	assert.NoError(err)
+	assert.Equal([]byte("updated-metadata-1"), existingGot)
+}


create again after delete?

mikebrow

LGTM

Only thing I would change is to consider refactoring the packaging a bit. For example: the metadata package has a sandbox in it ... might be better if the sandbox was in it's own package; also the metadata/store package has the metadata interface in it .. this might be better in the metadata package; the metadatastore interface could move to the metadata_store.go file..

Random-Liu · 2017-04-29T00:34:26Z

@yujuhong Addressed comments. The code does become cleaner after combine Metadata and MetadataStore.

Will squash the last commits before merging.

mikebrow

LGTM

yujuhong · 2017-04-28T18:20:43Z

pkg/metadata/store/metadata_store.go

+// into files during update. Metadata should serve from memory, but any modification
+// should be checkpointed, so that memory could be recovered after restart. It
+// should be possible to disable the checkpoint for testing.
+// Note that checkpoint update may fail, so the recovery logic should tolerant that.


s/tolerant/tolerate

yujuhong · 2017-05-02T22:08:58Z

pkg/metadata/store/metadata_store.go

+// MetadataStore is the interface for storing metadata.
+// TODO(random-liu): Initialize the metadata store with a type, and replace
+// []byte with interface{}, so as to avoid extra marshal/unmarshal on the
+// user side.


Are these methods thread-safe? Should clarify it in the comment.

yujuhong · 2017-05-02T22:10:38Z

pkg/metadata/store/metadata_store.go

+// UpdateFunc is function used to update a specific metadata. The value
+// passed in is the old value. The updated value should be returned.
+// If there is an error, the update will be rolled back.
+type UpdateFunc func([]byte) ([]byte, error)


The purpose of the UpdateFunc is unclear to me. Is it supposed to consume the existing/old data, modify, and output the new data? If that's the case, it's merely modifying an in-memory copy of the data. How would it guarantee a rollback, like the comment suggested? Rollback should be a feature provided by the MetadataStore itself, no?

As is comment above:

// All byte arrays are expected to be read-only. User MUST NOT modify byte // array element directly!!

We could pass in a copy of the current data. However, that introduces extra overhead.

In fact all higher level metadata store will just unmarshal the byte array instead of changing it directly.

Will make the comment more clear.

Updated the comments.

yujuhong · 2017-05-02T22:16:12Z

pkg/metadata/store/metadata_store.go

+// in memory and checkpoint metadata into files during update. Metadata should serve
+// from memory, but any modification should be checkpointed, so that memory could be
+// recovered after restart. It should be possible to disable the checkpoint for testing.
+// Note that checkpoint update may fail, so the recovery logic should tolerant that.


s/tolerant/tolerate

yujuhong · 2017-05-02T22:16:25Z

pkg/metadata/store/metadata_store.go

+// recovered after restart. It should be possible to disable the checkpoint for testing.
+// Note that checkpoint update may fail, so the recovery logic should tolerant that.
+
+// An atomic file operator is needed.


This is a part of TODO. We need atomic file writer to do checkpoint. Could be removed now.

yujuhong · 2017-05-03T00:03:34Z

pkg/metadata/sandbox.go

+	return s.store.Create(metadata.ID, data)
+}
+
+// Get gets a specified sandbox.


yujuhong · 2017-05-03T00:08:18Z

pkg/metadata/store/metadata_store.go

+	return nil
+}
+
+// cleanup cleans up all temporary files left-over.


Why would there be multiple temporary files for a single piece of metadata?

There may be temporary files left over from previous failed update operations.

yujuhong · 2017-05-03T00:09:44Z

pkg/metadata/store/metadata_store.go

+	return nil
+}
+
+// metadataStore is the implementation of MetadataStore


metadataStore implements MetadataStore or metadataStore is an implementation of MetadataStore

yujuhong · 2017-05-03T00:17:54Z

pkg/metadata/sandbox.go

+}
+
+// SandboxStore is the store for metadata of all sandboxes.
+type SandboxStore interface {


Do we need the interface?

Why not an interface? Should we avoid interface?

Not saying we should avoid interfaces, but just asking to see why interface is needed here. Do we expect other implmentations or fakes/mocks for testing?

I don't think we'll have other implementations at this level. But it seems more clear to me to have an interface to tell the caller what operations are supported, instead of going through the code.

May be a misusing. :)

yujuhong · 2017-05-03T00:18:22Z

pkg/metadata/sandbox_test.go

+	t.Logf("get should return nil without error after deletion")
+	meta, err := s.Get(testID)
+	assert.NoError(err)
+	assert.True(meta == nil)


Can use assert.Nil()

Random-Liu · 2017-05-03T04:49:09Z

@yujuhong Addressed comments in the latest commit. Thanks for reviewing!

yujuhong · 2017-05-03T21:03:23Z

LGTM

Random-Liu · 2017-05-03T21:06:36Z

Apply LGTM based on #9 (comment) and #9 (review).
@yujuhong @mikebrow Thanks for reviewing! Will squash and merge this one, and rebase the others! :)

Signed-off-by: Random-Liu <lantaol@google.com>

Add the metadata store interface and an in-memory implementation.

Added symlink resolution for host path and windows platform support in sandbox-platform

Vendor hcsshim commit 041736614696a7394edcb733292688c2bab69ead

k8s-ci-robot added the cncf-cla: yes label Apr 18, 2017

Random-Liu assigned mikebrow Apr 18, 2017

Random-Liu mentioned this pull request Apr 18, 2017

CRI Containerd Integration TODOs #6

Closed

34 tasks

Random-Liu force-pushed the add-metadata-store branch 2 times, most recently from 0c53a26 to db59882 Compare April 19, 2017 06:12

Random-Liu changed the title ~~Add the metadata store interface and an in-memory implementation.~~ [WIP] Add the metadata store interface and an in-memory implementation. Apr 19, 2017

Random-Liu force-pushed the add-metadata-store branch 3 times, most recently from 6a8e318 to 7e743dc Compare April 20, 2017 23:54

Random-Liu force-pushed the add-metadata-store branch from 7e743dc to 210b71e Compare April 21, 2017 00:13

Random-Liu assigned dchen1107 and yujuhong Apr 21, 2017

Random-Liu force-pushed the add-metadata-store branch from 210b71e to 7784624 Compare April 21, 2017 00:15

mikebrow reviewed Apr 25, 2017

View reviewed changes

Random-Liu mentioned this pull request Apr 25, 2017

Add sandbox implementation #19

Merged

Random-Liu force-pushed the add-metadata-store branch 2 times, most recently from a427278 to 45dc29b Compare April 26, 2017 01:28

Random-Liu changed the title ~~[WIP] Add the metadata store interface and an in-memory implementation.~~ Add the metadata store interface and an in-memory implementation. Apr 26, 2017

Random-Liu force-pushed the add-metadata-store branch 4 times, most recently from f2d405e to 1e83709 Compare April 26, 2017 04:54

mikebrow reviewed Apr 26, 2017

View reviewed changes

Random-Liu force-pushed the add-metadata-store branch from cbc7694 to 8b102e7 Compare April 28, 2017 18:50

mikebrow reviewed Apr 28, 2017

View reviewed changes

Random-Liu force-pushed the add-metadata-store branch 3 times, most recently from 5d257f2 to f9c48d3 Compare April 29, 2017 00:58

mikebrow approved these changes Apr 29, 2017

View reviewed changes

Random-Liu mentioned this pull request May 2, 2017

Add container implementation #22

Merged

Random-Liu force-pushed the add-metadata-store branch from f9c48d3 to 6aa2d84 Compare May 2, 2017 22:04

yujuhong reviewed May 3, 2017

View reviewed changes

Random-Liu added the lgtm label May 3, 2017

Random-Liu added 3 commits May 3, 2017 14:09

Add metadata store

3624616

Signed-off-by: Random-Liu <lantaol@google.com>

Add a sandbox metadata store based on the metadata store

0e7fa9d

Signed-off-by: Random-Liu <lantaol@google.com>

Add unit test for metadata store

86997f0

Signed-off-by: Random-Liu <lantaol@google.com>

Random-Liu force-pushed the add-metadata-store branch from 0e31b26 to 86997f0 Compare May 3, 2017 21:09

Random-Liu merged commit 1532079 into containerd:master May 3, 2017

Random-Liu deleted the add-metadata-store branch May 3, 2017 21:18

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#9 from Random-Liu/add-metadata-store

90374cc

Add the metadata store interface and an in-memory implementation.

adelina-t pushed a commit to adelina-t/cri that referenced this pull request Jun 20, 2019

Merge pull request containerd#9 from SaswatB/windows_port

0ceadab

Added symlink resolution for host path and windows platform support in sandbox-platform

dcantah pushed a commit to dcantah/cri that referenced this pull request Jan 7, 2022

Merge pull request containerd#9 from anmaxvl/hcsshim-revendor

d03e9c1

Vendor hcsshim commit 041736614696a7394edcb733292688c2bab69ead

Add the metadata store interface and an in-memory implementation. #9

Add the metadata store interface and an in-memory implementation. #9

Conversation

Random-Liu commented Apr 18, 2017 • edited

Random-Liu commented Apr 19, 2017

Random-Liu commented Apr 21, 2017 • edited

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Apr 25, 2017 • edited

Choose a reason for hiding this comment

Random-Liu commented Apr 26, 2017

mikebrow Apr 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Apr 27, 2017 • edited

Choose a reason for hiding this comment

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Apr 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Apr 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Apr 27, 2017 • edited

Choose a reason for hiding this comment

mikebrow Apr 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow left a comment

Choose a reason for hiding this comment

Random-Liu commented Apr 29, 2017 • edited

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu May 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Apr 18, 2017 •

edited

Random-Liu commented Apr 21, 2017 •

edited

Random-Liu Apr 25, 2017 •

edited

mikebrow Apr 26, 2017 •

edited

Random-Liu Apr 27, 2017 •

edited

Random-Liu Apr 27, 2017 •

edited

mikebrow Apr 27, 2017 •

edited

Random-Liu Apr 27, 2017 •

edited

mikebrow Apr 28, 2017 •

edited

Random-Liu commented Apr 29, 2017 •

edited

Random-Liu May 3, 2017 •

edited

Random-Liu May 3, 2017 •

edited

Random-Liu commented May 3, 2017 •

edited