Refactor cache metadata interface. by sipsma · Pull Request #2316 · moby/buildkit

sipsma · 2021-08-17T20:50:26Z

There are a few goals with this refactor:

Remove external access to fields that no longer make sense and/or
won't make sense soon due to other potential changes. For example,
there can now be multiple blobs associated with a ref (for different
compression types), so the fact that you could access the "Blob"
field from the Info method on Ref incorrectly implied there was just
a single blob for the ref. This is on top of the fact that there is
no need for external access to blob digests.
Centralize use of cache metadata inside the cache package.
Previously, many parts of the code outside the cache package could
obtain the bolt storage item for any ref and read/write it directly.
This made it hard to understand what fields are used and when. Now,
the Metadata method has been removed from the Ref interface and
replaced with getters+setters for metadata fields we want to expose
outside the package, which makes it much easier to track and
understand. Similar changes have been made to the metadata search
interface.
Use a consistent getter+setter interface for metadata, replacing
the mix of interfaces like Metadata(), Size(), Info() and other
inconsistencies.

Signed-off-by: Erik Sipsma erik@sipsma.dev

tonistiigi · 2021-08-23T03:56:01Z

cache/metadata.go

+const gitSnapshotIndex = keyGitSnapshot + "::"

-const keyDeleted = "cache.deleted"
+type MetadataStore interface {


Don't like this. Cache/metadata package should not know about every single possible caller it might have. A new caller can define a unique name for itself and get access to store information. If there are helper methods they should be on caller side. Same for GetSharedKey/GetGitRemote/GetGitSnapshot/Etag methods.

I liked it because it made it made it possible to easily know all the metadata that's being tracked for a given cache record by just checking one place (in the cache package) rather than searching all over the codebase. Understanding what metadata is being tracked for a given record can obviously be important when you are making behavioral changes to the cache package.

If you disagree with the approach, what would you think of exposing the generic methods for setting/getting arbitrary values and then having outside callers just use those? While not as convenient, you can at least still find usages of those generic getters/setters to track down metadata that's being associate with records.

know all the metadata that's being tracked

There shouldn't be any need to manage this together. A caller only knows about its use case, it should not have dependency/knowledge of other callers. If new callers are added or removed it should not concern cache pkg or other callers. Cache package just provides an arbitrary amount of callers access to persistent storage for a ref.

what would you think of exposing the generic methods for setting/getting arbitrary values

Yes, generic helpers without fixed keys are fine. Helpers with fixed keys are also fine on caller side (should be private though if they are only used by one package).

tonistiigi · 2021-08-23T03:57:29Z

cache/metadata.go

-		return si.SetValue(b, keyDiffID, v)
-	})
-	return nil
+type Metadata interface {


This is more like RefMetadata now. No changes needed if you disagree.

Sure, that clarifies that it's specific to a record rather than metadata for the cache as a whole, will update

tonistiigi · 2021-08-23T04:01:51Z

cache/manager.go

+	if err := MigrateV2(
+		context.TODO(),
+		filepath.Join(opt.MetadataStoreRoot, "metadata.db"),
+		filepath.Join(opt.MetadataStoreRoot, "metadata_v2.db"),


Not sure about this change. Why would the cache database need to have a constant name from the package standpoint?

The idea is that the cache package is the only one with direct access to the raw store and then just provides an interface for interacting with it to other packages (as opposed to before where a bunch of different packages had direct access to the raw store). It still needs to be told where to put the data on disk though, so I switched to this parameter.

This approach makes more sense to me but if you disagree I don't have any issue with switching back to passing the store as long as NewManager is the only place it gets passed to.

It doesn't make sense to me that you can't create two instances of manager with metadata.db in same dir. It is some internal structure limitation that leaks to the public API. I'm ok with for example just adding MetadataDBV1 or smth in manageropt if you want to move migration here. It is bit weird though that all other stores are initialized and passed in as interface, and these are now passed as strings.

tonistiigi · 2021-08-23T04:06:10Z

cache/refs.go

 type Ref interface {
 	Mountable
-	ID() string
+	Metadata


No change needed, but I kind of liked it as separate from the rest of the implementation. Guess it can still be nullable if implementation just defines embedded interface as well.

I just found that having all the getters/setters as methods on refs rather than detached functions made it way easier to write code and remember what metadata exists. It works much better with IDE autocompletion and is slightly less verbose.

tonistiigi · 2021-08-23T04:08:02Z

cache/refs.go

 	Release(context.Context) error
-	Size(ctx context.Context) (int64, error)
-	Metadata() *metadata.StorageItem
-	IdentityMapping() *idtools.IdentityMapping


Why is IdentityMapping removed. It is not fully connected (used atm only in moby) but seems like required data

I guess this can be accessed after Mount is called from snapshot.Mountable. Not sure if this is enough for all cases or something might need this info without actually doing a mount.

Yeah I didn't see that it was ever used so I got rid of it. If moby needs it though then I'll just re-add it, not a big deal at all.

tonistiigi · 2021-08-24T23:47:07Z

cache/metadata.go

+	GetString(string) string
+	GetExternal(string) ([]byte, error)
+
+	SetValue(string, interface{}, string) error


Where is this used where it requires to be public and interface{}? Current interface is weird as there is GetString/SetValue that don't have a matching getter/setter.

Agreed it didn't make sense anymore now that the methods were public, changed to SetString

tonistiigi · 2021-08-24T23:49:15Z

cache/metadata.go

+	GetExternal(string) ([]byte, error)
+
+	SetValue(string, interface{}, string) error
+	SetExternal(string, []byte) error


Pair with GetExternal

sipsma · 2021-08-25T00:44:09Z

There was a single random test failure in CI: https://gist.github.com/sipsma/a171fd146fa01cee69175e5a69ca793c

Failure is from this line:

buildkit/client/client_test.go

Line 2665 in f314c4b

checkAllRemoved(t, c, sb)

I can't reproduce locally, even running with -count=20 so not sure if it's related to any changes here or a pre-existing rare failure. Retrying CI to see if it happens again there for some reason.

There are a few goals with this refactor: 1. Remove external access to fields that no longer make sense and/or won't make sense soon due to other potential changes. For example, there can now be multiple blobs associated with a ref (for different compression types), so the fact that you could access the "Blob" field from the Info method on Ref incorrectly implied there was just a single blob for the ref. This is on top of the fact that there is no need for external access to blob digests. 2. Centralize use of cache metadata inside the cache package. Previously, many parts of the code outside the cache package could obtain the bolt storage item for any ref and read/write it directly. This made it hard to understand what fields are used and when. Now, the Metadata method has been removed from the Ref interface and replaced with getters+setters for metadata fields we want to expose outside the package, which makes it much easier to track and understand. Similar changes have been made to the metadata search interface. 3. Use a consistent getter+setter interface for metadata, replacing the mix of interfaces like Metadata(), Size(), Info() and other inconsistencies. Signed-off-by: Erik Sipsma <erik@sipsma.dev>

sipsma · 2021-08-25T19:52:51Z

Rebased. There was another ephemeral test failure on a previous run, different from the one I mentioned previously: https://gist.github.com/sipsma/593107a30f4886ff2b3dbb02a4862361

I looked around and can't see any clear way those failure could be related to the changes here and can't reproduce them locally after many runs. I also looked through past CI runs and found similar failures from before this PR:

So I don't think it's likely those ephemeral failures are related to the change here.

tonistiigi · 2021-08-25T20:43:14Z

@ktock #2316 (comment)

sipsma requested a review from tonistiigi August 17, 2021 20:50

tonistiigi reviewed Aug 23, 2021

View reviewed changes

sipsma force-pushed the rm-info branch 2 times, most recently from 0336182 to 8cbffe8 Compare August 23, 2021 23:15

sipsma requested a review from tonistiigi August 24, 2021 00:46

tonistiigi reviewed Aug 24, 2021

View reviewed changes

sipsma force-pushed the rm-info branch from 8cbffe8 to b30fb59 Compare August 25, 2021 00:12

sipsma requested a review from tonistiigi August 25, 2021 01:09

tonistiigi approved these changes Aug 25, 2021

View reviewed changes

sipsma force-pushed the rm-info branch from b30fb59 to a9f1980 Compare August 25, 2021 19:23

tonistiigi merged commit 89ebbe5 into moby:master Aug 25, 2021

crazy-max mentioned this pull request Oct 25, 2021

vendor buildkit v0.9.1-0.20211025222436-33fb83eb7166 moby/moby#42968

Closed

crazy-max added this to the v0.10.0 milestone Feb 4, 2022

Conversation

sipsma commented Aug 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipsma commented Aug 25, 2021

Uh oh!

sipsma commented Aug 25, 2021

Uh oh!

tonistiigi commented Aug 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants