Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

cache: Refactor out cacheRecord equal* fields #2223

Closed
wants to merge 1 commit into from

Conversation

sipsma
Copy link
Collaborator

@sipsma sipsma commented Jul 6, 2021

The equalMutable and equalImmutable fields were used to implement the
"lazy commit" feature (not to be confused with lazy refs), which allows
refs to put off committing their snapshots to the underlying driver until
absolutely needed.

However, this way of modeling it resulted in there being 2 actual
cacheRecords even when the underlying snapshot was the same. This also
lead to a lot of convoluted logic checking for the existence of the
equal* fields.

This commits switches to a new approach of only maintaining a single
cacheRecord for what used to be separate equal* records. The cacheRecord
internally keeps track of whether it has been finalized or not and
updates which snapshot ID is underlying it.

This results in overall much simpler logic, as can be seen by looking at
the before+after of GetMutable, Commit, prune, and release, among
others. The only meaningfully-sized block of new code added is in
cacheManager.init, which handles migrating records from the old format
and can thus eventually be deleted once the older versions of buildkit
are no longer supported.

This change is backwards compatible because of that migration logic in
cacheManager.init and because MutableRefs have never been saved in the
upper-level solver cache, only ImmutableRefs are there, so removing any
refs that were marked as an equalMutable and merging them into their
equalImmutable will not result in any cache keys unexpectedly missing.

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

  • Marking this as a Draft until I've added a test for the format migration code in cacheManager.init

@tonistiigi Decided to go with this refactor first because I think it improves my ability to understand the cache code and ref lifecycle management the most. I still want to address some of the other stuff we discussed (like cleaning up the metadata and extracting out snapshotter-specific code+logic from the cache code) as separate subsequent PRs, probably partially mixed in with the merge-op PRs.

@sipsma sipsma requested a review from tonistiigi July 6, 2021 06:04
@sipsma sipsma force-pushed the simplify-lazy-commit branch 3 times, most recently from 3b3dce1 to b96c92a Compare July 7, 2021 00:29
@sipsma sipsma marked this pull request as ready for review July 7, 2021 01:06
@sipsma
Copy link
Collaborator Author

sipsma commented Jul 7, 2021

@ktock FYI as this touches on a lot of the code you've been changing lately

cache/refs.go Outdated Show resolved Hide resolved
@sipsma sipsma force-pushed the simplify-lazy-commit branch 2 times, most recently from db6251f to c00efab Compare July 14, 2021 20:21
@sipsma sipsma requested a review from ktock July 14, 2021 20:21
@ktock ktock self-assigned this Jul 15, 2021
Copy link
Collaborator

@ktock ktock left a comment

Choose a reason for hiding this comment

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

please see the following reviews.
(confirmed this patch works with stargz-snapshotter mode as well)

cache/manager.go Outdated Show resolved Hide resolved
cache/refs.go Show resolved Hide resolved
cache/manager.go Show resolved Hide resolved
cache/manager.go Show resolved Hide resolved
cache/manager.go Show resolved Hide resolved
@sipsma
Copy link
Collaborator Author

sipsma commented Jul 15, 2021

please see the following reviews.
(confirmed this patch works with stargz-snapshotter mode as well)

Thanks for testing it!

Copy link
Collaborator

@ktock ktock left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

