Uh oh!
There was an error while loading. Please reload this page.
fix(test): wait for config-changed hook after juju.config() in set_app_runner_amount - #810
fix(test): wait for config-changed hook after juju.config() in set_app_runner_amount#810yanksyoon wants to merge 29 commits into
Conversation
Add a script that deletes dangling servers, keypairs, images and test security groups left by integration tests, plus a 6-hourly workflow on the private-endpoint runner.
Drop the scheduled cleanup workflow. Force-cancelled runs leave resources behind; the next integration suite now deletes CI-named OpenStack leftovers older than 6h before creating new ones.
Public cleanup entry point first; private helpers below. Module-level imports in conftest instead of inline.
Keypairs often lack created_at; treating unknown age as stale risked deleting in-progress CI resources. Skip those instead.
Wait for server delete before SGs, keep OpenStack teardown inside the connection context, best-effort start-up reaping, and tighten test-id regexes to the 8-char CI ids we generate.
Align OpenStack teardown/orphan helpers with existing integration suite call conventions.
Keep suite call style consistent; openstacksdk accepts the name positionally as used elsewhere in this repository.
Drop wait= on image delete (not supported), materialize list_servers, and log orphan cleanup failures with exc_info.
--use-existing-app-suffix is for local runs that keep long-lived resources; do not reap them at suite start.
Single source of truth for test resource names (charm naming.py, manager factories). Orphan cleanup consumes those matchers, no longer redefines regexes, deletes SGs, or uses future annotations. Only servers/images/keypairs that round-trips create are reaped.
Replace fuzzy shorthand (manager-IT, CI-named) with explicit references to github-runner-manager vs charm integration suites and what they create.
Drop matching algorithms and implementation notes from interface comments; the code already encodes those details.
Longer prefixes first is for readability, not early-exit matching.
Charm app suffixes always start with a lowercase letter; keep manager test-runner- ids free to begin with a digit.
Exact name filters miss runner VMs named test-<suffix>-N. Also soften naming module wording around suite scope and random suffixes.
Catch only ResourceNotFound and ConflictException instead of broad Exception. Unexpected failures (auth, bugs) now propagate. Inline the try/except at each resource loop instead of a shared helper.
Satisfies flake8 complexity (C901) and docstring (DCO010) rules.
…n arg - Extract suite teardown from openstack_connection_fixture to reduce flake8 complexity below 10. - Add Args/Returns docstring sections to satisfy DCO020/DCO030. - Remove unused connection param from _delete_resource helper.
jubilant.Juju.config() is fire-and-forget — it sets the config and returns before the config-changed hook fires. The test then polls check-runners while the service is still running with the previous base_virtual_machines value, causing a timeout waiting for runners. Three changes: 1. Verify config was applied by reading back after juju.config() 2. Wait for model to settle (juju.wait(all_active)) after config change so the config-changed hook completes and the service restarts with the new base_virtual_machines value 3. Add a 30s pre-poll grace period in wait_for_runner_ready to give the service time to start creating runners before the first check
yanksyoon
commented
Jul 31, 2026
Re-enabled tmate debug for the integration test jobs to inspect the runner and OpenStack VM state during failures. |
yhaliaw
left a comment
There was a problem hiding this comment.
Approved with minor changes needed
🤝 Human review with AI assistance.
| # Wait for the config-changed hook to complete and the service to be restarted | ||
| # with the new config. Without this, the test polls check-runners while the | ||
| # service is still running with the previous base_virtual_machines value. | ||
| self.juju.wait( | ||
| lambda status: jubilant.all_active(status, app_name), | ||
| timeout=60 * 5, | ||
| ) |
There was a problem hiding this comment.
self.juju.wait(lambda status: jubilant.all_active(status, app_name), timeout=60*5) doesn't guarantee config-changed has actually run before wait_for_runner_ready starts polling. The app is typically still active from before the config change, and all_active only inspects workload status — it can't distinguish "never left active" from "went through maintenance and came back."
Note that jubilant.wait already defaults to successes=3 (three consecutive polls must satisfy the predicate) with delay=1.0, so this isn't a bare one-shot check — but that's still only a ~2s settle window. If the unit is already active when the wait starts and config-changed hasn't kicked off yet, three quick polls of "active" can pass well before the hook actually runs, leaving the same race this PR is meant to close, just narrower.
Worth asserting on something that actually reflects the new config having been applied — e.g. a workload status message set post-config-changed, or the rendered service config on the unit — rather than relying on all_active alone. The read-back in the block above (L162-167) doesn't help either, since it only confirms the value in the Juju controller's datastore, not that the charm consumed it.
🤖 AI-assisted
Problem
test_check_runnerintest_charm_runner.pyfails withTimeout waiting for 2 runner(s) to be ready. Investigation on the debug tmate session revealed:base_virtual_machines: 0throughout the testdesired=0 current=0every reconcile cycleconfig-changedhook fired (at 15:34:20), none after the test calledjuju.config()jubilant.Juju.config()is fire-and-forget — it returns before the hook firesThe test polls
check-runnerswhile the service is still running with the previousbase_virtual_machinesvalue.Fix
Three changes in two files:
juju.config()and assertjuju.wait(all_active)after config change so the hook completescheck-runnerspoll inwait_for_runner_readyTesting