Timeout annotations are read from an ApplicationDefinition and therefore
affect every Application of that kind. Describing them as per-Application
hides their actual scope and the upgrade override precedence.
Fixescozystack/cozystack#4382
Assisted-by: LLM
Signed-off-by: frolov.so <opsmon.sh@yandex.com>
release.cozystack.io/helm-install-timeout sets both Install.Timeout and
Upgrade.Timeout to one value. Add release.cozystack.io/helm-upgrade-timeout
so an ApplicationDefinition can override only Upgrade.Timeout — letting a
kind keep a short install budget but a longer upgrade budget (or vice
versa). When set it wins over the value the install annotation would
otherwise apply to the upgrade side.
- config: add HelmUpgradeTimeoutAnnotation + ReleaseConfig.HelmUpgradeTimeout;
generalize ParseHelmInstallTimeoutAnnotation to ParseHelmTimeoutAnnotation
since it now parses both annotations (identical Flux unit validation).
- server: parse the new annotation at startup with the same loud-fail
behavior; update --helmrelease-upgrade-timeout help.
- rest: apply HelmUpgradeTimeout after the install-annotation copy so the
upgrade-specific value wins; restore the accurate carrier comment.
- tests: cover the upgrade-only override, the asymmetric precedence, and
the install-sets-both regression guard.
Behavior is unchanged for every existing ApplicationDefinition (none set
the new annotation today).
Address review feedback from coderabbitai on rest.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The XValidation justification was factually wrong: the deployed
helm-controller CRD has no rule rejecting a Strategy alongside a
Remediation entry (only .retryInterval is gated on Strategy.Name), so
the claimed "silently break every HR" safety net does not exist.
Reword the comment in all three test files to the real reason
Remediation stays nil: retries are driven by a single
Strategy.RetryInterval path, matching cozystack-operator's
PackageReconciler.buildHelmReleaseSpec.
Address review feedback from lexfrei on rest_helmrelease_spec_test.go,
rest_timeout_test.go and internal/operator/package_reconciler_test.go.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Production change:
- convertApplicationToHelmRelease now sets Install/Upgrade.Timeout
unconditionally (matching cozystack-operator's buildHelmReleaseSpec).
The previous "stay nil if zero" branch was a leaky test shim — it kept
fixtures with empty ReleaseConfig{} green at the cost of api/operator
drift.
Test changes:
- newRESTForTimeout seeds the new HelmRelease* globals with
production-shaped defaults so the unset-annotation case exercises
"global default applies" rather than "field stays nil".
- TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal is
table-driven and now covers asymmetric globals (install != upgrade)
with and without per-app override — guards against a regression that
only assigns one side under override.
- New start_test.go: table-driven validator coverage for each
malformed/non-positive duration flag, the negative MaxHistory branch,
the MaxHistory=0 unlimited branch, asymmetric timeout acceptance, and
a smoke test that NewCozyServerOptions defaults validate.
Refactor:
- Extracted parseAndValidateHelmReleaseFlags from Complete() so the
flag-validation portion is unit-testable without kubernetes I/O.
Documentation:
- Reworded --helmrelease-install/upgrade-timeout help so admins reading
--help see the dual-purpose annotation explicitly (one annotation
overrides both timeouts together).
- Dropped PR-#2509 references and "preserving the pre-#2509 behaviour
for tests" comments — kept the technical rationale (Strategy.Name
vs Remediation.Retries:-1 + 5m Interval coupling) on its own merits.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
convertApplicationToHelmRelease now builds the HelmRelease spec from
ReleaseConfig instead of hardcoding values, bringing it in lockstep
with cozystack-operator's PackageReconciler.buildHelmReleaseSpec
(PR #2509). The drift between the two HelmRelease-generating paths
was the rationale for this follow-up.
Changes to the generated spec:
- Spec.Interval reads ReleaseConfig.HelmReleaseInterval (was
hardcoded 5m).
- Spec.MaxHistory set as &maxHistory pointer from
ReleaseConfig.HelmReleaseMaxHistory; 0 survives as pointer-to-0
(unlimited per Helm semantics) - mirrors the operator's
TestBuildHelmReleaseSpecZeroMaxHistory.
- Install.Strategy{Name: RetryOnFailure, RetryInterval} replaces
Install.Remediation{Retries: -1}. Same for Upgrade.Strategy.
Functionally equivalent to "retry forever, never remediate" for
the api-side path (which also never relied on remediation
firing) but decouples failed-install retry timing from
Spec.Interval.
- Install.Remediation / Upgrade.Remediation stay nil:
helm-controller's XValidation rejects RetryOnFailure +
RetryInterval alongside a Remediation entry, so reintroducing
Retries:-1 "for safety" would silently break every HR.
Install/Upgrade.Timeout precedence:
- per-Application HelmInstallTimeout (annotation-driven) wins
when non-zero (preserves the kubernetes-rd opt-in path);
- otherwise HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout
globals apply;
- when both are zero, the field stays nil and flux defaults
apply (preserves the legacy code path for tests that
hand-construct ReleaseConfig without populating the new globals).
Tests:
- rest_timeout_test.go: Remediation.Retries == -1 assertions
replaced with Strategy.Name == RetryOnFailure +
Remediation == nil, matching the operator's spec test.
- rest_helmrelease_spec_test.go: new file mirroring
TestBuildHelmReleaseSpec / TestBuildHelmReleaseSpecZeroMaxHistory
from internal/operator/package_reconciler_test.go, plus a third
case asserting per-Application HelmInstallTimeout overrides the
global Install/Upgrade timeout defaults.
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The image-busybox Makefile target + images/busybox/Dockerfile wrapper
just rebuilt an upstream busybox digest as ghcr.io/cozystack/cozystack/
busybox. Payload is a one-shot sh loop run once per pod; the pinned
upstream digest is already immutable, so maintaining a private mirror
adds churn (rebuild on every release) for no real hardening benefit.
Delete the wrapper and reference docker.io/library/busybox:<digest>
directly from images/busybox.tag. Document the choice in _helpers.tpl.
Also drop the false coupling in the go table test: the "unrelated
kind without configured timeout" case used the real kind name Qdrant,
which tied the test to the Qdrant ApplicationDefinition for no reason.
Switch to a clearly fictional kind so a future Qdrant timeout override
does not break this assertion.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Replace the hardcoded r.kindName == "Kubernetes" switch in rest.go
with a config-driven path. The ApplicationDefinition CR now accepts a
release.cozystack.io/helm-install-timeout annotation that is parsed
at cozystack-api startup into config.ReleaseConfig.HelmInstallTimeout
and applied to both Install.Timeout and Upgrade.Timeout on the
rendered HelmRelease. Applications that leave the annotation unset
keep flux defaults so their failed installs remediate on the normal
cadence - only the Kubernetes kind carries the override and gets a
15m budget. New kinds with a similar race can opt in by setting the
same annotation; no rest.go patch needed.
Kubernetes-rd sets the annotation to 15m. Table-driven test in
rest_timeout_test.go covers three cases: Kubernetes with 15m, Qdrant
unset, and an arbitrary future kind with 20m - all of which pin the
Remediation.Retries == -1 contract as well.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The previous change set Install.Timeout and Upgrade.Timeout to 15m
on every Application's parent HelmRelease, but the admin-kubeconfig
race documented in #2412 is specific to the Kubernetes kind: only its
parent chart creates CAPI/Kamaji resources whose admin-kubeconfig
Secret is asynchronously provisioned and mounted by Deployments in
the same chart. Other kinds (Qdrant, MongoDB, Postgres, ...) have no
such race and should not have their failed installs linger three times
longer before flux triggers remediation.
Gate the timeout on r.kindName == "Kubernetes". Rewrite
rest_timeout_test.go to cover both sides: Kubernetes must get a
>= 15m timeout, other kinds must keep the flux defaults. Both tests
also pin Install/Upgrade Remediation.Retries == -1 so a future edit
that removes unbounded remediation would show up as a red test.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Adds a failing unit test for convertApplicationToHelmRelease asserting
that Install.Timeout and Upgrade.Timeout are at least 15 minutes. The
default flux helm-controller timeout is too short to cover cold-start
Kamaji control-plane bootstrap (image pull + etcd bootstrap + apiserver
Ready + admin-kubeconfig Secret generation) and causes install
remediation loops.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
frolov.so