cache/metadata.go Outdated Show resolved Hide resolved
cache/refs.go Outdated Show resolved Hide resolved
}
// hold cacheRecord.mu lock before calling
func (cr *cacheRecord) release(ctx context.Context, forceKeep bool) error {
if cr.refCount() > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

This is not a case for warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No this is a normal case, it's just checking whether there are still open refs after releasing this one (which is possible because you can have multiple immutable refs open for the same cache record). The equivalent line in the previous code was here:

if len(sr.refs) == 0 {

return sr.commit(ctx)
rec := sr.cacheRecord

ir := &immutableRef{
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that the ID() does not change after commit or am I missing something. That would be problematic for the cache (and other queries) that work on IDs if they are not unique.

Copy link
Collaborator Author

@sipsma sipsma Jul 20, 2021

Choose a reason for hiding this comment

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

Yes it does mean that, that's one of the main simplifications I think, now the underlying snapshot data is only ever represented by a single cache record rather than two different ones depending on whether it's being presented to the outside callers as mutable or immutable. So whereas previously Get and GetMutable would always return entirely different records for the same underlying snapshot data, now they can be called with the same ID to obtain either a immutable or mutable reference, respectively, to that record, which makes more sense IMO and simplifies the code quite a bit.

In terms of being backwards compatible, one important point is that the higher level solver cache never saved a MutableRef, it only ever saved ImmutableRefs. That meant that the migration logic I added in cacheManager.init just needed to handle finding cache records with equalMutable set and moving the snapshot and other metadata over to the now centralized record.

However, I did just realize there are also the various indexes scattered around and used to search for mutable records by git source, local source, exec's shared cache mounts, etc. Those call GetMutable with an ID found by searching the bolt db, so as of right now all those caches would get invalidated after upgrading to a buildkit with this change because those records would be something's equalMutable and thus removed as part of the migration logic in cacheManager.init.

I can fix that issue by just updating init to also migrate the indexes it finds on the equalMutable to the centralized record, which I will do.

Copy link
Member

Choose a reason for hiding this comment

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

important point is that the higher level solver cache never saved a MutableRef, it only ever saved ImmutableRefs

Yes, but it shouldn't mean the ImmutableRef was guaranteed to be finalized. The simple case should be 1) get immutableref 2) save cache for it 3) it becomes mutable again and changes 4) after commit it is the same id for immutableref but different id 4) cache lookup returns the wrong record.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I'm following where a wrong record gets returned by cache lookup. Are you assuming that the snapshot ID and ref ID are always the same? If so, that isn't the case with this change (and actually wasn't the case before this change, though in a smaller set of circumstances). The snapshot ID is now treated as just a piece of metadata tracked for the ref and can get updated during a call to finalize. In the case of a finalized ref, the snapshot ID and ref ID just so happen to actually be the same, but that's just because it made some crash recovery logic a bit simpler to write (see here and here), it's not strictly necessary or assumed anywhere else in the code.

So in the case I think you're describing, here's what happens:

  1. You get an immutable ref, it has ID 123 and snapshotID 456. The snapshot is not committed.
  2. You save that immutable ref for 123 in the cache somewhere and release the ref
  3. You obtain a mutable ref by calling GetMutable with ID set to 123 and change the data on underlying mount for snapshot 456
  4. You call Commit on the mutable ref for 123, turning it into an immutable ref and then finalize that immutable ref. During finalize, the record's snapshot ID is internally updated from 456 to 789
  5. You later get an immutable ref for the record again by calling Get with ID set to 123. When you mount it, you are using snapshot 789 now because the record has migrated to using that finalized snapshot.

Overall, the tradeoff here is that now the snapshot ID for a cache record can change, but I think that makes much more conceptual sense than changing a ref's ID just because the snapshot data got committed to the driver. Callers outside the cache package shouldn't care at all about snapshot IDs, only ref IDs, so having them be aware and handle the fact ref IDs change just because some underlying snapshot ID that they don't know about change felt like it wasn't providing any benefit.

Copy link
Member

Choose a reason for hiding this comment

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

Snapshot IDs are always internal, they don't matter to me. It is the ref ID that gets stored in the build cache boltdb https://github.com/moby/buildkit/tree/master/solver/bboltcachestorage . And yes there is also the case with the search for mutable that you mentioned where currently I think the metadata was different for these records, but I need to look at it more to understand that case.

Copy link
Member

Choose a reason for hiding this comment

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

I actually thought such cases already existed before this change and I also can't think of any reason why we would want to prevent them or make this be a requirement of the interface.

I don't think this problem existed in the previous implementation cause I actually remember hitting these cases when developing this. It might have even been a reason for the equal* design.

For such records, what specific problems could be caused by the underlying data changing when it gets re-opened as mutable (provided the rules about simultaneous opening of immutable and mutable refs are honored)? I thought that was the whole point of the lazy-commit optimization;

Yes, the point of lazy commit is that data can become mutable again. But once it does, the immutable should become invalid. The ID() is how other components(like the build cache system, or incremental context send) query references and store them for persistence. This is what they use to understand if two records are equal.

Copy link
Member

Choose a reason for hiding this comment

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

If you look at eac79f7 then previous implementation was based on Freeze logic. The ID not changing seems to be one of the main differences.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes sense that the ID is persisted and used to lookup records and check whether records are equal, but my point is that the only scenarios I can think of where problems could theoretically arise are either impossible or they are scenarios in which the records will have been finalized (they become the parent of another record or they got exported), which prevents the problems from happening in the first place.

Can you give a specific example of a scenario where you think this would cause problems so I can write a test for it and see what the behavior is before and after this change?

I've been trying to write a test myself but honestly can't even find a way to trigger it. I was trying something along the lines of:

// create a local dir, which will be imported and then copied to scratch, which will be exported to a different local dir
localdir, err := tmpdir(
  fstest.CreateFile("foo", []byte("foo"), 0600),
  fstest.CreateFile("bar", []byte("bar"), 0600),
)

destdir, err := ioutil.TempDir("", "buildkit")

local := llb.Local("source")

def := llb.Scratch().
  File(llb.Copy(local, "/", "/")).Marshal(sb.Context())

c.Solve(sb.Context(), def, SolveOpt{
  Exports: []ExportEntry{
	{
		Type:      ExporterLocal,
		OutputDir: destdir,
	},
  },
  LocalDirs: map[string]string{
	"source": localdir,
  },
}, nil)

// assert the exported dir has the expected contents
dt := ioutil.ReadFile(filepath.Join(destdir, "foo"))
require.Equal(t, []byte("foo"), dt)
dt = ioutil.ReadFile(filepath.Join(destdir, "bar"))
require.Equal(t, []byte("bar"), dt)

// change the local dir and then run the same solve again
os.WriteFile(filepath.Join(localdir, "bar"), []byte("doo"), 0600)

destdir = ioutil.TempDir("", "buildkit")

c.Solve(sb.Context(), def, SolveOpt{
Exports: []ExportEntry{
	{
		Type:      ExporterLocal,
		OutputDir: destdir,
	},
  },
  LocalDirs: map[string]string{
	"source": localdir,
  },
}, nil)

// assert the change happened as expected
dt = ioutil.ReadFile(filepath.Join(destdir, "foo"))
require.Equal(t, []byte("foo"), dt)
dt = ioutil.ReadFile(filepath.Join(destdir, "bar"))
require.Equal(t, []byte("doo"), dt)

But that can't actually reproduce the behavior we are talking about because local cache keys are based on session IDs, which means that the solver's cache won't ever try reloading a previous result for a local (so the test passed).

Then I tried the session re-use feature to force the cache keys to be the same for the local vertex, but the test just failed with errors like: DeadlineExceeded: no active session for 7fa0r79idbckfsm9kjkcdgann: context deadline exceeded. IIRC the shared session feature is known to be broken, so that's not a surprising error, right?

Either way, even if I was able to get the shared session to work, I still am pretty sure I wouldn't have been able to trigger the scenario we were talking about because it would have resulted in the local just getting reloaded rather than made mutable again (seems like a pre-existing bug hidden by the fact that shared sessions don't work, but not 100% sure).

So I'm at a loss for how it's possible to reproduce the problem you are mentioning, let me know if you have any ideas.

Copy link
Member

Choose a reason for hiding this comment

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

As far as the LLB API is concerned caching via sessionid is a valid feature as it is the only way for the client to confirm that the files did not change without retransfer. Possibly buildx bake will start to rely on this and for more complex cases this is definitely useful.

Even if you make a case that there is little use for affected callers atm, implementing the interface in a way that methods can only be called in a certain order limits future development and increases chances for this to unexpectedly break. In that case, the interface should be changed so callers would use some other, more reliable method for equality and persistence use-cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I can only imagine it breaking in situations where the rest of the code changes in pretty fundamental ways, but my imagination is limited here so that's alright.

Think I found a solution that retains the previous behavior of invalidating immutable ref IDs if they are reopened as mutable while still simplifying things relative to before. Namely, I changed it so that if you call cacheManager.GetMutable w/ the ID of a cache record that has been committed but not finalized, then you get a record that has the same metadata as before but with a new ID and the old ID is invalidated. This is implemented in GetMutable by just creating a new record and copying the metadata from the previous one, so it's pretty simple conceptually.

This seems to have been mostly seamless, with the one slight exception being the contenthash code, where I had to change from using the ref ID as key to using a metadata field in the cache record. This was very straightforward but let me know if there's any issues I'm not seeing as I'm not super familiar with that code.

Honestly, in the longer run, I suspect the best solution would be to replace the lazy-commit approach with something functionally equivalent that doesn't require you to be able to turn an immutable ref back into a mutable one. Not sure but I'm wondering if merge-op could help by letting you just merge new diffs onto refs instead of re-opening them as mutable in order to modify them (probably with periodic flattening of "merge chains" when using non-native snapshotters). But that's all too much for right now, so let me know what you think of the new approach here.

@sipsma sipsma force-pushed the simplify-lazy-commit branch 4 times, most recently from adab517 to 5c57c9a Compare July 27, 2021 06:06
@sipsma sipsma force-pushed the simplify-lazy-commit branch 4 times, most recently from 326a4b4 to 73162ef Compare August 16, 2021 18:18
@sipsma sipsma requested a review from tonistiigi August 16, 2021 21:43
@@ -49,6 +50,9 @@ type Accessor interface {
Get(ctx context.Context, id string, opts ...RefOption) (ImmutableRef, error)

New(ctx context.Context, parent ImmutableRef, s session.Group, opts ...RefOption) (MutableRef, error)
// GetMutable returns a mutable reference to the cache record with the given ID if it is possible to do so. If the
// provided cache record ID points to a committed but not finalized record, than a new cache record will be returned
// that is a copy of the previous but with a new ID (in order to invalidate any references to the previous ID).
Copy link
Member

Choose a reason for hiding this comment

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

why does GetMutable return a new ID? Mutable can change by definition so it is ok if the same ID has different content at different times(indexes rely on this, a client might as well). That is not ok for immutable refs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will only change ID if there used to be an immutable ref open w/ the previous ID.

So, if you have a ref open as mutable, release it, and then call GetMutable, the ID will not change.

If, however, you have a ref open as mutable, commit it to an immutable ref, release it and then call GetMutable, the ID will change in order to invalidate anyone who stored the ID of the previous immutable ref.

Copy link
Member

Choose a reason for hiding this comment

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

It still seems very hacky and unexpected. I'm not sure if you are rewriting indexes then or how otherwise the mutable reuse could work at all. The point of reuse is that 1) mutable x is created, id is stored 2) turned immutable y 3) x is not available 4) immutable released 5) x can be accessed again by the ID and modified 6) turned immutable z, as data changed and pointers to y and z should not be equal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if you are rewriting indexes then or how otherwise the mutable reuse could work at all

There's a new util function in the metadata code for copying the metadata/indexes from one id to another, which is what's used to create the new mutable ref: https://github.com/moby/buildkit/pull/2223/files#diff-ae44b7efd0878d4ad4013864764f0046c5c75f5f7dfe08718ad3bd32cabf9caeR401

The point of reuse is that 1) mutable x is created, id is stored 2) turned immutable y 3) x is not available 4) immutable released 5) x can be accessed again by the ID and modified 6) turned immutable z, as data changed and pointers to y and z should not be equal.

