From fad709b0c13cd282dc668dbf9d67ded4685aecf2 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Wed, 6 May 2026 08:57:37 +0500 Subject: [PATCH 01/10] refactor(config): lift ParsePositiveDuration and extend ReleaseConfig Lift parsePositiveDuration from cmd/cozystack-operator/main.go into pkg/config so cozystack-api can reuse the same startup validator without duplication. Operator main.go now imports and delegates; the table-driven test moves to pkg/config/config_test.go. Extend ReleaseConfig with the five HelmRelease generation knobs the cozystack-operator already exposes via flags (PR #2509): HelmReleaseInterval, HelmReleaseRetryInterval, HelmReleaseInstallTimeout, HelmReleaseUpgradeTimeout, HelmReleaseMaxHistory. The existing per-Application HelmInstallTimeout (set from the release.cozystack.io/helm-install-timeout annotation) keeps its override semantics - it still wins over the new global Install/Upgrade timeout defaults when set. No behaviour change in this commit; the new fields are wired up in follow-up commits. Signed-off-by: Myasnikov Daniil --- cmd/cozystack-operator/main.go | 18 ++--------- cmd/cozystack-operator/main_test.go | 35 -------------------- pkg/config/config.go | 50 ++++++++++++++++++++++++++--- pkg/config/config_test.go | 39 ++++++++++++++++++++++ 4 files changed, 86 insertions(+), 56 deletions(-) diff --git a/cmd/cozystack-operator/main.go b/cmd/cozystack-operator/main.go index 2ca09c709..0a252dc7b 100644 --- a/cmd/cozystack-operator/main.go +++ b/cmd/cozystack-operator/main.go @@ -54,6 +54,7 @@ import ( "github.com/cozystack/cozystack/internal/fluxinstall" "github.com/cozystack/cozystack/internal/operator" "github.com/cozystack/cozystack/internal/telemetry" + "github.com/cozystack/cozystack/pkg/config" // +kubebuilder:scaffold:imports ) @@ -150,7 +151,7 @@ func main() { ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) parseFlag := func(flagName, raw string) time.Duration { - d, err := parsePositiveDuration(flagName, raw) + d, err := config.ParsePositiveDuration(flagName, raw) if err != nil { setupLog.Error(err, "invalid duration flag") os.Exit(1) @@ -439,21 +440,6 @@ func installPlatformSourceResource(ctx context.Context, k8sClient client.Client, return nil } -// parsePositiveDuration parses raw as a time.Duration and rejects malformed -// or non-positive values. Flux HelmRelease fields (Interval, Timeout, -// RetryInterval) require strictly positive durations, so a misconfigured -// flag must fail fast at startup rather than propagating into every HR. -func parsePositiveDuration(flagName, raw string) (time.Duration, error) { - d, err := time.ParseDuration(raw) - if err != nil { - return 0, fmt.Errorf("invalid duration for %s=%q: %w", flagName, raw, err) - } - if d <= 0 { - return 0, fmt.Errorf("%s must be > 0 (got %q)", flagName, raw) - } - return d, nil -} - // parsePlatformSourceURL parses the source URL and returns the source type and repository URL. // Supports formats: // - oci://registry.example.com/repo diff --git a/cmd/cozystack-operator/main_test.go b/cmd/cozystack-operator/main_test.go index 1f872e7ba..325da96b1 100644 --- a/cmd/cozystack-operator/main_test.go +++ b/cmd/cozystack-operator/main_test.go @@ -19,7 +19,6 @@ package main import ( "context" "testing" - "time" cozyv1alpha1 "github.com/cozystack/cozystack/api/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -574,37 +573,3 @@ func TestGenerateGitRepository_InvalidRef(t *testing.T) { } } -func TestParsePositiveDuration(t *testing.T) { - tests := []struct { - name string - raw string - want time.Duration - wantErr bool - }{ - {name: "valid seconds", raw: "30s", want: 30 * time.Second}, - {name: "valid minutes", raw: "5m", want: 5 * time.Minute}, - {name: "valid compound", raw: "1h30m", want: 90 * time.Minute}, - {name: "zero rejected", raw: "0s", wantErr: true}, - {name: "negative rejected", raw: "-5m", wantErr: true}, - {name: "malformed rejected", raw: "5x", wantErr: true}, - {name: "empty rejected", raw: "", wantErr: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := parsePositiveDuration("--test-flag", tt.raw) - if tt.wantErr { - if err == nil { - t.Fatalf("expected error for %q, got nil", tt.raw) - } - return - } - if err != nil { - t.Fatalf("unexpected error for %q: %v", tt.raw, err) - } - if got != tt.want { - t.Errorf("got %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/config/config.go b/pkg/config/config.go index 21de27b20..9c952ae30 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -58,6 +58,23 @@ func ParseHelmInstallTimeoutAnnotation(raw string) (time.Duration, error) { return d, nil } +// ParsePositiveDuration parses raw as a time.Duration and rejects malformed +// or non-positive values. Flux HelmRelease fields (Interval, Timeout, +// RetryInterval) require strictly positive durations, so a misconfigured +// flag must fail fast at startup rather than propagating into every HR. +// Shared between cozystack-operator and cozystack-api so both paths reject +// the same set of inputs at startup. +func ParsePositiveDuration(flagName, raw string) (time.Duration, error) { + d, err := time.ParseDuration(raw) + if err != nil { + return 0, fmt.Errorf("invalid duration for %s=%q: %w", flagName, raw, err) + } + if d <= 0 { + return 0, fmt.Errorf("%s must be > 0 (got %q)", flagName, raw) + } + return d, nil +} + // ResourceConfig represents the structure of the configuration file. type ResourceConfig struct { Resources []Resource `yaml:"resources"` @@ -83,12 +100,35 @@ type ReleaseConfig struct { Prefix string `yaml:"prefix"` Labels map[string]string `yaml:"labels"` ChartRef ChartRefConfig `yaml:"chartRef"` - // HelmInstallTimeout overrides the Flux HelmRelease Install.Timeout and - // Upgrade.Timeout for this Application kind. When zero, flux defaults - // apply. Populated from the - // release.cozystack.io/helm-install-timeout annotation on the - // ApplicationDefinition at start-up. + // HelmInstallTimeout is a per-Application override of Install.Timeout + // and Upgrade.Timeout. When non-zero, it wins over + // HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout below. + // Populated from the release.cozystack.io/helm-install-timeout + // annotation on the ApplicationDefinition at start-up. HelmInstallTimeout time.Duration `yaml:"helmInstallTimeout,omitempty"` + // HelmReleaseInterval is the global default for Spec.Interval on + // HelmReleases generated by cozystack-api. Set from the api server + // --helmrelease-interval flag; mirrors the cozystack-operator flag of + // the same name so both HelmRelease-generating paths use the same + // reconcile cadence. + HelmReleaseInterval time.Duration `yaml:"helmReleaseInterval,omitempty"` + // HelmReleaseRetryInterval is the global default for + // Install/Upgrade.Strategy.RetryInterval. Decoupled from + // HelmReleaseInterval so failed install/upgrade retries recover fast + // without polling healthy releases at the same cadence. + HelmReleaseRetryInterval time.Duration `yaml:"helmReleaseRetryInterval,omitempty"` + // HelmReleaseInstallTimeout is the global default for + // Spec.Install.Timeout. Overridden per-Application by HelmInstallTimeout + // when the annotation is set. + HelmReleaseInstallTimeout time.Duration `yaml:"helmReleaseInstallTimeout,omitempty"` + // HelmReleaseUpgradeTimeout is the global default for + // Spec.Upgrade.Timeout. Overridden per-Application by HelmInstallTimeout + // when the annotation is set. + HelmReleaseUpgradeTimeout time.Duration `yaml:"helmReleaseUpgradeTimeout,omitempty"` + // HelmReleaseMaxHistory is the global default for Spec.MaxHistory. + // 0 means unlimited per Helm semantics; matches the cozystack-operator + // flag of the same name. + HelmReleaseMaxHistory int `yaml:"helmReleaseMaxHistory,omitempty"` } // ChartRefConfig references a Flux source artifact for the Helm chart. diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index eb6990afd..632175f5f 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -117,3 +117,42 @@ func TestParseHelmInstallTimeoutAnnotation(t *testing.T) { }) } } + +// ParsePositiveDuration is the startup validator shared by cozystack-operator +// and cozystack-api. Both binaries reject zero/negative/malformed values so a +// misconfigured flag fails fast instead of propagating into every generated +// HelmRelease and being rejected by helm-controller's CRD validation later. +func TestParsePositiveDuration(t *testing.T) { + tests := []struct { + name string + raw string + want time.Duration + wantErr bool + }{ + {name: "valid seconds", raw: "30s", want: 30 * time.Second}, + {name: "valid minutes", raw: "5m", want: 5 * time.Minute}, + {name: "valid compound", raw: "1h30m", want: 90 * time.Minute}, + {name: "zero rejected", raw: "0s", wantErr: true}, + {name: "negative rejected", raw: "-5m", wantErr: true}, + {name: "malformed rejected", raw: "5x", wantErr: true}, + {name: "empty rejected", raw: "", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParsePositiveDuration("--test-flag", tt.raw) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error for %q, got nil", tt.raw) + } + return + } + if err != nil { + t.Fatalf("unexpected error for %q: %v", tt.raw, err) + } + if got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} From 91423d684f5b3dd37bfc220cebcb8d0e2bf3f8b5 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Wed, 6 May 2026 08:57:48 +0500 Subject: [PATCH 02/10] feat(api): expose HelmRelease generation knobs as cozystack-api flags Wire five flags into cozystack-api that mirror the cozystack-operator flags introduced in PR #2509: --helmrelease-interval (default 5m) --helmrelease-retry-interval (default 30s) --helmrelease-install-timeout (default 10m) --helmrelease-upgrade-timeout (default 10m) --helmrelease-max-history (default 5) CozyServerOptions.Complete() validates each duration via config.ParsePositiveDuration so a misconfigured operator restarts loudly instead of waiting until the first Application is created, and rejects MaxHistory < 0 (0 is valid: unlimited per Helm semantics, matching the operator). Validated values are written into every ReleaseConfig built from the ApplicationDefinitionList. The per-Application HelmInstallTimeout (annotation-driven) still wins over the new global Install/Upgrade timeout defaults; this commit only plumbs the values - the consumer in convertApplicationToHelmRelease lands in the next commit. Signed-off-by: Myasnikov Daniil --- pkg/cmd/server/start.go | 76 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/pkg/cmd/server/start.go b/pkg/cmd/server/start.go index 7ebbbc899..8206c4da2 100644 --- a/pkg/cmd/server/start.go +++ b/pkg/cmd/server/start.go @@ -57,6 +57,17 @@ type CozyServerOptions struct { // Add a field to store the configuration ResourceConfig *config.ResourceConfig + + // Raw HelmRelease generation flag values; parsed and validated in + // Complete() with config.ParsePositiveDuration so a misconfigured flag + // fails fast at startup. Kept in sync with the cozystack-operator + // flags of the same name (PR #2509) so both HelmRelease-generating + // paths share the same retry strategy and cadence. + HelmReleaseInterval string + HelmReleaseRetryInterval string + HelmReleaseInstallTimeout string + HelmReleaseUpgradeTimeout string + HelmReleaseMaxHistory int } // NewCozyServerOptions returns a new instance of CozyServerOptions @@ -71,6 +82,13 @@ func NewCozyServerOptions(out, errOut io.Writer) *CozyServerOptions { ), StdOut: out, StdErr: errOut, + // Defaults match cozystack-operator: production semantics are + // identical between the two HelmRelease-generating paths. + HelmReleaseInterval: "5m", + HelmReleaseRetryInterval: "30s", + HelmReleaseInstallTimeout: "10m", + HelmReleaseUpgradeTimeout: "10m", + HelmReleaseMaxHistory: 5, } o.RecommendedOptions.Etcd = nil return o @@ -100,6 +118,31 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions) flags := cmd.Flags() o.RecommendedOptions.AddFlags(flags) + // HelmRelease generation knobs. Names, defaults, and validation match + // the cozystack-operator flags from PR #2509 so both HelmRelease- + // generating paths can be tuned together. + flags.StringVar(&o.HelmReleaseInterval, "helmrelease-interval", o.HelmReleaseInterval, + "Reconcile interval applied to HelmReleases generated from Application resources "+ + "(Spec.Interval). Mirrors the cozystack-operator flag of the same name; same default 5m "+ + "matches existing behaviour.") + flags.StringVar(&o.HelmReleaseRetryInterval, "helmrelease-retry-interval", o.HelmReleaseRetryInterval, + "Retry interval applied to Install.Strategy and Upgrade.Strategy of HelmReleases generated "+ + "from Application resources. With Strategy.Name=RetryOnFailure, this controls how long "+ + "the controller waits between failed install/upgrade attempts. Decoupled from "+ + "--helmrelease-interval so failures recover fast without polling healthy releases at the "+ + "same cadence.") + flags.StringVar(&o.HelmReleaseInstallTimeout, "helmrelease-install-timeout", o.HelmReleaseInstallTimeout, + "Default timeout for the Helm install action of HelmReleases generated from Application "+ + "resources (Spec.Install.Timeout). Overridden per-Application by the "+ + "release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition.") + flags.StringVar(&o.HelmReleaseUpgradeTimeout, "helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout, + "Default timeout for the Helm upgrade action of HelmReleases generated from Application "+ + "resources (Spec.Upgrade.Timeout). Overridden per-Application by the "+ + "release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition.") + flags.IntVar(&o.HelmReleaseMaxHistory, "helmrelease-max-history", o.HelmReleaseMaxHistory, + "Number of release revisions Helm keeps for HelmReleases generated from Application "+ + "resources (Spec.MaxHistory). 0 means unlimited; 5 matches Helm's default.") + // Note: KEP-4330 component versioning functionality (k8s.io/apiserver/pkg/util/version) // is not available in Kubernetes v0.34.1. The component versioning code has been removed. @@ -108,6 +151,29 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions) // Complete fills in the fields that are not set func (o *CozyServerOptions) Complete() error { + // Validate the HelmRelease generation flags before any kubernetes I/O + // so a misconfigured operator restarts loudly instead of waiting until + // the first Application is created. Same shape as cozystack-operator. + hrInterval, err := config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval) + if err != nil { + return err + } + hrRetryInterval, err := config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval) + if err != nil { + return err + } + hrInstallTimeout, err := config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout) + if err != nil { + return err + } + hrUpgradeTimeout, err := config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout) + if err != nil { + return err + } + if o.HelmReleaseMaxHistory < 0 { + return fmt.Errorf("--helmrelease-max-history must be >= 0 (got %d)", o.HelmReleaseMaxHistory) + } + scheme := runtime.NewScheme() if err := v1alpha1.AddToScheme(scheme); err != nil { return fmt.Errorf("failed to register types: %w", err) @@ -168,6 +234,16 @@ func (o *CozyServerOptions) Complete() error { Name: crd.Spec.Release.ChartRef.Name, Namespace: crd.Spec.Release.ChartRef.Namespace, }, + // Per-Application HelmRelease generation defaults from server + // flags. The same five values are applied to every Resource, + // matching cozystack-operator's PackageReconciler. The + // per-Application HelmInstallTimeout annotation populated below + // still wins over HelmReleaseInstallTimeout/UpgradeTimeout. + HelmReleaseInterval: hrInterval, + HelmReleaseRetryInterval: hrRetryInterval, + HelmReleaseInstallTimeout: hrInstallTimeout, + HelmReleaseUpgradeTimeout: hrUpgradeTimeout, + HelmReleaseMaxHistory: o.HelmReleaseMaxHistory, } // Per-Application HelmRelease Install/Upgrade timeout. Applications // whose parent chart contains asynchronously-provisioned resources From 5e148f7356ece486201d0d04d18025f54867be83 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Wed, 6 May 2026 08:58:05 +0500 Subject: [PATCH 03/10] feat(api): apply HelmRelease knobs to Application-generated HelmReleases 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 --- pkg/registry/apps/application/rest.go | 59 ++++-- .../application/rest_helmrelease_spec_test.go | 192 ++++++++++++++++++ .../apps/application/rest_timeout_test.go | 27 ++- 3 files changed, 257 insertions(+), 21 deletions(-) create mode 100644 pkg/registry/apps/application/rest_helmrelease_spec_test.go diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index 080e57ada..b652dbc61 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -1509,8 +1509,19 @@ func latestMonitorTime(m *cozyv1alpha1.WorkloadMonitor) metav1.Time { return m.CreationTimestamp } -// convertApplicationToHelmRelease implements the actual conversion logic +// convertApplicationToHelmRelease implements the actual conversion logic. +// +// Spec.Interval, Install/Upgrade.Strategy{Name=RetryOnFailure,RetryInterval}, +// and Spec.MaxHistory are populated from ReleaseConfig fields fed by the +// cozystack-api server flags (--helmrelease-interval, +// --helmrelease-retry-interval, --helmrelease-max-history). This mirrors the +// operator's PackageReconciler.buildHelmReleaseSpec so both HelmRelease- +// generating paths share the same retry strategy, history retention, and +// reconcile cadence — see cozystack-operator's PR #2509 for the rationale +// (recurring 2-min E2E install timeouts caused by Remediation{Retries:-1} +// being coupled to a 5m Interval). func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (*helmv2.HelmRelease, error) { + maxHistory := r.releaseConfig.HelmReleaseMaxHistory helmRelease := &helmv2.HelmRelease{ TypeMeta: metav1.TypeMeta{ APIVersion: "helm.toolkit.fluxcd.io/v2", @@ -1530,15 +1541,18 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* Name: r.releaseConfig.ChartRef.Name, Namespace: r.releaseConfig.ChartRef.Namespace, }, - Interval: metav1.Duration{Duration: 5 * time.Minute}, + Interval: metav1.Duration{Duration: r.releaseConfig.HelmReleaseInterval}, + MaxHistory: &maxHistory, Install: &helmv2.Install{ - Remediation: &helmv2.InstallRemediation{ - Retries: -1, + Strategy: &helmv2.InstallStrategy{ + Name: string(helmv2.ActionStrategyRetryOnFailure), + RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval}, }, }, Upgrade: &helmv2.Upgrade{ - Remediation: &helmv2.UpgradeRemediation{ - Retries: -1, + Strategy: &helmv2.UpgradeStrategy{ + Name: string(helmv2.ActionStrategyRetryOnFailure), + RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval}, }, }, ValuesFrom: []helmv2.ValuesReference{ @@ -1551,24 +1565,35 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* }, } - // Per-Application HelmRelease wait budget. The mechanism is generic: - // an ApplicationDefinition that sets - // release.cozystack.io/helm-install-timeout gets Install.Timeout and - // Upgrade.Timeout populated from ReleaseConfig.HelmInstallTimeout - // (parsed at startup). Applications that leave it unset keep flux - // defaults so their failed installs remediate on the normal cadence. - // kubernetes-rd and tenant-rd carry the annotation today: the - // Kubernetes Application's parent chart contains CAPI/Kamaji + // Install/Upgrade.Timeout: per-Application annotation override + // (HelmInstallTimeout) wins over the global default + // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). When both + // are zero the field stays nil and flux defaults apply — preserving + // the pre-#2509 behaviour for tests that hand-construct ReleaseConfig + // without populating the new global fields. + // + // The annotation mechanism is generic: an ApplicationDefinition that + // sets release.cozystack.io/helm-install-timeout gets Install.Timeout + // and Upgrade.Timeout populated from ReleaseConfig.HelmInstallTimeout + // (parsed at startup). kubernetes-rd and tenant-rd carry the annotation + // today: the Kubernetes Application's parent chart contains CAPI/Kamaji // resources whose admin-kubeconfig Secret is provisioned // asynchronously and Kamaji cold-start routinely exceeds flux's // default wait budget, and the Tenant parent chart bootstraps the // seaweedfs-db CNPG cluster whose first reconcile exceeds it too. // Any future kind with the same shape can opt in by setting the // same annotation. + installTimeout := r.releaseConfig.HelmReleaseInstallTimeout + upgradeTimeout := r.releaseConfig.HelmReleaseUpgradeTimeout if r.releaseConfig.HelmInstallTimeout > 0 { - timeout := metav1.Duration{Duration: r.releaseConfig.HelmInstallTimeout} - helmRelease.Spec.Install.Timeout = &timeout - helmRelease.Spec.Upgrade.Timeout = &timeout + installTimeout = r.releaseConfig.HelmInstallTimeout + upgradeTimeout = r.releaseConfig.HelmInstallTimeout + } + if installTimeout > 0 { + helmRelease.Spec.Install.Timeout = &metav1.Duration{Duration: installTimeout} + } + if upgradeTimeout > 0 { + helmRelease.Spec.Upgrade.Timeout = &metav1.Duration{Duration: upgradeTimeout} } return helmRelease, nil diff --git a/pkg/registry/apps/application/rest_helmrelease_spec_test.go b/pkg/registry/apps/application/rest_helmrelease_spec_test.go new file mode 100644 index 000000000..78d0c8224 --- /dev/null +++ b/pkg/registry/apps/application/rest_helmrelease_spec_test.go @@ -0,0 +1,192 @@ +/* +Copyright 2026 The Cozystack Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package application + +import ( + "testing" + "time" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1" + "github.com/cozystack/cozystack/pkg/config" +) + +// Mirrors the operator-side TestBuildHelmReleaseSpec +// (internal/operator/package_reconciler_test.go) so the api-side +// HelmRelease shape stays in lockstep with cozystack-operator. The two +// HelmRelease-generating paths must populate Interval, MaxHistory, +// Strategy, RetryInterval, and Install/Upgrade.Timeout identically — drift +// between them was the rationale for the api-side parity work that +// followed PR #2509. +func TestConvertApplicationToHelmRelease_BuildsSpecFromConfig(t *testing.T) { + r := &REST{ + kindName: "Postgres", + releaseConfig: config.ReleaseConfig{ + Prefix: "postgres-", + ChartRef: config.ChartRefConfig{ + Kind: "HelmChart", + Name: "postgres", + Namespace: "cozy-system", + }, + HelmReleaseInterval: 42 * time.Second, + HelmReleaseRetryInterval: 17 * time.Second, + HelmReleaseInstallTimeout: 11 * time.Minute, + HelmReleaseUpgradeTimeout: 13 * time.Minute, + HelmReleaseMaxHistory: 7, + }, + } + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "example", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + + if hr.Spec.Interval.Duration != 42*time.Second { + t.Errorf("Interval = %v, want 42s", hr.Spec.Interval.Duration) + } + if hr.Spec.MaxHistory == nil { + t.Fatal("MaxHistory is nil, want pointer to 7") + } + if *hr.Spec.MaxHistory != 7 { + t.Errorf("MaxHistory = %d, want 7", *hr.Spec.MaxHistory) + } + + if hr.Spec.ChartRef == nil { + t.Fatal("ChartRef is nil") + } + if hr.Spec.ChartRef.Kind != "HelmChart" { + t.Errorf("ChartRef.Kind = %q, want HelmChart", hr.Spec.ChartRef.Kind) + } + if hr.Spec.ChartRef.Name != "postgres" { + t.Errorf("ChartRef.Name = %q, want postgres", hr.Spec.ChartRef.Name) + } + if hr.Spec.ChartRef.Namespace != "cozy-system" { + t.Errorf("ChartRef.Namespace = %q, want cozy-system", hr.Spec.ChartRef.Namespace) + } + + if hr.Spec.Install == nil { + t.Fatal("Install is nil") + } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != 11*time.Minute { + t.Errorf("Install.Timeout = %v, want 11m", hr.Spec.Install.Timeout) + } + if hr.Spec.Install.Strategy == nil { + t.Fatal("Install.Strategy is nil") + } + if hr.Spec.Install.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Install.Strategy.Name = %q, want %q", hr.Spec.Install.Strategy.Name, helmv2.ActionStrategyRetryOnFailure) + } + if hr.Spec.Install.Strategy.RetryInterval == nil || hr.Spec.Install.Strategy.RetryInterval.Duration != 17*time.Second { + t.Errorf("Install.Strategy.RetryInterval = %v, want 17s", hr.Spec.Install.Strategy.RetryInterval) + } + // Remediation must remain nil: helm-controller's XValidation rule + // rejects Strategy.Name=RetryOnFailure with RetryInterval set + // alongside a Remediation entry, so a future "for safety" + // re-introduction of Remediation{Retries: -1} would silently break + // every HR. + if hr.Spec.Install.Remediation != nil { + t.Errorf("Install.Remediation = %+v, want nil", hr.Spec.Install.Remediation) + } + + if hr.Spec.Upgrade == nil { + t.Fatal("Upgrade is nil") + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != 13*time.Minute { + t.Errorf("Upgrade.Timeout = %v, want 13m", hr.Spec.Upgrade.Timeout) + } + if hr.Spec.Upgrade.Strategy == nil { + t.Fatal("Upgrade.Strategy is nil") + } + if hr.Spec.Upgrade.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Upgrade.Strategy.Name = %q, want %q", hr.Spec.Upgrade.Strategy.Name, helmv2.ActionStrategyRetryOnFailure) + } + if hr.Spec.Upgrade.Strategy.RetryInterval == nil || hr.Spec.Upgrade.Strategy.RetryInterval.Duration != 17*time.Second { + t.Errorf("Upgrade.Strategy.RetryInterval = %v, want 17s", hr.Spec.Upgrade.Strategy.RetryInterval) + } + if hr.Spec.Upgrade.Remediation != nil { + t.Errorf("Upgrade.Remediation = %+v, want nil", hr.Spec.Upgrade.Remediation) + } +} + +// Pins that MaxHistory=0 (unlimited history per Helm semantics) survives the +// conversion — i.e. is set as a non-nil pointer to 0 rather than dropped or +// replaced with a default. Mirrors the operator-side +// TestBuildHelmReleaseSpecZeroMaxHistory. +func TestConvertApplicationToHelmRelease_ZeroMaxHistory(t *testing.T) { + r := &REST{ + kindName: "Postgres", + releaseConfig: config.ReleaseConfig{ + Prefix: "postgres-", + HelmReleaseInterval: 30 * time.Second, + HelmReleaseRetryInterval: 30 * time.Second, + HelmReleaseMaxHistory: 0, + }, + } + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + if hr.Spec.MaxHistory == nil { + t.Fatal("MaxHistory is nil for HelmReleaseMaxHistory=0; want pointer to 0") + } + if *hr.Spec.MaxHistory != 0 { + t.Errorf("MaxHistory = %d, want 0", *hr.Spec.MaxHistory) + } +} + +// HelmInstallTimeout (the per-Application annotation override) wins over the +// global HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout defaults. This +// is the contract that lets ApplicationDefinitions like Kubernetes (with its +// long Kamaji bootstrap) opt out of the global default without operators +// having to widen the global flag for every kind. +func TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal(t *testing.T) { + r := &REST{ + kindName: "Kubernetes", + releaseConfig: config.ReleaseConfig{ + Prefix: "kubernetes-", + HelmReleaseInterval: 5 * time.Minute, + HelmReleaseRetryInterval: 30 * time.Second, + HelmReleaseInstallTimeout: 10 * time.Minute, + HelmReleaseUpgradeTimeout: 10 * time.Minute, + HelmReleaseMaxHistory: 5, + HelmInstallTimeout: 30 * time.Minute, + }, + } + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != 30*time.Minute { + t.Errorf("Install.Timeout = %v, want 30m (per-app override)", hr.Spec.Install.Timeout) + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != 30*time.Minute { + t.Errorf("Upgrade.Timeout = %v, want 30m (per-app override)", hr.Spec.Upgrade.Timeout) + } +} diff --git a/pkg/registry/apps/application/rest_timeout_test.go b/pkg/registry/apps/application/rest_timeout_test.go index 85ac0b974..725260871 100644 --- a/pkg/registry/apps/application/rest_timeout_test.go +++ b/pkg/registry/apps/application/rest_timeout_test.go @@ -4,12 +4,18 @@ import ( "testing" "time" + helmv2 "github.com/fluxcd/helm-controller/api/v2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1" "github.com/cozystack/cozystack/pkg/config" ) +// newRESTForTimeout builds a REST struct focused on the per-Application +// HelmInstallTimeout annotation override path. Global HelmRelease* defaults +// stay zero-valued so the test exercises the "annotation unset and no global +// default" → flux-defaults path; the global-default path is covered by +// rest_helmrelease_spec_test.go. func newRESTForTimeout(kind, prefix string, helmInstallTimeout time.Duration) *REST { return &REST{ kindName: kind, @@ -105,11 +111,24 @@ func TestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout(t *testing. } } - if hr.Spec.Install.Remediation == nil || hr.Spec.Install.Remediation.Retries != -1 { - t.Errorf("Spec.Install.Remediation.Retries must remain -1, got %+v", hr.Spec.Install.Remediation) + // Strategy must be RetryOnFailure with Remediation kept nil: + // helm-controller's XValidation rule rejects + // Strategy.Name=RetryOnFailure with RetryInterval set alongside + // a Remediation entry, so a future "for safety" re-introduction + // of Remediation{Retries: -1} would silently break every HR. + if hr.Spec.Install.Strategy == nil || + hr.Spec.Install.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Spec.Install.Strategy must be RetryOnFailure, got %+v", hr.Spec.Install.Strategy) } - if hr.Spec.Upgrade.Remediation == nil || hr.Spec.Upgrade.Remediation.Retries != -1 { - t.Errorf("Spec.Upgrade.Remediation.Retries must remain -1, got %+v", hr.Spec.Upgrade.Remediation) + if hr.Spec.Install.Remediation != nil { + t.Errorf("Spec.Install.Remediation must be nil, got %+v", hr.Spec.Install.Remediation) + } + if hr.Spec.Upgrade.Strategy == nil || + hr.Spec.Upgrade.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Spec.Upgrade.Strategy must be RetryOnFailure, got %+v", hr.Spec.Upgrade.Strategy) + } + if hr.Spec.Upgrade.Remediation != nil { + t.Errorf("Spec.Upgrade.Remediation must be nil, got %+v", hr.Spec.Upgrade.Remediation) } }) } From 30d84e99cb3750816ab79b69b7ed094f0de8373b Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Fri, 8 May 2026 17:32:00 +0500 Subject: [PATCH 04/10] refactor(api): align HelmRelease conversion with operator, add validator tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Myasnikov Daniil --- pkg/cmd/server/start.go | 84 +++++---- pkg/cmd/server/start_test.go | 169 ++++++++++++++++++ pkg/registry/apps/application/rest.go | 68 +++---- .../application/rest_helmrelease_spec_test.go | 92 +++++++--- .../apps/application/rest_timeout_test.go | 94 +++++----- 5 files changed, 365 insertions(+), 142 deletions(-) diff --git a/pkg/cmd/server/start.go b/pkg/cmd/server/start.go index 8206c4da2..2a37bcc19 100644 --- a/pkg/cmd/server/start.go +++ b/pkg/cmd/server/start.go @@ -61,8 +61,8 @@ type CozyServerOptions struct { // Raw HelmRelease generation flag values; parsed and validated in // Complete() with config.ParsePositiveDuration so a misconfigured flag // fails fast at startup. Kept in sync with the cozystack-operator - // flags of the same name (PR #2509) so both HelmRelease-generating - // paths share the same retry strategy and cadence. + // flags of the same name so both HelmRelease-generating paths share + // the same retry strategy and cadence. HelmReleaseInterval string HelmReleaseRetryInterval string HelmReleaseInstallTimeout string @@ -119,12 +119,11 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions) o.RecommendedOptions.AddFlags(flags) // HelmRelease generation knobs. Names, defaults, and validation match - // the cozystack-operator flags from PR #2509 so both HelmRelease- - // generating paths can be tuned together. + // the cozystack-operator flags of the same name so both + // HelmRelease-generating paths can be tuned together. flags.StringVar(&o.HelmReleaseInterval, "helmrelease-interval", o.HelmReleaseInterval, "Reconcile interval applied to HelmReleases generated from Application resources "+ - "(Spec.Interval). Mirrors the cozystack-operator flag of the same name; same default 5m "+ - "matches existing behaviour.") + "(Spec.Interval). Mirrors the cozystack-operator flag of the same name.") flags.StringVar(&o.HelmReleaseRetryInterval, "helmrelease-retry-interval", o.HelmReleaseRetryInterval, "Retry interval applied to Install.Strategy and Upgrade.Strategy of HelmReleases generated "+ "from Application resources. With Strategy.Name=RetryOnFailure, this controls how long "+ @@ -134,11 +133,13 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions) flags.StringVar(&o.HelmReleaseInstallTimeout, "helmrelease-install-timeout", o.HelmReleaseInstallTimeout, "Default timeout for the Helm install action of HelmReleases generated from Application "+ "resources (Spec.Install.Timeout). Overridden per-Application by the "+ - "release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition.") + "release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition; the "+ + "same annotation also overrides --helmrelease-upgrade-timeout.") flags.StringVar(&o.HelmReleaseUpgradeTimeout, "helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout, "Default timeout for the Helm upgrade action of HelmReleases generated from Application "+ - "resources (Spec.Upgrade.Timeout). Overridden per-Application by the "+ - "release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition.") + "resources (Spec.Upgrade.Timeout). Overridden per-Application by the same "+ + "release.cozystack.io/helm-install-timeout annotation as --helmrelease-install-timeout: "+ + "one annotation overrides both install and upgrade timeouts together.") flags.IntVar(&o.HelmReleaseMaxHistory, "helmrelease-max-history", o.HelmReleaseMaxHistory, "Number of release revisions Helm keeps for HelmReleases generated from Application "+ "resources (Spec.MaxHistory). 0 means unlimited; 5 matches Helm's default.") @@ -149,29 +150,48 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions) return cmd } -// Complete fills in the fields that are not set -func (o *CozyServerOptions) Complete() error { - // Validate the HelmRelease generation flags before any kubernetes I/O - // so a misconfigured operator restarts loudly instead of waiting until - // the first Application is created. Same shape as cozystack-operator. - hrInterval, err := config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval) - if err != nil { - return err +// helmReleaseFlagValues holds the parsed, validated HelmRelease generation +// flags. Populated by parseAndValidateHelmReleaseFlags before any kubernetes +// I/O so a misconfigured server restarts loudly instead of waiting until the +// first Application is created. +type helmReleaseFlagValues struct { + interval time.Duration + retryInterval time.Duration + installTimeout time.Duration + upgradeTimeout time.Duration + maxHistory int +} + +// parseAndValidateHelmReleaseFlags parses the five HelmRelease generation +// flags and rejects malformed or non-positive durations and negative +// MaxHistory. Same shape as cozystack-operator's main. +func (o *CozyServerOptions) parseAndValidateHelmReleaseFlags() (helmReleaseFlagValues, error) { + var v helmReleaseFlagValues + var err error + if v.interval, err = config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval); err != nil { + return v, err } - hrRetryInterval, err := config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval) - if err != nil { - return err + if v.retryInterval, err = config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval); err != nil { + return v, err } - hrInstallTimeout, err := config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout) - if err != nil { - return err + if v.installTimeout, err = config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout); err != nil { + return v, err } - hrUpgradeTimeout, err := config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout) - if err != nil { - return err + if v.upgradeTimeout, err = config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout); err != nil { + return v, err } if o.HelmReleaseMaxHistory < 0 { - return fmt.Errorf("--helmrelease-max-history must be >= 0 (got %d)", o.HelmReleaseMaxHistory) + return v, fmt.Errorf("--helmrelease-max-history must be >= 0 (got %d)", o.HelmReleaseMaxHistory) + } + v.maxHistory = o.HelmReleaseMaxHistory + return v, nil +} + +// Complete fills in the fields that are not set +func (o *CozyServerOptions) Complete() error { + hrFlags, err := o.parseAndValidateHelmReleaseFlags() + if err != nil { + return err } scheme := runtime.NewScheme() @@ -239,11 +259,11 @@ func (o *CozyServerOptions) Complete() error { // matching cozystack-operator's PackageReconciler. The // per-Application HelmInstallTimeout annotation populated below // still wins over HelmReleaseInstallTimeout/UpgradeTimeout. - HelmReleaseInterval: hrInterval, - HelmReleaseRetryInterval: hrRetryInterval, - HelmReleaseInstallTimeout: hrInstallTimeout, - HelmReleaseUpgradeTimeout: hrUpgradeTimeout, - HelmReleaseMaxHistory: o.HelmReleaseMaxHistory, + HelmReleaseInterval: hrFlags.interval, + HelmReleaseRetryInterval: hrFlags.retryInterval, + HelmReleaseInstallTimeout: hrFlags.installTimeout, + HelmReleaseUpgradeTimeout: hrFlags.upgradeTimeout, + HelmReleaseMaxHistory: hrFlags.maxHistory, } // Per-Application HelmRelease Install/Upgrade timeout. Applications // whose parent chart contains asynchronously-provisioned resources diff --git a/pkg/cmd/server/start_test.go b/pkg/cmd/server/start_test.go index 9dc2000c6..599b085fa 100644 --- a/pkg/cmd/server/start_test.go +++ b/pkg/cmd/server/start_test.go @@ -15,3 +15,172 @@ limitations under the License. */ package server + +import ( + "strings" + "testing" + "time" +) + +// validHelmReleaseFlags returns a CozyServerOptions populated with the same +// defaults NewCozyServerOptions sets, so each test case mutates a single +// flag and asserts the resulting validation behaviour in isolation. +func validHelmReleaseFlags() *CozyServerOptions { + return &CozyServerOptions{ + HelmReleaseInterval: "5m", + HelmReleaseRetryInterval: "30s", + HelmReleaseInstallTimeout: "10m", + HelmReleaseUpgradeTimeout: "10m", + HelmReleaseMaxHistory: 5, + } +} + +// Pins that each malformed/non-positive duration and a negative MaxHistory +// is rejected at startup. Without this, a regression that swaps `<` for +// `<=` on the MaxHistory branch — breaking the documented "0 means +// unlimited" contract — or that drops a `return err` line on any of the +// four duration branches would ship green. +func TestCozyServerOptions_ParseAndValidateHelmReleaseFlags_Rejects(t *testing.T) { + cases := []struct { + name string + mutate func(o *CozyServerOptions) + wantSubs string + }{ + { + name: "interval zero", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "0s" }, + wantSubs: "--helmrelease-interval", + }, + { + name: "interval negative", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "-5m" }, + wantSubs: "--helmrelease-interval", + }, + { + name: "interval malformed", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "5x" }, + wantSubs: "--helmrelease-interval", + }, + { + name: "retry-interval empty", + mutate: func(o *CozyServerOptions) { o.HelmReleaseRetryInterval = "" }, + wantSubs: "--helmrelease-retry-interval", + }, + { + name: "install-timeout zero", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInstallTimeout = "0s" }, + wantSubs: "--helmrelease-install-timeout", + }, + { + name: "upgrade-timeout malformed", + mutate: func(o *CozyServerOptions) { o.HelmReleaseUpgradeTimeout = "tenminutes" }, + wantSubs: "--helmrelease-upgrade-timeout", + }, + { + name: "max-history negative", + mutate: func(o *CozyServerOptions) { o.HelmReleaseMaxHistory = -1 }, + wantSubs: "--helmrelease-max-history must be >= 0", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + o := validHelmReleaseFlags() + tc.mutate(o) + _, err := o.parseAndValidateHelmReleaseFlags() + if err == nil { + t.Fatalf("expected error, got nil") + } + if !strings.Contains(err.Error(), tc.wantSubs) { + t.Errorf("error %q does not contain %q", err.Error(), tc.wantSubs) + } + }) + } +} + +// Pins the happy path: valid flag strings parse to the expected +// time.Duration values and MaxHistory=0 is accepted (the documented +// "unlimited per Helm semantics" branch). +func TestCozyServerOptions_ParseAndValidateHelmReleaseFlags_Accepts(t *testing.T) { + cases := []struct { + name string + o *CozyServerOptions + wantInterval time.Duration + wantRetry time.Duration + wantInstall time.Duration + wantUpgrade time.Duration + wantMaxHistory int + }{ + { + name: "production defaults", + o: validHelmReleaseFlags(), + wantInterval: 5 * time.Minute, + wantRetry: 30 * time.Second, + wantInstall: 10 * time.Minute, + wantUpgrade: 10 * time.Minute, + wantMaxHistory: 5, + }, + { + name: "max-history zero accepted as unlimited", + o: func() *CozyServerOptions { + o := validHelmReleaseFlags() + o.HelmReleaseMaxHistory = 0 + return o + }(), + wantInterval: 5 * time.Minute, + wantRetry: 30 * time.Second, + wantInstall: 10 * time.Minute, + wantUpgrade: 10 * time.Minute, + wantMaxHistory: 0, + }, + { + name: "asymmetric install/upgrade timeouts both honored", + o: func() *CozyServerOptions { + o := validHelmReleaseFlags() + o.HelmReleaseInstallTimeout = "8m" + o.HelmReleaseUpgradeTimeout = "12m" + return o + }(), + wantInterval: 5 * time.Minute, + wantRetry: 30 * time.Second, + wantInstall: 8 * time.Minute, + wantUpgrade: 12 * time.Minute, + wantMaxHistory: 5, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + v, err := tc.o.parseAndValidateHelmReleaseFlags() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if v.interval != tc.wantInterval { + t.Errorf("interval = %v, want %v", v.interval, tc.wantInterval) + } + if v.retryInterval != tc.wantRetry { + t.Errorf("retryInterval = %v, want %v", v.retryInterval, tc.wantRetry) + } + if v.installTimeout != tc.wantInstall { + t.Errorf("installTimeout = %v, want %v", v.installTimeout, tc.wantInstall) + } + if v.upgradeTimeout != tc.wantUpgrade { + t.Errorf("upgradeTimeout = %v, want %v", v.upgradeTimeout, tc.wantUpgrade) + } + if v.maxHistory != tc.wantMaxHistory { + t.Errorf("maxHistory = %d, want %d", v.maxHistory, tc.wantMaxHistory) + } + }) + } +} + +// Pins that NewCozyServerOptions seeds defaults that pass validation, so +// `cozystack-api ...` with no HelmRelease flags overridden does not fail +// at startup. Guards against a future change to defaults that accidentally +// violates the validator. +func TestNewCozyServerOptions_DefaultsValidate(t *testing.T) { + o := NewCozyServerOptions(nil, nil) + if _, err := o.parseAndValidateHelmReleaseFlags(); err != nil { + t.Fatalf("default options must validate, got: %v", err) + } +} diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index b652dbc61..6b5c6c834 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -1512,15 +1512,34 @@ func latestMonitorTime(m *cozyv1alpha1.WorkloadMonitor) metav1.Time { // convertApplicationToHelmRelease implements the actual conversion logic. // // Spec.Interval, Install/Upgrade.Strategy{Name=RetryOnFailure,RetryInterval}, -// and Spec.MaxHistory are populated from ReleaseConfig fields fed by the -// cozystack-api server flags (--helmrelease-interval, -// --helmrelease-retry-interval, --helmrelease-max-history). This mirrors the -// operator's PackageReconciler.buildHelmReleaseSpec so both HelmRelease- -// generating paths share the same retry strategy, history retention, and -// reconcile cadence — see cozystack-operator's PR #2509 for the rationale -// (recurring 2-min E2E install timeouts caused by Remediation{Retries:-1} -// being coupled to a 5m Interval). +// Install/Upgrade.Timeout, and Spec.MaxHistory are populated from +// ReleaseConfig fields fed by the cozystack-api server flags +// (--helmrelease-{interval,retry-interval,install-timeout,upgrade-timeout, +// max-history}). This mirrors cozystack-operator's +// PackageReconciler.buildHelmReleaseSpec so both HelmRelease-generating paths +// share the same retry strategy, history retention, and reconcile cadence. +// +// Strategy.Name=RetryOnFailure with an explicit RetryInterval (rather than +// Remediation{Retries:-1}) decouples failed install/upgrade retry timing +// from Spec.Interval — the previous coupling caused failed installs to +// retry only every 5m, exceeding E2E budgets. func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (*helmv2.HelmRelease, error) { + // Per-Application annotation override (HelmInstallTimeout, populated + // from release.cozystack.io/helm-install-timeout on the + // ApplicationDefinition at startup) wins over the global defaults + // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). Today only + // kubernetes-rd carries the annotation because the Kubernetes + // Application's parent chart contains CAPI/Kamaji resources whose + // admin-kubeconfig Secret is provisioned asynchronously and Kamaji + // cold-start routinely exceeds flux's default wait budget; any future + // kind with the same shape can opt in by setting the same annotation. + installTimeout := r.releaseConfig.HelmReleaseInstallTimeout + upgradeTimeout := r.releaseConfig.HelmReleaseUpgradeTimeout + if r.releaseConfig.HelmInstallTimeout > 0 { + installTimeout = r.releaseConfig.HelmInstallTimeout + upgradeTimeout = r.releaseConfig.HelmInstallTimeout + } + maxHistory := r.releaseConfig.HelmReleaseMaxHistory helmRelease := &helmv2.HelmRelease{ TypeMeta: metav1.TypeMeta{ @@ -1544,12 +1563,14 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* Interval: metav1.Duration{Duration: r.releaseConfig.HelmReleaseInterval}, MaxHistory: &maxHistory, Install: &helmv2.Install{ + Timeout: &metav1.Duration{Duration: installTimeout}, Strategy: &helmv2.InstallStrategy{ Name: string(helmv2.ActionStrategyRetryOnFailure), RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval}, }, }, Upgrade: &helmv2.Upgrade{ + Timeout: &metav1.Duration{Duration: upgradeTimeout}, Strategy: &helmv2.UpgradeStrategy{ Name: string(helmv2.ActionStrategyRetryOnFailure), RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval}, @@ -1565,37 +1586,6 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* }, } - // Install/Upgrade.Timeout: per-Application annotation override - // (HelmInstallTimeout) wins over the global default - // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). When both - // are zero the field stays nil and flux defaults apply — preserving - // the pre-#2509 behaviour for tests that hand-construct ReleaseConfig - // without populating the new global fields. - // - // The annotation mechanism is generic: an ApplicationDefinition that - // sets release.cozystack.io/helm-install-timeout gets Install.Timeout - // and Upgrade.Timeout populated from ReleaseConfig.HelmInstallTimeout - // (parsed at startup). kubernetes-rd and tenant-rd carry the annotation - // today: the Kubernetes Application's parent chart contains CAPI/Kamaji - // resources whose admin-kubeconfig Secret is provisioned - // asynchronously and Kamaji cold-start routinely exceeds flux's - // default wait budget, and the Tenant parent chart bootstraps the - // seaweedfs-db CNPG cluster whose first reconcile exceeds it too. - // Any future kind with the same shape can opt in by setting the - // same annotation. - installTimeout := r.releaseConfig.HelmReleaseInstallTimeout - upgradeTimeout := r.releaseConfig.HelmReleaseUpgradeTimeout - if r.releaseConfig.HelmInstallTimeout > 0 { - installTimeout = r.releaseConfig.HelmInstallTimeout - upgradeTimeout = r.releaseConfig.HelmInstallTimeout - } - if installTimeout > 0 { - helmRelease.Spec.Install.Timeout = &metav1.Duration{Duration: installTimeout} - } - if upgradeTimeout > 0 { - helmRelease.Spec.Upgrade.Timeout = &metav1.Duration{Duration: upgradeTimeout} - } - return helmRelease, nil } diff --git a/pkg/registry/apps/application/rest_helmrelease_spec_test.go b/pkg/registry/apps/application/rest_helmrelease_spec_test.go index 78d0c8224..4b8e01852 100644 --- a/pkg/registry/apps/application/rest_helmrelease_spec_test.go +++ b/pkg/registry/apps/application/rest_helmrelease_spec_test.go @@ -31,9 +31,8 @@ import ( // (internal/operator/package_reconciler_test.go) so the api-side // HelmRelease shape stays in lockstep with cozystack-operator. The two // HelmRelease-generating paths must populate Interval, MaxHistory, -// Strategy, RetryInterval, and Install/Upgrade.Timeout identically — drift -// between them was the rationale for the api-side parity work that -// followed PR #2509. +// Strategy, RetryInterval, and Install/Upgrade.Timeout identically; drift +// between them is the failure mode this test guards against. func TestConvertApplicationToHelmRelease_BuildsSpecFromConfig(t *testing.T) { r := &REST{ kindName: "Postgres", @@ -162,31 +161,74 @@ func TestConvertApplicationToHelmRelease_ZeroMaxHistory(t *testing.T) { // is the contract that lets ApplicationDefinitions like Kubernetes (with its // long Kamaji bootstrap) opt out of the global default without operators // having to widen the global flag for every kind. +// +// The asymmetric-globals case pins that the override wipes both install and +// upgrade timeouts to the same value, even when the globals differ — a +// regression that only assigned one would slip through if the test only +// checked symmetric globals. func TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal(t *testing.T) { - r := &REST{ - kindName: "Kubernetes", - releaseConfig: config.ReleaseConfig{ - Prefix: "kubernetes-", - HelmReleaseInterval: 5 * time.Minute, - HelmReleaseRetryInterval: 30 * time.Second, - HelmReleaseInstallTimeout: 10 * time.Minute, - HelmReleaseUpgradeTimeout: 10 * time.Minute, - HelmReleaseMaxHistory: 5, - HelmInstallTimeout: 30 * time.Minute, + cases := []struct { + name string + globalInstall time.Duration + globalUpgrade time.Duration + override time.Duration + wantInstall time.Duration + wantUpgrade time.Duration + }{ + { + name: "symmetric globals, override wins", + globalInstall: 10 * time.Minute, + globalUpgrade: 10 * time.Minute, + override: 30 * time.Minute, + wantInstall: 30 * time.Minute, + wantUpgrade: 30 * time.Minute, + }, + { + name: "asymmetric globals, override wipes both to same value", + globalInstall: 8 * time.Minute, + globalUpgrade: 12 * time.Minute, + override: 30 * time.Minute, + wantInstall: 30 * time.Minute, + wantUpgrade: 30 * time.Minute, + }, + { + name: "asymmetric globals, no override keeps each side independent", + globalInstall: 8 * time.Minute, + globalUpgrade: 12 * time.Minute, + override: 0, + wantInstall: 8 * time.Minute, + wantUpgrade: 12 * time.Minute, }, } - app := &appsv1alpha1.Application{ - ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "tenant-root"}, - } - hr, err := r.convertApplicationToHelmRelease(app) - if err != nil { - t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) - } - if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != 30*time.Minute { - t.Errorf("Install.Timeout = %v, want 30m (per-app override)", hr.Spec.Install.Timeout) - } - if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != 30*time.Minute { - t.Errorf("Upgrade.Timeout = %v, want 30m (per-app override)", hr.Spec.Upgrade.Timeout) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := &REST{ + kindName: "Kubernetes", + releaseConfig: config.ReleaseConfig{ + Prefix: "kubernetes-", + HelmReleaseInterval: 5 * time.Minute, + HelmReleaseRetryInterval: 30 * time.Second, + HelmReleaseInstallTimeout: tc.globalInstall, + HelmReleaseUpgradeTimeout: tc.globalUpgrade, + HelmReleaseMaxHistory: 5, + HelmInstallTimeout: tc.override, + }, + } + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != tc.wantInstall { + t.Errorf("Install.Timeout = %v, want %v", hr.Spec.Install.Timeout, tc.wantInstall) + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != tc.wantUpgrade { + t.Errorf("Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout, tc.wantUpgrade) + } + }) } } diff --git a/pkg/registry/apps/application/rest_timeout_test.go b/pkg/registry/apps/application/rest_timeout_test.go index 725260871..94b618ee0 100644 --- a/pkg/registry/apps/application/rest_timeout_test.go +++ b/pkg/registry/apps/application/rest_timeout_test.go @@ -11,10 +11,18 @@ import ( "github.com/cozystack/cozystack/pkg/config" ) +// Default HelmRelease global durations the api server applies after flag +// parsing. Tests use these so the fixture exercises the same code path the +// production binary takes — i.e. global timeouts are always non-zero. +const ( + testGlobalInstallTimeout = 10 * time.Minute + testGlobalUpgradeTimeout = 10 * time.Minute +) + // newRESTForTimeout builds a REST struct focused on the per-Application // HelmInstallTimeout annotation override path. Global HelmRelease* defaults -// stay zero-valued so the test exercises the "annotation unset and no global -// default" → flux-defaults path; the global-default path is covered by +// are populated with production-shaped values so the unset-annotation case +// exercises "global default applies". The full spec contract is covered by // rest_helmrelease_spec_test.go. func newRESTForTimeout(kind, prefix string, helmInstallTimeout time.Duration) *REST { return &REST{ @@ -26,7 +34,12 @@ func newRESTForTimeout(kind, prefix string, helmInstallTimeout time.Duration) *R Name: "x", Namespace: "cozy-system", }, - HelmInstallTimeout: helmInstallTimeout, + HelmReleaseInterval: 5 * time.Minute, + HelmReleaseRetryInterval: 30 * time.Second, + HelmReleaseInstallTimeout: testGlobalInstallTimeout, + HelmReleaseUpgradeTimeout: testGlobalUpgradeTimeout, + HelmReleaseMaxHistory: 5, + HelmInstallTimeout: helmInstallTimeout, }, } } @@ -35,41 +48,45 @@ func newRESTForTimeout(kind, prefix string, helmInstallTimeout time.Duration) *R // budget. The Kubernetes kind's parent chart contains CAPI/Kamaji resources // whose admin-kubeconfig Secret is provisioned asynchronously, so its // ApplicationDefinition sets release.cozystack.io/helm-install-timeout=15m -// (or longer). Other kinds leave the annotation unset and keep flux defaults -// so their failed installs remediate on the normal cadence. The test must -// cover both paths: a kind with the timeout set and one without. +// (or longer). Other kinds leave the annotation unset and inherit the +// global default. The test must cover both paths: a kind with the override +// set and one without. func TestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout(t *testing.T) { cases := []struct { - name string - kind string - prefix string - configured time.Duration - wantSet bool + name string + kind string + prefix string + configured time.Duration + wantInstall time.Duration + wantUpgrade time.Duration }{ { - name: "Kubernetes kind with 15m configured gets Install and Upgrade Timeout", - kind: "Kubernetes", - prefix: "kubernetes-", - configured: 15 * time.Minute, - wantSet: true, + name: "Kubernetes kind with 15m override beats global defaults", + kind: "Kubernetes", + prefix: "kubernetes-", + configured: 15 * time.Minute, + wantInstall: 15 * time.Minute, + wantUpgrade: 15 * time.Minute, }, { // Fictional kind on purpose: the test is about the unset path // regardless of which real Application kind ends up needing a // timeout override. Using a real kind name would create false // coupling to that Application's ApplicationDefinition. - name: "unrelated kind without configured timeout keeps flux defaults", - kind: "PlaceholderKindForDefaults", - prefix: "placeholder-", - configured: 0, - wantSet: false, + name: "unrelated kind without override inherits global defaults", + kind: "PlaceholderKindForDefaults", + prefix: "placeholder-", + configured: 0, + wantInstall: testGlobalInstallTimeout, + wantUpgrade: testGlobalUpgradeTimeout, }, { - name: "arbitrary future kind with 20m configured gets 20m", - kind: "TalosCluster", - prefix: "talos-", - configured: 20 * time.Minute, - wantSet: true, + name: "arbitrary future kind with 20m override gets 20m", + kind: "TalosCluster", + prefix: "talos-", + configured: 20 * time.Minute, + wantInstall: 20 * time.Minute, + wantUpgrade: 20 * time.Minute, }, } @@ -89,26 +106,11 @@ func TestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout(t *testing. t.Fatalf("Spec.Install/Upgrade must be non-nil") } - if tc.wantSet { - if hr.Spec.Install.Timeout == nil { - t.Fatalf("Spec.Install.Timeout must be set when HelmInstallTimeout=%v", tc.configured) - } - if hr.Spec.Install.Timeout.Duration != tc.configured { - t.Errorf("Spec.Install.Timeout = %v, want %v", hr.Spec.Install.Timeout.Duration, tc.configured) - } - if hr.Spec.Upgrade.Timeout == nil { - t.Fatalf("Spec.Upgrade.Timeout must be set when HelmInstallTimeout=%v", tc.configured) - } - if hr.Spec.Upgrade.Timeout.Duration != tc.configured { - t.Errorf("Spec.Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout.Duration, tc.configured) - } - } else { - if hr.Spec.Install.Timeout != nil { - t.Errorf("Spec.Install.Timeout must be nil when HelmInstallTimeout=0, got %v", hr.Spec.Install.Timeout.Duration) - } - if hr.Spec.Upgrade.Timeout != nil { - t.Errorf("Spec.Upgrade.Timeout must be nil when HelmInstallTimeout=0, got %v", hr.Spec.Upgrade.Timeout.Duration) - } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != tc.wantInstall { + t.Errorf("Spec.Install.Timeout = %v, want %v", hr.Spec.Install.Timeout, tc.wantInstall) + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != tc.wantUpgrade { + t.Errorf("Spec.Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout, tc.wantUpgrade) } // Strategy must be RetryOnFailure with Remediation kept nil: From f1b7bb4a211a9dc82472ffca50f8375c02302c15 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Tue, 16 Jun 2026 14:23:13 +0500 Subject: [PATCH 05/10] test(api,operator): reword Remediation-nil justification to operator parity 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 Signed-off-by: Myasnikov Daniil --- internal/operator/package_reconciler_test.go | 9 +++++---- .../apps/application/rest_helmrelease_spec_test.go | 11 ++++++----- pkg/registry/apps/application/rest_timeout_test.go | 9 +++++---- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/internal/operator/package_reconciler_test.go b/internal/operator/package_reconciler_test.go index e6b1e4bcb..f687998a5 100644 --- a/internal/operator/package_reconciler_test.go +++ b/internal/operator/package_reconciler_test.go @@ -122,10 +122,11 @@ func TestBuildHelmReleaseSpec(t *testing.T) { if spec.Install.Strategy.RetryInterval == nil || spec.Install.Strategy.RetryInterval.Duration != 17*time.Second { t.Errorf("Install.Strategy.RetryInterval = %v, want 17s", spec.Install.Strategy.RetryInterval) } - // Remediation must remain nil: helm-controller's XValidation rule rejects - // Strategy.Name=RetryOnFailure with RetryInterval set alongside a - // Remediation entry, so a future "for safety" re-introduction of - // Remediation{Retries: -1} would silently break every HR. + // Remediation must remain nil: retries are driven solely by + // Strategy.Name=RetryOnFailure with an explicit RetryInterval. + // Re-introducing Remediation{Retries: -1} "for safety" would add a + // second, conflicting retry mechanism alongside the strategy retry + // path. if spec.Install.Remediation != nil { t.Errorf("Install.Remediation = %+v, want nil", spec.Install.Remediation) } diff --git a/pkg/registry/apps/application/rest_helmrelease_spec_test.go b/pkg/registry/apps/application/rest_helmrelease_spec_test.go index 4b8e01852..f574989cc 100644 --- a/pkg/registry/apps/application/rest_helmrelease_spec_test.go +++ b/pkg/registry/apps/application/rest_helmrelease_spec_test.go @@ -97,11 +97,12 @@ func TestConvertApplicationToHelmRelease_BuildsSpecFromConfig(t *testing.T) { if hr.Spec.Install.Strategy.RetryInterval == nil || hr.Spec.Install.Strategy.RetryInterval.Duration != 17*time.Second { t.Errorf("Install.Strategy.RetryInterval = %v, want 17s", hr.Spec.Install.Strategy.RetryInterval) } - // Remediation must remain nil: helm-controller's XValidation rule - // rejects Strategy.Name=RetryOnFailure with RetryInterval set - // alongside a Remediation entry, so a future "for safety" - // re-introduction of Remediation{Retries: -1} would silently break - // every HR. + // Remediation must remain nil: retries are driven solely by + // Strategy.Name=RetryOnFailure with an explicit RetryInterval, the + // same single retry path cozystack-operator's PackageReconciler + // uses. Re-introducing Remediation{Retries: -1} "for safety" would + // add a second, conflicting retry mechanism and break parity with + // the operator path. if hr.Spec.Install.Remediation != nil { t.Errorf("Install.Remediation = %+v, want nil", hr.Spec.Install.Remediation) } diff --git a/pkg/registry/apps/application/rest_timeout_test.go b/pkg/registry/apps/application/rest_timeout_test.go index 94b618ee0..77983822f 100644 --- a/pkg/registry/apps/application/rest_timeout_test.go +++ b/pkg/registry/apps/application/rest_timeout_test.go @@ -114,10 +114,11 @@ func TestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout(t *testing. } // Strategy must be RetryOnFailure with Remediation kept nil: - // helm-controller's XValidation rule rejects - // Strategy.Name=RetryOnFailure with RetryInterval set alongside - // a Remediation entry, so a future "for safety" re-introduction - // of Remediation{Retries: -1} would silently break every HR. + // retries are driven solely by Strategy.RetryInterval, the + // same single retry path cozystack-operator's + // PackageReconciler uses. Re-introducing + // Remediation{Retries: -1} "for safety" would add a second, + // conflicting retry mechanism and break operator parity. if hr.Spec.Install.Strategy == nil || hr.Spec.Install.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { t.Errorf("Spec.Install.Strategy must be RetryOnFailure, got %+v", hr.Spec.Install.Strategy) From a3b3851970a9c52c103b9d35942bbb434ce3e9d3 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Tue, 16 Jun 2026 14:25:13 +0500 Subject: [PATCH 06/10] docs(api): restore tenant-rd to HelmRelease timeout-annotation carriers The convertApplicationToHelmRelease comment listed only kubernetes-rd as carrying release.cozystack.io/helm-install-timeout, dropping tenant-rd. tenant-rd/cozyrds/tenant.yaml sets the 15m annotation for the seaweedfs-db CNPG bootstrap whose first reconcile exceeds the flux default. Restore the accurate two-carrier list and rationale. Address review feedback from lexfrei on rest.go. Assisted-By: Claude Signed-off-by: Myasnikov Daniil --- pkg/registry/apps/application/rest.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index 6b5c6c834..7edf73ddc 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -1527,12 +1527,15 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* // Per-Application annotation override (HelmInstallTimeout, populated // from release.cozystack.io/helm-install-timeout on the // ApplicationDefinition at startup) wins over the global defaults - // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). Today only - // kubernetes-rd carries the annotation because the Kubernetes - // Application's parent chart contains CAPI/Kamaji resources whose - // admin-kubeconfig Secret is provisioned asynchronously and Kamaji - // cold-start routinely exceeds flux's default wait budget; any future - // kind with the same shape can opt in by setting the same annotation. + // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). + // kubernetes-rd and tenant-rd carry the annotation today: the + // Kubernetes Application's parent chart contains CAPI/Kamaji + // resources whose admin-kubeconfig Secret is provisioned + // asynchronously and Kamaji cold-start routinely exceeds flux's + // default wait budget, and the Tenant parent chart bootstraps the + // seaweedfs-db CNPG cluster whose first reconcile exceeds it too. + // Any future kind with the same shape can opt in by setting the + // same annotation. installTimeout := r.releaseConfig.HelmReleaseInstallTimeout upgradeTimeout := r.releaseConfig.HelmReleaseUpgradeTimeout if r.releaseConfig.HelmInstallTimeout > 0 { From c98232a7dffc7df8d83fb6160f6317e1213cc067 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Tue, 16 Jun 2026 14:25:47 +0500 Subject: [PATCH 07/10] style(api,operator): gofmt test case-table struct alignment gofmt -w on the two touched test files: the case-table struct fields in rest_helmrelease_spec_test.go and main_test.go were misaligned, and main_test.go carried a trailing blank line. CI runs no gofmt gate (pre-commit only runs make generate), so this would otherwise land unformatted. Address review feedback from lexfrei. Assisted-By: Claude Signed-off-by: Myasnikov Daniil --- cmd/cozystack-operator/main_test.go | 11 +++++------ .../apps/application/rest_helmrelease_spec_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/cmd/cozystack-operator/main_test.go b/cmd/cozystack-operator/main_test.go index 325da96b1..f13a22d0d 100644 --- a/cmd/cozystack-operator/main_test.go +++ b/cmd/cozystack-operator/main_test.go @@ -139,11 +139,11 @@ func TestInstallPlatformPackageSource_Updates(t *testing.T) { func TestParsePlatformSourceURL(t *testing.T) { tests := []struct { - name string - url string - wantType string - wantURL string - wantErr bool + name string + url string + wantType string + wantURL string + wantErr bool }{ { name: "OCI URL", @@ -572,4 +572,3 @@ func TestGenerateGitRepository_InvalidRef(t *testing.T) { t.Fatal("expected error for invalid Git ref key, got nil") } } - diff --git a/pkg/registry/apps/application/rest_helmrelease_spec_test.go b/pkg/registry/apps/application/rest_helmrelease_spec_test.go index f574989cc..7c210d606 100644 --- a/pkg/registry/apps/application/rest_helmrelease_spec_test.go +++ b/pkg/registry/apps/application/rest_helmrelease_spec_test.go @@ -169,12 +169,12 @@ func TestConvertApplicationToHelmRelease_ZeroMaxHistory(t *testing.T) { // checked symmetric globals. func TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal(t *testing.T) { cases := []struct { - name string - globalInstall time.Duration - globalUpgrade time.Duration - override time.Duration - wantInstall time.Duration - wantUpgrade time.Duration + name string + globalInstall time.Duration + globalUpgrade time.Duration + override time.Duration + wantInstall time.Duration + wantUpgrade time.Duration }{ { name: "symmetric globals, override wins", From e1677f17c0f88736acfc50df45ab5b770111626e Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Tue, 16 Jun 2026 14:26:14 +0500 Subject: [PATCH 08/10] fix(config): preserve HelmReleaseMaxHistory=0 across YAML marshal HelmReleaseMaxHistory carried omitempty, so a value of 0 (which Helm treats as "unlimited") would be dropped on marshal and become indistinguishable from an unset field. The runtime path (start.go always populates it from --helmrelease-max-history) is safe today, but drop omitempty so the explicit "unlimited" intent survives if ReleaseConfig is ever serialized. Address review feedback from coderabbitai on config.go. Assisted-By: Claude Signed-off-by: Myasnikov Daniil --- pkg/config/config.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 9c952ae30..a5854d968 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -127,8 +127,10 @@ type ReleaseConfig struct { HelmReleaseUpgradeTimeout time.Duration `yaml:"helmReleaseUpgradeTimeout,omitempty"` // HelmReleaseMaxHistory is the global default for Spec.MaxHistory. // 0 means unlimited per Helm semantics; matches the cozystack-operator - // flag of the same name. - HelmReleaseMaxHistory int `yaml:"helmReleaseMaxHistory,omitempty"` + // flag of the same name. No omitempty: 0 ("unlimited") must survive a + // round-trip distinct from an unset field if ReleaseConfig is ever + // marshalled. + HelmReleaseMaxHistory int `yaml:"helmReleaseMaxHistory"` } // ChartRefConfig references a Flux source artifact for the Helm chart. From da3c54b78f1e607bc438c15fec1fabe7267256b3 Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Tue, 16 Jun 2026 14:26:58 +0500 Subject: [PATCH 09/10] test(config): assert ParsePositiveDuration error branch in table The ParsePositiveDuration table exercised all error inputs but did not distinguish which branch fired. Add an errMatch field so the table pins "must be > 0" for zero/negative and "invalid duration" for malformed/empty inputs, mirroring TestParseHelmInstallTimeoutAnnotation and keeping the two parsers' diagnostics from drifting. Address review feedback from coderabbitai on config_test.go. Assisted-By: Claude Signed-off-by: Myasnikov Daniil --- pkg/config/config_test.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 632175f5f..fae58d016 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -124,18 +124,23 @@ func TestParseHelmInstallTimeoutAnnotation(t *testing.T) { // HelmRelease and being rejected by helm-controller's CRD validation later. func TestParsePositiveDuration(t *testing.T) { tests := []struct { - name string - raw string - want time.Duration - wantErr bool + name string + raw string + want time.Duration + // errMatch pins which of the two error branches fired: a + // time.ParseDuration failure ("invalid duration") versus a + // non-positive value ("must be > 0"). Keeps the two diagnostics + // from drifting and operators seeing the wrong message. + errMatch string + wantErr bool }{ {name: "valid seconds", raw: "30s", want: 30 * time.Second}, {name: "valid minutes", raw: "5m", want: 5 * time.Minute}, {name: "valid compound", raw: "1h30m", want: 90 * time.Minute}, - {name: "zero rejected", raw: "0s", wantErr: true}, - {name: "negative rejected", raw: "-5m", wantErr: true}, - {name: "malformed rejected", raw: "5x", wantErr: true}, - {name: "empty rejected", raw: "", wantErr: true}, + {name: "zero rejected", raw: "0s", wantErr: true, errMatch: "must be > 0"}, + {name: "negative rejected", raw: "-5m", wantErr: true, errMatch: "must be > 0"}, + {name: "malformed rejected", raw: "5x", wantErr: true, errMatch: "invalid duration"}, + {name: "empty rejected", raw: "", wantErr: true, errMatch: "invalid duration"}, } for _, tt := range tests { @@ -145,6 +150,9 @@ func TestParsePositiveDuration(t *testing.T) { if err == nil { t.Fatalf("expected error for %q, got nil", tt.raw) } + if tt.errMatch != "" && !strings.Contains(err.Error(), tt.errMatch) { + t.Errorf("error %q does not contain %q", err.Error(), tt.errMatch) + } return } if err != nil { From d405cdc16f15940ef3cda2e27d24ce791b4f3def Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Tue, 16 Jun 2026 14:31:20 +0500 Subject: [PATCH 10/10] feat(api): add helm-upgrade-timeout annotation for asymmetric budgets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Myasnikov Daniil --- pkg/cmd/server/start.go | 25 ++++++-- pkg/config/config.go | 26 ++++++-- pkg/config/config_test.go | 4 +- pkg/registry/apps/application/rest.go | 16 +++-- .../apps/application/rest_timeout_test.go | 60 +++++++++++++++++++ 5 files changed, 113 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/server/start.go b/pkg/cmd/server/start.go index 2a37bcc19..ce9785317 100644 --- a/pkg/cmd/server/start.go +++ b/pkg/cmd/server/start.go @@ -137,9 +137,10 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions) "same annotation also overrides --helmrelease-upgrade-timeout.") flags.StringVar(&o.HelmReleaseUpgradeTimeout, "helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout, "Default timeout for the Helm upgrade action of HelmReleases generated from Application "+ - "resources (Spec.Upgrade.Timeout). Overridden per-Application by the same "+ - "release.cozystack.io/helm-install-timeout annotation as --helmrelease-install-timeout: "+ - "one annotation overrides both install and upgrade timeouts together.") + "resources (Spec.Upgrade.Timeout). Overridden per-Application by the "+ + "release.cozystack.io/helm-install-timeout annotation (which sets both install and "+ + "upgrade), or by release.cozystack.io/helm-upgrade-timeout to override only the upgrade "+ + "side for a kind that needs an asymmetric budget.") flags.IntVar(&o.HelmReleaseMaxHistory, "helmrelease-max-history", o.HelmReleaseMaxHistory, "Number of release revisions Helm keeps for HelmReleases generated from Application "+ "resources (Spec.MaxHistory). 0 means unlimited; 5 matches Helm's default.") @@ -273,8 +274,10 @@ func (o *CozyServerOptions) Complete() error { // layer when building the HelmRelease Spec. The parser rejects // units Flux would reject at webhook time, so a bad annotation // surfaces as a loud startup failure instead of a silent drop to - // defaults. - d, err := config.ParseHelmInstallTimeoutAnnotation( + // defaults. helm-install-timeout covers both install and upgrade; + // helm-upgrade-timeout overrides only the upgrade side for kinds + // that need an asymmetric budget. + installTimeout, err := config.ParseHelmTimeoutAnnotation( crd.Annotations[config.HelmInstallTimeoutAnnotation], ) if err != nil { @@ -283,7 +286,17 @@ func (o *CozyServerOptions) Complete() error { crd.Name, config.HelmInstallTimeoutAnnotation, err, ) } - release.HelmInstallTimeout = d + release.HelmInstallTimeout = installTimeout + upgradeTimeout, err := config.ParseHelmTimeoutAnnotation( + crd.Annotations[config.HelmUpgradeTimeoutAnnotation], + ) + if err != nil { + return fmt.Errorf( + "ApplicationDefinition %q has invalid %s annotation: %w", + crd.Name, config.HelmUpgradeTimeoutAnnotation, err, + ) + } + release.HelmUpgradeTimeout = upgradeTimeout resource := config.Resource{ Application: config.ApplicationConfig{ Kind: crd.Spec.Application.Kind, diff --git a/pkg/config/config.go b/pkg/config/config.go index a5854d968..5a4c9d45d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -27,6 +27,14 @@ import ( // Upgrade.Timeout for a given Application kind. const HelmInstallTimeoutAnnotation = "release.cozystack.io/helm-install-timeout" +// HelmUpgradeTimeoutAnnotation is the ApplicationDefinition metadata +// annotation key that overrides only the Flux HelmRelease Upgrade.Timeout +// for a given Application kind. When set it takes precedence over the +// Upgrade.Timeout value HelmInstallTimeoutAnnotation would otherwise apply, +// letting a kind keep a short install budget but a longer upgrade budget +// (or vice versa). +const HelmUpgradeTimeoutAnnotation = "release.cozystack.io/helm-upgrade-timeout" + // helmTimeoutPattern mirrors the CRD validation pattern used by Flux // helm-controller on HelmReleaseSpec.Install.Timeout (ms/s/m/h units only). // time.ParseDuration accepts ns/us/µs, but Flux rejects them - parsing here @@ -36,14 +44,15 @@ const HelmInstallTimeoutAnnotation = "release.cozystack.io/helm-install-timeout" // in the go module cache. var helmTimeoutPattern = regexp.MustCompile(`^([0-9]+(\.[0-9]+)?(ms|s|m|h))+$`) -// ParseHelmInstallTimeoutAnnotation parses the value of the -// release.cozystack.io/helm-install-timeout annotation. The empty string is -// treated as "unset" and returns (0, nil) so callers can leave -// HelmInstallTimeout zeroed and let flux defaults apply. Values accepted by +// ParseHelmTimeoutAnnotation parses the value of a Flux HelmRelease timeout +// annotation (release.cozystack.io/helm-install-timeout or +// release.cozystack.io/helm-upgrade-timeout). The empty string is treated as +// "unset" and returns (0, nil) so callers can leave the corresponding +// ReleaseConfig field zeroed and let flux defaults apply. Values accepted by // time.ParseDuration but rejected by Flux (ns/us/µs) return a helpful // error instead of silently parsing and failing later at HelmRelease // admission. -func ParseHelmInstallTimeoutAnnotation(raw string) (time.Duration, error) { +func ParseHelmTimeoutAnnotation(raw string) (time.Duration, error) { if raw == "" { return 0, nil } @@ -106,6 +115,13 @@ type ReleaseConfig struct { // Populated from the release.cozystack.io/helm-install-timeout // annotation on the ApplicationDefinition at start-up. HelmInstallTimeout time.Duration `yaml:"helmInstallTimeout,omitempty"` + // HelmUpgradeTimeout is a per-Application override of Upgrade.Timeout + // only. When non-zero, it wins over both HelmReleaseUpgradeTimeout and + // the Upgrade.Timeout value HelmInstallTimeout would otherwise apply, + // so a kind can carry an asymmetric install/upgrade budget. Populated + // from the release.cozystack.io/helm-upgrade-timeout annotation on the + // ApplicationDefinition at start-up. + HelmUpgradeTimeout time.Duration `yaml:"helmUpgradeTimeout,omitempty"` // HelmReleaseInterval is the global default for Spec.Interval on // HelmReleases generated by cozystack-api. Set from the api server // --helmrelease-interval flag; mirrors the cozystack-operator flag of diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index fae58d016..fe7321163 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -15,7 +15,7 @@ import ( // - compound forms (the CRD pattern accepts repeats), // - units time.ParseDuration accepts but Flux rejects (ns, us, µs), // - outright garbage. -func TestParseHelmInstallTimeoutAnnotation(t *testing.T) { +func TestParseHelmTimeoutAnnotation(t *testing.T) { cases := []struct { name string input string @@ -98,7 +98,7 @@ func TestParseHelmInstallTimeoutAnnotation(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, err := ParseHelmInstallTimeoutAnnotation(tc.input) + got, err := ParseHelmTimeoutAnnotation(tc.input) if tc.wantErr { if err == nil { t.Fatalf("expected error, got duration=%v", got) diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index 7edf73ddc..d3014eac7 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -1524,11 +1524,14 @@ func latestMonitorTime(m *cozyv1alpha1.WorkloadMonitor) metav1.Time { // from Spec.Interval — the previous coupling caused failed installs to // retry only every 5m, exceeding E2E budgets. func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (*helmv2.HelmRelease, error) { - // Per-Application annotation override (HelmInstallTimeout, populated - // from release.cozystack.io/helm-install-timeout on the - // ApplicationDefinition at startup) wins over the global defaults - // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). - // kubernetes-rd and tenant-rd carry the annotation today: the + // Per-Application annotation overrides win over the global defaults + // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout): + // - HelmInstallTimeout (release.cozystack.io/helm-install-timeout) + // sets both Install.Timeout and Upgrade.Timeout; + // - HelmUpgradeTimeout (release.cozystack.io/helm-upgrade-timeout) + // then overrides only Upgrade.Timeout, so a kind can carry an + // asymmetric budget (short install, long upgrade or vice versa). + // kubernetes-rd and tenant-rd carry helm-install-timeout today: the // Kubernetes Application's parent chart contains CAPI/Kamaji // resources whose admin-kubeconfig Secret is provisioned // asynchronously and Kamaji cold-start routinely exceeds flux's @@ -1542,6 +1545,9 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* installTimeout = r.releaseConfig.HelmInstallTimeout upgradeTimeout = r.releaseConfig.HelmInstallTimeout } + if r.releaseConfig.HelmUpgradeTimeout > 0 { + upgradeTimeout = r.releaseConfig.HelmUpgradeTimeout + } maxHistory := r.releaseConfig.HelmReleaseMaxHistory helmRelease := &helmv2.HelmRelease{ diff --git a/pkg/registry/apps/application/rest_timeout_test.go b/pkg/registry/apps/application/rest_timeout_test.go index 77983822f..7c9f8e50b 100644 --- a/pkg/registry/apps/application/rest_timeout_test.go +++ b/pkg/registry/apps/application/rest_timeout_test.go @@ -136,3 +136,63 @@ func TestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout(t *testing. }) } } + +// HelmUpgradeTimeout (the per-Application release.cozystack.io/helm-upgrade-timeout +// annotation override) overrides only Upgrade.Timeout, and wins over the +// Upgrade.Timeout value HelmInstallTimeout would otherwise apply. This lets a +// kind carry an asymmetric budget — e.g. a short install but a long upgrade. +func TestConvertApplicationToHelmRelease_UpgradeTimeoutAnnotation(t *testing.T) { + cases := []struct { + name string + installTimeout time.Duration // helm-install-timeout annotation + upgradeTimeout time.Duration // helm-upgrade-timeout annotation + wantInstall time.Duration + wantUpgrade time.Duration + }{ + { + name: "upgrade override alone leaves install on the global default", + installTimeout: 0, + upgradeTimeout: 20 * time.Minute, + wantInstall: testGlobalInstallTimeout, + wantUpgrade: 20 * time.Minute, + }, + { + name: "upgrade override wins over the install-annotation copy", + installTimeout: 15 * time.Minute, + upgradeTimeout: 25 * time.Minute, + wantInstall: 15 * time.Minute, + wantUpgrade: 25 * time.Minute, + }, + { + name: "install override alone still sets both (regression guard)", + installTimeout: 15 * time.Minute, + upgradeTimeout: 0, + wantInstall: 15 * time.Minute, + wantUpgrade: 15 * time.Minute, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := newRESTForTimeout("Example", "example-", tc.installTimeout) + r.releaseConfig.HelmUpgradeTimeout = tc.upgradeTimeout + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "example", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + if hr.Spec.Install == nil || hr.Spec.Upgrade == nil { + t.Fatalf("Spec.Install/Upgrade must be non-nil") + } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != tc.wantInstall { + t.Errorf("Spec.Install.Timeout = %v, want %v", hr.Spec.Install.Timeout, tc.wantInstall) + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != tc.wantUpgrade { + t.Errorf("Spec.Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout, tc.wantUpgrade) + } + }) + } +}