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

Fixes for --stop-before flag. #1667

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

kacf
Copy link
Member

@kacf kacf commented Sep 25, 2024

No description provided.

@mender-test-bot
Copy link

@kacf, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".


my commands and options

You can trigger a pipeline on multiple prs with:

  • mentioning me and start pipeline --pr mender/127 --pr mender-connect/255

You can start a fast pipeline, disabling full integration tests with:

  • mentioning me and start pipeline --fast

You can trigger GitHub->GitLab branch sync with:

  • mentioning me and sync

You can cherry pick to a given branch or branches with:

  • mentioning me and:
 cherry-pick to:
 * 1.0.x
 * 2.0.x

This is done for two main reasons:

First, it is more intuitive to name the state you want to stop before
instead of the ones you want to stop after. For example, multiple
states lead to `ArtifactRollback_Enter`; with the `--stop-after` flag
you'd have to specify both `ArtifactInstall_Error` and
`ArtifactCommit_Error`.

Second, in the orchestrator it is not a good fit to use
`--stop-after`, for reasons explained in the corresponding commits
there, and renaming keeps them more in sync.

This has little effect on how it works, it's just using different
names, so both tests and usage stay nearly the same, just
name-adjusted.

Cancel-changelog: b60beaf
Cancel-changelog: 2c1d922

Changelog: Add `--stop-before` flag which can be used with the
`install`, `commit`, and `rollback` standalone commands to stop before
certain states. Use `resume` to continue, which also supports the same
flag. These are the allowed states:
* `ArtifactInstall_Enter`
* `ArtifactCommit_Enter`
* `ArtifactCommit_Leave`
* `ArtifactRollback_Enter`
* `ArtifactFailure_Enter`
* `Cleanup`
The flag can be specified multiple times.

Ticket: MEN-7115

Signed-off-by: Kristian Amlie <[email protected]>
This allows us to be a bit more selective when deciding whether or not
to print it, which helps in upcoming commits.

Signed-off-by: Kristian Amlie <[email protected]>
Changelog: Title
Ticket: None

Signed-off-by: Kristian Amlie <[email protected]>
First and foremost, make sure that repeated resuming before the same
state, with the same `--stop-before` flag, is a noop. It should keep
stopping there. Also make sure that once we have started executing the
path after that, it should no longer be possible to go back.

For this we introduce some new DB flags, but since the existing ones
have not been released yet, this should be fine.

Finally, simplify test conditions by removing the query states from
the log, since they have no effect on the flow.

Signed-off-by: Kristian Amlie <[email protected]>
@mender-test-bot
Copy link

mender-test-bot commented Sep 25, 2024

Merging these commits will result in the following changelog entries:

Changelogs

mender (rename)

New changes in mender since master:

Bug Fixes
  • Fall back on bootloader when boot env modification tool is broken.
Features
  • Add --stop-before flag which can be used with the
    install, commit, and rollback standalone commands to stop before
    certain states. Use resume to continue, which also supports the same
    flag. These are the allowed states:
    • ArtifactInstall_Enter
    • ArtifactCommit_Enter
    • ArtifactCommit_Leave
    • ArtifactRollback_Enter
    • ArtifactFailure_Enter
    • Cleanup
      The flag can be specified multiple times.
      (MEN-7115)

@kacf
Copy link
Member Author

kacf commented Sep 25, 2024

Must be merged together with mendersoftware/meta-mender#2171.

@kacf kacf changed the title Fixes for `--stop-before- flag. Fixes for --stop-before flag. Sep 25, 2024
Copy link
Contributor

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

See my comment wrt ArtifactCommit_Leave below.

Also, I think you got confused by testing this PR with mendersoftware/meta-mender#2171 - what we need to fix for this PR is mender-image-tests use of this new flag.

Comment on lines +1495 to +1509
case "$1" in
NeedsArtifactReboot|SupportsRollback)
:
;;
*)
case "$1" in
NeedsArtifactReboot|SupportsRollback)
:
;;
*)
echo "$1" >> $TEST_DIR/call.log
;;
esac
;;
esac
Copy link
Contributor

Choose a reason for hiding this comment

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

Search and replace error here 🔎

;;
esac
;;
esac
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

*)
echo "$1" >> $TEST_DIR/call.log
;;
esac
)";
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fix (thanks!) should have been in a separate commit.

Comment on lines -2177 to +2178
"--stop-after",
"ArtifactCommit",
"--stop-before",
"ArtifactCommit_Leave",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between stopping before ArtifactCommit_Leave or Cleanup? For the successful case, there is no real difference. And yes, for failure it will behave different (it would stop or not), but maybe we could merge these two use cases and stop only before Cleanup?

From the API point of view, to allow stopping before a Leave state sounds strange. Note that every other allowed state to stop before are Enter states (or Cleanup)

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.

3 participants