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

Remove unnecessary but potentially racy signalblocker log lines #2022

Merged
merged 1 commit into from Apr 11, 2022

Conversation

okurz
Copy link
Member

@okurz okurz commented Apr 9, 2022

As tina.mueller@suse.com found out in
https://progress.opensuse.org/issues/107998 the log lines in
signalblocker might cause race conditions in our tests.
Also, I consider the output in a usual openQA like in the following log
excerpt not helpful to read:

[2022-04-09T12:52:55.814954+02:00] [debug] loaded 10829 needles
[2022-04-09T12:52:56.491100+02:00] [debug] Blocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.589482+02:00] [debug] Unblocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.782965+02:00] [debug] 65300: channel_out 15, channel_in 14
[2022-04-09T12:52:56.799859+02:00] [debug] Blocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.901592+02:00] [debug] Unblocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.911728+02:00] [debug] 579: cmdpipe 13, rsppipe 16
[2022-04-09T12:52:56.912061+02:00] [debug] started mgmt loop with pid 579
[2022-04-09T12:52:57.078008+02:00] [debug] qemu version detected: 5.2.0

there are four log lines, two times blocking, two times unblocking,
without information from which process these lines come from. Given that
two uppercase words are used the lines also reads a bit bulky and are
quite unlikely to be helpful for test reviewers. So better we remove
the lines at all.

Related progress issue: https://progress.opensuse.org/issues/107998

As tina.mueller@suse.com found out in
https://progress.opensuse.org/issues/107998 the log lines in
signalblocker might cause race conditions in our tests.
Also, I consider the output in a usual openQA like in the following log
excerpt not helpful to read:

```
[2022-04-09T12:52:55.814954+02:00] [debug] loaded 10829 needles
[2022-04-09T12:52:56.491100+02:00] [debug] Blocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.589482+02:00] [debug] Unblocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.782965+02:00] [debug] 65300: channel_out 15, channel_in 14
[2022-04-09T12:52:56.799859+02:00] [debug] Blocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.901592+02:00] [debug] Unblocking SIGCHLD and SIGTERM
[2022-04-09T12:52:56.911728+02:00] [debug] 579: cmdpipe 13, rsppipe 16
[2022-04-09T12:52:56.912061+02:00] [debug] started mgmt loop with pid 579
[2022-04-09T12:52:57.078008+02:00] [debug] qemu version detected: 5.2.0
```

there are four log lines, two times blocking, two times unblocking,
without information from which process these lines come from. Given that
two uppercase words are used the lines also reads a bit bulky and are
quite unlikely to be helpful for test reviewers. So better  we remove
the lines at all.

Related progress issue: https://progress.opensuse.org/issues/107998
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

Users sometimes think these log messages are related to some other problem which was never actually true. So I suppose it is better to remove them anyways.

@perlpunk
Copy link
Contributor

perlpunk commented Apr 9, 2022

See my comment in the issue. If it is a problem that parent and child write both to STDERR, then removing only two log statements won't remedy the actual problem, because there are probably other log statements in the child. It might just make the test pass.

I set the PR to not-ready.

@okurz
Copy link
Member Author

okurz commented Apr 9, 2022

@perlpunk both Martchus and me consider the log messages confusing and not necessary for other reasons. I suggest to still merge this PR if you agree and we can still look into your minimum reproducer with the log lines still included into getting that fixed "properly".

@perlpunk perlpunk removed the not-ready label Apr 9, 2022
@mergify mergify bot merged commit 112ac9e into os-autoinst:master Apr 11, 2022
@okurz okurz deleted the feature/driver_log_blocking branch April 11, 2022 11:18
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.

None yet

3 participants