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..ef36ce1f0b 100644 --- a/pkg/kagent/install/helm.go +++ b/pkg/kagent/install/helm.go @@ -34,8 +34,6 @@ import ( releasev1 "helm.sh/helm/v4/pkg/release/v1" "helm.sh/helm/v4/pkg/storage/driver" "k8s.io/cli-runtime/pkg/genericclioptions" - - "go.ate.dev/substrate/internal/version" ) // release names a Helm release and the chart it installs. @@ -159,7 +157,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 } @@ -219,7 +218,7 @@ func asV1(rel any, err error) (*releasev1.Release, error) { } // loadChart reads the chart in directory name of charts. -func loadChart(charts fs.FS, name string) (*chart.Chart, error) { +func loadChart(charts fs.FS, name, tag string) (*chart.Chart, error) { var files []*archive.BufferedFile err := fs.WalkDir(charts, name, func(p string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() { @@ -239,7 +238,7 @@ func loadChart(charts fs.FS, name string) (*chart.Chart, error) { if err != nil { return nil, fmt.Errorf("while loading chart %s: %w", path.Base(name), err) } - stampVersion(ch, version.Version) + stampVersion(ch, tag) return ch, nil } diff --git a/pkg/kagent/install/install_test.go b/pkg/kagent/install/install_test.go index 245cca59f8..b80d9cc2f9 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" @@ -90,6 +91,7 @@ func TestStampVersion(t *testing.T) { {"v1.2.3-rc.1", "1.2.3-rc.1", "v1.2.3-rc.1"}, {"dev", "0.1.0", "0.1.0"}, {"v1.2.3-4-gabcdef-dirty+meta", "0.1.0", "0.1.0"}, + {"", "0.1.0", "0.1.0"}, } for _, tc := range tests { t.Run(tc.tag, func(t *testing.T) { @@ -105,10 +107,13 @@ func TestStampVersion(t *testing.T) { func TestLoadEmbeddedCharts(t *testing.T) { for _, r := range allReleases { t.Run(r.chart, func(t *testing.T) { - ch, err := loadChart(chartsfs.FS, r.chart) + ch, err := loadChart(chartsfs.FS, r.chart, "v1.2.3") if err != nil { t.Fatal(err) } + if ch.Metadata.Version != "1.2.3" || ch.Metadata.AppVersion != "v1.2.3" { + t.Errorf("got version %q app %q, want 1.2.3 v1.2.3", ch.Metadata.Version, ch.Metadata.AppVersion) + } if ch.Name() != r.chart { t.Errorf("chart name = %q, want %q", ch.Name(), r.chart) } @@ -280,24 +285,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..393495a0b0 100644 --- a/pkg/kagent/install/installer.go +++ b/pkg/kagent/install/installer.go @@ -30,6 +30,7 @@ import ( chartsfs "go.ate.dev/substrate/charts" "go.ate.dev/substrate/internal/ateclient" "go.ate.dev/substrate/internal/installdefaults" + "go.ate.dev/substrate/internal/version" ) const ( @@ -71,6 +72,10 @@ type Options struct { Charts fs.FS // HelmLog receives Helm's debug log. Nil discards it. HelmLog io.Writer + // Version is the release tag, like v1.0.1, stamped on the charts. Their + // images default to it. Empty uses version.Version, which kubectl-ate sets + // at build time. + Version string } // Installer runs install steps against one cluster. @@ -107,6 +112,9 @@ func New(opts Options, r Reporter) (*Installer, error) { if err != nil { return nil, fmt.Errorf("while creating Kubernetes client: %w", err) } + if opts.Version == "" { + opts.Version = version.Version + } charts := opts.Charts if charts == nil { charts = chartsfs.FS @@ -163,9 +171,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..521deca08c 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{}) @@ -96,7 +101,7 @@ func noValues(context.Context) (map[string]any, map[string]any, error) { return // real upgrade. func (in *Installer) helmStep(r release, vf valuesFunc) func(context.Context, func(string)) (string, error) { return func(ctx context.Context, progress func(string)) (string, error) { - ch, err := loadChart(in.charts, r.chart) + ch, err := loadChart(in.charts, r.chart, in.opts.Version) if err != nil { return "", err } @@ -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/uninstall.go b/pkg/kagent/install/uninstall.go index 375ba5a228..604916271c 100644 --- a/pkg/kagent/install/uninstall.go +++ b/pkg/kagent/install/uninstall.go @@ -53,11 +53,10 @@ type UninstallOptions struct { Confirm func(ctx context.Context, summary string) (bool, error) } -// Uninstall removes the releases in the reverse of install order. It keeps -// CRDs, secrets, the database volume and the namespaces unless -// opts.IncludeCRDs or opts.Purge say otherwise. +// Uninstall shows UninstallSummary to opts.Confirm, then runs +// UninstallSteps. func (in *Installer) Uninstall(ctx context.Context, opts UninstallOptions) error { - summary, err := in.inventory(ctx, opts) + summary, err := in.UninstallSummary(ctx, opts) if err != nil { return err } @@ -70,6 +69,14 @@ func (in *Installer) Uninstall(ctx context.Context, opts UninstallOptions) error return ErrAborted } } + return in.Run(ctx, in.UninstallSteps(opts)) +} + +// UninstallSteps returns the steps that remove the releases in the reverse of +// install order. They keep CRDs, secrets, the database volume and the +// namespaces unless opts.IncludeCRDs or opts.Purge say otherwise. They don't +// call opts.Confirm. +func (in *Installer) UninstallSteps(opts UninstallOptions) []Step { steps := []Step{ {"Delete worker pools", in.deleteWorkerPools}, {"Uninstall Substrate", in.uninstallStep(substrateRelease)}, @@ -86,11 +93,11 @@ func (in *Installer) Uninstall(ctx context.Context, opts UninstallOptions) error Step{"Delete namespaces", in.deleteNamespaces}, ) } - return in.Run(ctx, steps) + return steps } -// inventory describes what Uninstall will remove, for opts.Confirm. -func (in *Installer) inventory(ctx context.Context, opts UninstallOptions) (string, error) { +// UninstallSummary describes what UninstallSteps will remove. +func (in *Installer) UninstallSummary(ctx context.Context, opts UninstallOptions) (string, error) { var installed []string for _, r := range allReleases { rel, err := in.helm.get(r) diff --git a/pkg/kagent/install/upgrade.go b/pkg/kagent/install/upgrade.go index e57eaf01d8..1762a61938 100644 --- a/pkg/kagent/install/upgrade.go +++ b/pkg/kagent/install/upgrade.go @@ -52,8 +52,20 @@ type UpgradeOptions struct { Out io.Writer } -// Upgrade moves every release to the charts in this binary. +// Upgrade runs UpgradeSteps. func (in *Installer) Upgrade(ctx context.Context, opts UpgradeOptions) error { + steps, err := in.UpgradeSteps(ctx, opts) + if err != nil { + return err + } + return in.Run(ctx, steps) +} + +// UpgradeSteps returns the steps that move every release to the charts in +// this binary. It reads the cluster to check for the microVM SandboxConfig. +// After it is called, the Installer's helm steps upgrade releases across +// chart versions. +func (in *Installer) UpgradeSteps(ctx context.Context, opts UpgradeOptions) ([]Step, error) { in.upgrading = true steps := []Step{ {"Check versions", in.checkVersions}, @@ -63,7 +75,7 @@ func (in *Installer) Upgrade(ctx context.Context, opts UpgradeOptions) error { for _, u := range upgradeOrder { steps = append(steps, Step{"Diff " + u.r.name, in.diffStep(u.r, in.valuesFor(u.r), opts.Out)}) } - return in.Run(ctx, steps) + return steps, nil } steps = append(steps, Step{"Create pod certificate CAs", in.createPodCertCAs}, @@ -77,7 +89,7 @@ func (in *Installer) Upgrade(ctx context.Context, opts UpgradeOptions) error { // A new release may pin new microVM assets. microVM, err := in.hasMicroVMSandboxConfig(ctx) if err != nil { - return err + return nil, err } if microVM { steps = append(steps, @@ -86,7 +98,7 @@ func (in *Installer) Upgrade(ctx context.Context, opts UpgradeOptions) error { ) } steps = append(steps, Step{"Check API", in.checkAPI}) - return in.Run(ctx, steps) + return steps, nil } // hasMicroVMSandboxConfig reports whether the microvm SandboxConfig exists. @@ -126,7 +138,7 @@ func (in *Installer) checkVersions(_ context.Context, progress func(string)) (st if st := cur.Info.Status; st.IsPending() { return "", fmt.Errorf("release %s is %s; run helm rollback or helm uninstall on it, then retry", r.name, st) } - ch, err := loadChart(in.charts, r.chart) + ch, err := loadChart(in.charts, r.chart, in.opts.Version) if err != nil { return "", err } @@ -171,7 +183,7 @@ func checkSkew(from, to string) error { // checkCRDStorage refuses to drop a CRD version that objects are still // stored in, which would make them unreadable. func (in *Installer) checkCRDStorage(ctx context.Context, _ func(string)) (string, error) { - ch, err := loadChart(in.charts, crdsRelease.chart) + ch, err := loadChart(in.charts, crdsRelease.chart, in.opts.Version) if err != nil { return "", err } @@ -234,7 +246,7 @@ func checkStoredVersions(ctx context.Context, c apiextclient.Interface, crds []a // out. func (in *Installer) diffStep(r release, vf valuesFunc, out io.Writer) func(context.Context, func(string)) (string, error) { return func(ctx context.Context, _ func(string)) (string, error) { - ch, err := loadChart(in.charts, r.chart) + ch, err := loadChart(in.charts, r.chart, in.opts.Version) if err != nil { return "", err } @@ -242,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) - } - } - } -}