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

[master] Don't block when evaluating requisites of parallel states #66956

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

Conversation

lkubb
Copy link
Contributor

@lkubb lkubb commented Oct 10, 2024

What does this PR do?

Evaluates requisites between parallel states less synchronously.

What issues does this PR fix or reference?

Fixes #59959

Previous Behavior

A state with unmet requisites marked as parallel would block state execution until all requisites were met, i.e. its requisites would be evaluated synchronously.

New Behavior

Execution of a parallel state with unmet requisites is postponed, allowing other parallel states with met dependencies to be started immediately.

This does not implement complete parallelization of requisite checks. The stack of postponed parallel states is iterated over after each single state execution, meaning a non-parallel state in between causes parallel ones whose dependencies finish while it is executing to be started only after it has finished.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Don't block when evaluating requisites of parallel states [master] Don't block when evaluating requisites of parallel states Oct 10, 2024
@lkubb lkubb marked this pull request as ready for review October 10, 2024 19:22
@lkubb lkubb requested a review from a team as a code owner October 10, 2024 19:22
twangboy
twangboy previously approved these changes Oct 25, 2024
@lkubb
Copy link
Contributor Author

lkubb commented Nov 2, 2024

Sorry for dismissing the review, I noticed the patch introduced a regression when non-parallel states require parallel ones.

Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

Could we get a changelog for this. If you haven't already, you'll need to create an issue.

@lkubb
Copy link
Contributor Author

lkubb commented Nov 6, 2024

There is a news fragment at https://github.com/saltstack/salt/pull/66956/files#diff-895ed1969c9b8dd072a1cdd582b61bb05de4311f862a52dedebff1edce889248 and an issue at #59959, which is referenced in the changelog entry.

Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

My bad... the "files changed" wasn't showing me everything. I see the changelog now

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.

[BUG] Parallel orchestration needs improvement
2 participants