Makes sense, with the code in this PR the important constraints of that behavior are retained even if the semantics are slightly altered. Using your example, the modified behavior in this PR is:

  1. record x is created with a mutable ref pointing to it, id is stored
  2. mutable ref to x is made immutable
  3. mutable ref to x can't be opened
  4. immutable ref to x released
  5. mutable ref to x is requested, ID turns to y
  6. mutable ref to y turned immutable, as data changed pointers to x and y are not equal

While it's obviously subjective, the reason I prefer this modified behavior over the previous is that I think it both simplifies the mental model and the actual code.

Here's the model this PR is aiming for:

  • There are cache records, identified by an ID, which represent a single piece of snapshot (and related blob) state.
  • Mutable and Immutable refs are sort of like "smart pointers" to cache records that enforce safe access patterns and implement reference counting.
  • IDs will change when a mutable ref is reopened for a cache record which previously had an immutable ref pointing to it

Here's the model before this PR as I understand it:

  • There are cache records, identified by an ID, which represent either a single piece of snapshot+blob state or can themselves be pointers to other "equivalent" records (via the equal* fields) while still retaining their own ID.
  • Mutable and Immutable Refs are smart pointers like above but the rules for access patterns and reference counting now span not only the cache records they point to but also any cache records those cache records might themselves point to via equal*.
  • IDs will change when a mutable ref is committed into an immutable ref

