mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-04 02:57:27 +09:00
kafka-multi-instance
786
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Timofei Larkin
|
f5b957ee27 |
feat(kubernetes)!: manage worker pools as KubernetesNodes, adopt on upgrade (Phase 2, breaking) (#3315)
## What this PR does Phase 2 of the Kubernetes-app split ([cozystack/community#8](https://github.com/cozystack/community/pull/8)) — the **breaking** half. Stacked on #3314 (review that first). Removes worker node pools from the `kubernetes` chart (they moved to `kubernetes-nodes` in #3314) and adopts existing pools on upgrade: - `kubernetes` chart is now **control-plane only**: drops the pool `range` block, the `talos-reconcile` Job + RBAC (deleted; moved to `kubernetes-nodes`), the preserve-old-KMT block, the `kubernetes.nodeGroups` helper, and the per-group dashboard RBAC. - **`spec.nodeGroups` is removed from the `Kubernetes` CR** (BREAKING). Worker pools are now separate `KubernetesNodes` resources. - **Migration 54** (bumps `targetVersion` to 55): pre-upgrade hook that, for every `kubernetes-<cluster>` release, creates one `kubernetes-nodes-<cluster>-<pool>` HelmRelease and re-annotates the existing worker objects onto it with `helm.sh/resource-policy=keep`, so the now control-plane-only parent upgrade does not prune running workers. The default `md0` (implicit when `nodeGroups` was empty) is materialised explicitly. Idempotent. Genuine infrastructure/read failures fail closed (`exit 1`, retried on the next upgrade); hostile stored shapes (a pool name over Helm's 53-char release-name cap, an RFC-1123-invalid or whitespace/slash map key, a non-object `nodeGroups`) are warned, pinned, and skipped rather than deadlocking the entire fleet's upgrade. - The `ingressNginx` addon no longer auto-provisions a node (the implicit `md0` default is gone). If enabled without a pool carrying `roles: [ingress-nginx]`, the controller DaemonSet simply schedules nothing (degraded, not failed) — the requirement is documented in the chart NOTES.txt rather than enforced at render time. Because the child chart renders the pool objects **byte-identically** to the old parent render (golden parity in #3314; the KubevirtMachineTemplate content-hash name is preserved), adoption does not churn live worker VMs. ### Migration number `54` is contiguous with main (latest is `53`) and `targetVersion` is `55`. The `52`/#3201 collision noted in earlier revisions has been resolved: this branch was rebased and merged past it, so the numbering is contiguous and `run-migrations.sh` has no gap to hard-fail on. ### Not yet done (blocking draft → ready) - [ ] **dev e2e upgrade validation** (`cozystack-pr-test`): upgrade a custom-`nodeGroups` cluster and a default-`md0` cluster; assert zero worker-VM churn (KMT hash, Machine uid/creationTimestamp stable) and that the adopted child HRs reconcile. - [ ] downstream trigger-map walk (website docs for the `nodeGroups` → `KubernetesNodes` change; terraform-provider for the removed API field). ### Downstream repositories Deliberately unticked (draft) — the breaking API change affects docs and the terraform provider; follow-ups to be filed before ready. - [ ] 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: ```release-note Worker node pools of a managed Kubernetes cluster are now managed as separate `KubernetesNodes` resources. `spec.nodeGroups` on the `Kubernetes` CR is removed; existing pools are adopted automatically on upgrade. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Worker pools are now separate `KubernetesNodes` resources with their own lifecycle. * Added a render-time compatibility guard to prevent worker pools from running ahead of the parent control-plane minor version. * Improved upgrade/uninstall behavior to better preserve already-adopted worker objects. * **Breaking Changes** * Removed per-pool `nodeGroups` and worker node health settings from the `Kubernetes` chart/CR; pool configuration now lives in `KubernetesNodes`. * Ingress requires at least one `KubernetesNodes` pool with `roles: [ingress-nginx]`. * `cluster` and `storageClass` are immutable after creation; pool `version` may lag but must not be ahead of the parent minor. * **Documentation** * Updated READMEs, notes, and schemas to reflect the `KubernetesNodes` split and the immutability/version rules. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
32ec1b5a00 |
fix(migrations): correct unadopted-record count semantics and stale comments
- The unadopted-record WARNING said "N pool(s)" but counted SKIPPED_FILE lines, and the foreign-owner branch writes one line per object (so a single pool with MD+MHC+WM reported "3 pools"). Reword to "N record(s) ... one line per object or pool ... not a distinct-pool count" so the number does not mislead. - The inline-chart restamp comment implied the cozystack-apps source is enforced; it is not (matching by chart name only is a deliberate trade-off: gating on the historical sourceRef risks under-matching a real legacy parent, which loses its workers, whereas the loose match's worst case is a harmless mislabel). Spell out the trade-off and that the render guard is the data-loss backstop. - The overflow comment cited "up to 42 chars per rest.go"; the enforced cap is 32 (maxKubernetesClusterName = 53 - 17 - 4). Correct it and note legacy/hand-created names can still be longer, which is why the overflow branch exists. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
c0fcdd767c |
fix(migrations): discover pre-chartRef parents via inline spec.chart in the restamp
The restamp discovered unlabelled parent kubernetes releases by spec.chartRef only, but that misses the population it exists for. The apps.cozystack.io/ application.kind label was introduced 2025-12-19 ( |
||
|
IvanHunters
|
c6b6755eec |
fix(migrations): record foreign-owner skips, harden record_skip, scope restamp to kubernetes- names
Three hardening fixes on migration 56's new code: - The foreign-owner branch in adopt_one pins the object prune-proof but does not adopt it (the child release can then stay Failed on the ownership conflict), yet it was the one warn+pin+skip path that did not record itself in the durable unadopted ConfigMap -- so an operator relying on that record to find stuck pools would miss it. Record it like every sibling branch. - record_skip ran a bare append under set -e inside skip branches that are designed never to abort; a failed write (e.g. /tmp full) could deadlock the fleet upgrade for one tenant. Make it fail-open, matching the branches' intent. - The restamp selector matched any release by chartRef; scope it to names starting with kubernetes- so it cannot mislabel (and then have the sweep skip) a non-standard release, keeping restamp and the sweep's contract in sync. Also fix the previously-vacuous negative restamp test (its fixture lacked the chartRef, so it never reached the label guard it claimed to check). Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
3d1a83f419 |
fix(migrations): make the unadopted-pools record publish best-effort
The durable-record ConfigMap write sat under set -euo pipefail right before the version stamp, so a permanent failure (e.g. the record exceeding etcd's object size limit at a very large skip count) would abort the migration and deadlock the fleet upgrade with no path out -- even though every skipped pool was already pinned prune-proof earlier in the sweep, so the workers are safe regardless of whether the record lands. The record is a convenience artifact, not a safety mechanism, so swallow its failure with a warning instead of failing closed. Covered by a bats case that fails the publish and asserts the run still stamps. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
952901776a |
fix(migrations): adopt unlabelled parent kubernetes releases and record unadopted pools in migration 56
The worker-adoption sweep selected parents by the apps.cozystack.io/ application.kind label, which postdates v1.2. A parent kubernetes HelmRelease older than that, or one restored by hand, lacks the label: the sweep skipped it, stamped, and the parent's control-plane-only re-render then pruned its un-pinned workers with no pin and no warning. Discover such releases by their chartRef instead and restamp the three application labels before the sweep (additive, cannot corrupt a release, and repairs it for cozystack-api too). Every warn+pin+skip branch leaves a pool pinned prune-proof but unadopted, recorded only in the Job log, which the next upgrade reaps; the migration does not re-run, so an operator cannot find those pools afterward. Accumulate the skips and publish them as configmap/cozystack-migration-56-unadopted before the stamp. Extend the fake kubectl and add bats coverage for the restamp (positive and negative) and the unadopted-ConfigMap paths. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
5b08d48632 |
docs(migrations): pin DEFAULT_MD0 provenance to the pre-removal helper commit
The kubernetes.nodeGroups helper that DEFAULT_MD0 reproduces was deleted in this line of work, so nothing in-tree diffs against it and a future edit to the constant (instanceType/diskSize/...) would silently rename the content-hashed KubevirtMachineTemplate and roll every implicit-md0 pool. Reference the pre-removal definition (git show 92b9785da^:.../kubernetes/templates/_helpers.tpl) so the canonical default stays diffable. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
d93e136e23 |
fix(computeplane): provision worker pools via kubernetes-nodes after Phase 2 split
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> |
||
|
Aleksei Sviridkin
|
9ccb133b66 |
fix(ci): de-escalate select-e2e and name every escalation (#3817)
## Why
I replayed the 150 most recently merged pull requests through
`hack/select-e2e.sh` with every escalation branch instrumented. 118 of
them (78.7%) ran the full 21-suite Chainsaw run, and the causes, counted
once per escalating PR with the count where that cause was the only one
in brackets, were `FULL_PATTERN` 88 [68], `NO_SUITE_FOR_GROUP` 24 [24],
`CHAINSAW_SHARED` 18 [1] and `UNCLASSIFIED` 7 [4]. `YQ_BROKEN`,
`NO_GRAPH_OWNER` and `BACKSTOP_EMPTY` never fired.
Two problems come out of that. Most of the escalation is real but
unnecessary. Whole classes of path that provably cannot affect a
Chainsaw suite were escalating because no rule claimed them. And the
commonest cause of a full run was absent from the log, so "why did this
pull request run everything" could only be answered by re-deriving the
selection by hand.
## What changes
**Every escalation now names its cause on stderr.** Seven branches could
reach the full suite and four did it in silence, `full_suite_pattern`
among them, the commonest cause by a wide margin, so the usual answer
was the one the log never carried. The lines go to stderr and must stay
there: stdout is the suite list and both e2e lanes parse it, so a reason
line on stdout would be read as a suite name. A unit test asserts every
message, verified by muting each of the eleven in turn and confirming
the file goes red for all of them. A reason that regresses to silence
changes no selection and is otherwise invisible.
**`cozystack.etcd-application` gains its missing `dependsOn:
cozystack.etcd-operator`.** `packages/extra/etcd` renders `kind:
EtcdCluster` from `etcd-operator.cozystack.io/v1alpha2`, and this was
the only operator-backed application source in the tree with no edge to
its operator. It is a latent install bug as much as a selection gap:
without the edge `etcd-rd` registers the ApplicationDefinition as soon
as the engine is up, so a tenant can create an `Etcd` before the
operator exists and its HelmRelease fails on `no matches for kind
"EtcdCluster"`, and `hack/select-install.sh`'s forward closure for the
`etcd` suite omitted the operator for the same reason. On the selection
side `cozystack.etcd-operator` reached no runnable suite, so every
change to the operator ran all 21.
**`vm-disk-application` maps to the `vminstance` suite.** It resolved to
a `vm-disk` suite that does not exist, `intersect_suites()` dropped the
name, and the source reached nothing runnable. Neither did
`cozystack.kubevirt-cdi` above it, whose only other dependents are
`vm-default-images` and `vm-disk-application` itself, so every CDI
change ran all 21. The coverage was there and the table did not know it:
the `vminstance` suite creates a `VMDisk` and asserts the `DataVolume`
behind it (`hack/e2e-chainsaw/vminstance/vmdisk.yaml`,
`vmdisk-vmi.yaml`). Same defect class as the etcd edge above, on the
mapping side rather than the graph side, and with no production blast
radius of its own, since `src_to_suites` is read by the selector and by
nothing else.
**`cozystack.cozystack-basics` joins `cozystack.cozystack-engine` as a
propagation hub excluded from the reverse-dependency walk.** Every edge
into it exists so a namespace or a platform-wide policy is in place
before the dependent installs, which `kubevirt-cdi` states in its own
source ("Depend on cozystack-basics so the target namespace exists
first"), and that is install ordering rather than behaviour. The damage
runs opposite to the engine's: the engine fans one change out to every
app, basics narrows instead. It reaches no suite today so it escalates,
but it sits upstream of `kubevirt-cdi`, so the first suite to land under
CDI silently converts the platform's namespace-and-policy package from
the full run to that one suite. Not hypothetical — on #3426 enabling the
site-router suite takes `cozystack-basics` from 22 suites to
`site-router` alone, and nothing in the output says coverage was lost.
Dropping the reverse edges keeps the package reachable, stops it
propagating, and leaves a change to it running everything through the
per-path escalation.
**`hack/e2e-apps/<name>.bats` maps to the `<name>` suite**, off the
basename, exactly as the per-suite rule takes the name off a
`hack/e2e-chainsaw/<app>/` directory. Deliberately mapped rather than
marked inert: what remains there is wired to nothing after the Chainsaw
migration, and inert would bake that orphan status into the rule and go
quietly wrong the day a lane runs those files again. The basename is
membership-tested against the suite list on the spot, so an unmatched
one escalates there and then rather than having the verdict deferred to
the final intersection, where the rest of the diff would decide it.
**`packages/tests/` and three `.gitattributes` join
`inert_config_pattern`,** which is what the script's own header says to
do with a genuinely inert path instead of widening the fall-through.
`packages/tests/` is a helm-unittest fixture chart, and changing a test
*of* `cozy-lib` does not change `cozy-lib`, no PackageSource lists those
paths as a component, and nothing installs them, while a change to the
library itself still escalates through `packages/library/`. The
`.gitattributes` entries are enumerated rather than matched by filename,
because the justification is what those files contain (only
`linguist-generated` markers) and the name does not carry it:
`.gitattributes` can also set `filter`, `eol`, `working-tree-encoding`
and `export-subst`, each of which changes what lands in the working tree
and therefore what gets built.
**`full_suite_pattern` escalates only `hack/e2e-*.bats`, not every
`hack/*.bats`.** The root `Makefile` is the authority on the split and
draws it at exactly that prefix, `BATS_UNIT_FILES := $(filter-out
hack/e2e-%.bats,$(wildcard hack/*.bats))`, so the 60 files it keeps are
the unit lane and the e2e sandbox runs none of them, while the three it
filters out are what `packages/core/testing`'s recipes execute and those
still escalate. Being inert here does not leave them untested: `make
unit-tests` is gated on the `plan` job's `code` output, which
`pull-requests.yaml` computes as "any changed path outside `docs/`" and
never from `select-e2e.sh`, so a bats-only pull request still runs the
whole unit lane, plus install and the OpenAPI tests, since the `e2e` job
reads the same output rather than the selection.
## Measured effect
The before-and-after comparison below is a second measurement over a
slightly different population, stated separately because the two are not
interchangeable: it replays the last 150 first-parent merge commits on
`main` (`git diff --name-only $sha^1 $sha`) through the base selector
with base sources versus this branch with its own, where the histogram
above walks the 150 merged pull requests as GitHub lists them. The
populations overlap heavily and the direction is the same, and the
totals differ by two commits.
| | full suite | scoped | nothing |
|---|---|---|---|
| before | 120 | 20 | 10 |
| after | 106 | 25 | 19 |
Full-suite rate 80.0% → 70.7%, verdict changed on 15 commits. Fourteen
de-escalated and I read every one of their file lists: they are
unit-lane bats files, non-e2e workflows, `docs/`, `CODEOWNERS`,
helm-unittest fixtures and linguist markers. The fifteenth gets one
suite *wider*: a `cert-manager` change now also selects `etcd`, which is
correct, since `cert-manager` is a dependency of `etcd-operator` and the
etcd app now genuinely reaches it through the new edge.
That replay predates the two reachability rules, and replaying the same
150 first-parent merges across those two on their own — the selector at
`73bc4c0ae` against the selector at `adf231513`, same sources on both
sides — changes no verdict at all. Two of the 150 touch
`cozystack-basics` and both ran the full suite before and after, which
is the hub exclusion keeping something true rather than failing to do
anything, and none of the 150 touch `kubevirt-cdi` or `vm-disk`, so the
mapping shows up only in a direct selection.
A few individual selections, before → after:
| changed files | before | after |
|---|---|---|
| `hack/select-e2e_test.bats` | 21 suites | *nothing* |
| `hack/select-e2e_test.bats` +
`packages/apps/redis/templates/redis.yaml` | 21 suites | `redis` |
| `hack/e2e-install-cozystack.bats` | 21 suites | 21 suites |
| `packages/tests/cozy-lib-tests/**` | 21 suites | *nothing* |
| `packages/system/.gitattributes` | 21 suites | *nothing* |
| `hack/e2e-apps/postgres.bats` | 21 suites | `postgres` |
| `packages/system/etcd-operator/values.yaml` | 21 suites | `etcd` |
| `packages/system/kubevirt-cdi/values.yaml` | 21 suites | `vminstance`
|
| `packages/apps/vm-disk/values.yaml` | 21 suites | `vminstance` |
| `packages/system/cozystack-basics/values.yaml` | 21 suites | 21 suites
|
## Blast radius of the `dependsOn` edge
A `dependsOn` on a PackageSource is an install-ordering edge in
production, not only a test-selection hint, so this is the part worth
reviewing hardest. Both Packages are emitted by
`packages/core/platform/templates/bundles/system.yaml` under the single
`bundles.system.enabled` guard with no intervening conditional between
them, in every variant, so no supported configuration has the app
without the operator. `hack/select-install.sh --validate` reports the
graph still has no cycle and nothing dangling. The one new exposure is
an admin who lists `cozystack.etcd-operator` in
`bundles.disabledPackages` while keeping the app, which now leaves it
`DependenciesNotReady`. That is the same exposure postgres, mariadb,
kafka and redis already carry, with `Package.spec.ignoreDependencies` as
the escape hatch. On upgrade `etcd-rd` waits for the operator's two
HelmReleases, and an unhealthy `etcd-operator` already means broken etcd
apps.
## Testing
Every rule added or changed has a test in `hack/select-e2e_test.bats`,
the install-side change has one in `hack/select-install_test.bats`, and
the new graph edge has a helm-unittest guard at
`packages/core/platform/tests/sources_etcd_application_dependson_test.yaml`.
The round-trip test that walks every suite through both mapping tables
stays green, `vminstance` included now that two sources map onto it.
The two reachability rules were each checked by removing the rule and
confirming its own test reddens: without the mapping the CDI test falls
back to the full-suite escalation, and without the hub exclusion
`cozystack-basics` selects `redis vminstance` instead of everything. The
hub test has to seed its own hazard, since no suite sits downstream of
basics in this tree yet, so it gives `redis-application` an edge onto
basics in a copied sources directory and pins that a redis change still
selects redis, or the assertion would pass against a graph that resolved
nothing.
`make bats-unit-tests` aborts on the first failing file (`for f in …; do
… || exit 1; done`), and `hack/ghcr-mirror_test.bats` fails on `main`
today and sorts before `select-e2e_test.bats`, so the aggregate target
never reaches these tests. I ran the 60 unit files individually instead:
59 pass, and the one failure is that pre-existing one, byte-identical to
the base commit and referencing nothing this branch touches. Also green:
`make helm-unit-tests`, `make rd-presets-check migrations-target-check
test-check-readiness`, `hack/select-install.sh --validate`, and
`pre-commit run --all-files` leaving a clean tree.
## Two things a reviewer should know, neither addressed here
`assert_selection` in `hack/select-e2e_test.bats` aborts the whole file
with `exit 1` rather than failing a single test. That is the only thing
that works under `hack/cozytest.sh`, which provides no real `run`,
`$status` or `skip`, and it is the idiom the existing tests in that file
already use. It becomes wrong when #3497 and #3498 flatten these onto
real `bats(1)`, where a failing assert should end one test and let the
rest report. Worth converting with that work rather than ahead of it.
`cozystack.mongodb-application` carries the identical missing operator
edge: `packages/apps/mongodb` renders `psmdb.percona.com/v1` while the
source depends only on `cozystack.networking` and
`cozystack.cozystack-engine`, so `cozystack.mongodb-operator` still
reaches no runnable suite and every change to it escalates to all 21.
Same defect class and same fix shape as the etcd edge, but it is a
second production install-ordering change and belongs in its own pull
request.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **New Features**
- The default etcd application now includes its operator and required
platform dependencies, ensuring the EtcdCluster provider is available.
- **Bug Fixes**
- Improved end-to-end test selection for application-specific changes,
shared components, inert paths, and unresolved or unclassified files.
- Full-suite escalations now provide clear reasons through standard
error.
- **Tests**
- Expanded coverage for test selection, dependency reachability, and
etcd application dependency declarations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
IvanHunters
|
f035da8a0f |
Merge branch 'main' into feat/kubernetes-nodes-phase2b
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> |
||
|
Andrei Kvapil
|
851a86dcf1 |
feat(platform): orphan tenant FluxCD HelmReleases on upgrade
Add migration 54 for the removed addons.fluxcd feature. For every tenant that had it enabled, the FluxCD HelmReleases in the management cluster are suspended, stripped of their Flux finalizer and deleted, so the delete does not trigger a Helm uninstall inside the tenant cluster. The Flux release already running in the tenant keeps running untouched. Runs as a pre-upgrade hook, before the addon-less Kubernetes chart reaches the tenant HelmReleases. Mirrors the orphan idiom of migration 29. Bumps migrations.targetVersion to 55. Fail closed. Dropping the finalizer is the only thing that keeps the delete inert, and migrations never re-run, so the script runs under `set -euo pipefail` with no `|| true` on the critical path and gates the delete on the finalizer actually being gone. The verify read uses `--ignore-not-found` rather than `2>/dev/null || true`, so a failed read aborts instead of reading as an empty finalizer list that passes the gate on an object whose state was never checked. A migration that stops is recoverable; one that half-orphans a fleet is not. Cover it with a docker + fake-kubectl unit test running the real script by path inside the migrations image's own alpine base (busybox ash, jq baked on), so it exercises the shipped interpreter and load-bearing pipefail. The suite pins ordering and selection (only the addon HelmReleases are touched, each suspend -> finalizer-drop -> delete in order) and the fail-closed paths (a surviving finalizer, a failing verify read, and a failing fleet scan each abort before deleting and before stamping). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
7cca3d75e7 |
feat(kubernetes)!: remove the FluxCD tenant addon
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> |
||
|
Aleksei Sviridkin
|
4385990869 |
feat(redis-operator): migrate from archived spotahome to freshworks-oss fork (#3406)
## What this PR does `spotahome/redis-operator` was archived on 2026-06-11 (read-only; last release v1.2.4, 2022). This migrates the operator to the actively-maintained **freshworks-oss/redis-operator** fork (v3.3.5, Apache-2.0). ### Why freshworks-oss It keeps the same `RedisFailover` CRD and API group (`databases.spotahome.com`) — so existing managed Redis is unaffected — and adds a native `spec.engine: Redis|Valkey` switch, letting managed Valkey drop its Redis-compatibility shim image and run the official `valkey/valkey` image directly. ### Changes - Re-vendor the chart from `oci://ghcr.io/freshworks-oss/charts/redis-operator` (v3.3.5), which carries the `engine`-aware CRD. - Build the operator image from freshworks-oss v3.3.5 source. - Re-target the label/annotation-preservation patch onto the freshworks Go module (`github.com/freshworks/redis-operator`). ### Verification - freshworks-oss source + our `labels.diff` patch compiles natively with Go 1.26.3 (`service` and `cmd` packages). - `git apply --check` of the re-targeted patch against v3.3.5 succeeds. - The operator chart renders with the cozystack-registry image override. - The image is built (freshworks-oss v3.3.5 source + our patch, multi-arch amd64/arm64), pushed to `ghcr.io/cozystack/cozystack/redis-operator`, and pinned by digest in `values.yaml`. - `hack/promote-retag_test.bats` and `hack/nightly-mirror_test.bats` pass with the pinned digest. ### Note for maintainers `packages/system/redis-operator` is not part of the automated `make build`; its image is built out-of-band and the digest is pinned in `values.yaml`. That image is already built and pinned in this PR; re-run `make -C packages/system/redis-operator image` only if you bump the operator version. ### Follow-up The cozystack/redis-operator fork carries cert-manager TLS work (cozystack/cozystack#2729) that freshworks-oss does not have yet; the plan is to upstream it to freshworks-oss so it lands in the shared maintained fork rather than a cozystack-only one. ### Release note ```release-note feat(redis-operator): migrate from the archived spotahome/redis-operator to the actively-maintained freshworks-oss/redis-operator fork (same RedisFailover CRD, adds native spec.engine: Redis|Valkey) ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for injecting additional environment variables into the Redis Operator (including a default operator group id). * Added default configurable labels for the Redis Operator. * **Bug Fixes** * Preserves and merges existing Service labels and annotations during Redis Operator updates. * **Chores** * Updated the Redis Operator and Helm chart to the Freshworks-maintained 3.3.5 release, aligned chart/image sources, and simplified Helm chart retrieval. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
cd72d81bb4 |
Merge branch 'main' into feat/kubernetes-nodes-phase2b
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> |
||
|
Aleksei Sviridkin
|
0b8837e552 |
fix(telemetry): make the documented opt-out cover both collectors (#3791)
## What this PR does Telemetry is reported by two components — `cozystack-operator` (cluster facts) and `cozystack-controller` (application counts) — but only the operator had a reachable switch. The documented opt-out sets `cozystackOperator.disableTelemetry` on the `cozy-installer` chart. That chart deploys the operator; it cannot reach a package the platform deploys. `packages/system/cozystack-controller` has its own independent `disableTelemetry` key that nothing propagated to, so after an operator had followed the published procedure the controller kept posting `cozy_application_count` every 15 minutes. Nothing failed, and nothing indicated the opt-out was partial. Two changes: - `packages/core/platform` gains a `telemetry.disabled` key and threads it into the `cozystack-controller` component of the `cozystack.cozystack-engine` Package — the same mechanism the bundle already uses for cilium, multus and linstor values. - `packages/core/installer/values.yaml` declares `cozystackOperator.disableTelemetry`. It was only ever referenced from the template, so the `--set` target the documentation names did not appear in the chart's own values. Both keys default to `false`. Nobody who has not opted out sees a behaviour change. The opt-out remains two steps, because the two collectors are installed by two different charts. Collapsing it into one knob would mean the operator writing the value into the platform Package it does not own, which is a design change rather than a fix. `helm unittest` on the platform chart: 137 passed (133 before, plus the four added here). `helm lint` clean on both charts. ### Screenshots Not a UI change. ### Downstream repositories Trigger map walked against the diff. `packages/core/platform/values.yaml` changed, which the map routes to the website's hand-written platform-package table; the same change also makes the published opt-out procedure wrong, so the docs follow-up covers both. - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/pull/651 - [ ] [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(telemetry): the documented telemetry opt-out now covers both collectors. Setting `cozystackOperator.disableTelemetry=true` alone left `cozystack-controller` reporting application counts; the platform gains a `telemetry.disabled` key that silences it. Both default to false. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a platform telemetry opt-out setting, disabled by default. * Added a separate operator telemetry opt-out setting, also disabled by default. * Applying the platform setting now disables telemetry reporting for the relevant controller. * **Bug Fixes** * Ensured telemetry settings are correctly applied across supported platform variants while preserving existing configuration overrides. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrei Kvapil
|
c9ee4f6de8 |
test(platform): pin redis-operator upgradeCRDs=CreateReplace
Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
c7a3320bb4 |
fix(migrations): pin the engine and fail-close the CRD probe in migration 54
Two defects in migration 54's adoption pass: (1) The label landed before the image pins, so the operator adopted a labelled CR with empty image fields and rolled Redis to the fork's 7.2.4 default — rewriting dump.rdb into a format the later 6.2.6 chart pin can no longer load. Pin spec.redis.image/spec.sentinel.image to redis:6.2.6-alpine in the same pass, only where empty. (2) The CRD probe used a bare `get crd >/dev/null 2>&1` that could not tell NotFound from an API blip and stamped anyway; switch to migration 50's `--ignore-not-found -o name` so a real error aborts under set -e. Also self-contained the renumber note and qualified the resource. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
caf9511365 |
fix(platform): CreateReplace the redis-operator CRDs on upgrade
helm-controller skips CRD upgrades by default, so an upgraded cluster would keep the old spotahome schema and prune the fork's spec.engine field on admission. The schema diff is additive, so CreateReplace is safe. Also refresh the harbor e2e wait comment for the freshworks operator. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
0a3a3bfa77 |
fix(migrations): adopt existing RedisFailovers onto the operator group
The freshworks-oss operator watches RedisFailovers through a server-side operator-group label selector, so a CR without the label is silently dropped from its informer. The chart templates now stamp the label, but an existing CR only gets it when its own HelmRelease next upgrades — after the operator rolls — leaving a window with no healing, and a suspended/stuck HelmRelease never gets it at all. Label every RedisFailover in a pre-upgrade migration, ahead of the new operator. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Aleksei Sviridkin
|
b76cb2091c |
test(e2e): record the runner VM shape and region in the e2e report
An e2e lane asks for a runner class, not for a machine. A flex shape is filled from whatever capacity the pool has at the time, so one lane can be served by two processor generations, and the region is a preference with a fallback. None of that reached the report, so a red run and a green one could not be compared by the hardware they ran on. The read is taken on the runner VM, before the sandbox container is created, because the metadata service answers on a link-local address the container has no guaranteed route to: an "unreachable" answered from in there would look exactly like a capture that had tried. It is wired into the sandbox apply target rather than into a workflow step, so every lane picks it up through `make prepare-env` on both the passing and the failing path, and cozyreport folds the file into the artifact from inside without going near the endpoint. The keys are fetched one at a time and the root instance document never is. That document carries the instance and compartment OCIDs, the image OCID, the display name, the hostname and the user metadata bag, and the report is a public artifact, so asking per key makes that property something the wire settles rather than something a filter has to be trusted for. It also matches where the shape sizing is published: it has its own subpath and is not in the root document at all. From the sizing object only the numbers are kept, so the rule holds for keys nobody has seen yet, and an answer that is not one scalar is named rather than printed. Every read carries a wall-clock ceiling, since a read that never returns costs the step its whole budget without reaching any exit status, and the first key doubles as the probe so a machine that is not an OCI instance pays one ceiling rather than one per key. Nothing here is fatal: this runs inside the one retried step in the job, so a runner with no metadata service leaves an "imds unreachable" line and an exit status of 0. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Myasnikov Daniil
|
42bd45a87c |
fix(platform): make etcd-application depend on etcd-operator
extra/etcd renders kind: EtcdCluster from etcd-operator.cozystack.io/v1alpha2, but cozystack.etcd-application declared no dependsOn edge to cozystack.etcd-operator -- the only operator-backed application source in the tree that did not. The edge has been absent since the sources were first added. Two consequences, one per side of the graph. On install this is a latent ordering bug. Without the edge the etcd-rd HelmRelease registers the ApplicationDefinition as soon as cozystack-engine is up, so a tenant can create an Etcd before the operator exists and its HelmRelease then fails with `no matches for kind "EtcdCluster"`. hack/select-install.sh's forward closure for the etcd suite omitted the operator for the same reason, which would install the suite against a cluster with no CRD to apply to. On test selection cozystack.etcd-operator reached no runnable suite, so every change to the operator escalated to all 21 suites -- 5 of the last 150 merged pull requests. The etcd suite exists; only the edge that makes it reachable from the operator was missing. Scope of the ordering change: both Packages are emitted unconditionally by the same bundle (templates/bundles/system.yaml, under the single bundles.system.enabled guard, in every variant), so no supported configuration has one without the other. An admin who lists cozystack.etcd-operator in bundles.disabledPackages while keeping the app now leaves it DependenciesNotReady -- the same exposure postgres, mariadb, kafka and redis already carry, with Package.spec.ignoreDependencies as the escape hatch. select-install.sh --validate confirms the edge adds no cycle and nothing dangling. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
IvanHunters
|
f9065a9455 |
fix(migrations): correct the non-scalar-field skip remedy
The non-scalar scalar-field skip branch told the operator to restore the field and 're-run adoption', but migration 54 stamps the version unconditionally and does not re-run, so that remedy is a dead end. Align it with the sibling skip branches: restore the scalar, then recreate the pool, and state plainly that the migration has already stamped and will not re-run. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
8815468c9b |
fix(migrations): route non-scalar nodeGroup scalar-fields through warn+pin+skip
Migration 54 copied per-group scalar fields (storageClass, instanceType, diskSize, minReplicas, maxReplicas, logSerialConsole, maxUnhealthy, nodeStartupTimeout) verbatim into the child HelmRelease values. The nodeGroups map value was never fully schema-constrained, so a kubectl/dashboard edit could store one of these as an object or array; copied through, it fails the KubernetesNodes schema on the child install and leaves that child release stuck Failed. Validate the scalar fields before building the child values and route a non-scalar value through the same warn + pin + skip contract as the invalid name, overflow and non-object branches: pin the pool's objects prune-proof and skip adoption, so the pool keeps running unmanaged until the operator restores the stored value. Container-typed fields (roles, resources, gpus, kubelet) are left untouched. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Andrei Kvapil
|
c3cead8f64 |
fix(telemetry): make the documented opt-out cover both collectors
Telemetry is reported by two components, cozystack-operator (cluster facts) and cozystack-controller (application counts), but only the operator had a reachable switch. The documented opt-out sets cozystackOperator.disableTelemetry on the cozy-installer chart, which cannot reach a package the platform deploys, so cozystack-controller kept reporting after an operator had followed the procedure — silently, with nothing to indicate the opt-out was partial. The platform now exposes telemetry.disabled and threads it into the cozystack-controller component of the cozystack-engine Package, the same way the bundle already threads cilium, multus and linstor values. The installer key is also declared in values.yaml: it was only ever referenced from the template, so the documented --set target did not appear in the chart's own values. Both keys stay false by default; this changes no behaviour for anyone who has not opted out. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com> |
||
|
IvanHunters
|
9ec5f8ceca |
fix(migration): carry logSerialConsole through worker-pool adoption
Migration 54's adopt pick-list copied every worker NodeGroup field that feeds the content-hashed KubevirtMachineTemplate except logSerialConsole. That field renders into the same hashed template (kubernetes-nodes nodegroup.yaml), so an adopted pool that set logSerialConsole would render a KMT without it, change the content-hash, rename the KMT, and roll every worker VM of the pool on upgrade (and silently drop the setting). Add logSerialConsole to the pick-list (guarded by has(), so pools without it are unaffected) and assert it carries through. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
b4a2f6f791 |
Merge remote-tracking branch 'origin/main' into pr-3315-lgtm
# Conflicts: # api/apps/v1alpha1/kubernetes/types.go # api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go # hack/e2e-chainsaw/_lib/run-kubernetes.sh # packages/apps/kubernetes-nodes/tests/render-parity.sh # packages/apps/kubernetes/README.md # packages/apps/kubernetes/templates/cluster.yaml # packages/apps/kubernetes/templates/talos/talos-reconcile-job.yaml # packages/apps/kubernetes/tests/talos_templates_test.yaml # packages/apps/kubernetes/values.schema.json # packages/apps/kubernetes/values.yaml # packages/system/kubernetes-nodes-rd/cozyrds/kubernetes-nodes.yaml # packages/system/kubernetes-rd/cozyrds/kubernetes.yaml |
||
|
Aleksei Sviridkin
|
f4d812cfb9 |
fix(velero): apply CRD updates on upgrade via CreateReplace
velero evolves its CRD schema between versions; v1.18 adds the Queued and ReadyToStart backup phases. Helm never upgrades CRDs shipped in crds/, the generated HelmRelease left spec.upgrade.crds at the Skip default, and the package disables the chart's upgrade-crds Job, so live CRDs stayed frozen at first-install state: the server upgrades, the apiserver rejects the new phases, and backups sit in New while the HelmRelease stays green. Opt the velero PackageSource into CreateReplace, consistent with mongodb-operator, add a platform test pinning the policy, and correct the stale Job-disable rationale in the package. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
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
|
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
|
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
|
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> |
||
|
IvanHunters
|
c010745e8e |
Merge branch 'main' into feat/kubernetes-nodes-phase2b
Bring the worker-pool split branch up to date with main (111 commits), picking up the node-join and CI fixes it was missing: the 18m tenant node-join deadline, guest-fsync-off on ephemeral CI disks (#3455), kubevirt-csi client rate-limit raise (#3428), the Cilium ingress-IP race guard (#3430), and etcd-operator v0.5.4. Conflict resolution: main's #3535 (render the talos-reconcile Job for the default md0 group) modified packages/apps/kubernetes/templates/talos/ talos-reconcile-job.yaml, which this branch deletes because the worker split moves that Job into the kubernetes-nodes chart. Kept the parent template deleted and dropped the parent-scoped test talos_reconcile_nodegroups_test.yaml: the empty-nodeGroups md0-default gap it guards cannot occur in the per-pool child chart, and migration 54 materialises the implicit md0 into an explicit child HelmRelease. targetVersion resolves to 55 (migration 54 on top of main's 53). Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
e969d71379 |
chore(platform): use bash shebang for migration 54
Migration 54 uses set -o pipefail, a bashism. The migrations image ships bash (alpine:3.24 + apk add bash) and sibling migrations 45/47/50/51 already use #!/bin/bash for the same reason. Align 54 so the interpreter matches the options it sets and cannot silently no-op pipefail if /bin/sh were ever a non-bash shell. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
cb56f140ad |
fix(platform): pin keep on a foreign-owned worker object before skipping it
The foreign-owner skip branch warned and returned without applying helm.sh/resource-policy=keep, on the premise that a foreign release-name means the parent will not prune the object. That premise is unsound: Helm computes the upgrade prune set from the release's own stored old manifest minus the new manifest and gates deletion only on resource-policy=keep, not on the live meta.helm.sh/release-name. The worker object is in the parent's old pre-split manifest, so the control-plane-only re-render would prune it and cascade-destroy the tenant's worker VMs. Pin keep before returning, exactly as every sibling skip branch does, while still refusing to rewrite the foreign release-name (which could corrupt that release). Also correct the printed remedy: migration 54 stamps 55 unconditionally and never re-runs, so recovery is manual, not a re-run. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
4f323bfce5 |
fix(platform): pin live worker pools when migration-54 sees non-object nodeGroups
The top-level non-object nodeGroups branch warned and skipped the cluster without pinning any objects, unlike every sibling skip branch (RFC-1123 invalid key, 53-char overflow, per-group non-object) which pins first. If a cluster's pools were rendered under an earlier good revision and its nodeGroups was later mangled out-of-band into an array/string/number, the worker objects stay live but un-pinned; the parent's control-plane-only re-render then prunes them, destroying the tenant's running worker VMs. The declared pool keys are unreadable in this shape, so discover the live pools from their KubevirtMachineTemplates (kubernetes-<cluster>-<group>-<hash>) and pin each via pin_pool_objects before skipping adoption. Fail-closed through list_kmts so a transient read never masquerades as no-pools. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
2d73043518 |
fix(platform): scope migration-54 foreign-owner refusal to one object
A worker object whose meta.helm.sh/release-name is neither the parent nor the child release made adopt_one exit 1. Under set -e that aborts the whole per-cluster pre-upgrade hook, so run-migrations.sh never stamps the version and one tenant's out-of-band-edited annotation deadlocks the platform upgrade for every tenant, with no path out. This is the same hostile-stored-shape class the non-object, overflow and RFC-1123-invalid branches were already reshaped to warn-and-skip rather than deadlock the fleet. A foreign owner also means the parent release no longer owns the object, so its control-plane-only re-render will not prune it: skipping is prune-safe. Refuse to adopt (rewriting a foreign release-name could corrupt that release) but scope the blast radius to the single object: warn, skip it, document the operator remedy, and let the run complete. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
myasnikovdaniil
|
3f967ec7e0 |
fix(platform): restore the isp-hosted dependency closure (#3391)
## Problem Fixes #3376. The `isp-hosted` preset shipped `bundles.system.enabled: false` while `paas` and `naas` were enabled, so it enabled an app catalog whose dependency closure it simultaneously disabled. Every emitted Package sat in `DependenciesNotReady`, the `*-rd` HelmReleases failed with `no matches for kind "ApplicationDefinition" in version "cozystack.io/v1alpha1"`, and the `cozystack-platform` HelmRelease never went Ready. Rendering the preset at `origin/main` shows the scale: **24 emitted Packages, 24 of them blocked**, by 6 missing dependencies — two more than the issue lists (`cert-manager` and `prometheus-operator-crds` are also absent). `isp-full` renders 75 Packages with a fully satisfied closure, which is a clean control. ## Root cause `templates/bundles/system.yaml` is gated end to end on `bundles.system.enabled`, but it carries two separable things: a variant-selected **data-plane head** (kubeovn/cilium/linstor on `isp-full`, a noop networking Package on `isp-hosted`) and a variant-agnostic **control-plane body** — `cozystack-engine`, the sole source of the `application-definition-crd` component, plus `cert-manager`, `prometheus-operator-crds`, `postgres-operator` and `objectstorage-controller`. "Hosted" means the host already provides CNI, storage and virtualisation, so only the data-plane head should be dropped. Disabling the whole bundle also deleted the control-plane body that every variant needs. Three things show the disable was the mistake rather than the intent: the preset still sets `variant: "isp-hosted"`; `system.yaml` contains an `isp-hosted` branch (noop networking plus default scheduler) that could never render; and `iaas.yaml` already refuses to render on this variant, encoding the same "system bundle minus iaas" contract. The preset has never worked — it was born this way in `2d022e38e` (January 2026), the refactor that replaced `bundles/paas-hosted.yaml`, whose contents confirm hosted was always meant to ship the engine, `cert-manager` and `prometheus-operator-crds`. ## Changes 1. **`values-isp-hosted.yaml`** — enable the system bundle. Hosted now emits 53 Packages with every `dependsOn` satisfied, installing 59 components against the 60 of the pre-refactor `paas-hosted` bundle, with no LINSTOR, KubeVirt, CAPI or Kamaji leakage. 2. **Move `cozystack.bucket-application`** from the iaas bundle into the system bundle. Its own `dependsOn` closure is `networking` + `objectstorage-controller` + `cozystack-engine`, none of them iaas, while `cozystack.backupstrategy-controller` — a *default* package of the system bundle — hard-depends on it. Any system-on/iaas-off configuration therefore wedged `backupstrategy-controller`; both `isp-full` presets masked this by enabling iaas. This is a second, independent bug not reported in the issue. 3. **Guards** — `paas`, `naas` and `iaas` now also require `bundles.system.enabled`. The pre-existing checks looked only at `bundles.system.variant`, which keeps its value when the bundle is disabled, so they passed on exactly this configuration. `iaas` had the identical hole (12 Packages, 6 unsatisfied deps). This turns a silent multi-hour `DependenciesNotReady` hang into a render-time failure that names the remedy. 4. **Two helm-unittest suites** — one pinning the dependency closure the catalog needs, one pinning the guards. ## Verification - All four operator-registered variants render dependency-closed: `default` 0 Packages, `isp-hosted` 53, `isp-full` 75, `isp-full-generic` 75. The two `isp-full` variants are byte-identical before and after, so the bucket move is a no-op for them. - Hosted is a strict subset of full with zero data-plane leakage, and its exclusions match the pre-refactor hosted bundle. - 23 suites / 97 tests pass in `packages/core/platform`; repo-wide `hack/helm-unit-tests.sh` is green. - Every change was **mutation-tested** — reverting each one in turn makes the suite fail, so the tests are not vacuous. - The workaround documented in #3376 (`paas`/`naas` disabled) still renders cleanly; the new guards do not fire on it. - Hosted runtime spot-checks: `cozystack-api` is a Deployment with only a *preferred* control-plane affinity plus blanket tolerations, and `cozystack-scheduler` uses its `default` variant (plain Deployment, no LINSTOR extender) — both schedule fine on a cluster with no control-plane nodes. ## What this does not fix This restores the dependency closure. It is **not** by itself sufficient for a fully converged hosted install. Review surfaced further hosted-unaware defaults that the dead bundle had been masking, filed separately: - #3387 — `apps/tenant` renders 14 `cilium.io/v2` policies with no `.Capabilities` guard, so `tenant-root` fails on a non-Cilium cluster. This is the blocker for actual hosted convergence, and it needs a deliberate decision rather than a reflexive capability guard, because those policies implement tenant network isolation. - #3388 — `extra/monitoring` defaults `logsStorages` to the LINSTOR-only `replicated` StorageClass. - #3389 — `bundles.disabledPackages` can still break the closure with no render-time error; the guards here cover the bundle flags only. - #3390 — nothing in CI exercises `isp-hosted`, which is why all of the above accumulated undetected. ## Upgrade note Existing `*-application` Package CRs carry `helm.sh/resource-policy: keep`. An operator who applied the #3376 workaround but did not also delete the retained Package CRs (as that issue instructs) will find them install once this change makes their dependencies Ready, since the reconciler re-enqueues dependents on readiness. Deleting the unwanted Packages before upgrading avoids that. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added fail-fast validation that requires the **System bundle** when enabling **IaaS**, **NaaS**, or **PaaS** to prevent incomplete catalogs. * Updated bundle composition so the **bucket application** is provided by the **System bundle**. * Enabled the **System bundle** by default for the `isp-hosted` variant. * **Bug Fixes** * Improved error messaging to clearly indicate the required bundle/variant configuration. * **Tests** * Added Helm unittest coverage for rendering-gate failures, dependency/package expectations, and hosted data-plane exclusions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov DaniilandClaude Opus 4.8
|
65cd5f629c |
fix(ci): gate release tags/pushes off under OCI_EXPORT_DIR (defense-in-depth)
Belt-and-suspenders for the B2-class trap lexfrei flagged as latent in
image-tags. PUBLISH_* are 0 on fork PRs, so today the export path never emits a
versioned/floating tag — but if it ever did:
- image-tags: multiple --tag under `--output type=oci` write >1 manifest into
the archive's index.json, and `skopeo copy oci-archive:<f>` then refuses it
("more than one image in oci"). Suppress the versioned/floating tags when
OCI_EXPORT_DIR is set (macro-level $(if …), so `make -n` prints only the
taken branch).
- packages/core/talos: gate its versioned/floating `skopeo copy docker://…`
release copies on an empty OCI_EXPORT_DIR too.
The release (non-export) path is unchanged — versioned + :latest still emitted.
Covered by a new common-envs_test.bats case that forces PUBLISH_*=1 under
export.
Reported by gemini-code-assist on #3262.
Assisted-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
||
|
Myasnikov DaniilandClaude Opus 4.8
|
ab14922495 |
fix(ci): export talos image as an OCI archive on fork builds (#3257)
packages/core/talos `image-talos` pushed to the registry via `skopeo copy docker://…` unconditionally — it is not a buildx call, so neither OCI_EXPORT_DIR nor PUSH reached it. On a fork PR the registry login is skipped, so this was an anonymous push that died with `denied`, collapsing build-talos → finalize and the whole chain. Under OCI_EXPORT_DIR, copy the installer to oci-archive:<dir>/talos.oci.tar instead of pushing; keep the docker:// push otherwise. A make-level $(if …) is used (not a shell if/else) so `make -n` prints only the taken branch. Refs: #3257 Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
myasnikovdaniil
|
5d420475f4 |
refactor(ci): drop the vestigial /tmp workspace from e2e (both workflows) (#3454)
## What this PR does
The E2E job's `if: always()` cleanup ran `rm -rf /tmp/$SANDBOX_NAME`
with no check that `SANDBOX_NAME` was ever set. `Set sandbox ID` carries
no condition, so it inherits the implicit `success()` every
unconditional step gets: any failure ahead of it — a conflicting
`pr.patch`, a `download-artifact` miss, `Checkout code`, the
release-path `curl` — or a cancellation that reaches the job after it
has started (the job condition is `!cancelled()`) skips it, and
`SANDBOX_NAME` is never written to `$GITHUB_ENV`. The `if: always()`
steps then run anyway and interpolate the empty value; `Remove
workspace` is the destructive one, expanding to `rm -rf /tmp/` and
failing the cleanup step against root-owned entries in the runner's
`/tmp`. This is not a "re-run skips the setup" bug: a re-run restarts
the job at step one and re-runs the unconditional `Set sandbox ID` — the
trigger is incomplete initialization, by any of the routes above.
The `/tmp/$SANDBOX_NAME` workspace is a vestige of the retired multi-job
self-hosted-runner design, where several `runs-on: [self-hosted]` jobs
shared no state and rendezvoused through a `sha256(REPO:WORKFLOW:REF)`
key, with a per-run `cp -r` of the checkout into `/tmp`. Since CI moved
onto the ephemeral pool (`runs-on: oracle-vm-32cpu-128gb-x86-64`) it is
a single job: there is no cross-job rendezvous and the checkout is never
reused across runs, so copying it into `/tmp` and removing it afterwards
is dead weight. (An earlier review justified the change by "wiping the
entire `/tmp` of the self-hosted runner" — that describes the topology
we retired; the hazard is real, but not for that reason.)
So this drops the workspace rather than guarding the `rm`. Both the PR
(`pull-requests.yaml`) and nightly (`nightly.yaml`) E2E jobs now run in
`$GITHUB_WORKSPACE`: the `Prepare workspace` and `Remove workspace`
steps are gone, the `cd /tmp/$SANDBOX_NAME` prefixes are dropped, and
the upload paths point at `_out/…` instead of
`/tmp/${SANDBOX_NAME}/_out/…`. That takes the empty-value expansion out
of the `cd`s, the stray `mkdir`, the upload paths and the destructive
`rm` in one move, across both workflows rather than one step of many.
`Set sandbox ID` stays: the job still reads `$SANDBOX_NAME` directly
(failure diagnostics, the chainsaw `docker cp`, teardown, and every
`make SANDBOX_NAME=…` argument) — the container needs a name, just not a
directory.
It also removes the dead code the single-runner retirement left in
`packages/core/testing/Makefile`: the world-writable `mkdir -p
/tmp/${SANDBOX_NAME}` + `chmod 777` in the `apply` target (they backed a
`-v /tmp/${SANDBOX_NAME}:/workspace/hosttmp` bind mount that was later
removed — nothing under `images/e2e-sandbox/` references
`/workspace/hosttmp`), and the `-e SANDBOX_NAME=${SANDBOX_NAME}` passed
into the container (nothing under `images/e2e-sandbox/` reads it). Every
other use of `SANDBOX_NAME` — the container name and the `make`
arguments — is kept.
Note this cannot make a job with an empty `SANDBOX_NAME` go green: such
a job already failed initialization or was cancelled, so its conclusion
is fixed before cleanup runs. It removes a second, destructive failure
the old cleanup piled on top, and it does not change whether re-runs
work in general — they do.
The diagnosis of the real trigger, the corrected blast radius, and the
scope across both workflows plus the Makefile are credited to the
maintainer review on this PR.
### Evidence
Run [30067779342 attempt
3](https://github.com/cozystack/cozystack/actions/runs/30067779342/attempts/3)
exhibits the mechanism: `Download Talos image (regular PR)` failed on
`GetSignedArtifactURL` with a 404, which skipped `Set sandbox ID`, and
the `always()` steps then ran on an empty value — `Collect chainsaw
report` created `/tmp/_out`, the uploads resolved to `/tmp//_out/…`, and
`Remove workspace` did:
```
rm: cannot remove '/tmp/systemd-private-…-systemd-timedated.service-1oADZP': Operation not permitted
rm: cannot remove '/tmp/.X11-unix': Operation not permitted
##[error]Process completed with exit code 1.
```
The step ran unprivileged in `/home/ubuntu/_work/…`, so all it could hit
was `Operation not permitted` on root-owned entries in the sticky `/tmp`
— it deleted nothing. That run is a later, independent occurrence (it
started well after this PR was opened), not this change's motivating
case. The 404 that triggered it is a separate, non-deterministic problem
— the artifact listing succeeded and only the signed-URL call failed
~190 ms in, `download-artifact` treats 404 as non-retryable, and attempt
2 of the same run fetched the artifact fine — and it deserves its own
issue rather than being folded in here.
### Screenshots
Not applicable — CI workflow change only.
### Downstream repositories
The change is confined to this repo's E2E CI
(`.github/workflows/pull-requests.yaml`,
`.github/workflows/nightly.yaml`) and its testing Makefile
(`packages/core/testing/Makefile`). No downstream repository consumes
it.
- [x] No downstream repository is affected by this change
### Release note
```release-note
refactor(ci): drop the vestigial /tmp/$SANDBOX_NAME workspace from the E2E PR and nightly jobs — they now run in $GITHUB_WORKSPACE, removing an unguarded `rm -rf /tmp/$SANDBOX_NAME` cleanup that failed the job on cleanup when SANDBOX_NAME was empty
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved end-to-end test environment setup and sandbox cleanup.
* Updated test reporting and artifact collection for more reliable
results.
* Ensured OpenAPI and full-suite tests run from the correct prepared
environment.
* **Chores**
* Simplified container and workflow configuration for sandbox-based
testing.
* Clarified checkout token handling in nightly workflows.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Andrei Kvapil
|
ebeeab77aa |
docs(platform): correct the computeplane dependsOn and variant comments
The dependsOn comment claimed the kubernetes-application edge orders the addon artifacts the wrapped chart references; variant dependsOn only gates install: components — the operative effect is ordering the computeplane-rd install so the ComputePlane kind is not registered before the kubernetes machinery exists. Also document why the variant is named kubevirt. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
aa5198b758 |
fix(platform): add cozystack-engine dependency to the computeplane package
The computeplane-rd component installs an ApplicationDefinition, whose CRD is served by cozystack-engine — hack/check-applicationdefinition-crd-ordering.bats enforces the edge for every package with a system/*-rd component. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
4c3262a4ee |
feat(computeplane): deliver ComputePlane as a tenant module wrapping the kubernetes app
Add the operator-owned computeplane tenant module per the revised design (cozystack/community design-proposals/compute-plane): an extra/computeplane chart that provisions a single-tenant, Cozystack-managed cluster for untrusted-code apps by deploying the ordinary apps/kubernetes chart with operator-fixed values (fluxcd, cert-manager, ingress-nginx Proxied). The package re-declares apps/kubernetes as its own source-only component, so the module is self-contained; the cluster release is named computeplane so Kamaji writes the admin kubeconfig to computeplane-admin-kubeconfig — the contract consumed by placement: ComputePlane applications. The tenant holds no admin kubeconfig and can set cluster shape (nodeGroups) only. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
78fc3f9178 |
refactor(ci): drop the vestigial /tmp workspace from e2e (both workflows)
The E2E job once ran as several state-sharing self-hosted-runner jobs that rendezvoused through a sha256(REPO:WORKFLOW:REF) key and a per-run /tmp/$SANDBOX_NAME copy of the checkout. Since the move to the ephemeral pool it is a single job: there is no cross-job rendezvous and the checkout is never reused, so copying it into /tmp and removing it afterwards is dead weight. Run every step in $GITHUB_WORKSPACE instead: drop the Prepare/Remove workspace steps and the `cd /tmp/$SANDBOX_NAME` prefixes, and point the upload paths at _out/. This deletes the unguarded `rm -rf /tmp/$SANDBOX_NAME` rather than guarding it: when an upstream step fails or the job is cancelled, the unconditional `Set sandbox ID` is skipped, SANDBOX_NAME stays empty, and the `if: always()` steps interpolate it — the removal expanding to `rm -rf /tmp/` and failing the cleanup step. `Set sandbox ID` stays, since the container still needs a name. Apply the same treatment to nightly.yaml, and drop the Makefile dead code the single-runner retirement left behind: the world-writable mkdir/chmod of /tmp/$SANDBOX_NAME (backed a bind mount removed long ago) and the `-e SANDBOX_NAME` container env (nothing in the e2e-sandbox image reads it). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Aleksei Sviridkin
|
8bffa34b54 |
feat(platform): add opt-in in-cluster east-west encryption toggle (#2984)
## What this PR does Adds `networking.encryption.enabled` to the platform chart — a single operator-facing opt-in, off by default, that turns on in-cluster (east-west) transport encryption for the active datapath. When enabled, pod-to-pod traffic across nodes is encrypted at the network layer regardless of whether the workload implements TLS: defense-in-depth for the whole mesh without per-application changes. The default `isp-full` bundle selects the `kubeovn-cilium` datapath (and `isp-full-generic` selects `kubeovn-cilium-generic`), where KubeOVN owns the datapath and the overlay is a Geneve tunnel. The toggle therefore maps to KubeOVN's overlay IPsec (`kube-ovn` `ENABLE_OVN_IPSEC`). KubeOVN's own controller provisions the IPsec CA and per-node certificates through the Kubernetes CSR API, so no cert-manager wiring is needed. This complements, and does not replace, application-level / end-to-end TLS: in-cluster encryption never reaches an external client and gives a tenant no verifiable endpoint certificate. The values comments document the per-datapath scope — including that Kilo's WireGuard mesh (`meshGranularity: location`) covers only cross-location traffic and leaves the intra-location gap this toggle targets, and that the Cilium-native WireGuard/IPsec path on the `cilium` / `cilium-generic` variants is not reached because the system bundle does not select those variants — plus the MTU/throughput overhead. Scope: only the default KubeOVN datapath is wired through the platform toggle, which is what the system bundle actually selects. Wiring the Cilium-native path would require the bundle to expose cilium-only variant selection; that is out of scope here and called out in the values comments. Validation: `helm unittest` on the platform chart passes (66 tests, 7 new) covering on / off / absent / null and verifying the IPsec flag deep-merges without dropping the existing `kube-ovn` overrides, for both `isp-full` and `isp-full-generic`. Closes #2977 Part of #2811 ### Screenshots N/A — no UI changes. ### Release note ```release-note feat(platform): add opt-in `networking.encryption.enabled` (off by default) that enables in-cluster east-west transport encryption for the default KubeOVN datapath via overlay IPsec; complements, and does not replace, application-level TLS. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Introduced opt-in in-cluster network encryption using KubeOVN IPsec for east-west traffic protection. * Added `networking.encryption` configuration toggle (disabled by default) with full backward compatibility. * **Tests** * Added comprehensive test coverage for network encryption configurations across system variants and edge cases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
eeee87f0b1 |
Merge branch 'main' into feat/kubernetes-nodes-phase2b
Resolve the migrations pin conflict in packages/core/platform/values.yaml: keep targetVersion 55 (this branch owns migration 54) and take the newer platform-migrations image v1.6.0 from main. The in-tree image pin intentionally lags targetVersion until the release retag, and run-migrations.sh fails loud if a targeted migration is missing from the image, so the lag is safe by design. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Timofei Larkin
|
730d006ce8 |
fix(platform): skip pinning names no object can have
The pin path composed "kubernetes-<cluster>-<group>" and read it back, but a group key that cannot form a valid object name (a slash, a space, an underscore...) provably never had objects applied under it -- the old chart's apply would have been rejected by the apiserver. Worse, kubectl rejects a name containing '/' client-side, deterministically, so pin_keep's fail-closed read classified it as a real failure and the migration exited 1 on every retry: a permanent fleet-wide pre-upgrade deadlock over a pool that cannot exist. Lean on the invariant instead of the read: pin_pool_objects statically checks the composed name against the DNS-1123 subdomain shape and skips with a note when it cannot name an object -- nothing to pin, no kubectl call to misread. Keys that are label-invalid but still valid object names (e.g. dotted) may have live objects and keep today's pin behavior; the pin-path transient-error test now uses a dotted key so it still exercises a genuine read. The fake kubectl reproduces real kubectl's client-side slash rejection, and a new bats case pins the skip: a "pool/a" key must warn, pin nothing, apply nothing, and still reach the version stamp. Red against the previous script, which deadlocked at the first pin read. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
bdadb3ff04 |
fix(platform): detect absence via --ignore-not-found
The stderr-based classification this branch introduced had two holes of its own. Capturing stderr into the data variable (2>&1) contaminates the SUCCESS path: kubectl prints warnings on successful reads (API deprecation headers -- deterministic for a deprecated GVK, and CAPI v1beta1 is one -- plus client-side throttling notices), so the warning glued onto the owner annotation would trip adopt_one's foreign-owner refusal on every tenant and permanently deadlock the pre-upgrade hook. And the *"not found"* pattern over-matches: a transient conversion-webhook outage fails a read with '... service "capi-webhook-service" not found', which the classifier would treat as absence -- skipping the keep pin and re-opening the exact fail-open prune hazard this branch exists to close. Detect absence with --ignore-not-found instead: a missing object is SUCCESS with empty output, so a non-zero exit can only be a real failure and aborts before the version stamp, with no stderr parsing at all. adopt_one reads -o json and extracts the owner annotation with jq (already a dependency), keeping absence distinct from present-but- unannotated; pin_keep reads -o name. list_kmts still needs stderr for the one legitimate classification -- the two deterministic missing-CRD messages -- so it captures stderr to a temp file, never into data, and no longer accepts NotFound-shaped text for a list. The fake kubectl honors --ignore-not-found the way real kubectl does, gains FAKE_WARN_STDERR (success-path stderr warnings) and FAKE_GET_FAIL_MSG (transient error with arbitrary text). Two new bats tests, each red against the previous commit: a deprecation warning must not break adoption or fake a foreign owner, and a failure whose message contains "not found" must still abort without stamping. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
f8258c8859 |
fix(platform): fail migration 54 reads closed
kubectl exits 1 for NotFound and for transient server failures (timeout, 429, etcd hiccup) alike, and migration 54 treated every non-zero read as "object absent". A transient error while reading a worker object would skip the keep pin and the ownership rewrite, let the migration complete and stamp version 55 (so it never re-runs), and the parent's control-plane-only upgrade would then prune the un-pinned live MachineDeployment -- deleting every running worker VM in the pool. The doubly-masked KubevirtMachineTemplate listings (2>/dev/null plus `|| true`) had the same shape: a failed list adopted or pinned zero KMTs, permanently stranding a preserved older-revision KMT still referenced by an in-flight MachineSet, since the child chart only re-emits KMTs already annotated with its own release name. The `cur=$(... || true)` owner read additionally bypassed the foreign-owner refusal guard. Classify reads by stderr, the migration-47 precedent: NotFound means genuinely absent and is skipped; anything else exits 1 so run-migrations.sh leaves CURRENT_VERSION at 54 and retries on the next upgrade. adopt_one and pin_keep fold the existence check into the annotation read (one request instead of two); KMT listings go through a capture-first list_kmts helper because `for x in $(...)` discards the substitution's exit status even under set -e, leaving `|| true` to cover only the empty-grep-match case, mirroring the unpin hook. The child-HR existence check stays deliberately tolerant: its fallback is an idempotent apply that itself fails closed. adopt_one now also stamps meta.helm.sh/release-namespace so an object whose Helm annotations were stripped out-of-band cannot fail the child's ownership validation. The fake kubectl now reports absent objects the way the real apiserver does (NotFound on stderr, exit 1) and gains FAKE_GET_FAIL to inject transient errors; four new bats tests pin the classification: NotFound still completes and stamps, while a transient error on the adopt path, the KMT listing, or the pin path aborts before the version stamp. Each red against the previous script. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
IvanHunters
|
928df3c2d3 |
fix(platform): harden migration 54 against non-object and whitespace nodeGroups
The RFC-1123 key guard added in the previous round closed hostile map
keys, but migration 54 still trusted the shape of nodeGroups and of each
group value, and word-split keys on whitespace. Because it runs as the
platform pre-upgrade hook, either gap can deadlock the upgrade for every
tenant off one hand-mangled parent HelmRelease (kubectl edit bypasses the
aggregated API schema, the same vector the key guard addresses).
Shape: a non-object nodeGroups (array/string/number) crashes jq under
set -e (keys[] on an array yields indices, then .[$idx] fails); a
non-object group value aborts the value-mapping jq on `$grp | has(...)`.
Normalize both: skip the cluster for a non-object nodeGroups map, and
route a non-object group value through the same warn + pin + skip path as
an invalid name. {} / null still fall through to the implicit md0 default.
Whitespace: an unquoted `for group in $(... keys[])` word-splits (and
globs) a key like 'my pool' into fragments that each pass the RFC-1123
guard, fabricating garbage child releases. Iterate keys newline-safely so
the intact key reaches the guard. Fail-closed exit semantics are
preserved through the while pipeline.
Covered by three new bats fixtures in migration-54-adopt-path.bats, each
red against the current script and green with this change.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|