Skip to content

ZTS: include microsecond timestamps on all output #17045

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robn
Copy link
Member

@robn robn commented Feb 12, 2025

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

When reviewing test output after a failure, it's often quite difficult to work out the order and timing of events, and to correlate test suite output with kernel logs.

Description

This adds timestamps to ZTS output to help correlate events:

  • all of the standard log_XXX functions ultimately end up in _printline, which now prefixes output with a timestamp.
  • the test runner logging function log() also now prefixes its output with a timestamp.
  • on failure, when capturing the kernel log in zfs_dmesg.ksh, the iso time format is requested. This is gated behind a platform check, as FreeBSD's dmesg has no switches to control timestamps.

How Has This Been Tested?

Manually, running a test I know fails in my test VMs and considering the output.

$ zfs-tests.sh -DvKx -t zfs_load-key_https
...
/usr/local/share/zfs/test-runner/bin/test-runner.py  -D  -K -c "/var/tmp/zfs-tests.481.run" -T "functional" -i "/usr/local/share/zfs/zfs-tests" -I "1"
[    6.960295] ZTS run /usr/local/share/zfs/zfs-tests/tests/functional/cli_root/zfs_load-key/setup
[2025-02-12T01:05:34.353115] NOTE: begin default_setup_noexit
[2025-02-12T01:05:34.398547] SUCCESS: zpool create -f testpool loop0
[2025-02-12T01:05:34.432899] SUCCESS: zfs create testpool/testfs
[2025-02-12T01:05:34.463772] SUCCESS: zfs set mountpoint=/var/tmp/testdir testpool/testfs
[2025-02-12T01:05:35.520185] SUCCESS: openssl req -x509 -newkey rsa:4096 -sha256 -days 1 -nodes -keyout /testpool/snakeoil.key -out /testpool/snakeoil.crt -subj /CN=localhost
Traceback (most recent call last):
  File "<string>", line 16, in <module>
  File "<string>", line 10, in <module>
  File "/usr/lib/python3.11/socketserver.py", line 456, in __init__
    self.server_bind()
  File "/usr/lib/python3.11/http/server.py", line 136, in server_bind
    socketserver.TCPServer.server_bind(self)
  File "/usr/lib/python3.11/socketserver.py", line 472, in server_bind
    self.socket.bind(self.server_address)

[2025-02-12T01:06:25.290486] NOTE: Performing test-fail callback (/usr/local/share/zfs/zfs-tests/callbacks/zfs_dbgmsg.ksh)
...
[2025-02-12T01:06:25.315555] NOTE: Performing test-fail callback (/usr/local/share/zfs/zfs-tests/callbacks/zfs_dmesg.ksh)
=================================================================
 Tailing last 200 lines of dmesg log
=================================================================
2025-02-12T01:05:28,971544+00:00 .... node  #0, CPUs:      #1 #2 #3 #4 #5
2025-02-12T01:05:28,971544+00:00 smp: Brought up 1 node, 6 CPUs
2025-02-12T01:05:28,971544+00:00 smpboot: Max logical packages: 1
2025-02-12T01:05:28,971544+00:00 smpboot: Total of 6 processors activated (23904.00 BogoMIPS)
2025-02-12T01:05:29,090172+00:00 node 0 deferred pages initialised in 120ms
...
=================================================================
 End of dmesg log
=================================================================
[2025-02-12T01:06:25.334713] NOTE: Performing test-fail callback (/usr/local/share/zfs/zfs-tests/callbacks/zfs_mmp.ksh)
[2025-02-12T01:06:25.340432] Test: /usr/local/share/zfs/zfs-tests/tests/functional/cli_root/zfs_load-key/setup (run as root) [00:51] [FAIL]
[2025-02-12T01:06:25.345063] Test: /usr/local/share/zfs/zfs-tests/tests/functional/cli_root/zfs_load-key/zfs_load-key_https.ksh (run as root) [00:00] [SKIP]
[   58.006623] ZTS run /usr/local/share/zfs/zfs-tests/tests/functional/cli_root/zfs_load-key/cleanup
[2025-02-12T01:06:25.551120] SUCCESS: zpool destroy -f testpool
[2025-02-12T01:06:25.583079] SUCCESS: rm -rf /var/tmp/testdir
[2025-02-12T01:06:25.594580] Test: /usr/local/share/zfs/zfs-tests/tests/functional/cli_root/zfs_load-key/cleanup (run as root) [00:00] [PASS]
...

