Skip to content

Async batch#60269

Open
vzhestkov wants to merge 12 commits into
saltstack:masterfrom
vzhestkov:batch-async-impl
Open

Async batch#60269
vzhestkov wants to merge 12 commits into
saltstack:masterfrom
vzhestkov:batch-async-impl

Conversation

@vzhestkov

@vzhestkov vzhestkov commented May 28, 2021

Copy link
Copy Markdown
Contributor

What does this PR do?

Implementation of RFC#0002

#50546 port to master

All related tests migrated to pytest

@vzhestkov vzhestkov requested a review from a team as a code owner May 28, 2021 06:32
@vzhestkov vzhestkov requested review from twangboy and removed request for a team May 28, 2021 06:32
@garethgreenaway

garethgreenaway commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

@vzhestkov Just a heads up, there are quite a few merge conflicts here.

@vzhestkov

vzhestkov commented Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

@garethgreenaway thanks for pinging, I'll resolve them once I get some time for it, will try to do it as soon as possible

@garethgreenaway garethgreenaway added the Phosphorus v3005.0 Release code name and version label Apr 20, 2022
twangboy
twangboy requested changes May 2, 2022
View reviewed changes
Comment thread salt/cli/batch.py Outdated
Comment thread salt/cli/batch.py Outdated
Comment thread salt/cli/batch.py Outdated
Comment thread salt/cli/batch.py Outdated
Comment thread salt/cli/batch_async.py Outdated
Comment thread salt/cli/batch_async.py Outdated
Comment thread salt/cli/batch_async.py Outdated
Comment thread salt/master.py Outdated
Comment thread salt/utils/event.py Outdated
Comment thread tests/pytests/unit/cli/test_batch.py Outdated
@Ch3LL

Ch3LL commented May 20, 2022

Copy link
Copy Markdown
Contributor

There are test failures

@garethgreenaway

garethgreenaway commented May 26, 2022

Copy link
Copy Markdown
Contributor

@vzhestkov A few test failures here and some requested changes.

@Ch3LL

Ch3LL commented Jun 9, 2022

Copy link
Copy Markdown
Contributor

We are close to tagging for the RC release for phosphorus. I would like to get this one in if the tests get fixed up

@vzhestkov

vzhestkov commented Jun 9, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for pinging. I'll try to fix all the suggestions today or tomorrow morning

@vzhestkov vzhestkov force-pushed the batch-async-impl branch 9 times, most recently from 70c4853 to e1e4b03 Compare June 10, 2022 14:32
twangboy
twangboy requested changes Jun 10, 2022
View reviewed changes
Comment thread salt/cli/batch_async.py Outdated
Comment thread salt/cli/batch_async.py Outdated
Comment thread salt/master.py Outdated
Comment thread salt/master.py Outdated
Comment thread salt/utils/event.py Outdated
@vzhestkov

vzhestkov commented Jun 10, 2022

Copy link
Copy Markdown
Contributor Author

@twangboy thanks for pointing again, accidentally missed it. Fixed

twangboy
twangboy previously approved these changes Jun 14, 2022
View reviewed changes
@Ch3LL

Ch3LL commented Jun 16, 2022

Copy link
Copy Markdown
Contributor

Theres a few tests failing on multiple platforms that seem to be related.

@vzhestkov vzhestkov temporarily deployed to ci August 7, 2023 08:44 — with GitHub Actions Inactive
@vzhestkov vzhestkov temporarily deployed to ci August 7, 2023 09:00 — with GitHub Actions Inactive
@vzhestkov vzhestkov force-pushed the batch-async-impl branch 2 times, most recently from 40b7e09 to 6869add Compare August 7, 2023 09:40
@vzhestkov vzhestkov temporarily deployed to ci August 7, 2023 09:55 — with GitHub Actions Inactive
@vzhestkov vzhestkov temporarily deployed to ci August 7, 2023 09:55 — with GitHub Actions Inactive
@vzhestkov vzhestkov mentioned this pull request Feb 23, 2024
Closed
3 tasks
twangboy
twangboy previously approved these changes Feb 26, 2024
View reviewed changes
twangboy
twangboy previously approved these changes Jan 29, 2025
View reviewed changes
@vzhestkov vzhestkov mentioned this pull request Feb 13, 2025
Merged
3 tasks
@twangboy

twangboy commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

Looks like there are some test failures here

@twangboy

twangboy commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Please fix the conflicts

@dwoz

dwoz commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

@vzhestkov Can you please rebase this 🙏 . We'll try to get it into 3008.0

@whytewolf

whytewolf commented Mar 23, 2026

Copy link
Copy Markdown
Collaborator

looks like the current errors are related to the changes.

@vzhestkov

vzhestkov commented Mar 23, 2026

Copy link
Copy Markdown
Contributor Author

@whytewolf sure, I will check.

vzhestkov and others added 12 commits June 15, 2026 14:08
@vzhestkov @dincamihai @dwoz
Async batch implementation RFC#0002
c3390bd 
Co-authored-by: Mihai Dinca <dincamihai@gmail.com>
@vzhestkov @dwoz
Use asyncio
375089c
@vzhestkov @dwoz
Fix tornado usage
69af91c
@vzhestkov @dwoz
Fix async tests
d660bfb
@vzhestkov @dwoz
Fix connected check
623b8bb
@vzhestkov @dwoz
Remove remove_event_handler
413011b
@vzhestkov @dwoz
Fix batch call
dcf7ff2
@vzhestkov @dwoz
Fix async tests
a6746d7
@meaksh @dwoz
Make batch async working with asyncio
43f43be 
batch async: enhance trace logging

batch async: do not block event handler

additionally, only try to execute next batch once all previous minions
has been processed somehow

batch async: do not block current find_job execution

batch async: just start batch if all minions returns the ping

batch_async: ensure the start/done events are fired

batch async: do not wait to finalize batch execution

batch async: only trigger find job for current batch active minions

Prevent traceback due missing 'send' method when socket was closed

Modify some batch_async trace logs
@vzhestkov @dwoz
Make pre-commit check happy
81f3307
@vzhestkov @dwoz
Fix batch_async test after alignment with asyncio
aef3b5f
@dwoz
Fix test_gather_minions_with_batch_presence_ping KeyError and add docs
60a0231
@dwoz

dwoz commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Rebased onto master and addressed review feedback:

  • CI fix: gather_minions() fallback from self.opts["timeout"] to self.opts.get("timeout") — prevented KeyError when batch_presence_ping_timeout is absent and timeout is not in opts
  • Test fix: added missing "timeout": 10 to opts_no_pp in test_gather_minions_with_batch_presence_ping (same test asserted opts_no_pp["timeout"] at the assertion site, which would also have raised KeyError)
  • Docs: added .. versionadded:: 3009.0 to get_bnum, batch_get_opts, batch_get_eauth (batch.py) and BatchAsync class (batch_async.py); added Args/Returns to publish_batch in master.py
  • Changelog: added changelog/60269.added.md
  • Milestone: updated to Potassium v3009.0

The PR is not superseded by #69444. That PR fixed a 3008.x sync batch regression; this PR adds the new async batch subsystem (salt/cli/batch_async.py) implementing RFC-0002, which doesn't exist on master yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@twangboy twangboy twangboy left review comments

+1 more reviewer

@whytewolf whytewolf whytewolf approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

test:full Run the full test suite

Projects

None yet

Milestone

Potassium v3009.0

Development

Successfully merging this pull request may close these issues.

9 participants

@vzhestkov @garethgreenaway @Ch3LL @MKLeb @twangboy @dwoz @whytewolf @waynew @meaksh