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

No exit when union_helper not found #99

Merged
merged 8 commits into from
Jul 7, 2023
Merged

Conversation

angelhof
Copy link
Member

@angelhof angelhof commented Jul 6, 2023

This PR fixes try to not exit when unionfs/mergerfs are not found, but just produce warning messages.

Addresses: #96

@angelhof angelhof requested review from ezrizhu and mgree July 6, 2023 17:29
@angelhof angelhof linked an issue Jul 6, 2023 that may be closed by this pull request
@mgree mgree mentioned this pull request Jul 6, 2023
9 tasks
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.

Test cases worked on machines with mergerfs.
try now behaves correctly with machines nested mount and no unionfs/mergerfs.

eric@try-test-lvm:/srv$ /home/eric/try/try echo hi
try(/tmp/tmp.dRGKQR7sz5): Warning: Failed mounting /home as an overlay and mergerfs or unionfs not set and could not be found, see "/tmp/tmp.5rnwzXMHdm"
try(/tmp/tmp.dRGKQR7sz5): Warning: Failed mounting /run as an overlay and mergerfs or unionfs not set and could not be found, see "/tmp/tmp.5rnwzXMHdm"
try(/tmp/tmp.dRGKQR7sz5): Warning: Failed mounting /sys as an overlay and mergerfs or unionfs not set and could not be found, see "/tmp/tmp.5rnwzXMHdm"
try(/tmp/tmp.dRGKQR7sz5): Warning: Failed mounting /home as an overlay and mergerfs or unionfs not set and could not be found, see "/tmp/tmp.5rnwzXMHdm"
hi

home is repeated here

  • one re-run was caused by /home being a mount(listed in findmnt)
  • the other re-run is caused by the initial mounting of /home failing because it is a nested mount

Is this something that we want to look into or is this fine?

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.

I think the cp -R is going to be really expensive, so maybe symlinking instead? Or mark this test as CI only (i.e., the CI variable needs to be set).

test/run_tests.sh Outdated Show resolved Hide resolved
@mgree
Copy link
Contributor

mgree commented Jul 6, 2023

home is repeated here

* one re-run was caused by /home being a mount(listed in findmnt)

* the other re-run is caused by the initial mounting of /home failing because it is a nested mount

Is this something that we want to look into or is this fine?

We should try to avoid duplicate work... and we should definitely avoid redundant warnings/output. Definitely worth fixing, especially if it's easy.

@angelhof
Copy link
Member Author

angelhof commented Jul 6, 2023

home is repeated here

* one re-run was caused by /home being a mount(listed in findmnt)

* the other re-run is caused by the initial mounting of /home failing because it is a nested mount

Is this something that we want to look into or is this fine?

We should try to avoid duplicate work... and we should definitely avoid redundant warnings/output. Definitely worth fixing, especially if it's easy.

I think this is caused by for mountpoint in /* $(findmnt --real -r -o target -n) which lists both all top level directories and then all mounts. This is an issue that we figured out we need to solve indeed (but it doesn't have anything to do with my PR).

@ezrizhu
Copy link
Collaborator

ezrizhu commented Jul 6, 2023 via email

@angelhof
Copy link
Member Author

angelhof commented Jul 6, 2023

I think the repeating of home also has to do with the first part of this issue (#104)

@ezrizhu
Copy link
Collaborator

ezrizhu commented Jul 6, 2023 via email

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.

significant speed up! (my test vm is on a very old sas drive)

@angelhof
Copy link
Member Author

angelhof commented Jul 6, 2023

OK, fixed the duplicate issue too @ericzty can you check you dont get the warning for home twice? @mgree could you check the new changes?

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.

eric@try-test-lvm:/srv$ /home/eric/try/try echo hi
try(/tmp/tmp.4B1SvKhn8b): Warning: Failed mounting /home as an overlay and mergerfs or unionfs not set and could not be found, see "/tmp/tmp.NJCocX3VGY"
try(/tmp/tmp.4B1SvKhn8b): Warning: Failed mounting /run as an overlay and mergerfs or unionfs not set and could not be found, see "/tmp/tmp.NJCocX3VGY"
try(/tmp/tmp.4B1SvKhn8b): Warning: Failed mounting /sys as an overlay and mergerfs or unionfs not set and could not be found, see "/tmp/tmp.NJCocX3VGY"
hi

🚀

@ezrizhu ezrizhu requested a review from mgree July 7, 2023 04:08
@angelhof angelhof merged commit 0d08012 into main Jul 7, 2023
@angelhof angelhof deleted the no-exit-when-unionfs-not-found branch July 7, 2023 19:38
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.

don't exit on union_helper not found
3 participants