(fyi, this test fails because the VM has no network support at all, not even a loopback. It's no big deal)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@robn robn force-pushed the zts-timestamp-prefix branch 3 times, most recently from ed7589e to d0562f8 Compare February 15, 2025 23:54
@amotin
Copy link
Member

amotin commented Feb 20, 2025

"03:04:46.89 [2025-02-16T03:04:46.898708]" looks duplicate and excessively verbose. I suspect you meant to add the timestamps in some places where they weren't, but seems now there are places when they are duplicate.

And BTW, the CI seems very unhappy for some reason.

@robn robn force-pushed the zts-timestamp-prefix branch from d0562f8 to 4b62c60 Compare February 24, 2025 12:34
@robn
Copy link
Member Author

robn commented Feb 24, 2025

03:04:46.89 [2025-02-16T03:04:46.898708]

This doesn't happen locally. Might be something in the CI runner adding timestamps? I'll look soon.

@amotin amotin added the Status: Revision Needed Changes are required for the PR to be accepted label Mar 5, 2025
@tonyhutter
Copy link
Contributor

@robn can you rebase this and see if that fixes the CI problems?

@robn robn force-pushed the zts-timestamp-prefix branch from 4b62c60 to a6d6ffa Compare March 19, 2025 01:02
@robn
Copy link
Member Author

robn commented Mar 19, 2025

Done. I did have a very short look and decided the extra bit might have been the CI proper, but I had no big ideas and haven't had time to get back to it properly yet. So lets see, either this will work or give me a new data point. Cheers :)

@github-actions github-actions bot removed the Status: Revision Needed Changes are required for the PR to be accepted label Mar 19, 2025
@tonyhutter
Copy link
Contributor

If you need a reproducer - zpool_add_dryrun_output.ksh will pass on master and fail with this PR in local testing.

@AttilaFueloep
Copy link
Contributor

AttilaFueloep commented Mar 27, 2025

Yeah, while comparing the output of zpool add -n with what we expect we do:

        typeset out="$(log_must eval "zpool add -n '$TESTPOOL' $add" | \
            sed /^SUCCESS/d)"

        if [[ "$out" != "$want" ]]; then
                log_fail "Got:\n" "$out" "\nbut expected:\n" "$want"
        fi

If the output of zpool add -n is prepended with a timestamp, the tests obviously will fail. The same should aply to all other tests from PR #11167 as well. The fix would be to extend the sed expression to remove the timestamps.

@AttilaFueloep
Copy link
Contributor

Just wondering if

-            sed /^SUCCESS/d)"
+            sed /.*SUCCESS/d)"

would fix the test failure.

@robn
Copy link
Member Author

robn commented Mar 28, 2025

Good find, thanks. I think I don't like that pattern - I don't like a test that is dependent on the output of the test framework it's running inside. There's only three instances in the whole test suite, so I'll rework them to just say what they mean.

@AttilaFueloep
Copy link
Contributor

Yeah, agreed. At least I cleaned up my mess in #17198.

@robn robn force-pushed the zts-timestamp-prefix branch from a6d6ffa to 92f6b69 Compare March 29, 2025 23:47
@robn
Copy link
Member Author

robn commented Mar 29, 2025

Rebased onto #17198 and those three tests now pass. Pushed, lets see how it goes.

@robn robn force-pushed the zts-timestamp-prefix branch from 92f6b69 to f0a4068 Compare April 2, 2025 01:22
When reviewing test output after a failure, it's often quite difficult
to work out the order and timing of events, and to correlate test suite
output with kernel logs.

This adds timestamps to ZTS output to help with this, in three places:

- all of the standard log_XXX functions ultimately end up in _printline,
  which now prefixes output with a timestamp.

- the test runner logging function log() also now prefixes its output
  with a timestamp.

- on failure, when capturing the kernel log in zfs_dmesg.ksh, the "iso"
  time format is requested.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn robn force-pushed the zts-timestamp-prefix branch from f0a4068 to 28af226 Compare April 9, 2025 10:43
@amotin
Copy link
Member

amotin commented Apr 9, 2025

It is still somehow double-stamped in CI:

11:22:40.11 [2025-04-09T11:22:40.107876] SUCCESS: cp /var/tmp/cachefile.59027 /var/tmp/cachefile.59027.bkp

@AttilaFueloep
Copy link
Contributor

Forgot to mention, that I'm seeing the short timestamps as well. Seems not to be CI related. This is on a somewhat current
master without this PR applied.

$ /usr/share/zfs/zfs-tests.sh -t tests/functional/cli_root/zpool_split/zpool_split_dryrun_output.ksh
....
$ head -4 </var/tmp/test_results/current/log
Test: /usr/share/zfs/zfs-tests/tests/functional/cli_root/zpool_split/setup (run as root) [00:00] [PASS]
Test: /usr/share/zfs/zfs-tests/tests/functional/cli_root/zpool_split/zpool_split_dryrun_output.ksh (run as root) [00:06] [FAIL]
02:33:55.43 ASSERTION: 'zpool split -n <pool> <newpool> [<vdev>]...' can display the configuration
02:33:55.43 SUCCESS: truncate -s 67108864 /var/tmp/filedev00

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