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) - } - } - } -}