The fact that IDs change when a committed record is reopened as mutable instead of when mutable is committed to immutable seems like a fine trade given the other simplifications. It also worked out-of-the-box with most of the existing callers anyways because they just used search indexes instead of directly storing mutable IDs.

In terms of the actual code, the equal* fields seemed to create convolution all over the place in the cache code (see the previous interaction between them in the release methods of immutableRef and mutableRef, the need for stuff like the doubleRef field, etc.). The new model trades that convolution for what amounts mostly to isolated boilerplate (such as constructing a new ref in GetMutable).

Let me know if this makes more sense or if instead you consider it important that we retain the exact same behavior as before, in which case I'll just go ahead and close this PR as I'm not sure how to simplify under those constraints and don't want to drag this out forever.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to think this weird ID behavior, and duplicating indexes on getting a record adds more confusion than that local doubleRef variable. I wouldn't be even sure indexes can be moved like this, certainly was not designed with this capability in mind. I guess the implementation with a single cacherecord that maintains previous behavior and updates its immutable ID is still possible but with the current interface behavior and added lines I don't think it makes things much simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I disagree pretty strongly that the behavior is weird and more confusing but I've got other PRs to send out so I'll just close this for now. I hope that in the future we can figure out some way to simplify this so the release methods and other parts of the code impacted by this are more tractable, but for now I'll just try to avoid modifying any of the equal* stuff to the extent possible.

