From a99a75ff91d14366ddcf4d6f807c5e5fffdfb07f Mon Sep 17 00:00:00 2001 From: Eitan Yarmush Date: Mon, 9 Nov 2026 03:57:03 +0000 Subject: [PATCH] install: make install and upgrade values declarative Upgrades reused each release's previous values, so removing a value or switching storage modes from the config had no effect. Now every install and upgrade sets values from the chart and the config only. Install refuses once the substrate release is deployed, so changes to a finished install go through upgrade, which can diff them first. Signed-off-by: Eitan Yarmush --- cmd/kubectl-ate/internal/cmd/install.go | 12 ++++---- pkg/kagent/install/bootstrap.go | 17 ++--------- pkg/kagent/install/helm.go | 3 +- pkg/kagent/install/install_test.go | 40 ++++++++++++++----------- pkg/kagent/install/installer.go | 6 ++-- pkg/kagent/install/steps.go | 25 ++++++---------- pkg/kagent/install/upgrade.go | 2 +- pkg/kagent/install/upgrade_test.go | 20 ------------- 8 files changed, 48 insertions(+), 77 deletions(-) diff --git a/cmd/kubectl-ate/internal/cmd/install.go b/cmd/kubectl-ate/internal/cmd/install.go index 2b21932eec..70d984cccd 100644 --- a/cmd/kubectl-ate/internal/cmd/install.go +++ b/cmd/kubectl-ate/internal/cmd/install.go @@ -82,7 +82,8 @@ var installCmd = &cobra.Command{ Installs the substrate-crds, substrate-podcert, substrate-atelet and substrate Helm releases in order, and creates the CA pools and secrets they need. -Existing secrets are kept. Rerunning install resumes a failed install. +Existing secrets are kept. Rerunning install resumes a failed install. Once +the substrate release is deployed, install refuses to run; use upgrade. Without --config, install uses a single-replica PostgreSQL for evaluation and a RustFS for snapshots. On a terminal it asks for these choices first and @@ -128,10 +129,11 @@ kubectl ate: the certificate controller, atelet, the CRDs, then the substrate release. It doesn't roll WorkerPool worker images, which docs/reference/version-skew-policy.md upgrades after atelet. -Each release keeps the values it was installed with. The config's -releases values are layered on top. Upgrade also reads timeout and -microvm.assetBucket from the config. It refuses downgrades, skipped minor -releases, and CRD changes that would strand stored objects. +Upgrade doesn't reuse the values a release was installed with. Each +release's values come from the chart and the config, the same way install +sets them, so pass the config you installed with, changed as needed. +It refuses downgrades, skipped minor releases, and CRD changes that would +strand stored objects. --dry-run prints each release's manifest diff and changes nothing.`, Args: cobra.NoArgs, diff --git a/pkg/kagent/install/bootstrap.go b/pkg/kagent/install/bootstrap.go index f4cb5e810a..9bf43f8eed 100644 --- a/pkg/kagent/install/bootstrap.go +++ b/pkg/kagent/install/bootstrap.go @@ -346,21 +346,10 @@ func (in *Installer) configureObjectStore(ctx context.Context, _ func(string)) ( return "", err } if in.opts.ObjectStore.Type != ObjectStoreBundled { - // A resumed install keeps the values the releases already have. atelet, substrate := in.externalStoreValues() - vals := map[release]map[string]any{} - for r, typed := range map[release]map[string]any{ateletRelease: atelet, substrateRelease: substrate} { - rel, err := in.helm.get(r) - if err != nil { - return "", err - } - var stored map[string]any - if rel != nil { - stored = rel.Config - } - vals[r] = mergeValues(stored, in.user[r.name], typed) - } - return in.checkExternalObjectStore(ctx, vals[substrateRelease], vals[ateletRelease]) + return in.checkExternalObjectStore(ctx, + mergeValues(in.user[substrateRelease.name], substrate), + mergeValues(in.user[ateletRelease.name], atelet)) } if ok, err := secretExists(ctx, in.kube, SystemNamespace, rustfsCredentialsSecret); ok || err != nil { return "already present", err diff --git a/pkg/kagent/install/helm.go b/pkg/kagent/install/helm.go index ee1e501cd1..cf521bf8ce 100644 --- a/pkg/kagent/install/helm.go +++ b/pkg/kagent/install/helm.go @@ -159,7 +159,8 @@ func (h *helmClient) newUpgrade(r release) (*action.Upgrade, error) { } up := action.NewUpgrade(cfg) up.Namespace = r.namespace - up.ResetThenReuseValues = true + // Don't reuse the release's previous values. + up.ResetValues = true return up, nil } diff --git a/pkg/kagent/install/install_test.go b/pkg/kagent/install/install_test.go index 245cca59f8..d0ac658106 100644 --- a/pkg/kagent/install/install_test.go +++ b/pkg/kagent/install/install_test.go @@ -17,6 +17,7 @@ package install import ( "bytes" "context" + "fmt" "io" "reflect" "slices" @@ -280,24 +281,29 @@ func TestCheckExternalObjectStore(t *testing.T) { } } +// Install and upgrade set the same values from the config. func TestExternalObjectStoreValues(t *testing.T) { - in := &Installer{opts: Options{Config: Config{ObjectStore: ObjectStore{Type: ObjectStoreGCS}, Postgres: Postgres{Type: PostgresExternal}}}} - defaults, forced, err := in.ateletValues(context.Background()) - if err != nil { - t.Fatal(err) - } - if defaults != nil { - t.Errorf("atelet defaults = %v, want none", defaults) - } - if got := forced["objectStore"].(map[string]any)["backend"]; got != "gcs" { - t.Errorf("atelet objectStore.backend = %v, want gcs", got) - } - _, forced, err = in.substrateValues(context.Background()) - if err != nil { - t.Fatal(err) - } - if got := forced["rustfs"].(map[string]any)["enabled"]; got != false { - t.Errorf("substrate rustfs.enabled = %v, want false", got) + for _, upgrading := range []bool{false, true} { + t.Run(fmt.Sprintf("upgrading=%v", upgrading), func(t *testing.T) { + in := &Installer{upgrading: upgrading, opts: Options{Config: Config{ObjectStore: ObjectStore{Type: ObjectStoreGCS}, Postgres: Postgres{Type: PostgresExternal}}}} + defaults, forced, err := in.ateletValues(context.Background()) + if err != nil { + t.Fatal(err) + } + if defaults != nil { + t.Errorf("atelet defaults = %v, want none", defaults) + } + if got, _ := lookupString(forced, "objectStore", "backend"); got != "gcs" { + t.Errorf("atelet objectStore.backend = %q, want gcs", got) + } + _, forced, err = in.substrateValues(context.Background()) + if err != nil { + t.Fatal(err) + } + if rustfs, _ := forced["rustfs"].(map[string]any); rustfs["enabled"] != false { + t.Errorf("substrate rustfs = %v, want enabled false", forced["rustfs"]) + } + }) } } diff --git a/pkg/kagent/install/installer.go b/pkg/kagent/install/installer.go index 45ec7df55c..0cc92cadbc 100644 --- a/pkg/kagent/install/installer.go +++ b/pkg/kagent/install/installer.go @@ -163,9 +163,9 @@ func (in *Installer) Run(ctx context.Context, steps []Step) error { return nil } -// Install runs InstallSteps. Rerunning it is safe and resumes a failed -// install: existing secrets are kept and existing releases are upgraded in -// place when their chart version matches. +// Install runs InstallSteps. It refuses a finished install. Rerunning it +// resumes a failed install: existing secrets are kept and existing releases +// are upgraded in place when their chart version matches. func (in *Installer) Install(ctx context.Context) error { steps, err := in.InstallSteps() if err != nil { diff --git a/pkg/kagent/install/steps.go b/pkg/kagent/install/steps.go index 481255ef75..cb2095d1d9 100644 --- a/pkg/kagent/install/steps.go +++ b/pkg/kagent/install/steps.go @@ -54,6 +54,11 @@ func (in *Installer) preflight(ctx context.Context, progress func(string)) (stri if err != nil { return "", err } + if rel != nil && rel.Info.Status == common.StatusDeployed { + // The substrate release is the last one, so the install finished. + // Changing it is upgrade's job, which can diff it first. + return "", fmt.Errorf("release %s %s is already installed; use kubectl ate upgrade to change it", rel.Name, rel.Chart.Metadata.Version) + } if rel == nil { // ate-setup installs with kustomize, which this command can't manage. _, err := in.kube.AppsV1().Deployments(SystemNamespace).Get(ctx, "ate-controller", metav1.GetOptions{}) @@ -121,7 +126,7 @@ func (in *Installer) helmStep(r release, vf valuesFunc) func(context.Context, fu return "", fmt.Errorf("release %s has chart version %s, want %s; use kubectl ate upgrade", r.name, got, want) } } - vals, err := in.releaseValues(ctx, r, vf, cur == nil) + vals, err := in.releaseValues(ctx, r, vf) if err != nil { return "", err } @@ -144,17 +149,13 @@ func (in *Installer) helmStep(r release, vf valuesFunc) func(context.Context, fu } } -// releaseValues returns the values to install or upgrade r with. Installer -// defaults apply only to a first install. An upgrade reuses the release's -// values, so defaults would override what the user set before. -func (in *Installer) releaseValues(ctx context.Context, r release, vf valuesFunc, first bool) (map[string]any, error) { +// releaseValues returns the values to install or upgrade r with. The +// release's previous values aren't reused. +func (in *Installer) releaseValues(ctx context.Context, r release, vf valuesFunc) (map[string]any, error) { defaults, forced, err := vf(ctx) if err != nil { return nil, err } - if !first { - defaults = nil - } return mergeValues(defaults, in.user[r.name], forced), nil } @@ -162,10 +163,6 @@ func (in *Installer) releaseValues(ctx context.Context, r release, vf valuesFunc // backend of an external store. func (in *Installer) ateletValues(context.Context) (map[string]any, map[string]any, error) { forced := map[string]any{"substrateReleaseName": substrateRelease.name} - if in.upgrading { - // The release keeps its storage values. - return nil, forced, nil - } if in.opts.ObjectStore.Type != ObjectStoreBundled { atelet, _ := in.externalStoreValues() return nil, mergeValues(forced, atelet), nil @@ -187,10 +184,6 @@ func (in *Installer) substrateValues(ctx context.Context) (map[string]any, map[s // atelet is its own release so it can upgrade first. "atelet": map[string]any{"enabled": false}, } - if in.upgrading { - // The release keeps its storage and database values. - return nil, forced, nil - } if in.opts.ObjectStore.Type == ObjectStoreBundled { access, secret, err := in.rustfsCredentials(ctx) if err != nil { diff --git a/pkg/kagent/install/upgrade.go b/pkg/kagent/install/upgrade.go index f83b08430c..e6f8d590bf 100644 --- a/pkg/kagent/install/upgrade.go +++ b/pkg/kagent/install/upgrade.go @@ -254,7 +254,7 @@ func (in *Installer) diffStep(r release, vf valuesFunc, out io.Writer) func(cont if err != nil { return "", err } - vals, err := in.releaseValues(ctx, r, vf, false) + vals, err := in.releaseValues(ctx, r, vf) if err != nil { return "", err } diff --git a/pkg/kagent/install/upgrade_test.go b/pkg/kagent/install/upgrade_test.go index 852f840f19..3a3bbfcc2c 100644 --- a/pkg/kagent/install/upgrade_test.go +++ b/pkg/kagent/install/upgrade_test.go @@ -107,23 +107,3 @@ func TestWriteDiff(t *testing.T) { t.Errorf("identical manifests: n = %d, output %q", n, buf.String()) } } - -// Upgrade runs without install options, so it must keep the stored storage -// values instead of forcing new ones. -func TestUpgradeKeepsStorageValues(t *testing.T) { - in := &Installer{upgrading: true} - for _, f := range []valuesFunc{in.ateletValues, in.substrateValues} { - defaults, forced, err := f(context.Background()) - if err != nil { - t.Fatal(err) - } - if defaults != nil { - t.Errorf("defaults = %v, want none", defaults) - } - for _, k := range []string{"objectStore", "rustfs", "postgres"} { - if v, ok := forced[k]; ok { - t.Errorf("forced %s = %v on upgrade", k, v) - } - } - } -}