fix: Fix crash when Sync_Leave returns error during a deployment. #1653
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@kacf, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline". my commands and optionsYou can trigger a pipeline on multiple prs with:
You can start a fast pipeline, disabling full integration tests with:
You can trigger GitHub->GitLab branch sync with:
You can cherry pick to a given branch or branches with:
|
|
Being tested together with mendersoftware/integration#2640. |
|
@mender-test-bot start pipeline --pr integration/2640 |
|
Hello 😺 I created a pipeline for you here: Pipeline-1403668934 Build Configuration Matrix
|
lluiscampos
approved these changes
Aug 7, 2024
I started the build again (using the bot) to run the pipeline with |
Thanks! I had a "doh!" moment as soon as I saw your message! 😄 |
kacf
force-pushed
the
sync_leave_crash
branch
2 times, most recently
from
db58e6b
to
6a70ff1
Compare
lluiscampos
approved these changes
Aug 8, 2024
Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech>
The problem is that once we have entered the `sync_leave_download` internal state, a deployment has started, and a deployment always has to be ended, to properly clean up. This is fine when `sync_leave_download` is successful and proceeds into subsequent download states, which always end with the `end_of_deployment_state`. But if `sync_leave_download` fails, then it goes directly to `sync_error`, which does not know about the deployment, and does not go to `end_of_deployment_state`. Fix this by creating a dedicated `sync_error_download` state. This bug happened only occasionally in the test `test_state_scripts[Corrupted_script_version_in_etc-test_set11]` in the integration repository, typically under load. The reason it happened only occasionally is that the test is programmed to fail in `Sync_Leave` only once, and inventory submission is always run first. So by the time it got to the deployment it was always returning success. But under load it could happen that the inventory submission failed, which would run `Sync_Error` instead and skip `Sync_Leave`, leaving it around for the upcoming deployment, where the bug would then occur. Testing this in unit tests requires supporting more than one deployment run, which requires an extra member in the exit state. In the spirit of trying to limit space requirements for embedded system, I've made this member, which is only used for tests, debug-only. Changelog: Fix crash when `Sync_Leave` returns error during a deployment. The error message would be: ``` State machine event DeploymentStarted was not handled by any transition ``` and would happen on the next deployment following the `Sync_Leave` error. With a long polling interval, this could cause the bug to be latent for quite a while. Ticket: MEN-7379 Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech>
Although somewhat rare, it seems to have more than 50% chance of happening at least once in one of the state script tests of an integration test run. What's happening is that when running `systemctl restart mender-updated` from an `ArtifactReboot` script, systemd kills the whole control group, including the script. This is fine in itself, but if the script happens to terminate before Mender does, then it will be recorded as an error, and the Mender will start on its error path. What happens afterwards depends on how far it gets before it is also killed. Usually it will not get further than executing the first `ArtifactReboot_Error` script, but it could potentially go all the way to a rollback. Either of those is wrong. The issue won't affect users of `rootfs-image`, since it uses `NeedsArtifactReboot=Automatic`, which doesn't call the update module's `ArtifactReboot`, but it can affect other means of running `ArtifactReboot`, such as restarting it with systemctl after a package upgrade. The best way to mitigate this is to make sure the script survives longer than Mender. This can be done in the script itself with a shell `trap` or similar, since systemd sends SIGTERM first. But in order to make this less surprising for users, switch systemd to kill the client first in all cases, leaving scripts to be killed only if the termination times out and it has to resort to SIGKILL. This started appearing with Yocto scarthgap, and why it has appeared now is anyone's guess, it could be multiple reasons: * Exact killing pattern of systemd might have changed slightly. * The new kernel might kill processes in the same control group slightly differently. Whatever the reason, it causes the script to sometimes terminate before Mender, causing the issue. Changelog: Fix systemd race condition when restarting mender from `ArtifactReboot` script. The symptom would be an error message like: ``` Process returned non-zero exit status: ArtifactReboot: Process exited with status 15 ``` And the `ArtifactReboot_Error` state scripts would be executed, even though they should not. Ticket: None Signed-off-by: Kristian Amlie <kristian.amlie@northern.tech>
|
Merging these commits will result in the following changelog entries: Changelogsmender (sync_leave_crash)New changes in mender since master: Bug Fixes
|
|
One new fix. This one is unrelated, but started showing up independently once I removed flaky. |
|
Only one known spurious failure. Merging. |
|
Hello 😺 This PR contains changelog entries. Please, verify the need of backporting it to the following release branches: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem is that once we have entered the
sync_leave_downloadinternal state, a deployment has started, and a deployment always has
to be ended, to properly clean up. This is fine when
sync_leave_downloadis successful and proceeds into subsequentdownload states, which always end with the
end_of_deployment_state. But ifsync_leave_downloadfails, then itgoes directly to
sync_error, which does not know about thedeployment, and does not go to
end_of_deployment_state.Fix this by creating a dedicated
sync_error_downloadstate.This bug happened only occasionally in the test
test_state_scripts[Corrupted_script_version_in_etc-test_set11]inthe integration repository, typically under load. The reason it
happened only occasionally is that the test is programmed to fail in
Sync_Leaveonly once, and inventory submission is always runfirst. So by the time it got to the deployment it was always returning
success. But under load it could happen that the inventory submission
failed, which would run
Sync_Errorinstead and skipSync_Leave,leaving it around for the upcoming deployment, where the bug would
then occur.
Testing this in unit tests requires supporting more than one
deployment run, which requires an extra member in the exit state. In
the spirit of trying to limit space requirements for embedded system,
I've made this member, which is only used for tests, debug-only.
Changelog: Fix crash when
Sync_Leavereturns error during adeployment. The error message would be:
and would happen on the next deployment following the
Sync_Leaveerror. With a long polling interval, this could cause the bug to be
latent for quite a while.
Ticket: MEN-7379
Signed-off-by: Kristian Amlie kristian.amlie@northern.tech