Conversation
5d08c3a to
9c819e6
Compare
|
Should be good to review despite the current CI failures, which are:
I'll send out further updates as separate commits so any in progress reviews don't get interrupted. |
|
One other thing to be aware of here is that this implementation results in some very small slowdowns whenever an overlayfs based snapshotter is used, even if not using merge-op. This is due to needing to fixup unnecessary opaque dirs and create empty base layers to ensure we always have a workdir. Based on brief testing, this was only on the order of a few 100 ms in the worst cases I tried, but I will do a bit more thorough testing to confirm. Hopefully that's not an issue, but if it is there's a fallback option that avoids it at the cost of slowing down the merge-op case. Basically, instead of implementing the efficient merge-op by joining together lowerdirs, we could use the approach in the cc @tonistiigi |
|
Fixed the issues in CI |
cache/refs.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cr.mountCache = mountable |
There was a problem hiding this comment.
the lifecycle management of this looks confusing. If it is stateless then why does it need to be cached and can it be enforced. if it is not stateless then seems problematic and needs reference counts or smth.
There was a problem hiding this comment.
The context here is that there's multiple scenarios in which we may need to do work to make a record actually be mountable: unlazying of GetByBlob refs, extracting from the content store, creation of merge snapshots (which does involve actual work in the inefficient case), etc.
Rather than running through all the checks needed to see if a record is ready to mounted every time, I figured it made sense to just have a field indicating whether the record is ready. And rather than use a bool, we may as well just save the mountable and indicate that its ready by the field being non-nil.
So while the field is stateless, it's lazily initialized and we save a little bit of work by not having to do a bunch of checks every time to see if the it's been initialized and calling snapshotter.Mount to retrieve it. I don't think those checks are super expensive though, so if you prefer we get rid of this field that's fine with me too.
snapshot/snapshotter.go
Outdated
| func NewContainerdSnapshotter(s Snapshotter) (snapshots.Snapshotter, func() error) { | ||
| cs := &containerdSnapshotter{Snapshotter: s} | ||
| return cs, cs.release | ||
| func (sn *fromContainerd) View(ctx context.Context, key string, parent string, opts ...snapshots.Opt) error { |
There was a problem hiding this comment.
These snapshotter changes are hard to understand. Try to add more comments, eg. what parameters functions take, what they return and what side-effects they have in memory or on disk. A schematic/intro comment in the header or interface definition would not hurt as well.
There was a problem hiding this comment.
I added a bunch of comments to try to make it more clear. I commented on the interface methods where they have differences from the underlying containerd methods and commented inline throughout.
However, I think it would probably still benefit from something a bit more explanatory with schematics like you were saying. What do you think about me taking just the first doc from the draft PR I sent out a while ago (#2212), updating it with some more details on how the implementation actually ended up and then including that as part of this PR or a followup? I think that format might be a better fit than comments, but let me know if you disagree.
Either way, in the meantime let me know if there's still anything specific that's unclear after the updated comments and I'll do my best to clarify right away.
d92bf66 to
9086462
Compare
|
Rebased. @tonistiigi let me know what other comments you have |
tonistiigi
left a comment
There was a problem hiding this comment.
The comments help but still quite confusing and hard to understand if it could be easier or some design choices could make it simpler. This is not going to be easy to maintain. We may need to schedule a call and go over some things together.
Yeah there's a lot of obscure corner cases around overlay that create tons of complication. The fallback option I mentioned in this comment where we abandon trying to just join lowerdirs together in the overlay case and instead always use a hardlink-based merge may address a lot of your individual concerns in your most recent comments in addition to your overall concern about simplifying things as a whole:
So, while the performance of a merge may take a hit if we switch from joining overlay lowerdirs to creating hardlink merges, I'm no longer convinced it would be so significant as to justify all the complication associated with joining lowerdirs.
Overall, I'm thinking I'll give this approach a shot (shouldn't take too long to write out as most of the existing code is re-used or just deleted). Provided the performance hit is minimal, I think it may be worth it and address a lot of your concerns. Let me know what you think though. Ping me on Slack if you want to set up a call to discuss too. |
|
@tonistiigi In reference to my last comment, I tried out using the hardlink-based merge for overlay snapshotters, seems to work as expected and without any huge noticeable performance penalty. The change is quick and hacky right now but easily cleaned up, the commit is here if you want to get an idea: sipsma@679eb0f The demo I mentioned previously showed just as much improvement as the previous implementation and I can't see any difference in the execution time of |
|
(Didn't mean to close the PR, pressed the wrong button for the last comment on accident...) |
08ebf57 to
ff7bf8c
Compare
ff7bf8c to
134db88
Compare
6836e94 to
9379c59
Compare
|
@tonistiigi Ping, any comments for the updated code? |
|
@tonistiigi Thanks for the comments. I've worked through the initial batch now, pushing the updates+responses for those right away because a few of them require some more discussion. In the mean time, I'll start going through the more recent comments for the last commit now and push updates soon. Also, as you suggested I'll split out the overlay utils commit to a separate PR to get it merged faster (in case anyone else wants to make changes to that code very soon). The other commit you suggested I split out was too heavily based on its parent to split out; in the latest push I actually just merged it with that parent. |
5a0d9aa to
0ee849f
Compare
|
@tonistiigi updated from your comments on the merge commit.
Responded to your inline comment about this. Fixed it in this PR.
Responded to your inline comment about this. Will add this to the list of follow ups.
Responded to your inline comment about this. On a separate note, I noticed a bug in merge-op while cleaning the code up. The bug was that if you created layers on top of a merge ref (i.e. w/ an exec) and then used those layers as a merge input, the deletions present in the original merge weren't honored, which meant that the mounted merge was inconsistent w/ its exported image (where the re-used blobs end up including all such deletions). Added test coverage for this now. The fix was potentially really ugly (have to handle these nested merges in the snapshotter In addition to fixing the bug, this also:
Just wanted to explain why the |
Before this, descriptor handlers were not included with calls to the exporter, which then sometimes called LoadRef and failed to get a ref because it was lazy. This change results in the DescHandlers of the already loaded refs to get plugged into context so they can be re-used by the exporter if it needs to load the ref again. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
The Parent method will no longer make sense with forthcoming Merge and Diff support as refs will become capable of having multiple parents. It was also only ever used externally to get the full chain of refs for each layer in the ref's chain. The newly added LayerChain method replaces Parents with a method that just returns a slice of refs for each layer in the ref's chain. This will work more seamlessly with Merge and Diff (in which case it returns the "flattened" ancestors of the ref) in addition to being a bit easier to use for the exiting cases anyways. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
This is mostly just preparation for merge-op. The existing Extract method is updated to be usable for unlazying any type of refs rather than just lazy blobs. The way views are created is simplified and centralized in one location. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
|
@tonistiigi updated from your most recent comments and added TODOs to |
This consists of just the base MergeOp with support for merging LLB results that include deletions using hardlinks as the efficient path and copies as fallback. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
|
Thanks for the approval @tonistiigi ! Know this was a pretty large/gnarly change so I appreciate all the reviews and feedback. |
This adds test coverage for ensuring the readonly parameter is honored as expected in the ref Mount methods. There was a regression introduced during the moby#2335 that went unnoticed until identified and fixed in moby#2562. This test coverage should help prevent similar regressions in the future. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
This adds test coverage for ensuring the readonly parameter is honored as expected in the ref Mount methods. There was a regression introduced during moby#2335 that went unnoticed until identified and fixed in moby#2562. This test coverage should help prevent similar regressions in the future. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
This consists of just the base MergeOp with support for merging LLB
results that include deletions, including support for both efficient and
fallback merges.