mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-08 21:14:56 +09:00
02a3fda471ea756bbfd0d79d015665ebae669bc2
3305
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
92ec49e64e |
fix(tenant): retry failed monitoring and gateway installs in place (#3633)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The tenant chart renders monitoring and gateway with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither sets `spec.uninstall.disableHooks`, which defaults to false. So a release that missed nothing but a readiness budget gets uninstalled, and the app chart's `post-delete` cleanup Job runs against it. Those Jobs delete on purpose, and they are right to. The monitoring one removes PVCs labelled `apps.cozystack.io/application.name=<release>-system`. That label is not incidental either: the metrics and logs claim templates stamp it so the operator copies it onto the storage PVCs, and the Job selects on it to stay release-scoped. Both ends were built to meet. The result is that a slow first install deletes the tenant's metrics and logs, and nothing reconstructs them. The gateway Job deletes the `cozystack-acme-account` Secret. That key regenerates, so this one costs no data. It costs a re-registration against the ACME provider, and repeated cycles run into its rate limits, which can hold certificate issuance for the tenant down for hours. Smaller stake, same mechanism, and the two are not one class: only one of them owns storage. `RetryOnFailure` keeps the manifests applied and retries the failed release, so no uninstall happens and neither Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. Two things are traded away. A real upgrade failure now retries instead of rolling back, which gives up no stable safety net, since with `retries: -1` the default strategy was already rolling back and re-upgrading without end. And the uninstall was the only automatic clean slate, so an install wedged on a bad resource state retries against that state until an operator removes the release. For a release that owns data that is the right exchange. This removes the consequence rather than the trigger, and for monitoring the trigger is worth naming separately: the parent HelmRelease here waits `10m`, while the child `monitoring-system` release sets `15m` deliberately, with a comment explaining that 10m is too short for the OIDC users-Job inside its `activeDeadlineSeconds: 900`. The parent's wait covers the child, so the parent can fail while the child is behaving exactly as designed. That mismatch is untouched here and wants its own change. The chart's etcd and seaweedfs releases lose storage the same way and are outside this change. Beyond those four, computeplane, ingress and info stay on the default strategy, but their charts ship no destructive `post-delete` hook, so remediation costs them a wasteful teardown rather than data. Same shape as #3552 and #3581. Relates to #3624. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tenant): retry a failed monitoring or gateway install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the tenant's metrics and logs volumes or its ACME account key ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Monitoring and gateway deployments now automatically retry failed installs and upgrades every 30 seconds. * Existing unlimited remediation retries remain available, improving recovery from deployment issues. * **Validation** * Added automated coverage to verify retry behavior for both monitoring and gateway deployments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
36f83fa9b7 |
fix(tenant): retry failed etcd and seaweedfs installs in place (#3630)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The tenant chart renders the etcd and seaweedfs releases with no install strategy, so they get Flux's default, and install remediation is an uninstall in every case Flux offers. Neither release sets `spec.uninstall.disableHooks`, which defaults to false, so that uninstall runs the app chart's `post-delete` cleanup Job. Nothing unusual is configured anywhere along that chain: fail early, uninstall, hooks. Those Jobs delete data on purpose. The etcd one removes the `data-etcd-<ordinal>` PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, because nothing else would. Both are right for a real uninstall, and neither hook is at fault here. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. That budget is not a comfortable margin. The seaweedfs release has `timeout: 10m`, and `hack/e2e-install-cozystack.bats` already records that on a loaded runner the tenant stack only starts creating pods around 9 to 10 minutes in. The window CI observes and the window that triggers this are the same window. `RetryOnFailure` keeps the manifests applied and retries the failed release as an upgrade. No uninstall happens, so neither cleanup Job runs and the install already under way carries on instead of starting over. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. One trade is that a real upgrade failure retries instead of rolling back. That costs no stable safety net. With `retries: -1` and the default strategy, upgrade remediation was already rolling back and retrying without end, so the release never rested in the rolled-back state either. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data that is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The monitoring release carries the same default and a cleanup Job that deletes its PVCs as well, and is left for its own change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Same shape as #3552 and #3581. Relates to #3555 and #3576. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tenant): retry a failed etcd or seaweedfs install instead of uninstalling the release, so a slow first install no longer runs the cleanup hooks that delete the etcd data volumes, the SeaweedFS volumes and the filer metadata database ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Improved installation and upgrade recovery for etcd and SeaweedFS by automatically retrying failed operations every 30 seconds. * Preserved ongoing remediation attempts to help services recover without uninstalling or rolling back after failures. * **Tests** * Added coverage to verify retry behavior and remediation settings for both services. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
79ceaf1dc2 |
fix(harbor): retry the nested system install instead of uninstalling it (#3621)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Harbor's app chart renders a nested HelmRelease that owns the whole workload: the Deployments, the inline RedisFailover and its volume. It had no install strategy, so it got Flux's default, and install remediation is an uninstall in every case Flux offers. A first install that misses its budget gets torn down, and the next one provisions the redis volume and runs the master election again from scratch. That happens on a loaded runner. harbor-core waits on redis, redis waits on its volume and the master election, and the nginx Deployment hits the default `progressDeadlineSeconds` of 600s. Flux reads that as a stalled resource and fails the install early. The e2e wait budget is 10m, the same as the deadline, so after a teardown the second attempt has less time than a first install needs. `RetryOnFailure` keeps the manifests applied and retries as an upgrade. Nothing is deleted, so the volume, the elected master and harbor-core's startup all survive, and the retry costs 30s instead of a full install. A Deployment clears `ProgressDeadlineExceeded` by itself once its pods go ready, so the retry lands as soon as the stack converges. This drops the teardown, not the deadline. The suite applies the release in one step and waits for it in later ones, so part of the budget is still left when the fail-early hits. That leftover is useless to a fresh install and usable to a retry that resumes. Whether the deadline or the budget also needs moving is a separate question, and an early deadline only becomes safe once a failed install stops being torn down. In production the win holds without any of that: a slow first install no longer deletes the Redis volume and the database. Both actions carry the strategy. The controller runs the retry of a failed install as an upgrade, so with install alone the retry's own failure falls through to upgrade remediation. The remediation blocks stay: an absent release consults install, an out-of-sync one consults upgrade, both by retry count rather than by strategy. It goes on the nested release, not the parent. The parent is built from the Application by the apiserver, which already stamps this strategy on both actions. The nested one owns the workload and the volume a remediation deletes. Same shape as the tenant CNI and CSI releases in #3552 and #3581, and the unit test follows theirs. Relates to #3445. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(harbor): retry a failed Harbor install instead of uninstalling the release, so a slow first install no longer deletes the Redis volume and starts over ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Harbor installations and upgrades now automatically retry every 30 seconds when failures occur. * Existing unlimited remediation retries remain available for both operations. * **Tests** * Added automated coverage to verify Harbor retry behavior and remediation settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d956536461 |
fix(tenant): render the chart without the platform's injected values (#3636)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
`helm lint packages/apps/tenant` fails on main with three errors. This
removes two of the three; the branch name notwithstanding, lint still
exits 1 afterwards on the third, which is a repository-wide convention
question rather than a defect in this chart.
**The chart cannot be rendered outside a cluster at all.**
`templates/keycloakgroups.yaml` reads `index .Values._cluster
"oidc-enabled"`, and `_cluster` is injected by the platform, so it is
absent whenever the chart is rendered on its own. `index` on a nil map
is a hard error, not an empty result, so both `helm lint` and a plain
`helm template` die on line 1.
The idiom the neighbouring charts use for the same lookup does not
rescue this on its own, which is worth stating because it looks exactly
like a guard. `(index .Values._cluster "x") | default "y"` still fails:
`index` is evaluated first and errors on the nil map, so `default` never
receives a result to substitute; it only ever covered a key missing from
a map that exists.
The spelling alone does not decide whether a chart breaks, though.
`packages/system/cozystack-basics` uses it and lints clean, because line
1 of its `values.yaml` is `_cluster: {}`, which makes the map non-nil
before `index` runs. A chart is broken only when it neither declares the
empty map nor guards at the use site, and a lint sweep across the
catalog found 27 in that state. Guarding at the use site is the choice
here because it makes the template correct whatever `values.yaml`
contains, and because `_cluster` is platform-internal while this chart's
`values.yaml`, schema and README are generated together from
`api/apps/v1alpha1/tenant/types.go` as user-facing output. The guard has
to sit on the map rather than on the value it returns:
```
{{- $oidcEnabled := index (.Values._cluster | default dict) "oidc-enabled" }}
```
**This is behaviour-preserving, and that is checked rather than
asserted.** With `_cluster` present, `| default dict` returns it
untouched and the lookup is what it was; with it absent the lookup is
nil, the `eq` below is false, and the block stays unrendered, which is
what an unconfigured tenant should produce. Rendering before and after
gives byte-identical output:
| values | before | after |
| --- | --- | --- |
| `_cluster.oidc-enabled: "true"` | 4 KeycloakRealmGroups |
byte-identical |
| `_cluster.oidc-enabled: "false"` | no groups | byte-identical |
| no values at all | hard error | renders, no groups |
Worth recording how that comparison has to be run, because the obvious
way proves nothing. The KeycloakRealmGroup block sits behind
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, which is false
under a plain `helm template`, so a first pass rendered the same output
in *both* the enabled and disabled cases, and a diff of those two would
have compared one empty result against another. The renders above pass
`--api-versions v1.edp.epam.com/v1` so the guarded branch actually
executes. Anyone proving neutrality on a capability-gated template needs
that flag or the proof is vacuous.
**`Chart.yaml` does not declare `cozy-lib`**, which is present as a
symlink under `charts/`, so lint reports missing chart metadata. It is
declared the way `packages/apps/qdrant` does it, that being the one
chart in the tree which declares it and does not carry this error.
Rendering is unchanged by the declaration, checked the same way as
above.
Two things that could plausibly have broken and did not, checked rather
than assumed: rendering from a packaged `helm package` artifact is
byte-identical, and the `charts/cozy-lib` symlink is still expanded into
the tgz with the dependency declared. And neither `hack/update-crd.sh`
nor the `fix-charts` target in `packages/apps/Makefile` reads the
`dependencies:` block, so nothing needs regenerating.
**The icon error is left in place on purpose, and the relative path is
load-bearing.** `helm lint` wants a URL with a scheme, and
`/logos/tenant.svg` has none. But `hack/update-crd.sh` consumes exactly
this form: it rewrites a leading slash to `./`, then reads that file off
disk and base64-encodes it into `.spec.dashboard.icon` of the generated
ApplicationDefinition. `packages/apps/tenant/logos/tenant.svg` is that
file. Give the chart a URL with a scheme and the generator's `[[ ! -f
"$ICON_PATH" ]]` check no longer finds anything to encode. So the
relative form is a requirement of the pipeline rather than a stylistic
convention, all 31 icon-carrying charts use it, and satisfying helm lint
here means changing the generator, not the charts. That is a
repository-wide decision and not this PR's.
`tests/keycloakgroups_oidc_test.yaml` pins the gate in all three
directions: four realm groups when `oidc-enabled` is `"true"`, none when
it is `"false"`, and none when the platform injected no `_cluster` at
all. The last case is the one the guard exists for, and it needs no
mocking, since `_cluster` is absent from `values.yaml` and "not
injected" is just the chart's default state. Against the unguarded
template that suite fails with `index of untyped nil`. The chart's other
seven suites pass identically with and without the fix, which is
precisely why the change needed its own.
Two limits of the new suite, stated rather than left to be found. It
pins how many documents render, not which: renaming the four groups or
breaking a `realmRef` would pass. And the template's second gate,
`.Capabilities.APIVersions.Has "v1.edp.epam.com/v1"`, is enabled for the
whole suite, so no case covers its absence. Both are worth closing on
the next touch of this file; neither weakens what the suite does pin,
which is that the chart renders at all when the platform injected
nothing.
A trap for anyone reproducing this by hand: the gate compares against
the string `"true"`, so `--set _cluster.oidc-enabled=true` passes a YAML
boolean and renders nothing, while `--set-string` renders the four
groups. The platform writes a genuine string, and it survives the
round-trip through a Secret because `toYaml` quotes it. Note also that
this chart's helpers reject a release name that does not begin with
`tenant-`, which will silently render nothing for either form.
One note in case a future change teaches the linter to inject the
platform's values, which would hide this class of error without fixing
it: the guard is still worth having, because it is what lets `helm
template packages/apps/tenant` succeed with no values file at all. A
chart that renders standalone can be inspected, diffed and unit-tested
without reproducing the platform around it.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [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:
### Release note
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(tenant): the tenant chart now renders without the values the platform injects, so `helm template` works on it standalone, and its `cozy-lib` dependency is declared in Chart.yaml; this clears two of the three `helm lint` errors it reported, the remaining one being the repository-wide relative icon URL
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- Added support for using the shared Cozy library in tenant deployments.
- **Bug Fixes**
- Improved tenant configuration rendering when cluster settings are
unavailable.
- OIDC-related Keycloak groups now render reliably when OIDC is enabled
and required platform capabilities are present.
- Preserved expected behavior when OIDC is disabled or configuration is
omitted.
- **Tests**
- Added coverage for enabled, disabled, and missing cluster
configuration scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
c1071dab9b |
feat(kubernetes): let a node group log the guest serial console (#3637)
## What this PR does Both tenant Kubernetes e2e suites keep missing the node-Ready budget with zero Nodes registered, and the time goes inside the guest, between the worker VMI reaching Running and Talos asking for its apid certificate. Nothing collected today reaches that window. The in-guest capture the harness already has drives `talosctl`, so it needs the apid the worker never got to, and the management side has neither a Node nor a certificate request to read. Four attempts at this failure have been designed from outside the guest; the most recent raised the budget from 12m to 18m and the same fingerprint came back at the new deadline. KubeVirt can stream a VM's guest serial console into a `guest-console-log` container beside virt-launcher, readable with `kubectl logs` and needing nothing from inside the guest — no talosctl, no client certificate, no helper Pod, no reachable apid. That is the one property this failure class demands, so this turns it on for the suite's worker node group and collects it into the diagnostic bundle when node-join fails, ahead of the `talosctl` capture that cannot describe the same runs. Reads start at the beginning of the stream rather than tailing it: a guest that repaints its console would push the boot output out of any `--tail` window, while `--limit-bytes` truncates from the far end instead. Every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become an unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it — kubectl's own `container guest-console-log is not valid for pod` would otherwise fill the file, and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. The switch is a new opt-in `logSerialConsole` on a node group, off by default. It renders nothing unless set to true, including when set to false, because the `KubevirtMachineTemplate` is content-hash named and a byte emitted for a group that did not ask for it renames the template and rolls every live worker VM on upgrade. I verified that by rendering the base tree and this branch: absent and explicit-false produce a byte-identical template name on both charts. Worker VMs come from two charts, `kubernetes` and `kubernetes-nodes`; both carry the field, and the golden-parity check between them gains a case that keeps their output identical. The passing path checks separately that the container actually attached, and runs that check last, after everything the suite exists to prove — a debugging aid should not preempt the assertions that cover the cluster. Without it the suite would first learn the setting was inert in the one run that needed the console, when it can no longer be recovered. Relates to #3513. ### The risk this carries, stated up front The platform turns serial console logging off cluster-wide in the KubeVirt CR. That setting is a mitigation added in February 2026 for kubevirt/kubevirt#15989, where the `guest-console-log` container kept virt-launcher from starting and left VMs in `PodInitializing`. A per-VM `logSerialConsole` takes precedence over it — `isSerialConsoleLogEnabled` reads the VMI field first and only then falls back to the cluster setting — which is what makes this work, and also makes this the first thing on the platform to go through that mitigation. The mitigation was written against KubeVirt v1.6.x; the platform now ships v1.8.4, and the upstream issue is still open with no reproduction reported either way on the newer version. So the first run of these suites is also the experiment, and it doubles as a revalidation of that February mitigation on the version actually shipping, which nobody has done. The two outcomes are distinguishable rather than ambiguous: kubevirt/kubevirt#15989 shows up as virt-launcher wedged in `PodInitializing` with the VMI never reaching Running, while the failure this instruments shows the VMI Running and zero certificate requests. A red run with the first signature is an answer, not noise — and the collector is built so that it says which one happened instead of reporting silence. The blast radius is small: the product field is absent by default and only the e2e node group sets it, so backing this out is one line. The revert criterion, stated before the run rather than after it. If worker virt-launcher Pods sit in `Init` on `guest-console-log` and no Node registers, kubevirt/kubevirt#15989 still reproduces on v1.8.4: drop the `logSerialConsole: true` line from the e2e node group, keep the chart field, and the upstream issue gains the reproduction on a current version that its thread does not have. If the Pods reach Running and the console logs carry boot output, the mitigation is a candidate for removal platform-wide, which is its own change. Either way the run answers something that was open before it. Dropping `virtualMachineOptions.disableSerialConsoleLog` outright was the alternative. It is a smaller diff and needs no values key, but it exposes every VM on the platform to a race nobody has re-tested, instead of one node group in one test. That is its own revalidation exercise and belongs in its own change, on evidence this one produces. ### Verification `make unit-tests` passes. The new bats suite runs green under both `bash` and a `dash` `/bin/sh`, which matters because `hack/cozytest.sh` is `#!/bin/sh` and an earlier revision of this branch carried a bash-only construct that failed silently there. Every test in it is killed by a specific mutation of the code it covers. Both charts' helm-unittest suites pass, the golden-parity script reports byte-identical output across all four cases including the new one, and `make generate` is idempotent at the repo root. Reviewed by Claude across several rounds and by an independent second reader; the review loop found and fixed real defects in this branch, including a green-path check that had become unable to report the one thing it existed for. ### Screenshots Not a UI change. ### Downstream repositories - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/26 - [ ] [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: ### Release note ```release-note feat(kubernetes): worker node groups accept `logSerialConsole`, which streams a worker VM's guest serial console into a `guest-console-log` container readable with `kubectl logs`. Off by default. It is the only way to observe a worker that stalls during boot before Talos apid answers, when no Node and no certificate request exist to diagnose from. Enabling it rolls the node group and overrides the platform's cluster-wide `disableSerialConsoleLog`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an optional `logSerialConsole` setting for Kubernetes worker node groups. * Streams worker VM guest serial-console output to a dedicated log container when enabled. * Configuration is disabled by default and available through Kubernetes application settings. * **Bug Fixes** * Improved worker failure diagnostics with bounded serial-console output and pod-state information. * Validates that enabled worker pools attach the expected console-log container. * **Documentation** * Documented configuration behavior, rollout effects, defaults, and timeout considerations. * **Tests** * Added coverage for rendering, attachment validation, capture limits, and failure scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
93ffb38a9b |
fix(cozy-lib): make the cozyconfig helpers survive uninjected values (#3647)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
`_cluster` and `_namespace` reach a chart through valuesFrom on the
cozystack-values Secret, so they are absent whenever a chart is rendered
on its own: `helm lint`, a plain `helm template`, and any helm-unittest
suite that does not set them. Twelve of the helpers in `_cozyconfig.tpl`
read straight through the missing map and abort the render instead of
returning the empty default they document.
Two spellings, one cause. `index .Values._cluster "root-host"` raises
`index of untyped nil`; `.Values._namespace.host` raises `nil pointer
evaluating interface {}.host`. In both, the trailing `| default ""`
never runs, because it applies to the result and there is no result,
only an error. That default covered a key missing from a map that
exists, never a missing map.
The guard goes on the map, which is what `cozy-lib.images-registry` in
this same file already did:
```
(index (.Values._cluster | default dict) "root-host") | default ""
```
`index` on an empty map returns nil harmlessly, so the trailing default
applies as it always read, and with the map present `| default dict`
returns it untouched.
**This fixes no chart that is failing today.** The charts that cannot
render standalone read the injected values directly in their own
templates rather than through these helpers, so a change to the library
does not reach them; they are handled where they live. Worth being blunt
about the same point from the other side: twelve of the thirteen helpers
here currently have no callers anywhere in `packages/`. Only
`cozy-lib.images-registry` is reached, from `cozy-lib.image`, and it was
already guarded. The guard is still worth having on the rest, because
they are the library's public surface, they are what a chart author
copies when reaching for cluster configuration, and a broken spelling
propagates by being read. `cozy-lib.image` is called from four
templates, so these helpers do get adopted once they exist.
**Rendered output does not change, and that is measured rather than
argued.** All 34 charts that symlink cozy-lib were rendered before and
after, both with the injected values populated and with them set to
empty maps, and every result is byte-identical. That check needs a
control: the first comparison showed eight charts differing, which looks
like a regression in a library change. Rendering the *unchanged* tree
twice reproduces exactly those eight, because they generate passwords.
Subtracting the control leaves zero real differences.
The suite lives in `cozy-lib-tests`, which is where library behaviour is
pinned, since a library chart renders nothing of its own. One case
covers the uninjected state, which needs no mocking because neither key
appears in any values file, and one covers the injected state so the
guard cannot quietly change what a populated map returns. The injected
case pins `scheduling` and `branding` as whole YAML documents rather
than by substring: those two had their bodies restructured rather than
wrapped, and a substring assertion survives the loss of the `toYaml`
that produces the document, because the key name also appears in Go's
default map formatting. Against the unguarded helpers the first case
fails with `index of untyped nil`.
Three things left deliberately undone, so they are not mistaken for
oversights. The header comments in `_cozyconfig.tpl` say where these
values come from but not what the helpers now do when they are absent,
which is the contract this change creates and would be better stated
next to the helpers than only in the suite. `Chart.yaml` in
cozy-lib-tests still carries `name: quota` from when that chart covered
one helper family, so the runner prints `### Chart [ quota ]` for a
suite that now covers three. And the mixed state, one map injected and
the other not, has no case of its own; the two guards are independent so
composition covers it, and a third case would pin nothing new.
Relates to #3646.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [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:
### Release note
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
fix(cozy-lib): the cozyconfig helpers now return their documented empty defaults when the platform has not injected `_cluster` / `_namespace`, instead of aborting the render, so a chart using them can be rendered and unit-tested on its own
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Improved configuration handling when cluster or namespace settings are
missing, preventing template rendering errors.
- Preserved existing configuration values for scheduling, branding,
networking, registries, and component namespaces.
- **Tests**
- Added coverage for configurations with missing platform values.
- Added validation that injected cluster and namespace settings render
correctly.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
7b36a5d2f2 |
test(tenant): give these template lists their real reason (#3651)
## What this PR does Two helm-unittest suites in `packages/apps/tenant/tests/` said they restrict rendering because `namespace.yaml`'s `lookup` errors under helm-unittest. It does not. The lookup returns empty offline and the template renders, which `namespace.yaml`'s own header states and which `tests/namespace_ancestor_labels_test.yaml` depends on. I measured what each list actually does by removing it, and the two suites turned out to be different cases rather than two copies of one. In `cleanup_role_watch_test.yaml` the list is load-bearing, for a template the comment never mentioned. That suite sets no values, and `keycloakgroups.yaml` indexes `.Values._cluster` on its first line, so the whole-chart render fails with `index of untyped nil` and all 8 tests error. The comment now says that, and says it conditionally: a guard on that index is already in flight, and once it lands the list stops being load-bearing. I checked that too, by applying the guard and re-running with the list removed, which passes 8/8. In `vmo_webhook_dependson_test.yaml` the list is not load-bearing at all. That suite sets `_cluster`, the chart renders without the list, and all 5 tests pass. It stays only to scope the suite, and the comment says so instead of inventing a technical reason. Each comment now states what holds for its own file and nothing wider. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [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: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified test-rendering rationale and suite-scoping comments. * Documented that rendering requires `_cluster` values and that template lists control test scope. * **Tests** * Updated explanatory comments only; test behavior and configuration remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4740daa7be |
test(tenant, harbor, seaweedfs, api): anchor guards that pass on a missing document (#3639)
## What this PR does Five helm-unittest suites asserted things that stayed green when the thing they guard disappeared. No chart or template logic changes here — only the tests, and each fix is measured in both directions. **tenant `no_upgrade_force_test.yaml`.** Every test asserted `notExists: spec.upgrade.force` and nothing else. A `notExists` passes on a template that renders no document at all, so renaming the value that gates one of these releases leaves its test green while the release it guards is gone. Measured on `etcd.yaml`: with the gate renamed, the suite reported 6 passed and a sibling suite's `equal` was the only thing in the chart that noticed. Each test now pins the HelmRelease name alongside the absence. The same file also covers `computeplane.yaml` now. Seven templates in this chart render a HelmRelease and the list held six; the module landed after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression. That is the gap that produced this suite in the first place, when the first sweep missed `gateway.yaml`. Two rationales in that header were wrong and are corrected. The template list was said to be there because `namespace.yaml`'s `lookup` errors under helm-unittest; it does not, it returns empty offline and the template renders, which is what `namespace.yaml`'s own header says and what `namespace_ancestor_labels_test.yaml` relies on. Removing the whole list leaves the chart's suites passing, so the list stays for the reason that actually applies. And helm-controller does not deprecate `upgrade.force`: the CRD vendored at `internal/fluxinstall/manifests/fluxcd.yaml`, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy with no deprecation marker. `recreate` is the neighbouring field that carries one. **harbor, both suites.** Both addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object. Measured by inserting one document at the head of each template: the cleanup suite reddens outright, while two of the four jobservice tests keep passing: the two `notExists` ones, satisfied against an object that never had the path. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails loudly, including on the `notExists` tests. Indices are also easy to get wrong by hand: helm-unittest numbers documents in template source order while `helm template` prints them sorted by kind, so the rendered output is not the order being counted. **seaweedfs and cozystack-api.** Both forbade a rule by naming it in full, and helm-unittest compares every field it is handed by value equality, so one respelling walks past. For seaweedfs that is not theoretical: ungating the cnpg block and adding a verb to the rule is the same grant and more, and the guard stayed green while a Client instance held delete on another app's Postgres cluster. The Role is now bounded by shape. A Client renders exactly two rules, both in the core group, and the cnpg grant is gated between them, so ungating it adds a third whatever its spelling. For cozystack-api the count bound sits beside the existing named-rule assert rather than replacing it. Neither covers the other: `templates/rbac.yaml` carries no template directives, so the list is pinned at exactly sixteen rules from both sides, while a grant swapped in for an existing one leaves the count where it was and only the named-rule assert sees it. Measured on all three: append, delete, and swap. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches five files, all under `packages/*/*/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [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: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Tests** - Improved validation of rendered Kubernetes resources across Harbor, tenant, SeaweedFS, and system API configurations. - Tests now identify resources by kind and relevant properties, making checks more reliable when rendered document ordering changes. - Expanded coverage for tenant upgrade settings, compute plane resources, storage class propagation, cleanup permissions, and network policy access. - Added stricter assertions for expected resource counts, names, rules, and omitted settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
50c5e2c14c |
test(e2e): capture the tenant worker guest serial console
The tenant Kubernetes suites fail the node-Ready budget with zero Nodes registered, and the time is lost inside the guest between the worker VMI reaching Running and Talos requesting its apid certificate. Nothing collected today covers that window: the existing in-guest capture drives talosctl, so it needs the very apid the worker never reached, and on the management side there is neither a Node nor a certificate request to read. Turn on the guest console log for the suite's node group and collect each virt-launcher's guest-console-log container from the management cluster. Reads start at the beginning of the stream rather than tailing it, so a guest that repaints its console cannot push the boot output out of the window; every read is bounded, and the walk stops at a cap that records both counts, so the collector cannot become the unbounded term in a failure path that already runs several others under one deadline. A read that returns no console is not filed as a quiet guest. Three outcomes get three labels, because only one of them says anything about the guest: the read failed and returned nothing, the read succeeded and returned nothing, or it broke off part way. Each read's stderr is kept beside the capture rather than inside it, since kubectl's own "container is not valid for pod" would otherwise fill the file and a console that never started would read as one that had spoken. That case is the headline one here, so the Pod state and events that explain it are collected alongside, once for the whole selector. Enabling the console overrides a cluster-wide setting the platform holds for an open upstream defect, so the run is also an experiment, and its own failure mode is the one that hides best: the container wedges virt-launcher in Init, no worker boots, and the suite reports "fewer than 2 tenant nodes Ready" -- the same sentence the failure under study produces. Read as the known signature, the experiment's answer would be filed as the flake it was meant to explain. So the failure path names that case first, before any other diagnostic, and only when it is true. The passing path checks separately that the container attached at all, and reports the ratio it saw rather than a bare success, since the check is deliberately satisfied by any one Pod. It keeps three outcomes apart too: a read that did not answer, a selector that matched no Pod, and Pods that matched and carry nothing. It reads the Pod name alongside the container names so the last two cannot collapse, and both container lists so a future move of the container cannot fail a run over a diagnostic. It runs last, after everything the suite exists to prove, because a debugging aid must not preempt the assertions that cover the cluster. The capture runs before the talosctl one because it is the only of the two that can describe a worker which never reached apid, the shape this failure keeps taking. kubectl reports a warning and an empty result alike, on stderr with a zero status, so the exit status decides which artifact its output lands in. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1cb51c92aa |
test(tenant): give these template lists their real reason
Both suites here said they restrict rendering because namespace.yaml's `lookup` errors under helm-unittest. It does not. The lookup returns empty offline and the template renders, which namespace.yaml's own header states and which tests/namespace_ancestor_labels_test.yaml depends on. Measured what each list actually does by removing it. In cleanup_role_watch_test.yaml the list is load-bearing, for a template the comment never mentioned: the suite sets no values and keycloakgroups.yaml indexes `.Values._cluster` on its first line, so the whole-chart render fails with "index of untyped nil". Stated conditionally, because a guard on that index is already in flight and would end it. In vmo_webhook_dependson_test.yaml the list is not load-bearing at all, because that suite sets `_cluster` and the chart renders without it. It stays only to scope the suite. Each comment now states what holds for its own file and nothing wider. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
7dca642894 |
fix(ci): restore e2e for fork PRs and close the merge-gate bypass (#3262)
## What this PR does Fixes #3257 — fork PRs never ran e2e, and the skipped required check let them merge anyway. Fork PRs get no secrets on `pull_request`, so `make image` pushed anonymously and died with `denied`. That failed every build, collapsed the `build → finalize → e2e` chain, and left the required **E2E Tests** job *skipped* — which branch protection treats as satisfied, so an approved fork PR could merge with e2e never having run. This implements **Option A** from the issue (privileged `workflow_run` split), pushing fork e2e images to the existing CI registry (OCIR): **Build primitive (`hack/common-envs.mk`)** - New `OCI_EXPORT_DIR` mode: build each image to a per-image OCI archive instead of pushing, and force `PUSH`/`LOAD` off. The digest is captured via `--metadata-file` regardless of output type, so the refs baked into `pr.patch` match what the privileged run later pushes. Threaded through the `image-tags` macro every package already uses — no per-package Makefile change. **`pull-requests.yaml` (unprivileged, fork branch)** - `build` / `build-talos` / `finalize` export images to OCI archives and upload them as artifacts instead of pushing → the red `denied` build wall disappears and the chain no longer collapses. - The in-tree e2e job is renamed off `"E2E Tests"` and guarded to same-repo PRs. A new `e2e-report` job concludes the required `"E2E Tests"` context as an **explicit check-run** (success on a docs-only PR or when e2e passed, failure otherwise). A skipped job can no longer satisfy the required check — this also closes the same hole for same-repo PRs whose build fails. **`e2e-fork.yaml` (new, privileged `workflow_run` from the default branch)** - Resolves the fork PR, opens a pending `"E2E Tests"` check-run, and fails-closed if the fork build failed. - Pushes the pre-built image archives to OCIR **by digest** and flux-pushes the digest-pinned packages tree. The OCIR token is live only for trusted binaries (`git`/`skopeo`/`flux`/`yq`) over data — never for `make` or any fork-authored script. - Runs the e2e suite against the pushed closure with **no push token** and concludes the check-run on the PR head SHA. Fork build code never receives registry credentials; the fork's images and test scripts run only in the credential-less, ephemeral e2e job — a stronger posture than a `pull_request_target` run that would hand fork code the secrets directly. ### Validation Local: buildx OCI-archive output + digest capture, macro expansion for single- and multi-image packages, non-fork push path unchanged, `actionlint` clean. Needs live validation (why this is a **draft**): `workflow_run` artifact download across runs; that the `"E2E Tests"` commit status satisfies the required context and keeps a fork PR non-mergeable until the privileged run concludes it; `refs/pull/<N>/merge` fetch + `git apply --3way`; `skopeo --preserve-digests` end-to-end. `workflow_run` only runs from the default branch, so the privileged half is exercised by mirroring the workflow onto a personal fork's default branch and opening a throwaway fork PR into it — not by merging first. _Since review_: the fork `e2e` job ran the pre-#2826 BATS app loop that the merged Chainsaw migration deleted (already broken); it now runs the same Chainsaw + TIA path as the in-tree job, with suite selection and the `full-e2e` override sourced from `resolve`'s trusted base-repo file list / labels (not a merge-ref diff). Publish job hardened per the non-blocking follow-ups: base-tree image allowlist, `skopeo copy --all`, pinned flux CLI. The required `E2E Tests` gate is now published as a commit **status** (default token + `statuses:write`), not a COZYSTACK_CI app check-run — the app lacked `checks:write` (`checks.create` 403'd) and a default-token check-run floated under a labeler suite; a commit status has neither problem, and forks can't post one (their `pull_request` token is read-only). Two review-bot findings fixed too: fork `OCI_EXPORT_DIR` is now absolute (a relative path misplaced archives under `make -C <pkg>` and every fork build would fail at upload), and the "≥1 archive" guard no longer no-ops under `nullglob`. ### Repo-admin prerequisites One repository setting lives outside this diff: 1. **Branch protection: keep `E2E Tests` required.** The gate is now a commit **status** named `E2E Tests` (posted with the default `GITHUB_TOKEN` — see `docs/agents/e2e-testing.md` §10), not a job or an app check-run. Branch protection already requires `E2E Tests` (currently the e2e job's name, renamed here to `E2E (in-tree)`), so the required context now resolves to the status — **no GitHub App, no `checks:write` grant, no app-id pin needed**. Just don't pin the context to a specific app. 2. **Actions → "Require approval for all outside collaborators".** Each fork run must be a deliberate maintainer click before it executes, since the fork's unprivileged run produces the artifacts the privileged `e2e-fork.yaml` consumes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff (7 files: two CI workflows, `docs/agents/e2e-testing.md`, `hack/common-envs.mk` + its bats suite, and two package `Makefile`s). The one trigger worth naming is "change developer tooling (`cozyvalues-gen`, `cozypkg`, the package `Makefile`s) → `development.md`" on the website. It does not fire here: `OCI_EXPORT_DIR` is a CI-only build mode that defaults off, and the local developer workflow is unchanged — `make image` still builds and pushes exactly as before, which the new default-path case in `hack/common-envs_test.bats` now pins. No chart values, no API surface, no app list, no platform variant, no Talos version bump and no release asset rename are touched. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(ci): fork pull requests now run e2e via a privileged workflow, and a fork PR can no longer merge on a skipped (never-run) required E2E Tests check ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a dedicated fork E2E workflow that runs after the base PR and reports results via the required **E2E Tests** commit status. - Fork PRs now export build images as OCI archive artifacts for later publishing. - Added a same-repo E2E reporting job and renamed the in-tree E2E flow to **E2E (in-tree)**. - **Bug Fixes** - Added tighter, fail-closed gating to ensure merge blocking when **E2E Tests** isn’t correctly finalized. - **Documentation** - Documented fork vs same-repo E2E split, OCI export behavior, and required fork security settings. - **Tests** - Added Bats coverage validating `OCI_EXPORT_DIR` and preventing unintended registry pushes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
85fd592cc7 |
fix(cozy-lib): make the cozyconfig helpers survive uninjected values
`_cluster` and `_namespace` reach a chart through valuesFrom on the
cozystack-values Secret, so they are absent whenever a chart is rendered
on its own: `helm lint`, a plain `helm template`, and any helm-unittest
suite that does not set them. Twelve of the helpers in _cozyconfig.tpl
read straight through the missing map and abort the render rather than
returning their documented empty default.
Two spellings, one cause. `index .Values._cluster "root-host"` raises
`index of untyped nil`, and `.Values._namespace.host` raises
`nil pointer evaluating interface {}.host`. In both cases the trailing
`| default ""` never runs: it applies to the result, and there is no
result, only an error. That default covered a key missing from a map
that exists, never a missing map.
Guard the map instead, which is what `cozy-lib.images-registry` in this
same file already did:
(index (.Values._cluster | default dict) "root-host") | default ""
`index` on an empty map returns nil harmlessly, so the trailing default
applies as it always read. With the map present, `| default dict`
returns it untouched.
This changes no rendered output. Every one of the 34 charts that symlink
cozy-lib was rendered before and after, both with the injected values
populated and with them set to empty maps, and every result is
byte-identical. Eight charts differ between any two runs because they
generate passwords; rendering the unchanged tree twice reproduces
exactly those eight, which is how they are excluded.
What this does not do is fix a chart that fails today. The charts that
cannot render standalone read the injected values directly in their own
templates rather than through these helpers, so they are unaffected by a
change to the library and are being addressed where they live.
Twelve of the thirteen helpers here have no callers anywhere in
packages/ at present; only `cozy-lib.images-registry` is reached, from
`cozy-lib.image`, and it was already guarded. The guard is still worth
having on the rest: they are the library's public surface, they are what
a chart author copies when reaching for cluster configuration, and the
broken spelling propagates by being read. `cozy-lib.image` is called
from four templates, so these helpers do get adopted once they exist.
Add a suite in cozy-lib-tests, which is where library behaviour is
pinned because a library chart renders nothing of its own. One case
covers the uninjected state, which needs no mocking since neither key is
in values.yaml, and one covers the injected state so the guard cannot
quietly change what a populated map returns. The injected case includes
`scheduling` and `branding` specifically: those two are the helpers
whose body was restructured rather than wrapped, so they are the ones
whose present-map behaviour most needs pinning. Against the unguarded
helpers the first case fails with `index of untyped nil`.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
9d1d6808db |
test(seaweedfs,api): bound both Roles against a respelled rule
Each suite forbade a rule by naming it in full, and helm-unittest compares every field it is handed by value equality, so one respelling walks past. For seaweedfs that is not theoretical. Ungating the cnpg block and adding a verb to the rule (get,list,delete rather than get,delete) is the same grant and more, and the guard stayed green while a Client instance held delete on another app's Postgres cluster. Bound the Role by shape: a Client renders exactly two rules, both in the core group, and the cnpg grant is gated between them, so ungating it adds a third whatever its spelling. The rules themselves stay unpinned because the secret list is long and changes on its own schedule. For cozystack-api, add a count bound beside the existing spelling assert rather than in place of it. The two catch different things and neither covers the other: templates/rbac.yaml carries no template directives, so the list is pinned at exactly sixteen rules, while a grant swapped in for an existing one leaves the count where it was and only the named-rule assert sees it. The count is pinned from both sides — a dropped grant is the failure this suite is about, since the apiserver then returns forbidden and List silently drops the dropdown. Measured on all three: append, delete, and swap. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a3c1408d08 |
test(harbor): select hook and release documents by kind, not by index
Both suites addressed their subject by document index. An index is a position, so a template that gains a document ahead of the one under test moves every later assertion onto a different object, and what happens then depends on the assertion: measured by inserting one document at the head of each template, the cleanup suite reddens outright, while two of the four jobservice tests keep passing because a `notExists` against an object that never had the path is satisfied for the wrong reason. Indices are also easy to get wrong by hand. helm-unittest numbers documents in template source order, while `helm template` prints them sorted by kind, so the rendered output is not the order being counted. Every document these suites assert on is unique by kind within its template, so each test now names its subject. A selector that matches nothing fails, including on the `notExists` tests, so the anchor holds in the direction that was silent before. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
eac7e1ea15 |
test(tenant): anchor the upgrade.force guard to its HelmRelease
Each test asserted only `notExists: spec.upgrade.force`, which passes on a template that renders no document at all. Renaming the value that gates one of these releases makes the template render nothing and leaves its test green: measured on etcd.yaml, where the guard stayed green while a sibling suite's `equal` was the only thing that noticed the release had vanished. Pin each HelmRelease name alongside the absence so the assertion has a document to be about. Cover computeplane.yaml. Seven templates in this chart emit a HelmRelease and the list held six; the module arrived after the suite was written. The invariant holds there today, so this closes an unguarded template rather than a live regression — which is the same gap that produced this suite, whose first pass missed gateway.yaml. Correct two claims in the header. The template restriction was said to be there because namespace.yaml's `lookup` errors under helm-unittest; it does not, the lookup returns empty offline and the template renders, which is what its own header says and what namespace_ancestor_labels_test.yaml relies on. The restriction stays, for the reason that actually applies: it keeps an unrelated template breaking out of this suite's result. helm-controller also does not deprecate upgrade.force. The CRD vendored at internal/fluxinstall/manifests/fluxcd.yaml, which is what the pinned Flux 2.8.x ships, describes it as a replacement strategy and carries no deprecation marker; `recreate` is the neighbouring field that does. Later upstream documentation adds that the field is ignored under server-side apply, and that sentence is not in the pinned CRD, so the comment says which is which. The reason to keep the flag off is client-side apply, where it replaces resources instead of patching them. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
6d403c3015 |
fix(tenant): retry failed etcd and seaweedfs installs in place
The tenant chart renders both releases with Flux's default install strategy, and install remediation is an uninstall in every case Flux offers. Neither release disables hooks on it, so a remediated install runs each chart's post-delete cleanup Job, and those Jobs delete data on purpose. The etcd one removes the data-etcd-<ordinal> PVCs by name, because the etcd-operator does not reclaim them when the EtcdCluster goes. The seaweedfs one removes the volume PVCs by label and explicitly reclaims the keep-protected CNPG Cluster holding the filer metadata, precisely because nothing else removes it. Both are correct for a real uninstall. Remediation is what calls them, on a release that is only starting slowly. So an install that misses its budget does not merely restart. It destroys the datastore the tenant control planes keep their state in, or the object store's data together with the metadata database that indexes it, and the next attempt begins from nothing. RetryOnFailure keeps the manifests applied and retries the failed release, so no uninstall happens and neither cleanup Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. The cost is that a real upgrade failure retries rather than rolls back, which gives up no stable safety net: with retries: -1 and the default strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The other side of the trade is that the uninstall was also the only automatic clean slate. On a tenant holding no data yet, an install that wedges on a bad resource state now retries against that state until an operator removes the release. For a release that owns data this is the right exchange, because lost data does not come back and a wedged install does. This covers two of the tenant's data-owning releases. The chart's monitoring release destroys its storage on a remediated install as well and is outside this change, as is the rest of the catalog, whose nested releases want the question asked one at a time. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
bd3f3b93d0 |
fix(tenant): render the chart without the platform's injected values
`helm lint packages/apps/tenant` fails, and two of its three errors are
the chart's own.
`templates/keycloakgroups.yaml` reads `index .Values._cluster
"oidc-enabled"`. `_cluster` is injected by the platform, so it is absent
whenever the chart is rendered on its own, and `index` on a nil map is a
hard error: `index of untyped nil`. The chart cannot be rendered or
linted outside a cluster at all.
The idiom the neighbouring charts use for the same lookup does not
rescue this on its own, which is worth stating because it looks like it
does. `(index .Values._cluster "x") | default "y"` still errors: `index`
evaluates first and fails on the nil map, so `default` never sees a
result to substitute; it only ever covered a key missing from a map that
exists.
The spelling alone does not decide whether a chart breaks, though.
`packages/system/cozystack-basics` uses it and lints clean, because line
1 of its values.yaml is `_cluster: {}`, which makes the map non-nil
before `index` runs. So a chart is broken only when it neither declares
the empty map nor guards at the use site. Guarding at the use site is
preferred here because it makes the template correct whatever
values.yaml contains, which is what lets the chart render standalone;
and `_cluster` is platform-internal, while this chart's values.yaml,
schema and README are generated together from
api/apps/v1alpha1/tenant/types.go as user-facing output.
The guard has to sit on the map rather than on the value it returns,
hence `index (.Values._cluster | default dict) "oidc-enabled"`.
That is behaviour-preserving rather than merely quieter. With `_cluster`
present, `| default dict` returns it untouched and the lookup is what it
was. With it absent the lookup is nil, the `eq` below is false, and the
block stays unrendered, which is what an unconfigured tenant should
produce. Rendered both ways before and after, with
`--api-versions v1.edp.epam.com/v1` so the capability-gated block
actually executes, the output is identical byte for byte: four
KeycloakRealmGroups when `oidc-enabled` is `"true"`, none when it is
`"false"`. Rendering with no values at all goes from a hard error to a
clean render with no groups.
`Chart.yaml` also does not declare `cozy-lib`, which is present as a
symlink under `charts/`, so lint reports missing chart metadata. Declare
it the way `packages/apps/qdrant` does, that being the one chart in the
tree that declares it and does not carry this error. Rendering is
unchanged by the declaration, checked the same way.
`tests/keycloakgroups_oidc_test.yaml` pins all three branches of the
gate: four realm groups when `oidc-enabled` is `"true"`, none when it is
`"false"`, and none when the platform injected no `_cluster` at all.
That third case is the one the guard exists for, and it needs no mock,
because `_cluster` is absent from values.yaml and so "not injected" is
simply the chart's own default state. Against the unguarded template the
suite fails with `index of untyped nil`; the chart's other seven suites
pass identically either way, which is what left the change uncovered.
The third error, `invalid icon URL '/logos/tenant.svg'`, is deliberately
left. Lint wants a URL with a scheme, and all 31 charts in the tree that
carry an icon use a relative path, none an absolute one. Changing this
one would make it the only chart with a different convention, and
choosing the URL a relative path should become is a decision about where
those assets are served from, not about lint.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
a27c341843 |
fix(tenant): retry failed monitoring and gateway installs in place
Both releases carry Flux's default install strategy, and install remediation is an uninstall in every case Flux offers. Neither disables hooks on it, so a remediated install runs the app chart's post-delete cleanup Job against a release whose only fault was missing a readiness budget. The monitoring Job deletes PVCs labelled apps.cozystack.io/application.name=<release>-system. That label is not incidental: the metrics and logs claim templates stamp it precisely so the operator copies it onto the storage PVCs, and the cleanup Job selects on it to stay release-scoped. A slow first install therefore deletes the tenant's metrics and logs, and nothing reconstructs them. The gateway Job deletes the cozystack-acme-account Secret. That key regenerates on the next install, so this one costs no data. It costs a re-registration against the ACME provider, and repeated cycles run into its rate limits, which can hold certificate issuance for the tenant down for hours. RetryOnFailure keeps the manifests applied and retries the failed release, so no uninstall happens and neither cleanup Job runs. Both actions carry the strategy because the controller performs that retry as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. Two things are traded away. A real upgrade failure now retries rather than rolls back, which gives up no stable safety net, since with retries: -1 and the default strategy upgrade remediation already rolled back and retried without end. And the uninstall was the only automatic clean slate, so an install wedged on a bad resource state retries against that state until an operator removes the release. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The chart's etcd and seaweedfs releases lose storage the same way and are outside this change. Beyond those four, computeplane, ingress and info remain on the default strategy, but their charts ship no destructive post-delete hook, so remediation costs them a wasteful teardown rather than data. That is a different question from this one. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c20d47a245 |
feat(kubernetes): let a node group log the guest serial console
A worker VM that stalls during boot before Talos apid answers is invisible to every diagnostic the platform has: no Node registers, no certificate request arrives, and talosctl needs the apid that never came up. The guest serial console does show that window, but KubeVirt attaches its guest-console-log container only when the VMI asks for it, and the platform turns console logging off cluster-wide. Add an opt-in per-node-group logSerialConsole that sets the KubeVirt field, which takes precedence over the cluster-wide setting. The key renders nothing unless true -- including when set to false -- because the KubevirtMachineTemplate is content-hash named, so a byte emitted for a group that did not ask for it renames the template and rolls every live worker VM. The description carries the two costs the field name does not show. Changing the value rolls the group it is set on, so the stuck VM whose console was wanted is replaced along with the rest and the setting buys visibility into the next boot rather than the current one. And the cluster-wide default it overrides exists because that same container has been seen holding virt-launcher in PodInitializing, which would take a group down rather than explain it. Worker VMs are emitted by two charts. Both carry the field, and the golden-parity check gains a case that keeps their output identical. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
f4dc5d4d3c |
fix(harbor): retry the nested system install instead of uninstalling it
The harbor app chart renders a nested HelmRelease that owns the whole workload: the harbor Deployments, the inline RedisFailover and its volume. It carried Flux's default install strategy, and install remediation is an uninstall in every case Flux offers, so a first install that went over budget was torn down between attempts and the next one re-provisioned the redis volume and re-ran the master election from zero. That is reachable on a loaded runner. harbor-core blocks on redis while redis waits for its volume and its master election, and the nginx Deployment reaches the default progressDeadlineSeconds of 600s, which Flux reads as a stalled resource and fails the install early. The e2e wait budget for the release is the same 600s, so one teardown cycle consumes all of it and the second attempt cannot finish however fast it converges. RetryOnFailure keeps the manifests applied and retries the failed release, so the convergence already under way continues instead of restarting. Both actions carry the strategy because the controller performs the retry of a failed install as an upgrade, so on install alone the retry's own failure falls through to upgrade remediation. The cost is that a real upgrade failure retries rather than rolls back, which gives up no stable safety net: with retries: -1 and the default strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay because an absent release consults install and an out-of-sync one consults upgrade, both by retry count rather than by strategy. The change lands on the nested release rather than on the parent: the parent is built from the Application by the apiserver, which already stamps this strategy on both actions, while the nested release is the one that owns the workload and the volume a remediation deletes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
605030b5a7 |
test(backupstrategy-controller): anchor the metrics-absence guard to its container (#3605)
## What this PR does The `metrics.enabled=false` case in `deployment_test.yaml` guards against the `metrics.enable` typo regression by asserting that the metrics arg and port are absent. Both assertions address the controller by index, and `notContains` passes whenever its path resolves to something that lacks the forbidden content. A different container at that index qualifies, so a sidecar placed ahead of the controller leaves the guard green while the flag it exists to forbid comes back on the real container. This adds an `equal` on `containers[0].name` ahead of the two negatives. It fails when the index stops pointing at the controller, which is the case the negatives structurally cannot see. Verified rather than argued. Rendering a sidecar ahead of the controller against the current tests leaves the case green and reddens only the two neighbouring tests, which use positive `contains` and defend themselves; the guard that exists for this scenario is not among them. With the anchor, the case fails on the anchor. This is not a new convention. `packages/system/opensearch-operator/tests/leader_election_test.yaml` and `packages/system/dashboard/tests/gatekeeper_test.yaml` already anchor index-addressed negatives the same way, the second with the reasoning spelled out in a comment. This brings one more package in line with what the tree already does. ## What this PR does not do It does not switch the assertions to a name filter. helm-unittest resolves `containers[?(@.name=="...")]`, but `notContains` against a filter that matches nothing passes just as quietly, so that trade swaps "wrong container" for "no container" inside the same family of silent green. It does not touch the other two cases in the file. Both are positive `contains`, both went red under the same injection, so they anchor themselves. ## Tests The change is itself a test fix. Package suite is green: 11 tests, 3 suites. ### Screenshots None. This changes a test file. ### Downstream repositories Walked the trigger map against the diff. One helm-unittest file, nine added lines, no chart output changes. - [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: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved deployment validation by confirming the correct container is targeted when metrics are disabled. * Added checks that metrics-related arguments and ports are absent in this configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0ba567dd73 |
fix(tenant): bound the pre-delete cleanup waits and the hook Job (#3598)
## What this PR does The tenant pre-delete hook deletes the tenant's HelmReleases in two waves with `kubectl delete --wait=true` and no `--timeout`. kubectl reads `--timeout=0` as "wait forever" and substitutes a week, so a wave hangs for as long as any child HelmRelease cannot finalize. helm then returns from the uninstall without deleting anything: the release stays in storage, flux keeps the finalizer, and the same teardown gets retried. The delete policy carries no `hook-failed`, so the waiting Job survives between attempts too. Each wave now gets 90s. Neither is guarded, on purpose. Nothing stands behind these deletes, so a wave that did not finish ends the run instead of reporting a cleanup that never happened. That also keeps the order the two waves exist for, since the tenant modules are what the applications run on. Whatever got deleted stays deleted, so the next attempt resumes from there. The Job gets `activeDeadlineSeconds: 270`. It bounds what the waits do not, since the DELETE calls carry kubectl's default `--request-timeout` of 0, and it sits under the 300s uninstall budget flux applies when the HelmRelease has no timeout of its own. That is the budget for tenants cozystack-api generates; `tenant-root` is the exception, a static release in `cozystack-basics` carrying `timeout: 15m0s`, and the ordering holds there too since 280 is under 900. The 30s gap is not cosmetic. The deadline runs from the Job's `.status.startTime` while helm's budget runs from before the Job is created, so at equal values helm gives up first, on a Job that is still running. That is the state this change is meant to end, and it would look fixed, because a number would be there. `terminationGracePeriodSeconds: 10` is part of the same arithmetic. Since 1.31 the Job controller holds back the terminal condition until every pod has finished terminating, so what helm waits for is the deadline plus the termination. At the 30s default that lands on 270+30, level with the budget, which is the gap the deadline was set to buy. That period gets spent in full, every time. The SIGTERM goes to PID 1, which is the shell: the script runs several commands, so the shell never execs into kubectl and stays PID 1 itself. A signal carrying its default disposition is discarded by the kernel when the target is PID 1, and a non-interactive shell installs no SIGTERM handler, so nothing acts on it. kubectl is a child and was not signalled at all. The pod ends on SIGKILL. Nothing in that script needs to finish gracefully, the DELETE calls are already sent and the process is only watching. The pod also gets resource requests, matching the sibling hook in `packages/apps/kubernetes`. Without them it is BestEffort and first to go under node pressure, which mattered less when a retry inside the Job absorbed an eviction. With a single attempt it does not. `backoffLimit: 0` and `restartPolicy: Never` go together, and neither implies the other. At the default limit of 6 the Job does not reach `Failed` inside the budget, so helm falls back on its own timeout with the same nameless "timed out waiting for the condition" as before. Under `OnFailure` the pod survives its own failure and the kubelet re-runs the container in place, and the Job controller finds out afterwards by summing `restartCount`. The script starts over, re-enters the applications wave that just expired, and gets cut down mid-run when the controller catches up. `Never` puts a failed pod in front of the controller directly. ## What this PR does not do No remedy behind the bounded waits. The `packages/apps/kubernetes` hook force-clears child finalizers when its own wait expires. That needs `patch` on HelmReleases in the Role, and it is only safe there because the whole tenant control plane is going away in the same run. Here a wave that expires ends the attempt. `packages/apps/kubernetes/templates/delete.yaml` is untouched. It does not make a doomed teardown converge. A tenant whose children cannot finalize still gets its uninstall retried. What changes is that each attempt ends in bounded time, instead of eating the whole uninstall budget and leaving a pod behind. 90s is a guess, and it is tight. The first wave deletes every application HelmRelease in the namespace, including kinds that are slow to tear down: a nested Tenant runs this same hook one level down before its release finalizes, and an app of kind Kubernetes tears a control plane down. Either one can outrun 90s. The bound is what is left once the Job deadline sits under helm's budget instead of level with it, split between the two waves. The 90s the deadline keeps back for scheduling and the image pull is not calibrated either, and I would rather say so than imply it was. `docker.io/clastix/kubectl:v1.32` comes from Docker Hub with no mirror in front of it, and this repo has measured Docker Hub pulls of a smaller image at about 1m42s in CI. So that remainder is roughly the observed worst case rather than comfortably above it. A cold pull that slow lands on the `activeDeadlineSeconds` path, which is the one that deletes the pod and leaves nothing to read. There is a band that pays for this, and I would rather name it than round it off. Neither shape above fits inside 300s today, so for them the bound costs nothing that works. But every generated child HelmRelease carries a wait strategy, so a wave inherits its slowest child, and a wave that settles in something like 95s does complete inside the budget today. After this change it expires instead, and that tenant trades one clean attempt for an extra flux retry. In wall clock that retry is about five minutes, since `helmrelease-interval` defaults to `5m`, so a tenant in that band takes noticeably longer to delete than it does now. I think that is the right side of the trade: an unbounded wave does not fail, it spends the whole budget and leaves helm reporting a timeout that names nothing. If the band turns out to be populated, the waits and the deadline move together, and the bats suite pins their ordering rather than their values. One caveat on the diagnostics, since the comments in the chart used to promise more than they deliver. The failing wave leaves its output in the failed pod, which `ttlSecondsAfterFinished: 300` then reaps while flux's uninstall backoff is still climbing. And on the `activeDeadlineSeconds` path there is no log at all, because the Job controller deletes the active pods when the deadline expires. That path is the backstop for a kubectl that never returns; the expected path is the wave's own `--timeout`, where the script exits on its own and the pod survives to be read. ## Tests `hack/tenant-pre-delete-hook.bats` renders the chart, pulls the hook script out of the Job and runs it against a stub `kubectl` on `PATH`. It covers a clean run reporting success, every delete reaching kubectl with a non-zero bound, the ordering of the four numbers, and a wave that gave up ending the run without going on to the next one. `--timeout` is implemented inside kubectl, so no stub can make that deadline actually elapse. The tests read the arguments that reached kubectl and inject the non-zero exit an expired delete produces. No EXIT traps. The ordering test reads the numbers out of the rendered Job instead of pinning them, so moving a value keeps it honest while a value that breaks the ordering still fails. An absent deadline or termination period is rejected explicitly, otherwise `null` satisfies the numeric comparison and a Job with no bound at all passes. The Job's own shape is pinned in the chart's helm-unittest suite, since the script cannot observe that about itself. Both suites are green locally, and I mutated each pinned value to confirm they fail for the right reason: dropping either `--timeout`, dropping `activeDeadlineSeconds`, dropping `terminationGracePeriodSeconds`, dropping the requests, `backoffLimit` back to 6, `restartPolicy` back to `OnFailure`, and a `|| echo` appended to the applications wave. Each one goes red in the test that claims that ground. Worth flagging so the check marks are not read as more than they are: `make bats-unit-tests` is red at the merge base, unrelated to this branch, and the loop it runs exits on the first failing file. `hack/cozyreport.bats` sorts ahead of this suite, so on CI the new bats file is not reached at all right now. It is not failing, it is not running. The helm-unittest cases are unaffected and do execute. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the tenant chart, its helm-unittest suite, and a new `hack/*.bats` file. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is moved or renamed and no make target changes behaviour, since `bats-unit-tests` already globs `hack/*.bats`. - [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: ### Release note ```release-note fix(tenant): bound the pre-delete cleanup hook's HelmRelease deletions and give its Job a wall-clock deadline, so a tenant whose children cannot finalize no longer leaves the hook waiting indefinitely and blocking the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability Improvements** * Tenant cleanup jobs now enforce execution deadlines and terminate gracefully. * Cleanup operations have bounded deletion timeouts and no automatic retries. * Failed cleanup steps now stop subsequent operations and correctly report failure. * Cleanup jobs now specify CPU and memory resource requirements. * **Tests** * Added comprehensive coverage for cleanup behavior, timeouts, failure handling, and job lifecycle settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0b7a8f67d1 |
fix(apps/kubernetes): report the pre-delete cleanup outcome honestly (#3592)
## What this PR does The pre-delete cleanup Job for the `kubernetes` app ended with an unconditional `echo "Cleanup completed successfully"`. Several of its steps swallow a failure behind `|| echo "WARNING: ..."` so that `set -e` cannot end the run, which means that line was printed in exactly the situation those warnings exist to report: child HelmReleases still holding Flux finalizers, or a Kamaji datastore Secret still pinning its namespace. Someone reading the Job log to find out why a namespace will not delete was told the cleanup had worked. The Job now records what a step left behind and closes with either the success line or the list of steps that left something. Only residue gets recorded. A bounded delete that times out has a remedy right behind it (Step 2 falls through to force-clearing finalizers, Step 4 falls through to Step 4b), and whether anything survived is decided by that remedy, which reports its own outcome. A tenant with no working nodes takes that route as a matter of course, which is the reason the wait is bounded at all, so recording the timeout itself would report a finished teardown as incomplete on the commonest path there is and send an operator hunting for objects that are not there. That is the same defect as the one being fixed, pointing the other way. A read that fails looks exactly like one that found nothing. An empty list and a failed list both give a loop nothing to iterate; an absent object and an unreachable apiserver both make a probe non-zero. So the whole script was walked for that shape instead of patching the instances as they were reported. Four reads reach the closing line. Step 2's force-clear re-list now keeps its status and reports when it cannot list what it was about to clear. Step 4b's two probes became three-way, present, absent or unknown, so a Secret whose existence could not be established is no longer announced as absent. Step 1's list is the one left swallowing its status, and that is deliberate: Step 2 deletes by label rather than from that list, so a suspend that never happened leaves nothing of its own behind, and reporting it would name residue that does not exist. Making the TenantControlPlane probe three-way also repairs the gate above it, and that is intended. The gate is there so the datastore Secret's finalizer is not stripped while Kamaji may still own it, and a two-way probe defeated it in the one direction that does damage: any error read as "confirmed absent" and the strip went ahead. Its likeliest cause, a throttled or briefly unreachable apiserver, is also likeliest exactly when a broken tenant is being torn down. Step 4b declining to strip a finalizer while the TenantControlPlane is still alive is the one residue that no failing command marks: every call in that branch succeeds, and the namespace stays blocked regardless. It now reports, and names the TenantControlPlane as the thing holding it up. What this PR does not do: the hook still exits zero, even with every backstop engaged. That is the difference between a main path and a backstop. `packages/apps/tenant/templates/cleanup-job.yaml` is the same kind of hook and handles it the opposite way, correctly: deleting the HelmReleases is that hook's whole purpose, so a failure there means the cleanup did not happen, and `set -e` ending the run before the success line is honest. The swallowed sites here are backstops. They run only after the normal deletion has already timed out, and they exist to push teardown further along. Failing there aborts the Helm uninstall before the release is removed, so the release stays in storage and the same doomed teardown is retried forever, at precisely the moment the backstop was written for. The steps that are not backstopped still end the run under `set -e` and never reach the closing message at all. This makes the message truthful. It does not make it reachable. The Job carries `helm.sh/hook-delete-policy: hook-succeeded`, and the hook exits zero by design on every backstopped run, so Helm reaps the Job, and its pod log, right after the hook completes. Whether the closing line survives long enough to be read is a question about the delivery channel, not about its wording, and changing that channel (an Event on the CR, or keeping the Job when something was left behind) has a different blast radius. Left for a separate change. The step comment and the runtime warning both cited the tracking issue for the Kamaji datastore-secret leak as though it were live. It is closed, and closed by this step rather than by a fix in Kamaji, so following the pointer showed a resolved report and no reason for the code to exist. The comment now says the ordering it covers is still reachable, and the warning names the finalizer and the object to clear by hand instead of an issue number. The workaround itself is untouched: nothing indicates the upstream cause has gone away. `hack/kubernetes-pre-delete-hook.bats` renders the Job, extracts the script and runs it against a stub `kubectl`, covering a clean run, a fallback route that succeeded, a TenantControlPlane outliving Step 4, each probe that could not answer, every remedy failing at once, an unbackstopped step ending the run, and the zero-exit invariant. The helm-unittest cases match the script's source text and cannot tell whether the closing message is reachable. Every case was checked by mutation, including reinstating the unconditional success line and reinstating the over-reporting the fallback case guards against. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change ### Release note ```release-note fix(kubernetes): the pre-delete cleanup hook no longer reports success when a cleanup step left something behind, and no longer reports a completed teardown as incomplete when it merely fell back to force-clearing finalizers. The Job log now names the steps that left residue. The hook still exits zero so the Helm uninstall can remove the release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes cleanup handling during application deletion. * Cleanup now distinguishes unavailable resource checks from confirmed absence, avoiding unsafe finalizer removal. * Reports leftover resources when recoverable cleanup steps fail while still completing the deletion process. * Improved handling of child releases, control-plane resources, and datastore secrets. * **Tests** * Added comprehensive coverage for successful cleanup, partial failures, resource residue, probe errors, and unrecoverable steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
cab44d56ac |
test(tests): pin the keep annotation on cert-manager and etcd-operator CRDs (#3590)
## What this PR does
`cert-manager-crds` and `etcd-operator-crds` are the two packages this
repository points at as the in-tree precedent for protecting CRDs with
`helm.sh/resource-policy: keep`. Both carry the annotation, neither had
a test, so it could be lost with nothing reporting the loss. This adds a
suite to each, plus the `test:` target that `hack/helm-unit-tests.sh`
needs before it will run one. No annotation is added and no rendered
output changes: the point is to close the way of losing what is already
there.
The two are exposed differently, so the suites say so instead of sharing
one explanation. In `etcd-operator-crds` the annotation is ours. `make
update` stamps it with an awk step anchored on the
`controller-gen.kubebuilder.io/version:` line that the upstream CRDs
carry. If a future controller-gen stops emitting that line, the step
matches nothing, writes the CRDs out unstamped and exits zero.
In `cert-manager-crds` the annotation comes from the upstream template,
emitted under `{{- if .Values.crds.keep }}`, and the value that switches
it on is ours. That is two independent ways to lose it. The templates
are also not maintained in that package:
`packages/system/cert-manager/Makefile` deletes and refills its
`templates/` directory during its own `make update`, so a cert-manager
bump rewrites a neighbouring package that whoever runs the bump has no
reason to open.
Each suite is scoped with the wildcard `templates/*.yaml` and asserts
across every document rendered under it, rather than listing CRD names
or pinning a document count, so a CRD that upstream starts shipping is
covered without editing the test. Each also asserts the kind, because
the annotation assertion on its own would accept a non-CRD document that
happened to carry it. Both charts render only CRDs today, checked with
`helm template`.
The `test:` target is load-bearing rather than boilerplate.
`hack/helm-unit-tests.sh` runs a package's suite only when its Makefile
defines that target. With the target present, breaking a chart makes the
whole run exit 1 and name the directory. With the fixtures present but
the target removed, the same broken chart gives exit 0 and "All Helm
unit tests passed", having executed the suite zero times.
### 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:
I walked the trigger map in `docs/agents/contributing.md` against the
file list of this diff. It adds two test files and two `test:` targets
under `packages/system/`, and touches nothing else: no
`hack/package.mk`, no app `values.schema.json`, no platform values, no
`ApplicationDefinition`, no release asset names. The website's
"developer tooling (the package Makefiles)" trigger is the one worth
checking, so I read `content/en/docs/next/development.md` there: it
documents `make test` in general terms and does not go stale from a
package gaining that target.
### Release note
```release-note
NONE
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Tests**
* Added automated validation for cert-manager and etcd operator CRD
charts.
* Confirmed rendered resources are correctly identified as
CustomResourceDefinitions.
* Verified CRDs retain the required `keep` resource policy during Helm
operations.
* **Chores**
* Added convenient test commands for running Helm chart validation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
177da7843d |
fix(kubernetes): keep tenant addon CRDs when a release is uninstalled (#3586)
## What this PR does Four tenant addon HelmReleases render their CRDs from a chart's `templates/` directory and remediate a failed install by uninstalling the release. An uninstall deletes what the chart rendered, and deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every Gateway, VerticalPodAutoscaler, ServiceMonitor or VolumeSnapshot in the tenant cluster with it. This annotates the CRDs of the four packages behind them, `gateway-api-crds`, `vertical-pod-autoscaler-crds`, `prometheus-operator-crds` and `vsnap-crd` (that last one is declared by `helmreleases/volumesnapshot-crd.yaml`), with `helm.sh/resource-policy: keep`, which is what `cert-manager-crds` and `etcd-operator-crds` already carry in this tree. Three of the four regenerate `templates/` from upstream on `make update`: two delete the directory outright and the third overwrites its file. So the annotation is written into the vendored files and reapplied by a step in each `update:` target. Without that second half the next regeneration drops it silently, with nothing reporting the loss. I ran `make update` for real to check the pair holds: `gateway-api-crds` and `vsnap-crd` come back byte-identical to what is committed here, and `vertical-pod-autoscaler-crds` reproduces the annotation on both of its CRDs. The rest of the VPA diff is upstream drift, because that package's `update:` tracks a branch rather than a pinned ref; the bump itself is not in this PR. The step is idempotent, so running it twice does not stack a second annotation into the same mapping. `prometheus-operator-crds` ships its CRDs in a vendored subchart that `make update` re-pulls, so nothing under `charts/` is edited. The upstream template already renders `.Values.annotations` onto every CRD it emits, and the annotation is set through the package's `values.yaml`. The Gateway API kustomization also emits a `ValidatingAdmissionPolicy` and its binding. Those hold no user data, so the stamping step selects on `kind: CustomResourceDefinition` and leaves them alone. Each package gains a helm-unittest suite and the `test:` target that `hack/helm-unit-tests.sh` requires before it will run a suite at all. Every suite asserts the annotation across all CRDs its chart renders, not only the ones listed by name, so a CRD arriving in a later upstream bump is covered without editing the test. The annotation has a cost worth stating plainly: uninstalling one of these releases on purpose now leaves the CRDs behind, to be removed by hand. That is the same trade-off `cert-manager-crds` already carries. This covers the first of the two directions in #3555. The second one, setting `install.strategy.name: RetryOnFailure` on these releases, is a separate decision with a different blast radius and is not part of this PR. ### 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: I walked the trigger map in `docs/agents/contributing.md` against the file list of this diff. It touches four packages under `packages/system/` and nothing else: no `hack/package.mk`, no app `values.schema.json`, no platform values, no `ApplicationDefinition`, no release asset names. The one line that looked close is the website's "developer tooling (the package Makefiles)" trigger, so I read `content/en/docs/next/development.md` in the website repo: it documents `make update` and `make test` in general terms, and neither goes stale from this change. ### Release note ```release-note fix(kubernetes): tenant addon CRDs for Gateway API, VerticalPodAutoscaler, Prometheus Operator and VolumeSnapshot now carry `helm.sh/resource-policy: keep`. A failed install that remediates by uninstalling the release no longer deletes those CRDs and, with them, every custom resource of those kinds in the tenant cluster. In exchange, uninstalling one of these releases on purpose now leaves the CRDs in the cluster to be removed by hand. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Custom Resource Definitions for Gateway API, Prometheus Operator, Vertical Pod Autoscaler, and VolumeSnapshot are now retained when their Helm charts are uninstalled. * Prevents loss of custom resources and related configuration during chart lifecycle operations. * **Tests** * Added automated validation confirming all included CRDs receive the retention policy. * Added chart test commands to simplify verification of CRD updates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
c598067160 |
fix(etcd): fail the CRD vendoring when the keep stamp misses (#3595)
## What this PR does `make update` in `packages/system/etcd-operator-crds` vendors three CRDs and stamps `helm.sh/resource-policy: keep` onto each with an awk step anchored on the `controller-gen.kubebuilder.io/version:` line. That line is upstream's to emit. If a future controller-gen stops writing it, the awk matches nothing, writes the CRD out unstamped and exits zero, so the regeneration reports success and the CRDs land without the annotation that keeps a chart uninstall from deleting them, and with them every EtcdCluster, EtcdMember and EtcdSnapshot in the cluster. The step now counts what it produced and requires exactly one annotation per CRD. Zero means the anchor is gone; more than one means it matched somewhere unintended. Every CRD is staged in a temporary directory and `templates/` is replaced only after all of them have been counted, so a failure on any one leaves the vendored files exactly as they were rather than wiping the directory and refilling part of the set. I checked it by substituting the upstream response, so what ran was the recipe that ships rather than a copy of its logic: a CRD with no controller-gen line fails with `stamped 0 keep annotations, expected 1`, a CRD carrying that line twice fails with 2, a failure on the second of the three leaves all three committed files untouched, and the real upstream still vendors byte-identical output, with `make update` leaving `git status` showing nothing but this Makefile. On `main` today this is the only thing guarding those three CRDs. A helm-unittest suite that fails when one of them renders without the annotation is in #3590, which is open and not merged. Once that lands the two catch the same loss at different moments: the suite on a test run, this step in front of whoever ran the regeneration. The same stamping shape exists in three more packages, but only on the branch of #3586, which adds it to `gateway-api-crds`, `vsnap-crd` and `vertical-pod-autoscaler-crds`. There is nothing to guard there on `main` yet, so they are deliberately out of this change and need the same step once #3586 merges. ### 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: I walked the trigger map in `docs/agents/contributing.md` against the file list of this diff, which is one package Makefile and nothing else. `hack/package.mk` is untouched, so the `ccp` and `external-apps-example` triggers that key on it do not fire, and the website's "developer tooling (the package Makefiles)" line is the only near miss: `content/en/docs/next/development.md` describes `make update` in general terms and does not go stale from a package validating its own output. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved CRD update validation to ensure generated definitions include the required preservation annotation. * Prevented incomplete or invalid updates from replacing existing CRD templates. * Added automatic cleanup of temporary update files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
ab8675161c |
fix(kubernetes): bound the remaining pre-delete hook deletions (#3604)
## What this PR does Three of the six steps in the `kubernetes` pre-delete hook call `kubectl delete` with neither `--wait=false` nor `--timeout`. `--wait` defaults to true and `--timeout=0` is implemented as a week, so the KamajiControlPlane delete in step 3, the DataVolume delete in step 5 and the LoadBalancer Service delete in step 6 each park the hook for 168 hours on one stuck finalizer. A DataVolume held by a CDI importer or a PVC finalizer is the reachable case. The waits in steps 2 and 4 were bounded earlier, in response to a report that named the child-HelmRelease wait and nothing else. The fix stopped where the report did, so the shape it described stayed in the three steps nobody had looked at. None of the three waits now. Nothing downstream reads the result of steps 5 and 6. What matters after step 3 is the TenantControlPlane, and step 4 deletes that one directly and waits for it, so blocking on the KamajiControlPlane first only adds a second place for a finalizer to stall. An expired wait and `--wait=false` leave the same cluster state, with the same objects finalizing in the background. Not waiting also keeps these steps under `set -e` with no guard, which matters more than it looks. A guard cannot tell an expired wait from a rejected request, so it would report a failed delete as cleanup that happened. With no timeout to expire, every non-zero exit left is a real failure. The `watch` verb goes away for those three resources. kubectl opens a watch only on the wait path, so the grant was unexercised. `helmreleases` and `tenantcontrolplanes` keep it because steps 2 and 4 still wait, and there the verb is load-bearing in a way that is easy to miss: removing it does not produce an error, it hangs the delete. That is the failure this repo already hit once at the tenant level, so the whole rule set is pinned rather than left to the next reviewer of this Role. `backoffLimit` goes to 0, against a default of 6 that `restartPolicy: Never` turns into seven pods in series, each restarting the script from step 1 and each paying its own image pull. ## What this PR does not do It does not add `activeDeadlineSeconds`, and that is arithmetic rather than oversight. A deadline has to sit above the sum of the remaining waits, or it cuts runs that were going to finish, and far enough under helm-controller's uninstall budget to be the thing helm reports rather than something helm waits through. Those two bounds do not currently leave a gap, and the image pull comes from Docker Hub with no mirror in front of it. Room comes from raising `spec.uninstall.timeout` on the generated HelmRelease first; squeezing the waits further is the wrong direction. Worth stating plainly, because it is the one change here that is not purely a tightening: with `backoffLimit: 0` a pod eviction now fails the whole uninstall attempt, where six retries previously absorbed it. That is deliberate. Both remaining waits are guarded, so an attempt can spend its entire budget without failing, and a retry would restart from step 1 with the clock already there; on a nodeless tenant it would also re-pay step 4, since the first attempt leaves the TenantControlPlane still terminating. The failure a retry would cover is an eviction, which is not bounded in time and costs most when it arrives late. Flux still repeats the uninstall on its own interval, so the end state is preserved and the trade is wall-clock for predictability. It does not bound the requests. `--wait=false` and `--timeout` bound the waiting, so an apiserver that accepts the connection and then stops answering hangs this hook exactly as before. That is a different failure from the stuck finalizer, it is not made worse here, and it wants one answer across every call rather than a flag on the deletes. ## Tests `packages/apps/kubernetes/tests/delete_hook_test.yaml` gains cases for the bounds and for the Role. Every delete in the hook is asserted to carry one, the Job is pinned to a single attempt, and the rule set is pinned whole. The regex patterns walk only continuation lines, the ones ending in a backslash, so each stays inside the command it names. A cross-line wildcard reaches a flag belonging to the next command instead, and RE2 has no lookahead to exclude the `echo` in between. The Role assertions use whole-rule `contains` plus `lengthEqual` rather than per-index verb checks. `notContains` passes when its path does not resolve, so an index-addressed "and nowhere else" goes quiet the moment the rules list is reordered or shortened, which is exactly the edit it exists to catch. Matching the entire rule pins the verb set in both directions and names the resource when it fails. Every pinned value was mutation-tested: dropping `--wait=false` from each of the three deletes, dropping `--timeout` from either of the two that still wait, `backoffLimit` in both directions, adding `watch` back, removing `watch` from a resource that waits, appending a rule past everything asserted, and reordering the rules list. Each one fails the assertion that claims that ground and no other. ### Screenshots None. This changes a Job manifest and its tests, with no user-visible surface. ### Downstream repositories Walked the trigger map against the diff. The diff is one Job manifest inside the kubernetes chart and its helm-unittest suite. No package is added, renamed or removed; no `values.schema.json`, `values.yaml` default or version enum changes; no `ApplicationDefinition` semantics, namespace, variant or release asset changes; nothing under `hack/` is touched and no make target changes behaviour. - [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: ### Release note ```release-note fix(kubernetes): bound the pre-delete hook's remaining unbounded deletions and limit its Job to a single attempt, so a stuck finalizer on a DataVolume, LoadBalancer Service or KamajiControlPlane no longer parks the hook and blocks the uninstall ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes resource cleanup during deletion to avoid indefinite waits. * Cleanup operations now complete with bounded waiting and fewer retry attempts, improving reliability and responsiveness. * Removed unnecessary access permissions from cleanup operations, strengthening security. * **Tests** * Added coverage to verify deletion timeouts, retry behavior, and required permissions across supported resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4e3a679e7b |
test(apps): anchor index-addressed negative asserts to what they name
notContains passes whenever its path resolves to something that simply lacks the forbidden content, and a different element at the same index qualifies. These guards would go green if the Role's rule order changed or a nodePool were added ahead of the one they name, while the resource they forbid came back on the element they meant. Assert the addressed element before asserting what it must not have. The Kafka Role rules have no identifier, so the anchor is the in-cluster bootstrap Service that rule must keep. The OpenSearch pool does have one, `component`, and that is what is pinned: roles are shared between pools, so a role-based anchor would be satisfied by a second data-bearing pool inserted ahead of this one. Each anchor was checked against the shift it exists to catch, and against the same shift with the anchors removed: without them the blocks stay green while the forbidden content is back on the element they name. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
8aadbd5731 |
test(backupstrategy-controller): anchor the metrics-absence guard to its container
The two notContains assertions in the metrics.enabled=false case address the controller by index. notContains passes whenever its path resolves to something that lacks the forbidden content, and a different container at that index qualifies, so a sidecar inserted ahead of the controller leaves both assertions green while the flag they forbid comes back on the real container. Assert the container's name at the same index first. That fails when the index stops pointing at the controller, which is the case the negatives structurally cannot see. Verified by rendering a sidecar ahead of the controller: before this change the case stayed green and only unrelated tests in the same file went red, by accident rather than by design; after it, the anchor fails. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b60c08f110 |
fix(kubernetes): bound the remaining pre-delete hook deletions
Three of the six steps in the pre-delete hook call kubectl delete with neither --wait=false nor --timeout. kubectl defaults --wait to true and reads --timeout=0 as "wait forever", which it implements as a week, so the KamajiControlPlane delete in step 3, the DataVolume delete in step 5 and the LoadBalancer Service delete in step 6 each park the hook for 168 hours on a single stuck finalizer. A DataVolume held by a CDI importer or a PVC finalizer is the reachable case. None of the three waits now. Nothing downstream reads the result of steps 5 and 6, and what matters after step 3 is the TenantControlPlane, which step 4 deletes and waits for directly; blocking on the KamajiControlPlane first only adds a second place for a CAPI finalizer to stall. An expired wait and --wait=false leave the same cluster state either way, with the same objects finalizing in the background once the hook has moved on. Not waiting also keeps these steps under `set -e` with no guard. A guard cannot tell an expired wait from a rejected request, so it would report a failed delete as cleanup that happened; with no timeout to expire, every non-zero exit left is a real failure. Drop the watch verb from the three resources those steps delete, and pin the whole rule set. kubectl opens a watch only on the wait path, so the grant was unexercised for the three; for helmreleases and tenantcontrolplanes its absence would not error but hang, which is the failure mode hardest to read from the outside. Set backoffLimit to 0, against a default of 6 that restartPolicy Never turns into seven pods in series. Zero rather than one because both remaining waits are guarded, so an attempt can spend its whole budget without failing and a retry would start from step 1 with the clock already there; on a nodeless tenant it would also re-pay step 4, since attempt one leaves the TenantControlPlane still terminating. The failure a retry would cover is an eviction, which is not bounded in time and is worst exactly when it arrives late. The price is that such an eviction fails the whole attempt and flux repeats it on its own interval. No activeDeadlineSeconds: the waits and helm-controller's uninstall budget leave no gap a deadline could sit in, once a Docker Hub image pull is counted. That needs the budget raised first, through spec.uninstall.timeout on the generated HelmRelease, and is left out rather than set to a number that would cut runs which were going to finish. These bounds cover the waiting, not the request: --request-timeout is still 0 on every call in the hook, so an unresponsive apiserver hangs it as before. That is a different failure from the stuck finalizer, and it wants one answer across every call rather than a flag on the deletes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
cf85716444 |
fix(tenant): bound the pre-delete cleanup waits and the hook Job
kubectl reads --timeout=0 as "wait forever" and substitutes a week, so the two `kubectl delete helmreleases --wait=true` calls in the tenant pre-delete hook hang for as long as any child HelmRelease cannot finalize. helm returns from Uninstall.Run before deleting anything, so the release stays in storage and flux retries the same teardown; helm.sh/hook-delete-policy carries no hook-failed, so the waiting Job survives between attempts as well. Bound each wave at 90s. Neither is guarded: nothing stands behind these deletes, so a wave that did not finish ends the run rather than report a cleanup that did not happen, and ending it keeps the tenant modules from being torn out from under applications that are still deleting. Whatever was deleted stays deleted, so the next attempt resumes from there. The bound has a price and it is not zero. A nested Tenant, or an app of kind Kubernetes whose own hook spends two 120s waits first, misses 90s and converges across uninstall retries instead; neither fits the 300s budget as it stands either. Between those and the fast case sits a band that does pay: a wave inherits its slowest child, so one that settles around 95s completes inside the budget today and expires here instead, trading a clean attempt for one more retry. An unbounded wave does not fail, it spends the whole budget and reports a timeout naming nothing, so the trade is worth taking. Give the Job an activeDeadlineSeconds of 270. It bounds what the waits do not — the DELETE calls carry kubectl's default --request-timeout of 0 — and sits under the 300s uninstall budget flux applies when the HelmRelease carries no timeout of its own, because the deadline runs from the Job's startTime while that budget runs from just before the Job is created. At equal values helm gives up first, on a Job still running. Cap the pod's termination at 10s, which is part of that arithmetic rather than a detail of it. Since 1.31 the Job controller withholds the terminal condition until every pod has finished terminating, so helm learns of the failure at the deadline plus this period; the 30s default would land it at 270+30, level with the budget the deadline is set under. The period is spent in full: the SIGTERM goes to PID 1, which is a shell running a multi-command script, and a default disposition sent to PID 1 is discarded by the kernel with no handler installed to catch it. kubectl is a child and is never signalled. Set backoffLimit to 0 and restartPolicy to Never. The limit is needed under either policy, since the default of 6 keeps the failure from reaching helm inside the budget. The policy matters because under OnFailure the pod survives its own failure and the kubelet re-runs the container in place, with the controller applying the cap only afterwards by summing restartCount: the script re-enters the wave that just expired and is cut down mid-run. Never leaves a failed pod the controller sees directly. Add resource requests to match the sibling hook in packages/apps/kubernetes. Without them the pod is BestEffort and first out under node pressure, which a retry inside the Job used to absorb and a single attempt does not. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
d90a6a5f46 |
fix(prometheus-operator-crds): keep the CRDs on uninstall
The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every ServiceMonitor, PodMonitor, PrometheusRule and ScrapeConfig in the tenant cluster with it. The CRDs ship inside a vendored subchart that `make update` re-pulls, so the annotation is set through the chart values the subchart already renders rather than by editing templates that would be overwritten. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
82f5255f5a |
fix(vsnap-crd): keep the CRDs when the release is uninstalled
The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every VolumeSnapshot and VolumeSnapshotContent in the tenant cluster with it — the objects that carry the only reference to a snapshot on the storage backend. Stamp helm.sh/resource-policy: keep on each CustomResourceDefinition and teach `make update` to reapply it, since that target rebuilds templates/ from upstream and would otherwise drop the annotation silently. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5d5d8f58aa |
fix(vertical-pod-autoscaler-crds): keep the CRDs on uninstall
The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every VerticalPodAutoscaler in the tenant cluster with it, along with the recommendation history held in the checkpoints. Stamp helm.sh/resource-policy: keep on each CustomResourceDefinition and teach `make update` to reapply it, since that target refetches the manifest from upstream and would otherwise drop the annotation silently. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
a822ad10ff |
fix(gateway-api-crds): keep the CRDs when the release is uninstalled
The tenant addon HelmRelease remediates a failed install by uninstalling the release, and an uninstall deletes whatever the chart rendered. Deleting a CRD makes the API server garbage-collect every custom resource of that kind, so a failed reinstall takes every Gateway, HTTPRoute and ReferenceGrant in the tenant cluster with it. Stamp helm.sh/resource-policy: keep on each CustomResourceDefinition and teach `make update` to reapply it, since that target rebuilds templates/ from upstream and would otherwise drop the annotation silently. The ValidatingAdmissionPolicy pair the experimental kustomization also emits holds no user data and stays unannotated. Removing the release on purpose now leaves the definitions behind to be deleted by hand. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ac2f882829 |
test(etcd-operator-crds): pin the keep annotation on the CRDs
The CRDs carry helm.sh/resource-policy: keep, which is what stops a release uninstall from deleting them and taking every EtcdCluster, EtcdMember and EtcdSnapshot in the cluster with it. Nothing asserted that. The annotation is stamped by the awk step in `make update`, anchored on the controller-gen version line the upstream CRDs carry. If that line stops being emitted, the step matches nothing and exits zero. Scope by wildcard rather than a list of filenames, so a CRD added to the vendoring list is covered without editing the test, and assert the kind alongside the annotation, since the annotation assertion on its own accepts a non-CRD document that carries it. Add the test: target too. hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines that target, so without it the fixtures sit unread and the run reports success on a chart that has already lost the annotation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ead3ab4ed6 |
test(cert-manager-crds): pin the keep annotation on the CRDs
The CRDs carry helm.sh/resource-policy: keep, which is what stops a release uninstall from deleting them and taking every Certificate, Issuer and Order in the cluster with it. Nothing asserted that. The annotation is not local to this package on either side: it comes from the upstream template under a conditional, the value that switches it on is ours, and the templates are refilled from the vendored upstream chart by packages/system/cert-manager on its own update. The assertion reads the rendered output, so it does not depend on which of those moves. Scope by wildcard rather than a list of filenames, so a CRD that upstream starts shipping is covered without editing the test, and assert the kind alongside the annotation, since the annotation assertion on its own accepts a non-CRD document that carries it. Add the test: target too. hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines that target, so without it the fixtures sit unread and the run reports success on a chart that has already lost the annotation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ed6135d77d |
fix(etcd): fail the CRD vendoring when the keep stamp misses
`make update` stamps helm.sh/resource-policy: keep onto each vendored CRD with an awk step anchored on the controller-gen version line, which upstream owns. If upstream stops emitting that line the step matches nothing, writes the CRD out unstamped and exits zero, so the release quietly loses the protection that keeps a chart uninstall from deleting the CRDs and every EtcdCluster, EtcdMember and EtcdSnapshot under them. Count the annotations the step produced and require exactly one per CRD. Zero means the anchor is gone; more than one means it matched somewhere unintended. Stage every CRD in a temporary directory and replace templates/ only once all of them are counted, so a failure on any one leaves the vendored files exactly as they were instead of wiping the directory and refilling part of the set. Verified by substituting the upstream response: a CRD with no controller-gen line fails with "stamped 0 keep annotations, expected 1", one carrying the line twice fails with 2, a failure on the second CRD leaves all three committed files untouched, and the unmodified upstream still vendors byte-identical output. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
0827bcb300 |
fix(apps/kubernetes): report the pre-delete cleanup outcome honestly
The pre-delete cleanup Job ended with an unconditional "Cleanup completed successfully". Several of its steps swallow a failure behind `|| echo "WARNING: ..."` so that `set -e` cannot end the run, so that line was printed in exactly the situation the warnings exist to report: child HelmReleases still holding finalizers, or a Kamaji datastore Secret still pinning its namespace in Terminating. Record what a step leaves behind and close with either the success line or the list of steps that left something. Residue is the thing worth recording, not every fallback: a bounded delete that times out has a remedy right behind it -- Step 2 falls through to force-clearing finalizers, Step 4 falls through to Step 4b -- and whether anything survived is decided by that remedy, which reports its own outcome. A tenant with no working nodes takes that route as a matter of course, which is why the wait is bounded in the first place, and calling such a run incomplete would send an operator hunting for objects that are not there. A read that fails looks exactly like one that found nothing: an empty list and a failed list both give a loop nothing to iterate, and an absent object and an unreachable apiserver both make a probe non-zero. Every read the summary rests on now keeps its status. Step 2's force-clear re-list reports when it cannot list what it was about to clear, because everything it could not return is still holding its finalizer. Step 4b's two probes became three-way -- present, absent or unknown -- so a Secret whose existence could not be established is no longer announced as absent. Step 1's list is the one read still swallowing its status, and that is a decision rather than an omission: Step 2 deletes by label rather than from that list, so a suspend that never happened leaves nothing of its own behind and reporting it would name residue that does not exist. Making the TenantControlPlane probe three-way also repairs the gate above it, and that is intended rather than incidental. The gate exists so the datastore Secret's finalizer is not stripped while Kamaji may still own it, and a two-way probe defeated it in the one direction that does damage: any error reads as "confirmed absent" and the strip goes ahead. Its likeliest cause, a throttled or briefly unreachable apiserver, is also likeliest exactly when a broken tenant is being torn down. Step 4b declining to strip a finalizer while the TenantControlPlane is still alive is the one residue that no failing command marks: every call in that branch succeeds and the namespace stays in Terminating regardless. It now reports, and names the TenantControlPlane as the thing holding the namespace up. The exit code is deliberately unchanged and still zero. These are backstops, not the main path: they run after the normal deletion has already timed out, and they exist to push teardown further along, so failing there would abort the Helm uninstall at the very moment the backstop was written for, leaving the release in storage and the same teardown retried forever. That makes the closing message the only outcome report an operator gets, which is why it has to be accurate. The steps that are not backstopped still end the run under `set -e` and never reach that message. Rework the runtime warning and the RBAC comment that pointed at the tracking issue for the Kamaji datastore-secret leak. That issue is closed, and closed by this very step rather than by a fix in Kamaji, so a reader who followed the pointer found a resolved report and no reason for the code to exist. The step comment now says the ordering it covers is still reachable, and the warning names the finalizer and the object to clear by hand. Cover the behaviour with a bats file that renders the Job, extracts the script and runs it against a stub kubectl, since the helm-unittest cases match the script's source text and cannot tell whether the closing message is reachable. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b4e203164a |
fix(kubernetes): retry the tenant CSI install instead of uninstalling it (#3581)
## What this PR does The tenant CSI HelmRelease carried `remediation.retries: -1` with no strategy, so Flux applied its default, `RemediateOnFailure`. Install remediation is an uninstall in every case the API offers, so an install that missed its budget was torn down between attempts: the tenant lost its CSI node plugin and the StorageClasses this release propagates until a later attempt applied them again, and that attempt restarted the DaemonSet rollout instead of resuming one already under way. That teardown races the rollout it exists to recover, and the run recorded in #3576 shows how narrowly. Both DaemonSet pods were scheduled at the same second on both workers, so nothing was waiting on capacity; the ten minutes went to image pulls. The slower worker started its last container six seconds after the install timeout fired, and the uninstall removed it one second after that. `RetryOnFailure` leaves the manifests applied and retries the failed release. Both actions carry it because the controller picks the active retry from `.status.lastAttemptedReleaseAction` and performs that retry as an upgrade: set on install alone, the retry still happens, but its own failure is then handled by upgrade remediation, whose default is a rollback with no previous release to roll back to. The `remediation` blocks stay, because an absent release consults `install.remediation` and an out-of-sync one consults `upgrade`, both by retry count rather than by strategy. This is the change #3552 makes for the tenant CNI, and the reason for putting the strategy on both actions is the one given there. It is also the shape upstream prescribes: the "Recommended settings" block in the [HelmRelease spec shipped with the helm-controller this installs](https://github.com/fluxcd/helm-controller/blob/v1.5.1/docs/spec/v2/helmreleases.md#recommended-settings) puts `strategy.name: RetryOnFailure` under both `install` and `upgrade` for production deployments. Nothing changes on the test side, and that is deliberate rather than deferred. Because no manifest is removed at the attempt boundary, the rollout continues across it and the kubelet finishes what it started, while the retry re-applies the same manifests every 30s. The release therefore reports Ready once the rollout completes rather than once a whole attempt has fit inside the timeout, so what the existing e2e wait has to outlast is the rollout and no longer an attempt boundary that discards it. That widens what the wait tolerates without making it unbounded: a rollout slower than the wait itself still fails, and this does not claim otherwise. Scope. #3576 records more than this fixes, so this does not close it. The other tenant addon HelmReleases this chart renders keep the remediation-only shape; CSI is the one this changes. ### 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: The trigger map was walked against the diff. The change is a HelmRelease template and a chart test, not a `values.schema.json` field, a `values.yaml` default, a version enum, a `release.prefix`, an `ApplicationDefinition`, a chart source kind or a namespace. ### Release note ```release-note fix(kubernetes): the tenant CSI HelmRelease now retries a failed install instead of being uninstalled between attempts, so a failed install no longer takes the tenant's CSI node plugin and its propagated StorageClasses away while it retries. The same strategy applies to the upgrade action, which it has to: a retried install runs as an upgrade. One consequence is operator-visible — a failed CSI upgrade in a tenant cluster now retries the new manifests every 30s instead of rolling back to the previous release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved reliability when installing or upgrading the CSI component by automatically retrying failed actions every 30 seconds. * Retains unlimited remediation retries to help recover from transient failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
879d0f6db6 |
fix(installer): make operator readiness honest in the chart and the install gate (#3466)
## What this PR does
The operator Deployment ran `replicas: 1` with `maxUnavailable: 1`, so
the Deployment controller took the minimum available replica count as `1
- 1 = 0` and the `Available` condition reported `True` with no running
pod at all. Measured against an operator whose image tag does not exist:
the condition was `True` with `reason: MinimumReplicasAvailable` while
the only pod sat in `ImagePullBackOff`, `unavailableReplicas: 1`, and no
`availableReplicas` key in the status.
That broke two separate things, so this fixes both.
The E2E install gate asked `kubectl wait --for=condition=Available` and
passed in 156 ms against that dead operator. The install then carried on
and died about two minutes later on the `packages.cozystack.io` CRD wait
further down the file, which sends you after CRD installation when the
operator never started. It now uses `kubectl rollout status`, which
reads updated and available replicas directly, and dumps the deployment,
pods, operator logs and namespace events when it fails. This half is
deliberately independent of the chart: `rollout status` is correct under
any rollout strategy, so a later change to that strategy cannot quietly
make the gate vacuous again.
The chart is the other half, and fixing the gate does not cover it.
`Available` is read by more than this test, and until the strategy
changes an alert on that condition, a health dashboard, or a person
reading `kubectl get deploy` all show green while the operator is dead.
The strategy moves to `maxSurge: 1` / `maxUnavailable: 0`, which
restores the ordinary meaning of the condition. Kubernetes refuses
`maxSurge: 0` and `maxUnavailable: 0` together, so the surge has to move
for the unavailability to reach zero.
That pair is worth reading twice, because it is not a configuration I
picked. At `replicas: 1` the Kubernetes default of `25%` / `25%`
resolves to exactly `maxSurge: 1` and `maxUnavailable: 0`, since the
surge rounds up and the unavailability rounds down. What this PR does is
stop overriding the default, not introduce a tuning of its own, and the
risk of the change should be judged on that basis.
Moving the surge is safe here, and I checked rather than assumed,
because the pair came from somewhere. It was carried over from the
earlier `cozystack` Deployment, where a sidecar published host port 8123
and `hostNetwork` left the surge pod unschedulable on a single-node
cluster. It got here in two steps, which is worth knowing if you go
looking for the reason: by the commit before this file existed the
sidecar was already gone and no port was declared, yet the pair stayed.
It had outlived its own reason before it was ever copied, which is why
there is no justification to find in this file's own history. On a
single-node cluster the surge pod is scheduled next to the outgoing one
and the rollout completes; adding a declared container port reproduces
the original `didn't have free ports for the requested pod ports`
failure exactly. Nothing else in the pod holds a port either: metrics
and health probes are bound to `0`, and the configured webhook server
never starts because the operator registers no webhook and never asks
the manager for the server.
The surge makes leader election load-bearing, though it covers less of
the overlap than it first appears. While `maxSurge: 0` capped total pods
at `replicas` a second manager process could not exist, so
`--leader-elect=true` was defence in depth. With a surge pod the lease
is what keeps the incoming manager's reconcile loops off the cluster
while the outgoing one is still leader, and because there is no
readiness probe the incoming pod counts as available as soon as its
container starts, so that overlap covers the whole image pull. It does
not cover everything: the operator installs CRDs, installs Flux and
creates the platform PackageSource before `mgr.Start()`, and the lease
is only taken inside `mgr.Start()`, so those three privileged installs
run outside leader election entirely. They are safe because both pods
write the same objects through server-side apply under the same field
manager, which is a different guarantee from the lease. The
`k8s-await-election` wrapper this operator replaced did hold the whole
process until it won leadership, so if that property sounds familiar it
belongs to the predecessor rather than to this binary.
The sibling `opensearch-operator` chart turns leader election off at one
replica for a real reason: a transient lease-renewal blip makes
controller-runtime exit the process, which at one replica reads as a
crashlooping Deployment. This operator installs first, when the
apiserver is at its shakiest, so that scenario applies here too. I am
accepting the risk knowingly, because after `maxSurge: 1` the
alternative is two managers reconciling the same cluster, which is
worse. The flag is pinned in the unit test so it cannot be turned off
without the test going red.
One consequence is permanent, so I am writing it down now instead of
leaving it to be found in a year. Because any bound port breaks the
surge pod under `hostNetwork`, an `httpGet` readiness probe is off the
table for this Deployment for good and only an `exec` probe could ever
work. So `Available` still answers "a container started", not "the
operator is healthy": an operator that starts and then dies partway
through installing CRDs is caught by the CRD waits below the gate, not
by the gate itself.
The surge also opens one new window. The operator installs CRDs and Flux
before the manager starts, which puts that work outside leader election,
so the incoming pod runs those applies while the outgoing pod is still
leader. Both apply the same objects server-side and the outgoing pod
exits moments later.
`--wait` is dropped from the helm step in the same change. Helm decides
a Deployment is ready with the same `replicas - maxUnavailable`
arithmetic, so on the old strategy it was satisfied at zero ready pods
and gated nothing, which is why the broken install got past it as well.
Once the strategy is honest it would gate for real, but on its own 2m
budget, and it would fail the install before the rollout gate ran and
hide the diagnostics behind a bare Helm timeout. Hooks are waited for
with or without the flag, and the release otherwise carries only the
Deployment and its RBAC objects, so the rollout gate is the better
single place to decide the operator is up.
A helm unittest suite pins the strategy and all four preconditions the
surge makes load-bearing: no container or initContainer declares a port,
both listener arguments stay disabled, leader election stays on, and no
probe uses `httpGet`. That last one follows from the two disabled
listeners leaving no port for a probe to reach, and it is the worst of
the four to get wrong, because the others wedge an upgrade while a
`livenessProbe` against a disabled listener crashloops the pod on every
cluster, fresh install included. Only the `httpGet` field is forbidden
rather than the probes themselves, so an `exec` probe stays available.
All four run against all three chart variants, because the template
branches on variant inside the container spec and a port added under one
branch would render for that variant alone.
I checked the suite fails for the right reasons instead of trusting it.
Reverting the strategy, declaring a `containerPort` on the operator
container, declaring one under a single variant branch, declaring one on
an initContainer, pointing a bind-address at a real address, turning
leader election off, and adding a readiness, liveness or startup probe
with `httpGet` each turns it red on its own assert. Inserting a
container ahead of the operator trips the container-name pin before the
argument asserts can start describing the wrong container. An `exec`
probe correctly stays green.
The rationale is split by audience. What an operator applying the
rendered manifest has to respect stays in plain comments and travels
into `_out/assets/cozystack-operator-*.yaml`: why the strategy is what
it is, that no port may be declared or bound, and that leader election
has to stay on. Everything that is only about this repository moves into
a `{{/* */}}` template comment that stops at the chart boundary. That
covers the pointer to the test and which assert pins what, where the
strategy was copied from, why the webhook server on 9443 is not a third
port binder, and why `opensearch-operator` reaches the opposite
conclusion on leader election. The other templates in this chart already
keep maintainer notes that way.
### Screenshots
Not applicable, no UI changes.
### Downstream repositories
I left the boxes empty on purpose rather than to skip the question,
because the honest answer is not one of them and someone else should
make the call.
Walking the trigger map in `docs/agents/contributing.md`, nothing fires.
The `hack/` triggers name `hack/e2e-prepare-cluster.bats`,
`hack/package.mk`, `hack/update-crd.sh` and moves or renames under
`hack/`, and this PR only edits the body of
`hack/e2e-install-cozystack.bats`. The `ansible-cozystack` trigger is
renaming an object the role waits on, and
`deployment/cozystack-operator` keeps its name.
The behaviour that role sees does change, though. It runs `kubectl wait
deployment/cozystack-operator --for=condition=Available`, the same
vacuous check this PR replaces here, so the chart change turns its wait
from one that returns instantly into one that means something. Its
`cozystack_operator_wait_timeout` defaults to 300 seconds, the same
budget I gave the gate here, so a cold image pull fits and I do not
think anything needs to change over there. No follow-up PR seemed
warranted for a change that only makes an existing check work, but that
is a judgement call rather than a trigger, which is why the boxes are
empty.
- [ ] 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:
### Release note
```release-note
fix(installer): the cozystack-operator Deployment no longer reports Available with zero running pods. It ran replicas: 1 with maxUnavailable: 1, which made the minimum available replica count 0, so the condition stayed True while the operator was dead and anything reading it, including the E2E install gate, saw green. The rollout strategy is now maxSurge: 1 / maxUnavailable: 0 and the install gate waits for the rollout instead of the condition.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved operator installation checks by monitoring deployment
rollouts and providing deployment, pod, log, and event details when a
rollout fails.
* Updated operator upgrades to maintain availability during rollout,
including in single-node environments.
* Documented rollout requirements and behavior across supported
installation configurations.
* **Tests**
* Added coverage for rollout settings and upgrade compatibility across
supported installation variants.
* Clarified Helm unit-test directory coverage documentation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
a4a8e4143f |
fix(kubernetes): retry the tenant CNI install instead of uninstalling it (#3552)
## What this PR does The tenant Cilium HelmRelease carried no install strategy, so Flux applied its default, `RemediateOnFailure`, which uninstalls the release between attempts. Cilium is the tenant cluster's only CNI, so every cycle takes it away and gives it back when the next install applies the manifests again, and `retries: -1` lets that repeat without end. What sets a cycle off is an install that outruns its budget, which can happen while the tenant still has no registered node, since the operator Deployment cannot become Available until one exists. Remediation neither causes that delay nor is fixed by this change: what it adds is a repeated teardown of the CNI on top of a delay that resolves itself once the node arrives. This removes the amplifier, not the cause. `RetryOnFailure` keeps the applied manifests in place and retries a failed install as an upgrade, so the agent DaemonSet rolls out as soon as a worker registers. Both actions carry the strategy because helm-controller selects the active retry from `.status.lastAttemptedReleaseAction`, and the retry of a failed install runs as an upgrade. With the strategy on `install` alone that retry still happens, but once it is the last attempted action its own failure is handled by upgrade remediation, whose default is a rollback with no previous release to roll back to. The `remediation` blocks stay because two branches consult the retry count rather than the strategy and refuse to act once it is exhausted: an absent release reads the install block, an out-of-sync one reads upgrade. `retries: -1` keeps both open. The cost is that a genuine upgrade failure no longer rolls back either, it retries on the failed manifests instead. That is forced, since the controller cannot tell a retried install from a real upgrade, and what it replaces was an unbounded rollback-and-retry flap rather than a stable safety net: `upgrade.remediation.retries: -1` with the default rollback strategy already retried forever. This is the strategy and retry interval that `pkg/registry/apps/application/rest.go` and `internal/operator/package_reconciler.go` already set on every HelmRelease they generate, except that they leave `remediation` nil where this release keeps it. The other tenant addons this chart renders still carry the remediation-only shape; this PR changes the CNI alone, where the teardown removes the cluster's only CNI rather than churning a single component. ### 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: ### Release note ```release-note fix(kubernetes): the tenant Cilium HelmRelease now retries a failed install instead of being uninstalled between attempts, so a failed install no longer takes the tenant cluster's only CNI away while it retries. The same strategy applies to the upgrade action, which it has to: a retried install runs as an upgrade. One consequence is operator-visible, a failed Cilium upgrade in a tenant cluster now retries the new manifests every 30s instead of rolling back to the previous release. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Cilium installation and upgrade reliability by automatically retrying failed operations every 30 seconds. * Preserved deployed resources during retries and continued supporting unlimited remediation attempts. * **Tests** * Added coverage validating installation and upgrade retry behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
0c4c11c845 |
fix(kubernetes): retry the tenant CSI install instead of uninstalling it
Flux defaults a HelmRelease to the RemediateOnFailure strategy, whose install remediation is always an uninstall. A tenant CSI install that misses its timeout was therefore torn down between attempts, taking the CSI node plugin and the StorageClasses this release propagates with it, and the next attempt restarted the DaemonSet rollout from nothing instead of resuming one already under way. That teardown races the rollout it is meant to recover. In one observed run both DaemonSet pods were scheduled immediately and spent the whole budget pulling images; the slower node started its last container six seconds after the install timeout fired, and the uninstall removed it one second later. Retrying in place keeps that work. RetryOnFailure leaves the manifests applied and retries the failed release. Both actions carry it because the controller picks the active retry by .status.lastAttemptedReleaseAction and runs that retry as an upgrade: set on install alone, the retry happens but its own failure falls through to upgrade remediation, which rolls back by default with no previous release to roll back to. The cost is that a real upgrade failure now retries rather than rolls back, and that gives up no stable safety net: with retries: -1 and the default rollback strategy, upgrade remediation already rolled back and retried without end. The remediation blocks stay for a separate reason. An absent release consults install.remediation and an out-of-sync one consults upgrade, both by retry count rather than by strategy, so retries: -1 is what keeps those two branches acting. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
115d9bbaaf |
fix(installer): let the operator Deployment report Available honestly
At replicas: 1 with maxUnavailable: 1 the Deployment controller takes the minimum available replica count as 1 - 1 = 0, so the Available condition reports True with no running pod at all. That reading is what every consumer of the status sees: an alert on Available, a health dashboard, or a person reading kubectl get deploy all show green while the operator is dead. The replacement is not a tuned pair, it is the platform default written down. At replicas: 1 the default 25% / 25% resolves to maxSurge 1 and maxUnavailable 0, because the surge rounds up and the unavailability rounds down. So this change stops overriding the default rather than introducing a configuration of its own, and Kubernetes rejects maxUnavailable: 0 together with maxSurge: 0, so the surge could not have gone the other way regardless. The overridden pair was carried over from the earlier cozystack Deployment, where a darkhttpd sidecar published a host port and hostNetwork left a surge pod unschedulable on a single-node cluster. It arrived here in two steps: by the commit before this file was created the sidecar was already gone and no port was declared, yet the pair stayed, so it had outlived its own reason before it was ever copied. That is why looking for a justification in this file's history finds none. With no port declared anywhere in the pod there is no hostNetwork port conflict left, so the surge pod schedules beside the outgoing one; declaring one reintroduces exactly the scheduling failure the pair was added for. Not exercised on a live cluster in this rework. Stay on RollingUpdate rather than move to Recreate, which would fix the condition too, and by a shorter route: MaxUnavailable() reports 0 for any strategy that is not RollingUpdate, so the minimum available replica count becomes replicas - 0 = 1 and the condition is honest without any surge pod at all. The transition was not attempted, and the failure this project fixed in ouroboros 0.8.1 does not transfer to it. There the rollingUpdate block was the one the apiserver defaults onto a Deployment created without one, so it belonged to no applier and outlived the switch to Recreate. This Deployment has carried an explicit block since the commit that created it, and nothing applies it server-side, so whether Recreate would upgrade cleanly is an open question about the field-ownership history of the live object rather than a known rejection. What keeps RollingUpdate here is availability rather than apply semantics: maxUnavailable: 0 leaves the outgoing operator running when the replacement never starts, where Recreate deletes it first and leaves none. Editing the numbers inside the block is upgrade-safe from every state the field can already be in. The surge introduces new windows, and leader election covers less of them than it looks. The operator runs four privileged install steps before mgr.Start(), and the lease is only acquired inside mgr.Start(): the CRD install, the Flux install, the platform source resource and the platform PackageSource. All four therefore run in the incoming pod outside leader election entirely. What the lease does cover is the reconcile loops, the part that would otherwise conflict continuously instead of once at startup. That leaves three seams, and only the first two happen on a healthy upgrade. The routine one is the incoming pod's install steps against the outgoing pod's live reconcile loops, since the outgoing pod ran its own installs at its startup and has been inside mgr.Start() ever since. Those do not contend for the same fields: the installs write as field manager cozystack-operator, while the reconcilers write as cozystack-packagesource-controller and cozystack-package-controller and go mostly to status subresources. Different owners, different objects, so this seam is safe by separation rather than by any shared manager. The second is version skew rather than field contention, and maxSurge: 1 widens it rather than opening it. Under maxSurge: 0 the surge budget counts desired replicas rather than live pods, so the incoming pod is created while the outgoing one may still be Terminating - the overlap existed. What it did not have was a live counterparty: scale-down sends SIGTERM before the new pod is created, and that cancels the manager context, so the old loops were already unwinding. Here the outgoing pod is not signalled until the incoming one is available, so the overlap runs against a fully live reconciler for a termination grace period instead of a vestigial one. The incoming pod reports Ready the instant its container starts, because there is no readiness probe, so its CRD install runs while the outgoing binary's reconcile loops are still live and now writing against a schema the incoming pod has already moved. Old code against a new schema, which no field manager separates. It is bounded rather than benign: the outgoing pod exits on SIGTERM within seconds, a rejected write is transient and the next reconcile repairs it, and no object is left corrupt. Widening the skew, by adding a probe that delays Ready or by a migration the old code cannot round-trip, is what would make this seam matter. The third seam is install against install, and it needs the outgoing pod to be restarting, since a crashlooping pod re-runs its four steps on every restart, which is exactly the broken operator this gate exists to catch. Three of the four steps use server-side apply under the same field manager, so they never conflict and resolve last-writer-wins whenever the two pods carry different manifests. That ordering matters in one direction, and it is accepted here rather than guarded against: the CRD install and the Flux install both apply with Force: true, so an outgoing pod that writes last silently overwrites what the incoming pod has just applied, with no conflict and no error to exit on. The incoming pod is inside mgr.Start() by then and will not apply again until it restarts, so that revert of the upgrade's install phase persists rather than healing itself. Reaching it needs a restart landing in the seconds between the incoming pod's installs and the outgoing pod's SIGTERM. installPlatformSourceResource is the remaining exception, a read-modify-write of a Get followed by Create or by Update with the observed resourceVersion, so two pods running it at once can collide on AlreadyExists or on a conflict; that failure is loud and self-healing, since any error there exits the process and the pod restarts, and the object is not left corrupt. Pin the strategy and all four preconditions the surge makes load-bearing in a unit test: no container or initContainer declares a port, both listener arguments stay disabled, leader election stays on, and no probe uses httpGet. They fail differently and none is visible to the others, since a declared port stops the scheduler, a bound one only fails once the incoming process starts, and a second manager without a lease fails nothing at all until it writes. The httpGet precondition follows from the two disabled listeners leaving no port to target, and it is the worst of the four to get wrong: the other three wedge an upgrade, while a livenessProbe against a disabled listener crashloops the pod on every cluster, including a fresh install where no surge happens. Only the httpGet field is forbidden rather than the probes themselves, because an exec probe stays legitimate. All four are checked against all three variants, because the template branches on variant inside the container spec and a port added under one branch renders for that variant alone. The strategy type is pinned alongside its two values, since maxSurge and maxUnavailable say nothing on their own once the type is no longer RollingUpdate. The listener preconditions are pinned less tightly than the other two, and the test says so rather than implying coverage it does not have. A membership assert has no notion of position while Go's flag package keeps the last occurrence, so a duplicate after the disabled one wins and the assert still passes, while a duplicate before it loses and the assert passes just the same - the test cannot tell those two apart. Moving the disabled flag last fixes the process without making the assert catch anything. Rejecting the two addresses this binary defaults to closes the shape a values-gated block to enable metrics would take, and nothing available here closes an arbitrary address, since helm-unittest cannot express that a flag appears at most once. Note in hack/helm-unit-tests.sh that the loop covers packages/core, which its comment omitted. The suite added here lives there, so a later edit reconciling the loop with the stale comment would have silently stopped running it. Split the rationale by audience rather than keeping it all in shipped comments. What an operator applying the rendered manifest has to respect stays in plain comments and travels into the install artifacts: that the pair is the default, why maxUnavailable must stay 0, that no port may be declared or bound, that no probe may use httpGet, and that leader election has to stay on. Most of what stays in the template comment is what the chart's own suite pins, so it cannot drift out of step with the chart without a test going red: the four preconditions and which assert covers each, and why the variant loop is needed. Three things there are deliberate exceptions, kept because anyone editing the strategy has to know them and no assert can carry them - that the webhook server configured on 9443 is not a port binder, since controller-runtime adds it to the runnables only inside GetWebhookServer() and this binary never calls it; that two overlaps between the pods sit outside the lease; and why RollingUpdate is kept rather than Recreate. Those name what exists. The seam analysis above, which is how each one works, deliberately stays here instead. It describes another file's internals, nothing pins it, and a commit message is fixed in time, so it cannot quietly fall out of date the way a comment can. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
cd57ee6678 |
test(multus): anchor the master-CNI assertion to the top-level key
The check that multus is pointed at the cilium conflist matched its JSON fragment anywhere in the rendered daemon config. The same text nested inside any other key satisfies it, so a config whose effective multusMasterCNI names a different conflist passes. Multus uses that value as written when it is non-empty -- it delegates to whatever is named, so a wrong-but-existing name silently routes through the other CNI instead of cilium, which is the state this assertion exists to prevent. Auto-discovery happens only when the value is empty, and a name that does not exist fails manager startup rather than falling back. Anchor it to the top level, where the template writes its keys. That pins the position rather than parsing the document; deciding which key is effective needs a JSON parser, which helm-unittest does not have. Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
7739d1e294 |
feat(multus)!: stage reference CNI plugins from the multus image
Multus-backed NetworkAttachmentDefinitions delegate to the upstream containernetworking reference plugins (bridge, macvlan, ipvlan, ...), and nothing guarantees a node has them in the directory Multus searches. On k3s they live under /var/lib/rancher/k3s/data/cni while Multus and Cilium install into /opt/cni/bin, so a bridge-type NAD fails there with `failed to find plugin "bridge" in path [/opt/cni/bin]`. Bundle 14 of the release's plugins into the multus image and add an install-cni-plugins init container that installs them into the host /opt/cni/bin, mirroring the existing install-multus-binary container. loopback, dhcp, tap and dummy are left out; the Dockerfile records why. Gate it on a new chart value, stageCniPlugins, resolved per bundle and on for both. Talos bundles only bridge, firewall, flannel, host-local, loopback and portmap, so the other ten are missing there exactly as on generic Linux; the four that overlap are rewritten with the same upstream version, v1.9.1 on both sides, rather than an older one. A marker in the image Dockerfile records the Talos release that pin was compared against, and a test holds it to the release the platform ships. multus therefore moves out of system.common-packages into the two variant branches, the way linstor already does for talos.enabled. networking.stageCniPlugins overrides the variant default in either direction. Where staging is on the install is unconditional: a plugin is replaced on every pod recreation, not only when the destination is missing. A destination that is already a directory is reported and left alone, since replacing it is not something this can do safely. Turning the value off afterwards does not undo it; nothing here removes or restores a plugin. The container is not privileged and takes no mount propagation. It runs in the spc_t SELinux domain, without which an enforcing node refuses the write. Each plugin is installed under a temporary name and swapped in with a rename: /opt/cni/bin is shared with cilium and kube-ovn. The plugin tarball is checksummed before extraction. A plugin that cannot be installed is reported on stderr and to /dev/termination-log, and the container still exits 0: Multus stays the node's primary CNI and nothing removes its config, so an init container that never completes would leave the shim with no daemon behind it. The manifest change lives in customize-deployment.patch so it survives make update. BREAKING CHANGE: enabling stageCniPlugins replaces reference CNI plugins in /opt/cni/bin on existing nodes, Talos included -- four of the names it writes are ones the Talos node image also carries. Opt out before upgrading: declining afterwards does not restore what was replaced. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
80a2090444 |
fix(kubernetes): retry the tenant CNI install instead of uninstalling it
The tenant cilium HelmRelease carried no install strategy, so Flux applied its default, RemediateOnFailure, which uninstalls the release between attempts. Cilium is the tenant cluster's only CNI, so every cycle takes it away and gives it back when the next install applies the manifests again, and retries: -1 lets that repeat without end. What sets a cycle off is an install that outruns its budget, which can happen while the tenant still has no registered node, since the operator Deployment cannot become Available until one exists. Remediation neither causes that delay nor is fixed by this change; what it adds is a repeated teardown of the CNI on top of a delay that resolves itself once the node arrives. This removes the amplifier, not the cause. RetryOnFailure keeps the applied manifests in place and retries a failed install as an upgrade, so the rollout completes on its own once a worker registers. Both actions carry the strategy because the retry of a failed install runs as an upgrade, which would otherwise fall back to upgrade remediation. It is the strategy and retry interval cozystack-api and cozystack-operator already set on every HelmRelease they generate, except that they leave remediation nil where this release keeps it. The other tenant addons the chart renders still carry the remediation-only shape; this one is where the teardown removes the cluster's only CNI rather than churning a single component. The cost is that a genuine upgrade failure no longer rolls back either, it retries on the failed manifests instead. That is forced, since the controller cannot tell a retried install from a real upgrade, and what it replaces was an unbounded rollback-and-retry flap rather than a stable safety net. The remediation blocks stay, since they cover the branches no strategy reaches: an absent release, which consults the install block, and an out-of-sync one, which consults upgrade. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
ee26a50f0e |
fix(postgres-operator): align CNPG operator and CRDs to 1.28.2 for PVC resize-deadlock fix (#3510)
## What this PR does Raises the CloudNativePG operator image **and its CRDs together** to 1.28.2, which carries the PVC resize-deadlock fix (cloudnative-pg#9980 / cloudnative-pg#9981). Symptom: after a simultaneous `resources` + `size` change on a single-instance Postgres, the operator deletes the sole primary Pod, classifies the PVC as `resizing`, and never recreates the Pod — the cluster wedges with zero instances and the disk FS resize never completes. Upstream publishes a CloudNativePG chart only per minor (`.0`/`.1`), so there is no 1.28.2 chart on the 1.28 line. Following the pattern of #3526 / #3528, `patches/cloudnative-pg-1.28.2.patch` (applied by `make update`) raises the vendored chart's `appVersion` and CRDs from 1.28.1 to 1.28.2 in lockstep. The operator image follows `appVersion`, so there is no `image.tag` pin (keeping operator and CRDs aligned and avoiding #3479). Verified #3479-safe: there are no CRD `status` field changes between 1.28.1 and 1.28.2 — only the `extensions` spec grows. Backportable to release-1.6 as-is (same 0.27.1 base chart). ### Release note ```release-note fix(postgres-operator): align CloudNativePG operator and CRDs to 1.28.2, fixing the PVC resize-deadlock that wedged single-instance PostgreSQL clusters after a simultaneous resources+size change ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for PostgreSQL extension configuration in catalog and cluster schemas. - Made extension image settings optional where appropriate. - **Bug Fixes** - Updated CloudNativePG to version 1.28.2, including the PVC resize deadlock fix. - Kept the operator image and CRDs aligned at the same version. - **Documentation** - Clarified versioning and chart update guidance. - **Tests** - Added checks confirming operator image and CRD version alignment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
bcb36262ef |
fix(postgres-operator): align CNPG operator and CRDs to 1.28.2 for PVC resize-deadlock fix
Raises the CloudNativePG operator image and its CRDs together to 1.28.2, which carries the PVC resize-deadlock fix (cloudnative-pg#9980 / #9981): after a simultaneous resources+size change the operator deletes the sole primary Pod, leaves the PVC in the resizing class, and never recreates the Pod, wedging the cluster. Upstream publishes a chart only per minor .0/.1, so there is no 1.28.2 chart on the 1.28 line. Following #3526/#3528, patches/cloudnative-pg-1.28.2.patch (applied by make update) raises the vendored chart's appVersion and CRDs from 1.28.1 to 1.28.2 in lockstep; the operator image follows appVersion, so no image.tag pin. Verified #3479-safe: no CRD status field changes between 1.28.1 and 1.28.2 (only the extensions spec grows). Backportable to release-1.6. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
myasnikovdaniil
|
a7149d11a3 |
chore(etcd-operator): bump etcd-operator to v0.5.4 (#3529)
## What this PR does Bumps the cozystack etcd-operator packages from **v0.5.3 to v0.5.4**. `v0.5.4` is a controller bug-fix release — no API, RBAC or values changes: - fix(controllers): derive `--initial-cluster-state` from phase, not from the seed ([cozystack/etcd-operator#355](https://github.com/cozystack/etcd-operator/pull/355)) - fix(controllers): stop exempting the bootstrap seed from self-heal ([cozystack/etcd-operator#354](https://github.com/cozystack/etcd-operator/pull/354)) - fix(controllers): extend crash-loop self-heal to memory members ([cozystack/etcd-operator#352](https://github.com/cozystack/etcd-operator/pull/352)) - fix(controllers): switch the PDB from `maxUnavailable` to `minAvailable` ([cozystack/etcd-operator#351](https://github.com/cozystack/etcd-operator/pull/351)) Changes in this repo: - `packages/system/etcd-operator/Chart.yaml` — `appVersion: v0.5.3 → v0.5.4`. The manager image tag defaults to `.Chart.AppVersion` (`values.yaml` keeps `tag: ""`), so this reimages the controller to `ghcr.io/cozystack/etcd-operator:v0.5.4`. - `packages/system/etcd-operator/Makefile`, `packages/system/etcd-operator-crds/Makefile` — `ETCD_OPERATOR_REF: v0.5.3 → v0.5.4`. - `packages/system/etcd-operator-crds/templates/etcdmembers.yaml` — re-vendored at v0.5.4 via `make update`. Description-only change to the `/scale` `replicas`/`selector` field docs, tracking the PDB `minAvailable` fix. `etcdclusters` and `etcdsnapshots` are byte-identical to v0.5.3. - `templates/rbac.yaml` intentionally left as-is: `manager-role-rules.yaml` is byte-identical between v0.5.3 and v0.5.4. **Upgrade path (PDB switch, cozystack/etcd-operator#351).** This is the one change that rewrites live objects: the operator moves each EtcdCluster's PodDisruptionBudget from `maxUnavailable` to `minAvailable`. Setting both fields is invalid, but upstream `reconcilePDB` handles the migration — it treats a surviving pre-migration `maxUnavailable` as divergence and explicitly clears it (`MaxUnavailable = nil`) before writing `MinAvailable`, so existing clusters are reconciled cleanly on upgrade rather than wedging their PDB. Verified locally: `helm lint` and `helm template` pass for both packages; rendered manager image resolves to `ghcr.io/cozystack/etcd-operator:v0.5.4`. These `packages/system/*` packages have no `generate:` target and no `values.schema.json`, so there is nothing for `make generate` to regenerate. ### Screenshots Not applicable — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file-by-file against the diff: - The diff touches only `packages/system/etcd-operator*` — no `packages/apps/**` or `packages/extra/**` add/rename/remove, no `packages/core/platform` or `installer` values, no Talos bump, no asset-name or dev-tooling change → **website / ansible-cozystack** not reached. - The CRD edit is description-only, inside etcd-operator's own `etcd-operator.cozystack.io` CRDs — not the provider's hand-typed `Package`/`Plan`/`RestoreJob` types, and no new managed app → **terraform-provider-cozystack** not reached. - No `hack/` change, no `packages/system/<name>-rd/cozyrds/**`, no `ApplicationDefinition` CRD / `chartRef.kind` enum change, no `cozyhr`/`package.mk` contract change, no telemetry-metric or proxy-label rename → **ccp / talm / cozyhr / cozy-proxy / telemetry-server / examples / external-apps-example** not reached. - [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: ### Release note ```release-note chore(etcd-operator): bump etcd-operator to v0.5.4 (controller bug-fixes: PDB switched to minAvailable, crash-loop self-heal extended to memory members, --initial-cluster-state derived from phase, seed no longer exempt from self-heal) ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Updates** - Updated the etcd operator to version v0.5.4. - Clarified scale-related resource descriptions, including replica counts, selectors, and disruption budget behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |