Skip to content

test(e2e): disable tenant-root monitoring after its bring-up check - #4479

Closed
IvanHunters wants to merge 1 commit into
mainfrom
test/e2e-disable-tenant-root-monitoring
Closed

IvanHunters wants to merge 1 commit into
mainfrom
test/e2e-disable-tenant-root-monitoring

Conversation

@IvanHunters

Copy link
Copy Markdown
Collaborator

What this PR does

The e2e install phase enables monitoring on the root tenant to verify the stack comes up (VictoriaMetrics components operational, Grafana reachable through the ingress) and then leaves it running for the whole chainsaw phase. On the three-node sandbox that stack holds roughly 3.7 vCPU and 9.9Gi of pod requests, about 15% of allocatable CPU and 14% of memory, competing for the scheduling headroom the kubernetes-* suites need to join worker VMs, a known contributor to node-join flakiness. Once the bring-up check has passed that coverage is already banked, so this turns monitoring back off. Flipping the flag prunes the tenant's monitoring HelmRelease; its uninstall runs the chart's post-delete cleanup Job, which removes the metrics and logs PVCs. The wait on the pruned HelmRelease is bounded and non-fatal, so a slow teardown never reddens an install on a PR that never touched monitoring.

Verified on a live cluster: enabling monitoring on a throwaway tenant and then flipping it off tore the whole stack down (two CNPG clusters, the VictoriaMetrics and VictoriaLogs clusters and their 14 PVCs) in about 40 seconds, the cleanup Job completing in about 20 seconds, with no orphaned PVCs left behind.

Downstream repositories

  • No downstream repository is affected by this change

Release note

test(e2e): disable the root-tenant monitoring stack after its bring-up check so it stops holding sandbox scheduling headroom for the rest of the chainsaw phase

…after its check

The install phase enables monitoring on tenant-root only to prove the stack
comes up, then leaves it running for the whole chainsaw phase. There it holds
about 3.7 vCPU and 9.9Gi of pod requests on the 3-node sandbox and competes for
the scheduling headroom the kubernetes-* suites need to join worker VMs, a known
contributor to node-join flakiness. Once the bring-up check has passed that
coverage is banked, so turn monitoring back off. The wait on the pruned
HelmRelease is bounded and non-fatal so a slow uninstall never reddens an
install on a PR that never touched monitoring.

Assisted-by: LLM
Signed-off-by: IvanHunters <[email protected]>
@coderabbitai

coderabbitai Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size/S This PR changes 10-29 lines, ignoring generated files area/testing Issues or PRs related to testing (e2e, bats, unit tests) labels Sep 25, 2026
IvanHunters added a commit that referenced this pull request Oct 5, 2026
## What this PR does

Monitoring OIDC gets live-cluster e2e coverage again. It had none since
the two pre-Chainsaw BATS suites were removed without a port. This adds
a Chainsaw suite in `hack/e2e-chainsaw/monitoring/` that ports what they
asserted, plus the in-place switch from an inline config to a
`secretRef` that nothing covered.

The suite takes one Monitoring release in `tenant-test` through three
modes in a row: System, CustomConfig with an inline config, and
CustomConfig with a `secretRef`. It creates that release itself and
never reads the tenant-root monitoring instance, so it keeps working if
#4023 or #4479 turns root monitoring off.

What each phase checks:

- System mode: cozystack-api admits `spec.oidc` and both HelmReleases go
Ready. The Grafana CR points `auth.generic_oauth` at the cozy realm, has
no `role_attribute_path`, and has `skip_org_role_sync` and
`oauth_allow_insecure_email_lookup` forced. The KeycloakClient is
confidential and references the generated Secret, which holds 32
alphanumeric characters. The audience scope is bound to this release's
clientId. There is no chart-owned `KeycloakRealmGroup`. The users-Job
carries the users and the mode, and `grafana-deployment` reads the
client secret from the Secret.
- Switch to inline: the Grafana CR takes the tenant's map, keeps the
forced keys and drops every System-only key. The users-Job reruns with
the new list. The KeycloakClient, the audience scope and the client
Secret are pruned, and the Deployment stops referencing that Secret.
- Switch to `secretRef`: the inline `auth.generic_oauth` block is gone
from the Grafana CR. The operator Secret is mounted at
`/etc/grafana/oidc` on both the CR and the Deployment. This checks the
mount only and does not prove Grafana loads the file, see #4679.
- After the install and after each switch, neither release may show a
remediation cycle. That means no `failed` or `uninstalled` snapshot, and
the latest revision must be exactly 1, then 2, then 3. The revision
check is there because helm-controller truncates history after the
previous deployed snapshot. An upgrade that failed, rolled back and then
passed on retry leaves no failed entry, only extra revisions.

