-
Notifications
You must be signed in to change notification settings - Fork 183
mantle/kola: check console log for error even in timeout #4371
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
mantle/kola: check console log for error even in timeout #4371
Conversation
We don't need the message about creating the filesystem to show up in our kola logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to improve test reliability by checking console logs for expected errors even in timeout scenarios, which is a good approach for handling race conditions. The refactoring of the console checking logic into a helper function is a positive change for code clarity and reuse. However, I've identified a critical bug in the new timeout handling logic where the original search pattern is incorrectly overwritten, leading to the wrong error message being checked. I've provided a code suggestion to correct this. The other changes, such as reducing a timeout and cleaning up command execution, are reasonable.
There is a race condition that occurs for /dev/virtio-ports/com.coreos.ignition.journal and it's hard to track down. In this case we are just detecting a failure anyway. If we can find the original search failure term after the timeout happens let's consider that success too. See coreos/fedora-coreos-tracker#2019 for more context.
All of these tests run within 15 seconds today. Let's drop the timeout to one minute, which will enable to detect failures quicker and also allow us to fallback to the new "best effort" console check in the timeout path quicker.
dbb32ac to
7ab7057
Compare
prestist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes make sense to me LGTM!
|
/retest |
We've decided to just work around the race condition in coreos/coreos-assembler#4371 so we shouldn't see this failure any longer. Closes coreos/fedora-coreos-tracker#2019
We've decided to just work around the race condition in coreos/coreos-assembler#4371 so we shouldn't see this failure any longer. Closes coreos/fedora-coreos-tracker#2019
There is a race condition that occurs for
/dev/virtio-ports/com.coreos.ignition.journal and it's hard to
track down. In this case we are just detecting a failure anyway.
If we can find the original search failure term after the timeout
happens let's consider that success too.
See coreos/fedora-coreos-tracker#2019
for more context.