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

tail overhaul (--follow=name, etc.) #2695

Merged
merged 68 commits into from
Jun 7, 2022
Merged

Conversation

jhscheer
Copy link
Contributor

@jhscheer jhscheer commented Sep 28, 2021

I finally consider this ready for review!

TLDR:

  • Implement missing flags: --follow=name, --retry, -F, ---disable-inotify
  • Add 44 new test cases
  • Make 19 more GNU test suite tests pass that were previously failing

Long version:

  • use the notify crate for cross-platform filesystem notifications in order to implement -F i.e. --follow=name --retry with backend support from: inotify (Linux), kqueue (macOS/BSD) and ReadDirectoryChanges (Windows)
    Note: this uses a release candidate of the notify crate v5 because v4 is very old and doesn't have all the required features.
  • add support for polling in case a notification backend is not available or the resources are exhausted (e.g inotify.max_user_instances limit is reached), it can be forced with --use-polling
  • add support for stdin redirects, e.g. $ tail -f < file, $ tail < DIR, $ tail -F - < file, etc.
  • verify that -[nc]0 without -f, exit without reading
  • fix handling of files if permission is denied
  • fix handling of files if the file type is unsupported
  • add support for character devices and fifos

Ensure that:

  • --follow=name does not imply --retry
  • --follow={descriptor,name} (without --retry) does not wait for the files to appear
  • --retry without --follow results in a warning
  • tail --retry --follow={name,descriptor} waits for the file to appear
  • truncation is detected
  • tail --follow=descriptor --retry exits when the file appears untailable
  • --follow=descriptor (without --retry) does not try to open a file after an initial fail, even when there are other tailable files
  • tail -F retries when the file is initially untailable
  • inotify will switch to polling mode if directory of the watched file was initially missing and later created
  • inotify will switch to polling mode if directory of the watched file was removed and recreated
  • tail -f works when renaming the tailed files
  • the headers are correct for --verbose
  • tail --follow=name handles remove, move, truncation, rename, create events correctly
  • truncating a file with the same content it already has does not trigger a truncation event

Summary

Ensure that the following GNU test-suite tests pass:

  • F-headers.sh
  • F-vs-missing.sh
  • F-vs-rename.sh #
  • append-only.sh # skipped test: must be run as root
  • assert-2.sh
  • assert.sh
  • descriptor-vs-rename.sh
  • flush-initial.sh
  • follow-name.sh #
  • inotify-dir-recreate.sh
  • inotify-hash-abuse.sh
  • inotify-hash-abuse2.sh
  • inotify-rotate.sh #
  • overlay-headers.sh #
  • pid.sh
  • pipe-f2.sh
  • retry.sh #
  • tail-n0f.sh
  • truncate.sh
    Note: 6 of these do not show up in the CI as passes becauseappend-only needs root privileges, and the other 5 tests marked with # pass on my local machine only. I have no clue why and it's incredible cumbersome to debug the CI so I will refrain from that.

Note: there are further details in tail/README.md

Edit:
Now theoverlay-headers.sh test passes in the CI, although the tests rerun without changes. I suspect scheduling/load on the test VM is the reason.

On macOS only `kqueue` is suitable for our use case because `FSEvents`
waits until file close to delivers modify events.
@jhscheer jhscheer force-pushed the tail_notify branch 3 times, most recently from 32e12a7 to ccd8890 Compare October 7, 2021 10:18
* Change data structure from Vec to HashMap in order to better
keep track of files while watching them with `--follow=name`.
E.g. file paths that were removed while watching them and exit
if no files are remaining, etc.

* Move all logic related to file handling into a FileHandling trait

* Simplify handling of the verbose flag.
This makes uu_tail pass the "gnu/tests/tail-2/descriptor-vs-rename" test.

* add tests for descriptor-vs-rename (with/without verbose)
* fix some minor error messages
jhscheer added 3 commits June 2, 2022 17:01
* fix test_retry7
* fix test_follow_descriptor_vs_rename2
* set permissions with set_permissions instead of a call to chmod
* improve documentation
* add fixes to pass:
    - tail-2/F-vs-rename.sh
    - tail-2/follow-name.sh
    - tail-2/inotify-hash-abuse.sh
    - tail-2/inotify-only-regular.sh
    - tail-2/retry.sh
* add/improve documentation
@jhscheer jhscheer force-pushed the tail_notify branch 4 times, most recently from 80b70da to b71b0da Compare June 2, 2022 23:22
@jhscheer jhscheer marked this pull request as ready for review June 2, 2022 23:22
@sylvestre
Copy link
Contributor

The coverage job on linux fails with:


