[yanksyoon]

What this PR does

Lets the operator pin the OpenStack security group a GARM scale set's runners are launched in, by passing an ID or name straight through to the provider.

garm-configurator                        garm                            provider / Nova
─────────────────                        ────                            ───────────────
security-group: sg-runner
        └── relation: security_group ──► security_groups extra spec ──► runner port

• garm-configurator: new optional security-group configuration. It is published on the garm-configurator relation as security_group, like image_visibility.
• garm: reads security_group from the configurator's databag and puts it in the scale set's extra_specs.security_groups, which garm-provider-openstack (v0.1.5, already pinned in the rock) maps to the instance port. A change is picked up by the existing drift detection, so the scale set is updated in place.

Why we need it

Runners in one OpenStack project share its default security group, so they can reach each other, and a compromised job can attack its peers (ISD-7035). Pinning a locked-down group isolates them. The group itself stays the operator's to create and manage: the charms only pass the reference through.

This is the minimal version of ISD-7035. It does not create or converge the group. #436 manages a group's lifecycle from the charms; it is a separate, larger change and this PR neither builds on it nor modifies it. If both land, security-group can name the group #436 manages.

Test plan

• Unit tests (tox -c tox.toml -e unit, per charm): garm 454 passed, garm-configurator 66 passed. The new tests assert the configured group reaches the provider:
  • test_create_sends_security_group_as_an_extra_spec — the create call carries extra_specs == {"security_groups": ["sg-runner"]}.
  • test_update_converges_security_group — a changed or removed group converges in one update instead of re-updating on every reconcile.
  • test_security_group_follows_the_configurator_databag — the databag value lands on the scale set spec, and an absent value stays unset for an older configurator.
  • test_security_group_written_to_garm_configurator_relation — the charm config reaches the databag, empty when unset.
• fmt, lint, complexity, static: clean for both charms.
• Not covered by an integration test: attaching the group needs a real Neutron group to exist, which the tests do not model. tox -e garm-e2e against a tenant would be the check.

Review focus

• The group is read with str(charm.config.get(..., "")).strip() rather than _get_optional_string_config(...) plus or "": ScalesetConfig.from_charm and _configure_garm_relation both sit exactly at the cognitive-complexity limit, and either form pushes one of them over. That is why the field defaults to "" instead of None.
• The extra spec is a list (security_groups), matching garm-provider-openstack, but the configurator takes one group. A list-valued option is the obvious next step if operators need more than one.

Potential breaking changes

None. The option is optional; while unset the charm publishes an empty security_group and the scale set's extra specs are unchanged, so the provider keeps attaching the project's default group.

New dependencies, APIs, modules, or workflow changes

• New charm option: garm-configurator security-group.
• New relation databag key: security_group on the garm-configurator relation.
• No new dependencies. security_groups is an existing garm-provider-openstack extra spec.

Checklist

• Changes comply with the project's coding standards and guidelines (see CONTRIBUTING.md and STYLE.md)

• CONTRIBUTING.md has been updated upon changes to the contribution/development process (n/a: no process change)

• Technical author has been assigned to review the PR in case of documentation changes (usually *.md files)

• I updated docs/changelog.md with user-relevant changes

• I used AI to assist with preparing this PR

• I added or updated tests as needed (unit and integration)

• If integration test modules are used: (n/a: no integration module change)

• If this PR involves a Grafana dashboard: (n/a)

• If this PR involves Terraform: (n/a: the module takes a generic config map)

• If this PR involves Rockcraft: (n/a)

• If this PR adds/removes a charm, or changes a charm's base class, conventions, tooling, or repo structure: (n/a)

• If this PR changes .copilot-collections.yaml or .github/instructions/: (n/a)

• GARM E2E dispatched on this branch: https://github.com/canonical/github-runner-operators/actions/runs/39718034089.

[cbartz]

The design is sound: keeping a departed configurator's provider published until its last scaleset has drained is the right fix, and failing safe on an unknown scaleset state is the right default. One gap needs to be closed before merge.

To address before merge

1. Removing the last configurator bypasses the record. When no relation publishes provider data, restart() returns before _published_providers reads the record. The drain then only works while the old Pebble layer survives: if the pod is recreated mid-drain, pebble-ready re-runs restart(), which returns early again, so no provider is republished and the scaleset deadlocks as before. The record is also never trimmed, so the departed provider's credentials stay forever and are republished as soon as a new configurator relates. Suggest taking the early return only when there are no relations and the record is empty; otherwise publish the recorded set and trim as usual (with nothing related, the existing trim keeps the provider while its scaleset survives and drops it afterwards). Please add a unit test for this path.
2. Upgrade note. Please add a short note for operators (release notes or the upgrade docs) that deployments already stuck do not recover on upgrade, and are recovered by relating the same configurator again, as the description explains.

Suggestions

• Consider running the GARM E2E test from this branch before merge: the unit tests mock GARM, so they cannot show a real GARM purging the runners and deleting the scaleset once the provider is retained. Your call.
• Each change to the set writes a new revision of garm-providers, and old revisions (holding old passwords) are never removed. Consider pruning superseded revisions after set_content.
• The hard fail on an unreadable/unwritable record is fine as is — a visible error beats a silently stranded scaleset.

Nits

• get_content(refresh=True) on the charm's own garm-providers secret: the repo convention for owned secrets is plain get_content(), or peek_content() to read the latest revision without changing tracking (which also covers non-leader units).
• _PROVIDER_RECORD_FIELDS duplicates garm_entrypoint._PROVIDER_REQUIRED_FIELDS, which lives in the same src/ and could be imported, so the two can't drift.
• _referenced_providers returns None as soon as one surviving scaleset has no provider name; if that scaleset persists, trimming is disabled indefinitely with only a per-hook warning. Fine as the safe default, but worth an explicit note or a clearer log line that says trimming stays off until it's gone.
• ISD-7262 is referenced in code comments, the secret description and AGENTS.md. The tracker isn't public, so outside readers can't follow it; the surrounding text already explains the concern, so the id can be dropped from shipped text (it's fine in the PR title/commit).
• Several docstrings carry design rationale and the bug's history (ProviderRecordError, restart(), _read_provider_record, _trimmed_providers). Consider keeping docstrings to the interface and moving the rationale into one concise comment at the point of use (or the AGENTS.md entry, which already has it).
• _by_provider_name is a one-line wrapper around sorted; inlining it at its two call sites reads just as clearly.

AI-generated message.

[yanksyoon]

Thanks for the review — all points are addressed in d147d58 (on top of the merge from main).

1. Last configurator bypassing the record — fixed, with one extra condition

Agreed, this was a real gap: the early return skipped the record entirely, so a replaced pod re-published nothing, and the record was never trimmed.

The gate is now no related providers AND empty record. One addition to the suggestion: publishing and reconciling unconditionally would re-open the bug that test_unpopulated_configurator_relation_does_not_prune guards — a configurator that is related but has published nothing yet (secret-grant race, configurator mid-upgrade) yields no related providers, so the sweep would prune the scalesets it is about to describe. So with no related providers:

• record empty → unchanged early return;
• record non-empty, no configurator relation at all → publish the record, reconcile (every scaleset is legitimately orphaned), trim;
• record non-empty, configurator related but silent → publish the record, then wait without reconciling.

Two tests cover it: test_removing_the_last_configurator_keeps_the_drain_alive_across_a_pod (replaced pod still publishes and starts GARM with the retained provider) and test_record_is_emptied_once_its_last_scaleset_is_gone (the record empties, so a departed provider cannot come back when a new configurator relates).

2. Upgrade note — added to docs/changelog.md: already-stuck deployments do not recover on upgrade, and recovery is to relate the same configurator application again (its unit name reproduces the provider name) or clear the row by hand.

3. E2E — dispatched on this branch: https://github.com/canonical/github-runner-operators/actions/runs/39718034089. I will report the result.

4. Pruning superseded revisions — done, now that the single-writer assumption holds. The leader is the only writer and every reader uses peek_content(), so no unit depends on an older revision: after a write, _remove_superseded_revision() removes the revision the write replaced. It is best effort — a store that refuses keeps the revision, which costs space rather than correctness, and a refusal is logged at debug.

5–10. Nits — all applied:

• peek_content() instead of get_content(refresh=True), in the read and in the write comparison.
• PROVIDER_REQUIRED_FIELDS is now public in garm_entrypoint.py and imported by the charm; the duplicate is gone.
• the provider-less-scaleset warning now says trimming stays off until that scaleset is gone.
• the internal tracker id is dropped from shipped text (comments, secret description, AGENTS.md); it stays in the commit and PR title.
• docstrings trimmed to the interface; the rationale lives in AGENTS.md plus one-line comments where a refactor could undo it.
• _by_provider_name inlined.

Unit tests: 460 pass (with main's new tests merged in). ruff, codespell, the CCR001 gate and pyright are clean on the touched files. With the trim or the record read stubbed out, the lifecycle tests fail, so they pin the fix rather than the plumbing.