I did not assert `GF_PATHS_CUSTOM_INI`. I could not find anything in
Grafana that reads it (#4679), and pinning it would make possibly dead
wiring part of the contract.

The coverage split with the render tests is this.
`packages/system/monitoring/tests/oidc_test.yaml` keeps the
fresh-install render of each mode, including the CustomConfig negatives
(no Keycloak objects, no client Secret). This suite covers what a render
cannot see: admission, the Deployment the Grafana operator builds, and
what a switch between modes prunes or leaves behind. The Keycloak
objects are checked as written, so creating the client in Keycloak is
not asserted. Removing it is, because the operator's finalizer holds the
KeycloakClient until the client is gone from Keycloak.

Before the first apply the suite also waits until the tenant's
`cozystack-values` carries `oidc-enabled: "true"`. The outer release
copies `_cluster` into the inner release once, so a Monitoring applied
right after the tenant is created can render from stale values and fail
the System-mode guard for the full 15m. That is what the first CI run on
this PR hit.

The Keycloak CRDs are required, not probed for. The e2e install always
enables platform OIDC, so a missing CRD is a broken install. The removed
suites skipped in that case, and their probe could not tell an absent
API group from a present one.

The selector picks the suite up by name from
`cozystack.monitoring-application` and `cozystack.monitoring`. A change
to `system/monitoring`, `extra/monitoring`, `monitoring-rd` or
`grafana-operator` now runs only this suite instead of the full run. The
tenant-root Grafana bring-up stays covered by the install BATS while it
exists. One selector unit test relied on `cozystack.monitoring` reaching
no suite, so it now renames that source in its copy of the graph.

The second commit keeps Keycloak changes on every suite that drives a
Keycloak client. Before this PR a change to `system/keycloak` or
`system/keycloak-operator` escalated to the full run. With the
monitoring suite it would select monitoring alone, because
`monitoring-application` is the only source that depends on
`keycloak-operator`, and `kubernetes-latest` with its OIDC lifecycle
would drop out. `src_to_suites()` now maps both to `kubernetes-latest
monitoring`. The mapping has to be explicit because
`kubernetes-application` has no `dependsOn` on `keycloak-operator`
(#4680). A `postgres-operator` change now also reaches
`kubernetes-latest`, through Keycloak's database. The selector comment
and the e2e doc no longer use `system/postgres-operator` as the example
of a path with a source that reaches no suite.

Cost: one monitoring stack bring-up, two in-place upgrades and teardown.
I expect 10 to 20 minutes on every full run. The selector change makes
the CI run on this PR a full run, so the suite's own timing is in the
chainsaw report. A red run can spend up to 16m on each of four waits,
since each one sits a minute above the 15m timeout of the release it
waits on.

The suite cannot run locally, so I checked it this way:

- `chainsaw lint test` at v0.2.15, the sandbox version, passes.
- I ran the suite's own steps against a local envtest apiserver.
Chainsaw applies the real manifests, and a stand-in for helm and the
operators renders the chart from the stored Monitoring spec. With the
chart intact all nine steps pass. Each of 30 single breakages of the
chart or of the simulated controllers turns the expected step red.
- `hack/select-e2e_test.bats` and `hack/select-install_test.bats` pass.
The new Keycloak selection case fails without the mapping and when
`kubernetes-latest` is dropped from it.

A browser login through Keycloak is out of scope, same as in the removed
suites.

Left for later:

- KeycloakClient and scope `status` after the operator creates them in
Keycloak is not checked.
- `hack/e2e-chainsaw/README.md` lists suites by hand. I added
`monitoring`, but `backup`, `kafka-metadata` and `kubernetes-proxmox`
were already missing.
- The `operator diff selects all dependent app suites` selector test
could also check `monitoring`.
- The comment on the Keycloak mapping says both suites wait for the
finalizer to remove the client. The monitoring suite does wait.
`kubernetes-latest` only reads once after its upgrade.
- The ready checks compare `observedGeneration` with
`metadata.generation` through `contains()`. Both are fields of the same
object, not a number literal, so the v0.2.15 literal gotcha should not
apply, but no live run has passed this suite yet.
- The inline fixture carries no `enabled: "true"`, so that phase checks
a disabled OAuth block. The asserts do not depend on it.

Closes #4051

Closes #3785

### Screenshots

Not a UI change.

### Downstream repositories

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:

### Release note

```release-note
NONE
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **New Features**
* Added end-to-end coverage for Monitoring installations using system
OIDC, inline custom configuration, and Secret-based configuration. The
tests verify readiness, rendered resources, configuration cleanup, and
upgrade history.
* Updated test selection so Keycloak-related changes also run the
Monitoring suite.
* **Documentation**
* Added Monitoring to the documented Chainsaw suite list and clarified
how changed paths determine test coverage.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@lexfrei

Copy link
Copy Markdown
Contributor

IvanHunters Since #4023 the e2e install no longer enables monitoring in tenant-root, so there is nothing left to turn off. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/S This PR changes 10-29 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants