-
Notifications
You must be signed in to change notification settings - Fork 149
test: Fix factory reset test failure on CS10 #1759
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
Conversation
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 attempts to fix a failing factory reset test by introducing a sleep and a forceful reboot. While this might make the test pass, it introduces potential flakiness with the fixed sleep duration and uses a dangerous reboot command that can cause data loss and mask underlying system issues. My review provides a suggestion to use sync for better reliability and raises concerns about the forceful reboot, recommending an investigation into the root cause of the shutdown problem.
| sleep 10 | ||
| tmt-reboot -c "systemctl --force --force reboot" |
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.
Using a fixed sleep is not a reliable way to ensure file operations are complete and can lead to flaky tests. It's better to use sync to flush file system buffers to disk. This is more deterministic and avoids unnecessary delays.
Additionally, using systemctl --force --force reboot is very aggressive and can mask underlying issues that prevent a graceful reboot, potentially leading to data loss. While this might be a temporary measure for debugging, it would be best to investigate the root cause of the reboot failure for a more robust solution.
I suggest replacing sleep 10 with sync.
sync
tmt-reboot -c "systemctl --force --force reboot"
6460dd4 to
aa088bd
Compare
|
Before reboot, system looks good: After reboot |
739cc2c to
4bc0d12
Compare
Sometimes systemd daemons are still running old binaries and response "Access denied" when send reboot request Force a full sync before reboot and Allow more delay for bootc to settle Signed-off-by: Xiaofeng Wang <henrywangxf@me.com>
4bc0d12 to
3237c6e
Compare
|
The |
|
|
||
| # Sometimes systemd daemons are still running old binaries and response "Access denied" when send reboot request | ||
| # Force a full sync before reboot | ||
| sync |
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.
But this looks like purely a systemd bug that we should report right?
I don't understand why systemd would have old binaries...we shouldn't be touching the running OS?
|
Thanks for chasing this! |
Sometimes systemd daemons are still running old binaries and response "Access denied" when send reboot request.
This PR forces a full sync before reboot and allow more delay for bootc to settle.