Fresh KRaft clusters no longer need to be alone in a namespace. Their node pools
are now release-scoped (`<release>-broker`, `<release>-controller`) instead of the
fixed `kafka`/`controller`, which is unique per namespace (KafkaNodePool object
names are namespace-unique) and lets any number of Kafka instances coexist —
every pod/PVC/Service is already prefixed by the unique cluster name.
The fixed `kafka` name is still required to adopt an existing ZooKeeper cluster's
`<cluster>-kafka-N` brokers and data in place during migration, and that name is
namespace-unique — so only one ZooKeeper->KRaft migration can run per namespace.
The broker pool name is resolved by kafka.brokerPoolName: "kafka" for a cluster
that already migrated or is a classic ZK cluster about to migrate, otherwise
"<release>-broker". A second migration into a namespace that already holds a
"kafka" pool owned by another cluster fails the render with a clear message;
fresh KRaft clusters are never affected.
This replaces the previous blanket "one Kafka per namespace" guard
(kafka.assertSingleInstance), which over-restricted the fresh case. The migration
hook and workload monitors follow the same naming; unit tests updated. Verified
on a live cluster: two fresh single-node KRaft clusters (mi-a, mi-b) installed
side by side in one namespace came up with distinct node pools, pods and PVCs.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Addresses the second review round on the ZK->KRaft migration.
Migration hook:
- Fully qualify the Strimzi resources as kafkas.kafka.strimzi.io /
kafkanodepools.kafka.strimzi.io. The bare plural "kafkas" also exists in this
chart's apps.cozystack.io group, so a bare name resolves by per-cluster
discovery priority and can hit the tenant kind — which the hook Role does not
grant, failing the pre-upgrade hook deterministically.
- Classify NotFound structurally via `kubectl get --ignore-not-found` instead of
substring-matching stderr, so an unrelated "... not found" error can no longer
be mistaken for a fresh install and permanently skip a live ZooKeeper cluster.
This also drops the jq dependency.
- Treat a KRaft-native / already-migrated CR (annotated strimzi.io/kraft=enabled)
as done, never driving it backwards into kraft=migration.
- Keep only `before-hook-creation` on the Job's hook-delete-policy so a failed
attempt's pod logs survive for a post-mortem until the next run.
- Route the kubectl image through cozy-lib.image (cluster registry mirror) and
pin it by digest (clastix/kubectl:v1.32@sha256:...), matching the other
cozystack hooks, instead of an unpinned docker.io tag.
Controller quorum:
- Fix the controller node-pool replica count at creation instead of deriving it
from kafka.replicas on every reconcile. Strimzi 0.45 cannot scale a controller
pool, so a day-2 edit of kafka.replicas (e.g. 1 -> 3) must not flip the quorum;
the helper now keeps the existing "controller" pool's replica count when it is
present.
Docs:
- Document that migration to KRaft is one-way: neither a version-field downgrade
nor a pre-KRaft chart rollback is supported once migrated (Strimzi has no
KRaft->ZooKeeper rollback after completion).
E2E:
- Run the ZK->KRaft chainsaw migration in its own namespace instead of the
shared tenant-test. It raw-applies Strimzi resources plus a namespace-wide
LimitRange and leaves deleteClaim:false PVCs, which polluted the sibling kafka
suite. Raise the seed readiness timeout for CI headroom. Validated against a
live cluster: migration adopts the broker PVC in place and reaches KRaft under
a 128Mi LimitRange, in isolation.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
The ZK->KRaft pre-upgrade hook created the adopting broker ("kafka") and
"controller" KafkaNodePools without a resources block. Those hook-created pools
govern the Kafka pods for the entire migration window — Helm applies the
resourced steady-state kafkanodepools.yaml only after the pre-upgrade hook
finishes. In a tenant namespace the LimitRange defaults every container to
128Mi, so the migration's controller (and rolled broker) JVMs OOM, never reach
KRaftPostMigration, and the hook times out — wedging the migration and thrashing
it on every reconcile.
Give both hook pools the same resources the steady-state pools get
(cozy-lib.resources.defaultingSanitize from kafka.resourcesPreset/resources).
This also avoids a redundant second broker roll (resource-less -> resourced)
after the hook completes.
Tests: a helm-unittest assertion that both migration pools render a resources
block; the Chainsaw ZK->KRaft suite now seeds under a 128Mi LimitRange (and
gives the seed cluster explicit resources), reproducing the tenant-quota
condition, and asserts the migration still reaches KRaft with the broker PVC
adopted. Validated green against a live cluster under the LimitRange.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Follow-up to the KRaft-migration review. The broker KafkaNodePool must be named
exactly "kafka" so Strimzi adopts an existing ZooKeeper cluster's
<cluster>-kafka-N brokers and data in place during migration, but KafkaNodePool
object names are unique within a namespace — so two Kafka clusters cannot both
own a "kafka" pool there. This is an upstream constraint and Strimzi's own
guidance is one Kafka cluster per namespace
(strimzi/strimzi-kafka-operator discussions/11120).
Adopt that model instead of the earlier fresh-vs-migrated conditional naming:
- New kafka.assertSingleInstance helper fails the render (with a clear message
and the upstream reference) if the namespace already holds another Kafka
cluster. Reconciles/upgrades of the same release are allowed; lookup returns
nothing during `helm template`, so unit tests never trip it.
- Node pools are now named "kafka" (broker) and "controller" unconditionally,
which also adopts existing brokers on migration and yields clean
<cluster>-kafka-N / <cluster>-controller-N names. The fresh-vs-migrated
brokerPoolName helper is removed.
- WorkloadMonitor pool-name selectors and the migration hook updated to match;
unit tests updated for the fixed pool names.
- README documents the one-Kafka-per-namespace rule and links the Strimzi
discussion.
The ZK->KRaft Chainsaw migration suite (seed ZooKeeper cluster, migrate, assert
the original broker PVC survives and no wrongly-named PVC is created, cluster
reaches KRaft) was run green against a live cluster.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Addresses the branch review on the ZK->KRaft migration path.
CRITICAL — broker node pool adoption. Strimzi derives broker/PVC names as
<cluster>-<pool>-<id>; a pre-node-pool ZooKeeper cluster has brokers
<cluster>-kafka-N, so the adopting broker pool must be named exactly "kafka"
or the operator provisions fresh empty brokers and orphans the tenant's data.
A new kafka.brokerPoolName helper resolves the broker pool to "kafka" for an
existing (migrating) cluster and keeps the release-scoped "<release>-broker"
for fresh installs, so multiple Kafka instances still coexist in one namespace.
The migration hook creates the pool as "kafka" and fails closed if a "kafka"
pool already belongs to another cluster (node pool names are namespace-unique,
so in-place migration of co-namespaced ZK clusters is unsupported). Wired
through kafkanodepools.yaml, workloadmonitor.yaml and the hook.
MAJOR — migration robustness.
- Raise the HelmRelease timeout: the migration Job can run tens of minutes but
the release timeout defaults to 10m, so helm-controller aborted and thrashed
the pre-upgrade hook. Add release.cozystack.io/helm-install-timeout=45m on the
kafka ApplicationDefinition, above the Job's activeDeadlineSeconds (2400s).
- Idempotent migration: do not re-issue kraft=migration once the cluster is past
the ZooKeeper phase (an invalid backward transition Strimzi rejects on retry).
- Fail closed on the decision gates: distinguish a genuine NotFound (skip) from
any other kubectl error (abort), and never treat an empty kafkaMetadataState on
an existing CR as "migrated".
MINOR — version downgrade documented as unsupported; integer (not lexical)
compare in the version gate; deleteClaim/PVC-retention and the one-per-namespace
migration limitation documented in the README; docs/storage-immutability.md
updated for KRaft.
Tests — helm-unittest coverage for the broker pool name, the hook guards and the
idempotent/fail-closed logic; a bats check for the helm-install-timeout budget
ordering; and a Chainsaw ZK->KRaft migration suite that seeds a ZooKeeper
cluster, drives the migration, and asserts the original broker PVC survives (not
recreated) and the cluster reaches KRaft. The migration sequence and the
Chainsaw assertions were validated against a live cluster.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Follow-up to the KRaft migration addressing the branch review.
- kafka-rd cozyrds: restore the two public CA certs in secrets.include
(clients-ca-cert + cluster-ca-cert). The rebase had collapsed them to
the bare clients-ca, which is Strimzi's clients-CA *private key* secret
— a private-key exposure to the tenant and a TLS regression (clients
need cluster-ca-cert to trust the broker listener). Covered by the
existing hack/check-kafka-rd-secrets.bats.
- migration-hook: guard the KRaft finalize step. The wait loop only exited
via break on success and fell through to unconditionally apply
strimzi.io/kraft=enabled on timeout — unsafe and irreversible if the
cluster is still in ZooKeeper/KRaftMigration. Add a migration_ready flag
and exit 1 when the safe state (KRaftPostMigration+) is not reached,
leaving the deployed-version ConfigMap unstamped so Helm retries. New
matchRegex assertions in migration-hook_test.yaml cover the guard.
- migration-hook: raise activeDeadlineSeconds 1800 -> 3600 so a
legitimately-progressing migration is not SIGKILLed before the in-script
waits and error paths can run.
- Extract the duplicated controller-quorum logic into a shared
kafka.controllerReplicas helper (used by kafkanodepools, workloadmonitor
and migration-hook) so the three stay in lockstep.
- Reflow the README migration section to one line per paragraph per the
repo prose rule.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Replace ZooKeeper-based Kafka with KRaft (Kafka Raft) metadata mode.
Strimzi 0.45 is the last line that still supports ZooKeeper, so this
prepares the transition before ZooKeeper support is removed upstream.
Chart changes:
- The Kafka CR always renders strimzi.io/kraft=enabled with node pools
and no spec.zookeeper block. Storage, replicas and resources move to
separate broker + controller KafkaNodePools (a homogeneous separate-pool
layout works for both fresh installs and migrations; Strimzi 0.45 does
not support migration into a combined pool).
- Kafka version is now user-configurable via the standard files/versions.yaml
pattern (v3.9 default, v3.8 available); appVersion bumped to 3.9.0.
- WorkloadMonitors track the broker and controller pools (no zookeeper
monitor); the dashboard RBAC grants the controller monitor and drops the
zookeeper one.
Migration of existing ZooKeeper clusters is handled by a Helm pre-upgrade
Job (templates/migration-hook.yaml) gated by a <release>-kafka-deployed-version
ConfigMap, following the established seaweedfs/etcd hook pattern. The Job
creates the broker + controller pools with Helm ownership labels, drives the
Strimzi ZK->KRaft state machine (node-pools=enabled, kraft=migration ->
kraft=enabled once KRaftPostMigration is reached) and is skipped once the
ConfigMap records version >= "1".
Testing:
- Helm unit tests cover the Kafka CR, node pools, migration hook, metrics,
topics, version map, dashboard RBAC and workload monitors.
- The e2e suite is ported to the current Chainsaw framework
(hack/e2e-chainsaw/kafka): it asserts the KRaft topology (both node pools,
kafkaMetadataState=KRaft), the absence of ZooKeeper, and bootstrap
endpoints.
Rebased onto current main: TLS listener support, entity-operator resource
guards, x-cozystack-options schema annotations and the WorkloadMonitor
replica-path fix are all preserved and reconciled with KRaft.
Signed-off-by: Matthieu <matthieu@hidora.com>
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
## What this PR does
The ClickHouse server and Keeper images were hardcoded to `24.9.2.42` in
the chart templates, so the chart exposed no top-level `version` values
key. The Cozystack API returns a managed database's engine version by
passing the HelmRelease's `spec.values.version` straight through into
the Application object, so ClickHouse instances carried no
`spec.version` in the API — unlike postgres, mariadb, mongodb,
opensearch, rabbitmq and redis, which all ship a `version` parameter.
This closes the ClickHouse half of the gap tracked in #1246.
This adds the same version-selection scaffolding the other
managed-database charts already use:
- `files/versions.yaml` maps a `major.minor` key to the full image tag,
shared by the ClickHouse server and Keeper images (both are pinned to
one version).
- `templates/_versions.tpl` (`clickhouse.versionMap`) resolves the tag
and fails the render on an unsupported version.
- `values.yaml` gains a `version` enum (`v25.8`, `v25.3`, `v24.9`),
consumed by `templates/clickhouse.yaml` and `templates/chkeeper.yaml`.
- `hack/update-versions.sh` + `make update` refresh the map from Docker
Hub, keeping only tags published for *both* the server and Keeper images
and preserving the current default so a regeneration never bumps the
major of existing deployments.
The default stays `v24.9` → `24.9.2.42`, so existing installations
render byte-for-byte identically and are never upgraded to a new major
implicitly. The generated `values.schema.json`, `README.md`, the Go API
type and the `clickhouse-rd` `openAPISchema` are regenerated with
`cozyvalues-gen` v1.6.0. A new `tests/version_test.yaml` helm-unittest
suite covers the server/Keeper mapping for each version and the
schema-level rejection of an unsupported value.
### Downstream repositories
The diff changes `packages/apps/clickhouse/values.schema.json` by adding
a new `version` field with an `enum`. Per the trigger map, this reaches
`terraform-provider-cozystack`, whose ClickHouse resource is
hand-written and would need a matching `version` attribute with a
`stringvalidator.OneOf` list and its expand/flatten pair. There is no
existing PR or issue there for it yet. This is left for a maintainer
decision rather than a speculative cross-repo PR, so the box below is
intentionally left unticked and flagged here. No other downstream
repository is reached (the website reference page is regenerated from
`README.md` by the release docs bot; ClickHouse is already in its app
list).
- [ ] 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: needs a `version` attribute on the ClickHouse resource
(maintainer decision — see note above)
- [ ]
[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(clickhouse): add a configurable `version` parameter (v25.8, v25.3, v24.9; default v24.9) so the deployed ClickHouse version is selectable and returned through the API. Existing installations are unchanged.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added a configurable ClickHouse version setting for both server and
Keeper images, defaulting to `v24.9`.
* Supported versions are `v25.8`, `v25.3`, and `v24.9`, with compatible
image patches selected automatically.
* **Documentation**
* Documented the version setting, default, and supported values.
* **Validation**
* Unsupported versions are rejected with an error listing allowed
options.
* **Tests**
* Added coverage for version mapping, defaults, invalid values, and
update behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Address review of the version generator (hack/update-versions.sh):
- Fetch tags with `skopeo list-tags` (registry v2 /tags/list) instead of
the Docker Hub v2 API, whose anonymous pagination caps at 1000 entries
and then returns an error page with no `.results`, silently truncating
the list so the frozen 24.9.2.42 fell outside the window and the run
failed. skopeo is already in build-deps and is how the sibling
generators (postgres, redis, mongodb, opensearch, kubernetes) fetch.
- Make a frozen default tag missing from the registry a hard error
instead of silently falling back to a newer patch, which would move
the default image for existing installs.
- Restore 0644 on the regenerated files (mktemp's 0600 survived the mv).
- Exclude /hack from the chart tarball via .helmignore.
- Refresh files/versions.yaml: v25.8 -> 25.8.32.4, the true latest 25.8
patch now visible with the complete tag list.
- bats: add a missing-frozen-tag hard-error case and make the atomicity
test reach the resolution phase after a valid default rather than
failing at the default-in-set check.
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
The guard's refusal message told the operator to run the platform migration,
but the case that reaches it is precisely one the migration already ran and
stamped for (its slot was renumbered out on a backport, or the parent
HelmRelease predates the application.kind label and the selector skipped it) --
re-running does nothing. Name the manual recovery instead: annotate
resource-policy=keep on the listed MachineDeployments, then stamp the
cozystack-version ConfigMap back below the worker-adoption migration so it
re-runs. Also note in the Phase 2 README that computeplane clusters render
their worker pools as separate KubernetesNodes releases too.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Add a `version` value (v25.8, v25.3, v24.9; default v24.9) to the
ClickHouse chart, backed by files/versions.yaml and resolved by
templates/_versions.tpl for both the ClickHouse server and Keeper
images, so the Cozystack API returns the engine version (spec.version)
like the other managed-DB charts. Default v24.9 -> 24.9.2.42 keeps
existing installations byte-for-byte identical.
The maintainer-run hack/update-versions.sh generator intersects the
server/keeper Docker Hub tags byte-collated (LC_ALL=C) so comm keeps
tags across the X.9 -> X.10 boundary; freezes the default major to the
tag it already ships (read from versions.yaml, so the freeze follows the
default); errors on a dropped default or an unresolvable major; writes
atomically; and runs on stock macOS bash 3.2 / BSD. Covered by
hack/clickhouse-update-versions_test.bats (10 cases) and the
tests/version_test.yaml helm-unittest suite.
Refs: #1246
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Phase 2 moved worker nodeGroups out of apps/kubernetes into apps/kubernetes-nodes,
but the ComputePlane module still passed nodeGroups to the wrapped kubernetes
chart, which now ignores it: a fresh ComputePlane cluster came up with zero
workers (and the pinned ingress-nginx addon stuck Pending), while existing ones
were unreachable by migration 56 (it only adopts kubernetes-* releases, not
computeplane-cluster).
The module now emits one kubernetes-nodes child HelmRelease per nodeGroup,
shaped like a natively created pool, and materialises the pre-split default md0
(roles: [ingress-nginx]) when nodeGroups is empty. The pool chart's clusterName
gains an internal clusterReleaseName override so pools attach to the module's
fixed computeplane-cluster release, which is off the aggregated API's
kubernetes-<cluster> convention; the default keeps every API-created pool
byte-identical.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The Kubernetes CR still accepts and stores spec.nodeGroups, spec.nodeHealthCheck
and spec.maxNodeProvisionTime after the Phase 2 split (the adoption migration
leaves them on an upgraded CR rather than rewriting parent values mid-upgrade),
but they no longer render anything, so an operator scaling via
spec.nodeGroups.<x>.minReplicas silently got nothing.
Emit a non-blocking client-facing admission warning on Create/Update when any
of these fields is present, pointing the operator at the KubernetesNodes
resources. A hard rejection is intentionally avoided: it would break edits to
already-upgraded clusters that still carry the fields. Also enumerate all the
Phase 2 breaking changes (three removed fields, two KubernetesNodes
immutability rules, the 32-char cluster-name cap) in the chart README.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The merge renumbered the worker-pool adoption migration from 54 to 56, but
six references still named 54 (README, pre-delete-unpin hook, cluster.yaml,
worker_adoption_guard test), which now points at main's redis-operator
adoption migration. Repoint them at 56.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The 'emits no worker-pool objects' assertions used 'not: true' with
'documentIndex: -1', which scopes the check to the last rendered document
only, so a stray MachineDeployment/KubevirtMachineTemplate/MachineHealthCheck/
TalosConfigTemplate anywhere else passed silently. Move 'any: true' inside
the containsDocument matcher and drop documentIndex so the negation covers
every document. Verified the asserts now redden when a worker object is
present.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Resolve conflicts introduced by main:
- Drop the FluxCD tenant addon (API type, deepcopy, values schema, RD) in
favour of the GPU type main added; main removed the fluxcd addon wholesale.
- Renumber the Phase 2 worker-pool adoption migration 54 -> 56, behind main's
redis-operator (54) and fluxcd-removal (55) migrations; targetVersion 55 -> 57.
- Accept the branch's removal of the in-chart talos-reconcile Job; it moved to
the kubernetes-nodes chart, main only touched its comment.
- Regenerate kubernetes values.schema.json / README and the kubernetes-rd
ApplicationDefinition schema.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
## What this PR does
Break the tenant `bootstrap-token-tenant` Job out of its `helm.sh/hook:
post-install,post-upgrade` lifecycle and move it into the main install
phase, so the `bootstrap-token-<id>` Secret lands in the tenant
`kube-system` before `MachineDeployment` can converge.
### Why it deadlocks today
- Job wired as `post-install,post-upgrade` fires only after Helm's wait
phase resolves
- Wait phase blocks on `MachineDeployment` Ready
- MD Ready blocks on nodes registering
- Node registration blocks on the kubelet CSR path, which needs the very
`bootstrap-token-<id>` Secret this Job produces
- Result: any fresh install, or any HR reconcile that finds MD in a
non-Ready state, sits in a deadlock; `HR.status.upgradeFailures` climbs
without the post-upgrade hook ever getting its turn
`pre-install,pre-upgrade` is not a fix either — those fire before
non-hook templates apply, so `<release>-admin-kubeconfig` (materialised
by Kamaji after `KamajiControlPlane` is applied) is not yet available.
### The shape that works
Same shape `talos-reconcile` already uses in this chart: main-phase
resources. Helm applies the SA and Job together with
`KamajiControlPlane` and `talos-secrets`. Job's Pod stays pending until
Kamaji publishes the admin kubeconfig Secret (`backoffLimit` covers the
retry window). Job completes before MachineDeployment can converge.
Helm's wait treats the Job as another resource to reach terminal state —
it doesn't gate it on MD.
### Verification
- Reproduced the deadlock on two independent long-running tenant
clusters (v1.6.x), documented in #3875
- Reproduced the manual unblock (creating the `bootstrap-token-<id>`
Secret by hand in the tenant `kube-system`) on both — nodes register
within seconds, MD converges within minutes, HR clears
- Extended `packages/apps/kubernetes/tests/talos_templates_test.yaml` to
pin the absence of `helm.sh/hook` on both rendered documents; `helm
unittest` passes 9/9
### Screenshots
Not a UI change.
### Downstream repositories
Walked the trigger map in `docs/agents/contributing.md`. The diff
touches
`packages/apps/kubernetes/templates/talos/bootstrap-token-tenant-job.yaml`
and its test — no app add/remove, no `values.schema.json`, no
core/platform values, no `hack/`, no CRD, no `ApplicationDefinition`, no
telemetry metric.
- [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): tenant `bootstrap-token-<id>` Secret is now created in the main install phase instead of a post-install/post-upgrade hook, unblocking fresh installs and any HR reconcile that finds MachineDeployment in a non-Ready state.
```
Closes#3875
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved bootstrap-token job scheduling by running it as a standard
chart resource instead of a Helm lifecycle hook.
* Jobs are automatically recreated when relevant configuration changes,
ensuring updates are applied reliably.
* Added startup handling to wait for the admin kubeconfig when needed.
* Improved retry and cleanup behavior so deployments are not
unnecessarily blocked.
* Ensured related resources are created consistently during chart
deployment.
* Improved job naming stability while respecting Kubernetes naming
limits.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
The optional `addons.fluxcd` toggle of the Kubernetes app deployed a
Flux Operator + FluxInstance into a tenant cluster. It was served by two
vendored charts, packages/system/fluxcd-operator and packages/system/
fluxcd, which were the last AGPL-3.0 content in this Apache-2.0
repository and a governance blocker for CNCF Incubation.
The management cluster does not use these charts (it runs classic Flux
via internal/fluxinstall; migration 21 already removed the operator from
it), and the addon defaulted to disabled. Rather than maintain an AGPL
payload for an unused feature, drop it entirely:
- delete both system packages and their platform source entries
- remove the addons.fluxcd HelmReleases, values and generated schema
from the Kubernetes app
- drop the addons.fluxcd block still submitted by the e2e harness
- drop the removed fluxcd package entries from CODEOWNERS
BREAKING CHANGE: the addons.fluxcd option of the Kubernetes app is
removed. Tenant clusters that had it enabled keep their running Flux
release; see the companion migration for the upgrade path.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Fold bootstrapTokenSecret into the Job content hash alongside
bootstrapTokenId. talos-secrets.yaml regenerates the two token halves
independently, so hashing only the id left a rotated secret no-oping a
completed Job and the tenant stuck on the stale bootstrap-token.
Also address review feedback: drop a review-round reference from the
tracked test, correct a stale hash in a test comment, pin the
alternate-image hash case exactly instead of by regex, and fix a comment
indent in talos-reconcile-job.yaml.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Address review of the main-phase bootstrap-token Job.
The deadlock the header claimed cannot occur on this release: the
kubernetes cozyrd carries helm-install-disable-wait, which cozystack-api
maps to Install.DisableWait and Upgrade.DisableWait, so there is no
main-resource wait phase in front of the hooks. Rewrite the rationale
around the real reason to leave the hook shape: a post-upgrade hook never
runs on a release whose upgrade keeps failing, so the token Secret never
lands. Fix the stale 'stays a post-install hook' note in
talos-reconcile-job.yaml and the 'fails the chart upgrade loudly' comment,
which is untrue once Helm no longer waits on the Job.
Give the immutable Job a content-hash name suffix over its whole rendered
spec, mirroring talos-reconcile: a static name would fail an upgrade that
lands while the Job is live (or inside its TTL) with 'spec.template: field
is immutable'. Fold the looked-up bootstrapTokenId into the hash so a
rotated token also rolls the Job instead of Helm no-oping a completed one.
Make the admin-kubeconfig volume optional and gate the Pod on the shared
wait-for-kubeconfig init container, so it waits in init (uncounted by
backoffLimit) instead of FailedMount at t0.
Extend the template tests for the hashed name, its rotation on an image
change, and the optional volume + init container.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The bootstrap-token-tenant Job was wired as a Helm post-install/post-upgrade
hook. post-* hooks fire only after Helm's wait phase resolves, and the tenant
kubernetes release wait blocks on MachineDeployment Ready, which blocks on
nodes registering, which needs the kubelet CSR path — for which the very
bootstrap-token this Job produces has to already exist in the tenant
kube-system. Fresh installs and any HR reconcile that finds MD in a
non-Ready state deadlock; HR.upgradeFailures climbs while post-upgrade
hooks never get their turn to run.
pre-install/pre-upgrade would not help either: they fire before non-hook
templates apply, so <release>-admin-kubeconfig (materialised by Kamaji
after KamajiControlPlane is applied) is not yet available.
Move the ServiceAccount and Job to the main install phase, matching what
talos-reconcile already does in this chart. Helm applies them together
with the KamajiControlPlane and talos-secrets; the Job Pod stays pending
until Kamaji publishes the admin kubeconfig Secret (backoffLimit covers
the retry window) and then completes before MachineDeployment can
converge. Helm's wait treats the Job as another resource to reach
terminal state and does not gate it on MD.
Extend the existing template test to pin the absence of helm.sh/hook
on both the ServiceAccount and the Job, so this cannot regress silently.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Reconcile the phase-2b split with main's configurable worker-health
feature (MHC condition timeouts, maxNodeProvisionTime, podCpuLimit/
podCpuRequest), which landed on both charts after this branch forked:
- kubernetes (parent): pool rendering stays removed; main's parent
worker-pool additions do not apply post-split.
- kubernetes-nodes (pool): absorb the feature; keep the directional
parent-version skew guard and the immutable cluster/storageClass rules.
KubevirtMachineTemplate content hashes are unchanged (render snapshot).
- migration 54: carry podCpuLimit/podCpuRequest (KMT-hash inputs) and the
cluster-wide readyUnknownTimeout/readyFalseTimeout/maxNodeProvisionTime
into adopted pools, so adoption neither rolls nor resets a tuned pool.
Covered by a new adopt-values bats case.
- drop the parent chart's obsolete MHC-timeout suites; the
kubernetes-nodes twins cover the feature.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The freshworks-oss operator's compiled-in default image moved from
redis:6.2.6-alpine to 7.2.4; Harbor pinned no image at all and apps/redis left
its sentinels unpinned, so an upgrade would move them silently. 7.2.4 rewrites
dump.rdb into a format 6.2.6 refuses to load, so a platform rollback would leave
Harbor's Redis crash-looping on its own data. Pin redis+sentinel (Harbor) and
the sentinel (apps/redis) at the version they run today; bump deliberately and
separately.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
The freshworks-oss operator filters RedisFailovers by the
redis-failover.freshworks.com/operator-group label matching its
OPERATOR_GROUP_ID; without the label it ignores the CR and no Redis is
provisioned. Stamp the group (cozystack) so the migrated operator adopts
the app's clusters.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
The CAPI webhook validates nodeStartupTimeout with a 30s floor but does
not validate unhealthyConditions timeouts at all, so a zero or negative
readyUnknownTimeout or readyFalseTimeout applied and remediated a
Machine the moment its Ready condition flipped. Route both fields
through the same render-time duration guard as maxNodeProvisionTime,
extracted into a kubernetes.positiveDuration helper in each chart.
The guard now also accepts the canonical form a duration serializes
back to ("20m0s", "1h0m0s"): metav1.Duration prints that shape, so a
value copied from kubectl output previously failed the render. The
first segment must still be positive and later segments may be zero,
which keeps every accepted string a positive duration.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
## What this PR does
Removes `--force-delete-unregistered-nodes=true` from the tenant
cluster-autoscaler Deployment. On the clusterapi provider the flag
cannot do what its name promises: the provider's `ForceDeleteNodes`
returns `cloudprovider.ErrNotImplemented`, the autoscaler falls back to
the ordinary `DeleteNodes` path, and that path enforces the node group's
minimum size. The one behavior the flag does change is harmful here: it
skips the step that trims a long-unregistered delete batch down to the
group's headroom above min size, so a batch larger than the headroom is
refused whole where the default would trim it and delete what fits.
Context: #3513.
A regression test renders the Deployment, proves the args list is live
with a positive anchor on `--cloud-provider=clusterapi`, and asserts the
flag is absent, so reintroducing it fails the suite.
### 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
chore(kubernetes): drop the cluster-autoscaler force-delete-unregistered-nodes flag; the clusterapi provider does not implement force deletion, and the flag only made batch removal of long-unregistered machines all-or-nothing
```
`--force-delete-unregistered-nodes` reads as an override that lets the
autoscaler remove a long-unregistered machine below its node group's
min size. On the clusterapi provider it grants nothing of the sort:
force deletion is not implemented there, the call falls back to the
ordinary delete path, and that path refuses to take a group below its
min size. The flag never bought the safety valve its name suggests.
What it does buy is a behaviour nobody asked for. With the flag set the
autoscaler skips trimming the delete batch down to the group's headroom
above min size, and the ordinary delete path then refuses a batch
larger than that headroom outright instead of removing what it can. A
group of three machines above a min size of one loses two
long-unregistered machines with the flag unset, and none with it set.
Removing it restores the documented default and takes away a signal
that advertised a protection the provider does not offer.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
nodeStartupTimeout is the window a Machine has to produce a Node at
all, measured from the later of the Machine's creation and its
InfrastructureReady transition. What has to fit inside it is the guest
boot and the kubelet registering with the tenant apiserver, and on a
host under load neither is quick. At 10m a worker that was still
booting was deleted before it ever joined, and its replacement started
the same race from the beginning.
Also record what a percentage maxUnhealthy does at small group sizes,
because the interaction is not what the field name suggests: the value
is scaled against the machines the check covers and rounded down, and
remediation proceeds only while the unhealthy count is at or below the
result. At the default "50%" a group of one never remediates, and a
group of two remediates one unhealthy machine but not two.
The golden-parity gate pinned the child chart's MachineHealthCheck
values to the parent's defaults as inputs, which made a default moved
on both sides fail there for the wrong reason. Drop the pins so the
comparison is between the two charts' defaults, which is the contract
it exists to hold, and pin the defaults themselves in each chart's own
unit tests instead.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
The two unhealthyConditions timeouts were fixed in the template at 30s
for `Ready=Unknown` and 300s for `Ready=False`, and both are too tight
for a worker joining on a loaded host.
`Ready=Unknown` is what the node lifecycle controller writes when a
kubelet stops reporting, on a grace period of its own, so 30s on top
of that deleted the VM of a guest that had merely stopped being
scheduled for a stretch, before it had a chance to answer again. And
`Ready=False` is the condition a registered Node holds until its CNI
is up, so 300s was the entire budget a first boot got for the CNI
rollout to reach it -- a worker deleted mid-rollout takes the rollout
with it and its replacement starts over.
Move both to values, defaulting to 5m and 10m. The remediation of a
worker that is genuinely dead is correspondingly later, which costs
less than it appears to: Pods leave an unreachable Node on the node
lifecycle controller's taint-based eviction and the toleration that
carries, not on either of these timers.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
The cluster-autoscaler treats a machine as long-unregistered once its
own max-node-provision-time elapses, and a worker that has not
registered by then stops counting towards its node group's upcoming
capacity. On a host slow enough that the Talos image import, the guest
boot and the CNI rollout do not fit inside that window, the timer
fires on machines that were still joining: the group reads as short of
capacity, so the autoscaler asks for replacements, and the extra
provisioning load makes the join it was waiting for slower still.
Render the per-node-group override the autoscaler reads off the
MachineDeployment, sourced from a new maxNodeProvisionTime value that
defaults to 30m. The duration is checked at render time because the
autoscaler discards one it cannot parse and falls back to its own
default without failing, and because the values schema types the field
as a string and so accepts every malformed duration.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
The README told a tenant to fetch <release>.tenant-ca, but the release
name carries the postgres- prefix the application definition applies, so
the object is postgres-<name>.tenant-ca. Following the old text returned
NotFound.
Also drop two comments promising a companion ValidatingAdmissionPolicy
that pins sentinel writes to helm-controller. No such policy ships: the
proposal for it was rejected because authorization by username duplicates
RBAC in a more brittle form. The RBAC boundary is the whole control.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Both podCpuLimit and podCpuRequest were rendered under `with`, which
skips a zero as readily as an absent field, and their guards were keyed
the same way. The generated schema behind them admits a leading minus
and a zero as readily as a sane value, so rather than patch whichever
edge turned up, the whole value domain is settled in one place.
Presence, not truth, is the predicate everywhere either field is read.
A negative value is refused: it never reaches a running Pod. A zero
request on its own is allowed and reserves nothing. A zero request
beside a non-zero limit is refused: KubeVirt's defaulter tests the
request with IsZero, which an explicit zero satisfies exactly as an
absent key does, so it overwrites the zero with the limit and the
worker asks the scheduler for its whole ceiling. That is the outcome
podCpuRequest exists to prevent, reached silently and reported nowhere,
which is why it is refused rather than written down.
A zero limit removes the ceiling rather than being the tightest one --
the kubelet turns a zero CPU limit into no CFS quota -- but it reaches
a Pod only paired with a zero request. KubeVirt keeps deriving a
request from the vCPU count when the VM declares none, the defaulter
that would have copied the limit across does not fire because the limit
is itself zero, and API server validation refuses a request above a
limit that exists. Zero is a limit that exists. So a lone zero limit is
refused too.
Both fields document the same rules and each class has a test on both
charts. A group that sets neither field renders the bytes it rendered
before, so the content-hash name of the worker VM template does not
move.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
cozy-lib.resources.toFloat knew every suffix of the Kubernetes quantity
grammar except `u` and `n`. A suffix it does not recognise is not an
error: the unit letter stays on the string, `float64` cannot parse the
result and yields zero silently, and the caller divides or compares
that zero as if it were the value.
The node group CPU request guard is one such caller. A request of 100u
above a limit of 50u compared zero against zero and rendered, so the
pair reached API server validation on the worker's Pod instead of
failing where the values were written.
Pinned by a suite over the whole suffix grammar, one value per form,
because the failure has no error to notice it by.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
Adds nodeGroups.<name>.podCpuRequest, rendering into
domain.resources.requests.cpu on the worker VM, and the same field on
the kubernetes-nodes chart, which is the other half of a byte-identity
contract rather than a separate feature surface.
podCpuLimit on its own cannot raise the ceiling of a worker without
raising what that worker asks the scheduler for, because KubeVirt
copies a declared CPU limit into the request when the VM declares none.
The request is the half that has to hold still, so the pair can now be
moved apart.
Both fields conflict with instancetype sizing for the same reason and
in the same upstream check, so the request gets the guard the limit
already had. A request above its own limit fails the render too, rather
than reaching API server validation on a Pod two objects away from the
CR that named the values; the quantities are compared as numbers, so
500m under a ceiling of 1 is accepted where a lexical guard would
refuse it.
A node group that sets neither field renders the bytes it rendered
before, which the content-hash name of the KubevirtMachineTemplate
depends on, and the render-parity gate gains a case that compares the
pair across the two charts.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
The field was documented as moving the ceiling alone and leaving the CPU
request at the value KubeVirt derives from the guest vCPU count and the
cluster CPU allocation ratio. KubeVirt does the opposite: its VMI
defaulter copies a declared CPU limit into the CPU request whenever the
VM declares no request of its own, so a node group that sets only the
limit asks the scheduler for its whole ceiling, and a ceiling the nodes
cannot spare leaves the workers Pending on Insufficient cpu.
The same paragraph promised a floor that does not exist either. A limit
below the derived request was said to leave the container with a request
above its limit; the copy makes the two equal instead, whatever the
limit is.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
The split-out pool chart renders the same worker VM as the monolithic
one, and tests/render-parity.sh exists to prove it renders it
byte-identically, because the KubevirtMachineTemplate is content-hash
named and any divergence rolls every live worker VM. A field that only
one of the two charts knows breaks that quietly rather than loudly: a
pool adopted into a kubernetes-nodes release would render without the
CPU limit it was given, the hash would change, and the ceiling would
fall back to the vCPU count with nothing reporting it.
So the field, its render-time guard against combining it with an
instancetype, and its tests arrive here in the shape the parent chart
has them, and the parity gate gains a case that compares the two
renders with the limit set.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
The chart had no way to give a worker VM a CPU ceiling different from
its vCPU count. `resources.cpu` sizes the guest, and the pod side is
filled in by KubeVirt: with a ResourceQuota limiting CPU in the
namespace, which every tenant carrying resourceQuotas has, it derives
limits.cpu from the guest vCPU count. The QEMU emulator thread and the
IO threads then come out of the same allowance as the vCPU threads,
which is tight for a nested guest that pays emulation overhead.
Declaring the limit on the VM is what stops KubeVirt deriving one, and
it is the only way out that does not make things worse: dropping cpu
from the tenant resourceQuotas would let the LimitRange apply its 250m
default instead, four times tighter than what it replaced.
The limit is usable only on a group sized by resources, because
KubeVirt's instancetype applier reports domain.resources.limits.cpu as
a conflict rather than resolving it, and a VM carrying both is refused
by the admission webhook with nothing on the CR to say why. Sizing by
resources is what drops the instancetype from the VM, so the chart
fails the render for a group that kept its instancetype, beside the
existing guard for the same conflict class one field over.
The field renders no CPU request, so the request stays derived from the
vCPU count and the cluster CPU allocation ratio, and the scheduler
weight of a worker does not move with its ceiling. It renders nothing
at all when unset, byte for byte: the enclosing KubevirtMachineTemplate
is named by a hash of its content, so anything else there would roll
every live worker VM on upgrade.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>
The worker-adoption render guard required meta.helm.sh/release-name == the
parent release as a precondition to refuse a control-plane-only render. But
Helm's prune gates solely on helm.sh/resource-policy=keep and never consults
the live release-name (migration 54 documents this and its foreign-owner branch
depends on it). A same-cluster, unpinned worker MachineDeployment whose
release-name is empty (Helm annotations stripped out-of-band, or a DR restore
of the CAPI objects) or foreign therefore slipped past the guard, yet Helm
still entered it into the parent's prune set and deleted it on the transition
upgrade: the exact worker-VM data loss the guard exists to prevent, in the
exact case (migration 54 never ran for this cluster) it was written for.
Refuse when a same-cluster, unpinned MachineDeployment is NOT owned by its
kubernetes-nodes child release (release-name lacks the kubernetes-nodes-<cluster>-
prefix), which catches parent, empty and foreign owners alike while exempting
adopted and natively-created child pools. Add a regression fixture with an
empty release-name that reddens under the old parent-release-name predicate,
and keep every conjunct non-vacuous by mutation.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
Complete the per-conjunct coverage of the guard: add a synthetic fixture where
the MachineDeployment carries the parent release-name and no keep pin but names
a different cluster, so spec.clusterName is the sole predicate keeping it out of
the refusal set. Deleting any one of the guard's three predicates now reddens
exactly one fixture (clusterName, release-name, resource-policy=keep).
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The guard lives in the else of 'if not $etcd', so it does not run during the
awaiting-etcd window, where the chart renders only the status beacon. Note that
an owned+unpinned MachineDeployment is not refused in that transient window.
This is a pre-existing exposure (the pre-split chart also rendered pools only in
this branch) that closes once etcd resolves and a real upgrade re-renders; the
comment previously claimed an unconditional refuse-to-render.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The adoption guard in cluster.yaml refuses a control-plane-only render only
when a MachineDeployment matches all three predicates: spec.clusterName,
meta.helm.sh/release-name owned by this parent release, and not pinned with
resource-policy=keep. The suite left the release-name predicate untested: the
adopted-pool case carried both a child release-name and a keep pin, so the keep
predicate alone exempted it and deleting the release-name predicate kept every
case green.
Add a natively-created (greenfield) fixture: child release-name, no keep pin.
Its clusterName and unpinned state are both true, so the release-name predicate
is the sole reason it renders. This is the routine post-split parent upgrade of
every cluster, the guard's highest-blast-radius path.
Per-conjunct mutation now: dropping release-name reddens the greenfield case,
dropping keep reddens the pinned case, dropping clusterName reddens nothing (an
MD from another cluster cannot carry this release name, so clusterName is
belt-and-braces). Baseline 6 of 6 green.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
## What this PR does
The qdrant and harbor post-delete cleanup hooks could not fail. Every
delete was guarded by `|| echo "WARNING: ..." >&2`, neither script set
`-e`, and both ended on an unconditional completion message, so the
container exited 0 on every path. Helm read that as a successful hook,
`helm.sh/hook-delete-policy: hook-succeeded` removed the Job together
with the pod that held the warning, and an uninstall that reclaimed
nothing looked exactly like one that had nothing to reclaim. Written up
in #3566.
Both hooks now count failures and set the exit code at the end. `set -e`
would abort at the first failed delete and skip the rest of the sweep,
which is the opposite of what is wanted: every remaining object should
still be attempted and the outcome reported once. Already-gone objects
stay a success, since every delete keeps `--ignore-not-found`. Each
failure message names the object it left behind. `packages/apps/mariadb`
already uses this shape.
Both hooks also consumed `kubectl get` output directly: harbor by
iterating `$(kubectl get ...)`, qdrant by piping it into `grep`, whose
status the pipeline then reports. A failed listing arrived as an empty
list, and the sweep reported success without having looked. Each listing
is now captured before it is used.
#3566 attributes that shape to harbor's two loops alone. qdrant has it
too: `pvcs=$(kubectl get pvc ... | grep -E ... || true)` reports grep's
status, so a `kubectl get` that failed left `$pvcs` empty and the hook
announced "no data PVCs found; nothing to delete". Both instances are
fixed here.
### The cost
A hook that exits non-zero keeps its Job and its pods, because
`hook-succeeded` does not fire for it. That is the point, the pods hold
the log, but it means a failed uninstall now leaves objects behind where
it previously left none. Nothing collects them on its own:
`before-hook-creation` only fires when the hook runs again, and a
`post-delete` hook runs only on an uninstall, so they stay until a
release of that name is installed and uninstalled again, or until the
namespace goes.
The release itself is not stranded. Helm appends the post-delete hook
error to its error list instead of returning on it, marks the release
uninstalled, purges it from storage anyway, and only then reports
`uninstallation completed with N error(s)`. The failure surfaces once
and the app still goes away.
Both Jobs get one retry instead of the Kubernetes default of six, and
instead of none. Every delete is idempotent, so a second attempt
re-issues an API call rather than re-running any logic, and one short
backoff separates a transient apiserver error from a real one. These
failures become visible for the first time with this change, and a
signal that fires on noise stops being read, including on the day it is
right. Six is the far end of the same mistake: ten minutes of backoff
across seven pods outlasts the hook timeout, so Helm reports that
timeout instead of the hook's own error.
`packages/apps/clickhouse` and `packages/apps/mariadb` already exit
non-zero from their cleanup hooks and set no retry limit, so a real
failure in either spends the full backoff and reaches the operator as a
timeout rather than as the error the hook wrote. Adding the exit code
without the cap reproduces that.
One limitation is deliberate. harbor's ACME sweep cannot tell "this kind
is not served" from "I could not look". If cert-manager is torn down
before the release, `kubectl get challenge` fails and the uninstall is
reported failed with nothing to sweep. cert-manager is a hard dependency
of the bundle harbor ships in, so the window is narrow, and a
CRD-existence probe would add a second silent-skip path of its own: it
would swallow a discovery failure the way this change stops swallowing a
listing failure.
### Tests
`hack/app-cleanup-hooks_test.bats` renders each hook's script out of its
chart and runs it against a fake `kubectl`, because the property under
test is the container's exit code and matching against manifest text
cannot establish it. There are eight failure paths across the two hooks,
six in harbor and two in qdrant, and each has a case that goes red when
that one path alone is reverted to a swallowed warning. Also covered: a
clean delete, and an object that is already gone in both its shapes, an
empty listing and a `NotFound` on the delete itself.
The fake `kubectl` models `--ignore-not-found` instead of exiting 0
unconditionally, so dropping that flag from a hook turns the
already-gone cases red. Message assertions are anchored on the hook's
own `ERROR: ` prefix, because the fake names the object in its own
diagnostic too, and an unanchored match is satisfied by the fixture.
Two manifest fields are pinned in each chart's existing helm-unittest
suite, where manifest fields belong. The retry cap is one. The other is
`helm.sh/hook-delete-policy`: adding `hook-failed` to that annotation
hands the failed Job straight back to Helm to delete, which undoes the
whole change. I checked that before adding the assert, and the reversal
left both suites fully green.
The qdrant Chainsaw suite explained its post-delete pod collector by
quoting the guard string this change removes, and concluded that Helm
always collects the pod away. That conclusion inverts here, so the note
is rewritten, along with the sentence in the harbor suite that points at
it.
### Screenshots
Not a UI change.
### Downstream repositories
I walked the trigger map in `docs/agents/contributing.md` against the
diff, file by file. It touches two app chart templates and adds one file
under `hack/`. No package added, renamed or removed, no
`values.schema.json`, no `values.yaml` default, no version enum, no
`ApplicationDefinition`, no namespace, no node prerequisite, and neither
`hack/package.mk` nor `hack/update-crd.sh`.
- [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(apps): the qdrant and harbor post-delete cleanup hooks now exit non-zero when a delete or a listing fails, instead of reporting success on every path. `helm uninstall` reports the failure rather than returning clean, and the hook Job and its pods stay in the namespace so the log survives to be read. Nothing collects them on its own: they remain until a release of that name is installed and uninstalled again, or until the namespace is removed.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Improved Harbor and Qdrant cleanup reliability by detecting listing
and deletion failures.
- Cleanup jobs now retry once and report failures with a non-zero
status.
- Cleanup continues processing remaining resources after individual
failures.
- Successful cleanup remains safe and repeatable, including when
resources are already missing.
- Harbor cleanup remains limited to resources associated with the
configured Harbor host.
- **Tests**
- Added comprehensive coverage for successful, repeated, partial, and
failed cleanup scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does
Two tests in
`packages/apps/tenant/tests/vmo_webhook_dependson_test.yaml` say the
seaweedfs and gateway HelmReleases deliberately carry no operator gate,
and assert that with `notExists: spec.dependsOn` and nothing else. A
bare absence assert also passes when the template renders no document at
all, so both stayed green with those two releases switched off entirely.
With `seaweedfs: false, gateway: false` the suite still reported 5 of 5
passing; pinning `metadata.name` next to the absence turns the same run
into 2 failures.
An earlier check on these asserts injected a `dependsOn` and watched
them redden. That proves they catch an added gate, which is the other
direction. For a negative assert the direction that matters is the
document going away, and that one was never covered.
Pairing `equal: metadata.name` with the absence is not a new idiom
introduced here. `no_upgrade_force_test.yaml`, in the same directory,
already anchors all seven of its releases that way, and this brings the
last two suites in the chart into line with it.
Both suite headers also carried a reason for their `templates:` list
that has expired: that rendering the whole chart fails because
`keycloakgroups.yaml` indexes `.Values._cluster` unguarded. #3636 added
the guard, so a no-values render of the chart now succeeds. The list is
still load-bearing, for the reason `no_upgrade_force_test.yaml` already
records — it keeps an unrelated template breaking out of this suite's
result. Checked by adding a nil dereference to a template neither suite
asserts on: both suites stay green with their lists and error without
them.
### 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
```
The release name kubernetes-nodes-<cluster>-<pool> does not encode where the
cluster segment ends, so several (.Values.cluster, pool) splits reconstruct the
same object name kubernetes-<cluster>-<pool>. An operator who edits spec.cluster
to another such value (immutability is enforced only by the dashboard, not the
aggregated apiserver, per docs/storage-immutability.md) prunes no worker VM but
silently flips spec.clusterName and the pool WorkloadMonitor selector; CAPI
rejects the immutable MachineDeployment.spec.clusterName Update loudly while the
WorkloadMonitor drift stays silent.
Extend assertNoForeignPool to refuse the render when the live MachineDeployment
of the reconstructed name carries a spec.clusterName that disagrees with the
clusterName this pool renders, naming the value to restore. Inert offline.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The previous suite only pinned that the guard is inert on an empty offline
lookup; reverting the guard kept it green. Seed a live lookup through
kubernetesProvider (as parent_version_skew_test does) and pin the real
contract: an owned, unpinned MachineDeployment fails the render; an owned MD
pinned with resource-policy=keep renders; an MD adopted onto its child release
renders; an MD from a different cluster renders.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The Phase 2 worker-pool adoption guard flagged any worker MachineDeployment
still owned by the parent release, without checking helm.sh/resource-policy.
Migration 54 deliberately pins keep on pools it cannot adopt (non-object
nodeGroups, invalid rfc-1123 child name, child name over 53 chars, non-object
group value) and leaves the parent release-name in place, so keep is its
processed-and-made-prune-proof signature. The guard therefore converted that
documented degraded-but-running state into a permanent upgrade deadlock for
the tenant.
Skip MachineDeployments carrying helm.sh/resource-policy=keep: a pinned pool
is already safe from Helm prune, so the only remaining data-loss case is an MD
owned by this release and unpinned, which means the migration never processed
the pool. Reword the failure message accordingly.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The two "carries no operator gate" tests asserted only
`notExists: spec.dependsOn`. A bare absence assert passes on a template
that renders no document, so toggling `seaweedfs` and `gateway` off left
both green while the releases they guard were gone. Measured: all 5 tests
passed with the templates rendering nothing, and 2 of them fail once the
name is pinned alongside the absence.
An earlier check on these two asserts injected a `dependsOn` and watched
them redden, which proves only that they catch an added gate. That is the
other direction; the one that matters for a negative assert is the
document disappearing.
Both suite headers also justified their `templates:` list with a reason
that has expired: keycloakgroups.yaml now guards its `.Values._cluster`
index, so a no-values render of the whole chart succeeds. The list is
still load-bearing, for the reason the sibling suite already records. It
keeps an unrelated template out of this suite's result: with a nil
dereference added outside the guard of a template neither suite asserts
on, both suites stay green with their list and error without it.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The qdrant and harbor post-delete hooks guarded every delete with
`|| echo "WARNING: ..." >&2`, set no `-e`, and ended on an
unconditional completion message, so the container exited 0 on
every path. Helm read that as a successful hook and the
hook-succeeded delete policy removed the Job together with the pod
that carried the warning, leaving an uninstall that reclaimed
nothing indistinguishable from one that had nothing to reclaim.
Count the failures instead and set the exit code at the end, so a
failed delete does not skip the deletes that follow it. An object
that is already gone stays a success: every delete keeps
--ignore-not-found, which exits 0 in that case. This is the shape
packages/apps/mariadb already uses for the same problem.
Both hooks also consumed the output of `kubectl get` directly --
harbor by iterating it, qdrant by piping it into grep, whose status
the pipeline then reports. A listing that failed therefore arrived
as an empty list and the sweep was skipped rather than reported.
Each listing is now captured before it is used, so a listing that
failed and one that returned nothing are different events.
A hook that exits non-zero keeps its Job and pods, which is the
point: they hold the log. Give the Job one retry rather than the
Kubernetes default of six or none at all. Every delete is
idempotent, so a second attempt re-issues an API call rather than
re-running any logic, and one short backoff is what separates a
transient apiserver error from a real one -- these failures become
visible for the first time here, and a signal that fires on noise
stops being read, including on the day it is right. Six is the far
end of the same mistake: ten minutes of backoff across seven pods
outlasts the hook timeout, and Helm reports that timeout instead of
the hook's own error.
The qdrant Chainsaw suite explained its post-delete pod collector
by quoting the guard string removed here and concluding the pod is
always collected away. That is now backwards: a failed cleanup is
exactly when the pod survives and the collector is the evidence.
Rewrite the note, and the sentence in the harbor suite pointing at
it.
hack/app-cleanup-hooks_test.bats renders both scripts and runs them
against a fake kubectl. There are eight failure paths across the two
hooks -- six in harbor, two in qdrant -- and each has a case that
goes red when that one path alone is reverted to a swallowed
warning. Message assertions are anchored on the hook's own `ERROR: `
prefix, because the fake kubectl names the object in its own
diagnostic too and an unanchored match is satisfied by the fixture.
The helm-unittest suites pin the retry limit and the hook delete
policy: adding hook-failed to that annotation hands the Job back to
Helm to delete on failure and destroys the evidence this change
exists to keep, and until these asserts that reversal left both
suites green.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Alexey Artamonov