cozystack

0 Fork 0

mirror of https://github.com/cozystack/cozystack.git synced 2026-11-03 10:37:21 +09:00

Author	SHA1	Message	Date
Myasnikov Daniil	c6ea951b7d	ci(backport): open backport PRs with the cozystack-ci App token Backport PRs opened by the bot sat waiting for a maintainer to click "Approve and run workflows" before any check would start. Nine were parked that way on 2026-08-18, and nothing surfaces them, so the backports that carry fixes to the release lines were the ones sitting still. The cause is not this workflow but the repository's policy: GET /repos/cozystack/cozystack/actions/permissions/fork-pr-contributor-approval {"approval_policy":"all_external_contributors"} github-actions[bot] is external under that policy, so every run triggered by a PR it authors lands in action_required, even though the branch is same-repo (isCrossRepository: false). An org-installed App is not an external contributor. #3550, opened by cozystack-ci[bot] through this same App token, drew its full check set unapproved, including a 54-minute E2E job. So mint the App token and hand it to backport-action, matching the pattern already used by retention, cut-prerelease, promote-rc, tags, pull-requests and pull-requests-release. Same action pin and the same owner: cozystack as the other ten call sites. Scope worth being precise about: backport-action documents github_token as "used to create and label pull requests and to comment", so this moves PR authorship and nothing else. The git push still runs on the credentials actions/checkout persists, so a backport carrying a .github/workflows/ change is still rejected, and that stays true until the App is granted workflows: write -- a permission that would let anything able to trigger the App rewrite CI and reach every secret, and one this change deliberately does not ask for. The token step is gated on the same already_merged guard as the two steps around it, so a backport the guard has already decided against mints no credential. That makes three gated steps where the contract test counted exactly two; it now counts three, and the count still fails if any single gate is dropped. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-18 12:52:48 +05:00
Aleksei Sviridkin	b1b8cbecc2	test(e2e): price the layers above the sandbox on the node-join window Every CPU reading on this failure path has a subject inside the sandbox: a cgroup, a tenant worker's threads, a sandbox node's own /proc/stat. Each of them can say a guest was given its ticks while getting no work done, and none can say what the layers above spent. That is where this suite's own tax is paid: the sandbox nodes' guest time is charged to the runner kernel as user time, and whatever the runner VM loses to the machine hosting it is charged to nobody the sandbox can see. Two readings close that. /proc/stat is not namespaced, so reading it from the sandbox container reads the runner VM's kernel -- the only place steal on that layer appears, which is what separates a slow suite from a machine sharing a core with somebody else. The sandbox VMs are QEMU processes in this container's own PID namespace, so their threads are a local read too, and they say which node spent the kernel's user time and whether the three spent it evenly. Both are taken at the same two points as the KVM counters and for the same reason: the values are cumulative, so a rate needs a pair, and the interval worth measuring is the window the guest is failing to boot in. The thread reading runs the probe the worker capture already uses rather than a copy of it, since a second copy would drift exactly at the failure paths. Each carries the layer it describes in its own legend, because three kernels in this report publish a /proc/stat and two captures read QEMU threads, one layer apart in each direction -- and a reader who takes one for the other compares a hypervisor against its own guest. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-18 07:25:49 +03:00
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>	2026-08-18 03:31:42 +03:00
Aleksei Sviridkin	0a1d2673d0	test(e2e): capture worker block IO and volume state on node-join failure A worker that burns its whole vCPU while making no progress is either computing without result or waiting on blocks, and every counter this path already collects is blind to the second: the CPU families say what the group was allowed and what it got, the byte counters say what crossed the network, and neither counts a read. Read the Pod's block IO counters from the same kubelet endpoint the CPU ones come from, once rather than twice, because they accumulate from the container starting and a worker container starts with the guest it carries. Read the state of the replicated volumes behind the worker disks as well. A replica that is SyncTarget, Inconsistent or Diskless serves the guest's system disk from a peer over the network rather than locally, and the report collects the same tables only after the suite, by which time cleanup has deleted the volumes a red run would need them for. The guest's own /proc/diskstats would answer this from inside and cannot be reached: the credential minted for the guest carries the os:reader role, which is not allowed to read file contents, and Talos publishes no resource carrying disk statistics. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-17 14:22:09 +03:00
Aleksei Sviridkin	c3247c0ca0	test(e2e): capture the sandbox kernel KVM exit counters across the node-join wait (#3886 ) ## What this PR does Every CPU reading this suite takes on a failing node-join describes a subject inside the sandbox: a cgroup, a QEMU thread, a sandbox node's own `/proc/stat`. All of them can report a guest that was handed its ticks while the guest gets no work done, and none of them can say what a tick cost. That is the state the node-join failures are in now, with quota untouched, throttling near zero, steal at zero and the sandbox host between a third and two thirds idle, while the guest does several times less work per core than the same image does on a run that passes. The price is paid one layer down. A tenant worker is a guest of a sandbox node, which is itself a guest of the runner, and the runner's kernel absorbs the exits, nested page faults and TLB flushes that the arrangement provokes. That kernel publishes the totals in debugfs, and the suite runs in a container sharing it, so this is a local read rather than a walk over nodes. Being local is what makes it affordable. The budget guard in `hack/run-kubernetes-node-join_test.bats` computes both sides of its own inequality, and on this tree the sampling pair can overshoot by 662s against the 680s the derivation allows, which leaves 18 seconds of room. A per-node reading costs two samples times three nodes times the read bound and its kill grace, or 150 seconds at today's knobs, and no transport changes that: the cost sits in the number of per-node calls rather than in the number of counters. Reading the layer the sandbox shares with the runner costs one bounded read instead, and takes nothing away from the collectors already there. debugfs is mounted in a mount namespace belonging to the call. The suite runs for another twenty minutes after this returns, and a mount left behind would be a diagnostic editing the environment the rest of the run measures. The pair is taken across the node-join wait rather than inside the diagnostics block. A counter here accumulates over the life of the VM it belongs to, so one reading is an average over that life rather than a rate over anything; taking the first before the wait and the second after it makes the interval a difference divides by the window the guest was failing to boot in, instead of a few seconds after the deadline had already expired. The kernel-wide files add a wrinkle the artifact states beside the numbers: the kernel answers them by summing over the VMs alive at the moment of the read, so a VM that went away between the samples takes its whole contribution with it while the survivors keep accumulating. The difference is then understated by a whole VM life and its sign proves nothing when positive; when it does come out negative, that is a finding about the sandbox rather than a rate. The second reading is taken on the passing path too, so a green run leaves a baseline to read the failing ones against, and it is taken immediately after the wait rather than at the end of the run. That placement is the measurement: the happy-path tail is another ten to fifteen minutes of a tenant cluster doing storage and LoadBalancer work, so a reading taken after it would price a different window over a different workload while the capture's legend says it priced the join, and the green-versus-red comparison the collector exists for would be gone. A guard in `hack/run-kubernetes-node-join_test.bats` pins both calls to either side of the wait and fails if either drifts. What the capture cannot answer is written into the artifact rather than left for a reader to assume. The counters price the sandbox VMs' own execution and not what runs inside them. A per-VM directory is named after a pid in the kernel's initial namespace, which the container cannot map to its own processes, so the split says how evenly the sandbox VMs paid and not which node paid what. Several counters sitting in that same directory are gauges or running maxima rather than cumulative, so a difference of those is a number with no meaning. And there is no `ept_violation` counter to look for: the nested-paging faults are counted as `pf_taken` and `pf_fixed`, under those names whichever vendor the runner turns out to be. Every branch that can come up short is named where a reader will look for it: no `unshare` or no `mount` on PATH, a mount the sandbox refused, a kernel whose debugfs holds no `kvm` directory at all (which would be a finding about how the sandbox VMs are being run, not a collector that failed), a `kvm` directory holding nothing readable, a walk the wrapper cut short, and a report directory that could not be created. The pairing instruction rides only on the arm that holds a whole reading, so the artifact never invites a difference against a file that has no counters in it. A capture holding no counter answers its caller non-zero, the two findings included, so the passing path writes the shortfall into the job log instead of leaving it in a report nobody downloads. A walk cut short after it wrote counters does not: it left a usable partial reading, marked as partial, and telling the caller nothing was collected would contradict the file sitting beside it. That distinction is decided by whether a counter reached the file rather than by whether the file has bytes in it, because the walk writes a heading before it reads anything. Covered by `hack/run-kubernetes-kvm-exits_test.bats`, which runs under `make unit-tests`. The guards in `hack/run-kubernetes-node-join_test.bats` that enumerate the collectors were updated rather than worked around: the new capture gets a cost arm in the budget guard, an entry in the missing-timeout warning, and an explicit exemption from the spend-order list, because it is not behind the phase gate. It is not behind that gate deliberately: every other collector the phase declines loses only itself, while declining this one would orphan a reading already taken eighteen minutes earlier. Two guards there are new: one pins where the two readings sit rather than what they contain, the other pins that both call sites consume the collector's status, since this is the only collector in that file whose return value is read at all and it is read so a passing run's shortfall reaches the job log. The collector is also stubbed in that suite, at the shared helper and at each of the five inline stub lists inside its `bash -c` bodies. Left out of them the node-join unit tests call it for real, which on Linux means the unit suite runs `unshare --mount` and `mount -t debugfs` with whatever privileges the host gives it, while on macOS the collector stops at its own `command -v unshare` precondition and the path is never exercised at all: a unit test whose result is set by the machine rather than by the code. Measured with a recording fake on `PATH`, the suite makes 21 real invocations with no stub, 9 with the shared helper alone, and none with every site covered. Stubbing costs the block-level read audit nothing, because that audit works by letting real collector bodies reach the kubectl mock and this collector issues no kubectl read. Context for the measurement is cozystack/cozystack#3513. ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff. The diff adds one collector to `hack/e2e-chainsaw/_lib/run-kubernetes.sh`, adds one BATS suite under `hack/`, and edits guards in an existing BATS suite. Nothing under `hack/` is moved or renamed, no make target changes what it does, and `hack/e2e-prepare-cluster.bats` is not touched, so neither the `ccp` trigger on `hack/` layout nor the `ansible-cozystack` and `talm` triggers on node prerequisites fire. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note test(e2e): capture the runner kernel's KVM exit counters on either side of the tenant node-join wait, so a node-join failure can be told apart from a nested-virtualisation slowdown that no counter inside the sandbox can see ```	2026-08-17 14:19:30 +03:00
Aleksei Sviridkin	827f4e7e10	test(e2e): measure the sandbox kernel KVM exit counters across the node-join wait Every CPU reading on this failure path describes a subject inside the sandbox -- a cgroup, a QEMU thread, a node's own /proc/stat -- and all of them can report a guest that was handed its ticks while the guest gets no work done. None of them can say what a tick cost. That price is paid one layer down: a tenant worker is a guest of a sandbox node, which is itself a guest of the runner, and the runner's kernel absorbs the exits, nested page faults and TLB flushes the arrangement provokes. Read those counters from the debugfs that kernel publishes them in. The suite runs in a container sharing it, so this is a local read rather than a walk over nodes, which is what makes it affordable at all: the phase budget has no room left for a further per-node read. debugfs is mounted in a mount namespace belonging to the call, so nothing is left behind for the twenty minutes of suite that follow it. The pair is taken across the node-join wait rather than inside the diagnostics block: once before the wait on every run, once after it on the failing path and on the passing one. The counters are running totals -- per-VM since that VM was created, kernel-wide summed over the VMs alive at the read -- so the interval a difference divides by becomes the window the guest was failing to boot in, and a passing run leaves a baseline to read the failing ones against. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-17 14:18:21 +03:00
myasnikovdaniil	9f5550abdd	fix(release): publish rewritten packages during RC promotion (#3795 ) ## What this PR does Fixes #3477. - Publishes a new packages OCI candidate after rewriting RC image tags to stable tags. - Pins the candidate digest in the stable installer without rebuilding container images. - Verifies the candidate against the prospective merge and original RC artifact before creating stable names. - Refuses promotion when the target maintenance branch lacks the candidate-aware pipeline. - Protects candidates referenced by open release PRs from retention cleanup. - Leaves already published v1.6.0 and v1.6.1 unchanged. ### Screenshots Not applicable. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Testing - `bats --no-tempdir-cleanup hack/promote-packages-artifact_test.bats hack/verify-promoted-packages_test.bats hack/promote-packages-contract.bats hack/promotion-retention-contract.bats` - `actionlint .github/workflows/promote-rc.yaml .github/workflows/pull-requests.yaml .github/workflows/pull-requests-release.yaml .github/workflows/retention.yaml` - `sh -n hack/promote-packages-artifact.sh hack/verify-promoted-packages.sh` - `git diff --check origin/main...HEAD` ### Release note ```release-note fix(release): publish the rewritten stable packages artifact when promoting a release candidate ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Release candidates now use temporary, immutable package artifacts verified before stable publication. - Stable promotions validate package contents, installer references, container digests, and release metadata before tagging. - Temporary promotion artifacts are retained while needed and cleaned up after expiration. - Bug Fixes - Improved handling of release labels, fork pull requests, and promotion pull requests. - Added end-to-end verification before release promotion. - Prevented invalid, ambiguous, or untrusted package references from being promoted. - Documentation - Updated release and promotion guidance for candidate verification, digest pinning, and transactional promotion. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-17 12:52:19 +05:00
Aleksei Sviridkin	73b8afc256	test(e2e): run tenant workers on one vCPU with CPU headroom above it Under a two-vCPU worker both vCPU threads burn 1.4 to 1.8 cores between them while the guest kernel never gets past SMP bringup, and raising the CFS ceiling from two cores to three changed neither the burn nor the progress, so the spin is between the two vCPUs, not against the quota. A single-vCPU guest has no sibling to spin on. The ceiling of two keeps the QEMU emulator and IO threads from eating into the one core the guest computes with, and the request keeps scheduling where the workers always were. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-17 03:09:04 +03:00
Aleksei Sviridkin	b8964c8233	test(e2e): give tenant workers CPU headroom with a request to match Restores the ceiling the kubernetes suites briefly carried, now that a node group can declare the CPU request that goes with it. The limit on its own was defaulted into the request by KubeVirt, so every worker asked the scheduler for the whole ceiling and sat Pending on Insufficient cpu before a guest existed. The request is set to what KubeVirt derived for these workers before, which leaves scheduling where it was and leaves the ceiling as the only variable the run changes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-16 21:59:19 +03:00
Aleksei Sviridkin	80e2753bf4	fix(e2e): drop the worker CPU limit until it can carry its own request A container that sets a CPU limit and no request is given a request equal to the limit, so each tenant worker asked the scheduler for its whole three-CPU ceiling and every kubernetes suite worker sat Pending on Insufficient cpu across all three sandbox nodes, before any guest booted. The headroom experiment comes back when the chart can pair the limit with an explicit request. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-16 21:01:30 +03:00
Aleksei Sviridkin	c2c50b4119	test(e2e): give tenant workers CPU headroom above their vCPU count Workers that fail to join have been measured burning their vCPU threads flat out under a ceiling equal to their vCPU count while the guest kernel recorded no progress at all, with the emulator, IO, vhost and vnc threads flat at zero. That is the shape of a vCPU spinning on a lock whose holder is the other vCPU, preempted out of the quota the two share, and it predicts that a ceiling with room above the vCPU count lets the preempted one back in. What the next runs answer is whether the join failure rate moves. If it does not, the lever is elsewhere and the value goes back. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-16 17:06:16 +03:00
Aleksei Sviridkin	cf2d9c6d13	test(e2e): split the tenant worker CPU time across QEMU threads The node-join diagnostics phase samples the tenant worker's CPU counters twice on failure, but it reads them from the compute container's cgroup, and that cgroup holds every thread QEMU runs: the guest's vCPUs, QEMU's main thread, and its IO and worker threads. A container burning most of its quota while the guest reports no progress is two incompatible findings wearing one number, and no container-level counter separates them. Add a collector that execs into each Running worker's compute container, finds the QEMU process and copies every /proc/<pid>/task/<tid>/stat line into the artifact verbatim, with a legend beside them. Nothing is computed here. The legend names where utime and stime sit and why counting fields over whitespace reads the wrong column for exactly the threads the split is about: a KVM vCPU thread is named "CPU 0/KVM", that space is inside the comm parentheses, and every field after it shifts by one. It runs inside the sampling loop the two existing subjects already share, so it costs no second wait: both readings ride the interval already being paid, and the rate comes from the two files plus the stamps each one carries. Every way the read can come up short is a named outcome rather than an empty directory, and the legend and the pairing instruction are withheld from each of them: telling a reader to subtract two files asserts that both hold a whole reading. The probe shells out to nothing but getconf, whose absence it names, because a probe that needed cat would report a container missing coreutils as a container whose QEMU had exited, and it counts what it managed to read so that having read no process at all stays separable from having read them all without finding QEMU. Its stderr is suppressed on the walk rather than on either read: redirections are applied left to right, so a 2>/dev/null after an input redirect is not in place yet when opening the input fails, and a thread that exits between the glob and the read would otherwise put a shell complaint in the artifact beside a healthy capture. It also reports a QEMU that was named and then gone before its task files could be opened, because a process heading alone is enough to make the capture non-empty and would otherwise carry a legend describing columns the file does not have. The Pod listing the cAdvisor captures already share now takes its whole query as a parameter, so this collector reuses that read rather than carrying a third copy of it, and asks for the Running Pods only: a launcher left over from an earlier attempt costs the sibling caller nothing and would cost this one a bounded exec and a slot under its cap. The guard that holds the sampling group against the room the tenant snapshot needs now counts every capped walk in the loop rather than the two spelled the same way, so the new subject is inside the arithmetic rather than beside it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-16 05:17:16 +03:00
Aleksei Sviridkin	959e90c9ab	test(e2e): time the guest console capture and baseline it on a green run A tenant worker's guest console is stamped in seconds since that guest's own kernel started, so a log ending mid-boot says how far the guest got and nothing about when. Whether it ends there because the guest started late or because the guest stopped printing decides which failure is being looked at, and neither instant was recorded: the read's own instant was nowhere in the file, surviving only as the artifact's mtime, which is metadata rather than content and so says nothing to anyone reading the log; and the guest's start only inside a describe dump that a capture with no silent console never triggers. Across three collected runs every capture exited zero and every log began at the first kernel line, so the reads were complete at both ends and the logs end where the guests stopped -- and which of the two happened could not be read off any of them. Each console log now carries the instant its own read started and returned, and CAPTURE-TIMELINE.txt beside it carries each Pod's phase, its start time and the start of its compute and console containers, from one bounded read taken once for the whole selector. Those subtract to how long a guest had been alive when it was read, which is what the log's last kernel stamp is then compared against. The console container is asked for in both status lists, since KubeVirt runs it as an init container and reading only one of them would report a container that started fine as one that never ran. A read that failed, a read cut off part way and a read that answered with nothing are recorded as themselves rather than left as a missing file, and the note names the error log only when there is one. The same capture also runs on the passing path, before the tenant is torn down and the Pod holding the stream goes with it. Every console this tree has collected came from a failure, so what a healthy boot looks like has never been measured, while "slower than usual" is the claim the failure keeps resting on. That caller asks for the pool's minimum rather than its maximum, since two workers booting is the same measurement as ten, and it warns in the job log rather than failing a suite that has already proved everything it exists to prove. Both paths write the same directory, so the capture also records which of them produced it. That capture costs real wall clock at the tail of an operation with a fixed ceiling, which is a way for a run that passed to be killed while collecting a debugging aid. So it is declined out loud when the ceiling no longer has room for it, against a reserve derived from its own cost and held in step with the timeout both suites give the operation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-15 23:33:33 +03:00
Aleksei Sviridkin	2b3c163d15	fix(e2e): size tenant workers by resources, not by u1.large Sizing the workers with u1.large doubled their memory along with the vCPUs, and four 8Gi workers do not fit on the sandbox beside the rest of the suite: the second worker of each cluster stayed Pending with Insufficient memory on all three nodes, so the node group could never reach two Ready nodes and node-join failed for a reason that has nothing to do with the CPU ceiling it was meant to test. Setting resources.cpu and resources.memory keeps the two vCPUs at the memory the workers had before. The chart drops the instancetype from the VM when a group sets both, and the values schema still requires the field, so it stays in the values as the sizing fallback it now is. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-15 18:34:13 +03:00
Aleksei Sviridkin	3b49be07fa	test(e2e): give tenant workers two vCPUs The tenant worker boots Talos and unpacks the kubelet image on a single vCPU whose CFS quota also covers the QEMU emulator and IO threads, and measurements on both quiet and loaded runs show the compute container pinned at 93-98% of that one-core ceiling for the whole node-Ready budget. The join deadline is then decided entirely by how much CPU work the boot happens to need, and under parallel e2e load it does not fit. Doubling the worker to u1.large raises the ceiling instead of the weight, which measurements showed changes nothing since the workers never lose a scheduling fight. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-15 15:08:31 +03:00
Aleksei Sviridkin	ef96292b64	fix(ci): give the nightly e2e runner headroom over its guest vCPUs (#3833 ) ## What this PR does The e2e sandbox boots three QEMU nodes at `-smp 8`, so 24 guest vCPUs are committed before any tenant VM exists. Four jobs boot that sandbox. Three run on the 32-vCPU class; nightly ran on the 24-vCPU one, where 24 guests against 24 host vCPUs leaves nothing for QEMU's own emulation and IO threads, containerd, the kernel and the runner agent. The other lanes moved to the larger class at some point and nightly was not moved with them. The sandbox keeps its 24 vCPUs. That guest count is part of what e2e claims to test, and a smaller nightly cluster stops matching the one PRs run against. This is hygiene, not a fix. Measured boot-phase data across 18 runs says the load a workflow puts on the host dominates guest starvation, not the runner class: the lanes already on 32 vCPU boot guests slower than nightly did on 24, and class and workflow kind are fully confounded there, so neither separates out. Do not read this as making anything faster or as addressing a particular red run. What is left is the arithmetic. A 1:1 commit has no headroom at all, and two lanes running the identical sandbox on different classes is drift nothing was watching. The rest is a check, because the guest demand and the runner classes live in different files and nothing compared them. `hack/sandbox-runner-headroom.bats` reads `-smp` and `-m` times the loop count out of `hack/e2e-prepare-cluster.bats`, reads the runner class of every job that calls the sandbox make targets, and wants the runner strictly larger on both axes. Neither side is a hardcoded list, so bumping `-smp`, adding a management node, or adding a fifth lane on a smaller runner turns it red on the same commit. Memory is in there because the same argument applies to it and `-m 24576` sits on the same line as `-smp 8`, not because a class that violates it exists today. It stops instead of guessing when it cannot read something, since a guess comes out smaller than the real demand and passes a runner that cannot hold the sandbox. Refused: a missing or non-literal `-smp` / `-m`, a launch written outside any `@test` block or inside one left open at end of file, a `for` opener the strict pattern does not match, a `for` list holding anything but literal words, a `while` sharing the block, two launches or two loops in one block, and a `runs-on` with no `oracle-vm-<n>cpu-<m>gb` in it. Unparsed loop openers are counted separately from parsed ones, so moving `do` to the next line is refused rather than silently multiplied by one. There is also a cross-check that every workflow calling the sandbox make targets yields at least one matched job, so a renamed target turns the parser red instead of quietly shrinking what it covers. The derivation runs against the live tree, which passes whether or not those refusals still work, so each one is pinned by a fixture that feeds a synthetic file to the same program. That matters more than it sounds: two of the refusals above exist because the first version of this file did not have them, and a launch moved into a file-level helper would have halved the measured demand and stayed green. The numbers come from: ```bash sed -n '98,107p' hack/e2e-prepare-cluster.bats # three nodes, -smp 8, -m 24576 grep -rn 'prepare-env' .github/workflows/ # the four jobs that boot it grep -rn 'runs-on: oracle-vm-' .github/workflows/ # their runner classes ``` The rule also goes into `docs/agents/e2e-testing.md` next to the other environment invariants, so whoever adds the fifth lane picks its runner class from the docs instead of from a red unit suite. Checked that it bites, twice over. Fourteen mutations of the inputs: reverting the runner class, `-smp 11`, a fourth and fifth management node, `-smp cpus=8`, `-m size=24576`, `do` on the next line, `$(seq 1 3)` as the list, a `while` loop, the launch moved into a file-level helper, renaming the make target, `ubuntu-latest`, a matrix expression in `runs-on`, a hypothetical `32cpu-64gb` class, and a new lane on a 4-vCPU runner. Then eleven mutations of the parser itself, one per refusal, each of which kills exactly the fixture that names it and nothing else. Green on this tree, and the same numbers out of gawk, mawk and BSD awk. Closes #3831. ### Screenshots No UI change. ### Downstream repositories Walked the trigger map against the diff. It changes one `runs-on` line plus a comment in `.github/workflows/nightly.yaml` and adds one file under `hack/`. The `ccp` trigger is moving or renaming something under `hack/`, or changing what a make target does; `hack/package.mk` and `hack/common-envs.mk` are untouched, and the new test is picked up by the existing `hack/.bats` wildcard without the target changing. The `ansible-cozystack` and `talm` triggers are node prerequisites in `hack/e2e-prepare-cluster.bats`, which this reads and does not modify. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(ci): run the nightly e2e suite on a runner class larger than the sandbox's guest vCPU demand, and add a unit check that fails when an e2e lane loses that headroom ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Bug Fixes * Updated nightly end-to-end testing to run on a higher-capacity runner, improving support for multiple QEMU guests and required resource headroom. * Added validation to prevent sandbox runners from being undersized or oversubscribed. * Documentation * Documented the resource requirements and deterministic conventions for end-to-end sandbox testing. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-15 12:59:55 +03:00
Aleksei Sviridkin	527d4ea00f	test(e2e): measure tenant worker CPU as a rate, and the sandbox nodes under it Two instruments for the node-join failure the kubernetes-* suites keep hitting. Neither changes what the suites assert. The worker CPU capture now reads container_cpu_usage_seconds_total beside the CFS counters, and reads them twice a fixed interval apart instead of once. The throttled counters say a container met its ceiling and cannot say how much CPU it got, which are the two candidate explanations for a stalled worker: a guest held at a one-core ceiling, and a guest whose vCPU thread is rarely scheduled at all. Both leave throttled periods behind. Every counter involved accumulates from container start, so a single reading is an average over the whole uptime and no rate at all, and the profile under investigation is a guest that freezes for tens of seconds and then runs flat out: the average comes out low, the instantaneous figure comes out at the cap, and only the second separates it from a guest that is simply capped. The sandbox nodes are a new subject. The worker captures describe a cgroup, so a guest that is runnable and never scheduled looks from there like a guest that asked for nothing. Whether the sandbox node itself was given its turn is recorded only in the steal column of its own /proc/stat: no kubelet endpoint carries it, and crust-gather cannot reach it because PodSecurity refuses its node-shell Pod here, so the read goes over the Talos API, which is already how the report reaches those nodes for dmesg. The rows are captured verbatim with their column legend beside them rather than reduced to a percentage, because /proc/stat has no header, steal and guest sit next to each other, and guest is large on a node running VMs and already counted inside user. Both subjects are read on either side of one shared wait, so the pair pays it once. The wait is not the interval either subject's counters span, though: each pass walks its own nodes and the other subject's pass falls between a subject's two readings, and on the run this exists for those passes are the slow part. Each capture therefore stamps both ends of its read, so a rate comes from subtracting stamps rather than from assuming the knob, and the width of those brackets is the uncertainty. That wait is a measurement interval rather than a stand-in for a condition nobody wrote a wait for, so the e2e conventions carve it out by name instead of marking it as a fixed timeout to be replaced. The pair is gated below the serial console rather than above it. Its ceiling is sixteen bounded reads plus the interval, which is most of the phase budget, and admission gates only when a collector may start, so above the console a run that hit those bounds would leave the console never started rather than merely cut short. The console is the only capture that survives a worker which never reached apid, which is the shape this failure usually takes. That overrun is inside the term the budget derivation already allows for one late admission, and a guard holds the pair's ceiling against it. No collector here may leave an empty directory, since an empty directory reads as a healthy node: a missing binary, a missing talosconfig, a listing that never answered, a node with no address and a read that was cut off each land as a named note or a COLLECTION-FAILED.txt. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-15 10:19:19 +03:00
Aleksei Sviridkin	3dd142a718	fix(e2e): capture both ends of the worker byte path on node-join failure (#3804 ) ## What this PR does A tenant worker that misses the node-Ready deadline while its kubelet image pull crawls leaves two mechanisms behind one symptom, and the failure artifact could not tell them apart: either the link is slow, or the pull keeps restarting and discarding what it fetched. From inside the guest both are a progress line that barely moves. This adds the host-side counters that separate them, and the guest-side reads that explain a kubelet which never got as far as pulling at all. Host side. A per-worker network capture reads the eight `container_network_` families from the same kubelet cAdvisor endpoint the CPU throttling capture already uses. cAdvisor builds them from the interface counters inside the Pod's own network namespace and publishes one row per interface, so what arrived at the worker is counted there in full, including the bytes a restart threw away, set against the progress the guest reported, that separates a pull which kept restarting from a slow link. Which row answers is not obvious, and getting it wrong inverts the reading, so the collector states it rather than leaving it to the reader. A bridge-bound worker presents four interfaces in its Pod netns: the one still named `eth0` is a dummy that holds the Pod IP so the kubelet's check passes and carries no traffic, so it reports zero, and it is the row a reader reaches for first, by name, where zero reads as a worker that received nothing. The number that answers is the renamed NIC's receive; the bridge counts the same bytes again, and the tap's counters are named from the host's end. The filter deliberately carries no interface predicate, so all four reach the artifact and can be told apart. Two further properties of the family fail by capturing nothing rather than loudly, so both are stated where the filter is: the dropped-packet series are spelled `packets_dropped` and there is no `_drops_total`, and every row carries an empty container label because cAdvisor publishes them on the Pod sandbox, which means a filter borrowed from the CPU counters beside it matches nothing at all. The fixture's row shape and values are real kubelet output with the identifying labels rewritten. Its four interface names are derived rather than observed: no virt-launcher Pod was available to sample, so they are read from the name generators in the KubeVirt version this repo ships, and the counters attached to them illustrate direction rather than magnitude. Guest side. The Talos capture gains the service state machine and the link table. Talos creates a service's log buffer when its runner opens the log writer, as it starts the process, so a kubelet still waiting on its volumes, on time sync, or on the container runtime reporting healthy never reached its runner and has no log at all, and `talosctl logs kubelet` then fails naming a log that was never registered, which is true about the log and silent about the service. `talosctl services` answers with the state and the condition being waited on, in one call for every service, so the runtime a kubelet is blocked behind arrives in the same file. The link table is read as YAML because MTU lives in the `LinkStatus` spec and not among the resource's printed columns, and it is the value that decides whether a path-MTU reading of a stalled transfer survives. Both new reads are made with the short-lived `os:reader` credential the capture already mints; neither needs a wider role. A worker that answered nothing now says so once, for the worker. Each read already carried its own error and exit code, which is accurate per file and adds up to a directory indistinguishable from a worker that simply had little to report. ### What this does not do This collects evidence; it fixes nothing and is not expected to change any suite's outcome. The comparison it enables is not symmetric either, and the source says so: the host side reports bytes on an interface, while the guest side reports an image pull's own progress, which is unpacking rather than arrival. They bound each other; they are not two measurements of one quantity. ### Cost The diagnostics phase budget drops from 8m to 7m. That is forced rather than chosen: the inequality that keeps the tenant crust-gather snapshot reachable had ten seconds of slack, and the two added guest reads cost sixty across the minimum worker pool. A shorter budget declines more, and what it declines first is what is gated last: the mirror capture, whose subject is partly recoverable elsewhere, and the image-cache re-probe, which has no substitute and was already first to go. The new network capture also spends a listing and one read per node ahead of the guest captures, so those reach their gate with less headroom than before. All of it is priced where the number lives, as a ceiling the read bounds impose rather than a duration measured on a live cluster, and a collector the budget declines says so in the log rather than going quiet. The remaining slack is ten seconds, which is worth knowing before the next collector is added rather than after: it buys no bounded read at any bound this file uses, so adding one means re-deriving the budget again. ### Testing `hack/run-kubernetes-network-counters_test.bats` is new (15 tests). The node-join, Talos-diagnostics and CPU-throttle suites gain coverage for the phase ordering, the per-worker marker boundary, the spend-order comments in both Chainsaw suites, and the enumeration of collectors that survive a missing `timeout`. Every claim the two Chainsaw comment blocks make about spend order is derived from the source rather than restated, so a collector added without a line there fails. The two cAdvisor captures now run one shared body. They previously carried near-identical copies, and the copies cost more than duplication normally does: the block-level bound audit pins each collector by something only it produces, and two collectors issuing identical reads left neither with anything of its own, so removing one from the audit went unnoticed. Each is now pinned by the report directory it writes. Verified with `hack/cozytest.sh` across eleven suites, checking that the number of tests executed matches the number declared, because an exit code answers "did anything fail", never "did anything run". Each behavioural change was confirmed by mutating the source and observing the intended test fail. ### Screenshots No UI changes. ### Downstream repositories Walked the trigger map file by file against the diff. All seven changed files live under `hack/`, and the change adds one file and modifies six; no repository file is moved, renamed or deleted. The `ccp` entry is the only one that keys on `hack/` at all, and it keys on `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh` as anchors for `make generate`; none of those is touched, and no make target changes behaviour. Nothing here is a package, a chart value, an `ApplicationDefinition`, a CRD, a release asset, a namespace, a variant, or a node prerequisite, so no other entry applies. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * New Features * Added worker network-counter diagnostics with interface-level and cumulative traffic data. * Expanded Talos diagnostics with service-state and link-state information. * Bug Fixes * Improved reporting for partial captures, timeouts, unavailable data, and zero-traffic workers. * Clarified failure reporting and fallback messages when timeout controls are unavailable. * Documentation * Updated diagnostic timing and collector documentation for the seven-minute budget and new metrics. * Tests * Added comprehensive coverage for network counters, filtering, truncation, and expanded Talos diagnostic scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-15 03:44:05 +03:00
Aleksei Sviridkin	db2ddfab2b	fix(ci): give the nightly e2e runner headroom over its guest vCPUs The e2e sandbox boots three QEMU management nodes at `-smp 8`, so 24 guest vCPUs are committed before any tenant VM starts. The nightly lane ran that sandbox on a 24-vCPU runner, a 1:1 commit that leaves the host nothing for QEMU's emulation and IO threads, containerd, the kernel and the runner agent. The tag, fork and pull-request lanes already run the same sandbox on the 32-vCPU class, so move nightly there too. This aligns headroom and claims no performance effect. Measured boot data shows the load a workflow puts on the host, not the runner class, dominates guest starvation, and the two are confounded in that data; no red run is attributed to the class here. The case is the arithmetic: a 1:1 commit leaves no headroom, and two lanes running the identical sandbox on different classes is drift nothing was watching. Add hack/sandbox-runner-headroom.bats to keep the two sides paired. It derives the guest vCPU and memory demand from the sandbox bats file, and the runner class of every job that drives the sandbox make targets, then requires the runner to be strictly larger on both axes. Neither side is a literal list, so raising `-smp`, adding a management node or adding a lane on a smaller runner turns it red instead of drifting apart silently. Input the derivation cannot read in full is refused rather than guessed: a guess produces a demand smaller than the real one, which passes a runner that cannot hold the sandbox, while a refusal names the line. That covers a launch written outside any `@test` block, a block left open at end of file, a `for` opener the strict pattern misses, a non-literal loop list, and a `while` loop sharing the block. The derivation runs against the live tree, which passes whether or not those refusals still work, so each one is pinned by a fixture feeding a synthetic file to the same program. Loosening the parser turns a fixture red rather than quietly widening what the check accepts. Record the rule alongside the other environment invariants in docs/agents/e2e-testing.md, so a new lane picks its runner class from the documentation rather than from a failing unit suite. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-15 02:41:19 +03:00
Aleksei Sviridkin	e441edc2aa	test(e2e): stop the mongodb backup suite before the restore leg A to-copy restore replays the source cluster's system users into the target, so the target's own generated credentials stop authenticating and the harness cannot read the sentinel back out of the restored copy. The suite has never passed for that reason, holding the in-tree e2e run red on every branch that selects it. Drive run-all.sh with SKIP_RESTORE=1 so the suite still proves the backup half end to end, and pin the skip by asserting the skip branch's own log line: exit zero alone is also what a completed round-trip returns, so without that assertion the step could not tell a skipped flow from a full one, nor notice SKIP_RESTORE failing to reach the harness. The to-copy manifest and the example README both stated that CI covers the restore, which stops being true here, so both now say where coverage ends. The asserted log line is annotated at its source, since rewording it would red the suite with a bare match failure. Coverage returns by dropping SKIP_RESTORE together with the assertion on that line, once the credential handling behind the restore is fixed. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com>	2026-08-14 21:18:09 +03:00
Myasnikov Daniil	ffae003d6b	fix(release): stop a failed org package listing from exiting green The nightly sweep's own listing shared one `\|\| true` with the grep that filters it, and grep legitimately exits 1 when no cozystack/* package matches. A failed /orgs/.../packages read therefore arrived as an empty list, took the "No cozystack/* container packages found" branch and exited 0 -- below the nightly sweep but ABOVE the deferred guard, so a protection set that could not be resolved reported success having swept nothing. That bypasses the guard rather than varying it: replaying the step against a stub gh, a failed /pulls combined with a legitimately empty org listing exited 0 after printing its own ::error::. List and filter as two statements so one `\|\| true` stops meaning two things, handle the listing failure like the other two (::error::, blocked flag, emptied list), and consult the flag before the early exit. That exit is non-zero, matching the other two listing failures: no sweep is left to carry the run down to the guard, so this is the last place the flag can be honoured, and an unresolved protection set is exactly what fail-closed means. The legitimate empty-org case still prints the same line and exits 0. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:59 +05:00
Myasnikov Daniil	a37a3178e5	docs(release): name all four sourcers of the image-ref enumeration The library header still claimed three call sites. There are four: this PR made hack/verify-promoted-packages.sh source it to compare the candidate's refs against the rc artifact's. The historical drift story stays with the original three, since the verifier has only ever read the shared enumeration. The consumer table gained hack/promote-packages-artifact.sh under a bullet requiring every consumer of references to read all three shapes through the library, while the exception below it still named only hack/overlay-main-images.sh. The publisher sources nothing and needs nothing: it hands the whole tree to `flux push artifact` and touches one reference, the installer's own self-pin, by key with yq. Say that, rather than implying a gap it does not have -- the real exception remains overlay-main-images.sh, which does read references and does walk the tree itself. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:59 +05:00
Myasnikov Daniil	dee398f3b3	fix(release): defer a retention listing failure to the promotion guard Both candidate-protection sources open with a `gh api --paginate` listing and neither handled a failure. Under `set -euo pipefail` a transient /pulls or /branches blip therefore exited the step before the unrelated nightly GHCR sweep had run -- the exact coupling the deferred guard at the bottom exists to remove, and which hack/promotion-retention-contract.bats already asserts positionally. Route both listings into that guard instead: raise the blocked flag and empty the list, so the promotion sweep is skipped and the job still fails while the nightly sweep finishes. Emptying is not cosmetic -- a half-paginated list left in place is a smaller protected set, and a smaller protected set is the one shape that can authorise a delete. Replaying the step's real shell against a stub gh: with /pulls failing it exited 5 and never reached the nightly line; it now exits 1 with the nightly sweep done and the promotion sweep skipped. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:59 +05:00
Myasnikov Daniil	b65b6aa3ae	test(release): pin every requiredBaseFiles entry, not just the list The promote gate refuses a target base that lacks any of five [path, marker] pairs, and the contract test asserted the list as a block: deleting the whole thing turned it red, deleting the hack/lib/image-refs.sh entry alone did not. Four of five pairs were therefore unpinned individually. Pin the missing one. The verifier sources that library through the same `dirname $0` as lib/promoted-packages.sh, so a base carrying the script without it fails at exactly the same point -- after the candidate, the staging branch and the draft release exist. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:59 +05:00
Myasnikov Daniil	acd0a9e0a2	test(release): cover the verifier's rc-reference scan guard The scan treats any grep diagnostic as a failed scan, because `find ... -exec ... +` reports the legitimate "nothing matched" as a failure too and the `\|\| true` absorbing that also absorbs a file that went unscanned. Deleting the whole guard left the suite green: its two siblings -- a failing collector and an empty collection -- each have a dedicated test, and this one had none. Reach the state the guard exists for by putting a grep on PATH that answers the scan's invocation the way the real one answers a file it cannot open. A chmod 000 fixture would stop proving anything wherever the suite runs as root. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:59 +05:00
Myasnikov Daniil	04fa8471fe	docs(release): say what the gates actually guarantee Three claims that were stronger than the code, plus one legibility fix. The PR gate's comment said a `packages/` edit "on the PR or its base" fails the status before merge. The base half is not true: GitHub fires no `pull_request` event when the base branch advances, it only recomputes refs/pull/N/merge, so a change landing on the base after this went green is never re-verified and the PR merges on the stale result. Checking the merge tree does make base drift visible WHEN the job runs, which is worth keeping — but finalize is what actually catches the drift case, after the merge and before any stable name. Say both, in the workflow and in the reviewer checklist, so nobody reads the green as covering more than it does. "Transactional at every consumer-visible name" is a judgement call dressed as an absolute, inherited from the "this dispatch performs NO registry mutation" it replaced. The dispatch now does write to the registry: the `promotion-` candidate is a real, publicly listable tag. What it creates is no stable* name — no vX.Y.Z, no :latest, no stable installer. Stated that way the sentence stops needing the reader to already know which writes were meant. The retention paragraph listed two of the conditions the workflow enforces. It now lists all of them, including the ones the workflow gained here: same-repository, bot-authored, `release-X.Y.Z` branch, the `release` label, exactly one valid digest pin, and protection from a base-branch tip as well as an open PR. Also: `flux pull artifact` reads its credentials from ~/.docker/config.json, which is why finalize now runs `docker login` — but its runner class is not one of the docker-using build ones and the setup step installs everything except docker. Check for it there, where the message can name what is missing, instead of failing with "command not found" halfway down the release path. And the verifier's exclusion list is a restatement of flux's `excludeOCI`, not a derivation — the CLI does not expose it — so the coupling and the pinned version are now written down next to the list. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:58 +05:00
Myasnikov Daniil	820e72d572	fix(release): protect a candidate until its retag, not until PR merge Retention protected a packages candidate only while an OPEN bot release PR pinned its digest. The digest stops being eligible for deletion only when finalize's retag adds a stable tag to the manifest, because that is what makes the `all(tags match promotion-*)` predicate stop matching — and finalize runs after the merge that closes the PR. The window between them is where a half-finished release sits. Finalize creates the write-once stable git tag, then dies before the retag: a lost runner, a 5xx publishing the draft, a non-fast-forward on the maintenance branch. What is left pins the candidate from a merge commit and a tag nobody can move, with no open PR and only a temporary tag on the manifest. Thirty days later retention deletes it, after which re-running finalize 404s at the candidate check and re-dispatching promote is refused because the stable tag exists. The release can then only be finished by hand — the exact class of manual cleanup this design exists to remove. Protect the tips of the branches a promote PR can merge into as well. The two sources are read differently on purpose. An open promote PR's head is supposed to carry exactly one candidate pin, so anything else is an anomaly and blocks. A branch tip is an additional source: every digest pin found is protected and a branch without one contributes nothing, because a maintenance line cut before this pipeline existed cannot pin a candidate and must not wedge retention forever. Blocking now stops the promotion sweep alone. It used to `exit 1` inside the resolution loop, which also killed the unrelated nightly prune further down the same step — so a WIP commit adding a trailing comment to a pin on a release branch would silently stop GHCR nightly pruning. The job still fails; it fails after the nightly sweep has run. Verified by running the step's script against a stubbed `gh`: a digest pinned by main's tip and referenced by no open PR is now excluded while a genuinely abandoned one is still listed; a manifest that also carries a stable tag stays excluded by the existing predicate; and a malformed pin on a PR head leaves the nightly sweep running, skips the promotion sweep and exits 1. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:58 +05:00
Myasnikov Daniil	05b2ce90cf	fix(release): close the verifier's fail-open paths and cover them Three gaps in the guards that decide whether the published candidate is still the tree being tagged. All three passed silently rather than failing, which is the worst shape for a check that runs immediately before a write-once name is created. `normalized_refs` piped `collect_image_refs` straight into its parsing loop, so the pipeline reported `sort -u`'s status and the collector's was discarded. POSIX sh has no `pipefail` and this script has to stay POSIX, so `set -e` never saw it: a failed or truncated collection became a smaller set and the rc-vs-candidate comparison matched it happily. The verifier then reported that no container bytes changed across promotion having proven nothing — on the guard that carries that entire claim. Collect into a file first, so a nonzero collector aborts before anything is compared. An empty collection is the same failure wearing a zero exit status and needs its own check: two empty sets compare equal, so the proof passes having examined nothing, and every packages tree carries image references — none means the collector understood nothing it was handed. The rc-leftovers scan swallowed grep's stderr, turning "could not read this file" into "this file is clean" — the silent-skip shape hack/promote-rewrite-tags.sh refuses by name. The `\|\| true` has to stay, because `-exec … +` reports the legitimate "nothing matched" as a failure too, so the diagnostics are the only remaining signal that a file went unscanned. Fail on any of them. Defence in depth rather than a live hole: a surviving rc string would also fail the per-file compare, since the release tree has none. The per-file content comparison had no test at all. It is the only leg that catches a file present on BOTH sides with different bytes — exactly the threat the job exists for, a maintainer editing packages/ on the release PR after promotion published the candidate. The file-set compare sees the same names, the installer compare reads one file, and the rc-refs compare never looks at the release tree. Replacing the comparison with a no-op left all nine other tests green. Red phases, all observed before the fixes. The content-compare case changes one non-image line on one side, so no other leg can fire and claim the credit; it fails with the comparison no-op'd and passes with it restored. The two collector cases run the verifier from a copy whose sibling lib/ holds a stub — which is how the workflows already invoke it, `.release-tooling/hack/…` with libraries resolved next to the script, so the failure is reached the way production would reach it. Against the piped version both fail on `[ "$rc" -ne 0 ]`: the verifier exits 0 and prints its success line whether the collector returns 3 or returns nothing. Also pins `e2e`'s dependency on the candidate gate. The contract pinned `e2e-report`'s, so removing `verify-release-candidate` from `e2e`'s `needs` together with the `success` clause on its release arm left all six tests green — and running a full-e2e promotion suite against a candidate that never verified is the worse of the two failures. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: Claude <noreply@anthropic.com>	2026-08-14 18:37:58 +05:00
Myasnikov Daniil	9fa8eeeda2	ci(release): pin the promotion toolchain and cap the candidate gate Three review findings on the new promotion steps. `yq` was fetched from `releases/latest/download` in all three steps this pipeline installs it in — the promote dispatch, the promote PR gate and finalize. That makes an unpinned moving dependency part of what decides whether a stable release may be created, and part of what rewrites the installer pin: two promotions of the same rc can be served different binaries. Pin it the way FLUX_VERSION already is, version-gated so a matching preinstalled yq is reused, on the version the e2e sandbox image already tracks. A version authenticates nothing about the bytes it names, though, and these are the workflows that mint write-once stable names. yq is therefore also pinned BY CONTENT: its SHA-256 is committed beside its version and verified before install. Scoped to yq deliberately. It is the one downloaded binary with write authority over what a release contains — it stamps the candidate digest into installer values at promote time and platformVersion into the stable chart at finalize. skopeo, helm and oras only move bytes that are already digest-addressed, so a substituted one is caught downstream; a substituted yq writes the wrong pin and every later check verifies that pin faithfully. The Flux installer is left as it is, on purpose. It fetches flux_${FLUX_VERSION}_checksums.txt from the pinned release and verifies the binary against it, so the binary is not the exposure — the ~6 KB bootstrap script from fluxcd.io is, and it is mutable by design and fetched identically in ten other workflows here. Pinning its bytes in these three files would break on the next regeneration of a script the project does not control while leaving the same fetch unpinned everywhere else. The comment names the residual risk instead. No precedent existed either way: nothing under .github/workflows/ or hack/ verified a downloaded tool before this, and every version-pinned download in the tree (oras 1.2.0, cozyvalues-gen 1.6.0) is unverified. This is a new convention rather than an application of one, confined to the release path and justified by write authority, and the comment says so where the next reader will be standing. The three unpinned fetches outside this pipeline (nightly.yaml, e2e-fork.yaml) are left alone. The checksum is trust-on-first-use, which is the honest description: it fixes the bytes to what was checked when the version was chosen and makes any later substitution loud. Provenance is the release's own published checksums list cross-checked against the downloaded asset; both agree on fa52a4e758c63d38299163fbdd1edfb4c4963247918bf9c1c5d31d84789eded4, and that binary self-reports v4.53.3. Verified locally against the real asset: the version gate skips a matching preinstalled yq and re-installs a mismatched one, the checksum line reports OK for the genuine binary and exits 1 for a tampered copy, which aborts the step under the runner's `bash -e`. `verify-release-candidate` had no `timeout-minutes`, so it inherited the 6-hour default while `e2e-report` waits on it through `needs` — a stalled `flux pull artifact` would hold the required `E2E Tests` status pending for the whole of it. 30 minutes, matching `checks` and `finalize`. The contract pins that all three install steps carry both versions and the checksum, run the checksum check, name no `latest/download`, and agree on one yq version and one checksum. So the unpinned one-line idiom cannot come back into a release step by copy-paste from the workflows that still use it, and bumping the version in one file without its checksum cannot pass. Both mutations are caught. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> Assisted-By: Claude <noreply@anthropic.com>	2026-08-14 18:37:58 +05:00
Myasnikov Daniil	2d734fb229	fix(release): preflight the rc tree's own pin before publishing hack/promote-packages-artifact.sh pushes the candidate BEFORE rewriting installer values, so the artifact carries the rc tree's platformSourceUrl/platformSourceRef verbatim — and hack/verify-promoted-packages.sh reads exactly those two fields back out of it as the candidate's rc baseline, requiring a trusted repository and an immutable `digest=sha256:<64 hex>` pin. The publisher checked neither. A tag-form ref or an untrusted URL in the rc tree published a candidate and pushed two commits regardless; the refusal arrived later, in the promote PR gate or in finalize, once the staging branch and the draft release already existed. Check both at dispatch time instead, while nothing has been written. A copied regex would drift from the guard it is bringing forward and quietly restore the late failure, so the trusted repository and the digest pattern move into hack/lib/promoted-packages.sh and both halves source it. That makes the library part of what the verifier needs at run time, so promote's base-branch precondition gains it: the list exists because the base's own workflows run after merge, and a base carrying the verifier without its library fails at the same two late points. Tests: both preflights are covered, each asserting the mock registry client was never invoked — "exited non-zero" alone does not say nothing was written. Both fail with the preflight removed. The publisher fixtures now pin the trusted repository the way a real rc tree does, and still send the candidate elsewhere via REGISTRY so the rewrite assertions keep their teeth. Also covers a malformed PROMOTION_ID. It is not attacker-influenced, but it is the run-uniqueness suffix of the candidate tag and retention.yaml deletes a candidate only when every tag on the manifest matches `promotion-v…-run-[0-9]+-[0-9]+`; a value outside that shape publishes an artifact no cleanup pass can ever collect. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:58 +05:00
Myasnikov Daniil	54e8533a9d	fix(release): compare executable bits unambiguously in the verifier The mode check was written as a single chain: if { [ -x "$a" ] && [ ! -x "$r" ] \|\| [ ! -x "$a" ] && [ -x "$r" ]; } which reads like a symmetric difference and is not one. POSIX gives `&&` and `\|\|` equal precedence and left associativity, so it groups as `((A && B) \|\| C) && D` with `D = [ -x "$r" ]`. A file executable in the candidate and not in the release tree makes `A && B` true but `D` false, so the whole condition is false and the mismatch is accepted. Only the mirror case was rejected — and it is the benign one. The direction that got through is content the merge reviewer never saw made runnable inside the very artifact the operator installs, which cmp(1) cannot see because it compares bytes. Resolve each side to a value and compare those, so no precedence rule is load-bearing. Both directions now have coverage. Before the fix the candidate-only test failed against the shipped condition and the release-only test passed, which is what makes the pair meaningful rather than a check that only goes red when the whole block is deleted. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:57 +05:00
Myasnikov Daniil	5d56681f8c	fix(ci): put every label guard in lockstep, forks included Promotion needs the `release` label to start work, so this PR added it to the set of labels whose `labeled` event is not discarded. That set is stated in four places and was updated in three. `gate-concurrency-contract.bats` caught the shape of the problem and then became the first casualty of it. It asserted that the concurrency key excludes exactly ONE label from the `-label` group, by counting occurrences of `github.event.label.name != '`. A second publishing label made the count 2, turning the suite red — and with it `checks`, then `finalize`, then `e2e`, then the required `E2E Tests` status. Raising the constant to 2 would re-hardcode the same brittleness one label later and throw away the invariant the test exists for: the set of labels excluded from the `-label` suffix is exactly the set `plan` runs for. Both sets are now extracted from the workflow and compared, so the assertion holds at any number of labels. `resolve_assets` was the guard that got missed. It sits on `e2e`'s release arm, so a label event `plan` runs for and it skips for leaves `e2e` with both arms false — `finalize` is force-skipped for release PRs — while `e2e-report` still runs. On a promote PR already carrying `full-e2e`, re-applying `release` therefore superseded the green run and posted `E2E Tests` = failure, because `full-e2e` bypasses the "e2e not repeated" branch and `E2E_RESULT` is `skipped`. `unlabeled` is not a trigger, so removing the label could not clear it; only a push could. `release` must also not publish on the fork lane. e2e-fork.yaml's `resolve` decides "this run built nothing, so it is not a verdict" by asking whether `Plan build` skipped. Admitting `release` at `plan` made that false: a fork run reaches `plan`, has `build`/`finalize` gated off by its own label, succeeds having produced no pr-patch, and the fork lane fails closed on the missing artifact — red on a suite that was never going to run, on a PR whose only fault is a mislabel. `plan` and `resolve_assets` now admit `release` for same-repo PRs only, and the concurrency key carries the complementary term, so NOT(A OR B OR (C AND NOT F)) stays exactly (NOT A) AND (NOT B) AND (NOT C OR F). For a same-repo PR F is false and every one of them reduces to what it was, so none of this changes non-fork behaviour. The same shape still reaches the fork lane from a PUSH to a fork PR somebody mislabelled `release` — `build` is gated on the label, not on the event, so this predates promotion using the label. Left alone; the comment in e2e-fork.yaml now records it instead of asserting an invariant this PR would have made false. The contract enumerated `plan` and `e2e-report` by name, which is why it did not catch `resolve_assets`: a named-job loop cannot notice a job nobody thought to name. It now derives the job list from the file and requires every job that decides from a label name to decide from the same set, and pins the fork term on both halves of the complement. The file's existing properties are kept — extraction runs over executable-only lines, so a commented-out key cannot satisfy the contract, and every comparison asserts a non-empty input first, so an empty extraction fails closed under both bats and cozytest.sh. Verified by mutation, each caught: excluding a label `plan` skips for, admitting one the key does not exclude, reverting `resolve_assets`, deleting `e2e-report`'s guard, narrowing `verify-release-candidate`'s, renaming the label in one guard only, commenting out the group key, and dropping the fork term from either side alone. Deleting `plan`'s step-level fork guard is caught too, by an assertion that had to stop matching on the bare identifier once `plan`'s own `if:` named it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:57 +05:00
Myasnikov Daniil	f21d277eba	fix(release): retain live promotion candidates Protect package candidates referenced by open release PRs while pruning abandoned run-specific promotion artifacts. Document the stable-candidate lifecycle and maintenance-branch rollout requirement. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:57 +05:00
Myasnikov Daniil	c2e145709b	fix(release): publish stable packages candidate Serialize the tag-rewritten packages tree during RC promotion and pin its immutable digest in the installer. Verify the candidate against the prospective merge and the original RC artifact before creating stable names. Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>	2026-08-14 18:37:57 +05:00
Aleksei Sviridkin	61edf33ef4	fix(e2e): capture both ends of the worker byte path on node-join failure A tenant worker that misses the node-Ready deadline while its kubelet image pull crawls leaves two mechanisms behind one symptom, and the artifact could not tell them apart. Either the link is slow, or the pull keeps restarting and discarding what it fetched; from inside the guest both are a progress line that barely moves. Per-worker network counters, read from the kubelet's cAdvisor endpoint the CPU throttling capture already uses, count every byte that crossed into the Pod including the ones a restart threw away, so the gap between that and the progress the guest reported separates the two. Which row carries it is not obvious and getting it wrong inverts the reading, so the collector states it. cAdvisor publishes one row per interface in the Pod netns, and a bridge-bound worker has four: the one still named eth0 is a dummy holding the Pod IP, carries no traffic, and reports zero -- the row a reader reaches for first, where zero reads as a worker that received nothing. The renamed NIC's receive is the number that answers; the bridge counts the same bytes again and the tap is named from the host's end. No interface predicate is applied, so all four reach the artifact and can be told apart. Two further properties of the family fail by capturing nothing rather than loudly: the dropped-packet series are spelled packets_dropped, and every row carries an empty container label because cAdvisor publishes them on the Pod sandbox, so a filter borrowed from the CPU counters beside it matches nothing at all. Both are stated where the filter is. The fixture's rows are real kubelet output relabelled; its interface names are read from the KubeVirt version this repo ships rather than sampled, since no virt-launcher Pod was available. The guest side gains the service state machine and the link table. Talos creates a service's log buffer when its runner opens the log writer, as it starts the process, so a kubelet still waiting on its volumes, on time sync, or on the container runtime reporting healthy never reached its runner, has no log at all, and the capture recorded an rpc error naming a log that was never registered: true about the log, silent about the service, while the state and the awaited condition were the finding. The link table is read as yaml because MTU lives in the spec rather than among the printed columns, and it is the value that decides whether a path-MTU reading of a stalled transfer survives. Both carry a tighter bound than the two reads beside them, being single tabular RPCs rather than streams, and the walk they sit in is the term the phase budget sizes itself against. A worker whose reads all failed now says so once for the worker. Each log already carried its own error and exit code, which is accurate per file and adds up to a directory indistinguishable from a worker that had little to report. The phase budget drops from 8m to 7m to pay for the two added guest reads, because the inequality keeping the tenant snapshot reachable had ten seconds of slack. A shorter budget declines more, and what it declines first is what is gated last: the mirror capture, whose subject is partly recoverable from its own request counts and the host snapshot, and the image-cache re-probe, which has no substitute and was already the first to go. The second cost lands on the guest captures, which are gated after the new counters and reach their gate with both the shortened budget and a listing plus a read per node already spent -- up to 100s at the read bound and the three-node cap, a ceiling those numbers impose rather than a duration measured on a live cluster. Sharing that read would mean the two captures becoming one walk, which is left out for scope rather than merit: the phase gate is a deadline rather than a per-collector allowance, so one walk admitted at the first of the two gates would cost less and collect more. The cost is stated rather than netted away, and the source says which way the argument runs. Both captures run one shared body, which also settles what the two copies had already broken once: the audit that catches an unbounded read pins each collector by something only it produces, and two collectors issuing byte-identical reads left neither with anything of its own, so stubbing one out of the audit went unnoticed. It is pinned by the report directory each writes. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-14 14:34:37 +03:00
Aleksei Sviridkin	829edfadb3	test(e2e): warm the ghcr.io mirror before the tenant suites need it The in-sandbox pull-through cache fetches a blob only when a client asks for it, so the first tenant worker's kubelet pull is a live fetch from ghcr.io. That leg is what misses the node-Ready budget: measured at 40-165 KB/s for a 61 MiB image, with two blob streams dying mid-transfer after 7-8 MiB on an upstream HTTP/2 reset. Talos holds the kubelet service in PreFunc until the pull completes, so the node never registers, and the tenant cilium HelmRelease then fails for want of a schedulable node rather than for any fault of its own. Fill the cache from a Job started in the same install step that deploys the mirror, tens of minutes before the kubernetes suites run. The Job waits for the mirror to serve, then asks it for each kubelet tag's index, walks the manifests it gets back, and discards the blob bodies; distribution populates its blobstore on the way past. It warms one platform per tag -- the one the workers run -- picked out of the index by record, which keeps the walk a grep. Walking both platforms was a true addition to the same throttled leg: arm64 is bytes nothing in the sandbox pulls, and the digest ordering put it first for both tags, so on the slow runs this exists for the deadline expired with the second tag still cold. It warms the two minors the suites select, not the whole version map. The map would be ~300 MiB at the one-platform walk this ships (~61 MiB per tag), three fifths of it for minors no suite can resolve, spent on the same throttled egress the rest of the install is using. The tag list still comes from the chart's map rather than a copy, ordered the way the suites resolve it, so the set is theirs without restating their expression. The readiness wait is a wall clock, not a per-attempt count. Cilium answers a Service with no ready endpoints by rejecting, so a probe costs nothing when the mirror is absent and the full timeout when it is merely slow; a constant was wrong in both directions across two attempts at picking one. Tags are refused by character class before the pattern is applied, so a value cannot pass by having a valid first line: grep matches per line and the pattern's anchor is end-of-line, not end-of-input. The Job exits non-zero on any miss, so backoffLimit is a real bound and a warm-up that reached nothing cannot report success. It names its own requests, because the node-join diagnostic counts kubelet GETs in the access log to tell a worker that pulled through the mirror from one that fell back to public ghcr.io. Naming them is only half of that. The same diagnostic now reports the Job's status and result and the Job outlives the suites, because excluding the warm-up without reporting it would leave a red run unable to distinguish a warm cache from a warm-up that never started. That counter also moved onto the access-line shape: registry:2.8.3 writes three lines carrying the path per request, so matching the path counted one pull as three. It reads the repository from the same variable the warm-up does, so a rename cannot leave the diagnostic counting zero while the workers pull under the new name. GET and HEAD both count: containerd resolves a manifest with a HEAD first, so a GET-anchored count reports a pull that died before its first GET as no request at all. The failure path also prints how long the mirror took to answer the worker. A warm cache and a cold one are otherwise indistinguishable in the artifacts -- the Job reports success either way, because the proxy stores the blob in the background and a completed GET is a different event from a stored blob -- and server-side duration separates them by orders of magnitude. It filters the log in the shell that read it: a single argv element is capped at 131072 bytes on Linux regardless of ARG_MAX, and probe traffic carries this log past that within about twenty minutes of mirror uptime, so handing it to a helper would empty the section on every run worth reading. Nothing is printed when the log could not be read at all, because a heading with no lines under it reads as a finding about the workers. Measured against a local pull-through registry using the script the builder emits: 32m47s cold, 1.1s warm for the same tags, and the blobs it fetches are the same digests the failing runs stalled on. A warm-up that fails, times out or never starts does not fail the step, and the suites behave as they did before it existed. It is not free, though: it pulls across the same throttled egress the install is using, which is why the warm set is two tags and not five. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-14 11:08:49 +03:00
Aleksei Sviridkin	9dc49de1c9	fix(apps): make the post-delete cleanup hooks report failure (#3593 ) ## What this PR does The qdrant and harbor post-delete cleanup hooks could not fail. Every delete was guarded by `\|\| echo "WARNING: ..." >&2`, neither script set `-e`, and both ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook, `helm.sh/hook-delete-policy: hook-succeeded` removed the Job together with the pod that held the warning, and an uninstall that reclaimed nothing looked exactly like one that had nothing to reclaim. Written up in #3566. Both hooks now count failures and set the exit code at the end. `set -e` would abort at the first failed delete and skip the rest of the sweep, which is the opposite of what is wanted: every remaining object should still be attempted and the outcome reported once. Already-gone objects stay a success, since every delete keeps `--ignore-not-found`. Each failure message names the object it left behind. `packages/apps/mariadb` already uses this shape. Both hooks also consumed `kubectl get` output directly: harbor by iterating `$(kubectl get ...)`, qdrant by piping it into `grep`, whose status the pipeline then reports. A failed listing arrived as an empty list, and the sweep reported success without having looked. Each listing is now captured before it is used. #3566 attributes that shape to harbor's two loops alone. qdrant has it too: `pvcs=$(kubectl get pvc ... \| grep -E ... \|\| true)` reports grep's status, so a `kubectl get` that failed left `$pvcs` empty and the hook announced "no data PVCs found; nothing to delete". Both instances are fixed here. ### The cost A hook that exits non-zero keeps its Job and its pods, because `hook-succeeded` does not fire for it. That is the point, the pods hold the log, but it means a failed uninstall now leaves objects behind where it previously left none. Nothing collects them on its own: `before-hook-creation` only fires when the hook runs again, and a `post-delete` hook runs only on an uninstall, so they stay until a release of that name is installed and uninstalled again, or until the namespace goes. The release itself is not stranded. Helm appends the post-delete hook error to its error list instead of returning on it, marks the release uninstalled, purges it from storage anyway, and only then reports `uninstallation completed with N error(s)`. The failure surfaces once and the app still goes away. Both Jobs get one retry instead of the Kubernetes default of six, and instead of none. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff separates a transient apiserver error from a real one. These failures become visible for the first time with this change, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, so Helm reports that timeout instead of the hook's own error. `packages/apps/clickhouse` and `packages/apps/mariadb` already exit non-zero from their cleanup hooks and set no retry limit, so a real failure in either spends the full backoff and reaches the operator as a timeout rather than as the error the hook wrote. Adding the exit code without the cap reproduces that. One limitation is deliberate. harbor's ACME sweep cannot tell "this kind is not served" from "I could not look". If cert-manager is torn down before the release, `kubectl get challenge` fails and the uninstall is reported failed with nothing to sweep. cert-manager is a hard dependency of the bundle harbor ships in, so the window is narrow, and a CRD-existence probe would add a second silent-skip path of its own: it would swallow a discovery failure the way this change stops swallowing a listing failure. ### Tests `hack/app-cleanup-hooks_test.bats` renders each hook's script out of its chart and runs it against a fake `kubectl`, because the property under test is the container's exit code and matching against manifest text cannot establish it. There are eight failure paths across the two hooks, six in harbor and two in qdrant, and each has a case that goes red when that one path alone is reverted to a swallowed warning. Also covered: a clean delete, and an object that is already gone in both its shapes, an empty listing and a `NotFound` on the delete itself. The fake `kubectl` models `--ignore-not-found` instead of exiting 0 unconditionally, so dropping that flag from a hook turns the already-gone cases red. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake names the object in its own diagnostic too, and an unanchored match is satisfied by the fixture. Two manifest fields are pinned in each chart's existing helm-unittest suite, where manifest fields belong. The retry cap is one. The other is `helm.sh/hook-delete-policy`: adding `hook-failed` to that annotation hands the failed Job straight back to Helm to delete, which undoes the whole change. I checked that before adding the assert, and the reversal left both suites fully green. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string this change removes, and concluded that Helm always collects the pod away. That conclusion inverts here, so the note is rewritten, along with the sentence in the harbor suite that points at it. ### Screenshots Not a UI change. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff, file by file. It touches two app chart templates and adds one file under `hack/`. No package added, renamed or removed, no `values.schema.json`, no `values.yaml` default, no version enum, no `ApplicationDefinition`, no namespace, no node prerequisite, and neither `hack/package.mk` nor `hack/update-crd.sh`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(apps): the qdrant and harbor post-delete cleanup hooks now exit non-zero when a delete or a listing fails, instead of reporting success on every path. `helm uninstall` reports the failure rather than returning clean, and the hook Job and its pods stay in the namespace so the log survives to be read. Nothing collects them on its own: they remain until a release of that name is installed and uninstalled again, or until the namespace is removed. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Improved Harbor and Qdrant cleanup reliability by detecting listing and deletion failures. - Cleanup jobs now retry once and report failures with a non-zero status. - Cleanup continues processing remaining resources after individual failures. - Successful cleanup remains safe and repeatable, including when resources are already missing. - Harbor cleanup remains limited to resources associated with the configured Harbor host. - Tests - Added comprehensive coverage for successful, repeated, partial, and failed cleanup scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-14 10:55:01 +03:00
Aleksei Sviridkin	340483fdf0	fix(hack): declare the shared package targets and the root test target phony (#3353 ) ## What this PR does `hack/package.mk` set `.PHONY` with `=` instead of `:`: ```make .PHONY=help show diff apply delete update image ``` That assigns a variable which happens to be named `.PHONY`; it declares nothing. `make -p` prints it back among the variables (`.PHONY = help show diff apply delete update image`) rather than as a special target, so none of the targets `package.mk` defines was phony in any package that includes it. Nothing in the tree collides with those names today, so this is preventive rather than a fix for present breakage. The failure it forecloses is quiet: with a directory named `image` beside a package Makefile, `make image` prints `make: 'image' is up to date.` and exits 0 without building. A file of that name is enough on its own where the rule has no prerequisites; where it has one, the recipe keeps running until that prerequisite is shadowed too. The list changed as well. `suspend`, `resume`, `check` and `clean` are defined in this file and were missing, and `check` is the one that matters most, since it is the prerequisite that keeps `show`, `diff`, `apply` and `delete` rebuilding. `update` and `image` are gone, because `package.mk` does not define them; the including package Makefile does. Naming an undefined target in `.PHONY` creates it as an empty target, turning `No rule to make target` (exit 2) into `Nothing to be done` (exit 0) wherever the rule is missing. That reaches CI too: `hack/build-matrix.sh` parses the root Makefile `build:` target and runs `make -C <pkg> image` per unit, so a package arriving in that list without an `image:` rule would go from a red job to a green one that builds nothing. `.DEFAULT_GOAL` on the line above keeps its `=`. That one is a real variable, so the assignment form is right there. A comment now says which of the two lines is which, since they read alike and one of them just changed shape. `hack/package-mk-phony.bats` covers the declaration and is picked up automatically by the `hack/.bats` glob in `bats-unit-tests`. Five of its six tests cover `package.mk`: that the declared names are really phony with a file of each name sitting beside the Makefile; a control that mutates a copy back to the assignment form and requires the probe to report up-to-date, so the first cannot pass vacuously; that the declared list and the defined targets are the same set, checked both directions; and two pinning the `update`/`image` exclusion, one requiring the loud failure to survive and one extending the list in a copy and requiring it to disappear. The loud-failure test pins both the exit status 2 and the diagnostic. A wrapper that kept make's wording while returning something else would pass a check on the message alone. One point decides where the guarantee actually lives. The first test reads its names from the `.PHONY:` line, so it cannot see a name dropped from that line at all. The set-equality test is what guards every name's presence. One known gap is left in place. The first test decides by matching make's `is up to date`, which make prints only for a target that has a recipe, and a recipe-less alias never draws it: with no prerequisite both forms print `Nothing to be done`, and with a prerequisite that is itself out of date, which every target in this file is for being phony, the working form prints that prerequisite's recipe while the broken form prints `Nothing to be done`. The probe separates neither pair, so such a name would pass the first test either way. Measured on both shapes, not assumed. Every target `package.mk` defines has a recipe, so nothing is uncovered today; the control test does not close the gap either, since an alias is a defined target and the control requires `is up to date` from it, which turns it red against the probe rather than against the alias. Worth closing if an alias is ever added. One more for whoever edits the suite next: `hack/cozytest.sh` rewrites every bare `}` at column 0 into `return 0` followed by `}`. That is aimed at `@test` blocks, but it does not distinguish them from a plain shell function, so a helper factored out of these tests would silently become unable to return a non-zero status. Tests 1 and 2 repeat a loop rather than share one for reasons of their own, but this is the trap waiting for anyone who merges them. Two properties of the suite are worth stating, because both are easy to get wrong in the other direction. The set-equality test extracts the defined targets with a pattern that accepts a rule naming several targets at once, so `suspend resume: check` keeps both names in the set; anchoring the colon to a single name drops every name on such a line, and for a target nothing else depends on that goes unnoticed. And the control test probes the targets `package.mk` defines rather than the names on its `.PHONY:` line, because `is up to date` is only a meaningful answer for a name that has a rule: a declared-but-undefined name prints `Nothing to be done` in both the working and the broken form, so probing it in the control fails against the probe, while the set-equality test holds the message that names the defect. Last one, about how the tests find the repo. All six read it as `repo=$(pwd)`, which assumes the runner starts at the repo root. That holds for `make bats-unit-tests` and for the invocation the header documents, so nothing is broken, and the assumption is the ordinary shape in `hack/` rather than an outlier: about half the bats files there resolve repo-relative paths against the cwd the same way, several stating it in their own headers, and this is not even the only one spelling it with `$(pwd)`. What is worth naming is the failure mode: started elsewhere, these tests go red with `no '.PHONY:' declaration found in hack/package.mk`, which reads like a real finding rather than a wrong directory. The root `Makefile` had the same defect, and there it had already fired. `test` was not on its `.PHONY:` line and a `test/` directory sits beside it, so `make test` printed `'test' is up to date` and exited 0 without running anything, while `docs/agents/overview.md` advertises that command as the way to run the full suite. CI never used it: the workflow calls `test-controllers` and drives e2e out of `packages/core/testing`, so the cost fell on anyone following the docs by hand. One name on the declaration closes it, and the suite gained a sixth test that goes red without it. Every other undeclared target in that file is unshadowed today, so none of them is reachable the way `test` was, and they are left alone here. #3709 covers the same class in the four `packages/core/` Makefiles, which include only `hack/common-envs.mk` and so are out of reach of this change entirely. ### Downstream repositories `scripts/package.mk` in cozystack/external-apps-example was a byte-for-byte copy of `hack/package.mk` (identical sha256 before this change), carrying the same inert `.PHONY=` line. This PR splits the two files, and the copy was fixed separately. It landed the same nine names, and `update` and `image` are absent there for the reason they are absent here: `scripts/package.mk` does not define them either, and neither does any Makefile that includes it. What separates the two files now is the two-line comment added here, and nothing else. - [ ] 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: - [x] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: https://github.com/cozystack/external-apps-example/pull/4 - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Bug Fixes * Fixed task declaration handling so supported commands are correctly recognized and executed. * Updated available commands by replacing `update` and `image` with `suspend`, `resume`, `check`, and `clean`. * Tests * Added coverage to verify command declarations, prevent regressions, and ensure invalid or missing commands report errors correctly. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-14 10:54:10 +03:00
Aleksei Sviridkin	d978a371a2	test(hack): cover the .PHONY declarations this tree relies on `.PHONY=a b c` and `.PHONY: a b c` differ in the `=` against the `:`, and only the second declares anything. The first defines an ordinary variable named `.PHONY` that nothing reads, leaving every listed target file-backed. A file or directory of that name beside a package Makefile then stands in as the product of the rule: for a target with no prerequisites make reports it up to date, skips the recipe and exits 0, and a target whose prerequisite is itself file-less keeps rebuilding until that prerequisite is shadowed too. The neighbouring `.DEFAULT_GOAL=help` is a real variable and is correct with `=`, so the two lines read alike while only one of them works, which is what let the inert form sit there unnoticed. Add hack/package-mk-phony.bats, six tests, picked up automatically by the hack/*.bats glob in `bats-unit-tests`. Three cover the declaration itself. The first reads the names off the `.PHONY:` line rather than repeating them, plants a file named after each beside a stub Makefile, and requires `make --dry-run` to print the recipe instead of "is up to date". A negative control mutates a copy back to the assignment form and requires the probe to report up-to-date, over the targets package.mk defines rather than the names it declares, because "is up to date" is only a meaningful answer for a name that has a rule. The third keeps those two sets equal: it requires the declared list and the defined targets to be the same set. Defined but undeclared is the direction the list rots in: check, clean, suspend and resume sat undeclared for as long as the declaration itself was inert. Declared but undefined is the empty-target trap in its general form, so a future name cannot slip in the way update and image once did. Two more pin what the list deliberately leaves out. package.mk defines neither `update` nor `image`; the including package Makefile does, and most packages define neither. Naming them here would create them as empty targets everywhere, turning "No rule to make target" into "Nothing to be done". Since the CI build matrix is parsed from the root Makefile `build:` target and runs `make -C <pkg> image` per unit, a package reaching that list without an `image:` rule would go from a red job to a green one that builds no image. One test requires that loud failure to survive, pinning both of the observables it is made of: make's exit status 2 and its diagnostic. They are independent, and a wrapper that kept the wording while returning something else would satisfy a check on the message alone. Its control extends the declaration with the two names in a copy and requires the failure to disappear, so the pin tracks the list rather than some property of the harness. Every decoy carries one fixed timestamp. Six of the nine targets take check as a prerequisite, and apply and delete reach it through suspend as well, so touching the decoys in list order can leave check newer than its own dependents. Make then rebuilds them for that reason and prints a recipe whatever .PHONY says, and the probe reports success against a declaration that does nothing. Equal mtimes remove the question in any list order. Measured on GNU Make 4.3: list order left the per-target assertion inert for up to six of the nine, moving with where the clock tick fell, while 3.81 fits the loop inside a single timestamp and shows none of it. The control test plants its decoys the same way for the same reason, and goes red if that is undone in its own loop. The two loops are independent, so neither guards the other, and the header is what keeps the arrangement from being unpicked. A sixth test leaves package.mk and covers the root Makefile, where this class had already bitten: `test` is the one target in the tree with a path of its own name beside it. It probes with --dry-run because the recipe wants a live cluster, and fails loudly if that path ever goes away rather than passing on a question with no subject. `%-update` is not coverable: .PHONY matches target names literally and does not expand patterns. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-14 01:25:34 +03:00
IvanHunters	09dc7f47fb	feat(backups): add a native MongoDB (psmdb) backup-strategy driver (#3562 ) ## What this PR does Adds a native MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore into a differently-named instance, and point-in-time recovery — all through the unified `BackupJob` / `RestoreJob` / `BackupClass` / `Plan` interface (closes #3545). The driver orchestrates the operator's `PerconaServerMongoDBBackup` / `PerconaServerMongoDBRestore` CRDs: - BackupJob: creates a one-shot `PerconaServerMongoDBBackup`, polls `status.state`, and persists the S3 destination + storage descriptor (by reference — Secret name only, never a raw credential) onto the Cozystack `Backup` artifact. - RestoreJob: drives `PerconaServerMongoDBRestore` via `backupSource` (read back from the source backup status, with a snapshot fallback for a reaped operator CR) so the same artifact restores in-place or into a differently-named instance. PITR is exposed as `spec.options.recoveryTime` (RFC3339), matching the CNPG driver's option exactly; unrecognised `spec.options` keys are surfaced as a `UnknownRestoreOption` Warning event rather than silently dropped. Because psmdb resolves the backup storage by name against the cluster's `spec.backup.storages`, the driver names the chart's storage (`s3-storage`) via the `cozy-default-mongodb` Strategy rather than injecting it, and surfaces a clear `Ready=False` precondition when a target has backups disabled instead of hanging until the deadline. Wiring - `api/backups/strategy/v1alpha1`: `MongoDB` strategy type + generated CRD/deepcopy. - `internal/backupcontroller`: `psmdbstrategy_controller` + `psmdbtypes` / `mongodbapp` partial-schema mirrors; BackupJob/RestoreJob dispatch, supported-kinds, cleanup-on-delete branch, and scheme registration. - `backupstrategy-controller`: `cozy-default` MongoDB route, `cozy-default-mongodb` Strategy CR, and `psmdb.percona.com` RBAC (get/list/watch, +create on backups/restores, no delete — the driver does not own the S3 archive). - `apps/mongodb`: a `backup.insecureSkipTLSVerify` value (psmdb's s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed in-cluster seaweedfs. Default `false`, fully backward-compatible; regenerated schema/README/RD. Tests + e2e - Unit tests for the driver: applicationRef validation, ensure-idempotency (backup + restore), the backups-enabled/storage precondition gate, `backupSource` resolution (live-preferred / snapshot fallback / no-destination), snapshot round-trip, the producer-seam (`createMongoDBBackupArtifact`) driver-metadata, the cleanup-on-delete no-Velero-fall-through guard, and `recoveryTime` → psmdb-UTC mapping + unknown-option reporting. - `examples/backups/mongodb/` + a `mongodb-2-backup-roundtrip` Chainsaw suite that drives `run-all.sh` end-to-end (a sentinel document round-trips through S3 into a restored copy while the source stays untouched), per `docs/agents/e2e-testing.md`. - `docs/operations/backup-classes.md` updated (new route, the app-owns-the-storage model, and the `recoveryTime` PITR usage). Verified end-to-end on a live dev cluster against the platform-default `cozy-backups` bucket: backup via `cozy-default` reached the bucket and the Cozystack `Backup` went `Ready`, and a to-copy `RestoreJob` restored the sentinel into a differently-named instance with the source left intact. ### Screenshots N/A — no UI changes (controller, chart values, CRD, docs and tests only). ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file by file against the diff: - cozystack/website — the only app-facing change is `packages/apps/mongodb` (a new `backup.insecureSkipTLSVerify` value + regenerated `README.md`). The managed-app reference page is regenerated from the package `README.md` by the release bot for apps already in the website `Makefile` (mongodb is one), so it refreshes automatically on the next tag — no manual follow-up. `docs/operations/backup-classes.md` is not a website-mirrored doc in the trigger map. - cozystack/terraform-provider-cozystack — the one candidate trigger ("a field added to an app's `values.schema.json`") lands inside the mongodb `backup` block, which the provider's `internal/provider/mongodb_schema.go` explicitly states it does not manage ("The deprecated backup block, the bootstrap restore block, and detailed shardingConfig are not managed"). No enum, default, `release.prefix`, Secret/Service name, or hand-typed CRD (`Plan`, `RestoreJob`) changed; the new `strategy.backups.cozystack.io/MongoDB` CRD is opaque to the provider. Not affected. - All other repositories — no matching trigger (no `hack/` layout or make-target change, no namespace rename, no `ApplicationDefinition` semantics change, no `packages/core/platform` values, no telemetry metric or `cozy-proxy` label rename). - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(backups): add a native MongoDB (psmdb) backup-strategy driver — logical backup to the cozy-default BackupClass, restore into a differently-named instance, and point-in-time recovery via spec.options.recoveryTime ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * New Features * Added MongoDB backup and restore support, including scheduled and on-demand backups, restores to new or existing clusters, and point-in-time recovery. * Added configurable S3 TLS certificate verification for MongoDB backups, defaulting to secure verification. * Added MongoDB support to the default backup configuration. * Documentation * Added MongoDB backup and restore examples, workflows, and operational guidance. * Tests * Added automated coverage for MongoDB backup, restore, cleanup, and end-to-end scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-13 11:18:46 +03:00
Aleksei Sviridkin	61a236cd96	feat(ci): auto-apply triage labels to issues (#3574 ) ## What this PR does Issues that arrive through the API or `gh issue create` bypass the issue templates, so they carry no `triage/` label and nothing in the repository adds one afterwards. Listing every open issue through the same endpoint the workflow uses, `gh api --paginate "/repos/cozystack/cozystack/issues?state=open&per_page=100"`, returns 567 entries, 171 of them pull requests, at the time of writing. That leaves 396 open issues, and 252 of them carry no triage label at all. Every count in this description is a point-in-time reading of a backlog that keeps moving, so a later run of the same query will differ by a few. The triage queue is impossible to filter on in that state, and there is no way to see how much of the backlog nobody has looked at. This adds a workflow that labels an issue when it is opened or reopened, plus a daily sweep that backstops labels removed by hand and works through the existing backlog. An issue somebody has assigned, or marked with one of `priority/critical-urgent`, `priority/important-soon`, `priority/important-longterm` or `epic`, gets `triage/accepted`, since a maintainer has already looked at it. Everything else gets `triage/needs-triage`. An issue already carrying any `triage/` label is left alone, so the sweep never overwrites a decision someone made by hand, and pull requests returned by the same listing endpoint are skipped. Against the listing above, the sweep applies `triage/accepted` to 39 issues and `triage/needs-triage` to 213, and touches none of the 144 that are already triaged. Deleting a triage label by hand does not undo any of this. An issue left carrying no `triage/` label is relabeled by the next sweep, so a triage decision is changed by replacing the label. `AGENTS.md`'s labeling section now says so, because that is the file somebody reads before labeling by hand. Two conditions decide what counts as a signal. Membership of `stale.yaml`'s `exempt-issue-labels` is necessary: `triage/accepted` is itself on that list, so applying it does more than reset a clock, the issue stops being auto-closable, and a signal that is not already exempt would hand out that permanent reprieve as a side effect of being labeled. `priority/backlog`, the one tier `stale.yaml` deliberately leaves reapable, is excluded for exactly that reason. Membership is not sufficient, which is why the signal list is not simply the exempt list. `lifecycle/frozen` is exempt and still not a signal: a `lifecycle/` label states what the stale bot may do with an issue and says nothing about whether anybody reviewed it. Freezing an untriaged issue to keep the bot off it is the case where reading it as accepted is wrong. Of the five open issues whose only signal would have been `lifecycle/frozen`, one is blocked on an upstream project and carries `help wanted`, and one is an older feature request that an open epic now covers. Neither is ready to be worked on, and stamping either accepted would have kept it out of triage for good, since the sweep never revisits an issue that already carries a triage label. `epic` passes the same test from the other side: it is a maintainer-authored planning artifact rather than an inbound report, and all eight open issues whose only signal is `epic` are roadmap trackers that `needs-triage` would describe falsely. A contract test pins the rule and not the instance, so no label from the `lifecycle/` namespace can quietly become a signal later. `security/` stays out on the same grounds, though the case is closer: those labels record what is known about a vulnerability rather than whether anyone triaged the report, and the cost of being wrong runs the cheap way, since a `needs-triage` on a confirmed report is one click to remove while a wrong `accepted` is permanent because the sweep never revisits. One caveat stated plainly: the signals stand for a maintainer decision already recorded on the issue, which is a weaker claim than the one `triage/accepted` makes in `.github/labels.yml`, and `priority/important-longterm` says outright that the work may not be staffed. Of the two labels this workflow can write it is still the closer one, since `needs-triage` on an issue somebody already prioritised or took asks for a triage that has happened, and a maintainer who disagrees replaces the label in one click. An assignee is the single signal that is neither a label nor already exempt, so an issue whose only signal is an assignee does gain a permanent reprieve. That is the intended reading rather than a side effect: somebody took ownership, which makes it a known long-tail task rather than an abandoned one, and it is the same argument `stale.yaml`'s own comment makes for exempting `triage/accepted` in the first place. Three operational details. First, the sweep paces its writes a second apart and caps a run at 400, because GitHub allows 80 content-generating requests a minute and 500 an hour, this client carries no throttling plugin and no retries, and a throttled write answers `403`, which is exempt from retry anyway. Unpaced, the first pass over the backlog would not slow down when throttled, it would fail the job partway and leave the backlog half labeled. Whatever a run leaves behind stays unlabeled, so the next one picks it up. Second, adding a label bumps an issue's `updated_at`, which `stale.yaml` reads as activity, so the first sweep restarts the 60-day staleness clock on every issue it writes to, all 252 of them, and un-marks the 32 that carry `lifecycle/stale` today. Both are deliberate and one-time: the backlog gets triaged now and ages again from there, rather than being closed unread while it is still unlabeled. Third, a run writes a job summary with the counts, the issue numbers under each label, and both remainders it can leave behind: the write cap's deferral, and how much of the listing it never reached when a refusal stopped it early. A sweep over the backlog logs a line per issue, and the summary is what makes the outcome readable without paging through the 252 the current backlog produces. A write that fails does not take the rest of the run with it, but the two failure shapes are told apart rather than counted together. Any status other than a `403` or `429` lets the run carry on, most often a `404` or `410` from an issue transferred or deleted mid-sweep, since a failure about one issue says nothing about the next. A `403` or `429` means the token lost `issues: write` or the pacing stopped being enough, which applies to every write left in the run, so the sweep stops at the first one and the job fails loudly. Repeating a rejected request up to the cap is what gets an integration throttled harder. `workflow_dispatch` takes a `dry_run` input that logs what would be labeled without writing anything. It is on by default, so a manual run writes nothing until somebody unchecks the box, and `schedule` is the one event that writes without being asked. The cap applies in dry-run too, so a dry run predicts what a real one would do instead of printing a longer list nobody gets. The job asks for `issues: write` and everything else is read-only, the action it uses is pinned by digest, and a 20-minute timeout keeps a hung call from holding the queue for the default six hours. No pull request lane can exercise this workflow, so its first real execution is a sweep over every open issue with nothing having run it before. `hack/issue-triage-contract.bats` pins the executable lines that bound that blast radius: the pull-request skip, the already-triaged skip, the pacing rate and the write cap, the job timeout, the failure handling, the digest-pinned action, the token scopes, the serialised queue, the dry-run default and the derivation that does not depend on it, the cron ordering against `stale.yaml`, that both labels it writes exist in `.github/labels.yml`, that no accepted-signal label sits outside `stale.yaml`'s exempt list or inside the `lifecycle/` namespace, and that the summary is written with the `await` that makes it flush and accounts for every outcome a run can end on. Each assertion was checked against a mutated copy of the workflow to confirm it goes red rather than merely existing, with the mutations run through `hack/cozytest.sh` rather than `bats`. The two runners disagree about negated assertions, so a pin that `bats` reports as live can be dead under the runner CI actually uses, and every negative assertion in the file captures grep's status instead of leading with `!`. Suggested first step after merge: run the workflow by hand and read the summary before the 05:53 cron arms itself. The box arrives checked, so that run costs nothing. ### Screenshots Not a UI change, so there is nothing to show. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff. The diff adds two files, `.github/workflows/issue-triage.yaml` and `hack/issue-triage-contract.bats`, and appends one line to `AGENTS.md`. No row matches: it adds and renames no package, changes no chart values, no schema, no version enum, no `ApplicationDefinition`, no CRD, no namespace, no variant or bundle, no label or annotation any downstream repository matches on, no metric and no release asset. Its `hack/` rows name `hack/upload-assets.sh`, `hack/e2e-prepare-cluster.bats`, `hack/package.mk`, `hack/update-crd.sh` and `hack/common-envs.mk`, which downstream repositories copy, anchor on or mirror by hand, plus one row for moving or renaming anything under `hack/`; a new bats file added next to them is none of those. The map has no row for the agent documentation, and no downstream repository copies `AGENTS.md`. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note feat(ci): new and reopened issues now get a `triage/` label automatically, and a daily sweep labels any open issue that has none: `triage/accepted` when it is assigned or carries an `epic` or a priority above `priority/backlog`, `triage/needs-triage` otherwise. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * New Features * Added automated issue triage triggered by new or reopened issues, scheduled reviews, or manual runs. * Issues can be labeled as needing triage or accepted, with pull requests and already-triaged items excluded. * Added dry-run support and reporting for triage results. * Documentation * Added guidance for replacing existing triage labels when making decisions. * Tests * Added coverage for workflow behavior, permissions, scheduling, failure handling, and safety limits. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-13 02:41:54 +03:00
Aleksei Sviridkin	59878850f9	test(e2e): capture worker CPU throttling counters on node-join failure (#3723 ) <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does When a tenant worker misses the node-Ready deadline, the failure bundle currently cannot tell two different causes apart: a worker held at its own CFS ceiling, and a worker losing host CPU it was entitled to. Both look identical from outside the Pod, and node-level utilisation answers neither, because it shows what the node used rather than what the container was allowed. This adds a collector that reads the CFS counters and the ceiling itself from the kubelet's cAdvisor endpoint, for every tenant worker on every node in the sandbox. The counters are the only place that difference is recorded, and nothing else in the tree collects them for these workers. The motivating measurement is on nightly run 31290999592: the `compute` container reports 0.968 and 0.920 core against a `limits.cpu` of 1, which is 92-97% of a one-core ceiling. Whether the container ever actually met that ceiling is exactly what the artifact cannot say, and what these counters answer. Most of the change is the failure taxonomy rather than the read. An empty capture is the dangerous outcome here, because zero bytes reads as a container that never hit its ceiling -- the conclusion the collector exists to stop a reader reaching by default. So every way the capture can come up short gets its own sentence: the kubelet was not read, the read was cut short part way, the metric stream could not be read back on this runner, the kubelet answered and carried no series for this namespace, the namespace was there and the worker Pods were not. The one non-failure outcome is written down too: a container with no CPU limit puts a single line on the wire, which is also what a truncated read leaves behind, so it says which one it is. ### Ordering The collector runs first among the gated collectors, ahead of the guest serial console. Its ceiling is four bounded reads, about 100s of a 480s phase budget, so on a slow run the console capture starts that much later. That is a deliberate trade and not a side effect of putting the cheapest first: cost is not what settles the order, survivability of the answer is. The console evidence cannot be re-taken, and this collector's question has no other answer in the tree, so both go ahead of the captures whose state is partly recoverable from reads above them. ### Known limits, named rather than left to be found - The outcome ladder enumerates combinations of read status, filter status and result emptiness. Every new source of partial output needs a new rung, and that has already happened twice while this branch was in review. Deriving the label from the three axes instead would make a new source land in an existing axis. - The uncapped note is decided over the whole per-node file, so one quota line anywhere in it suppresses the note. That is the conservative direction -- it never claims uncapped over a file that shows a ceiling -- but it makes the note rare. - The messages say `see read-error.log` while the file on disk is `<node>.read-error.log`. That is the convention the neighbouring serial-console collector already uses, and changing one side alone would make the two inconsistent. - The `mkdir -p` for the report directory is unchecked, as it is in four neighbouring collectors in the same file. Worth fixing across the file rather than in one collector. - The section letters in the failure block no longer track emission order. They already did not before this change; this adds one more entry to the sequence. - The count of bats files sourcing the library, in `docs/agents/e2e-testing.md`, is corrected here from three to seven. Nothing pins it, so it will drift again. - The node walk iterates `for node in ${nodes}` unquoted, so pathname expansion applies alongside word splitting. Node names are RFC1123 so it cannot bite today; `while IFS= read -r node` would make that structural instead of incidental. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note test(e2e): capture tenant worker CPU throttling counters and their CFS ceiling when a node-join fails, so a worker held at its own limit can be told apart from one losing host CPU ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * New Features * Added worker CPU-throttling diagnostics to Kubernetes troubleshooting reports. * Reports now identify CPU quota gaps, partial reads, filtering issues, and incomplete results. * Added safeguards for bounded node traversal and configurable collection budgets. * Bug Fixes * Improved failure handling when diagnostic commands, timeouts, or metric reads are unavailable. * Tests * Expanded coverage for CPU-throttling capture, node-join diagnostics, timeout behavior, configuration validation, and partial failures. * Documentation * Updated diagnostic collector counts and collection-order documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-13 02:24:50 +03:00
Aleksei Sviridkin	3ad4d452fc	fix(tests): fail when a chart runs no Helm unit test suites (#3632 ) <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `helm unittest` is fail-closed on a suite it cannot parse and on one declaring `tests: []`, and fail-open on suites that are not there at all: it prints `Test Suites: 0 passed, 0 total` and exits 0. `hack/helm-unit-tests.sh` judges each package by that exit code alone, so a chart whose suites were deleted, moved, or renamed past the `tests/_test.yaml` glob reports success having asserted nothing. That is measurable in both directions rather than argued. Move `packages/system/velero/tests/velero_test.yaml` aside and run the script as it exists on main today: ```text Running tests in packages/system/velero helm unittest . ### Chart [ cozy-velero ] . Charts: 1 passed, 1 total Test Suites: 0 passed, 0 total Tests: 0 passed, 0 total ``` The run ends with `All Helm unit tests passed.` and exit 0. The chart passed, and the count of things it checked was zero. With this PR the same tree exits 1 and names the package and the reason. Neither `--strict` nor an explicit non-matching `-f` glob changes this on its own; all three forms exit 0, checked against plugin 1.1.1. So the script now reads the captured output and fails the package when the zero-suite line appears. Matching that line names a cause and a remedy where it applies, but the set of ways to run nothing is not closed. A recipe that never invokes `helm unittest`, and a `test:` rule that make finds nothing to do for, print no marker of any kind and exit 0, so no additional match reaches them; enumerating absences leaves a fresh hole every time a new shape turns up. So the script also requires the line a real run always emits, a `Test Suites:` count of at least one. That turns the question round: every way of asserting nothing exits 0, and "what did this run assert" is the question that has an answer. All 76 packages the script visits emit that line today. The tradeoff is that a `test` target which legitimately runs no `helm unittest` would need renaming or an opt-out, which is the same bargain the zero-suite check already strikes. The discovery gate has the same shape from the other side. `make -C dir -n test` succeeds against a file-backed target, so a path named `test` beside a package Makefile whose rule is not phony would make both the gate and the run exit 0 without the recipe ever firing. The check keys on what make reports rather than on the path existing, because a package that declares the target phony runs its recipe whatever sits next to it, and refusing that would be a false failure. The quoting around the target name differs between make 3.x and 4.x, so it accepts either opening quote; anchoring on the trailing quote alone would also match a sub-make reporting some other target whose name ends in `test`. That second check is guarding a live gap rather than a hypothetical one. `hack/package.mk` line 2 reads `.PHONY=help show diff apply delete update image`, which is a variable assignment and not a target declaration, so it makes nothing phony. Of the packages the script visits that define a `test` rule, 18 declare it phony in their own Makefile; the rest are unprotected if a path of that name ever appears. No such path exists today. Fixing the shared include is #3353 and is not this PR. One deliberate non-change, stated so it is not rediscovered as an oversight. Running the suite now captures output instead of streaming it, because a pipeline's exit status in POSIX `sh` reports the last command rather than `make`. Streaming reads better and loses the status, so it stays buffered. The run is pinned to `LC_ALL=C`, since both checks match English wording and a localized `make` would disarm one of them silently, which is the failure mode the whole change exists to remove. The guard is all-or-nothing, which is worth stating plainly. A chart that loses four of its five suites still reports `Test Suites: 1 passed` and passes; only total disappearance is caught. Tying the expected count to what each chart actually has would be a per-chart number to maintain, and a number maintained in one place while the suites move in another is the failure this repo has been paying for elsewhere, so the cheap check that catches the total loss is the one worth having. One behaviour worth stating because the message does not: a suite in which every test carries `skip:` reports `Test Suites: 0 passed, 1 skipped, 1 total` and exits 0, and the positive-evidence check refuses it. That refusal is intended, since a wholly skipped suite asserted nothing, but the message talks about suites expected under `tests/`, which in that case are present and skipped on purpose. No package is in that state today. Partial skips are unaffected: `2 passed, 1 skipped, 3 total` satisfies the check. Three known gaps in what the change ships, none of them a wrong statement. The positive-evidence message explains its cause but stops short of naming a remedy, where the other two messages both end in an action; the remedy for the legitimate case, renaming the target or giving it an opt-out, currently lives only in the code comment. That same message enumerates two ways a run reports nothing, and the skipped-suite case above is a third, so it reads as a diagnosis where it is really a list of the common causes. And no document in the tree states the convention this tightens: `docs/agents/overview.md` still describes the script accurately as running over every package that defines a `test` target, but the contract is now that such a package must also report at least one passing suite. All three are improvements to make on the next touch of these files rather than reasons to hold the change. One adjacent gap stays open, and it is worth naming rather than leaving someone to assume otherwise. This catches a chart whose suite files* vanished; it does not catch a package that loses its `test:` rule along with them. Nothing ties the presence of `tests/_test.yaml` to the presence of a `test` target, so a change that removes both is skipped in silence, and the script only complains when no package in the tree has a `test` target at all. The test named `a package with no test target is skipped, not failed` pins that permissiveness deliberately, because the many packages that legitimately define no `test` rule would otherwise turn every run red. Closing it needs an instrument keyed on the suite files rather than on the Makefile target, which is a separate change. `hack/helm-unit-tests.bats` adds eight tests, picked up automatically by the `hack/.bats` glob in `make unit-tests`. Three pin the new refusals, and each matches its own message so none can stay green on a refusal that came from another. Two pin what must not be refused: a phony target with a colliding path, and a sub-make reporting an unrelated target up to date. One pins that a chart which does run suites still passes, which is what stops the refusals being satisfied by rejecting everything. Two pin behaviour that was already there, namely that a failing package is still reported by name and that a package with no `test` rule is skipped rather than failed. That last one builds a tree carrying a second package that does run a suite: with only the target-less package present, the script takes an early exit above the failure summary and the assertion would hold whatever the script had done. The fixtures stub `make` output instead of invoking helm, so they need no plugin installed. Relates to #3453. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tests): `make helm-unit-tests` now fails a chart that runs no Helm unit test suites, instead of reporting success for a chart whose suite files were deleted, moved, or renamed out of the `tests/_test.yaml` glob ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Bug Fixes * Improved Helm test validation to detect skipped test recipes, zero discovered suites, and runs without evidence of a passing suite. * Helm test failures now provide clearer diagnostics, including affected directories and failed commands. * Test output is consistently captured and replayed for easier troubleshooting. * Tests * Added comprehensive coverage for successful, failed, skipped, empty, and invalid Helm test scenarios. * Expanded coverage across package test targets, including phony and unrelated sub-make behavior. * Documentation * Clarified requirements for package test targets and suite reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-12 23:37:43 +03:00
Aleksei Sviridkin	94c41d100f	fix(hack): fail when a chart runs no Helm unit test suites helm-unittest is fail-closed on a suite it cannot parse and on one declaring `tests: []`, and fail-open on suites that are not there at all: it prints "Test Suites: 0 passed, 0 total" and exits 0. Measured on plugin 1.1.1 against a throwaway chart, all three forms. hack/helm-unit-tests.sh judges each package by that exit code alone, so a chart whose suites were deleted, moved, or renamed past the tests/*_test.yaml glob reports success having asserted nothing. Moving packages/system/velero/tests aside reproduces it: the script prints "All Helm unit tests passed" and exits 0. Check the captured output for the zero-suite line and fail the package when it appears. Neither `--strict` nor an explicit non-matching `-f` glob does this on its own; both exit 0. Matching that line names the cause where it applies, but the set of ways to run nothing is not closed: a recipe that never invokes helm-unittest, and a `test:` rule make finds nothing to do for, both print no marker at all, so no further match reaches them. So also require the line a real run always emits, a `Test Suites:` count of at least one. Every way of asserting nothing exits 0, and asking what the run asserted is the question that has an answer. All 77 packages the script visits emit that line today. The tradeoff is that a `test` target which legitimately runs no helm-unittest would need renaming or an opt-out, which is the bargain the zero-suite check already struck. That is a contract contributors have to know about, so docs/agents/overview.md states it where it describes the sweep. Because the check matches helm-unittest's own summary wording, a plugin upgrade that reworded it would fail every package at once while blaming each package's Makefile. The error text says so, so the reader of a tree-wide red has the real cause in front of them rather than 77 copies of the wrong one. The discovery gate has the same shape from the other side. `make -C dir -n test` succeeds against a file-backed target, so a path named `test` beside a package Makefile whose rule is not phony would make both the gate and the run exit 0 without the recipe firing. Of the packages the script visits that define a `test` rule, 18 declare it phony, and no such path exists today. Match what make reports rather than looking for the path, because a package that declares the target phony runs its recipe whatever sits next to it, and refusing that would be a false failure. The quoting around the target name differs between make 3.x and 4.x, so the match accepts either opening quote; the trailing quote alone would also match a sub-make reporting another target whose name ends in "test". Running the suite now captures output rather than streaming it, because a pipeline's exit status in POSIX sh reports the last command rather than make. The capture file is allocated once and removed by the EXIT trap that already owns the failure list, and the run is pinned to LC_ALL=C: all three checks match English wording, one on make's and two on helm-unittest's, so a localized make would disarm the first of them silently, which is the failure mode this guard exists to remove. The locale reaches every package's recipe, not only make. Extend hack/helm-unit-tests.bats with eight tests. Three pin the new refusals; two pin what they must not refuse, a phony target with a colliding path and a sub-make reporting an unrelated target up to date; one pins that a chart which does run suites still passes; two pin behaviour that was already there, namely that a failing package is still reported by name and a package with no `test` rule is skipped rather than failed. That last one carries a second package that does run a suite, because with only the target-less package present the script takes its early exit above the failure summary and the assertion holds whatever the script did. The fixtures stub make output instead of invoking helm, so they need no plugin installed. Both tests that assert a package is named in the failure summary match the summary's " - " prefix rather than the bare path. The script announces "Running tests in $dir" before it runs anything, so the bare path is present in the output whatever the summary does; matching it asserted nothing, and the summary could stop naming packages entirely with the suite still green. The failing-package fixture reports a passing suite count alongside its non-zero exit, so that the exit status is the only thing that can decide the outcome. A fixture that fails without printing the marker is refused by the positive-evidence check first, which leaves the exit-status branch unpinned. That shape occurs in the tree: a `test` target that runs helm-unittest and then a second step, a render-parity script or a vendored-version guard or go tests, emits the marker and still fails. The two tests already in that file cover a package reached under packages/tests, and their fixture printed a marker and nothing else. That is the shape the positive-evidence check refuses, so the fixture now also emits the line a real run emits; without it both would fail for a reason unrelated to what they assert. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-12 18:45:22 +03:00
Aleksei Sviridkin	b8a5b9ba1d	test(e2e): capture worker CPU throttling counters on node-join failure When a tenant worker misses the node-Ready deadline, the sandbox node it runs on is routinely at half its capacity with a clean kernel log. In that state "the host starved the VM" and "the VM was held at its own CFS ceiling" produce identical evidence: node-level utilisation reports what the node used, never what the container was allowed. The CFS counters are the only place the difference is recorded, and nothing in the tree collected them for the tenant workers, so every diagnosis of this failure so far had to be inferred from outside. Read them from the kubelet's cAdvisor endpoint on the node hosting each worker. Five series carry the whole answer and cAdvisor publishes all of them per container, so nothing is computed here: the scheduled and throttled period counts, the time spent throttled, and the quota and period that form the ceiling. The throttled counters alone say a container hit some ceiling, not which one, and a VM capped at one core and a VM capped at eight are the same number without the quota. Absence of the quota series is itself a reading rather than a gap. cAdvisor emits it only when the quota is non-zero, so a container reporting the period and no quota is one running uncapped - which is one of the two answers this collector exists to distinguish, arriving without being derived. Reading the kubelet rather than the container matters beyond convenience. A reader running inside the container shares the cgroup it measures, so it slows down in proportion to how much the answer matters and would need a budget sized for the pathological case; the kubelet is outside that cgroup. It also drops any dependency on what the container image ships and on how the host lays cgroups out. The namespace is not a worker filter and is not used as one: it also holds the tenant control plane, whose apiserver and etcd carry CPU limits of their own and can be genuinely throttled. The Pod name carries the second half of the filter, so a throttled apiserver cannot answer under a heading that says worker. The walk is over nodes rather than Pods, because the endpoint is per node and one read covers every worker it hosts. It is gated on the diagnostics phase budget, and placed first among the five gated collectors: the budget declines whatever has not started when it runs out, its own ceiling is four bounded reads rather than the minutes the four after it can spend, and it is the only one whose question has no other answer in the tree. That order is a trade, stated here because it is a decision rather than a detail - it puts the guest serial console one place further from the budget, and that capture is the only one describing a worker which never reached apid. Both reads take the block's own read-budget knob rather than a literal equal to it, so lowering that knob lowers these too. Each keeps its own exit status rather than routing through the shared helper, which always returns zero by design: this collector has to tell "the kubelet was not read" from "the kubelet answered and carried no worker", and only the status separates them. The stream is captured whole and filtered after, since a pipeline reports its last command's status and would otherwise hand that decision to grep. Every way a read can come up short is named rather than left as zero bytes: a kubelet that was not read, one that answered with nothing for the namespace, a stream cut short, and exit 124 - which cannot distinguish this collector timing out from the read exiting 124 on its own - each get their own line, and each says whether an error log exists to explain it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-12 18:43:11 +03:00
Aleksei Sviridkin	e9411d6b8c	fix(apps): make the post-delete cleanup hooks report failure The qdrant and harbor post-delete hooks guarded every delete with `\|\| echo "WARNING: ..." >&2`, set no `-e`, and ended on an unconditional completion message, so the container exited 0 on every path. Helm read that as a successful hook and the hook-succeeded delete policy removed the Job together with the pod that carried the warning, leaving an uninstall that reclaimed nothing indistinguishable from one that had nothing to reclaim. Count the failures instead and set the exit code at the end, so a failed delete does not skip the deletes that follow it. An object that is already gone stays a success: every delete keeps --ignore-not-found, which exits 0 in that case. This is the shape packages/apps/mariadb already uses for the same problem. Both hooks also consumed the output of `kubectl get` directly -- harbor by iterating it, qdrant by piping it into grep, whose status the pipeline then reports. A listing that failed therefore arrived as an empty list and the sweep was skipped rather than reported. Each listing is now captured before it is used, so a listing that failed and one that returned nothing are different events. A hook that exits non-zero keeps its Job and pods, which is the point: they hold the log. Give the Job one retry rather than the Kubernetes default of six or none at all. Every delete is idempotent, so a second attempt re-issues an API call rather than re-running any logic, and one short backoff is what separates a transient apiserver error from a real one -- these failures become visible for the first time here, and a signal that fires on noise stops being read, including on the day it is right. Six is the far end of the same mistake: ten minutes of backoff across seven pods outlasts the hook timeout, and Helm reports that timeout instead of the hook's own error. The qdrant Chainsaw suite explained its post-delete pod collector by quoting the guard string removed here and concluding the pod is always collected away. That is now backwards: a failed cleanup is exactly when the pod survives and the collector is the evidence. Rewrite the note, and the sentence in the harbor suite pointing at it. hack/app-cleanup-hooks_test.bats renders both scripts and runs them against a fake kubectl. There are eight failure paths across the two hooks -- six in harbor, two in qdrant -- and each has a case that goes red when that one path alone is reverted to a swallowed warning. Message assertions are anchored on the hook's own `ERROR: ` prefix, because the fake kubectl names the object in its own diagnostic too and an unanchored match is satisfied by the fixture. The helm-unittest suites pin the retry limit and the hook delete policy: adding hook-failed to that annotation hands the Job back to Helm to delete on failure and destroys the evidence this change exists to keep, and until these asserts that reversal left both suites green. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-12 18:43:02 +03:00
Aleksei Sviridkin	91b9f58057	fix(hack): declare the shared package targets phony package.mk set .PHONY with `=` rather than `:`, which assigns a variable named .PHONY instead of declaring anything phony, so none of the targets package.mk defines was phony in any package that includes it. Declare exactly the concrete targets package.mk defines: help, show, diff, apply, delete, suspend, resume, check and clean. The earlier list named update and image, which package.mk does not define — naming an undefined target in .PHONY creates it as an empty target, so `make image` and `make update` would report success without running anything in the many packages that lack those recipes. It also omitted check and clean, the targets a same-named file or directory could actually shadow. The neighbouring .DEFAULT_GOAL is a variable and correctly keeps its equals sign. A comment now records which of the two adjacent lines is which, so the corrected one does not pull the other into its shape by analogy. The trigger map in docs/agents/contributing.md called the two files a byte-for-byte copy, which was exact rather than loose: they were the same object. They stop being one here, so the entry now records that they were identical and no longer are, and asks for a diff-and-port, the wording the map already uses for the other copy that has diverged. It deliberately does not enumerate which lines differ: that set moves as the copy catches up, and an entry that names it would be wrong again by the next port. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-12 18:42:58 +03:00
Aleksei Sviridkin	62204d4903	test(cozystack-controller): guard the Secret grant by effective permission The suite's "does not grant a blanket wildcard verb on secrets" assert was a notContains over a rule object. helm-unittest compares every field it is handed by value equality, and `any: true` changes which fields are compared rather than how, so no spelling of that assert bounds what a verbs list may hold: `verbs: ["get", ""]` satisfies all of them. Measured against the rendered ClusterRole, seven realistic spellings of a widened Secret grant left that assert green: a wildcard in resources, a wildcard in apiGroups, a "" appended to the declared verb list, an extra field beside the verbs, deletecollection spelled out, and two where the grant is added as a separate rule. Those last two leave the whole suite green - appending apiGroups:[''] resources:[''] verbs:[''] keeps the positive pin satisfied while the controller gains every verb on every Secret cluster-wide. Replace it with a bats guard over the rendered manifests. It unions the verbs that every ClusterRole and Role rule confers on core Secrets, expanding "" on each of the three axes, and compares that union with the declared set. Equality rather than an upper bound: the controller is cluster-scoped, so a widening reaches every tenant namespace, and a narrowing breaks the two reconcilers that share the grant. The positive pin stays in tests/rbac_test.yaml. Value equality is the right primitive for pinning a rule as written and the wrong one for bounding what the rule may become. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-12 18:37:09 +03:00
Aleksei Sviridkin	ea78d09dd0	feat(ci): auto-apply triage labels to issues Issues created via the API or gh bypass the issue templates and arrive without any triage/* label; 252 of the 396 open issues carry none. Label issues on arrival (opened/reopened) and run a daily sweep as a backstop: issues already prioritised, assigned, or marked as epics get triage/accepted, the rest triage/needs-triage. A signal has to be on stale.yaml's exempt-issue-labels, because triage/accepted is itself exempt and a signal that is not would grant a permanent stale reprieve as a side effect of being labeled. That is necessary and not sufficient: lifecycle/frozen is exempt and still not a signal, because a lifecycle/* label says what the stale bot may do with an issue and nothing about whether anybody reviewed it. The sweep paces its writes and caps how many it makes per run, so a pass over a large backlog stays inside GitHub's secondary rate limits instead of failing partway through. Each run writes a job summary with the counts, the issue numbers per label, and both remainders it can leave behind, because a backlog pass logs a line per issue and nobody reads 252 of those to find out what happened. The manual dispatch defaults to dry-run, so the one entry point a human can reach writes nothing until somebody unchecks the box. The schedule is the one event that writes without being asked. Deleting a triage label does not undo a decision: an issue left with none is relabeled by the next sweep, so AGENTS.md's labeling section says to change a decision by replacing the label instead. No PR lane can exercise this workflow, so the first real run is a sweep over every open issue. A contract test pins the executable lines that bound that: the pull-request skip, the already-triaged skip, the pacing and the cap, the dry-run default, the pinned action, the token scopes, that no signal comes from the lifecycle/* namespace, and that the summary is written with the await that makes it flush. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>	2026-08-12 16:35:36 +03:00
myasnikovdaniil	516be4abfe	fix(cozystack-basics): normalize tenant hostname VAP comparisons with lowerAscii (#3208 ) ## What this PR does The Ingress, Gateway, and Route hostname `ValidatingAdmissionPolicy` objects compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is always lowercase — the apiserver's hostname validation permits nothing else — but both apex operands can carry uppercase: the `namespace.cozystack.io/host` label value, which Kubernetes permits to carry uppercase and which the tenant chart writes verbatim from `tenant.spec.host`, and the templated `_cluster.root-host`, a free-form platform value that nothing validates for case. A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase `_cluster.root-host` is the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy admits any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including a hostname belonging to another tenant and the root apex itself. Only the anti-wildcard gate still holds. This normalizes both operands of every hostname comparison with `.lowerAscii()` (host, apex label, and the templated root-host literal) across all three policies, so the match follows DNS case semantics. There is no behavior change in the default all-lowercase configuration. The Gateway policy's fail-open on an absent or empty host label is deliberately left as it was: that is the platform's one documented fail-open exception in `docs/security/threat-model.md`, so changing the posture is a security-model decision rather than a side effect of normalizing case. Its current shape is pinned by a test. Test coverage: - helm-unittest pins each comparison as a whole operand pair — equality and `endsWith`, across the rule-host and tls-host branches — so dropping the normalization from any single operand is caught. A pattern matching a lone `.lowerAscii()` would not: the apex appears four times in the Ingress expression, so stripping one operand still leaves three matches. - A rendering case with `_cluster.root-host: Example.ORG` pins the outside-root escape shut; it is red pre-fix. - e2e probes set a mixed-case apex on an isolated throwaway namespace and assert both halves of the match: a lowercase in-apex HTTPRoute and Ingress are still admitted (red pre-fix, green post-fix), while a foreign apex and a hostname ending with the apex without a dot boundary are still denied. Vacuous-pass guards re-read the persisted label and assert the probe apex really is mixed-case. Normalizing the apex at the write site — the tenant namespace template and `_cluster.root-host` — is tracked separately in #3328. It would not remove the need for this change: the host label is settable out-of-band by a trusted caller, so these policies must not assume the apex arrives normalized. Closes #3205. ### Screenshots N/A (no UI changes). ### Release note ```release-note fix(cozystack-basics): tenant hostname admission policies now compare against the tenant apex and the platform root apex case-insensitively. This closes a bypass where an uppercase `_cluster.root-host` made the Ingress policy's outside-root exclusion stop matching, so the policy admitted any non-wildcard host — including a hostname under the platform apex belonging to another tenant, and the root apex itself. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * Bug Fixes * Hostname validation now consistently matches case-insensitively across Gateway, Ingress, HTTPRoute, and TLSRoute. * Mixed-case tenant and apex hostnames are accepted when they meet the intended domain boundaries. * Foreign domains and prefix-adjacent hostnames remain correctly rejected; validation behaves as documented when the tenant host is empty. * Tests * Expanded unit and e2e coverage for case-insensitive hostname rendering and boundary rejections across the supported resources. <!-- end of auto-generated comment: release notes by coderabbit.ai -->	2026-08-12 15:35:41 +05:00

1 2 3 4 5 ...