---- test_tail::test_follow_name_move_create2 stdout ----
current_directory_resolved: 
touch: /tmp/.tmprXSPHB/1
touch: /tmp/.tmprXSPHB/2
touch: /tmp/.tmprXSPHB/3
touch: /tmp/.tmprXSPHB/4
touch: /tmp/.tmprXSPHB/5
touch: /tmp/.tmprXSPHB/6
touch: /tmp/.tmprXSPHB/7
touch: /tmp/.tmprXSPHB/8
touch: /tmp/.tmprXSPHB/9
run: /home/runner/work/coreutils/coreutils/target/debug/coreutils tail -s.1 --max-unchanged-stats=1 -q -F 1 2 3 4 5 6 7 8 9
write(truncate): /tmp/.tmprXSPHB/9
rename: "/tmp/.tmprXSPHB/1" "/tmp/.tmprXSPHB/f"
write(truncate): /tmp/.tmprXSPHB/1
remove: "/tmp/.tmprXSPHB/f"
run: /home/runner/work/coreutils/coreutils/target/debug/coreutils tail -s.1 --max-unchanged-stats=1 -q -F 1 2 3 4 5 6 7 8 9 ---disable-inotify
write(truncate): /tmp/.tmprXSPHB/9
rename: "/tmp/.tmprXSPHB/1" "/tmp/.tmprXSPHB/f"
write(truncate): /tmp/.tmprXSPHB/1
---- test_tail::test_follow_name_move_create2 stderr ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"tail: 9: file truncated\ntail: '1' has become inaccessible: No such file or directory\ntail: '1' has appeared;  following new file\n"`,
 right: `"tail: '1' has become inaccessible: No such file or directory\ntail: '1' has appeared;  following new file\n"`', /home/runner/work/coreutils/coreutils/tests/by-util/test_tail.rs:1891:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

* add clippy fixes
* add cicd fixes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

This is excellent! Bravo for sticking with this, the complexity of this feature is wild. I have left some comments for further cleanup, but it looks very good.

src/uu/tail/Cargo.toml Outdated Show resolved Hide resolved
src/uu/tail/Cargo.toml Outdated Show resolved Hide resolved
src/uu/tail/README.md Outdated Show resolved Hide resolved
src/uu/tail/src/tail.rs Show resolved Hide resolved
src/uu/tail/src/tail.rs Outdated Show resolved Hide resolved
src/uu/tail/src/tail.rs Outdated Show resolved Hide resolved
src/uu/tail/src/tail.rs Show resolved Hide resolved
src/uu/tail/src/tail.rs Outdated Show resolved Hide resolved
src/uu/tail/src/tail.rs Show resolved Hide resolved
src/uu/tail/src/tail.rs Show resolved Hide resolved
@sylvestre
Copy link
Contributor

And the CI is now green, bravo :)

@sylvestre sylvestre requested a review from tertsdiepraam June 5, 2022 07:34
* minor code clean-up
* remove test-suite summary from README
@jhscheer
Copy link
Contributor Author

jhscheer commented Jun 6, 2022

@tertsdiepraam and @sylvestre thank you very much for your review and comments. I addressed all your concerns.

Copy link
Member

@tertsdiepraam tertsdiepraam 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! Thanks for addressing those comments and, again, this is a great PR!

src/uu/tail/src/tail.rs Show resolved Hide resolved
src/uu/tail/src/tail.rs Show resolved Hide resolved
Copy link
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

LGTM, bravo!

@sylvestre sylvestre merged commit 0532c74 into uutils:main Jun 7, 2022
@sylvestre
Copy link
Contributor

I see an intermittent here:
https://github.com/uutils/coreutils/runs/6808598985?check_suite_focus=true


---- test_tail::test_follow_name_move_create2 stdout ----
current_directory_resolved: 
touch: /tmp/.tmpVYRqaL/1
touch: /tmp/.tmpVYRqaL/2
touch: /tmp/.tmpVYRqaL/3
touch: /tmp/.tmpVYRqaL/4
touch: /tmp/.tmpVYRqaL/5
touch: /tmp/.tmpVYRqaL/6
touch: /tmp/.tmpVYRqaL/7
touch: /tmp/.tmpVYRqaL/8
touch: /tmp/.tmpVYRqaL/9
run: /target/x86_64-unknown-linux-gnu/debug/coreutils tail -s.1 --max-unchanged-stats=1 -q -F 1 2 3 4 5 6 7 8 9
write(truncate): /tmp/.tmpVYRqaL/9
rename: "/tmp/.tmpVYRqaL/1" "/tmp/.tmpVYRqaL/f"
write(truncate): /tmp/.tmpVYRqaL/1
remove: "/tmp/.tmpVYRqaL/f"
run: /target/x86_64-unknown-linux-gnu/debug/coreutils tail -s.1 --max-unchanged-stats=1 -q -F 1 2 3 4 5 6 7 8 9 ---disable-inotify
write(truncate): /tmp/.tmpVYRqaL/9
rename: "/tmp/.tmpVYRqaL/1" "/tmp/.tmpVYRqaL/f"
write(truncate): /tmp/.tmpVYRqaL/1
thread 'test_tail::test_follow_name_move_create2' panicked at 'assertion failed: `(left == right)`
  left: `"tail: 9: file truncated\ntail: '1' has become inaccessible: No such file or directory\ntail: '1' has appeared;  following new file\n"`,
 right: `"tail: '1' has become inaccessible: No such file or directory\ntail: '1' has appeared;  following new file\n"`', /project/tests/by-util/test_tail.rs:1898:9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants