CI pipeline reliability: netapi key-leak, event tagger, and docs dedup fixes (#69728, #69730, #69724)#69733
Open
ggiesen wants to merge 5 commits into
Open
CI pipeline reliability: netapi key-leak, event tagger, and docs dedup fixes (#69728, #69730, #69724)#69733ggiesen wants to merge 5 commits into
ggiesen wants to merge 5 commits into
Conversation
The rest_cherrypy, rest_tornado and rest_wsgi doc pages document
overlapping HTTP routes, so each shared route registered multiple
httpdomain index entries. sphinxcontrib-httpdomain only detects the
duplicates in merge_domaindata, which runs when sphinx -j parallel
reader chunks are merged, so the -W builds in tools docs (Prepare
Release, Documentation) fail intermittently depending on where the
chunk boundary lands:
WARNING: duplicate HTTP post method definition / in
doc/ref/netapi/all/salt.netapi.rest_tornado.rst, other instance
is in doc/ref/netapi/all/salt.netapi.rest_wsgi.rst
When it hits, the release patch artifact is never produced and every
downstream build job in the run fails with 'Artifact not found'.
Mark the tornado and wsgi copies of the shared routes with :noindex:
so each route is registered exactly once, by the canonical
rest_cherrypy reference. noindex'd directives never enter the domain
data, so the merge collision is impossible under any chunking. Page
content is unchanged and no :http: cross-references exist that could
be affected. This also makes the HTTP routing index deterministic; it
previously pointed at whichever page the readers processed last.
Fixes saltstack#69724
sphinxcontrib-httpdomain's add_target_and_index always appends the signature anchor and only gates the global route registration behind :noindex:, but Sphinx's ObjectDescription.run skips the whole method when noindex is set, so the previous commit's dedup also dropped the per-page anchors and permalinks from the rest_tornado and rest_wsgi endpoint signatures. Anchors are per-document HTML ids and cannot collide across pages; only the global registration can produce the parallel-merge duplicate warnings. Add a small extension that hides the noindex option from Sphinx's outer gate and re-presents it to httpdomain's inner gate, restoring the anchors and permalinks (existing deep links into those pages keep working) while the routes stay out of the domain data, so the duplicate-route collision remains impossible under any chunking.
The startup_states and salt_call ownership test modules start extra
minions against the shared session master via 'with factory.started():
yield'. That stops the minion process at teardown but leaves the
accepted key on the master, so later tests in the same session that
target '*' (the netapi integration tests) key-match minions that no
longer exist. The result is the Rocky Linux 9 integration tcp/zeromq
pair failing most 3006.x PR runs:
AssertionError: assert ['minion-X', 'minion-X-empty-string',
'minion-X-highstate', 'minion-X-sls', 'minion-X-top',
'non-root-minion-Y', 'sub-minion-Z'] == ['minion-X', 'sub-minion-Z']
plus 30 second TimeoutErrors waiting for returns from the dead minions.
Nightly runs do not hit it because test sharding separates these
modules from the netapi tests.
Remove each extra minion's key from the master once the minion is
stopped, restoring the isolation the netapi assertions rely on.
Fixes saltstack#69728
A failing state compilation returns a list of error strings (or a plain string from some renderers) rather than a mapping of per-state results. _fire_ret_load_specific_fun assumed a dict and crashed on ret.items(), logging 'Event iteration failed with exception: list object has no attribute items' at ERROR for every failed compile. The integration tests that assert that message never appears (tests/pytests/integration/states/test_state_test.py) fail whenever they share a run with a failing compile, which is part of the Rocky Linux 9 integration job noise on 3006.x. There are no state tags in a non-dict return, so skip it at debug level instead. Dict-shaped returns keep firing the per-tag failure events unchanged, covered by a new regression pair: the list-return test fails on the previous code via the logged error, and the dict-return test pins the two sub events (old-style dup tag and the namespaced job error tag) with their enriched payload. Fixes saltstack#69730 Refs saltstack#69728
test_slsutil.py::test_renderer_file runs slsutil.renderer on a file
that does {% from "map.jinja" import abc %}. Over salt-ssh,
slsutil.renderer fetches the requested file but does not ship its
jinja-imported files to the target; only a state run syncs the full
state tree to the target's file cache. So the render fails with
'Jinja error: map.jinja / TemplateNotFound' unless an earlier test in
the same session already ran a state command that warmed the shared
salt-ssh target cache -- flaky by test ordering in CI, deterministic
when run in isolation.
Prime the cache once in the state_tree fixture with a test-mode state
apply, so the renderer tests do not depend on another test having
warmed it. Verified against a local 3006.x reproduction: the test
failed 3/3 in isolation before and passes 3/3 after, with the rest of
the module unaffected.
Fixes saltstack#69738
ggiesen
force-pushed
the
fix-ci-pipeline-omnibus
branch
from
c89f9e0 to
392915d
Compare
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
2 participants
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?
Bundles the CI pipeline reliability fixes that are individually open as #69725, #69729 and #69731. They are combined here because none can demonstrate a green 3006.x PR run on its own: the Rocky/Ubuntu integration jobs only go green when the test-isolation and event-handling fixes are present together, so a single branch carrying all of them is the only way to show the pipeline actually recovers.
Contents (each is a clean, separately-reviewable commit):
Deduplicate HTTP route index entries across netapi doc pages (Flaky docs -W build: duplicate HTTP method definitions across netapi pages fail Prepare Release under -j auto #69724) -- fixes the intermittent
duplicate HTTP post method definitionfailure that fails Prepare Release under-W -j autoand cascades "Artifact not found" into every build job.Keep HTML anchors on noindex'd httpdomain directives -- restores the per-page anchors/permalinks the dedup would otherwise drop, via a small
doc/_extextension.Delete leaked minion keys at teardown in integration test fixtures (Rocky 9 integration tcp/zeromq jobs fail most 3006.x PR runs: leaked minion keys from earlier test modules break netapi '*' targeting #69728) -- the
startup_statesand salt-call ownership fixtures left their extra minions' accepted keys on the shared session master, so later netapi tests targeting*matched dead minions (inflated minion lists, plus 50 s command timeouts as the master waited on returns that never came).Skip sub events for non-dict job returns in the event tagger (Master event tagger crashes on non-dict job returns (failing state compiles): 'list' object has no attribute 'items' at event.py:941 #69730) -- a failing state compilation returns a list of error strings, and
_fire_ret_load_specific_funcrashed onret.items(), loggingEvent iteration failed with exception: 'list' object has no attribute 'items'for every failed compile and failing thetest_state_testassertions that scan for that message.Prime the salt-ssh file cache for the renderer tests (Flaky ssh test_renderer_file: salt-ssh slsutil.renderer does not ship jinja imports; test depends on cache warmed by another test #69738) --
test_renderer_filerenders asalt://file with a jinja{% from "map.jinja" %}import; over salt-ssh,slsutil.rendererships the requested file but not its jinja imports, so the test only passed when an earlier state test had warmed the shared salt-ssh target cache (flaky by ordering; reproduced failing 3/3 in isolation, passing 3/3 after the fix).What issues does this PR fix or reference?
Fixes #69724
Fixes #69728
Fixes #69730
Fixes #69738
Diagnosis
The Rocky Linux 9 and Ubuntu 24.04 integration failures on 3006.x PRs are one root cause with several faces. The leaked minion keys (#69728) make netapi
*targeting match dead minions, and while the master waits on those minions the event stream backs up, so unrelated commands (jinja.load_map,state.orchestrate, netapi POSTs) time out at 50 s. The event tagger crash (#69730) adds error-log noise that trips thetest_state_testassertions. With both fixed, the integration jobs recover; the event fix is already visibly effective (the crash signature drops to zero on #69731's own run). The docs fixes (#69724) address a separate, orthogonal part of the pipeline (the release/docs build cascade).Merge requirements satisfied?
The event fix carries direct + inverse unit tests (list-return skips quietly, dict-return still fires both sub events); the isolation fix is exercised by the same integration jobs this PR stabilizes; the docs changes were validated against the pinned docs toolchain (
make man/make htmlclean under-W -j auto, anchors preserved, routes registered once).If maintainers prefer to land these granularly, #69725 / #69729 / #69731 remain open and this PR can be closed in their favor.
Commits signed with GPG?
No