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

Implement Multi-Lower-Layer Overlay Support through Merging #122

Merged
merged 21 commits into from
Jan 13, 2024

Conversation

gliargovas
Copy link
Collaborator

This Pull Request introduces the -L flag to try, enabling the specification and merging of multiple lower directories for overlay. The lower directories are specified as a colon-separated list (dir_1:dir_2:...:dir_n), operating with a precedence logic where directories on the left have a higher precedence over those on the right. This feature allows users to overlay multiple directories together during the command execution within a single overlay environment.

@gliargovas
Copy link
Collaborator Author

Should I also add some tests for this?

README.md Show resolved Hide resolved
@gliargovas gliargovas changed the base branch from main to future October 7, 2023 07:12
@gliargovas gliargovas requested review from mgree and angelhof October 7, 2023 07:13
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Looks good. Let's do a separate version bump once the tests pass on this.

README.md Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
@angelhof
Copy link
Member

Could you rebase this to not have Eric's and Michael's commits? Maybe you just need to start your branch from future instead of main?

@gliargovas gliargovas force-pushed the multiple_lower_dir_merging branch from b1f5873 to 6c6b8e0 Compare October 11, 2023 15:06
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Looks great! One small potential bug and a few small fixes (to save on processes).

try Show resolved Hide resolved
try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
@gliargovas
Copy link
Collaborator Author

@mgree I have addressed all your comments. Feel free to merge!

@ezrizhu
Copy link
Collaborator

ezrizhu commented Oct 11, 2023

before we merge, i’d like to take a bit and look into why CI is failing, i’ll try to repro this on my machines.

try Outdated Show resolved Hide resolved
try Show resolved Hide resolved
Copy link
Member

@angelhof angelhof left a comment

Choose a reason for hiding this comment

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

I left some questions :)

try Outdated Show resolved Hide resolved
@mgree
Copy link
Contributor

mgree commented Oct 20, 2023

Talking with @angelhof about this, we realized there's a tricky interaction with process_changes when you have multiple lowerdirs: you have to replay the merge to do the comparison.

The correct solution is to mergerfs mount the layers of lowerdir and compare against that merged lowerdir (rather than the $local_file which just absolutizes to the real system root).

But until we have a use case where someone would want to use summaries and multiple lowerdirs (hs doesn't want the summaries), we should punt on this and give a warning that -L implies -n.

docs/try.1.md Outdated Show resolved Hide resolved
@gliargovas gliargovas force-pushed the multiple_lower_dir_merging branch from 4240060 to b0538b4 Compare October 31, 2023 15:20
@gliargovas
Copy link
Collaborator Author

Rebased this off main

@gliargovas gliargovas requested review from mgree and angelhof October 31, 2023 15:22
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Nice test! I think there's some tiny argparsing fixup to do and we're good to go.

try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
docs/try.1.md Outdated Show resolved Hide resolved
try Show resolved Hide resolved
@gliargovas gliargovas force-pushed the multiple_lower_dir_merging branch from b0538b4 to d468590 Compare November 15, 2023 07:12
@gliargovas gliargovas requested a review from mgree November 15, 2023 07:14
@gliargovas
Copy link
Collaborator Author

@mgree, I addressed the comments you left.

@gliargovas
Copy link
Collaborator Author

Main workflow/test is expected to fail because the workflow is changed to run ./scripts/setup.sh in the' future' branch.

@gliargovas gliargovas force-pushed the multiple_lower_dir_merging branch from c080ca5 to 3b0e768 Compare November 16, 2023 16:23
@gliargovas gliargovas force-pushed the multiple_lower_dir_merging branch from 3b0e768 to 7b0e233 Compare November 16, 2023 16:26
@gliargovas
Copy link
Collaborator Author

Cleaned everything unrelated to this PR

@ezrizhu ezrizhu self-requested a review November 16, 2023 16:42
Copy link
Collaborator

@ezrizhu ezrizhu left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🎉 🚀 👍

@ezrizhu
Copy link
Collaborator

ezrizhu commented Nov 16, 2023

Oh, is this supposed to merge into future or main branch?

@ezrizhu ezrizhu changed the base branch from future to main November 16, 2023 16:46
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

A few tiny nitpicks, looking good!

try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
try Show resolved Hide resolved
try Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
@ezrizhu
Copy link
Collaborator

ezrizhu commented Nov 29, 2023

Could you also rebase off latest main or add in my latest commit so CI tests pass 🙏
nvm i got it!

Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

lgtm! thank you!!

@ezrizhu ezrizhu merged commit 0647f69 into main Jan 13, 2024
17 checks passed
@ezrizhu ezrizhu deleted the multiple_lower_dir_merging branch January 13, 2024 21:33
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.

4 participants