The equalMutable and equalImmutable fields were used to implement the
"lazy commit" feature (not to be confused with lazy refs), which allows
refs to put off committing their snapshots to the underlying driver until
absolutely needed.

However, this way of modeling it resulted in there being 2 actual
cacheRecords even when the underlying snapshot was the same. This also
lead to a lot of convoluted logic checking for the existence of the
equal* fields.

This commits switches to a new approach of only maintaining a single
cacheRecord for what used to be separate equal* records. The cacheRecord
internally keeps track of whether it has been finalized or not and
updates which snapshot ID is underlying it.

If a cache record has been committed, GetMutable can still be called on
it but a new record with the same underlying metadata except for a new
ID is returned. This ensures we retain the previous behavior of
invalidating committed but not finalized immutable ref IDs.

This results in overall much simpler logic, as can be seen by looking at
the before+after of GetMutable, Commit, prune, and release, among
others. The only meaningfully-sized block of new code added is in
cacheManager.init, which handles migrating records from the old format
and can thus eventually be deleted once the older versions of buildkit
are no longer supported.

This change is backwards compatible because of that migration logic in
cacheManager.init and because MutableRefs have never been saved in the
upper-level solver cache, only ImmutableRefs are there, so removing any
refs that were marked as an equalMutable and merging them into their
equalImmutable will not result in any cache keys unexpectedly missing.

Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants