DiffOp Preparation Commits #2517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using an interface instead of a func is more flexible while achieving the same effect. It allows you to succintly define a large number of test cases as structs, as is common in table-driven testing. A helper func is added that converts the existing test funcs into the interface, so the change is fairly seamless. Signed-off-by: Erik Sipsma <[email protected]>
Before this change, test cases were running with an env var that forces the overlay differ to be on even when the native snapshotter was being used, which resulted in failures. Now, that env var is skipped when using the native snapshotter. Additionally, this includes a related change to skip even trying to use the overlay differ when the native snapshotter is in use. Previously, the blob creation code first tried to use the overlay differ and then failed and fell back to the double-walking differ. Now, it just jumps right to the double-walking differ when the native snapshotter is in use. Signed-off-by: Erik Sipsma <[email protected]>
This breaks the giant blob that was the diffApply function into two separate parts, a differ and an applier, which results in more modular code that should be easier to follow and easier to make any future updates to. For example, if we want to optimize by allowing differ and applier to run in parallel in the future, that's straightforward now. There are also some fixes that weren't needed for MergeOp, but will be for DiffOp, such as correctly handling the case where a deletion is applied that is under parent directories which don't exist yet (the correct behavior is, surprisingly, to create the parent directories as that is what the image import/export code ends up doing). Signed-off-by: Erik Sipsma <[email protected]>
tonistiigi
approved these changes
Dec 10, 2021
| upperView, err := upperMnter.Mount() | ||
| if err != nil { | ||
| return err | ||
| type applier struct { |
There was a problem hiding this comment.
As commented in earlier PR I think this would be better as a separate pkg in some follow up that would hopefully lead to deduplication between this and similar tar implementations in containerd.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are the initial commits of #2434 split out as suggested in that PR. @tonistiigi responded and updated from your comments about these commits in that PR, let me know if you'd prefer there be one PR-per-commit rather than grouping them here.