fix(arc): count this host's orgs however they were supplied - #16
Merged
Merged
Conversation
The fleet the role validates and looks things up in was built from every inventory host's hostvars, which hold inventory variables and facts but not play variables. Orgs passed to the role as a play variable were therefore missing from it, and measured sizing, which leaves the fleet's scale-set namespaces out of each node's requests, counted the running runner pods as other workloads and set too low a ceiling. An implicit localhost was missing for the same reason. This host's orgs now come from github_runner_arc_orgs as the play resolves it, and other hosts' still from their hostvars. The check for whether any host has a heartbeat gist id reads this host's the same way, so a gist id passed as a play variable no longer triggers the gist bootstrap.
Mearman
marked this pull request as ready for review
September 26, 2026 05:35
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🎉 This PR is included in version 1.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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.
This came out of a consumer adopting v1.2.0. It passes its orgs to the role as a play variable, and its measured
maxRunnerscame out far too low.The role builds the fleet from
hostvarsfor every host ingroups['all']. The fleet is what the cross-host checks use, and it also supplies the namespaces measured sizing leaves out of each node's requests.hostvarsholds inventory variables and facts but not play variables. An org passed as a play variable was missing from the fleet, so its namespaces weren't left out, and the runner pods already running counted as other workload against the ceiling they're sized by. An implicit localhost isn't ingroups['all']at all, so it had the same problem. The consumer's workaround was aset_factofgithub_runner_arc_orgsbefore including the role.Now this host's orgs come from
github_runner_arc_orgsas the play resolves it, and other hosts' still come from theirhostvars. This host is taken out of thehostvarspass, so inventory orgs aren't counted twice. The heartbeat gist bootstrap had the same blind spot: a gist id passed as a play variable looked like no gist anywhere, and would have created one. It now reads this host's id the same way.tests/unit/test_fleet_orgs.pyruns the real validation tasks in a play. It covers orgs as a play variable on an inventory localhost and on an implicit one, and a measured ceiling with two runner pods already running in the scale set's namespace (2 with the fix, 1 without). It also covers another host's orgs fromhost_vars, this host's inventory orgs counted once, the same org on two hosts still flagged as a duplicate, and a play-variable gist id not triggering the bootstrap, using a stand-inghso nothing can reach GitHub. Six of the seven fail against v1.2.0; the one about counting this host's inventory orgs once passes on both, as it should.The fix making
app_idoptional with an existing App Secret is stacked on this one, because both change the same lines ofvalidate.yml. Against a live cluster in check mode, a fleet with its orgs in host_vars ran the same tasks with the same result as v1.2.0.