Fix manage.status reporting all minions as up (#69582)#69585
Open
dwoz wants to merge 2 commits into
Open
Conversation
manage._ping iterates the events yielded by LocalClient.get_cli_event_returns and adds every minion id it sees to the returned set. After get_cli_event_returns started defaulting expect_minions to True, the iteration also receives synthesized ``no_return`` rows for minions that did not respond. _ping treated those rows as successful returns, so manage.status reported every targeted minion as up and manage.down returned nothing. Pass expect_minions=False explicitly from _ping and skip any ``no_return`` row that does sneak through, so the runner once again derives up/down from real returns. Fixes saltstack#69582
Cover the runner end to end against live salt-minions plus a fake accepted-but-absent minion key. Reproduces the original symptom of issue saltstack#69582 without mocks: before the runner fix, manage.status returned every targeted minion in ``up`` and an empty ``down`` list because the synthesized ``no_return`` rows from LocalClient.get_cli_event_returns were counted as successful returns. The new tests verify that live minions land in ``up``, the fake absent minion lands in ``down``, and no ``no_return`` placeholder strings leak through to the runner output.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What does this PR do?
salt-run manage.status,manage.up, andmanage.downonce againreport which minions actually responded. They were reporting every
targeted minion as up and never listing any minion as down.
What issues does this PR fix or reference?
Fixes #69582
Previous Behavior
On 3008.1,
salt-run manage.downalways returned an empty list, evenfor minions that were stopped or unreachable, and
salt-run manage.upreturned every targeted minion.
New Behavior
manage.status,manage.up, andmanage.downdistinguish betweenminions that returned and minions that timed out. Down minions show
up in
manage.down(and are excluded frommanage.up) the way theydid on prior releases.
Root cause
manage._pingiterates the events yielded byLocalClient.get_cli_event_returnsand adds every minion id it seesto its
returnedset. Afterget_cli_event_returnsstarted defaultingexpect_minionsto True so thesaltCLI emits per-target timeoutrows, the manage runner also started receiving synthesized
no_returnrows for minions that did not respond.
_pingtreated those rows assuccessful returns, leaving
not_returnedempty.The fix passes
expect_minions=Falsefrom_ping(the runner derivesup/down from real returns, not from synthesized rows) and skips
no_returnrows defensively in case a caller threadsexpect_minions=Truethrough kwargs.Merge requirements satisfied?
Commits signed with GPG?
Yes