From d571f55aa01e0e873b9912bad1c3e6904467b00e Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 20 Aug 2026 17:19:27 +0300 Subject: [PATCH 01/15] [Feature] Add config-driven CE installer script Move the MLRun CE installer from its standalone repo into this one, so it ships alongside the chart it installs. The installer keeps installing the PUBLISHED chart by default. This repo's chart is used only when the caller passes --chart-path ./charts/mlrun-ce. The script is also served via curl | bash, where no repo exists around it, so the same invocation has to mean the same thing in both places. Repo integration: - make installer-test / installer-lint targets (bats, bash -n, shellcheck) - Installer CI workflow: lint and unit tests on PRs touching scripts/**, plus a workflow_dispatch-only kind end-to-end install - gitignore ce-config.yaml so filled-in registry details can't be committed - installer linked from the root and chart READMEs Align the pre-install version validators with the chart's own prerequisites rather than the product install docs: Helm >= 3.6 blocking, matching charts/mlrun-ce/README.md, and no Kubernetes floor at all, since the chart declares no kubeVersion and the README states no cluster version. The previous K8s >= 1.34 / Helm >= 4.1 floors rejected nearly every supported cluster and every Helm 3 user for a chart that renders fine on Helm 3. MIN_K8S_VERSION now only warns; MIN_HELM_VERSION remains the one hard floor and can be raised to tighten. --- .github/workflows/installer-ci.yaml | 89 ++ .gitignore | 4 + AGENTS.md | 2 + CONTRIBUTING.md | 6 + Makefile | 9 + README.md | 3 + charts/mlrun-ce/Chart.yaml | 2 +- charts/mlrun-ce/README.md | 6 + scripts/AGENTS.md | 339 +++++++ scripts/README.md | 150 +++ scripts/ce-config.yaml.example | 77 ++ scripts/docs/configuration.md | 352 +++++++ scripts/docs/faq.md | 143 +++ scripts/docs/parameters.md | 97 ++ scripts/install.sh | 1417 +++++++++++++++++++++++++++ 15 files changed, 2695 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/installer-ci.yaml create mode 100644 scripts/AGENTS.md create mode 100644 scripts/README.md create mode 100644 scripts/ce-config.yaml.example create mode 100644 scripts/docs/configuration.md create mode 100644 scripts/docs/faq.md create mode 100644 scripts/docs/parameters.md create mode 100755 scripts/install.sh diff --git a/.github/workflows/installer-ci.yaml b/.github/workflows/installer-ci.yaml new file mode 100644 index 00000000..bba62382 --- /dev/null +++ b/.github/workflows/installer-ci.yaml @@ -0,0 +1,89 @@ +name: Installer CI + +# Kept separate from ci.yaml because `paths` filters apply to the whole +# workflow trigger, not to individual jobs — putting this in ci.yaml would +# either skip the helm jobs on chart-only PRs or run this on every PR. +on: + pull_request: + branches: + - development + - "[0-9]+.[0-9]+.x" + paths: + - "scripts/**" + - "tests/install_tests.bats" + - ".github/workflows/installer-ci.yaml" + workflow_dispatch: + +permissions: + contents: read + +env: + BATS_VERSION: v1.13.0 + +jobs: + lint-and-test: + name: Lint and unit-test install.sh + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Syntax check + run: bash -n scripts/install.sh + + # shellcheck and yq (mikefarah) ship with the ubuntu-latest runner image. + # yq is needed for real: the load_config tests parse actual YAML. + - name: Show tool versions + run: | + shellcheck --version + yq --version + + - name: Run shellcheck + run: shellcheck scripts/install.sh + + - name: Install bats + run: | + git clone --depth 1 --branch "${BATS_VERSION}" \ + https://github.com/bats-core/bats-core.git /tmp/bats-core + sudo /tmp/bats-core/install.sh /usr/local + bats --version + + - name: Run installer unit tests + run: make installer-test + + kind-install: + name: End-to-end install on kind + # Dispatch-only for the same reason ci.yaml's `test:` job is commented out: + # pulling the full MLRun CE image set takes too long for every PR. + if: github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + # No node_image/kubectl_version pin: the installer enforces no Kubernetes floor, so + # the action's own default node image (matched to the kind release it bundles) is + # the safest choice. + - name: Set up Kubernetes cluster + uses: helm/kind-action@v1.10.0 + with: + config: ./.github/assets/kind.yaml + wait: 180s + + - name: Set up Helm + uses: azure/setup-helm@1a275c3b69536ee54be43f2070a358922e12c8d4 # v4.3.1 + + - name: Install MLRun CE from the in-repo chart + # --local-registry deploys a registry in-cluster and derives the pull + # secret from it, so no external registry credentials are needed. + run: | + ./scripts/install.sh \ + --chart-path ./charts/mlrun-ce \ + --local-registry \ + --non-interactive + + - name: Dump cluster state on failure + if: failure() + run: | + kubectl get pods -A -o wide + kubectl get events -A --sort-by=.lastTimestamp | tail -50 diff --git a/.gitignore b/.gitignore index b0c9b07b..0297e920 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,7 @@ otlp-pro/ # Claude Code local settings (machine-specific, not for commit) .claude/settings.local.json + +# Filled-in installer config (copied from scripts/ce-config.yaml.example). +# Unanchored, so it matches at any depth; ce-config.yaml.example is unaffected. +ce-config.yaml diff --git a/AGENTS.md b/AGENTS.md index 27681f1f..b8ed5090 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,6 +4,8 @@ This is a Helm umbrella chart repository for **MLRun Community Edition (CE)** — an open-source MLOps stack. The main chart lives at `charts/mlrun-ce/` and bundles: Nuclio, MLRun, Jupyter, MPI Operator, SeaweedFS (S3-compatible storage), Spark Operator, Kubeflow Pipelines, Prometheus stack, TimescaleDB, and Strimzi Kafka Operator. +The repo also ships `scripts/install.sh`, a bash installer that wraps `helm install` for this chart. It installs the **published** chart by default and this repo's chart only when given `--chart-path ./charts/mlrun-ce`. Its own conventions, phase history and bug log live in [`scripts/AGENTS.md`](scripts/AGENTS.md) — read that when working under `scripts/`; this file covers the chart. + ## Commands to lint, package, and manage the chart: ```bash diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6caf85f8..da92977e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,9 +6,15 @@ |---|---|---| | helm | 3.6 | Chart rendering, linting, install | | kubectl | 1.24 | Cluster interaction | +| bats-core | 1.5 | Only for `make installer-test` (the `scripts/install.sh` unit tests) | +| shellcheck | any | Only for `make installer-lint` | For Kubernetes storage class setup and cluster prerequisites, see [charts/mlrun-ce/README.md](charts/mlrun-ce/README.md#prerequisites). +`scripts/install.sh` enforces the same helm 3.6 floor at install time and imposes no +Kubernetes floor, so a cluster you can develop against is one you can install against — see +[scripts/docs/configuration.md](scripts/docs/configuration.md#version-floors). + ## First-Time Setup ```bash diff --git a/Makefile b/Makefile index 6415679a..02fb241d 100644 --- a/Makefile +++ b/Makefile @@ -33,6 +33,15 @@ tests: ## Run tests package: ## Package the application @./tests/package.sh +.PHONY: installer-test +installer-test: ## Run the scripts/install.sh unit tests (requires bats-core) + @bats tests/install_tests.bats + +.PHONY: installer-lint +installer-lint: ## Syntax-check and shellcheck scripts/install.sh + @bash -n scripts/install.sh + @shellcheck scripts/install.sh + .PHONY: helm-lint helm-lint: helm-repo-add ## Lint Helm Chart @helm lint charts/mlrun-ce diff --git a/README.md b/README.md index 88267190..85e76aed 100644 --- a/README.md +++ b/README.md @@ -16,3 +16,6 @@ The Open source MLRun CE chart includes the following stack: ## Installation Refer to the installation instructions in the [README](charts/mlrun-ce/README.md) of the `mlrun-ce` chart. + +For a scripted install, [`scripts/install.sh`](scripts/README.md) wraps those steps — registry +secret, pre-install validation and `helm install` — behind a single command. diff --git a/charts/mlrun-ce/Chart.yaml b/charts/mlrun-ce/Chart.yaml index d3307933..7fa7aaa2 100644 --- a/charts/mlrun-ce/Chart.yaml +++ b/charts/mlrun-ce/Chart.yaml @@ -1,6 +1,6 @@ apiVersion: v1 name: mlrun-ce -version: 0.12.0-rc.10 +version: 0.12.0-rc.11 appVersion: 1.12.0-rc25 description: MLRun Open Source Stack home: https://iguazio.com diff --git a/charts/mlrun-ce/README.md b/charts/mlrun-ce/README.md index e00b12f3..0351707c 100644 --- a/charts/mlrun-ce/README.md +++ b/charts/mlrun-ce/README.md @@ -29,6 +29,12 @@ The Open source MLRun ce chart includes the following stack: ## Installing the Chart +> **Scripted alternative:** [`scripts/install.sh`](../../scripts/README.md) wraps everything +> below — namespace creation, the registry secret, pre-install validation and the `helm +> install` itself — behind one command, and can read its settings from a `ce-config.yaml` +> for repeatable installs. It installs the published chart by default. The manual steps +> below remain fully supported. + Create a namespace for the deployed components: ```bash kubectl create namespace mlrun diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md new file mode 100644 index 00000000..f422b753 --- /dev/null +++ b/scripts/AGENTS.md @@ -0,0 +1,339 @@ +## What this directory is + +`scripts/install.sh` — single-file bash wrapper around `helm install mlrun-ce/mlrun-ce` +(published chart repo: `https://mlrun.github.io/ce`). Local execution only, no SSH, no git +fetching. Full design/rationale: `docs/design-proposal.md`. + +It lives in the same repo as the chart it installs (`charts/mlrun-ce`), but installs the +**published** chart by default — the in-repo chart is used only when the caller passes +`--chart-path ./charts/mlrun-ce` explicitly. That's deliberate: the script is also served +via `curl | bash`, where no repo exists around it, so the same invocation has to mean the +same thing in both places. + +For chart-side conventions (values.yaml layout, `requirements.lock`, adding components) see +the repo-root `AGENTS.md`/`CONTRIBUTING.md`. This file covers the installer only. + +## Install flow (`main()`, install.sh ~1319-1378) + +1. `parse_args` — flags/env, precedence flag > env > default +2. `check_requirements` — helm, kubectl, docker present and reachable +3. `ensure_namespace` — creates `NAMESPACE`; in `--dry-run` only logs what it would do (no cluster mutation) +4. `resolve_external_host` — only if `LOCAL_REGISTRY` or `ENABLE_INGRESS` +5. `deploy_local_registry` — only if `--local-registry` +6. `create_registry_secret` — skipped via `--skip-secret`, or when `-f VALUES_FILE` is given **and** `--config` is absent (pure `-f`-only mode keeps its old self-contained behavior; `-f` + `--config` together still creates the secret). In `--dry-run` only logs, doesn't touch the cluster +7. `gather_install_params` — resolves `REGISTRY_URL` (same `-f`-only skip condition as above) +8. `run_validators` — skippable via `--skip-validators`; includes `validate_ingress_controller` (warns if `--enable-ingress`'s IngressClass isn't found — see Phase 4/5 notes below) +9. `helm_install` → `resolve_chart_source` (published repo vs `--chart-path`) → `helm install/upgrade`, with `--values`/`--set` composed per the precedence rule below + +`install_ingress_controller` (which used to `helm install` the `ingress-nginx` chart when +`--enable-ingress` was passed) was **removed** — see "Phase 5" below. + +## Phase status (see docs/design-proposal.md for full plan) + +- **Phase 1 (done):** `--ce-version` fix, `--dry-run`, `--non-interactive` + `CI=true` + auto-detect. Originally shipped as `--helm-version`/`HELM_VERSION`; renamed (hard + rename, no alias) once it became clear the flag pins the **mlrun-ce chart** version, + not the Helm CLI binary — `check_requirements`/`validate_helm_version` (Phase 4) are + the ones actually about the Helm binary, and shared the word "helm" confusingly with + this one. +- **Phase 2 (done):** `--chart-path DIR` / `resolve_chart_source` — install from a locally cloned chart dir instead of the published repo +- **Phase 3 (done):** `--config`/`CONFIG_FILE` + `load_config()` — reads the `installer:` + block of a `ce-config.yaml` (requires `yq`, only when `--config` is used) and fills in + registry url/username/server/email, `externalHostAddress`, `chartSource.{kind,chartVersion,chartPath}` + and the new `KUBE_CONTEXT` as defaults, at flag/env > config-file > default precedence. + `KUBE_CONTEXT` is threaded through every `kubectl`/`helm` call via wrapper functions + (install.sh top-of-file) rather than editing each call site — `check_requirements` uses + `type -P` instead of `command -v` for helm/kubectl so those wrappers don't cause a false + "installed" detection. `installer.ingress.*` from the original design draft was **not** + implemented (deferred; use existing `--enable-ingress` instead) — at the time this + triggered real infra (nginx-ingress install) beyond a `--set`; `--enable-ingress` no + longer installs anything (see "Phase 5" below), but `installer.ingress.*` config-file + keys remain unimplemented since `--enable-ingress`/`--enable-ingress CLASS` already + cover the same knobs via flag/env. + `installer.components.{monitoring,spark,mpi,modelMonitoring}` **was** added in a + follow-up pass — mirrors the `--disable-*` flags' exact `--set` effects 1:1 (pure value + resolution, `false` disables, flag/env always wins and is never re-enabled by the + file); `--local-registry`'s infra-deploying behavior was explicitly scoped out the same + way as `ingress.*`. OpenTelemetry support was added in a later follow-up pass as its + own `installer.otel.{operator,collector,namespaceLabel,instrumentation}` block (not + under `components.*`, since it's 4 independent knobs, not one flat bool) — the reverse + direction from `components.*`: those 4 chart values (`opentelemetry-operator.enabled`, + `opentelemetry.collector.enabled`, `opentelemetry.namespaceLabel.enabled`, + `opentelemetry.instrumentation.enabled`) all ship `false` by default (unlike + `kube-prometheus-stack`/`spark-operator`/etc., which ship `true`), so each key opts IN + via the matching `--enable-otel-{operator,collector,namespace-label,instrumentation}`/ + `ENABLE_OTEL_*` flag/env pair (a bare `--enable-otel` is a convenience that sets all + four). Kept as 4 independent toggles rather than one bundled flag deliberately — + `namespaceLabel` auto-instruments every pod in the release namespace once + `instrumentation` is also on, a materially bigger blast radius than `operator`/ + `collector` alone, so a user should be able to pick just the latter two. A flag/env-set + `ENABLE_OTEL_*=true` still always wins and is never unset by the file, same "add, + never override" rule as `components.*`. Later given an optional MODE argument — + `--enable-otel [off|collector|full]` (bare = `full`, matching the original behavior + exactly) — as a convenience on top of, not a replacement for, the 4 granular flags + (which still work individually and combine with it). `collector` exists as a named + middle ground: operator+collector only (a metrics pipeline), without opting into + `namespaceLabel`'s bigger blast radius. + **`--config` and `-f`/`--values` compose** (a later follow-up reverted an earlier + deviation): the original design draft had `--config` knobs layer as `--set` overrides + on top of `-f`'s `--values` (helm applies `--set` after `--values`, so `--config` + always wins, no merge logic needed) — the first implementation pass never actually + wired that up (silently dead instead) and was "fixed" at the time by making the two + mutually exclusive (`main()` exit 1). That exclusivity was itself the wrong fix and was + removed: `main()` now runs `create_registry_secret`/`gather_install_params` whenever + `--config` is given (even alongside `-f`) rather than only when `-f` is absent, and + `helm_install` moved the 3 registry `--set`s (`global.registry.{url,secretName}`, + `global.externalHostAddress`) into the same `extra_set_flags` array the other + `--set`s already used, gated off only in pure `-f`-only mode (no `--config`) — so + every config-resolved field now actually reaches helm as a `--set` layered on top of + `--values` in every combination. `-f` used alone (no `--config`) keeps its exact prior + self-contained behavior unchanged (no secret creation, values file must reference an + existing secret) — only adding `--config` changes that. Precedence documented in + README as: flag > env > `ce-config.yaml` (→ `--set`) > `-f`/`--values` (raw) > chart + defaults. Added `installer.versions.{mlrun,nuclio}` / `MLRUN_VERSION`/`NUCLIO_VERSION` + env vars (→ `--set mlrun.{api,ui}.image.tag`, `nuclio.{controller,dashboard}.image.tag`) + for per-service version pins, independent of `--ce-version`/`chartSource.chartVersion` + which pins the mlrun-ce umbrella chart as a whole. Also added + `verify_existing_registry_secret()`: `--skip-secret` now exits 1 immediately if the + named secret doesn't exist in the namespace, instead of failing later via an opaque + `helm --wait` timeout. `REGISTRY_PASSWORD_FILE` was added as a third way to supply the + registry password (path to a local file, trailing newline stripped, never echoed) — + same "never settable via `ce-config.yaml`" rule as `REGISTRY_PASSWORD`; precedence is + `REGISTRY_PASSWORD` env > `REGISTRY_PASSWORD_FILE` > interactive masked prompt. +- **Phase 4 (done):** `run_validators()` — a pre-install dispatcher, called from `main()` + right after the `-f`/`--config` secret-creation branch and right before `helm_install`, + skippable via `--skip-validators`/`SKIP_VALIDATORS` (mirrors `--skip-secret`'s exact + flag/env/`main()` pattern). Runs unconditionally in every mode, including pure + `-f`-only (the cluster-level checks don't depend on registry resolution; the + registry-auth check self-skips when nothing's resolved yet). Six checks, all read-only + (no `--dry-run` gating needed): + - **Blocking** (`validate_helm_version`, `validate_storage_class`): Helm CLI >= 3.6 + (parsed from `helm version --short`) and a default StorageClass exists + (`is-default-class` annotation). These `return 1` instead of calling `exit 1` directly + (the one deliberate deviation from the rest of the script's inline + `log_error; exit 1` style) so `run_validators` can run every check and report *all* + failures in one pass, then exit 1 once at the end — rather than stopping at the first + problem found. + - **Warning-only** (`validate_k8s_version`, `validate_registry_auth`, + `validate_nodeport_conflicts`, `validate_node_capacity`): the cluster's Kubernetes + version, read via `kubectl get nodes` `.status.nodeInfo.kubeletVersion` (the same + jsonpath-on-nodes style `resolve_external_host` already uses — avoids depending on + `kubectl version` supporting `-o jsonpath`), reported always and compared only against + an explicitly set `MIN_K8S_VERSION`; best-effort `docker login` with the resolved registry + creds (skipped, not warned, under `--local-registry` or when nothing's resolved yet); + the chart's fixed NodePorts (`30010/20/40/50/60/70`, `30093/94`, `30100`, `30110`) + already bound by a Service outside the target namespace (excluding the target + namespace so `helm upgrade` of the same release never self-flags); total cluster + allocatable RAM/ephemeral-storage under the documented 8Gi/8Gi floor (no CPU floor + exists in the docs to check against, despite the original proposal bullet loosely + saying "CPU/mem"). + - **Supporting fix needed along the way:** `create_registry_secret`'s + `username`/`password`/`server` are locals — `prompt_or_env` returns a value via + stdout, it never sets the named env var globally, so in the (common) interactive-prompt + case the real entered password/server were invisible outside the function. Only + `REGISTRY_USERNAME_VALUE` existed as a side-channel export (for `gather_install_params`'s + suggested-URL default). Added the same-shaped `REGISTRY_PASSWORD_VALUE` and + `REGISTRY_SERVER_VALUE` so `validate_registry_auth` can see what was actually entered, + regardless of whether it came from env, `REGISTRY_PASSWORD_FILE`, or an interactive + prompt. +- **Phase 5 — dropped, folded into existing `--enable-ingress` instead of a new phase:** + the chart's UI Ingress toggle (`mlrun.ui.ingress.enabled`) already ships as part of the + pre-existing `--enable-ingress` flag (alongside `jupyterNotebook`/`nuclio.dashboard`/ + `mlrun.api` Ingress — install.sh `helm_install`'s `extra_set_flags`), so there was no + separate toggle left to add. The one real gap — `--enable-ingress` used to `helm + install` the actual `ingress-nginx` controller itself, a real infra dependency the + installer had no business installing — was fixed directly on that flag: removed + `install_ingress_controller()` entirely, and added `validate_ingress_controller()` as a + warning-only check in `run_validators` (Phase 4) that looks for an existing IngressClass + matching `--enable-ingress`'s class and warns (doesn't block, doesn't install) if none + is found. `--enable-ingress` is now BYO-controller-only. `print_ui_ingress_url()` (the + other original Phase 5 idea — reading the created Ingress back and printing its URL in + the final access table) was not built; out of scope unless asked for separately. +- **Phase 6 (done, as real CI rather than the originally planned samples):** + `.github/workflows/installer-ci.yaml` runs `bash -n`, shellcheck and the bats suite + (via `make installer-lint`/`make installer-test`) on PRs touching `scripts/**` or + `tests/install_tests.bats`, plus a `workflow_dispatch`-only kind end-to-end install + using `--chart-path ./charts/mlrun-ce --local-registry`. It's a separate workflow file + rather than a job inside `ci.yaml` because GitHub applies `paths:` filters at the + workflow trigger, not per job — folding it into `ci.yaml` would either run it on every + PR or skip the chart jobs on a scripts-only PR. The kind job is dispatch-only for the + same reason `ci.yaml`'s `test:` job is commented out (pulling the full image set is too + slow for every PR). No Jenkinsfile — this repo is GitHub Actions only. +## Phase 3 pre-work (resolved, see docs/design-proposal.md §6) + +Before implementing Phase 3 (`ce-config.yaml` + `yq`), these open questions were +resolved by checking the official MLRun docs and the chart itself (`../charts/mlrun-ce`, +then still a separate repo): + +- **`yq` dependency:** acceptable. No network calls at runtime, so runtime CVE exposure + is limited to YAML parsing. Require it only when `--config` is passed (no new dep for + existing flows); pin an exact release version and verify its checksum rather than an + unpinned install. +- **K8s/Helm version floor** (for Phase 4's validator): originally taken from the official + install docs (**Kubernetes >= 1.34**, **Helm >= 4.1**) and enforced as blocking. **This was + later reversed** — see "Version floors realigned" below. Neither is enforced by the chart + itself (no `kubeVersion` in `Chart.yaml`). +- **Mandatory `ce-config.yaml` fields** (cross-checked against what `install.sh` already + hard-enforces in `create_registry_secret`/`gather_install_params`): + `installer.registry.url` (unless `--local-registry`), `installer.registry.secret.username`, + `installer.registry.secret.password` (required but **never read from the file** — env/ + prompt only), `installer.chartSource.chartPath` (only when `chartSource.kind: path`). + Everything else (`registry.secret.server`, `registry.secret.email`, `chartVersion`, + `kubeContext`, `externalHostAddress`, `ingress.*`, `components.*`) is optional — + documented in full in `docs/configuration.md`'s "Config file (`ce-config.yaml`)" section. +- **NodePorts/components for Phase 4:** no additions beyond the list already in + `docs/design-proposal.md` Phase 4. + +## Version floors realigned (supersedes the Phase 3 pre-work finding above) + +The blocking **K8s >= 1.34 / Helm >= 4.1** floors, sourced from docs.mlrun.org, were replaced +with the chart's own stated requirement: + +- **Helm >= 3.6**, blocking — mirrors `charts/mlrun-ce/README.md`'s prerequisites, so the + installer can't refuse a Helm the chart itself supports. Helm 4.1 as a *minimum* excluded + every Helm 3 user for a chart that renders fine on Helm 3. +- **No Kubernetes floor.** `validate_k8s_version` is now informational: it reports the + detected version, returns 0 on every path, and is no longer in `run_validators`' + `|| failed=1` group. `MIN_K8S_VERSION` defaults to empty and only produces a *warning* + when set. The chart declares no `kubeVersion` and the README states no cluster version, + so there was no requirement to enforce — 1.34 as a hard minimum rejected nearly every + supported managed cluster, including the local `docker-desktop` (1.30.5) used for testing. + +An intermediate step (before this realignment) kept the strict floors but made them +overridable via `MIN_K8S_VERSION`/`MIN_HELM_VERSION`. Those env vars survive, but their +purpose inverted: they now exist to *tighten* rather than loosen. `MIN_HELM_VERSION` is the +only hard floor; `MIN_K8S_VERSION` never blocks. + +Knock-on effect: `.github/workflows/installer-ci.yaml`'s kind job had pinned +`kubectl_version`/`node_image` to `v1.34.0` and Helm to `v4.1.1` purely to satisfy those +floors. Those pins were removed — `helm/kind-action@v1.10.0` bundles a kind release +predating K8s 1.34, so that node image likely wasn't even published for it. + +## Known non-bugs + +- `--dry-run` uses `helm --dry-run=server`, which validates against the live API + server. If the target cluster lacks the Prometheus Operator CRDs, the + `kube-prometheus-stack` subchart's `PrometheusRule`/`ServiceMonitor` resources + fail server-side validation. This is a Helm limitation (charts with CRDs + can't fully dry-run without those CRDs present), not an `install.sh` bug. +- **Two `mlrun-ce` releases can't coexist on one cluster**, even in different + namespaces with different release/secret names and NodePort overrides via `-f`. + Confirmed live: the chart's `workflow-controller` `PriorityClass` is + cluster-scoped with a hardcoded name (no values.yaml knob), so a second + release's `helm install` fails immediately with an ownership-metadata error + once one release already owns it. Not an `install.sh` bug — the chart itself + has no multi-release story on a shared cluster short of patching that template. +- `helm uninstall` (and `--hard-clean`) can leave orphaned Strimzi `Kafka`/ + `KafkaNodePool`/`StrimziPodSet` custom resources and their broker pod behind: + once the `strimzi-kafka-operator` Deployment is gone, nothing reconciles those + CRs, so the broker pod keeps running and its PVC's `kubernetes.io/pvc-protection` + finalizer blocks `--hard-clean`'s PVC deletion indefinitely. Fix is manual: + delete the `strimzipodset` and pod directly (releases the finalizer), then the + `kafka`/`kafkanodepool` CRs. Not something `do_hard_clean` can anticipate from + `install.sh` alone — it's a chart/Strimzi ordering issue. + +## Fixed bugs + +- **`do_hard_clean()`'s force-delete fallback could hang indefinitely** (found via live + testing against the `vmdev137` lab cluster — a real `--hard-clean` run sat blocked for + 18+ hours): both the PVC and PV delete loops fall back to + `kubectl delete ... --force --grace-period=0` when the graceful `--timeout 60s` delete + fails, but neither fallback passed `--wait=false` — by default `kubectl delete` still + blocks waiting for the object to actually disappear from the API, `--force` only skips + *graceful* deletion of the underlying pod, not the wait. When a PVC has a lingering + `kubernetes.io/pvc-protection` finalizer (the exact orphaned-Strimzi-Kafka scenario in + "Known non-bugs" below), nothing ever removes that finalizer, so the fallback hung just + as long as the primary attempt — defeating the point of having a fallback at all, + worse still under `run_in_background` where a silently hung command gives no signal + anything is wrong. Fix: added `--wait=false` to both fallback commands, so they return + immediately once the delete request is accepted, regardless of whether the object's + removal actually completes. +- **`validate_node_capacity()` silently read ephemeral-storage as 0Gi on some clusters** + (found via live testing against local `docker-desktop`): the parser only matched + Ki-suffixed quantities (the form `.status.allocatable.memory` always uses), but + `.status.allocatable.ephemeral-storage` is commonly reported as a bare byte integer + with no unit suffix (cAdvisor-sourced, confirmed live: `56403987978` on this cluster, + vs. memory's `7922684Ki`) — the regex silently skipped every line, so the sum stayed 0 + and the warning read "~0Gi" instead of the real ~52Gi. Fix: added `_allocatable_to_ki()`, + a small quantity parser that handles `Ki`/`Mi`/`Gi`/`Ti` suffixes and a bare + byte-integer form, used by both the memory and ephemeral-storage loops. +- **`resolve_external_host()`'s docker-desktop/minikube autodetect ignored `KUBE_CONTEXT`** + (install.sh:602, found via live testing against a remote `--kube-context`): the + `kubectl config current-context` check always reports the kubeconfig's *ambient* + current-context, not the one selected by `--context`/`KUBE_CONTEXT` — that flag + has no effect on that particular subcommand. So targeting a non-current + `KUBE_CONTEXT` (e.g. a Jenkins agent with a shared kubeconfig selecting a named + remote cluster by context, a common CI pattern especially with concurrent jobs on + one agent where mutating global current-context per job is a race condition) + could silently misdetect the ambient ("docker-desktop") environment instead of the + actual target cluster's, and that value flows straight into the chart via + `--set global.externalHostAddress=...` — not just cosmetic. Fix: when + `KUBE_CONTEXT` is non-empty, skip the minikube/docker-desktop heuristics entirely + (they're statements about the *local machine's* own environment, meaningless once + a specific — possibly remote — context is explicitly selected) and go straight to + the node-IP fallback, which already goes through the `KUBE_CONTEXT`-aware + `kubectl` wrapper. Still just a suggested default (`prompt_or_env` default arg) — + set `EXTERNAL_HOST_ADDRESS`/`installer.externalHostAddress` explicitly when the + node IP itself isn't reachable from where `install.sh` runs (e.g. still behind an + SSH tunnel to the target cluster). +- **`resolve_external_host()`'s generic fallback (no heuristic matched) now suggests + `localhost` instead of a node-IP lookup.** Only the truly generic case changed — the + `KUBE_CONTEXT` branch (node IP; needed for e.g. the `vmdev137` lab pattern, where the + node's real internal IP is reachable on the corporate network but `localhost` would + resolve to nothing since only the API server port is SSH-tunneled) and the minikube/ + docker-desktop heuristics are untouched. The generic case (no `KUBE_CONTEXT`, not + minikube, not docker-desktop — e.g. kind/k3d) previously did a node-IP lookup that's + frequently unreachable for those tools, which typically NodePort-map to `localhost` + instead. Still just a suggested default — override with `EXTERNAL_HOST_ADDRESS` when + it's wrong for a given cluster. + +## Testing + +- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 96 tests, no cluster needed (sources + `install.sh` with `INSTALL_SH_SOURCE_ONLY=true`, stubs external binaries). +- Live/integration: exercise `--chart-path` against a real chart checkout (see + below). Non-interactive runs need `REGISTRY_USERNAME`/`REGISTRY_PASSWORD` + (or `REGISTRY_PASSWORD_FILE`)/`REGISTRY_EMAIL` set or they'll fail on the + required-value check in `create_registry_secret`. +- **Verified against a real remote cluster via `--kube-context`**: `install.sh` + itself never SSHes anywhere (still local-execution-only), but `kubectl`/`helm` + can target any cluster reachable from the local machine — including one behind + SSH, via a local port-forward tunnel (`ssh -f -N -L ::6443 + user@jumphost`) plus a kubeconfig context whose `server:` points at + `localhost:` (works cleanly when the cert's SANs already include + `localhost`/`127.0.0.1`, true for kubeadm/rke2 defaults). `--config` + `-f` + composition and `REGISTRY_PASSWORD_FILE` were both confirmed working through + such a tunnel against a live `rke2` lab cluster, in addition to local + `docker-desktop` runs. The remote run reached a fully healthy state (26/26 + containers ready, `helm status` → `deployed`) — notably including `mlrun-ui`, + which fails on local Apple Silicon `docker-desktop` runs only because that + image has no `linux/arm64` build; this lab cluster is x86_64. + + **Not yet torn down** (deliberately, as of this writing): the `mlrun-ce` + release from this verification is still `deployed` in the `mlrun` namespace + on the shared `vmdev137` lab cluster, and the local SSH tunnel + (`ssh -f -N -L 16443:192.168.236.51:6443 iguazio@app1.vmdev137ig4.lab.iguaz.io`, + backing the local `vmdev137` kubeconfig context) is still running. Tear down + with `KUBE_CONTEXT=vmdev137 ./scripts/install.sh --uninstall --hard-clean + --non-interactive` (also deletes its PVCs) when done needing it — that + command is destructive enough against shared remote infra that it's worth + running deliberately rather than as a matter of course. + +## Cross-reference: the chart + +The chart is now in this same repo at `charts/mlrun-ce` (it used to be a separate clone +reached via an absolute `--chart-path`; the installer was merged into the chart repo). +It has `Chart.yaml`, and its dependency subcharts are fetched into +`charts/mlrun-ce/charts/` by `helm dependency update`, which `resolve_chart_source` runs +automatically in local-path mode. + +The installer only ever **reads** the chart — it never writes to `charts/`. Chart changes +follow the repo-root `AGENTS.md`/`CONTRIBUTING.md` (values.yaml conventions, +`requirements.lock`, version bumps), which are a separate concern from this directory. + +Re-run the live dry-run test from the repo root: + +``` +REGISTRY_USERNAME=x REGISTRY_PASSWORD=y REGISTRY_EMAIL=z@z.com \ + ./scripts/install.sh --chart-path ./charts/mlrun-ce --dry-run --non-interactive +``` diff --git a/scripts/README.md b/scripts/README.md new file mode 100644 index 00000000..744ed34f --- /dev/null +++ b/scripts/README.md @@ -0,0 +1,150 @@ +# MLRun CE Installer + +Interactive installer for the MLRun CE Helm chart on a local or CI Kubernetes cluster. + +By default it installs the **published** chart from `https://mlrun.github.io/ce`, so the +`curl | bash` one-liner below works with no repo checked out. To install this repo's own +chart instead — the working tree, on whatever branch you have checked out — pass +`--chart-path ./charts/mlrun-ce`. See [Install this repo's chart](#install-this-repos-chart). + +**More docs:** [Parameters reference](docs/parameters.md) (all flags/env vars) · +[Configuration](docs/configuration.md) (`ce-config.yaml`, ingress, local registry, +OpenTelemetry, precedence) · [FAQ](docs/faq.md) (known gotchas) + +--- + +## Requirements + +| Tool | Notes | +|-----------|-----------------------------------------------------| +| `helm` | [install](https://helm.sh/docs/intro/install/) | +| `kubectl` | Configured and pointing at your cluster | +| `docker` | Daemon running and accessible by your user | +| `yq` | Only required when using `--config`/`CONFIG_FILE` — [install](https://github.com/mikefarah/yq/releases) (pin a version, verify its checksum) | + +--- + +## Quick Start + +### Run directly (no download) + +```bash +curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh | bash +``` + +### Install as a named command + +```bash +curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh \ + -o /usr/local/bin/mlrun-install && chmod +x /usr/local/bin/mlrun-install +mlrun-install +``` + +### From a clone of this repo + +```bash +./scripts/install.sh +``` + +Still installs the published chart. Add `--chart-path ./charts/mlrun-ce` to install the +chart from your working tree instead. + +--- + +## Installation flows + +### Interactive install + +Run the script with no flags. You will be prompted for: + +1. Docker registry username, password, server URL, and email +2. External host address (auto-detected — see [FAQ](docs/faq.md) for the fallback chain) +3. Docker registry URL for images + +```bash +./scripts/install.sh +``` + +### Non-interactive / CI install + +Set `CI=true` (or pass `--non-interactive`) and export the required values. The script will never prompt — any required value that is missing causes an immediate exit 1 instead of hanging on stdin. + +```bash +export CI=true +export REGISTRY_USERNAME=myuser +export REGISTRY_PASSWORD=mypassword +export REGISTRY_SERVER=https://index.docker.io/v1/ +export REGISTRY_EMAIL=me@example.com +export REGISTRY_URL=index.docker.io/myuser +export EXTERNAL_HOST_ADDRESS=localhost # or minikube ip + +./scripts/install.sh +``` + +### Install this repo's chart + +The installer ships alongside the chart it installs, but does **not** install it by +default — pass `--chart-path ./charts/mlrun-ce` to install the chart from your working +tree instead of the published release. This is how you test a branch or PR: check it out +here, then point the installer at the chart directory. The installer runs +`helm dependency update` on the path before installing; no git operations happen inside +the script. + +```bash +# 1. Check out the branch/PR you want to test +git checkout my-branch + +# 2. Dry-run first to validate without deploying +./scripts/install.sh --chart-path ./charts/mlrun-ce --dry-run + +# 3. Install for real +./scripts/install.sh --chart-path ./charts/mlrun-ce +``` + +`--ce-version` is ignored in local-path mode — the chart version comes from +`charts/mlrun-ce/Chart.yaml` as it exists on your branch. Drop `--chart-path` (and +optionally add `--ce-version`) to go back to installing a published release. + +### Dry run + +Renders the Helm chart and validates it against the cluster API server without creating any resources. Useful to confirm a configuration is valid before a real install. + +```bash +./scripts/install.sh --dry-run +``` + +Pre-install validators still run in `--dry-run` — see [Configuration](docs/configuration.md#pre-install-validators). + +### Uninstall + +```bash +./scripts/install.sh --uninstall +``` + +This runs `helm uninstall` with a 960s timeout. The namespace and CRDs are **not** deleted. For deleting persistent data too, see the [FAQ](docs/faq.md#deleting-everything-including-the-namespace). + +--- + +## What's next + +- Configuring the registry, chart source, ingress, local registry, disabled components, or OpenTelemetry → [docs/configuration.md](docs/configuration.md) +- Every flag and environment variable → [docs/parameters.md](docs/parameters.md) +- Something looks like a bug but isn't → [docs/faq.md](docs/faq.md) + +--- + +## After Installation + +Once complete, the script prints an access table with URLs and credentials for each service: + +``` +============================================== + MLRun CE - Access URLs +============================================== +SERVICE | URL | CREDENTIALS +-------------------+--------------------------+------------------ +MLRun UI | http://localhost:80 | +Grafana | http://localhost:3000 | admin / prom-operator +... +============================================== +``` diff --git a/scripts/ce-config.yaml.example b/scripts/ce-config.yaml.example new file mode 100644 index 00000000..651d6f46 --- /dev/null +++ b/scripts/ce-config.yaml.example @@ -0,0 +1,77 @@ +# Sample config for scripts/install.sh --config ce-config.yaml (or CONFIG_FILE=ce-config.yaml). +# Only the reserved `installer:` block is read; everything else is ignored. +# See docs/configuration.md's "Config file (ce-config.yaml)" section for the full field list. +# +# Copy this file, edit it, and do NOT commit real registry credentials into it — +# the password is never read from this file in the first place (REGISTRY_PASSWORD env, +# REGISTRY_PASSWORD_FILE env, or the interactive prompt, only). +# +# Precedence (highest wins): CLI flag > env var > this file (resolved to --set) > +# -f/--values file (raw values) > chart defaults. --config can be combined with +# -f/--values: helm applies --set after --values, so any field this file sets always +# wins over the same key in a -f file, with no extra merge logic needed. -f used alone +# (no --config) keeps its own self-contained behavior (you supply global.registry.*, +# versions, everything, install.sh sets nothing on top, no secret creation). + +installer: + # kubectl/helm context to use. Blank = current local kubeconfig context (no SSH, + # no remote exec — install.sh always runs locally). + kubeContext: "" + + # "auto" runs the existing local autodetect (minikube ip / docker-desktop / node IP). + # Pin a value (e.g. an IP or hostname) to skip autodetection. + externalHostAddress: auto + + registry: + # Registry URL used for chart images (global.registry.url), e.g. index.docker.io/. + # Required unless installing with --local-registry. + url: index.docker.io/myuser + secret: + # Required. Falls back to REGISTRY_USERNAME / the interactive prompt if unset here. + username: myuser + # Never set a password here — it is not read from this file. Set REGISTRY_PASSWORD, + # REGISTRY_PASSWORD_FILE (path to a file containing just the password), or answer + # the interactive prompt instead. + server: https://index.docker.io/v1/ + email: me@example.com + + chartSource: + # repo (default): install the published mlrun-ce/mlrun-ce chart, optionally pinned + # to chartVersion (same as --ce-version). + # path: install from a local chart directory (chartPath is then required — + # same as --chart-path). Use ./charts/mlrun-ce for this repo's chart. + kind: repo + chartVersion: "" + chartPath: "" + + # Pin individual service versions on top of the chart defaults (same as MLRUN_VERSION / + # NUCLIO_VERSION env vars). Independent of chartSource/chartVersion above, which pins the + # mlrun-ce umbrella chart as a whole. + versions: + mlrun: "" # -> --set mlrun.{api,ui}.image.tag=..., mlrun.api.sidecars.logCollector.image.tag=... + nuclio: "" # -> --set nuclio.{controller,dashboard}.image.tag=... + + # Mirrors --disable-system-monitoring / --disable-spark / --disable-mpi / + # --disable-model-monitoring exactly: false disables (same --set flags), true/omitted + # leaves the component enabled. A --disable-* flag/env always wins — this file can only + # add a disable, never remove one set on the command line. + components: + monitoring: true # false -> --set kube-prometheus-stack.enabled=false + spark: true # false -> --set spark-operator.enabled=false + mpi: true # false -> --set mpi-operator.{deployment,crd,rbac}.*=false + modelMonitoring: true # false -> --set {strimzi-kafka-operator,kafka,timescaledb}.enabled=false + + # OpenTelemetry — all 4 ship OFF in the chart, so each key here opts IN (mirrors + # --enable-otel-* / ENABLE_OTEL_* flags, not the --disable-* mirror above). Independent + # knobs, not one bundled toggle, so you can enable e.g. just the operator+collector + # without namespaceLabel/instrumentation's namespace-wide auto-instrumentation. + # A flag/env-set ENABLE_OTEL_* always wins — this file can only turn one on. + otel: + operator: false # -> --set opentelemetry-operator.enabled=true (CRDs, webhook, manager) + collector: false # -> --set opentelemetry.collector.enabled=true (Collector Deployment -> Prometheus) + namespaceLabel: false # -> --set opentelemetry.namespaceLabel.enabled=true + # Labels/annotates NAMESPACE so every Python pod in it gets + # auto-instrumented once instrumentation is also on. Cluster-wide + # blast radius within the namespace — review before enabling. + instrumentation: false # -> --set opentelemetry.instrumentation.enabled=true (Instrumentation CR: + # propagators, sampler, Python/Java agent images) \ No newline at end of file diff --git a/scripts/docs/configuration.md b/scripts/docs/configuration.md new file mode 100644 index 00000000..b397f5df --- /dev/null +++ b/scripts/docs/configuration.md @@ -0,0 +1,352 @@ +# Configuration + +How to drive `install.sh` beyond the interactive defaults: the `ce-config.yaml` file, +values files, precedence between all the input sources, and the flows that configure +ingress, a local registry, disabled components, and OpenTelemetry. + +For the full flag/env-var list, see [parameters.md](parameters.md). For "why is X +behaving this way" gotchas, see [faq.md](faq.md). + +--- + +## Install from a config file + +Reads registry/chart-source/host defaults from a `ce-config.yaml`. See +[Config file schema](#config-file-ce-configyaml) below. CLI flags and env vars still +take precedence over the file. + +**`--config` works in both interactive and non-interactive mode — you don't have to +choose.** Without `--non-interactive`/`CI=true`, you still get prompted for anything not +already set by a flag/env var, but each prompt's default is now pre-filled from the +config file — press Enter to accept it, or type something else to override just that one +field for this run: + +```bash +cp scripts/ce-config.yaml.example ce-config.yaml # edit with your registry/chart-source values +./scripts/install.sh --config ce-config.yaml +# e.g. "Docker registry password [from a value you can't put in the file]: " still prompts, +# but "Docker registry URL for images [index.docker.io/myuser]: " now shows the file's value — +# just press Enter to accept it. +``` + +For CI, add `--non-interactive` (or `CI=true`) so any field missing from *both* the file +and a flag/env fails fast with exit 1 instead of hanging on a prompt: + +```bash +CI=true ./scripts/install.sh --config ce-config.yaml +``` + +## Install from a values file + +Skips secret creation and all prompts: + +```bash +./scripts/install.sh -f my-values.yaml +``` + +## Install with a remote kubeconfig context + +```bash +KUBE_CONTEXT=my-remote-cluster ./scripts/install.sh +``` + +`install.sh` itself never SSHes anywhere — it still only runs `kubectl`/`helm` against +whatever cluster `KUBE_CONTEXT` (or the ambient current-context) points at. To reach a +cluster that's only accessible over SSH (e.g. a lab VM), open a local port-forward +tunnel yourself first, then add a context whose `server:` points at `localhost:`: + +```bash +ssh -f -N -L 16443::6443 user@jump-host +kubectl config set-cluster my-remote-cluster --server=https://localhost:16443 \ + --certificate-authority= --embed-certs=true +kubectl config set-credentials my-remote-cluster --client-certificate= \ + --client-key= --embed-certs=true +kubectl config set-context my-remote-cluster --cluster=my-remote-cluster --user=my-remote-cluster + +KUBE_CONTEXT=my-remote-cluster ./scripts/install.sh --dry-run --non-interactive +``` + +This works cleanly when the remote cluster's serving cert already lists `localhost`/ +`127.0.0.1` in its SANs (true for kubeadm/rke2 defaults). When targeting a non-current +`KUBE_CONTEXT`, `EXTERNAL_HOST_ADDRESS` autodetection skips the minikube/docker-desktop +heuristics (which only make sense for the *ambient* local environment) and falls back to +the target cluster's node IP — set `EXTERNAL_HOST_ADDRESS`/`installer.externalHostAddress` +explicitly if that node IP isn't actually reachable from where you're running +`install.sh` (e.g. still behind the SSH tunnel). See [faq.md](faq.md) for the full +autodetect fallback chain. + +## Install with live progress UI + +Shows a refreshing table of deployments and statefulsets while Helm runs: + +```bash +./scripts/install.sh --show-progress +``` + +## Pin the chart version + +```bash +./scripts/install.sh --ce-version 0.11.0 +``` + +## Install with optional components disabled + +```bash +./scripts/install.sh \ + --disable-system-monitoring \ + --disable-spark \ + --disable-mpi \ + --disable-model-monitoring +``` + +## Install with OpenTelemetry + +```bash +# Basic metrics pipeline (operator + collector), no auto-instrumentation +./scripts/install.sh --enable-otel collector + +# Everything, including namespace-wide auto-instrumentation +./scripts/install.sh --enable-otel full +# same as: +./scripts/install.sh --enable-otel + +# Or pick individual knobs directly +./scripts/install.sh --enable-otel-operator --enable-otel-collector +``` + +`--enable-otel-namespace-label` has a namespace-wide blast radius (it auto-instruments +every Python pod in the release namespace once `--enable-otel-instrumentation` is also +on) — `full` mode includes it, `collector` mode doesn't. + +## Install with ingress + +Enables the chart's own Ingress resources (UI, API, Jupyter, Nuclio dashboard) for HTTP +hostname routing. **This installer does not install an ingress controller** — bring your +own (e.g. [ingress-nginx](https://kubernetes.github.io/ingress-nginx/)) and have it +already running in the cluster first. If no IngressClass matching `--enable-ingress`'s +class (default `nginx`) is found, the pre-install validators warn but the install still +proceeds — the Ingress resources are created either way, they just won't resolve until a +controller providing that class exists. + +```bash +./scripts/install.sh --enable-ingress + +# Use a custom ingress class name (must match a controller already in the cluster) +./scripts/install.sh --enable-ingress myclass +``` + +## Install with local registry + +Deploys a `registry:2` container inside the cluster as a ClusterIP service and wires +MLRun CE to use it. + +Without `--enable-ingress`, the registry is reachable only in-cluster via its Kubernetes +service DNS name (`local-registry..svc.cluster.local:5000`): + +```bash +./scripts/install.sh --local-registry +``` + +## Install with ingress + local registry + +Combines both flags: an Ingress route is created for the local registry at +`registry.`, on top of whatever ingress controller you already have running in the +cluster (see "Install with ingress" above — this installer never installs one). This +makes the registry reachable from the same hostname both inside pods and from your +terminal. + +```bash +./scripts/install.sh --enable-ingress --local-registry +``` + +Docker Desktop TLS/hosts-file setup for this combo is covered in [faq.md](faq.md). + +## Pre-install validators + +Before every real (or dry-run) install, the installer runs a set of read-only pre-flight +checks against the target cluster: + +- **Blocking** (exit 1, no `helm` call is made): Helm CLI version >= 3.6, a default + StorageClass exists. +- **Warning only** (logged, install continues): the cluster's Kubernetes version (reported + always, and compared only against an explicitly set `MIN_K8S_VERSION`), registry login with the resolved + credentials (skipped for `--local-registry` or when no credentials were resolved yet, + e.g. `-f`-only mode), an IngressClass matching `--enable-ingress`'s class exists + (skipped when `--enable-ingress` isn't used — this installer never installs a + controller itself), the chart's fixed NodePorts + (`30010/20/40/50/60/70`, `30093/94`, `30100`, `30110`) already in use by another + Service outside the target namespace, and total cluster node capacity below the + documented floor (8Gi allocatable RAM / 8Gi allocatable ephemeral storage). + +All checks run and report together — a blocking failure doesn't stop the others from +running, so you see every problem in one pass. Skip the whole dispatcher with +`--skip-validators` / `SKIP_VALIDATORS=true` if you need to proceed anyway: + +```bash +./scripts/install.sh --skip-validators +``` + +### Version floors + +The Helm >= 3.6 floor mirrors the prerequisite in +[the chart README](../../charts/mlrun-ce/README.md#prerequisites), so the installer never +refuses a Helm version the chart itself supports. + +**There is no Kubernetes floor.** The chart declares no `kubeVersion` in `Chart.yaml` and the +README states no cluster version, so the installer has nothing to enforce and doesn't invent +one — it reports the version it finds and moves on. + +Both are overridable, which is mainly useful for tightening rather than loosening. Set +`MIN_K8S_VERSION` to get a warning on clusters below a version you care about, and raise +`MIN_HELM_VERSION` to hard-require a newer Helm: + +```bash +MIN_K8S_VERSION=1.34 MIN_HELM_VERSION=4.1 ./scripts/install.sh --chart-path ./charts/mlrun-ce --dry-run +``` + +`MIN_K8S_VERSION` only ever warns; it never blocks the install. Only `MIN_HELM_VERSION` is +enforced as a hard floor. + +--- + +## Config file (`ce-config.yaml`) + +`--config FILE` (or `CONFIG_FILE` env) reads a single reserved `installer:` block from a +YAML file. Every key resolves to the same flag/env var above it, following the usual +precedence: **flag > env var > `ce-config.yaml` > default**. In interactive mode, a value +found in the config file becomes the prompt's default (press Enter to accept it); in +`--non-interactive`/CI mode it's used directly with no prompt. + +```yaml +installer: + kubeContext: "" # optional; blank = current local kubeconfig context + externalHostAddress: auto # "auto" = existing autodetect (minikube/docker-desktop/node IP); or pin a value + registry: + url: index.docker.io/myuser # used if REGISTRY_URL isn't set on the CLI/env + secret: + username: myuser # used if REGISTRY_USERNAME isn't set + server: https://index.docker.io/v1/ + email: me@example.com + # password: never put this here — set REGISTRY_PASSWORD (env), REGISTRY_PASSWORD_FILE + # (path to a file containing just the password), or use the interactive + # masked prompt. A password key here is detected and ignored with a warning. + chartSource: + kind: repo # repo | path + chartVersion: 0.11.0 # repo mode -> --ce-version + chartPath: "" # path mode -> --chart-path; REQUIRED (and validated) when kind: path + versions: + mlrun: "" # -> --set mlrun.{api,ui}.image.tag, mlrun.api.sidecars.logCollector.image.tag + nuclio: "" # -> --set nuclio.{controller,dashboard}.image.tag + components: # mirrors --disable-system-monitoring/-spark/-mpi/-model-monitoring + monitoring: true # false -> same --set as --disable-system-monitoring + spark: true # false -> same --set as --disable-spark + mpi: true # false -> same --set as --disable-mpi + modelMonitoring: true # false -> same --set as --disable-model-monitoring + otel: # all 4 ship OFF in the chart, so each key opts IN (mirrors + # --enable-otel-*/ENABLE_OTEL_*) — independent knobs, not one + # bundled toggle, so you can enable e.g. just operator+collector + # without namespaceLabel/instrumentation's namespace-wide effect. + operator: false # -> same --set as --enable-otel-operator + collector: false # -> same --set as --enable-otel-collector + namespaceLabel: false # -> same --set as --enable-otel-namespace-label + instrumentation: false # -> same --set as --enable-otel-instrumentation +``` + +### Required fields + +`load_config` exits 1 with every missing field listed, rather than one at a time: + +| Config key | Mirrors today's | Required unless | +|---------------------------------------|----------------------|---------------------------------------------| +| `installer.registry.url` | `REGISTRY_URL` | `--local-registry` is used | +| `installer.registry.secret.username` | `REGISTRY_USERNAME` | — | +| `installer.registry.secret.password` | `REGISTRY_PASSWORD` | — (**never put this in the file** — `REGISTRY_PASSWORD`/`REGISTRY_PASSWORD_FILE` env, or the interactive prompt, only) | +| `installer.chartSource.chartPath` | `CHART_PATH` | `installer.chartSource.kind` isn't `path` (checked immediately, in every mode — there's no prompt fallback for this one) | + +The username/url/password checks above only hard-fail in `--non-interactive` mode (there's +no prompt to fall back on there); interactively, a missing value just falls through to the +existing prompt. + +### Optional fields + +Have working defaults: + +| Config key | Default / behavior when omitted | +|----------------------------------------|-------------------------------------------------------| +| `installer.registry.secret.server` | `https://index.docker.io/v1/` | +| `installer.registry.secret.email` | unset | +| `installer.chartSource.kind` | `repo` (published chart) | +| `installer.chartSource.chartVersion` | latest | +| `installer.kubeContext` | current local kubeconfig context | +| `installer.externalHostAddress` | `auto` — existing autodetect (minikube/docker-desktop/node IP) | +| `installer.versions.mlrun` | chart default (no override) | +| `installer.versions.nuclio` | chart default (no override) | +| `installer.components.monitoring` | `true` — enabled | +| `installer.components.spark` | `true` — enabled | +| `installer.components.mpi` | `true` — enabled | +| `installer.components.modelMonitoring` | `true` — enabled | +| `installer.otel.operator` | `false` — disabled (chart default) | +| `installer.otel.collector` | `false` — disabled (chart default) | +| `installer.otel.namespaceLabel` | `false` — disabled (chart default) | +| `installer.otel.instrumentation` | `false` — disabled (chart default) | + +`installer.components.{monitoring,spark,mpi,modelMonitoring}` mirror the existing +`--disable-*` **flags** exactly (`false` maps to the same `--set` as the matching flag; +a flag/env-set `DISABLE_*` is never un-set by the file — there's no "explicitly +re-enable" flag to begin with, so the file can only ever add a disable, never remove +one). `installer.otel.*` is the reverse: the chart ships all 4 OpenTelemetry values +**disabled** by default, so `true` on any of these keys mirrors the matching +`--enable-otel-*`/`ENABLE_OTEL_*` flag instead — opt-IN, not opt-out — and a +flag/env-set `ENABLE_OTEL_*=true` is never un-set by the file the same way. The 4 are +independent (not one bundled toggle): `operator` installs the operator subchart +(CRDs/webhook/manager); `collector` deploys the Collector (receives OTLP, exports to +Prometheus); `instrumentation` creates the Instrumentation CR (propagators, sampler, +Python/Java agent images); `namespaceLabel` labels/annotates the release namespace so +every Python pod in it is auto-instrumented once `instrumentation` is also on — this +one has a namespace-wide blast radius, review before enabling. `--enable-otel [MODE]` +(`off`/`collector`/`full`, bare flag = `full`) is a convenience over the 4 granular +flags/vars — it doesn't replace them, and they still work individually alongside it. +`ingress.*` toggles, and anything that triggers real infra beyond a `--set` (e.g. +`--local-registry` deploying a registry), are **not** implemented — use the existing +`--enable-ingress`/`--local-registry` flags for those. (`--enable-ingress` itself is now +just chart `--set`s plus a warning check — it doesn't install an ingress controller.) + +### Combining `--config` with `-f`/`--values` + +`--config` and `-f`/`--values` can be used together. `-f` supplies the base values file; +`ce-config.yaml`'s curated fields resolve to `--set` flags exactly as they do when +`--config` is used alone. Nothing extra to configure — helm always applies `--set` after +`--values`, so a config-resolved field wins over the same key in a `-f` file with no merge +logic needed. + +Used alone (no `--config`), `-f` keeps its original, fully self-contained behavior: you +supply `global.registry.*`, image tags, everything yourself, `install.sh` sets nothing on +top, and it skips creating the registry secret entirely (the secret named in your values +file must already exist). Add `--config` and that changes: secret creation and registry/ +host resolution run exactly as they do in `--config`-only mode, using the config file's +(or flag/env's) registry fields — so `-f` no longer needs to carry the registry secret +itself when `--config` is supplying it. + +### Precedence + +The full waterfall, highest wins: + +**CLI flag > environment variable > `ce-config.yaml` (`installer:` block, resolved to `--set`) > `-f`/`--values` file (raw values) > chart defaults** + +Concretely: for any key the curated `installer:` schema covers (registry, chartSource, +versions, components, otel), a flag, env var, or `ce-config.yaml` value always overrides +the same key in a `-f` file or the chart's own default — because it's applied as `--set`, +and helm applies `--set` after `--values`. For anything **outside** that curated schema +(arbitrary chart values — resource limits, replica counts, etc.), `-f` is authoritative; +nothing in `install.sh` touches those keys. + +Requires `yq` — but only when `--config`/`CONFIG_FILE` is actually used; installs that +don't use a config file have no new dependency. + +```bash +./scripts/install.sh --config scripts/ce-config.yaml.example --dry-run + +# Combined with a values file — config's registry/chart-source/component fields still +# resolve as --set, layered on top of my-values.yaml: +./scripts/install.sh --config scripts/ce-config.yaml.example -f my-values.yaml --dry-run +``` diff --git a/scripts/docs/faq.md b/scripts/docs/faq.md new file mode 100644 index 00000000..b9c8dd6b --- /dev/null +++ b/scripts/docs/faq.md @@ -0,0 +1,143 @@ +# FAQ / Known gotchas + +Things that look like bugs but aren't, plus a few setup gotchas worth knowing about +before you hit them. + +--- + +### The registry secret doesn't exist / install fails with a `helm --wait` timeout on image pull + +`--skip-secret` means "use an existing secret, don't create one" — `install.sh` verifies +that secret actually exists in the target namespace and exits 1 immediately with a clear +message if it doesn't, rather than letting the install proceed and fail later with an +opaque `helm --wait` timeout once pods can't pull images. + +With `-f`/`--values`, there's no equivalent check today — the secret your values file +references must already exist, or the install will similarly time out once pods try to +pull images using a nonexistent `imagePullSecrets` entry. + +### `--dry-run` fails on `PrometheusRule`/`ServiceMonitor` validation + +`--dry-run` uses `helm --dry-run=server`, which validates against the live API server. If +the target cluster lacks the Prometheus Operator CRDs, the `kube-prometheus-stack` +subchart's `PrometheusRule`/`ServiceMonitor` resources fail server-side validation. This +is a Helm limitation (charts with CRDs can't fully dry-run without those CRDs present), +not an `install.sh` bug. + +### Can I install two `mlrun-ce` releases on one cluster? + +No, even in different namespaces with different release/secret names and NodePort +overrides via `-f`. The chart's `workflow-controller` `PriorityClass` is cluster-scoped +with a hardcoded name (no values.yaml knob), so a second release's `helm install` fails +immediately with an ownership-metadata error once one release already owns it. Not an +`install.sh` bug — the chart itself has no multi-release story on a shared cluster short +of patching that template. + +### `--hard-clean` didn't delete everything — a Kafka pod and its PVC are still there + +`helm uninstall` (and `--hard-clean`) can leave orphaned Strimzi `Kafka`/ +`KafkaNodePool`/`StrimziPodSet` custom resources and their broker pod behind: once the +`strimzi-kafka-operator` Deployment is gone, nothing reconciles those CRs, so the broker +pod keeps running and its PVC's `kubernetes.io/pvc-protection` finalizer blocks +`--hard-clean`'s PVC deletion indefinitely. + +Fix is manual: delete the `strimzipodset` and pod directly (releases the finalizer), then +the `kafka`/`kafkanodepool` CRs: + +```bash +kubectl delete strimzipodset --all -n mlrun +kubectl delete pod -l strimzi.io/cluster -n mlrun --force --grace-period=0 +kubectl delete kafka,kafkanodepool --all -n mlrun +``` + +Not something `do_hard_clean` can anticipate from `install.sh` alone — it's a +chart/Strimzi ordering issue. + +### What address does the installer suggest for `EXTERNAL_HOST_ADDRESS`, and why? + +`resolve_external_host()` picks a suggested default in this order (always just a +default — override with `EXTERNAL_HOST_ADDRESS`/`installer.externalHostAddress` any time +it's wrong for your cluster): + +1. **`KUBE_CONTEXT` is set** → the target cluster's node internal IP (via `kubectl get + node`). The minikube/docker-desktop heuristics below are statements about the *local + machine's own* ambient environment and are meaningless once a specific — possibly + remote — context is explicitly selected; `kubectl config current-context` also can't + be made `--context`-aware (it always reports the kubeconfig's ambient current-context + regardless of `--context`), so this case is handled separately. This is the right + default for e.g. a lab cluster reachable directly on the corporate network via a + named context — even if `kubectl`/`helm` reach the API server itself through an SSH + tunnel (only the API server port is tunneled in that setup; NodePort services aren't, + so `localhost` would resolve to nothing there). +2. **minikube is installed and has an IP** → that IP. +3. **Current context matches `docker-desktop`** → `host.docker.internal` (resolves to the + host from both pods and your terminal on Docker Desktop). +4. **None of the above** (e.g. kind, k3d, or another local cluster type) → `localhost`. + These tools typically NodePort-map to `localhost` rather than an internal + Docker-network IP, so it's a better generic guess than a node-IP lookup that's often + unreachable from the host. + +### Docker Desktop: pushing to `--local-registry --enable-ingress` fails with a TLS error + +The local registry serves plain HTTP through your ingress controller (port 80). Docker +and kaniko default to HTTPS for any non-`localhost` registry, which causes a TLS error on +port 443. Two things need configuring: + +**1. Docker CLI on your machine** + +1. Open **Docker Desktop → Settings → Docker Engine** +2. Add `registry.host.docker.internal` to `insecure-registries`: + +```json +{ + "insecure-registries": ["registry.host.docker.internal"] +} +``` + +3. Click **Apply & Restart** + +After that you can push images from your machine: + +```bash +docker tag myimage registry.host.docker.internal/myimage +docker push registry.host.docker.internal/myimage +``` + +**2. Kaniko inside the cluster** + +The installer automatically passes `mlrun.api.kaniko.insecureRegistry=true` to the Helm +chart when `--local-registry --enable-ingress` are both set, so MLRun build jobs use HTTP +when pushing to the local registry. No manual action needed. + +For reference, once installed, the registry is reachable at +`registry.host.docker.internal` from everywhere: + +```bash +# From your terminal +curl http://registry.host.docker.internal/v2/_catalog + +# From inside a pod +kubectl run test --rm -it --image=curlimages/curl --restart=Never \ + -n mlrun -- curl http://registry.host.docker.internal/v2/_catalog +``` + +### `--enable-ingress` is set but the Ingress URLs don't resolve + +This installer never installs an ingress controller for you — `--enable-ingress` only +flips the chart's own Ingress resources on via `--set`. If no IngressClass matching your +`--enable-ingress`'s class (default `nginx`) exists in the cluster, the pre-install +validators warn about this before the install even starts, but the install proceeds +anyway (the Ingress resources get created either way). Install a controller providing +that class — e.g. [ingress-nginx](https://kubernetes.github.io/ingress-nginx/) — and the +existing Ingress resources will start resolving with no re-install needed. + +### Deleting everything, including the namespace + +`--uninstall --hard-clean` runs `helm uninstall` and then deletes every PVC in the +namespace and every PV bound to it. **This is irreversible and will cause data loss.** +The namespace and CRDs themselves are not deleted by `install.sh`; remove them yourself +if needed: + +```bash +kubectl delete namespace mlrun +``` diff --git a/scripts/docs/parameters.md b/scripts/docs/parameters.md new file mode 100644 index 00000000..fcaea249 --- /dev/null +++ b/scripts/docs/parameters.md @@ -0,0 +1,97 @@ +# Parameters Reference + +Full flag and environment variable reference for `install.sh`. For guided walkthroughs +see the main [README](../README.md); for the `ce-config.yaml` schema and precedence +rules see [configuration.md](configuration.md). + +Every flag has an environment-variable equivalent (for CI / non-interactive use), and +**flag > env var > `ce-config.yaml` > built-in default** everywhere. + +--- + +## Flags + +``` +Usage: install.sh [options] + +Options: + -h, --help Show help + --uninstall Uninstall the MLRun CE Helm release + --hard-clean Use with --uninstall: delete all PVCs and PVs (data loss!) + --skip-secret Skip creating the Docker registry secret + --skip-validators Skip the pre-install validators (K8s/Helm version, + StorageClass, registry auth, NodePort conflicts, node capacity) + -f, --values FILE Use this YAML values file as the base for installation. + Alone (no --config): fully self-contained, skips secret + creation and prompts. Combined with --config: --config still + creates the secret and resolves as --set overrides that win + over this file. See configuration.md's "Precedence" section. + --show-progress Live deployment progress UI during install + --disable-system-monitoring Disable Grafana/Prometheus stack + --disable-spark Disable Spark operator + --disable-mpi Disable MPI operator resources + --disable-model-monitoring Disable Kafka + TimescaleDB components + --enable-ingress [CLASS] Enable the chart's Ingress resources (class defaults to "nginx"). + Requires a controller already in the cluster — not installed for you. + --enable-otel [MODE] off|collector|full (default when bare: full). collector = operator + +collector only; full = all 4 below. Granular flags still work too. + --enable-otel-operator Install the OpenTelemetry Operator (CRDs/webhook/manager) + --enable-otel-collector Deploy the OpenTelemetry Collector (OTLP -> Prometheus) + --enable-otel-namespace-label Auto-instrument every Python pod in the namespace + --enable-otel-instrumentation Create the Instrumentation CR + --local-registry Deploy a local registry:2 registry inside the cluster + --chart-path DIR Install from a local chart directory instead of the published repo; + use ./charts/mlrun-ce for this repo's chart (runs helm dependency + update on the path first) + --ce-version VERSION Pin the MLRun CE Helm chart version (default: latest; ignored with --chart-path) + --dry-run Render the chart without deploying (helm --dry-run=server) + --non-interactive Never prompt; fail with exit 1 if a required value is missing + (auto-set when CI=true) + --config FILE Read defaults from a ce-config.yaml file's 'installer:' block + (requires yq; flag/env values always win over the file). + Can be combined with -f/--values — see configuration.md's "Precedence". +``` + +--- + +## Environment Variables + +| Variable | Default | Description | +|------------------------|-----------------------------------|----------------------------------------------------------| +| `NAMESPACE` | `mlrun` | Kubernetes namespace | +| `RELEASE_NAME` | `mlrun-ce` | Helm release name | +| `REGISTRY_SECRET_NAME` | `registry-credentials` | Name of the K8s Docker registry secret | +| `HELM_REPO_URL` | `https://mlrun.github.io/ce` | Helm chart repository URL | +| `REGISTRY_USERNAME` | — | Docker registry username | +| `REGISTRY_PASSWORD` | — | Docker registry password (never read from ce-config.yaml) | +| `REGISTRY_PASSWORD_FILE` | — | Path to a file containing just the password (never read from ce-config.yaml; `REGISTRY_PASSWORD` wins if both are set) | +| `REGISTRY_SERVER` | `https://index.docker.io/v1/` | Docker server URL | +| `REGISTRY_EMAIL` | — | Docker registry email | +| `REGISTRY_URL` | — | Registry URL for images (e.g. `index.docker.io/myuser`) | +| `EXTERNAL_HOST_ADDRESS`| — | Host address the cluster is reachable at (see [FAQ](faq.md) for the autodetect fallback chain) | +| `SKIP_REGISTRY_SECRET` | `false` | Set to `true` to skip secret creation | +| `SKIP_VALIDATORS` | `false` | Set to `true` to skip the pre-install validators | +| `MIN_K8S_VERSION` | — (no floor) | Kubernetes version to warn below (`MAJOR.MINOR`); never blocks the install | +| `MIN_HELM_VERSION` | `3.6` | Helm CLI version floor the blocking validator enforces (`MAJOR.MINOR`) | +| `DISABLE_SYSTEM_MONITORING` | `false` | Set to `true` to disable the Grafana/Prometheus stack | +| `DISABLE_SPARK` | `false` | Set to `true` to disable the Spark operator | +| `DISABLE_MPI` | `false` | Set to `true` to disable MPI operator resources | +| `DISABLE_MODEL_MONITORING` | `false` | Set to `true` to disable Kafka + TimescaleDB components | +| `SHOW_PROGRESS` | `false` | Set to `true` for live progress UI | +| `PROGRESS_INTERVAL_SEC`| `10` | Refresh interval (seconds) for progress UI | +| `ENABLE_INGRESS` | `false` | Set to `true` to enable the chart's Ingress resources (requires your own controller) | +| `INGRESS_CLASS` | `nginx` | Ingress class name | +| `ENABLE_OTEL_OPERATOR` | `false` | Set to `true` for `--set opentelemetry-operator.enabled=true` | +| `ENABLE_OTEL_COLLECTOR` | `false` | Set to `true` for `--set opentelemetry.collector.enabled=true` | +| `ENABLE_OTEL_NAMESPACE_LABEL` | `false` | Set to `true` for `--set opentelemetry.namespaceLabel.enabled=true` | +| `ENABLE_OTEL_INSTRUMENTATION` | `false` | Set to `true` for `--set opentelemetry.instrumentation.enabled=true` | +| `LOCAL_REGISTRY` | `false` | Set to `true` to deploy a local `registry:2` registry | +| `CHART_PATH` | — | Path to a local chart directory, e.g. `./charts/mlrun-ce` | +| `CE_VERSION` | — | Pin the chart version (ignored when `CHART_PATH` is set)| +| `DRY_RUN` | `false` | Set to `true` to render the chart without deploying | +| `NON_INTERACTIVE` | `false` | Set to `true` to suppress all prompts | +| `CI` | — | Set to `true` to auto-enable non-interactive mode | +| `CONFIG_FILE` | — | Path to a `ce-config.yaml` file (same as `--config`) | +| `KUBE_CONTEXT` | — | kubectl/helm context to use (default: current kubeconfig context) | +| `MLRUN_VERSION` | — | Pin mlrun api/ui image tag | +| `NUCLIO_VERSION` | — | Pin nuclio controller/dashboard image tag | diff --git a/scripts/install.sh b/scripts/install.sh new file mode 100755 index 00000000..d59d16d7 --- /dev/null +++ b/scripts/install.sh @@ -0,0 +1,1417 @@ +#!/usr/bin/env bash +# Copyright 2025 Iguazio +# +# 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. +# +# Interactive installer for MLRun CE Helm chart on local Kubernetes. +# Creates a Docker registry secret from your credentials, then installs the +# chart with your local URL and registry URL. +# +# Run as a command (no download, no ./ or sh): +# curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh | bash +# +# Or install as a named command, then run from any directory: +# curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh -o /usr/local/bin/mlrun-install && chmod +x /usr/local/bin/mlrun-install +# mlrun-install +# +# From a clone of this repo (installs the published chart): +# ./scripts/install.sh +# +# From a clone, installing this repo's own chart on the current branch: +# ./scripts/install.sh --chart-path ./charts/mlrun-ce +# +# Non-interactive (CI): set REGISTRY_* and EXTERNAL_HOST_ADDRESS, REGISTRY_URL; see -h. +# Requirements: helm, kubectl (configured with a cluster), docker (installed and configured) + +set -o errexit +set -o nounset +set -o pipefail + +NAMESPACE="${NAMESPACE:-mlrun}" +RELEASE_NAME="${RELEASE_NAME:-mlrun-ce}" +REGISTRY_SECRET_NAME="${REGISTRY_SECRET_NAME:-registry-credentials}" +HELM_REPO_URL="${HELM_REPO_URL:-https://mlrun.github.io/ce}" +SKIP_REGISTRY_SECRET="${SKIP_REGISTRY_SECRET:-false}" +SKIP_VALIDATORS="${SKIP_VALIDATORS:-false}" +REGISTRY_PASSWORD_FILE="${REGISTRY_PASSWORD_FILE:-}" +VALUES_FILE="" +SHOW_PROGRESS="${SHOW_PROGRESS:-false}" +UNINSTALL="${UNINSTALL:-false}" +HARD_CLEAN="${HARD_CLEAN:-false}" +DISABLE_SYSTEM_MONITORING="${DISABLE_SYSTEM_MONITORING:-false}" +DISABLE_SPARK="${DISABLE_SPARK:-false}" +DISABLE_MPI="${DISABLE_MPI:-false}" +DISABLE_MODEL_MONITORING="${DISABLE_MODEL_MONITORING:-false}" +ENABLE_INGRESS="${ENABLE_INGRESS:-false}" +INGRESS_CLASS="${INGRESS_CLASS:-nginx}" +ENABLE_OTEL_OPERATOR="${ENABLE_OTEL_OPERATOR:-false}" +ENABLE_OTEL_COLLECTOR="${ENABLE_OTEL_COLLECTOR:-false}" +ENABLE_OTEL_NAMESPACE_LABEL="${ENABLE_OTEL_NAMESPACE_LABEL:-false}" +ENABLE_OTEL_INSTRUMENTATION="${ENABLE_OTEL_INSTRUMENTATION:-false}" +LOCAL_REGISTRY="${LOCAL_REGISTRY:-false}" +LOCAL_REGISTRY_URL="" +EXTERNAL_HOST_ADDRESS="${EXTERNAL_HOST_ADDRESS:-}" +CE_VERSION="${CE_VERSION:-}" +CHART_PATH="${CHART_PATH:-}" +DRY_RUN="${DRY_RUN:-false}" +NON_INTERACTIVE="${NON_INTERACTIVE:-false}" +CONFIG_FILE="${CONFIG_FILE:-}" +KUBE_CONTEXT="${KUBE_CONTEXT:-}" +MLRUN_VERSION="${MLRUN_VERSION:-}" +NUCLIO_VERSION="${NUCLIO_VERSION:-}" + +# Colors for output (use $'...' so escape sequences are actual bytes, not literal \033) +RED=$'\033[0;31m' +GREEN=$'\033[0;32m' +YELLOW=$'\033[1;33m' +NC=$'\033[0m' + +log_info() { printf '%s\n' "${GREEN}[INFO]${NC} $1"; } +log_warn() { printf '%s\n' "${YELLOW}[WARN]${NC} $1"; } +log_error() { printf '%s\n' "${RED}[ERROR]${NC} $1" >&2; } + +# Wrap kubectl/helm so every call in this script honors KUBE_CONTEXT (installer.kubeContext) +# without threading --context/--kube-context through every call site individually. +kubectl() { command kubectl ${KUBE_CONTEXT:+--context "${KUBE_CONTEXT}"} "$@"; } +helm() { command helm ${KUBE_CONTEXT:+--kube-context "${KUBE_CONTEXT}"} "$@"; } + +usage() { + cat < Prometheus) + --enable-otel-namespace-label --set opentelemetry.namespaceLabel.enabled=true (labels NAMESPACE; auto-instruments + every Python pod in it once the operator+instrumentation are also on) + --enable-otel-instrumentation --set opentelemetry.instrumentation.enabled=true (creates the Instrumentation CR) + --local-registry Deploy a local registry:2 registry inside the cluster + --ce-version [VERSION] Pin a specific mlrun-ce chart version (default: latest) + --chart-path DIR Install from a local chart directory instead of the published repo — + use ./charts/mlrun-ce for this repo's own chart, on whatever branch is + checked out. Runs helm dependency update on the path first. + --dry-run Render the helm chart without deploying (passes --dry-run=server to helm) + --non-interactive Never prompt; fail with exit 1 if a required value is missing (auto-set when CI=true) + --config FILE Read defaults from a ce-config.yaml file's 'installer:' block (requires yq). + Flag/env values always win over the file; see docs/configuration.md for the schema. + Can be combined with -f/--values — see docs/configuration.md's "Precedence" section. +Environment variables (for non-interactive / CI): + SKIP_REGISTRY_SECRET Set to 'true' to skip creating the registry secret + SKIP_VALIDATORS Set to 'true' to skip the pre-install validators + MIN_K8S_VERSION Kubernetes version to warn below (unset by default; never blocks) + MIN_HELM_VERSION Helm CLI version floor the validators enforce (default: 3.6) + DISABLE_SYSTEM_MONITORING Set to 'true' to disable the Grafana/Prometheus stack + DISABLE_SPARK Set to 'true' to disable the Spark operator + DISABLE_MPI Set to 'true' to disable MPI operator resources + DISABLE_MODEL_MONITORING Set to 'true' to disable Kafka + TimescaleDB components + REGISTRY_USERNAME Docker registry username + REGISTRY_PASSWORD Docker registry password (never read from ce-config.yaml) + REGISTRY_PASSWORD_FILE Path to a file containing just the password (never read from ce-config.yaml; + REGISTRY_PASSWORD wins if both are set) + REGISTRY_SERVER Docker server URL (e.g. https://index.docker.io/v1/) + REGISTRY_EMAIL Docker registry email + REGISTRY_URL Registry URL for chart (e.g. index.docker.io/) + REGISTRY_SECRET_NAME Secret name (default: registry-credentials) + EXTERNAL_HOST_ADDRESS Local URL to reach the cluster (e.g. localhost or minikube ip) + RELEASE_NAME Helm release name (default: mlrun-ce) + NAMESPACE Kubernetes namespace (default: mlrun) + SHOW_PROGRESS Set to 'true' to show pod progress during install + ENABLE_INGRESS Set to 'true' to enable the chart's Ingress resources for + UI/API/Jupyter/Nuclio (requires an ingress controller already in the + cluster — this installer does not install one for you) + INGRESS_CLASS Ingress class name (default: nginx) + ENABLE_OTEL_OPERATOR Set to 'true' for --set opentelemetry-operator.enabled=true + ENABLE_OTEL_COLLECTOR Set to 'true' for --set opentelemetry.collector.enabled=true + ENABLE_OTEL_NAMESPACE_LABEL Set to 'true' for --set opentelemetry.namespaceLabel.enabled=true + ENABLE_OTEL_INSTRUMENTATION Set to 'true' for --set opentelemetry.instrumentation.enabled=true + LOCAL_REGISTRY Set to 'true' to deploy a local registry:2 registry + CE_VERSION Helm chart version (default: latest; ignored when CHART_PATH is set) + CHART_PATH Path to a local chart directory, e.g. ./charts/mlrun-ce (disables + published-repo mode) + DRY_RUN Set to 'true' to render the chart without deploying + NON_INTERACTIVE Set to 'true' to suppress all prompts (auto-set when CI=true) + CI Set to 'true' to auto-enable non-interactive mode + CONFIG_FILE Path to a ce-config.yaml file (same as --config) + KUBE_CONTEXT kubectl/helm context to use (default: current kubeconfig context) + MLRUN_VERSION Pin mlrun api/ui image tag (--set mlrun.{api,ui}.image.tag=...) + NUCLIO_VERSION Pin nuclio controller/dashboard image tag (--set nuclio.{controller,dashboard}.image.tag=...) +EOF +} + +check_requirements() { + log_info "Checking prerequisites (helm, kubectl, docker)..." + + # Helm installed (type -P bypasses the kubectl/helm wrapper functions defined above, + # so this checks for the real binary rather than always finding our own wrapper) + if ! type -P helm &> /dev/null; then + log_error "Helm is not installed or not in PATH." + log_info "Install Helm: https://helm.sh/docs/intro/install/" + exit 1 + fi + log_info " Helm: $(helm version --short 2>/dev/null || helm version 2>/dev/null | head -1)" + + # kubectl installed + if ! type -P kubectl &> /dev/null; then + log_error "kubectl is not installed or not in PATH." + log_info "Install kubectl: https://kubernetes.io/docs/tasks/tools/" + exit 1 + fi + + # kubectl configured and connected to a cluster + if ! kubectl cluster-info &> /dev/null; then + log_error "kubectl is not configured or cannot reach a Kubernetes cluster." + log_info "Configure kubeconfig (e.g. set KUBECONFIG or run your cluster's setup)." + log_info "See: https://kubernetes.io/docs/concepts/configuration/organize-cluster-access-kubeconfig/" + exit 1 + fi + log_info " kubectl: connected to cluster" + + # Docker installed + if ! command -v docker &> /dev/null; then + log_error "Docker is not installed or not in PATH." + log_info "Install Docker: https://docs.docker.com/get-docker/" + exit 1 + fi + + # Docker daemon running and configured + if ! docker info &> /dev/null; then + log_error "Docker daemon is not running or not accessible (e.g. permission or not in docker group)." + log_info "Start Docker and ensure your user can run 'docker info'." + log_info "See: https://docs.docker.com/config/daemon/" + exit 1 + fi + log_info " Docker: installed and configured" + + log_info "All prerequisites met." +} + +ensure_namespace() { + if kubectl get namespace "${NAMESPACE}" &> /dev/null; then + log_info "Namespace '${NAMESPACE}' already exists" + elif [[ "${DRY_RUN}" == "true" ]]; then + log_info "Dry-run: would create namespace '${NAMESPACE}'" + else + log_info "Creating namespace '${NAMESPACE}'..." + kubectl create namespace "${NAMESPACE}" + fi +} + +patch_coredns_for_registry() { + local registry_host="$1" + local ingress_clusterip + ingress_clusterip="$(kubectl get svc ingress-nginx-controller \ + --namespace "${NAMESPACE}" \ + -o jsonpath='{.spec.clusterIP}' 2>/dev/null || true)" + + if [[ -z "${ingress_clusterip}" ]]; then + log_warn "Could not get ingress controller ClusterIP; skipping CoreDNS patch." + log_warn "Pods may not resolve ${registry_host} — add a hosts entry manually if needed." + return + fi + + local corefile + corefile="$(kubectl get configmap coredns -n kube-system -o jsonpath='{.data.Corefile}' 2>/dev/null || true)" + + if grep -qF "${registry_host}" <<< "${corefile}"; then + log_info "CoreDNS already has an entry for ${registry_host}; skipping patch." + return + fi + + # CoreDNS only allows one hosts{} block per server. If one already exists (e.g. Docker Desktop + # adds host.docker.internal), insert our entry inside it before 'fallthrough'. Otherwise + # create a new hosts{} block before the first 'forward' line. + local patched_corefile + if grep -q 'hosts {' <<< "${corefile}"; then + patched_corefile="$(awk -v ip="${ingress_clusterip}" -v host="${registry_host}" ' + /hosts \{/ { in_hosts=1 } + in_hosts && /^[[:space:]]*}/ { + if (!inserted) { print " " ip " " host; inserted=1 } + in_hosts=0 + } + !inserted && in_hosts && /fallthrough/ { + print " " ip " " host + inserted=1 + } + { print } + ' <<< "${corefile}")" + else + patched_corefile="$(awk -v ip="${ingress_clusterip}" -v host="${registry_host}" ' + !inserted && /forward / { + print " hosts {" + print " " ip " " host + print " fallthrough" + print " }" + inserted=1 + } + { print } + ' <<< "${corefile}")" + fi + + kubectl create configmap coredns \ + --from-literal="Corefile=${patched_corefile}" \ + --namespace kube-system \ + --dry-run=client -o yaml | kubectl apply -f - + + kubectl rollout restart deployment/coredns --namespace kube-system + kubectl rollout status deployment/coredns --namespace kube-system --timeout=60s + + log_info "CoreDNS patched: ${registry_host} → ${ingress_clusterip}" +} + +deploy_local_registry() { + # Resolve URL early so create_registry_secret (called before gather_install_params) has it + if [[ "${ENABLE_INGRESS}" == "true" ]]; then + LOCAL_REGISTRY_URL="registry.${EXTERNAL_HOST_ADDRESS}" + else + LOCAL_REGISTRY_URL="local-registry.${NAMESPACE}.svc.cluster.local:5000" + fi + + log_info "Deploying local Docker registry..." + kubectl apply -f - --namespace "${NAMESPACE}" <&2 + else + read -r -p "${prompt_msg}${default_prompt}: " value + fi + echo "${value:-$default}" +} + +# Reads the reserved 'installer:' block from a ce-config.yaml (--config/CONFIG_FILE) and +# fills in vars that flag/env didn't already set. Never touches helm values directly — +# every key here just becomes a default for existing flags/env/prompts, so precedence stays +# flag > env > config file > built-in default everywhere. +load_config() { + CONFIG_REGISTRY_URL="" + CONFIG_REGISTRY_USERNAME="" + CONFIG_REGISTRY_SERVER="" + CONFIG_REGISTRY_EMAIL="" + CONFIG_EXTERNAL_HOST_ADDRESS="" + + [[ -z "${CONFIG_FILE}" ]] && return 0 + + if [[ ! -f "${CONFIG_FILE}" ]]; then + log_error "Config file not found: ${CONFIG_FILE}" + exit 1 + fi + + if ! command -v yq &> /dev/null; then + log_error "yq is required to use --config/CONFIG_FILE but is not installed or not in PATH." + log_info "Install yq (pin a version, verify its checksum): https://github.com/mikefarah/yq/releases" + exit 1 + fi + + yq_get() { + local val + val="$(yq eval "$1" "${CONFIG_FILE}" 2>/dev/null || true)" + [[ "$val" == "null" ]] && val="" + printf '%s' "$val" + } + + if [[ -n "$(yq_get '.installer.registry.secret.password')" ]]; then + log_warn "ce-config.yaml has 'installer.registry.secret.password' set — ignored." + log_warn "Passwords are never read from the config file; set REGISTRY_PASSWORD or use the prompt." + fi + + CONFIG_REGISTRY_URL="$(yq_get '.installer.registry.url')" + CONFIG_REGISTRY_USERNAME="$(yq_get '.installer.registry.secret.username')" + CONFIG_REGISTRY_SERVER="$(yq_get '.installer.registry.secret.server')" + CONFIG_REGISTRY_EMAIL="$(yq_get '.installer.registry.secret.email')" + + CONFIG_EXTERNAL_HOST_ADDRESS="$(yq_get '.installer.externalHostAddress')" + [[ "${CONFIG_EXTERNAL_HOST_ADDRESS}" == "auto" ]] && CONFIG_EXTERNAL_HOST_ADDRESS="" + + local cfg_kube_context + cfg_kube_context="$(yq_get '.installer.kubeContext')" + [[ -z "${KUBE_CONTEXT}" && -n "${cfg_kube_context}" ]] && KUBE_CONTEXT="${cfg_kube_context}" + + local cfg_chart_kind cfg_chart_version cfg_chart_path + cfg_chart_kind="$(yq_get '.installer.chartSource.kind')" + cfg_chart_version="$(yq_get '.installer.chartSource.chartVersion')" + cfg_chart_path="$(yq_get '.installer.chartSource.chartPath')" + [[ -z "${CE_VERSION}" && -n "${cfg_chart_version}" ]] && CE_VERSION="${cfg_chart_version}" + [[ -z "${CHART_PATH}" && "${cfg_chart_kind}" == "path" && -n "${cfg_chart_path}" ]] && CHART_PATH="${cfg_chart_path}" + + local cfg_mlrun_version cfg_nuclio_version + cfg_mlrun_version="$(yq_get '.installer.versions.mlrun')" + cfg_nuclio_version="$(yq_get '.installer.versions.nuclio')" + [[ -z "${MLRUN_VERSION}" && -n "${cfg_mlrun_version}" ]] && MLRUN_VERSION="${cfg_mlrun_version}" + [[ -z "${NUCLIO_VERSION}" && -n "${cfg_nuclio_version}" ]] && NUCLIO_VERSION="${cfg_nuclio_version}" + + # components.* mirrors the --disable-* flags exactly: "false" disables, same as passing + # the flag; a flag/env-set DISABLE_* is never un-set by the file (there's no "explicitly + # re-enable" flag to begin with, so config can only ever add a disable, never remove one). + [[ "${DISABLE_SYSTEM_MONITORING}" != "true" && "$(yq_get '.installer.components.monitoring')" == "false" ]] && DISABLE_SYSTEM_MONITORING="true" + [[ "${DISABLE_SPARK}" != "true" && "$(yq_get '.installer.components.spark')" == "false" ]] && DISABLE_SPARK="true" + [[ "${DISABLE_MPI}" != "true" && "$(yq_get '.installer.components.mpi')" == "false" ]] && DISABLE_MPI="true" + [[ "${DISABLE_MODEL_MONITORING}" != "true" && "$(yq_get '.installer.components.modelMonitoring')" == "false" ]] && DISABLE_MODEL_MONITORING="true" + + # otel is its own block (not under components.*): the chart ships all 4 of these + # opentelemetry-operator/opentelemetry.* values disabled by default (unlike the + # components.* above, which default enabled), so each key here opts IN, mirroring + # the --enable-otel-* flags rather than the --disable-* pattern. Independently + # settable so a user can enable e.g. just the operator+collector without the + # namespace-wide auto-instrumentation of namespaceLabel/instrumentation. A + # flag/env-set ENABLE_OTEL_* still always wins — the file can only turn one on, + # same "add, never override" rule as the --disable-* mirrors above. + [[ "${ENABLE_OTEL_OPERATOR}" != "true" && "$(yq_get '.installer.otel.operator')" == "true" ]] && ENABLE_OTEL_OPERATOR="true" + [[ "${ENABLE_OTEL_COLLECTOR}" != "true" && "$(yq_get '.installer.otel.collector')" == "true" ]] && ENABLE_OTEL_COLLECTOR="true" + [[ "${ENABLE_OTEL_NAMESPACE_LABEL}" != "true" && "$(yq_get '.installer.otel.namespaceLabel')" == "true" ]] && ENABLE_OTEL_NAMESPACE_LABEL="true" + [[ "${ENABLE_OTEL_INSTRUMENTATION}" != "true" && "$(yq_get '.installer.otel.instrumentation')" == "true" ]] && ENABLE_OTEL_INSTRUMENTATION="true" + + # chartPath is a pure config-authoring error with no flag/env/prompt fallback, so it's + # checked unconditionally (not just in --non-interactive mode). + if [[ "${cfg_chart_kind}" == "path" && -z "${CHART_PATH}" ]]; then + log_error "ce-config.yaml sets installer.chartSource.kind: path but installer.chartSource.chartPath is empty." + exit 1 + fi + + # Everything else (registry.url/username, REGISTRY_PASSWORD) already has a flag/env/prompt + # fallback — only fail fast here in --non-interactive mode, where there is no prompt left + # to catch it, so every missing field is reported together instead of one exit-1 at a time. + # --skip-secret bypasses the username/password checks below (main() still runs + # gather_install_params for the registry URL, but skips create_registry_secret). Pure + # -f-only mode (no --config) bypasses this whole block — main() skips + # gather_install_params/create_registry_secret entirely in that case; -f combined with + # --config does not bypass it, since --config still drives secret creation there. + if [[ "${NON_INTERACTIVE}" == "true" && ( -z "${VALUES_FILE}" || -n "${CONFIG_FILE}" ) ]]; then + local -a missing=() + if [[ "${LOCAL_REGISTRY}" != "true" && -z "${REGISTRY_URL:-}" && -z "${CONFIG_REGISTRY_URL}" ]]; then + missing+=("installer.registry.url (or REGISTRY_URL)") + fi + if [[ "${SKIP_REGISTRY_SECRET}" != "true" && "${LOCAL_REGISTRY}" != "true" ]]; then + if [[ -z "${REGISTRY_USERNAME:-}" && -z "${CONFIG_REGISTRY_USERNAME}" ]]; then + missing+=("installer.registry.secret.username (or REGISTRY_USERNAME)") + fi + if [[ -z "${REGISTRY_PASSWORD:-}" && ! -f "${REGISTRY_PASSWORD_FILE:-}" ]]; then + missing+=("REGISTRY_PASSWORD or REGISTRY_PASSWORD_FILE (env/file only — never set via ce-config.yaml)") + fi + fi + if (( ${#missing[@]} > 0 )); then + log_error "Missing required configuration (non-interactive mode, no prompt to fall back on):" + local m + for m in "${missing[@]}"; do + log_error " - ${m}" + done + exit 1 + fi + fi +} + +verify_existing_registry_secret() { + if ! kubectl get secret "${REGISTRY_SECRET_NAME}" --namespace "${NAMESPACE}" &> /dev/null; then + log_error "--skip-secret was used but secret '${REGISTRY_SECRET_NAME}' does not exist in namespace '${NAMESPACE}'." + log_error "Create it first (kubectl create secret docker-registry ...), or drop --skip-secret to let install.sh create it." + exit 1 + fi +} + +create_registry_secret() { + if [[ "${LOCAL_REGISTRY}" == "true" ]]; then + if [[ "${DRY_RUN}" == "true" ]]; then + log_info "Dry-run: would create local registry secret '${REGISTRY_SECRET_NAME}'" + return 0 + fi + if kubectl get secret "${REGISTRY_SECRET_NAME}" --namespace "${NAMESPACE}" &> /dev/null; then + log_info "Secret '${REGISTRY_SECRET_NAME}' already exists; replacing..." + kubectl delete secret "${REGISTRY_SECRET_NAME}" --namespace "${NAMESPACE}" + fi + log_info "Creating local registry secret '${REGISTRY_SECRET_NAME}'..." + kubectl create secret docker-registry "${REGISTRY_SECRET_NAME}" \ + --namespace "${NAMESPACE}" \ + --docker-server "${LOCAL_REGISTRY_URL}" \ + --docker-username "local" \ + --docker-password "local" \ + --docker-email "local@local" + return 0 + fi + + local username password server email + username="$(prompt_or_env "REGISTRY_USERNAME" "Docker registry username" "${CONFIG_REGISTRY_USERNAME}")" + + # REGISTRY_PASSWORD (env) > REGISTRY_PASSWORD_FILE > interactive masked prompt. Never + # settable via ce-config.yaml, same as REGISTRY_PASSWORD — env/file/prompt only. + if [[ -z "${REGISTRY_PASSWORD:-}" && -n "${REGISTRY_PASSWORD_FILE:-}" ]]; then + if [[ ! -f "${REGISTRY_PASSWORD_FILE}" ]]; then + log_error "REGISTRY_PASSWORD_FILE is set but the file does not exist: ${REGISTRY_PASSWORD_FILE}" + exit 1 + fi + REGISTRY_PASSWORD="$(<"${REGISTRY_PASSWORD_FILE}")" + fi + password="$(prompt_or_env "REGISTRY_PASSWORD" "Docker registry password" "" "secret")" + server="$(prompt_or_env "REGISTRY_SERVER" "Docker server URL" "${CONFIG_REGISTRY_SERVER:-https://index.docker.io/v1/}")" + email="$(prompt_or_env "REGISTRY_EMAIL" "Docker registry email" "${CONFIG_REGISTRY_EMAIL}")" + + if [[ -z "$username" || -z "$password" ]]; then + log_error "Registry username and password are required." + exit 1 + fi + + if [[ "${DRY_RUN}" == "true" ]]; then + log_info "Dry-run: would create registry secret '${REGISTRY_SECRET_NAME}'" + REGISTRY_USERNAME_VALUE="${username}" + REGISTRY_PASSWORD_VALUE="${password}" + REGISTRY_SERVER_VALUE="${server}" + return 0 + fi + + if kubectl get secret "${REGISTRY_SECRET_NAME}" --namespace "${NAMESPACE}" &> /dev/null; then + log_info "Secret '${REGISTRY_SECRET_NAME}' already exists; replacing..." + kubectl delete secret "${REGISTRY_SECRET_NAME}" --namespace "${NAMESPACE}" + fi + + log_info "Creating Docker registry secret '${REGISTRY_SECRET_NAME}'..." + kubectl create secret docker-registry "${REGISTRY_SECRET_NAME}" \ + --namespace "${NAMESPACE}" \ + --docker-username "${username}" \ + --docker-password "${password}" \ + --docker-server "${server}" \ + --docker-email "${email}" + REGISTRY_USERNAME_VALUE="${username}" + REGISTRY_PASSWORD_VALUE="${password}" + REGISTRY_SERVER_VALUE="${server}" +} + +resolve_external_host() { + [[ -n "${EXTERNAL_HOST_ADDRESS}" ]] && return + + local suggested_host="localhost" + # minikube/docker-desktop are heuristics about the ambient local environment — meaningless + # once KUBE_CONTEXT explicitly selects a different (possibly remote) cluster, and + # "kubectl config current-context" always reports the kubeconfig's ambient current-context + # regardless of --context, so it can't be made KUBE_CONTEXT-aware. Skip straight to the + # node-IP fallback below, which already goes through the KUBE_CONTEXT-aware kubectl wrapper. + if [[ -n "${KUBE_CONTEXT}" ]]; then + local node_ip + node_ip="$(kubectl get node -o jsonpath='{.items[0].status.addresses[?(@.type=="InternalIP")].address}' 2>/dev/null || true)" + [[ -n "$node_ip" ]] && suggested_host="$node_ip" + elif command -v minikube &> /dev/null && minikube ip &> /dev/null 2>&1; then + suggested_host="$(minikube ip)" + elif kubectl config current-context 2>/dev/null | grep -q "docker-desktop"; then + # host.docker.internal resolves to the host from both pods and the host terminal on Docker Desktop + suggested_host="host.docker.internal" + fi + # No heuristic matched (e.g. kind/k3d, or a local cluster type we don't special-case): + # suggested_host keeps its "localhost" default from above. Those tools typically + # NodePort-map to localhost rather than an internal Docker-network IP, so this is a + # better generic guess than a node-IP lookup that may not be reachable from here. + + EXTERNAL_HOST_ADDRESS="$(prompt_or_env "EXTERNAL_HOST_ADDRESS" "Local URL / address to reach the cluster (e.g. localhost or minikube ip)" "${CONFIG_EXTERNAL_HOST_ADDRESS:-$suggested_host}")" + + if [[ -z "${EXTERNAL_HOST_ADDRESS}" ]]; then + log_error "External host address is required." + exit 1 + fi +} + +gather_install_params() { + local registry_url + + resolve_external_host + + if [[ "${LOCAL_REGISTRY}" == "true" ]]; then + if [[ "${ENABLE_INGRESS}" == "true" ]]; then + LOCAL_REGISTRY_URL="registry.${EXTERNAL_HOST_ADDRESS}" + else + LOCAL_REGISTRY_URL="local-registry.${NAMESPACE}.svc.cluster.local:5000" + fi + log_info "Local registry URL: ${LOCAL_REGISTRY_URL}" + REGISTRY_URL="${LOCAL_REGISTRY_URL}" + else + local suggested_registry_url="" + [[ -n "${REGISTRY_USERNAME_VALUE:-}" ]] && suggested_registry_url="index.docker.io/${REGISTRY_USERNAME_VALUE}" + registry_url="$(prompt_or_env "REGISTRY_URL" "Docker registry URL for images (e.g. index.docker.io/)" "${CONFIG_REGISTRY_URL:-$suggested_registry_url}")" + if [[ -z "$registry_url" ]]; then + log_error "Registry URL is required (e.g. index.docker.io/)." + exit 1 + fi + REGISTRY_URL="$registry_url" + fi +} + +# Live-updating UI: show deployments and statefulsets progress until helm_pid exits. +# Redraws a table in place every PROGRESS_INTERVAL_SEC seconds. Only runs when stdout is a TTY. +run_deployment_progress_ui() { + local helm_pid=$1 + local interval="${PROGRESS_INTERVAL_SEC:-10}" + local lines=0 + local box_inner_width=62 + local green=$'\033[0;32m' + local yellow=$'\033[1;33m' + local cyan=$'\033[0;36m' + local nc=$'\033[0m' + + [[ ! -t 1 ]] && return 0 + command -v tput &>/dev/null || return 0 + kill -0 "$helm_pid" 2>/dev/null || return 0 + + tput civis 2>/dev/null || true + + _render_workload_section() { + local data="$1" + while IFS= read -r line; do + if [[ "$line" =~ ^NAME ]]; then + printf ' %s\n' "$line" + elif [[ "$line" =~ ([0-9]+)/([0-9]+) ]]; then + local ready="${BASH_REMATCH[1]}" desired="${BASH_REMATCH[2]}" + if [[ "$ready" == "$desired" ]]; then + printf ' %s %sReady%s\n' "$line" "$green" "$nc" + else + printf ' %s %sDeploying...%s\n' "$line" "$yellow" "$nc" + fi + else + printf ' %s\n' "$line" + fi + done <<< "$data" + } + + while kill -0 "$helm_pid" 2>/dev/null; do + local deploy_output ss_output deploy_count ss_count new_lines progress_title + deploy_output="$(kubectl get deployments -n "${NAMESPACE}" 2>/dev/null || true)" + ss_output="$(kubectl get statefulsets -n "${NAMESPACE}" 2>/dev/null || true)" + + # Count output lines (0 if empty) + deploy_count=0; [[ -n "$deploy_output" ]] && deploy_count=$(printf '%s\n' "$deploy_output" | wc -l) + ss_count=0; [[ -n "$ss_output" ]] && ss_count=$(printf '%s\n' "$ss_output" | wc -l) + + # Frame height: box(4) + "Deployments:\n\n"(2) + rows + "\n\nStatefulSets:\n\n"(3) + rows + "\n⏳..."(2) + new_lines=$(( 4 + 2 + deploy_count + 3 + ss_count + 2 )) + + # Extend reservation if content grew since last frame + if (( new_lines > lines )); then + for ((i=lines; i/dev/null || true +} + +# Parse Helm NOTES and print a table: Service | URL | Credentials +print_notes_table() { + local notes + notes="$(helm get notes "${RELEASE_NAME}" --namespace "${NAMESPACE}" 2>/dev/null)" || return 0 + [[ -z "$notes" ]] && return 0 + + local service="" url="" user="" pass="" + local line + printf '\n%s\n' "==============================================" + printf '%s\n' " MLRun CE - Access URLs" + printf '%s\n' "==============================================" + printf '%-18s | %-24s | %s\n' "SERVICE" "URL" "CREDENTIALS" + printf '%-18s-+-%-24s-+-%s\n' "------------------" "------------------------" "------------------" + + while IFS= read -r line; do + line="${line#"${line%%[![:space:]]*}"}" + if [[ "$line" =~ ^(.+)\ is\ available\ at:\ *$ ]]; then + if [[ -n "$service" ]]; then + local c="" + [[ -n "$user" || -n "$pass" ]] && c="${user} / ${pass}" + printf '%-18s | %-24s | %s\n' "$service" "$url" "$c" + fi + service="${BASH_REMATCH[1]}" + url="" + user="" + pass="" + elif [[ "$line" =~ ^-\ \ *username:\ *(.*)$ ]]; then + user="${BASH_REMATCH[1]}" + elif [[ "$line" =~ ^-\ \ *password:\ *(.*)$ ]]; then + pass="${BASH_REMATCH[1]}" + elif [[ -n "$service" && -n "$line" && "$line" != "You're up and running!" && "$line" != "Happy MLOPSing!!! :]" ]]; then + url="$line" + fi + done <<< "$notes" + if [[ -n "$service" ]]; then + local c="" + [[ -n "$user" || -n "$pass" ]] && c="${user} / ${pass}" + printf '%-18s | %-24s | %s\n' "$service" "$url" "$c" + fi + printf '%s\n' "==============================================" + printf '\n' +} + +resolve_chart_source() { + if [[ -n "${CHART_PATH}" ]]; then + if [[ ! -d "${CHART_PATH}" ]]; then + log_error "Chart path not found: ${CHART_PATH}" + exit 1 + fi + if [[ ! -f "${CHART_PATH}/Chart.yaml" ]]; then + log_error "No Chart.yaml found in ${CHART_PATH} — is this a valid Helm chart directory?" + exit 1 + fi + if [[ -n "${CE_VERSION}" ]]; then + log_warn "--ce-version is ignored in local-path mode (chart version comes from ${CHART_PATH}/Chart.yaml)." + fi + log_info "Using local chart: ${CHART_PATH}" + log_info "Running helm dependency update..." + helm dependency update "${CHART_PATH}" + CHART_REF="${CHART_PATH}" + else + log_info "Adding Helm repository..." + helm repo add mlrun-ce "${HELM_REPO_URL}" 2>/dev/null || true + helm repo update > /dev/null 2>&1 + CHART_REF="mlrun-ce/mlrun-ce" + fi +} + +helm_install() { + resolve_chart_source + + if [[ "${DRY_RUN}" == "true" ]]; then + log_info "Dry-run mode: rendering chart without deploying..." + else + log_info "Installing MLRun CE (release: ${RELEASE_NAME})..." + fi + + local helm_exit=0 + local -a extra_set_flags=() + local -a version_flag=() + local -a dry_run_flag=() + local -a values_flag=() + [[ -n "${CE_VERSION}" && -z "${CHART_PATH}" ]] && version_flag=(--version "${CE_VERSION}") + [[ "${DRY_RUN}" == "true" ]] && dry_run_flag=(--dry-run=server) + [[ -n "${VALUES_FILE}" ]] && values_flag=(--values "${VALUES_FILE}") + # Pure -f-only mode (no --config) stays fully self-contained (main() never resolved + # REGISTRY_URL/EXTERNAL_HOST_ADDRESS in that case) — every other mode (--config alone, + # or -f + --config together) sets these as --set, which helm applies after --values, + # so a config-resolved value always wins over the same key in a -f file. + if [[ -z "${VALUES_FILE}" || -n "${CONFIG_FILE}" ]]; then + extra_set_flags+=(--set "global.registry.url=${REGISTRY_URL}") + extra_set_flags+=(--set "global.registry.secretName=${REGISTRY_SECRET_NAME}") + extra_set_flags+=(--set "global.externalHostAddress=${EXTERNAL_HOST_ADDRESS}") + fi + if [[ -n "${MLRUN_VERSION}" ]]; then + extra_set_flags+=(--set "mlrun.api.image.tag=${MLRUN_VERSION}") + extra_set_flags+=(--set "mlrun.ui.image.tag=${MLRUN_VERSION}") + extra_set_flags+=(--set "mlrun.api.sidecars.logCollector.image.tag=${MLRUN_VERSION}") + fi + if [[ -n "${NUCLIO_VERSION}" ]]; then + extra_set_flags+=(--set "nuclio.controller.image.tag=${NUCLIO_VERSION}") + extra_set_flags+=(--set "nuclio.dashboard.image.tag=${NUCLIO_VERSION}") + fi + if [[ "${DISABLE_SYSTEM_MONITORING}" == "true" ]]; then + extra_set_flags+=(--set "kube-prometheus-stack.enabled=false") + fi + if [[ "${DISABLE_SPARK}" == "true" ]]; then + extra_set_flags+=(--set "spark-operator.enabled=false") + fi + if [[ "${DISABLE_MPI}" == "true" ]]; then + extra_set_flags+=(--set "mpi-operator.deployment.create=false") + extra_set_flags+=(--set "mpi-operator.crd.create=false") + extra_set_flags+=(--set "mpi-operator.rbac.create=false") + fi + if [[ "${DISABLE_MODEL_MONITORING}" == "true" ]]; then + extra_set_flags+=(--set "strimzi-kafka-operator.enabled=false") + extra_set_flags+=(--set "kafka.enabled=false") + extra_set_flags+=(--set "timescaledb.enabled=false") + fi + if [[ "${ENABLE_INGRESS}" == "true" ]]; then + extra_set_flags+=(--set "jupyterNotebook.ingress.enabled=true") + extra_set_flags+=(--set "jupyterNotebook.ingress.ingressClassName=${INGRESS_CLASS}") + extra_set_flags+=(--set "nuclio.dashboard.ingress.enabled=true") + extra_set_flags+=(--set "mlrun.api.ingress.enabled=true") + extra_set_flags+=(--set "mlrun.ui.ingress.enabled=true") + fi + if [[ "${ENABLE_OTEL_OPERATOR}" == "true" ]]; then + extra_set_flags+=(--set "opentelemetry-operator.enabled=true") + fi + if [[ "${ENABLE_OTEL_COLLECTOR}" == "true" ]]; then + extra_set_flags+=(--set "opentelemetry.collector.enabled=true") + fi + if [[ "${ENABLE_OTEL_NAMESPACE_LABEL}" == "true" ]]; then + extra_set_flags+=(--set "opentelemetry.namespaceLabel.enabled=true") + fi + if [[ "${ENABLE_OTEL_INSTRUMENTATION}" == "true" ]]; then + extra_set_flags+=(--set "opentelemetry.instrumentation.enabled=true") + fi + if [[ "${LOCAL_REGISTRY}" == "true" && "${ENABLE_INGRESS}" == "true" ]]; then + # Local registry serves HTTP via nginx; tell kaniko to push/pull without TLS + extra_set_flags+=(--set "mlrun.api.kaniko.insecureRegistry=true") + fi + + if [[ "${SHOW_PROGRESS}" == "true" && "${DRY_RUN}" != "true" ]]; then + local helm_output helm_pid + helm_output="$(mktemp)" + trap 'tput cnorm 2>/dev/null || true; rm -f "${helm_output:-}"; exit 130' INT TERM + helm upgrade --install "${RELEASE_NAME}" "${CHART_REF}" \ + --namespace "${NAMESPACE}" \ + --wait \ + ${values_flag[@]+"${values_flag[@]}"} \ + ${version_flag[@]+"${version_flag[@]}"} \ + ${dry_run_flag[@]+"${dry_run_flag[@]}"} \ + ${extra_set_flags[@]+"${extra_set_flags[@]}"} > "$helm_output" 2>&1 & + helm_pid=$! + run_deployment_progress_ui "$helm_pid" + wait "$helm_pid" || helm_exit=$? + if [[ $helm_exit -ne 0 ]]; then + printf '%s\n' "${RED}[ERROR]${NC} Helm upgrade/install failed. Output below:" + if [[ -f "$helm_output" ]]; then + cat "$helm_output" + else + printf '%s\n' "${YELLOW}[WARN]${NC} Helm output log file was already removed: $helm_output" + fi + rm -f "$helm_output" + exit "$helm_exit" + fi + rm -f "$helm_output" + trap - INT TERM + + else + helm upgrade --install "${RELEASE_NAME}" "${CHART_REF}" \ + --namespace "${NAMESPACE}" \ + --wait \ + ${values_flag[@]+"${values_flag[@]}"} \ + ${version_flag[@]+"${version_flag[@]}"} \ + ${dry_run_flag[@]+"${dry_run_flag[@]}"} \ + ${extra_set_flags[@]+"${extra_set_flags[@]}"} || helm_exit=$? + fi + + if [[ $helm_exit -ne 0 ]]; then + log_error "Helm installation failed (exit code ${helm_exit})." + exit "$helm_exit" + fi + + # When we used the progress UI, the cursor is after it; print a clear separator and the URL table + if [[ "${SHOW_PROGRESS}" == "true" ]]; then + printf '\n' + fi + if [[ "${DRY_RUN}" == "true" ]]; then + log_info "Dry-run complete (no resources deployed)." + else + log_info "Installation complete." + if helm status "${RELEASE_NAME}" --namespace "${NAMESPACE}" &> /dev/null; then + print_notes_table + fi + fi +} + +parse_args() { + while [[ $# -gt 0 ]]; do + case "$1" in + -h|--help) + usage + exit 0 + ;; + --skip-secret) + SKIP_REGISTRY_SECRET="true" + shift + ;; + --skip-validators) + SKIP_VALIDATORS="true" + shift + ;; + -f|--values) + if [[ -z "${2:-}" ]]; then + log_error "Option $1 requires a value (path to YAML file)." + exit 1 + fi + VALUES_FILE="$2" + shift 2 + ;; + --show-progress) + SHOW_PROGRESS="true" + shift + ;; + --uninstall) + UNINSTALL="true" + shift + ;; + --hard-clean) + HARD_CLEAN="true" + shift + ;; + --disable-system-monitoring) + DISABLE_SYSTEM_MONITORING="true" + shift + ;; + --disable-spark) + DISABLE_SPARK="true" + shift + ;; + --disable-mpi) + DISABLE_MPI="true" + shift + ;; + --disable-model-monitoring) + DISABLE_MODEL_MONITORING="true" + shift + ;; + --enable-ingress) + ENABLE_INGRESS="true" + if [[ -n "${2:-}" && "${2}" != --* ]]; then + INGRESS_CLASS="$2"; shift + fi + shift + ;; + --enable-otel) + local otel_mode="full" + if [[ -n "${2:-}" && "${2}" != --* ]]; then + otel_mode="$2"; shift + fi + case "$otel_mode" in + off) + ENABLE_OTEL_OPERATOR="false" + ENABLE_OTEL_COLLECTOR="false" + ENABLE_OTEL_NAMESPACE_LABEL="false" + ENABLE_OTEL_INSTRUMENTATION="false" + ;; + collector) + ENABLE_OTEL_OPERATOR="true" + ENABLE_OTEL_COLLECTOR="true" + ;; + full) + ENABLE_OTEL_OPERATOR="true" + ENABLE_OTEL_COLLECTOR="true" + ENABLE_OTEL_NAMESPACE_LABEL="true" + ENABLE_OTEL_INSTRUMENTATION="true" + ;; + *) + log_error "Invalid --enable-otel mode: '${otel_mode}' (expected off, collector, or full)." + exit 1 + ;; + esac + shift + ;; + --enable-otel-operator) + ENABLE_OTEL_OPERATOR="true" + shift + ;; + --enable-otel-collector) + ENABLE_OTEL_COLLECTOR="true" + shift + ;; + --enable-otel-namespace-label) + ENABLE_OTEL_NAMESPACE_LABEL="true" + shift + ;; + --enable-otel-instrumentation) + ENABLE_OTEL_INSTRUMENTATION="true" + shift + ;; + --local-registry) + LOCAL_REGISTRY="true" + shift + ;; + --chart-path) + if [[ -z "${2:-}" || "${2:-}" == --* ]]; then + log_error "Option $1 requires a directory path (e.g. --chart-path ./charts/mlrun-ce)." + exit 1 + fi + CHART_PATH="$2" + shift 2 + ;; + --ce-version) + if [[ -z "${2:-}" || "${2:-}" == --* ]]; then + log_error "Option $1 requires a version value (e.g. --ce-version 0.11.0)." + exit 1 + fi + CE_VERSION="$2" + shift 2 + ;; + --dry-run) + DRY_RUN="true" + shift + ;; + --non-interactive) + NON_INTERACTIVE="true" + shift + ;; + --config) + if [[ -z "${2:-}" || "${2:-}" == --* ]]; then + log_error "Option $1 requires a file path (e.g. --config ce-config.yaml)." + exit 1 + fi + CONFIG_FILE="$2" + shift 2 + ;; + *) + log_warn "Unknown option: $1 (ignored)" + shift + ;; + esac + done +} + +do_hard_clean() { + log_warn "Hard clean: deleting all PVCs in namespace '${NAMESPACE}'..." + local pvcs + pvcs="$(kubectl get pvc --namespace "${NAMESPACE}" --no-headers -o custom-columns=':metadata.name' 2>/dev/null)" || true + if [[ -z "$pvcs" ]]; then + log_info "No PVCs found in namespace '${NAMESPACE}'." + else + while IFS= read -r pvc; do + [[ -z "$pvc" ]] && continue + log_info " Deleting PVC: ${pvc}" + kubectl delete pvc "${pvc}" --namespace "${NAMESPACE}" --timeout 60s 2>/dev/null || \ + kubectl delete pvc "${pvc}" --namespace "${NAMESPACE}" --force --grace-period=0 --wait=false 2>/dev/null || true + done <<< "$pvcs" + fi + + log_warn "Hard clean: deleting released/failed PVs bound to namespace '${NAMESPACE}'..." + local pvs + pvs="$(kubectl get pv --no-headers -o custom-columns=':metadata.name,:spec.claimRef.namespace,:status.phase' 2>/dev/null \ + | awk -v ns="${NAMESPACE}" '$2 == ns { print $1 }')" || true + if [[ -z "$pvs" ]]; then + log_info "No PVs found for namespace '${NAMESPACE}'." + else + while IFS= read -r pv; do + [[ -z "$pv" ]] && continue + log_info " Deleting PV: ${pv}" + kubectl delete pv "${pv}" --timeout 60s 2>/dev/null || \ + kubectl delete pv "${pv}" --force --grace-period=0 --wait=false 2>/dev/null || true + done <<< "$pvs" + fi + + log_info "Hard clean complete." +} + +do_uninstall() { + check_requirements + if ! helm status "${RELEASE_NAME}" --namespace "${NAMESPACE}" &>/dev/null; then + log_warn "Release '${RELEASE_NAME}' not found in namespace '${NAMESPACE}' (already uninstalled?)." + else + log_info "Uninstalling MLRun CE release '${RELEASE_NAME}' from namespace '${NAMESPACE}'..." + helm uninstall "${RELEASE_NAME}" --namespace "${NAMESPACE}" --timeout 960s + log_info "Uninstall complete." + fi + + if [[ "${HARD_CLEAN}" == "true" ]]; then + do_hard_clean + fi +} + +# Chart's fixed NodePorts (not configurable via values.yaml) — see docs/design-proposal.md §6. +REQUIRED_NODEPORTS=(30010 30020 30040 30050 30060 30070 30093 30094 30100 30110) + +# The Helm floor mirrors charts/mlrun-ce/README.md's "Helm >=3.6" — the chart's own stated +# requirement, so the installer never refuses a Helm the chart itself supports. +# +# Kubernetes has no floor by default: the chart declares no kubeVersion in Chart.yaml and +# the README states no cluster version, so there is nothing to enforce. The check reports +# what it finds and only warns when MIN_K8S_VERSION is set explicitly. Both are overridable +# upward for anyone who wants to enforce a stricter environment. +MIN_K8S_VERSION="${MIN_K8S_VERSION:-}" +MIN_HELM_VERSION="${MIN_HELM_VERSION:-3.6}" +MIN_K8S_MAJOR="" +MIN_K8S_MINOR="" +if [[ -n "${MIN_K8S_VERSION}" ]]; then + if [[ "${MIN_K8S_VERSION}" =~ ^([0-9]+)\.([0-9]+)$ ]]; then + MIN_K8S_MAJOR="${BASH_REMATCH[1]}" + MIN_K8S_MINOR="${BASH_REMATCH[2]}" + else + log_error "MIN_K8S_VERSION must be in MAJOR.MINOR form (e.g. 1.30), got: '${MIN_K8S_VERSION}'" + exit 1 + fi +fi +if [[ "${MIN_HELM_VERSION}" =~ ^([0-9]+)\.([0-9]+)$ ]]; then + MIN_HELM_MAJOR="${BASH_REMATCH[1]}" + MIN_HELM_MINOR="${BASH_REMATCH[2]}" +else + log_error "MIN_HELM_VERSION must be in MAJOR.MINOR form (e.g. 3.6), got: '${MIN_HELM_VERSION}'" + exit 1 +fi + +# Informational: reports the cluster's Kubernetes version. Never blocks — neither the chart +# nor its README states a required cluster version. Warns only against an explicitly set +# MIN_K8S_VERSION. +validate_k8s_version() { + local raw major minor + raw="$(kubectl get nodes -o jsonpath='{.items[0].status.nodeInfo.kubeletVersion}' 2>/dev/null || true)" + if [[ -z "$raw" ]]; then + log_warn " Could not determine Kubernetes version; skipping version check." + return 0 + fi + if [[ "$raw" =~ v([0-9]+)\.([0-9]+) ]]; then + major="${BASH_REMATCH[1]}" + minor="${BASH_REMATCH[2]}" + else + log_warn " Could not parse Kubernetes version '${raw}'; skipping version check." + return 0 + fi + # Keyed off the derived global, not MIN_K8S_VERSION: `MIN_K8S_VERSION=x source install.sh` + # (how the tests load it) discards the prefix assignment once source returns, which would + # trip `set -u` here. + if [[ -z "${MIN_K8S_MAJOR}" ]]; then + log_info " Kubernetes version: ${major}.${minor}" + return 0 + fi + if (( major < MIN_K8S_MAJOR || (major == MIN_K8S_MAJOR && minor < MIN_K8S_MINOR) )); then + log_warn " Kubernetes version ${major}.${minor} is below the requested minimum (${MIN_K8S_MAJOR}.${MIN_K8S_MINOR})." + return 0 + fi + log_info " Kubernetes version: ${major}.${minor} (>= ${MIN_K8S_MAJOR}.${MIN_K8S_MINOR} required)" +} + +# Blocking: helm CLI version must be >= MIN_HELM_MAJOR.MIN_HELM_MINOR. +validate_helm_version() { + local raw major minor + raw="$(helm version --short 2>/dev/null || true)" + if [[ -z "$raw" ]]; then + log_warn " Could not determine Helm version; skipping version check." + return 0 + fi + if [[ "$raw" =~ v([0-9]+)\.([0-9]+) ]]; then + major="${BASH_REMATCH[1]}" + minor="${BASH_REMATCH[2]}" + else + log_warn " Could not parse Helm version '${raw}'; skipping version check." + return 0 + fi + if (( major < MIN_HELM_MAJOR || (major == MIN_HELM_MAJOR && minor < MIN_HELM_MINOR) )); then + log_error " Helm version ${major}.${minor} is below the minimum supported version (${MIN_HELM_MAJOR}.${MIN_HELM_MINOR})." + return 1 + fi + log_info " Helm version: ${major}.${minor} (>= ${MIN_HELM_MAJOR}.${MIN_HELM_MINOR} required)" +} + +# Blocking: cluster must have a default StorageClass (chart's PVCs rely on one). +validate_storage_class() { + local default_sc + default_sc="$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{"="}{.metadata.annotations.storageclass\.kubernetes\.io/is-default-class}{"\n"}{end}' 2>/dev/null | grep '=true$' || true)" + if [[ -z "$default_sc" ]]; then + log_error " No default StorageClass found in the cluster. MLRun CE requires a default StorageClass for its PVCs." + return 1 + fi + log_info " Default StorageClass: ${default_sc%%=*}" +} + +# Warning only: --enable-ingress flips the chart's own Ingress resources on, but this +# installer never installs a controller for them (BYO-controller only). Check one exists +# so the user finds out now, not after the Ingress silently never gets an address. +validate_ingress_controller() { + if [[ "${ENABLE_INGRESS}" != "true" ]]; then + return 0 + fi + if kubectl get ingressclass "${INGRESS_CLASS}" &> /dev/null; then + log_info " Ingress: IngressClass '${INGRESS_CLASS}' found" + else + log_warn " Ingress: no IngressClass named '${INGRESS_CLASS}' found in the cluster." + log_warn " --enable-ingress only configures the chart's Ingress resources — it does not install a controller." + log_warn " Install one providing that class (e.g. https://kubernetes.github.io/ingress-nginx/) or the Ingress won't resolve." + fi +} + +# Warning only: best-effort docker login with the resolved registry credentials. +validate_registry_auth() { + if [[ "${LOCAL_REGISTRY}" == "true" ]]; then + log_info " Registry auth: skipped (--local-registry in use, no external registry to check)" + return 0 + fi + if [[ -z "${REGISTRY_USERNAME_VALUE:-}" || -z "${REGISTRY_PASSWORD_VALUE:-}" ]]; then + log_info " Registry auth: skipped (no registry credentials resolved, e.g. -f-only mode)" + return 0 + fi + local server="${REGISTRY_SERVER_VALUE:-https://index.docker.io/v1/}" + if printf '%s' "${REGISTRY_PASSWORD_VALUE}" | docker login "${server}" -u "${REGISTRY_USERNAME_VALUE}" --password-stdin &> /dev/null; then + log_info " Registry auth: login to ${server} succeeded" + else + log_warn " Registry auth: could not log in to ${server} with the provided credentials (best-effort check; install will continue)" + fi +} + +# Warning only: chart's fixed NodePorts already bound by another Service. +validate_nodeport_conflicts() { + local used_ports port conflicts=() + used_ports="$(kubectl get svc --all-namespaces -o jsonpath='{range .items[*]}{.metadata.namespace}{" "}{range .spec.ports[*]}{.nodePort}{"\n"}{end}{end}' 2>/dev/null | awk -v ns="${NAMESPACE}" '$1 != ns { print $2 }' || true)" + for port in "${REQUIRED_NODEPORTS[@]}"; do + if grep -qx "${port}" <<< "$used_ports"; then + conflicts+=("${port}") + fi + done + if [[ ${#conflicts[@]} -gt 0 ]]; then + log_warn " NodePort conflict: already in use by another Service outside namespace '${NAMESPACE}': ${conflicts[*]}" + else + log_info " NodePorts: no conflicts detected" + fi +} + +# Converts a Kubernetes allocatable-resource quantity to Ki. Memory is always Ki-suffixed, +# but ephemeral-storage is commonly reported as a bare byte count (no suffix) depending on +# the underlying cAdvisor source — handle both, plus Mi/Gi/Ti for good measure. Echoes +# nothing (and returns non-zero) if the value doesn't match any known form. +_allocatable_to_ki() { + local raw="$1" + if [[ "$raw" =~ ^([0-9]+)Ki$ ]]; then + echo "${BASH_REMATCH[1]}" + elif [[ "$raw" =~ ^([0-9]+)Mi$ ]]; then + echo $(( BASH_REMATCH[1] * 1024 )) + elif [[ "$raw" =~ ^([0-9]+)Gi$ ]]; then + echo $(( BASH_REMATCH[1] * 1024 * 1024 )) + elif [[ "$raw" =~ ^([0-9]+)Ti$ ]]; then + echo $(( BASH_REMATCH[1] * 1024 * 1024 * 1024 )) + elif [[ "$raw" =~ ^[0-9]+$ ]]; then + echo $(( raw / 1024 )) + else + return 1 + fi +} + +# Warning only: cluster-wide allocatable RAM/storage above the documented floor (8Gi each). +validate_node_capacity() { + local mem_ki=0 disk_ki=0 value value_ki + while read -r value; do + [[ -z "$value" ]] && continue + value_ki="$(_allocatable_to_ki "$value")" || continue + mem_ki=$(( mem_ki + value_ki )) + done < <(kubectl get nodes -o jsonpath='{range .items[*]}{.status.allocatable.memory}{"\n"}{end}' 2>/dev/null || true) + while read -r value; do + [[ -z "$value" ]] && continue + value_ki="$(_allocatable_to_ki "$value")" || continue + disk_ki=$(( disk_ki + value_ki )) + done < <(kubectl get nodes -o jsonpath='{range .items[*]}{.status.allocatable.ephemeral-storage}{"\n"}{end}' 2>/dev/null || true) + + if [[ "$mem_ki" -eq 0 && "$disk_ki" -eq 0 ]]; then + log_warn " Could not determine node capacity; skipping check." + return 0 + fi + + local mem_gi=$(( mem_ki / 1024 / 1024 )) + local disk_gi=$(( disk_ki / 1024 / 1024 )) + if (( mem_gi < 8 )); then + log_warn " Node capacity: total allocatable memory ~${mem_gi}Gi is below the documented floor of 8Gi" + else + log_info " Node capacity: total allocatable memory ~${mem_gi}Gi" + fi + if (( disk_gi < 8 )); then + log_warn " Node capacity: total allocatable ephemeral storage ~${disk_gi}Gi is below the documented floor of 8Gi" + else + log_info " Node capacity: total allocatable ephemeral storage ~${disk_gi}Gi" + fi +} + +# Dispatcher: runs every check, reports all problems, and exits 1 once at the end if any +# blocking check failed (rather than stopping at the first one). +run_validators() { + log_info "Running pre-install validators..." + local failed=0 + + validate_k8s_version + validate_helm_version || failed=1 + validate_storage_class || failed=1 + validate_registry_auth + validate_ingress_controller + validate_nodeport_conflicts + validate_node_capacity + + if [[ "$failed" == "1" ]]; then + log_error "One or more required pre-install checks failed (see above). Bypass with --skip-validators if you must proceed anyway." + exit 1 + fi + log_info "Pre-install validation passed." +} + +main() { + parse_args "$@" + + # Auto-enable non-interactive when running inside a CI environment + if [[ "${CI:-}" == "true" ]]; then + NON_INTERACTIVE="true" + fi + + load_config + + if [[ "${HARD_CLEAN}" == "true" && "${UNINSTALL}" != "true" ]]; then + log_error "--hard-clean requires --uninstall." + exit 1 + fi + + if [[ "${UNINSTALL}" == "true" ]]; then + do_uninstall + return + fi + + check_requirements + ensure_namespace + + if [[ "${LOCAL_REGISTRY}" == "true" || "${ENABLE_INGRESS}" == "true" ]]; then + resolve_external_host + fi + + if [[ "${LOCAL_REGISTRY}" == "true" ]]; then + deploy_local_registry + fi + + if [[ -n "${VALUES_FILE}" && ! -f "${VALUES_FILE}" ]]; then + log_error "Values file not found: ${VALUES_FILE}" + exit 1 + fi + + # Pure -f-only mode (no --config) stays fully self-contained: no secret creation, no + # registry/host resolution — the values file must already reference an existing secret. + # -f combined with --config runs the same secret creation / registry resolution as + # --config-only, so the config-resolved fields have something to layer their --set on top of. + if [[ -n "${VALUES_FILE}" && -z "${CONFIG_FILE}" ]]; then + log_info "Using values file: ${VALUES_FILE} (skipping secret creation and install prompts)" + else + if [[ "${SKIP_REGISTRY_SECRET}" != "true" ]]; then + create_registry_secret + else + log_info "Skipping registry secret creation (--skip-secret or SKIP_REGISTRY_SECRET)" + verify_existing_registry_secret + fi + gather_install_params + fi + + if [[ "${SKIP_VALIDATORS}" != "true" ]]; then + run_validators + else + log_info "Skipping pre-install validators (--skip-validators or SKIP_VALIDATORS)" + fi + + helm_install +} + +[[ "${INSTALL_SH_SOURCE_ONLY:-}" == "true" ]] || main "$@" \ No newline at end of file From 4197b2f64577a54d671af9abb568c61bb961bbce Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 20 Aug 2026 17:19:42 +0300 Subject: [PATCH 02/15] same --- .claude/skills/run-tests/SKILL.md | 149 ++++ scripts/docs/design-proposal.md | 330 +++++++ tests/install_tests.bats | 1383 +++++++++++++++++++++++++++++ 3 files changed, 1862 insertions(+) create mode 100644 .claude/skills/run-tests/SKILL.md create mode 100644 scripts/docs/design-proposal.md create mode 100644 tests/install_tests.bats diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md new file mode 100644 index 00000000..bbae10f3 --- /dev/null +++ b/.claude/skills/run-tests/SKILL.md @@ -0,0 +1,149 @@ +--- +name: run-tests +description: >- + Run and extend the bats test suite for the CE installer (scripts/install.sh). + Use when a developer asks to run installer tests, check coverage, add a new + test, or verify a change to scripts/install.sh didn't break existing behaviour. +--- + +# CE installer test suite + +Tests live in `tests/install_tests.bats` and use +[bats-core](https://github.com/bats-core/bats-core). They source +`scripts/install.sh` without executing it (via `INSTALL_SH_SOURCE_ONLY=true`) and +stub out external binaries so no live cluster is needed. + +These cover the installer only. The chart's own tests are the other scripts in +`tests/` (`helm-template-test.sh`, `kind-test.sh`) plus `make helm-lint` — all +unrelated to this suite. + +## Prerequisites + +```bash +brew install bats-core # macOS; already installed if tests have been run before +``` + +## Run the full suite + +From the repo root: + +```bash +make installer-test +# or directly: +bats tests/install_tests.bats +``` + +Expected output: `1..96` followed by `ok N ` for every test. + +Keep the count in this file in sync when you add tests — it's the quickest way to +notice a test silently failing to register. + +## Run a single test by name + +```bash +bats --filter "CI=true sets NON_INTERACTIVE" tests/install_tests.bats +``` + +## Run with verbose output + +```bash +bats --verbose-run tests/install_tests.bats +``` + +## How tests source the script safely + +Every test opens with: + +```bash +run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + ... +" +``` + +`SCRIPT` is defined once at the top of the file as +`"$BATS_TEST_DIRNAME/../scripts/install.sh"`, so the suite works regardless of the +directory bats is invoked from. + +`INSTALL_SH_SOURCE_ONLY=true` skips the `main "$@"` call at the bottom of +`install.sh` (guarded by `[[ "${INSTALL_SH_SOURCE_ONLY:-}" == "true" ]] || main "$@"`), +so sourcing only defines functions and global variables — no cluster, no +prompts, no helm calls. + +External binaries (`helm`, `kubectl`, `docker`) are **not** needed for +flag-parsing or `prompt_or_env` tests. For tests that exercise `main()`, stub +every function it calls: + +```bash +check_requirements() { :; } +ensure_namespace() { :; } +create_registry_secret() { :; } +verify_existing_registry_secret(){ :; } +gather_install_params() { :; } +run_validators() { echo "run_validators called"; } +helm_install() { echo "sentinel output"; } +``` + +Tests that exercise the validators individually stub `kubectl`/`helm`/`docker` as +shell functions instead, echoing whatever the check parses (a `kubeletVersion`, +a `helm version --short` string, an allocatable quantity, and so on). + +## Current coverage — 96 tests + +| Phase / area | Tests | +|--------------|-------| +| **Phase 1** — `--ce-version` parsing | stores value, rejects missing arg, respects env var, defaults empty | +| **Phase 1** — `--dry-run` / `--non-interactive` | each sets its var, each defaults false, flag consumed cleanly | +| **Phase 1** — `prompt_or_env` non-interactive | returns default, exits 1 with no default, env var wins over default | +| **Phase 1** — CI auto-detect | `CI=true` sets `NON_INTERACTIVE`; unset CI leaves it false | +| **Phase 2** — `--chart-path` / `resolve_chart_source` | flag parsing (missing arg, flag-looking value), missing dir, missing `Chart.yaml`, `CHART_REF` in both path and published-repo mode, `--ce-version` ignored in path mode | +| **Phase 3** — `--config` / `load_config` | flag parsing, no-op when unset, missing file, missing `yq`, registry field parsing, config value as interactive prompt default, password key warned+ignored, `chartPath` required when `kind: path`, all missing required fields listed together in one pass, `--skip-secret`/`--local-registry` relaxations, never overriding flag/env-set values | +| **Phase 3** — `-f` + `--config` composition | secret still created when both are passed, `-f` alone still skips it (back-compat), registry `--set`s present with both and absent in pure `-f`-only mode | +| **Phase 3** — versions / components / otel | `installer.versions.*` → image-tag `--set`s, `components.*` → `DISABLE_*` (never re-enabling one set by flag), `otel.*` → the 4 `ENABLE_OTEL_*` opt-ins, `--enable-otel [off\|collector\|full]` modes + invalid mode | +| **Phase 3** — registry secret / password | `REGISTRY_PASSWORD_FILE` read, env password wins over it, missing file exits 1, file satisfies the non-interactive password requirement, `verify_existing_registry_secret` present/absent | +| **Phase 3** — `KUBE_CONTEXT` | wrapper functions inject `--context`/`--kube-context`; `resolve_external_host` skips the docker-desktop/minikube heuristics when set, keeps them when unset, falls back to `localhost` when nothing matches | +| **Phase 4** — validators | `--skip-validators` flag and `main()` honoring it; Helm version and StorageClass blocking failures and passes; k8s version reported but never blocking; ingress-controller, registry-auth, NodePort and node-capacity warnings; bare-byte ephemeral-storage parsing; `MIN_HELM_VERSION` raising the floor, `MIN_K8S_VERSION` warning without blocking, empty default accepted and a malformed value rejected at load time; `run_validators` aggregating multiple blocking failures into one `exit 1` | + +## Adding a new test + +1. Open `tests/install_tests.bats`. +2. Add a `@test` block after the relevant section comment. +3. Follow the sourcing pattern above — `INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT'` + inside a `run bash -c "..."` block. +4. Assert with standard bats: `[ "$status" -eq 0 ]`, `[ "$output" = "..." ]`, + `[[ "$output" == *"substring"* ]]`. +5. Run `bats tests/install_tests.bats` to confirm green. + +### Minimal test template + +```bash +@test "description of what is being tested" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + # exercise the function or flag + parse_args --your-flag + echo \"\$YOUR_VAR\" + " + [ "$status" -eq 0 ] + [ "$output" = "expected" ] +} +``` + +## Key scripts/install.sh pointers + +| Symbol | Location | Notes | +|--------|----------|-------| +| Global vars | lines 37-68 | All flags and env vars initialised here | +| `kubectl()` / `helm()` wrappers | lines 82-83 | Inject `KUBE_CONTEXT` into every call | +| `prompt_or_env()` | ~line 377 | Handles interactive/non-interactive/env-var precedence | +| `load_config()` | ~line 411 | Reads the `installer:` block of a `ce-config.yaml` | +| `resolve_external_host()` | ~line 602 | `EXTERNAL_HOST_ADDRESS` autodetect fallback chain | +| `resolve_chart_source()` | ~line 778 | Published-repo vs `--chart-path` mode | +| `helm_install()` | ~line 803 | Builds `extra_set_flags` and runs helm | +| `parse_args()` | ~line 934 | Flag → variable mapping; add new flags here | +| `run_validators()` | ~line 1300 | Pre-install check dispatcher (blocking checks `return 1`) | +| `main()` | ~line 1319 | Orchestration; CI auto-detect lives here | +| Source guard | last line | `[[ "${INSTALL_SH_SOURCE_ONLY:-}" == "true" ]] \|\| main "$@"` | + +These line numbers drift with every change to `install.sh` — prefer grepping for +the function name over trusting them. \ No newline at end of file diff --git a/scripts/docs/design-proposal.md b/scripts/docs/design-proposal.md new file mode 100644 index 00000000..2e80bbd8 --- /dev/null +++ b/scripts/docs/design-proposal.md @@ -0,0 +1,330 @@ +# Proposal: Config-driven, validated MLRun CE installer + +**Status:** Historical design record. Phases 1-5 are implemented; see `../AGENTS.md` for +what actually shipped and where it deviated from this document. +**Note:** written while the installer lived in its own repo, separate from the chart. Since +then it has moved into the chart repo as `scripts/install.sh`, so "this repo" below means +the installer, and the chart — described here as a separate clone — is now `charts/mlrun-ce`. +**Scope:** `scripts/install.sh` — stays pure bash + helm, single file, **local execution only**. + +--- + +## 1. Context & motivation + +`install.sh` is a single bash wrapper around `helm install mlrun-ce/mlrun-ce` (from the +published repo `https://mlrun.github.io/ce`). It works well, but has gaps we keep hitting: + +- **Config sprawl** — ~20 env vars + 11 flags. The only file input (`-f values.yaml`) + *bypasses* secret creation and other wiring. +- **Dead flag** — `--ce-version` is parsed but never passed to helm (`parse_args`, ~line 701). +- **Weak validation** — the only pre-flight check is "is the binary installed." Misconfig + (wrong K8s version, no storage class, bad registry auth, NodePort clash) surfaces only + after a multi-minute helm timeout. No validation of the config file itself. +- **Can't install a local chart checkout** — only the published, packaged chart. We want to + install a `mlrun/ce` branch/PR that the developer has **cloned locally**, by pointing at + its chart path. +- **No first-class CI story** for Jenkins / GitHub Actions. + +**Goal:** adopt the high-value patterns from igzctl (typed config, validators, version +pinning, local chart-path install) while staying a **single, self-contained, pure bash + +helm** script that runs entirely on the local machine and still supports +`curl -sSL .../install.sh | bash`. + +## 2. Design decisions (agreed) + +| Decision | Choice | Rationale | +|---|---|---| +| Structure | **Single-file `install.sh`** | Preserves the `curl \| bash` one-liner; no `lib/` sourcing | +| Config format | **`yq`-parsed `ce-config.yaml`** | One declarative file; `yq` needed only when a config file is used | +| Chart source | **Published repo OR local path** | **No git fetching in the installer** — the developer clones `mlrun/ce` and passes `--chart-path` | +| Execution | **Local only — no SSH** | Unlike mlefi, the installer never remotes into a host; it runs locally against the current kubeconfig | +| Registry input | **CLI value, else YAML fallback** | A flag/env value wins; pressing Enter (or leaving it unset) falls back to the config YAML | +| Config safety | **Validate file exists + no empty required fields** | Fail fast with a clear message instead of a bad helm run | +| UI ingress | **Opt-in; react to the chart** | Chart ships the UI Ingress (`mlrun.ui.ingress.enabled: false`) but no controller — installer only flips that value, never installs a controller (see Phase 5) | +| Build order | **Quick wins first** | Safe, isolated, immediate value | + +**Precedence everywhere:** flag/CLI > env var > config file > built-in default. Existing +flags/env keep working unchanged (back-compat). + +--- + +## 3. Phased plan + +### Phase 1 — Quick wins (no behavior change to existing flows) +1. **Fix `--ce-version`** — pass `--version "${CE_VERSION}"` to helm in published-repo mode. +2. **`--dry-run`** — inject `--dry-run=server`, skip the progress UI and the post-install + notes table (no release exists yet). +3. **`--non-interactive` + CI auto-detect** — set automatically when `CI=true`; in + `prompt_or_env`, return the default instead of calling `read`, so missing required values + fail cleanly with exit 1 instead of hanging on stdin. +4. Update `usage()`. + +### Phase 2 — Install from a locally cloned chart (`--chart-path`) +No git operations in the installer. The developer clones `mlrun/ce` themselves (any +branch/PR), then points the installer at the chart directory. + +- New flag/env: **`--chart-path DIR`** / `CHART_PATH` (e.g. `./charts/mlrun-ce`). +- New `resolve_chart_source()` (called at the top of `helm_install`): + - **Published-repo mode** (default, no `--chart-path`): `helm repo add/update`; + `CHART_REF="mlrun-ce/mlrun-ce"`; optional `--version` from `--ce-version`. + - **Local-path mode** (`--chart-path` set): verify the dir exists and contains + `Chart.yaml` (**error otherwise**); `helm dependency update "${CHART_PATH}"`; + `CHART_REF="${CHART_PATH}"`. +- In `helm_install`, the chart reference becomes `${CHART_REF}` (replaces the 4 hardcoded + `mlrun-ce/mlrun-ce` occurrences). + +### Phase 3 — `ce-config.yaml` (yq) + validation — done + +Implemented as designed below, with two deviations from the original draft: + +1. **Partial reduction in scope, later revisited:** `ingress.*`/`components.*` from the + example YAML were initially **not** wired up — deferred to avoid half-finishing + something that overlaps Phase 5's ingress work. `components.*` was added back in a + follow-up pass (`installer.components.{monitoring,spark,mpi,modelMonitoring}`, + mirroring the existing `--disable-*` flags' exact `--set` effects — pure value + resolution, no new infra logic) after explicit scoping: `ingress.*` and anything that + triggers real infra beyond a `--set` (installing nginx-ingress, deploying a local + registry) stays deferred to Phase 5; only the pure `--set` toggles were in scope here. +2. **`-f`/`--config` composition — reversed, then reverted back:** the original design + said `--config` knobs become `--set` flags that layer on top of `-f`'s `--values` + (helm applies `--set` after `--values`, so `--config` would always win, "no extra + merge logic needed"). The first implementation pass didn't actually do this — `-f` + silently made every `--config` registry/host field dead code with no warning. That + was "fixed" at the time by making **`--config` and `-f`/`--values` mutually + exclusive** (`main()` exit 1 if both given) rather than building the layering — a + misread of the actual ask, corrected in a later follow-up back to the originally + designed composable behavior described in "Combining with `-f`/`--values`" below. + `main()` now runs `create_registry_secret`/`gather_install_params` whenever + `--config` is given (even alongside `-f`), and the 3 registry `--set`s + (`global.registry.{url,secretName}`, `global.externalHostAddress`) moved into the + same `extra_set_flags` array the other `--set`s already used — gated off only in + pure `-f`-only mode (no `--config`), which keeps `-f`-alone's exact prior + self-contained behavior (no secret creation, no registry `--set`s) unchanged. Added + `installer.versions.{mlrun,nuclio}` (→ + `--set mlrun.{api,ui}.image.tag`/`nuclio.{controller,dashboard}.image.tag`) to close + the version-pinning gap this raised — usable via `--config` or the `MLRUN_VERSION`/ + `NUCLIO_VERSION` env vars (the latter also works with `-f`, since it's not gated by + `--config`). +3. **New safety check found along the way:** `--skip-secret` previously trusted the user + completely — if the named secret didn't actually exist, the failure only surfaced + after a `helm --wait` timeout with a cryptic pod-level error. Added + `verify_existing_registry_secret()`: exits 1 immediately with a clear message if the + secret is missing. +4. **`REGISTRY_PASSWORD_FILE` added:** the password was (and still is) deliberately + never read from `ce-config.yaml` — only `REGISTRY_PASSWORD` (env) or the interactive + masked (`read -s`) prompt could supply it. Added `REGISTRY_PASSWORD_FILE` as a third + option — path to a local file containing just the password, trailing newline + stripped, never echoed — for the common case where a CI runner or secrets manager + mounts a secret as a file rather than an env var. Precedence: `REGISTRY_PASSWORD` + env > `REGISTRY_PASSWORD_FILE` > interactive prompt; still never settable via + `ce-config.yaml`. +One YAML file, **only** the reserved **`installer:`** block — a curated set of basic, +typed knobs (registry, chart source, ingress toggle, component enables). No generic +passthrough section: `ce-config.yaml` is not a values-file substitute, and never becomes +a `-f` values file itself. New **`--config FILE`** flag. **`-f`/`--values` stays fully +independent** and can be combined with `--config` — arbitrary/complex helm values always +go through `-f`, never through the config file. (Design note: an earlier draft of this +phase let `ce-config.yaml` pass "everything else" straight through to helm as a second +values file — dropped, since that duplicated `-f`'s job and risked the two silently +overwriting each other.) + +```yaml +installer: # consumed by install.sh; stripped before helm + kubeContext: "" # optional; blank = current local context (no SSH, no remote exec) + externalHostAddress: auto # auto-detect locally, or pin a value + registry: + url: index.docker.io/myuser # used if not given on the CLI (Enter falls back to this) + secret: + name: registry-credentials + create: true # user brings creds; PASSWORD via env/prompt only, never in file + server: https://index.docker.io/v1/ + username: myuser + chartSource: + kind: repo # repo | path + chartVersion: 0.11.0 # repo mode -> --version + chartPath: "" # path mode -> local cloned chart dir + ingress: + ui: false # opt-in -> --set mlrun.ui.ingress.enabled=true (chart's own ingress) + className: "" # ingress class (blank = chart/cluster default) + host: "" # optional host override; else chart uses global.externalHostAddress + components: { monitoring: false, spark: true } # -> --set .enabled=... +``` + +- **`load_config()`**: + - **Error if `--config FILE` does not exist.** + - Require `yq`; read `installer.*` into vars **only if not already set** by flag/env + (precedence: flag > env > `ce-config.yaml` > default — same chain as every other + setting; `ce-config.yaml` just slots in as the config layer). For registry values + this is the "CLI wins, else YAML" behavior; in interactive mode the YAML value + becomes the prompt default (Enter accepts it). + - **Validate required fields are non-empty** (e.g. `registry.url`, `registry.secret.username`, + and `chartPath` when `chartSource.kind: path`); **raise an error listing every empty + required field** and exit 1. + - Every `installer.*` key resolves to an explicit `--set` flag in `helm_install`, the same + pattern `extra_set_flags` already uses for `DISABLE_SPARK`/`ENABLE_INGRESS`/etc. There is + nothing else in the file to strip or pass through. +- **Host / context:** `externalHostAddress: auto` reuses the existing local autodetect + (minikube / docker-desktop / node IP). `kubeContext` (if set) is passed as + `--kube-context` to kubectl/helm — still local execution, just selects a context. +- **Combining with `-f`/`--values`:** the two are orthogonal, not ranked against each other. + `--config` knobs become `--set` flags; `-f` is a raw values file. Helm applies `--set` + after `--values`, so a `--config` knob always wins over the same key in a `-f` file with + no extra merge logic needed. Implemented: `helm_install` uses a single code path with an + optional `values_flag` (`--values`, only when `-f` is given) alongside `extra_set_flags` + (which now includes the registry `--set`s too, gated off only in pure `-f`-only mode) — + so `--config` and `-f` can be passed together, matching how `extra_set_flags` already + layered on top of `-f` for versions/components/otel/ingress even before this change. + +### Phase 4 — Pre-install validators (fail fast) — done + +Implemented as designed, with one addition and one scope note: + +- **Helm CLI version check added:** the original draft only listed the K8s version floor + as blocking. Since §6 also resolved a Helm CLI floor (>= 4.1) for the same docs page, + `validate_helm_version()` was added as a third blocking check alongside K8s version and + StorageClass — same shape, same rationale (docs-level requirement, not chart-enforced). +- **Node capacity scoped to RAM + storage, no CPU floor:** the bullet above says + "CPU/mem above a documented floor," but the only actual floor resolved in §6 is "≥8Gi + RAM / 8Gi storage" — no CPU number exists in the docs to check against. Implemented + `validate_node_capacity()` against the RAM/storage floor only. +- **Aggregate-then-report, not fail-at-first-check:** the three blocking checks + (`validate_k8s_version`, `validate_helm_version`, `validate_storage_class`) `return 1` + instead of calling `exit 1` directly, so `run_validators()` runs every check (blocking + and warning) and reports all problems in one pass before exiting 1 — a deliberate, + one-off deviation from the rest of the script's inline `log_error; exit 1` style, chosen + so a user fixing pre-flight issues doesn't have to re-run the installer once per problem. +- **Bug found via live testing against `docker-desktop`:** `validate_node_capacity()`'s + quantity parser only handled `Ki`-suffixed values (memory's format); this cluster's real + `.status.allocatable.ephemeral-storage` is a bare byte integer with no suffix, so the + check silently read it as 0Gi. Fixed with a small `_allocatable_to_ki()` helper that + handles `Ki`/`Mi`/`Gi`/`Ti` suffixes and the bare-byte-integer form. +- **Supporting fix in `create_registry_secret`:** its `username`/`password`/`server` were + locals invisible outside the function once resolved interactively (only + `REGISTRY_USERNAME_VALUE` existed as a side-channel export). Added the same-shaped + `REGISTRY_PASSWORD_VALUE`/`REGISTRY_SERVER_VALUE` so `validate_registry_auth` can + actually see what was entered, regardless of source (env, `REGISTRY_PASSWORD_FILE`, or + prompt). + +Checks, as implemented: + +> **Superseded:** the two version floors below were later realigned to the chart's own +> prerequisites — Helm >= 3.6 blocking, and no Kubernetes floor at all (the check is +> informational). See `scripts/AGENTS.md` "Version floors realigned" and +> `docs/configuration.md` "Version floors". The rest of this section still holds. + +- **K8s version** ≥ 1.34 (blocking) +- **Helm CLI version** ≥ 4.1 (blocking) +- **Default StorageClass** exists (blocking) +- **Registry auth** — best-effort `docker login` with provided creds (warning; skipped + under `--local-registry` or when no credentials were resolved yet, e.g. `-f`-only mode) +- **NodePort conflicts** — chart's fixed NodePorts (30010/20/40/50/60/70, 30093/94, 30100, 30110) + not already bound by a Service outside the target namespace (warning) +- **Node capacity** — allocatable RAM/ephemeral-storage above 8Gi/8Gi (warning) + +New flag/env: `--skip-validators` / `SKIP_VALIDATORS`, mirroring `--skip-secret`'s exact +pattern. `run_validators()` is called from `main()` unconditionally (in every mode, +including pure `-f`-only) right before `helm_install`. + +### Phase 5 — dropped (folded into `--enable-ingress` directly) + +The original idea was a UI-ingress toggle plus a controller story. Revisiting after Phase +4 shipped: `--enable-ingress` already flips `mlrun.ui.ingress.enabled` (and +`jupyterNotebook`/`nuclio.dashboard`/`mlrun.api` Ingress alongside it) — there was no +separate chart-value toggle left to add. The only real gap was that `--enable-ingress` +used to `helm install` the actual `ingress-nginx` controller itself +(`install_ingress_controller()`) — installing a third-party controller is more than this +installer should be doing on a user's behalf. Fixed directly on the existing flag instead +of as a new phase: + +- **Removed** `install_ingress_controller()` and its call in `main()`. `--enable-ingress` + is now BYO-controller-only — it configures the chart's Ingress resources via `--set`, + nothing else. +- **Added** `validate_ingress_controller()` to Phase 4's `run_validators` (warning-only, + no-op when `--enable-ingress` isn't used): checks `kubectl get ingressclass + ` and warns — doesn't block, doesn't install — if it's missing, so the + Ingress resources not resolving is a known-cause warning instead of a silent mystery. +- **Not built:** `print_ui_ingress_url()` (reading the created Ingress back and printing + its URL in the final access table) and `installer.ingress.*` ce-config.yaml keys (the + CLI/env flag already covers the same knobs). Out of scope unless requested separately. + +### Phase 6 — CI samples (Jenkins + GitHub Actions) +CI does its own `git checkout` (via `actions/checkout` / Jenkins SCM) — the installer never +fetches. To test a `ce` PR, CI checks out that ref and passes `--chart-path`. + +- **`.github/workflows/ce-install.yml`** — `workflow_dispatch` with inputs `ce_ref` + (branch/PR ref to checkout) and `dry_run`; steps: checkout this repo → checkout `mlrun/ce` + at `ce_ref` → `azure/setup-helm` → install `yq` → `helm/kind-action` → run `install.sh` + with `CI=true`, `REGISTRY_*` from `secrets`, and `--chart-path` to the checked-out chart. + A separate `pull_request` job runs `bash -n` + `shellcheck` only. +- **`Jenkinsfile`** — parameters (CE_REF, DRY_RUN), `withCredentials` for the registry, + `post { failure { sh './install.sh --uninstall || true' } }` teardown. + +--- + +## 4. Files touched / added + +| File | Change | +|---|---| +| `install.sh` | Phases 1-4: new vars, `resolve_chart_source`, `load_config` (+existence/empty-field validation), `run_validators`; edits to `parse_args`, `prompt_or_env`, `helm_install`, `main`, `usage` | +| `ce-config.yaml.example` | New — documented sample config | +| `.github/workflows/ce-install.yml` | New — GitHub Actions | +| `Jenkinsfile` | New — Jenkins pipeline | +| `README.md` | Document `--config`, `--chart-path`, validators, CI usage | + +## 5. Verification + +1. **Static:** `bash -n install.sh` + `shellcheck install.sh`. +2. **Help:** `./install.sh --help` lists all new flags. +3. **Config validation:** + - `./install.sh --config missing.yaml` → clear "file not found" error, exit 1. + - Config with an empty required field → error naming the empty field(s), exit 1. +4. **Dry-run (needs kind/minikube/docker-desktop):** + - `./install.sh --dry-run` — renders published chart, no deploy. + - `./install.sh --ce-version 0.11.0 --dry-run` — confirms `--version` is passed. + - `./scripts/install.sh --chart-path ./charts/mlrun-ce --dry-run` — `helm dependency update` + + renders from the local chart directory. + - `./install.sh --config ce-config.yaml.example --dry-run` — `installer:` block consumed + and stripped; registry falls back to YAML when not passed on the CLI. +5. **Ingress (opt-in, BYO controller):** `./install.sh --enable-ingress` on a cluster with + an existing IngressClass → no warning, `kubectl get ingress` shows `mlrun-ui` etc. On a + cluster with no matching IngressClass → `validate_ingress_controller` warns but the + install still proceeds (no controller ever gets installed by this script). +6. **Non-interactive:** `CI=true ./install.sh` with a required var missing → clean exit 1 (no hang). +7. **CI:** trigger the GH Actions `workflow_dispatch` with a `ce_ref`; run the parameterized Jenkins build. + +## 6. Open questions for reviewers — resolved + +- **Is `yq` an acceptable dependency for the config path?** Yes, with conditions. `yq` + (mikefarah/yq, Go) makes no network calls at runtime, so runtime CVEs are limited to + YAML-parsing bugs — but the *installer* (or a CI image) fetching the `yq` binary is a + supply-chain vector like any curl-installed tool. Mitigate: pin an exact released + version, verify its checksum (GitHub releases publish `checksums`), and prefer an + already-present `yq` (package manager / CI base image) over installing one at runtime. + `load_config()` should hard-require `yq` only when `--config` is passed — no new + dependency for users who don't use the config file (matches existing "quick wins first, + no behavior change" principle). +- **Minimum supported K8s version to enforce in the validator (Phase 4)?** Per the + official install docs (docs.mlrun.org, `install-mlrun-ce/kubernetes-install.md`): + **Kubernetes >= 1.34**, **Helm >= 4.1** CLI, a default StorageClass, and >= 8Gi RAM / + 8Gi storage available. The chart itself sets no `kubeVersion` in `Chart.yaml` — the + version floor is a docs-level requirement, not chart-enforced, so `run_validators()` + is the only place it's actually checked today. +- **Which fields count as required (non-empty) in `ce-config.yaml`?** Cross-checked + against what `install.sh` already hard-enforces (`create_registry_secret`, + `gather_install_params` in the current `main`, ~lines 369-467): + - `installer.registry.url` — required, unless `installer.local` registry mode is used + (mirrors `REGISTRY_URL` today, install.sh:461-464). + - `installer.registry.secret.username` — required (mirrors `REGISTRY_USERNAME`, + install.sh:395-398). + - `installer.registry.secret.password` — required, but **never read from the file** — + only from `REGISTRY_PASSWORD` env or the interactive prompt (already a design + decision in §3; the password is checked non-empty the same way the username is). + - `installer.chartSource.chartPath` — required only when `installer.chartSource.kind: + path`. + - Everything else in the schema (`registry.secret.server` — has a working default; + `registry.secret.email`; `chartVersion`; `kubeContext`; `externalHostAddress`; + `ingress.*`; `components.*`) is optional, matching that these either have defaults + or are opt-in features today. +- **Any additional NodePorts/components to validate beyond the chart defaults listed in + Phase 4?** No — no additional ports/components identified; the list in Phase 4 stands + as-is. \ No newline at end of file diff --git a/tests/install_tests.bats b/tests/install_tests.bats new file mode 100644 index 00000000..12bb39e8 --- /dev/null +++ b/tests/install_tests.bats @@ -0,0 +1,1383 @@ +#!/usr/bin/env bats +# Tests for install.sh — Phase 1 (flag parsing, dry-run, non-interactive, CI), +# Phase 2 (--chart-path / resolve_chart_source), and Phase 3 (--config / load_config). +# Uses INSTALL_SH_SOURCE_ONLY=true so sourcing the script only defines +# functions and globals without calling main(). + +SCRIPT="$BATS_TEST_DIRNAME/../scripts/install.sh" + +# Source helper: load the script without running main(). +# Usage: _load [extra env assignments...] +_src() { + # shellcheck source=/dev/null + INSTALL_SH_SOURCE_ONLY=true source "$SCRIPT" +} + +# --------------------------------------------------------------------------- +# --ce-version parsing +# --------------------------------------------------------------------------- + +@test "--ce-version stores the supplied value" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --ce-version 0.11.0 + echo \"\$CE_VERSION\" + " + [ "$status" -eq 0 ] + [ "$output" = "0.11.0" ] +} + +@test "--ce-version with no argument exits 1" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --ce-version + " + [ "$status" -eq 1 ] +} + +@test "CE_VERSION env var pre-set is preserved after sourcing" { + run bash -c " + CE_VERSION=0.10.0 + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + echo \"\$CE_VERSION\" + " + [ "$status" -eq 0 ] + [ "$output" = "0.10.0" ] +} + +@test "CE_VERSION defaults to empty string" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + echo \"'\$CE_VERSION'\" + " + [ "$status" -eq 0 ] + [ "$output" = "''" ] +} + +# --------------------------------------------------------------------------- +# --dry-run flag +# --------------------------------------------------------------------------- + +@test "--dry-run sets DRY_RUN=true" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --dry-run + echo \"\$DRY_RUN\" + " + [ "$status" -eq 0 ] + [ "$output" = "true" ] +} + +@test "DRY_RUN defaults to false" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + echo \"\$DRY_RUN\" + " + [ "$status" -eq 0 ] + [ "$output" = "false" ] +} + +@test "--dry-run flag is consumed (no leftover args)" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + # parse_args silently drops unknown args; DRY_RUN must be set + parse_args --dry-run + [ \"\$DRY_RUN\" = 'true' ] && echo ok + " + [ "$status" -eq 0 ] + [ "$output" = "ok" ] +} + +# --------------------------------------------------------------------------- +# --non-interactive flag +# --------------------------------------------------------------------------- + +@test "--non-interactive sets NON_INTERACTIVE=true" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --non-interactive + echo \"\$NON_INTERACTIVE\" + " + [ "$status" -eq 0 ] + [ "$output" = "true" ] +} + +@test "NON_INTERACTIVE defaults to false" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + echo \"\$NON_INTERACTIVE\" + " + [ "$status" -eq 0 ] + [ "$output" = "false" ] +} + +# --------------------------------------------------------------------------- +# prompt_or_env behaviour in non-interactive mode +# --------------------------------------------------------------------------- + +@test "prompt_or_env returns default when NON_INTERACTIVE=true and var is unset" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + NON_INTERACTIVE=true + result=\$(prompt_or_env SOME_UNSET_VAR 'Label' 'thedefault') + echo \"\$result\" + " + [ "$status" -eq 0 ] + [ "$output" = "thedefault" ] +} + +@test "prompt_or_env exits 1 when NON_INTERACTIVE=true, var unset, and no default" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + NON_INTERACTIVE=true + prompt_or_env SOME_UNSET_VAR 'Label' '' + " + [ "$status" -eq 1 ] +} + +@test "prompt_or_env returns the env var even when NON_INTERACTIVE=true" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + NON_INTERACTIVE=true + MY_VAR=fromenv + result=\$(prompt_or_env MY_VAR 'Label' '') + echo \"\$result\" + " + [ "$status" -eq 0 ] + [ "$output" = "fromenv" ] +} + +@test "prompt_or_env env var takes priority over default in non-interactive mode" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + NON_INTERACTIVE=true + MY_VAR=fromenv + result=\$(prompt_or_env MY_VAR 'Label' 'thedefault') + echo \"\$result\" + " + [ "$status" -eq 0 ] + [ "$output" = "fromenv" ] +} + +# --------------------------------------------------------------------------- +# CI auto-detect in main() +# --------------------------------------------------------------------------- + +@test "CI=true sets NON_INTERACTIVE before any work is done" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + + # Stub everything main() calls so we don't need a live cluster or registry creds + check_requirements() { :; } + ensure_namespace() { :; } + create_registry_secret() { :; } + gather_install_params() { :; } + run_validators() { :; } + helm_install() { echo \"NON_INTERACTIVE=\$NON_INTERACTIVE\"; } + + CI=true main + " + [ "$status" -eq 0 ] + [[ "$output" == *"NON_INTERACTIVE=true"* ]] +} + +@test "CI unset leaves NON_INTERACTIVE false" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + + check_requirements() { :; } + ensure_namespace() { :; } + create_registry_secret() { :; } + gather_install_params() { :; } + run_validators() { :; } + helm_install() { echo \"NON_INTERACTIVE=\$NON_INTERACTIVE\"; } + + unset CI + main + " + [ "$status" -eq 0 ] + [[ "$output" == *"NON_INTERACTIVE=false"* ]] +} + +# --------------------------------------------------------------------------- +# Phase 2 — --chart-path flag and resolve_chart_source +# --------------------------------------------------------------------------- + +@test "--chart-path stores the supplied path" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --chart-path ./ce/charts/mlrun-ce + echo \"\$CHART_PATH\" + " + [ "$status" -eq 0 ] + [ "$output" = "./ce/charts/mlrun-ce" ] +} + +@test "--chart-path with no argument exits 1" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --chart-path + " + [ "$status" -eq 1 ] +} + +@test "--chart-path rejects a flag-looking value" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --chart-path --dry-run + " + [ "$status" -eq 1 ] +} + +@test "CHART_PATH defaults to empty string" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + echo \"'\$CHART_PATH'\" + " + [ "$status" -eq 0 ] + [ "$output" = "''" ] +} + +@test "resolve_chart_source exits 1 when CHART_PATH directory does not exist" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CHART_PATH=/nonexistent/path + resolve_chart_source + " + [ "$status" -eq 1 ] +} + +@test "resolve_chart_source exits 1 when CHART_PATH has no Chart.yaml" { + local tmpdir + tmpdir="\$(mktemp -d)" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CHART_PATH='$BATS_TMPDIR/emptychart' + mkdir -p \"\$CHART_PATH\" + resolve_chart_source + " + [ "$status" -eq 1 ] +} + +@test "resolve_chart_source sets CHART_REF to CHART_PATH when valid" { + local chart_dir="$BATS_TMPDIR/fakechart" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { :; } + CHART_PATH='$chart_dir' + resolve_chart_source 2>/dev/null + echo \"\$CHART_REF\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"$chart_dir"* ]] +} + +@test "resolve_chart_source sets CHART_REF to mlrun-ce/mlrun-ce in published-repo mode" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { :; } + CHART_PATH= + resolve_chart_source 2>/dev/null + echo \"\$CHART_REF\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"mlrun-ce/mlrun-ce"* ]] +} + +@test "--ce-version is ignored in local-path mode (version_flag stays empty)" { + local chart_dir="$BATS_TMPDIR/fakechart2" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { :; } + CHART_PATH='$chart_dir' + CE_VERSION=0.11.0 + check_version_flag() { + local -a vf=() + [[ -n \"\$CE_VERSION\" && -z \"\$CHART_PATH\" ]] && vf=(--version \"\$CE_VERSION\") + echo \"count=\${#vf[@]}\" + } + check_version_flag + " + [ "$status" -eq 0 ] + [[ "$output" == *"count=0"* ]] +} + +# --------------------------------------------------------------------------- +# Phase 3 — --config / CONFIG_FILE and load_config +# --------------------------------------------------------------------------- + +@test "--config stores the supplied path" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --config ce-config.yaml + echo \"\$CONFIG_FILE\" + " + [ "$status" -eq 0 ] + [ "$output" = "ce-config.yaml" ] +} + +@test "--config with no argument exits 1" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --config + " + [ "$status" -eq 1 ] +} + +@test "load_config is a no-op (returns 0, no yq required) when CONFIG_FILE is empty" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + PATH=/usr/bin:/bin + load_config + echo done + " + [ "$status" -eq 0 ] + [[ "$output" == *"done"* ]] +} + +@test "load_config exits 1 when CONFIG_FILE does not exist" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE=/nonexistent/ce-config.yaml + load_config + " + [ "$status" -eq 1 ] + [[ "$output" == *"not found"* ]] +} + +@test "load_config exits 1 when yq is not installed" { + local cfg="$BATS_TMPDIR/cfg_noyq.yaml" + printf 'installer:\n registry:\n url: x\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + PATH=/usr/bin:/bin + load_config + " + [ "$status" -eq 1 ] + [[ "$output" == *"yq"* ]] +} + +@test "load_config parses registry fields into CONFIG_ vars" { + local cfg="$BATS_TMPDIR/cfg_registry.yaml" + cat > "$cfg" <<'EOF' +installer: + registry: + url: index.docker.io/myuser + secret: + username: myuser + server: https://index.docker.io/v1/ + email: me@example.com +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + echo \"url=\$CONFIG_REGISTRY_URL user=\$CONFIG_REGISTRY_USERNAME server=\$CONFIG_REGISTRY_SERVER email=\$CONFIG_REGISTRY_EMAIL\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"url=index.docker.io/myuser"* ]] + [[ "$output" == *"user=myuser"* ]] + [[ "$output" == *"server=https://index.docker.io/v1/"* ]] + [[ "$output" == *"email=me@example.com"* ]] +} + +@test "in interactive mode, a config-file value pre-fills the prompt default (Enter accepts it)" { + local cfg="$BATS_TMPDIR/cfg_interactive.yaml" + cat > "$cfg" <<'EOF' +installer: + registry: + url: index.docker.io/configuser + secret: + username: configuser +EOF + # Empty stdin simulates pressing Enter at the prompt with no typed override. + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + prompt_or_env REGISTRY_URL 'Docker registry URL' \"\${CONFIG_REGISTRY_URL}\" <<< '' + " + [ "$status" -eq 0 ] + [ "$output" = "index.docker.io/configuser" ] +} + +@test "in interactive mode, typing a value overrides the config-file default for that field" { + local cfg="$BATS_TMPDIR/cfg_interactive_override.yaml" + cat > "$cfg" <<'EOF' +installer: + registry: + url: index.docker.io/configuser + secret: + username: configuser +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + prompt_or_env REGISTRY_URL 'Docker registry URL' \"\${CONFIG_REGISTRY_URL}\" <<< 'index.docker.io/typeduser' + " + [ "$status" -eq 0 ] + [ "$output" = "index.docker.io/typeduser" ] +} + +@test "load_config warns and ignores a password key in the config file" { + local cfg="$BATS_TMPDIR/cfg_password.yaml" + cat > "$cfg" <<'EOF' +installer: + registry: + secret: + username: myuser + password: shouldbeignored +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + " + [ "$status" -eq 0 ] + [[ "$output" == *"ignored"* ]] +} + +@test "load_config exits 1 immediately when chartSource.kind is path but chartPath is empty (interactive mode too)" { + local cfg="$BATS_TMPDIR/cfg_path_missing.yaml" + cat > "$cfg" <<'EOF' +installer: + chartSource: + kind: path +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + NON_INTERACTIVE=false + load_config + " + [ "$status" -eq 1 ] + [[ "$output" == *"chartSource.chartPath"* ]] +} + +@test "load_config sets CHART_PATH from chartSource.chartPath when kind is path" { + local cfg="$BATS_TMPDIR/cfg_path_ok.yaml" + cat > "$cfg" <<'EOF' +installer: + chartSource: + kind: path + chartPath: /some/chart/dir +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + echo \"\$CHART_PATH\" + " + [ "$status" -eq 0 ] + [ "$output" = "/some/chart/dir" ] +} + +@test "load_config lists every missing required field together in non-interactive mode" { + local cfg="$BATS_TMPDIR/cfg_empty.yaml" + printf 'installer:\n registry:\n url: ""\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + NON_INTERACTIVE=true + load_config + " + [ "$status" -eq 1 ] + [[ "$output" == *"installer.registry.url"* ]] + [[ "$output" == *"installer.registry.secret.username"* ]] + [[ "$output" == *"REGISTRY_PASSWORD"* ]] +} + +@test "load_config does not require registry.url in non-interactive mode when LOCAL_REGISTRY=true" { + local cfg="$BATS_TMPDIR/cfg_local.yaml" + printf 'installer:\n registry:\n secret:\n username: myuser\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + NON_INTERACTIVE=true + LOCAL_REGISTRY=true + REGISTRY_PASSWORD=x + load_config + echo ok + " + [ "$status" -eq 0 ] + [[ "$output" == *"ok"* ]] +} + +@test "load_config does not require registry username/password in non-interactive mode when --skip-secret is used" { + local cfg="$BATS_TMPDIR/cfg_skipsecret.yaml" + printf 'installer:\n registry:\n url: myregistry.example.com/user\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + NON_INTERACTIVE=true + SKIP_REGISTRY_SECRET=true + load_config + echo ok + " + [ "$status" -eq 0 ] + [[ "$output" == *"ok"* ]] +} + +@test "load_config still requires registry fields in non-interactive mode when --config is combined with -f/VALUES_FILE" { + local cfg="$BATS_TMPDIR/cfg_valuesfile.yaml" + printf 'installer: {}\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + NON_INTERACTIVE=true + VALUES_FILE=/some/values.yaml + load_config + " + [ "$status" -eq 1 ] + [[ "$output" == *"installer.registry.url"* ]] +} + +@test "load_config is a no-op when -f/VALUES_FILE is used without --config (CONFIG_FILE empty)" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + NON_INTERACTIVE=true + VALUES_FILE=/some/values.yaml + load_config + echo ok + " + [ "$status" -eq 0 ] + [[ "$output" == *"ok"* ]] +} + +@test "load_config does not override CHART_PATH/CE_VERSION/KUBE_CONTEXT already set by flag or env" { + local cfg="$BATS_TMPDIR/cfg_precedence.yaml" + cat > "$cfg" <<'EOF' +installer: + kubeContext: from-config + chartSource: + kind: path + chartPath: /from/config + chartVersion: 9.9.9 +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + KUBE_CONTEXT=from-env + CHART_PATH=/from/flag + CE_VERSION=1.2.3 + load_config + echo \"ctx=\$KUBE_CONTEXT path=\$CHART_PATH version=\$CE_VERSION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"ctx=from-env"* ]] + [[ "$output" == *"path=/from/flag"* ]] + [[ "$output" == *"version=1.2.3"* ]] +} + +@test "load_config parses installer.versions.mlrun/nuclio into MLRUN_VERSION/NUCLIO_VERSION" { + local cfg="$BATS_TMPDIR/cfg_versions.yaml" + cat > "$cfg" <<'EOF' +installer: + versions: + mlrun: 1.13.0 + nuclio: 1.16.0 +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + echo \"mlrun=\$MLRUN_VERSION nuclio=\$NUCLIO_VERSION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"mlrun=1.13.0"* ]] + [[ "$output" == *"nuclio=1.16.0"* ]] +} + +@test "helm_install passes mlrun/nuclio image.tag --set flags to helm when MLRUN_VERSION/NUCLIO_VERSION are set" { + local chart_dir="$BATS_TMPDIR/fakechart3" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo \"HELM_ARGS: \$*\"; } + CHART_PATH='$chart_dir' + MLRUN_VERSION=1.13.0 + NUCLIO_VERSION=1.16.0 + REGISTRY_URL=x + EXTERNAL_HOST_ADDRESS=x + DRY_RUN=true + helm_install + " + [ "$status" -eq 0 ] + [[ "$output" == *"mlrun.api.image.tag=1.13.0"* ]] + [[ "$output" == *"mlrun.ui.image.tag=1.13.0"* ]] + [[ "$output" == *"mlrun.api.sidecars.logCollector.image.tag=1.13.0"* ]] + [[ "$output" == *"nuclio.controller.image.tag=1.16.0"* ]] + [[ "$output" == *"nuclio.dashboard.image.tag=1.16.0"* ]] +} + +@test "load_config maps installer.components.* to DISABLE_* the same as the --disable-* flags" { + local cfg="$BATS_TMPDIR/cfg_components.yaml" + cat > "$cfg" <<'EOF' +installer: + components: + monitoring: false + spark: true + mpi: false + modelMonitoring: true +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + echo \"mon=\$DISABLE_SYSTEM_MONITORING spark=\$DISABLE_SPARK mpi=\$DISABLE_MPI mm=\$DISABLE_MODEL_MONITORING\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"mon=true"* ]] + [[ "$output" == *"spark=false"* ]] + [[ "$output" == *"mpi=true"* ]] + [[ "$output" == *"mm=false"* ]] +} + +@test "load_config does not re-enable a component already disabled by --disable-* flag/env" { + local cfg="$BATS_TMPDIR/cfg_components_enable.yaml" + printf 'installer:\n components:\n spark: true\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + DISABLE_SPARK=true + load_config + echo \"spark=\$DISABLE_SPARK\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"spark=true"* ]] +} + +@test "--enable-otel sets all 4 granular ENABLE_OTEL_* vars" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel + echo \"op=\$ENABLE_OTEL_OPERATOR col=\$ENABLE_OTEL_COLLECTOR ns=\$ENABLE_OTEL_NAMESPACE_LABEL inst=\$ENABLE_OTEL_INSTRUMENTATION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=true col=true ns=true inst=true"* ]] +} + +@test "--enable-otel-operator/-collector/-namespace-label/-instrumentation set only their own var" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel-collector + echo \"op=\$ENABLE_OTEL_OPERATOR col=\$ENABLE_OTEL_COLLECTOR ns=\$ENABLE_OTEL_NAMESPACE_LABEL inst=\$ENABLE_OTEL_INSTRUMENTATION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=false col=true ns=false inst=false"* ]] +} + +@test "--enable-otel full sets all 4 (same as bare --enable-otel)" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel full + echo \"op=\$ENABLE_OTEL_OPERATOR col=\$ENABLE_OTEL_COLLECTOR ns=\$ENABLE_OTEL_NAMESPACE_LABEL inst=\$ENABLE_OTEL_INSTRUMENTATION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=true col=true ns=true inst=true"* ]] +} + +@test "--enable-otel collector sets only operator+collector, not namespaceLabel/instrumentation" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel collector + echo \"op=\$ENABLE_OTEL_OPERATOR col=\$ENABLE_OTEL_COLLECTOR ns=\$ENABLE_OTEL_NAMESPACE_LABEL inst=\$ENABLE_OTEL_INSTRUMENTATION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=true col=true ns=false inst=false"* ]] +} + +@test "--enable-otel off sets all 4 false" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel off + echo \"op=\$ENABLE_OTEL_OPERATOR col=\$ENABLE_OTEL_COLLECTOR ns=\$ENABLE_OTEL_NAMESPACE_LABEL inst=\$ENABLE_OTEL_INSTRUMENTATION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=false col=false ns=false inst=false"* ]] +} + +@test "--enable-otel rejects an invalid mode" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel bogus + " + [ "$status" -eq 1 ] + [[ "$output" == *"Invalid --enable-otel mode"* ]] +} + +@test "--enable-otel with no mode does not consume the next unrelated flag" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel --dry-run + echo \"op=\$ENABLE_OTEL_OPERATOR dry=\$DRY_RUN\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=true dry=true"* ]] +} + +@test "load_config maps installer.otel.* to the 4 granular ENABLE_OTEL_* vars independently (opt-in, opposite direction from components.*)" { + local cfg="$BATS_TMPDIR/cfg_otel.yaml" + cat > "$cfg" <<'EOF' +installer: + otel: + operator: true + collector: true + namespaceLabel: false + instrumentation: false +EOF + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + load_config + echo \"op=\$ENABLE_OTEL_OPERATOR col=\$ENABLE_OTEL_COLLECTOR ns=\$ENABLE_OTEL_NAMESPACE_LABEL inst=\$ENABLE_OTEL_INSTRUMENTATION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=true col=true ns=false inst=false"* ]] +} + +@test "load_config does not override an ENABLE_OTEL_* var already set by flag/env when the file omits/disables it" { + local cfg="$BATS_TMPDIR/cfg_otel_noop.yaml" + printf 'installer:\n otel:\n operator: false\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + ENABLE_OTEL_OPERATOR=true + load_config + echo \"op=\$ENABLE_OTEL_OPERATOR\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=true"* ]] +} + +@test "helm_install passes only the --set flags for the otel toggles that are enabled" { + local chart_dir="$BATS_TMPDIR/fakechart_otel" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo \"HELM_ARGS: \$*\"; } + CHART_PATH='$chart_dir' + ENABLE_OTEL_OPERATOR=true + ENABLE_OTEL_COLLECTOR=true + REGISTRY_URL=x + EXTERNAL_HOST_ADDRESS=x + DRY_RUN=true + helm_install + " + [ "$status" -eq 0 ] + [[ "$output" == *"opentelemetry-operator.enabled=true"* ]] + [[ "$output" == *"opentelemetry.collector.enabled=true"* ]] + [[ "$output" != *"opentelemetry.namespaceLabel.enabled"* ]] + [[ "$output" != *"opentelemetry.instrumentation.enabled"* ]] +} + +@test "helm_install passes all 4 opentelemetry --set flags when all ENABLE_OTEL_* vars are true" { + local chart_dir="$BATS_TMPDIR/fakechart_otel_all" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo \"HELM_ARGS: \$*\"; } + CHART_PATH='$chart_dir' + ENABLE_OTEL_OPERATOR=true + ENABLE_OTEL_COLLECTOR=true + ENABLE_OTEL_NAMESPACE_LABEL=true + ENABLE_OTEL_INSTRUMENTATION=true + REGISTRY_URL=x + EXTERNAL_HOST_ADDRESS=x + DRY_RUN=true + helm_install + " + [ "$status" -eq 0 ] + [[ "$output" == *"opentelemetry-operator.enabled=true"* ]] + [[ "$output" == *"opentelemetry.collector.enabled=true"* ]] + [[ "$output" == *"opentelemetry.namespaceLabel.enabled=true"* ]] + [[ "$output" == *"opentelemetry.instrumentation.enabled=true"* ]] +} + +@test "main() with --config and -f/--values together still creates the registry secret (composition, not exclusivity)" { + local cfg="$BATS_TMPDIR/cfg_combo.yaml" + printf 'installer:\n registry:\n url: index.docker.io/myuser\n secret:\n username: myuser\n' > "$cfg" + local values="$BATS_TMPDIR/values_combo.yaml" + printf 'foo: bar\n' > "$values" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + check_requirements() { :; } + ensure_namespace() { :; } + create_registry_secret() { echo CREATE_SECRET_CALLED; } + gather_install_params() { echo GATHER_PARAMS_CALLED; } + run_validators() { :; } + helm_install() { :; } + REGISTRY_PASSWORD=x + main --config '$cfg' -f '$values' --non-interactive + " + [ "$status" -eq 0 ] + [[ "$output" == *"CREATE_SECRET_CALLED"* ]] + [[ "$output" == *"GATHER_PARAMS_CALLED"* ]] +} + +@test "main() with -f/--values alone (no --config) still skips secret creation (backward compat)" { + local values="$BATS_TMPDIR/values_alone.yaml" + printf 'foo: bar\n' > "$values" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + check_requirements() { :; } + ensure_namespace() { :; } + create_registry_secret() { echo CREATE_SECRET_CALLED; } + gather_install_params() { echo GATHER_PARAMS_CALLED; } + run_validators() { :; } + helm_install() { :; } + main -f '$values' + " + [ "$status" -eq 0 ] + [[ "$output" != *"CREATE_SECRET_CALLED"* ]] + [[ "$output" != *"GATHER_PARAMS_CALLED"* ]] +} + +@test "helm_install includes --values AND config-resolved registry --set flags when CONFIG_FILE is set alongside VALUES_FILE" { + local chart_dir="$BATS_TMPDIR/fakechart_combo" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + local values="$BATS_TMPDIR/values_helm_combo.yaml" + printf 'foo: bar\n' > "$values" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo \"HELM_ARGS: \$*\"; } + CHART_PATH='$chart_dir' + CONFIG_FILE=dummy.yaml + VALUES_FILE='$values' + REGISTRY_URL=index.docker.io/myuser + REGISTRY_SECRET_NAME=registry-credentials + EXTERNAL_HOST_ADDRESS=localhost + DRY_RUN=true + helm_install + " + [ "$status" -eq 0 ] + [[ "$output" == *"--values ${values}"* ]] + [[ "$output" == *"global.registry.url=index.docker.io/myuser"* ]] + [[ "$output" == *"global.registry.secretName=registry-credentials"* ]] + [[ "$output" == *"global.externalHostAddress=localhost"* ]] +} + +@test "helm_install omits registry --set flags in pure -f-only mode (no CONFIG_FILE) — backward compat" { + local chart_dir="$BATS_TMPDIR/fakechart_fonly" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + local values="$BATS_TMPDIR/values_fonly.yaml" + printf 'foo: bar\n' > "$values" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo \"HELM_ARGS: \$*\"; } + CHART_PATH='$chart_dir' + VALUES_FILE='$values' + DRY_RUN=true + helm_install + " + [ "$status" -eq 0 ] + [[ "$output" == *"--values ${values}"* ]] + [[ "$output" != *"global.registry.url"* ]] + [[ "$output" != *"global.registry.secretName"* ]] + [[ "$output" != *"global.externalHostAddress"* ]] +} + +@test "create_registry_secret reads the password from REGISTRY_PASSWORD_FILE when REGISTRY_PASSWORD is unset" { + local pwfile="$BATS_TMPDIR/pw.txt" + printf 'supersecret\n' > "$pwfile" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + load_config + REGISTRY_USERNAME=myuser + REGISTRY_PASSWORD_FILE='$pwfile' + DRY_RUN=true + create_registry_secret + echo \"pw=\$REGISTRY_PASSWORD\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"pw=supersecret"* ]] +} + +@test "REGISTRY_PASSWORD env wins over REGISTRY_PASSWORD_FILE when both are set" { + local pwfile="$BATS_TMPDIR/pw2.txt" + printf 'fromfile\n' > "$pwfile" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + load_config + REGISTRY_USERNAME=myuser + REGISTRY_PASSWORD=fromenv + REGISTRY_PASSWORD_FILE='$pwfile' + DRY_RUN=true + create_registry_secret + echo \"pw=\$REGISTRY_PASSWORD\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"pw=fromenv"* ]] +} + +@test "create_registry_secret exits 1 with a clear error when REGISTRY_PASSWORD_FILE does not exist" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + load_config + REGISTRY_USERNAME=myuser + REGISTRY_PASSWORD_FILE='/nonexistent/path/pw.txt' + DRY_RUN=true + create_registry_secret + " + [ "$status" -eq 1 ] + [[ "$output" == *"REGISTRY_PASSWORD_FILE"* ]] +} + +@test "load_config's non-interactive required-field check accepts REGISTRY_PASSWORD_FILE as satisfying the password requirement" { + local pwfile="$BATS_TMPDIR/pw3.txt" + printf 'x\n' > "$pwfile" + local cfg="$BATS_TMPDIR/cfg_pwfile.yaml" + printf 'installer:\n registry:\n url: index.docker.io/myuser\n secret:\n username: myuser\n' > "$cfg" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + CONFIG_FILE='$cfg' + NON_INTERACTIVE=true + REGISTRY_PASSWORD_FILE='$pwfile' + load_config + echo ok + " + [ "$status" -eq 0 ] + [[ "$output" == *"ok"* ]] +} + +@test "verify_existing_registry_secret exits 1 when the secret does not exist" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { return 1; } + REGISTRY_SECRET_NAME=registry-credentials + NAMESPACE=mlrun + verify_existing_registry_secret + " + [ "$status" -eq 1 ] + [[ "$output" == *"does not exist"* ]] +} + +@test "verify_existing_registry_secret passes silently when the secret exists" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { return 0; } + REGISTRY_SECRET_NAME=registry-credentials + NAMESPACE=mlrun + verify_existing_registry_secret + echo ok + " + [ "$status" -eq 0 ] + [[ "$output" == *"ok"* ]] +} + +@test "kubectl/helm wrapper functions inject --context/--kube-context when KUBE_CONTEXT is set" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + command() { + echo \"real: \$*\" + } + KUBE_CONTEXT=myctx + kubectl get pods + helm list + " + [ "$status" -eq 0 ] + [[ "$output" == *"real: kubectl --context myctx get pods"* ]] + [[ "$output" == *"real: helm --kube-context myctx list"* ]] +} + +@test "resolve_external_host skips the docker-desktop/minikube heuristics when KUBE_CONTEXT is set, uses node IP instead" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + command() { + case \"\$1\" in + kubectl) + shift + if [[ \"\$1\" == config && \"\$2\" == current-context ]]; then + echo docker-desktop + elif [[ \"\$1\" == get && \"\$2\" == node ]]; then + echo '192.168.236.51' + fi + ;; + minikube) return 1 ;; + esac + } + KUBE_CONTEXT=vmdev137 + NON_INTERACTIVE=true + resolve_external_host + echo \"host=\$EXTERNAL_HOST_ADDRESS\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"host=192.168.236.51"* ]] + [[ "$output" != *"host=host.docker.internal"* ]] +} + +@test "resolve_external_host still uses the docker-desktop heuristic when KUBE_CONTEXT is unset (regression)" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + command() { + case \"\$1\" in + kubectl) + shift + [[ \"\$1\" == config && \"\$2\" == current-context ]] && echo docker-desktop + ;; + minikube) return 1 ;; + esac + } + NON_INTERACTIVE=true + resolve_external_host + echo \"host=\$EXTERNAL_HOST_ADDRESS\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"host=host.docker.internal"* ]] +} + +@test "resolve_external_host falls back to localhost when no heuristic matches (e.g. kind/k3d)" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + command() { + case \"\$1\" in + kubectl) return 0 ;; + minikube) return 1 ;; + esac + } + NON_INTERACTIVE=true + resolve_external_host + echo \"host=\$EXTERNAL_HOST_ADDRESS\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"host=localhost"* ]] +} + +# --------------------------------------------------------------------------- +# Phase 4 — pre-install validators (run_validators / --skip-validators) +# --------------------------------------------------------------------------- + +@test "--skip-validators sets SKIP_VALIDATORS" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --skip-validators + echo \"\$SKIP_VALIDATORS\" + " + [ "$status" -eq 0 ] + [ "$output" = "true" ] +} + +@test "validate_k8s_version never blocks: no floor is enforced by default" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { echo 'v1.30.2'; } + validate_k8s_version + " + [ "$status" -eq 0 ] + [[ "$output" == *"Kubernetes version: 1.30"* ]] + [[ "$output" != *"below"* ]] +} + +@test "validate_k8s_version reports the detected version when MIN_K8S_VERSION is unset" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { echo 'v1.34.0'; } + validate_k8s_version + " + [ "$status" -eq 0 ] + [[ "$output" == *"Kubernetes version: 1.34"* ]] + [[ "$output" != *"required"* ]] +} + +@test "validate_k8s_version warns and does not fail when the version can't be determined" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { return 1; } + validate_k8s_version + " + [ "$status" -eq 0 ] + [[ "$output" == *"Could not determine Kubernetes version"* ]] +} + +@test "validate_helm_version fails when helm is below the minimum version" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo 'v3.5.0+g12345'; } + validate_helm_version + " + [ "$status" -eq 1 ] + [[ "$output" == *"below the minimum supported version"* ]] +} + +@test "validate_helm_version passes when helm is at the minimum version" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo 'v3.6.0+g12345'; } + validate_helm_version + " + [ "$status" -eq 0 ] + [[ "$output" == *"Helm version: 3.6"* ]] +} + +@test "MIN_K8S_VERSION warns but still does not block when the cluster is below it" { + run bash -c " + MIN_K8S_VERSION=1.34 INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { echo 'v1.30.2'; } + validate_k8s_version + " + [ "$status" -eq 0 ] + [[ "$output" == *"below the requested minimum (1.34)"* ]] +} + +@test "MIN_HELM_VERSION raises the Helm floor the validator enforces" { + run bash -c " + MIN_HELM_VERSION=4.1 INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo 'v3.9.0+g12345'; } + validate_helm_version + " + [ "$status" -eq 1 ] + [[ "$output" == *"below the minimum supported version"* ]] +} + +@test "a malformed MIN_K8S_VERSION is rejected at load time" { + run bash -c " + MIN_K8S_VERSION=1 INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + " + [ "$status" -eq 1 ] + [[ "$output" == *"MIN_K8S_VERSION must be in MAJOR.MINOR form"* ]] +} + +@test "an empty MIN_K8S_VERSION is accepted (no floor is the default)" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + echo \"floor='\$MIN_K8S_VERSION' helm='\$MIN_HELM_VERSION'\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"floor='' helm='3.6'"* ]] +} + +@test "validate_storage_class fails when no default StorageClass exists" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { echo 'standard=false'; } + validate_storage_class + " + [ "$status" -eq 1 ] + [[ "$output" == *"No default StorageClass found"* ]] +} + +@test "validate_storage_class passes when a default StorageClass exists" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { printf 'standard=false\nfast=true\n'; } + validate_storage_class + " + [ "$status" -eq 0 ] + [[ "$output" == *"Default StorageClass: fast"* ]] +} + +@test "validate_ingress_controller skips when --enable-ingress is not used" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { echo 'kubectl should not be called'; return 1; } + ENABLE_INGRESS=false + validate_ingress_controller + " + [ "$status" -eq 0 ] + [[ "$output" != *"should not be called"* ]] +} + +@test "validate_ingress_controller warns (not fails) when no matching IngressClass exists" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { return 1; } + ENABLE_INGRESS=true + INGRESS_CLASS=nginx + validate_ingress_controller + " + [ "$status" -eq 0 ] + [[ "$output" == *"no IngressClass named 'nginx' found"* ]] + [[ "$output" == *"does not install a controller"* ]] +} + +@test "validate_ingress_controller passes when the IngressClass exists" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { return 0; } + ENABLE_INGRESS=true + INGRESS_CLASS=nginx + validate_ingress_controller + " + [ "$status" -eq 0 ] + [[ "$output" == *"IngressClass 'nginx' found"* ]] +} + +@test "validate_registry_auth skips when --local-registry is in use" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + docker() { echo 'docker should not be called'; return 1; } + LOCAL_REGISTRY=true + validate_registry_auth + " + [ "$status" -eq 0 ] + [[ "$output" == *"skipped (--local-registry in use"* ]] + [[ "$output" != *"should not be called"* ]] +} + +@test "validate_registry_auth skips when no credentials were resolved (-f-only mode)" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + docker() { echo 'docker should not be called'; return 1; } + LOCAL_REGISTRY=false + validate_registry_auth + " + [ "$status" -eq 0 ] + [[ "$output" == *"skipped (no registry credentials resolved"* ]] + [[ "$output" != *"should not be called"* ]] +} + +@test "validate_registry_auth warns (not fails) when docker login fails" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + docker() { return 1; } + LOCAL_REGISTRY=false + REGISTRY_USERNAME_VALUE=alice + REGISTRY_PASSWORD_VALUE=secret + REGISTRY_SERVER_VALUE=https://index.docker.io/v1/ + validate_registry_auth + " + [ "$status" -eq 0 ] + [[ "$output" == *"could not log in"* ]] +} + +@test "validate_registry_auth logs success when docker login succeeds" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + docker() { return 0; } + LOCAL_REGISTRY=false + REGISTRY_USERNAME_VALUE=alice + REGISTRY_PASSWORD_VALUE=secret + REGISTRY_SERVER_VALUE=https://index.docker.io/v1/ + validate_registry_auth + " + [ "$status" -eq 0 ] + [[ "$output" == *"login to https://index.docker.io/v1/ succeeded"* ]] +} + +@test "validate_nodeport_conflicts warns when a required NodePort is already in use outside the namespace" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { printf 'other-ns 30093\nmlrun 30010\n'; } + NAMESPACE=mlrun + validate_nodeport_conflicts + " + [ "$status" -eq 0 ] + [[ "$output" == *"NodePort conflict"* ]] + [[ "$output" == *"30093"* ]] + [[ "$output" != *"30010"* ]] +} + +@test "validate_nodeport_conflicts reports no conflicts when required ports are free" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { printf 'other-ns 40000\n'; } + NAMESPACE=mlrun + validate_nodeport_conflicts + " + [ "$status" -eq 0 ] + [[ "$output" == *"no conflicts detected"* ]] +} + +@test "validate_node_capacity warns when allocatable memory/storage is below the 8Gi floor" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { + case \"\$*\" in + *ephemeral-storage*) printf '2000000Ki\n' ;; + *) printf '2000000Ki\n' ;; + esac + } + validate_node_capacity + " + [ "$status" -eq 0 ] + [[ "$output" == *"below the documented floor of 8Gi"* ]] +} + +@test "validate_node_capacity passes when allocatable memory/storage is above the 8Gi floor" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { + case \"\$*\" in + *ephemeral-storage*) printf '9000000Ki\n' ;; + *) printf '9000000Ki\n' ;; + esac + } + validate_node_capacity + " + [ "$status" -eq 0 ] + [[ "$output" == *"total allocatable memory ~8Gi"* ]] + [[ "$output" == *"total allocatable ephemeral storage ~8Gi"* ]] +} + +@test "validate_node_capacity parses a bare byte count (no Ki suffix) for ephemeral-storage" { + # Some clusters (e.g. docker-desktop, cAdvisor-sourced) report ephemeral-storage + # allocatable as a plain byte integer rather than a Ki-suffixed quantity. + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { + case \"\$*\" in + *ephemeral-storage*) printf '56403987978\n' ;; + *) printf '9000000Ki\n' ;; + esac + } + validate_node_capacity + " + [ "$status" -eq 0 ] + [[ "$output" == *"total allocatable ephemeral storage ~52Gi"* ]] + [[ "$output" != *"ephemeral storage ~0Gi"* ]] +} + +@test "run_validators aggregates multiple blocking failures into a single exit 1" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { + case \"\$*\" in + *nodeInfo.kubeletVersion*) echo 'v1.30.0' ;; + *storageclass*) echo '' ;; + *) echo '' ;; + esac + } + helm() { echo 'v3.5.0+g12345'; } + docker() { return 0; } + LOCAL_REGISTRY=false + NAMESPACE=mlrun + run_validators + " + [ "$status" -eq 1 ] + [[ "$output" == *"Helm version 3.5 is below"* ]] + [[ "$output" == *"No default StorageClass"* ]] + [[ "$output" == *"One or more required pre-install checks failed"* ]] + # The cluster is 1.30 but K8s is informational now, so it must not contribute a failure. + [[ "$output" == *"Kubernetes version: 1.30"* ]] +} + +@test "main() skips run_validators entirely when --skip-validators is passed" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { return 0; } + helm() { return 0; } + docker() { return 0; } + check_requirements() { :; } + ensure_namespace() { :; } + create_registry_secret() { :; } + gather_install_params() { :; } + helm_install() { :; } + run_validators() { echo 'run_validators should not be called'; } + NON_INTERACTIVE=true + main --skip-validators + " + [ "$status" -eq 0 ] + [[ "$output" == *"Skipping pre-install validators"* ]] + [[ "$output" != *"should not be called"* ]] +} \ No newline at end of file From 95a959c2ebb245efdcaedad98f844759006c0f5c Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 20 Aug 2026 17:24:30 +0300 Subject: [PATCH 03/15] - Installer CI workflow: lint and unit tests on every PR, plus a workflow_dispatch-only kind end-to-end install --- .github/workflows/installer-ci.yaml | 20 ++++++++------------ scripts/AGENTS.md | 21 ++++++++++++--------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/.github/workflows/installer-ci.yaml b/.github/workflows/installer-ci.yaml index bba62382..02968392 100644 --- a/.github/workflows/installer-ci.yaml +++ b/.github/workflows/installer-ci.yaml @@ -1,17 +1,16 @@ name: Installer CI -# Kept separate from ci.yaml because `paths` filters apply to the whole -# workflow trigger, not to individual jobs — putting this in ci.yaml would -# either skip the helm jobs on chart-only PRs or run this on every PR. +# Runs on every PR, not just those touching scripts/**. The unit tests are +# hermetic and take about a minute, and always running them means the suite +# can't sit broken unnoticed until the next installer change. +# +# Kept as its own workflow rather than a job in ci.yaml so it reports as an +# independent status check, matching this repo's one-workflow-per-concern layout. on: pull_request: branches: - development - "[0-9]+.[0-9]+.x" - paths: - - "scripts/**" - - "tests/install_tests.bats" - - ".github/workflows/installer-ci.yaml" workflow_dispatch: permissions: @@ -28,9 +27,6 @@ jobs: - name: Checkout code uses: actions/checkout@v4 - - name: Syntax check - run: bash -n scripts/install.sh - # shellcheck and yq (mikefarah) ship with the ubuntu-latest runner image. # yq is needed for real: the load_config tests parse actual YAML. - name: Show tool versions @@ -38,8 +34,8 @@ jobs: shellcheck --version yq --version - - name: Run shellcheck - run: shellcheck scripts/install.sh + - name: Lint install.sh + run: make installer-lint - name: Install bats run: | diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index f422b753..b3d32fbe 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -150,15 +150,18 @@ the repo-root `AGENTS.md`/`CONTRIBUTING.md`. This file covers the installer only other original Phase 5 idea — reading the created Ingress back and printing its URL in the final access table) was not built; out of scope unless asked for separately. - **Phase 6 (done, as real CI rather than the originally planned samples):** - `.github/workflows/installer-ci.yaml` runs `bash -n`, shellcheck and the bats suite - (via `make installer-lint`/`make installer-test`) on PRs touching `scripts/**` or - `tests/install_tests.bats`, plus a `workflow_dispatch`-only kind end-to-end install - using `--chart-path ./charts/mlrun-ce --local-registry`. It's a separate workflow file - rather than a job inside `ci.yaml` because GitHub applies `paths:` filters at the - workflow trigger, not per job — folding it into `ci.yaml` would either run it on every - PR or skip the chart jobs on a scripts-only PR. The kind job is dispatch-only for the - same reason `ci.yaml`'s `test:` job is commented out (pulling the full image set is too - slow for every PR). No Jenkinsfile — this repo is GitHub Actions only. + `.github/workflows/installer-ci.yaml` runs `make installer-lint` (`bash -n` + + shellcheck) and `make installer-test` (the bats suite) via the Makefile targets rather + than duplicating the commands, so CI and local can't drift. It runs on **every** PR: an + earlier `paths: scripts/**` filter was dropped because the job takes about a minute and + a filtered job lets the suite rot unnoticed between installer changes. Note the unit + tests are hermetic (they stub `kubectl`/`helm`/`docker`), so running them on chart PRs + does *not* catch chart/installer drift — only the kind job would, and that's + `workflow_dispatch`-only, installing with `--chart-path ./charts/mlrun-ce + --local-registry`, for the same reason `ci.yaml`'s `test:` job is commented out (pulling + the full image set is too slow per-PR). It's a separate workflow file rather than a job + in `ci.yaml` so it reports as an independent status check. No Jenkinsfile — this repo is + GitHub Actions only. ## Phase 3 pre-work (resolved, see docs/design-proposal.md §6) Before implementing Phase 3 (`ce-config.yaml` + `yq`), these open questions were From 259d24b22b5cc50094a3e908fabe60f368672663 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 13:45:10 +0300 Subject: [PATCH 04/15] adding helm timeout. refer to kafka upgrade crush loop --- .claude/skills/run-tests/SKILL.md | 4 ++-- scripts/AGENTS.md | 34 ++++++++++++++++++++++++++- scripts/docs/parameters.md | 1 + scripts/install.sh | 7 ++++++ tests/install_tests.bats | 39 +++++++++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 3 deletions(-) diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md index bbae10f3..bffbe782 100644 --- a/.claude/skills/run-tests/SKILL.md +++ b/.claude/skills/run-tests/SKILL.md @@ -33,7 +33,7 @@ make installer-test bats tests/install_tests.bats ``` -Expected output: `1..96` followed by `ok N ` for every test. +Expected output: `1..98` followed by `ok N ` for every test. Keep the count in this file in sync when you add tests — it's the quickest way to notice a test silently failing to register. @@ -88,7 +88,7 @@ Tests that exercise the validators individually stub `kubectl`/`helm`/`docker` a shell functions instead, echoing whatever the check parses (a `kubeletVersion`, a `helm version --short` string, an allocatable quantity, and so on). -## Current coverage — 96 tests +## Current coverage — 98 tests | Phase / area | Tests | |--------------|-------| diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index b3d32fbe..8b714fe5 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -214,6 +214,20 @@ predating K8s 1.34, so that node image likely wasn't even published for it. ## Known non-bugs +- **CE does not officially support upgrades**, so a `helm upgrade` over an existing release + is out of scope as a supported path. The concrete symptom seen live (0.11.0 → + 0.12.0-rc.11 on the `vmdev137` lab): the Kafka broker crash-loops with + `Invalid cluster.id in: /var/lib/kafka/data/kafka-log0/meta.properties. Expected + ByHirbmSVDCwP7YDBt3V2A, but read `. Commit 19fc711 pins + `kafka.clusterId: "ByHirbmSVDCwP7YDBt3V2A"` in `values.yaml` so *re-installs* reuse + retained PVC data, but a volume formatted before that pin holds a random ID that nothing + migrates. **Fix: delete the Kafka PVC and pod** — `kubectl delete pvc + data-kafka-stream-kafka-stream-pool- -n --wait=false` then `kubectl delete pod + kafka-stream-kafka-stream-pool- -n ` (deleting the pod releases the + `pvc-protection` finalizer); Strimzi reprovisions and reformats with the pinned ID. + Only transient model-monitoring stream data is lost. Setting `kafka.clusterId: ""` + restores the pre-19fc711 random-ID behaviour if keeping the existing volume matters more. + - `--dry-run` uses `helm --dry-run=server`, which validates against the live API server. If the target cluster lacks the Prometheus Operator CRDs, the `kube-prometheus-stack` subchart's `PrometheusRule`/`ServiceMonitor` resources @@ -237,6 +251,24 @@ predating K8s 1.34, so that node image likely wasn't even published for it. ## Fixed bugs +- **`helm_install`'s `--wait` had no `--timeout`, so a slow image pull failed the release** + (found via live testing against the `vmdev137` lab): both helm invocations in + `helm_install` (the progress-UI branch and the plain branch) passed `--wait` without + `--timeout`, silently inheriting helm's **5 minute** default. A single cold pull of + `quay.io/mlrun/jupyter` (4.2Gi) took **5m40s** on that cluster, so helm gave up mid-pull + with `UPGRADE FAILED: resource Deployment/mlrun/mlrun-jupyter not ready ... Pending + termination: 1` and marked the release `failed` — even though the rollout completed + seconds later and every pod went Running. A failed release record is worse than a slow + one: it misreports a working install and leaves the release in a state that invites an + unnecessary rollback. Notably `helm uninstall` (`do_uninstall`) *already* passed + `--timeout 960s`, so this was an inconsistency rather than a deliberate choice. Fix: + added `HELM_TIMEOUT` (default `960s`, matching uninstall) and passed + `--timeout "${HELM_TIMEOUT}"` in both branches. Re-running with the fix took 3m12s and + the release went `deployed`. Two regression tests assert the default and the override — + note the override test must `export HELM_TIMEOUT` on its own line rather than using the + `VAR=x source install.sh` prefix form, since bash discards that prefix assignment when + `source` returns and `set -u` then trips inside `helm_install`. + - **`do_hard_clean()`'s force-delete fallback could hang indefinitely** (found via live testing against the `vmdev137` lab cluster — a real `--hard-clean` run sat blocked for 18+ hours): both the PVC and PV delete loops fall back to @@ -292,7 +324,7 @@ predating K8s 1.34, so that node image likely wasn't even published for it. ## Testing -- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 96 tests, no cluster needed (sources +- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 98 tests, no cluster needed (sources `install.sh` with `INSTALL_SH_SOURCE_ONLY=true`, stubs external binaries). - Live/integration: exercise `--chart-path` against a real chart checkout (see below). Non-interactive runs need `REGISTRY_USERNAME`/`REGISTRY_PASSWORD` diff --git a/scripts/docs/parameters.md b/scripts/docs/parameters.md index fcaea249..4ce1b780 100644 --- a/scripts/docs/parameters.md +++ b/scripts/docs/parameters.md @@ -71,6 +71,7 @@ Options: | `EXTERNAL_HOST_ADDRESS`| — | Host address the cluster is reachable at (see [FAQ](faq.md) for the autodetect fallback chain) | | `SKIP_REGISTRY_SECRET` | `false` | Set to `true` to skip secret creation | | `SKIP_VALIDATORS` | `false` | Set to `true` to skip the pre-install validators | +| `HELM_TIMEOUT` | `960s` | Timeout for helm's `--wait` on install/upgrade. Raise it on slow networks — a cold pull of the 4.2Gi jupyter image alone can take ~6 minutes | | `MIN_K8S_VERSION` | — (no floor) | Kubernetes version to warn below (`MAJOR.MINOR`); never blocks the install | | `MIN_HELM_VERSION` | `3.6` | Helm CLI version floor the blocking validator enforces (`MAJOR.MINOR`) | | `DISABLE_SYSTEM_MONITORING` | `false` | Set to `true` to disable the Grafana/Prometheus stack | diff --git a/scripts/install.sh b/scripts/install.sh index d59d16d7..d53023dd 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -41,6 +41,10 @@ NAMESPACE="${NAMESPACE:-mlrun}" RELEASE_NAME="${RELEASE_NAME:-mlrun-ce}" REGISTRY_SECRET_NAME="${REGISTRY_SECRET_NAME:-registry-credentials}" HELM_REPO_URL="${HELM_REPO_URL:-https://mlrun.github.io/ce}" +# --wait without --timeout inherits helm's 5m default, which a single image pull can +# outrun: the 4.2Gi jupyter image alone takes ~5m40s on a cold node, failing the release +# even though the rollout goes on to succeed. Matches `helm uninstall --timeout 960s`. +HELM_TIMEOUT="${HELM_TIMEOUT:-960s}" SKIP_REGISTRY_SECRET="${SKIP_REGISTRY_SECRET:-false}" SKIP_VALIDATORS="${SKIP_VALIDATORS:-false}" REGISTRY_PASSWORD_FILE="${REGISTRY_PASSWORD_FILE:-}" @@ -139,6 +143,7 @@ Options: Environment variables (for non-interactive / CI): SKIP_REGISTRY_SECRET Set to 'true' to skip creating the registry secret SKIP_VALIDATORS Set to 'true' to skip the pre-install validators + HELM_TIMEOUT Timeout for helm's --wait on install/upgrade (default: 960s) MIN_K8S_VERSION Kubernetes version to warn below (unset by default; never blocks) MIN_HELM_VERSION Helm CLI version floor the validators enforce (default: 3.6) DISABLE_SYSTEM_MONITORING Set to 'true' to disable the Grafana/Prometheus stack @@ -889,6 +894,7 @@ helm_install() { helm upgrade --install "${RELEASE_NAME}" "${CHART_REF}" \ --namespace "${NAMESPACE}" \ --wait \ + --timeout "${HELM_TIMEOUT}" \ ${values_flag[@]+"${values_flag[@]}"} \ ${version_flag[@]+"${version_flag[@]}"} \ ${dry_run_flag[@]+"${dry_run_flag[@]}"} \ @@ -913,6 +919,7 @@ helm_install() { helm upgrade --install "${RELEASE_NAME}" "${CHART_REF}" \ --namespace "${NAMESPACE}" \ --wait \ + --timeout "${HELM_TIMEOUT}" \ ${values_flag[@]+"${values_flag[@]}"} \ ${version_flag[@]+"${version_flag[@]}"} \ ${dry_run_flag[@]+"${dry_run_flag[@]}"} \ diff --git a/tests/install_tests.bats b/tests/install_tests.bats index 12bb39e8..c0d05be3 100644 --- a/tests/install_tests.bats +++ b/tests/install_tests.bats @@ -617,6 +617,45 @@ EOF [[ "$output" == *"nuclio.dashboard.image.tag=1.16.0"* ]] } +# Regression: --wait without --timeout inherits helm's 5m default, which is shorter than a +# single cold pull of the 4.2Gi jupyter image (~5m40s observed on a real cluster) — the +# release ends up marked failed even though the rollout succeeds moments later. +@test "helm_install passes --timeout alongside --wait so a slow image pull can't fail the release" { + local chart_dir="$BATS_TMPDIR/fakechart_timeout" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo \"HELM_ARGS: \$*\"; } + CHART_PATH='$chart_dir' + REGISTRY_URL=x + EXTERNAL_HOST_ADDRESS=x + DRY_RUN=true + helm_install + " + [ "$status" -eq 0 ] + [[ "$output" == *"--wait"* ]] + [[ "$output" == *"--timeout 960s"* ]] +} + +@test "HELM_TIMEOUT overrides the helm --wait timeout" { + local chart_dir="$BATS_TMPDIR/fakechart_timeout2" + mkdir -p "$chart_dir" + printf 'apiVersion: v2\nname: mlrun-ce\nversion: 0.0.1\n' > "$chart_dir/Chart.yaml" + run bash -c " + export HELM_TIMEOUT=1800s + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + helm() { echo \"HELM_ARGS: \$*\"; } + CHART_PATH='$chart_dir' + REGISTRY_URL=x + EXTERNAL_HOST_ADDRESS=x + DRY_RUN=true + helm_install + " + [ "$status" -eq 0 ] + [[ "$output" == *"--timeout 1800s"* ]] +} + @test "load_config maps installer.components.* to DISABLE_* the same as the --disable-* flags" { local cfg="$BATS_TMPDIR/cfg_components.yaml" cat > "$cfg" <<'EOF' From c2f895f611d3f4fde62f04348022cdc964d967a3 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 13:59:04 +0300 Subject: [PATCH 05/15] Fix CI: KUBE_CONTEXT test stub and chart version collision The resolve_external_host KUBE_CONTEXT test stubbed `command` but matched on the subcommand immediately after shifting off "kubectl", so it never saw the --context that the kubectl wrapper injects ahead of the real arguments. The stub returned nothing, node_ip came back empty and the suggested host fell back to localhost. It passed locally anyway: bats aborts a test on the first failed assertion via set -e, and under macOS's system bash 3.2 that only holds for the last statement in a @test. The broken assertion was second-to-last, so it was swallowed and the test reported ok. CI runs bash 5, where it fails. Both AGENTS.md and the run-tests skill now warn about this, since any test whose stub drifts from the code can hide the same way. Also bump the chart to 0.12.0-rc.12: development is already at rc.11, so ct lint saw no version bump and failed. Co-authored-by: Cursor --- .claude/skills/run-tests/SKILL.md | 9 +++++++++ charts/mlrun-ce/Chart.yaml | 2 +- scripts/AGENTS.md | 10 ++++++++++ tests/install_tests.bats | 3 +++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md index bffbe782..edd4b383 100644 --- a/.claude/skills/run-tests/SKILL.md +++ b/.claude/skills/run-tests/SKILL.md @@ -114,6 +114,15 @@ a `helm version --short` string, an allocatable quantity, and so on). `[[ "$output" == *"substring"* ]]`. 5. Run `bats tests/install_tests.bats` to confirm green. +> **Green on macOS is not green on CI.** Under macOS's system bash (3.2), a +> failed assertion that isn't the *last* statement of a `@test` is silently +> swallowed and the test still prints `ok`; CI runs bash 5, where it fails. +> After writing a test that stubs external commands, run its inner `bash -c` +> body standalone once and eyeball the output, or `brew install bash` so local +> runs behave like CI. Note that stubs of `command` must account for the +> `kubectl`/`helm` wrappers injecting `--context`/`--kube-context` before the +> real arguments. + ### Minimal test template ```bash diff --git a/charts/mlrun-ce/Chart.yaml b/charts/mlrun-ce/Chart.yaml index 7fa7aaa2..f6a8b919 100644 --- a/charts/mlrun-ce/Chart.yaml +++ b/charts/mlrun-ce/Chart.yaml @@ -1,6 +1,6 @@ apiVersion: v1 name: mlrun-ce -version: 0.12.0-rc.11 +version: 0.12.0-rc.12 appVersion: 1.12.0-rc25 description: MLRun Open Source Stack home: https://iguazio.com diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 8b714fe5..ad981e70 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -326,6 +326,16 @@ predating K8s 1.34, so that node image likely wasn't even published for it. - Unit: `make installer-test` (`bats tests/install_tests.bats`) — 98 tests, no cluster needed (sources `install.sh` with `INSTALL_SH_SOURCE_ONLY=true`, stubs external binaries). +- **A green local run on macOS does not mean a green CI run.** bats aborts a test + on the first failed assertion via `set -e`, and under macOS's system bash (3.2) + that only works for the *last* statement in a `@test` — a failed `[[ ]]` + anywhere before it is silently swallowed and the test still reports `ok`. CI + runs bash 5, where every assertion counts. A test whose stub doesn't match what + the code actually calls can therefore pass locally and fail in CI (this is + exactly how the `resolve_external_host` `KUBE_CONTEXT` test shipped broken). + When a test is doing real work, verify the assertion holds — run the inner + `bash -c` body standalone and look at the output, or install bash >= 4 + (`brew install bash`) so local runs match CI. - Live/integration: exercise `--chart-path` against a real chart checkout (see below). Non-interactive runs need `REGISTRY_USERNAME`/`REGISTRY_PASSWORD` (or `REGISTRY_PASSWORD_FILE`)/`REGISTRY_EMAIL` set or they'll fail on the diff --git a/tests/install_tests.bats b/tests/install_tests.bats index c0d05be3..f27641cf 100644 --- a/tests/install_tests.bats +++ b/tests/install_tests.bats @@ -1037,6 +1037,9 @@ EOF case \"\$1\" in kubectl) shift + # The kubectl wrapper injects --context ahead of the real + # arguments, so drop it before matching on the subcommand. + [[ \"\$1\" == --context ]] && shift 2 if [[ \"\$1\" == config && \"\$2\" == current-context ]]; then echo docker-desktop elif [[ \"\$1\" == get && \"\$2\" == node ]]; then From 11531a466e1130e390e6c104b78e1e4720a12df7 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 14:21:46 +0300 Subject: [PATCH 06/15] Fix CI: make the yq-absence tests hermetic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "load_config exits 1 when yq is not installed" hid yq by setting PATH=/usr/bin:/bin. That works on macOS, where yq lives in /opt/homebrew/bin, but the GitHub runners ship yq in /usr/bin — so on CI yq stayed on PATH, load_config parsed the file and returned 0, and the test failed. install.sh's yq guard was correct all along; only the test's isolation was wrong. Replace the hardcoded PATH with an _empty_bin helper pointing at a directory that provably holds no executables, so the assertion no longer depends on where the host installs yq. The neighbouring "no yq required when CONFIG_FILE is empty" test used the same hardcoded PATH and was therefore vacuous on CI; it now uses the helper too. Co-authored-by: Cursor --- scripts/AGENTS.md | 6 ++++++ tests/install_tests.bats | 20 ++++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index ad981e70..639d4fff 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -336,6 +336,12 @@ predating K8s 1.34, so that node image likely wasn't even published for it. When a test is doing real work, verify the assertion holds — run the inner `bash -c` body standalone and look at the output, or install bash >= 4 (`brew install bash`) so local runs match CI. +- **Never hide a tool by hardcoding a PATH of real system directories.** The + GitHub runners ship `yq` in `/usr/bin`, so `PATH=/usr/bin:/bin` hides it on a + macOS box (where it's in `/opt/homebrew/bin`) but not in CI — which is how the + "load_config exits 1 when yq is not installed" test came to assert nothing in + the only environment that was checking it. Use the `_empty_bin` helper, which + points PATH at a directory that provably contains no executables. - Live/integration: exercise `--chart-path` against a real chart checkout (see below). Non-interactive runs need `REGISTRY_USERNAME`/`REGISTRY_PASSWORD` (or `REGISTRY_PASSWORD_FILE`)/`REGISTRY_EMAIL` set or they'll fail on the diff --git a/tests/install_tests.bats b/tests/install_tests.bats index f27641cf..7a773a14 100644 --- a/tests/install_tests.bats +++ b/tests/install_tests.bats @@ -13,6 +13,18 @@ _src() { INSTALL_SH_SOURCE_ONLY=true source "$SCRIPT" } +# Path to a directory guaranteed to hold no executables, for tests that need a +# tool to be genuinely absent. Setting PATH to a real system directory doesn't +# work: the GitHub runners ship yq in /usr/bin, so PATH=/usr/bin:/bin hides it +# on macOS (yq lives in /opt/homebrew/bin there) while leaving it visible in CI. +# The log_* helpers are printf-only, so an empty PATH still produces their output. +_empty_bin() { + local dir="$BATS_TMPDIR/empty_bin" + rm -rf "$dir" + mkdir -p "$dir" + printf '%s' "$dir" +} + # --------------------------------------------------------------------------- # --ce-version parsing # --------------------------------------------------------------------------- @@ -329,9 +341,11 @@ _src() { } @test "load_config is a no-op (returns 0, no yq required) when CONFIG_FILE is empty" { + local empty_bin + empty_bin="$(_empty_bin)" run bash -c " INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' - PATH=/usr/bin:/bin + PATH='$empty_bin' load_config echo done " @@ -351,11 +365,13 @@ _src() { @test "load_config exits 1 when yq is not installed" { local cfg="$BATS_TMPDIR/cfg_noyq.yaml" + local empty_bin printf 'installer:\n registry:\n url: x\n' > "$cfg" + empty_bin="$(_empty_bin)" run bash -c " INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' CONFIG_FILE='$cfg' - PATH=/usr/bin:/bin + PATH='$empty_bin' load_config " [ "$status" -eq 1 ] From 3041d2947b32dd8ffecd3541f923ad4f10d6c432 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 14:29:30 +0300 Subject: [PATCH 07/15] Keep the installer design proposal out of the repo It's a working design document rather than reference material for the chart, so it stays on disk and out of the PR via .git/info/exclude instead of the tracked .gitignore. Strip the four AGENTS.md pointers and the install.sh NodePort comment that referenced it, so nothing in the repo links to a file reviewers won't have. The NodePort note now points at REQUIRED_NODEPORTS, which is the actual source of truth for that list. Co-authored-by: Cursor --- scripts/AGENTS.md | 10 +- scripts/docs/design-proposal.md | 330 -------------------------------- scripts/install.sh | 2 +- 3 files changed, 6 insertions(+), 336 deletions(-) delete mode 100644 scripts/docs/design-proposal.md diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 639d4fff..eb55b5ca 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -2,7 +2,7 @@ `scripts/install.sh` — single-file bash wrapper around `helm install mlrun-ce/mlrun-ce` (published chart repo: `https://mlrun.github.io/ce`). Local execution only, no SSH, no git -fetching. Full design/rationale: `docs/design-proposal.md`. +fetching. It lives in the same repo as the chart it installs (`charts/mlrun-ce`), but installs the **published** chart by default — the in-repo chart is used only when the caller passes @@ -28,7 +28,7 @@ the repo-root `AGENTS.md`/`CONTRIBUTING.md`. This file covers the installer only `install_ingress_controller` (which used to `helm install` the `ingress-nginx` chart when `--enable-ingress` was passed) was **removed** — see "Phase 5" below. -## Phase status (see docs/design-proposal.md for full plan) +## Phase status - **Phase 1 (done):** `--ce-version` fix, `--dry-run`, `--non-interactive` + `CI=true` auto-detect. Originally shipped as `--helm-version`/`HELM_VERSION`; renamed (hard @@ -162,7 +162,7 @@ the repo-root `AGENTS.md`/`CONTRIBUTING.md`. This file covers the installer only the full image set is too slow per-PR). It's a separate workflow file rather than a job in `ci.yaml` so it reports as an independent status check. No Jenkinsfile — this repo is GitHub Actions only. -## Phase 3 pre-work (resolved, see docs/design-proposal.md §6) +## Phase 3 pre-work (resolved) Before implementing Phase 3 (`ce-config.yaml` + `yq`), these open questions were resolved by checking the official MLRun docs and the chart itself (`../charts/mlrun-ce`, @@ -184,8 +184,8 @@ then still a separate repo): Everything else (`registry.secret.server`, `registry.secret.email`, `chartVersion`, `kubeContext`, `externalHostAddress`, `ingress.*`, `components.*`) is optional — documented in full in `docs/configuration.md`'s "Config file (`ce-config.yaml`)" section. -- **NodePorts/components for Phase 4:** no additions beyond the list already in - `docs/design-proposal.md` Phase 4. +- **NodePorts/components for Phase 4:** no additions beyond the ports already + covered by `REQUIRED_NODEPORTS` in `install.sh`. ## Version floors realigned (supersedes the Phase 3 pre-work finding above) diff --git a/scripts/docs/design-proposal.md b/scripts/docs/design-proposal.md deleted file mode 100644 index 2e80bbd8..00000000 --- a/scripts/docs/design-proposal.md +++ /dev/null @@ -1,330 +0,0 @@ -# Proposal: Config-driven, validated MLRun CE installer - -**Status:** Historical design record. Phases 1-5 are implemented; see `../AGENTS.md` for -what actually shipped and where it deviated from this document. -**Note:** written while the installer lived in its own repo, separate from the chart. Since -then it has moved into the chart repo as `scripts/install.sh`, so "this repo" below means -the installer, and the chart — described here as a separate clone — is now `charts/mlrun-ce`. -**Scope:** `scripts/install.sh` — stays pure bash + helm, single file, **local execution only**. - ---- - -## 1. Context & motivation - -`install.sh` is a single bash wrapper around `helm install mlrun-ce/mlrun-ce` (from the -published repo `https://mlrun.github.io/ce`). It works well, but has gaps we keep hitting: - -- **Config sprawl** — ~20 env vars + 11 flags. The only file input (`-f values.yaml`) - *bypasses* secret creation and other wiring. -- **Dead flag** — `--ce-version` is parsed but never passed to helm (`parse_args`, ~line 701). -- **Weak validation** — the only pre-flight check is "is the binary installed." Misconfig - (wrong K8s version, no storage class, bad registry auth, NodePort clash) surfaces only - after a multi-minute helm timeout. No validation of the config file itself. -- **Can't install a local chart checkout** — only the published, packaged chart. We want to - install a `mlrun/ce` branch/PR that the developer has **cloned locally**, by pointing at - its chart path. -- **No first-class CI story** for Jenkins / GitHub Actions. - -**Goal:** adopt the high-value patterns from igzctl (typed config, validators, version -pinning, local chart-path install) while staying a **single, self-contained, pure bash + -helm** script that runs entirely on the local machine and still supports -`curl -sSL .../install.sh | bash`. - -## 2. Design decisions (agreed) - -| Decision | Choice | Rationale | -|---|---|---| -| Structure | **Single-file `install.sh`** | Preserves the `curl \| bash` one-liner; no `lib/` sourcing | -| Config format | **`yq`-parsed `ce-config.yaml`** | One declarative file; `yq` needed only when a config file is used | -| Chart source | **Published repo OR local path** | **No git fetching in the installer** — the developer clones `mlrun/ce` and passes `--chart-path` | -| Execution | **Local only — no SSH** | Unlike mlefi, the installer never remotes into a host; it runs locally against the current kubeconfig | -| Registry input | **CLI value, else YAML fallback** | A flag/env value wins; pressing Enter (or leaving it unset) falls back to the config YAML | -| Config safety | **Validate file exists + no empty required fields** | Fail fast with a clear message instead of a bad helm run | -| UI ingress | **Opt-in; react to the chart** | Chart ships the UI Ingress (`mlrun.ui.ingress.enabled: false`) but no controller — installer only flips that value, never installs a controller (see Phase 5) | -| Build order | **Quick wins first** | Safe, isolated, immediate value | - -**Precedence everywhere:** flag/CLI > env var > config file > built-in default. Existing -flags/env keep working unchanged (back-compat). - ---- - -## 3. Phased plan - -### Phase 1 — Quick wins (no behavior change to existing flows) -1. **Fix `--ce-version`** — pass `--version "${CE_VERSION}"` to helm in published-repo mode. -2. **`--dry-run`** — inject `--dry-run=server`, skip the progress UI and the post-install - notes table (no release exists yet). -3. **`--non-interactive` + CI auto-detect** — set automatically when `CI=true`; in - `prompt_or_env`, return the default instead of calling `read`, so missing required values - fail cleanly with exit 1 instead of hanging on stdin. -4. Update `usage()`. - -### Phase 2 — Install from a locally cloned chart (`--chart-path`) -No git operations in the installer. The developer clones `mlrun/ce` themselves (any -branch/PR), then points the installer at the chart directory. - -- New flag/env: **`--chart-path DIR`** / `CHART_PATH` (e.g. `./charts/mlrun-ce`). -- New `resolve_chart_source()` (called at the top of `helm_install`): - - **Published-repo mode** (default, no `--chart-path`): `helm repo add/update`; - `CHART_REF="mlrun-ce/mlrun-ce"`; optional `--version` from `--ce-version`. - - **Local-path mode** (`--chart-path` set): verify the dir exists and contains - `Chart.yaml` (**error otherwise**); `helm dependency update "${CHART_PATH}"`; - `CHART_REF="${CHART_PATH}"`. -- In `helm_install`, the chart reference becomes `${CHART_REF}` (replaces the 4 hardcoded - `mlrun-ce/mlrun-ce` occurrences). - -### Phase 3 — `ce-config.yaml` (yq) + validation — done - -Implemented as designed below, with two deviations from the original draft: - -1. **Partial reduction in scope, later revisited:** `ingress.*`/`components.*` from the - example YAML were initially **not** wired up — deferred to avoid half-finishing - something that overlaps Phase 5's ingress work. `components.*` was added back in a - follow-up pass (`installer.components.{monitoring,spark,mpi,modelMonitoring}`, - mirroring the existing `--disable-*` flags' exact `--set` effects — pure value - resolution, no new infra logic) after explicit scoping: `ingress.*` and anything that - triggers real infra beyond a `--set` (installing nginx-ingress, deploying a local - registry) stays deferred to Phase 5; only the pure `--set` toggles were in scope here. -2. **`-f`/`--config` composition — reversed, then reverted back:** the original design - said `--config` knobs become `--set` flags that layer on top of `-f`'s `--values` - (helm applies `--set` after `--values`, so `--config` would always win, "no extra - merge logic needed"). The first implementation pass didn't actually do this — `-f` - silently made every `--config` registry/host field dead code with no warning. That - was "fixed" at the time by making **`--config` and `-f`/`--values` mutually - exclusive** (`main()` exit 1 if both given) rather than building the layering — a - misread of the actual ask, corrected in a later follow-up back to the originally - designed composable behavior described in "Combining with `-f`/`--values`" below. - `main()` now runs `create_registry_secret`/`gather_install_params` whenever - `--config` is given (even alongside `-f`), and the 3 registry `--set`s - (`global.registry.{url,secretName}`, `global.externalHostAddress`) moved into the - same `extra_set_flags` array the other `--set`s already used — gated off only in - pure `-f`-only mode (no `--config`), which keeps `-f`-alone's exact prior - self-contained behavior (no secret creation, no registry `--set`s) unchanged. Added - `installer.versions.{mlrun,nuclio}` (→ - `--set mlrun.{api,ui}.image.tag`/`nuclio.{controller,dashboard}.image.tag`) to close - the version-pinning gap this raised — usable via `--config` or the `MLRUN_VERSION`/ - `NUCLIO_VERSION` env vars (the latter also works with `-f`, since it's not gated by - `--config`). -3. **New safety check found along the way:** `--skip-secret` previously trusted the user - completely — if the named secret didn't actually exist, the failure only surfaced - after a `helm --wait` timeout with a cryptic pod-level error. Added - `verify_existing_registry_secret()`: exits 1 immediately with a clear message if the - secret is missing. -4. **`REGISTRY_PASSWORD_FILE` added:** the password was (and still is) deliberately - never read from `ce-config.yaml` — only `REGISTRY_PASSWORD` (env) or the interactive - masked (`read -s`) prompt could supply it. Added `REGISTRY_PASSWORD_FILE` as a third - option — path to a local file containing just the password, trailing newline - stripped, never echoed — for the common case where a CI runner or secrets manager - mounts a secret as a file rather than an env var. Precedence: `REGISTRY_PASSWORD` - env > `REGISTRY_PASSWORD_FILE` > interactive prompt; still never settable via - `ce-config.yaml`. -One YAML file, **only** the reserved **`installer:`** block — a curated set of basic, -typed knobs (registry, chart source, ingress toggle, component enables). No generic -passthrough section: `ce-config.yaml` is not a values-file substitute, and never becomes -a `-f` values file itself. New **`--config FILE`** flag. **`-f`/`--values` stays fully -independent** and can be combined with `--config` — arbitrary/complex helm values always -go through `-f`, never through the config file. (Design note: an earlier draft of this -phase let `ce-config.yaml` pass "everything else" straight through to helm as a second -values file — dropped, since that duplicated `-f`'s job and risked the two silently -overwriting each other.) - -```yaml -installer: # consumed by install.sh; stripped before helm - kubeContext: "" # optional; blank = current local context (no SSH, no remote exec) - externalHostAddress: auto # auto-detect locally, or pin a value - registry: - url: index.docker.io/myuser # used if not given on the CLI (Enter falls back to this) - secret: - name: registry-credentials - create: true # user brings creds; PASSWORD via env/prompt only, never in file - server: https://index.docker.io/v1/ - username: myuser - chartSource: - kind: repo # repo | path - chartVersion: 0.11.0 # repo mode -> --version - chartPath: "" # path mode -> local cloned chart dir - ingress: - ui: false # opt-in -> --set mlrun.ui.ingress.enabled=true (chart's own ingress) - className: "" # ingress class (blank = chart/cluster default) - host: "" # optional host override; else chart uses global.externalHostAddress - components: { monitoring: false, spark: true } # -> --set .enabled=... -``` - -- **`load_config()`**: - - **Error if `--config FILE` does not exist.** - - Require `yq`; read `installer.*` into vars **only if not already set** by flag/env - (precedence: flag > env > `ce-config.yaml` > default — same chain as every other - setting; `ce-config.yaml` just slots in as the config layer). For registry values - this is the "CLI wins, else YAML" behavior; in interactive mode the YAML value - becomes the prompt default (Enter accepts it). - - **Validate required fields are non-empty** (e.g. `registry.url`, `registry.secret.username`, - and `chartPath` when `chartSource.kind: path`); **raise an error listing every empty - required field** and exit 1. - - Every `installer.*` key resolves to an explicit `--set` flag in `helm_install`, the same - pattern `extra_set_flags` already uses for `DISABLE_SPARK`/`ENABLE_INGRESS`/etc. There is - nothing else in the file to strip or pass through. -- **Host / context:** `externalHostAddress: auto` reuses the existing local autodetect - (minikube / docker-desktop / node IP). `kubeContext` (if set) is passed as - `--kube-context` to kubectl/helm — still local execution, just selects a context. -- **Combining with `-f`/`--values`:** the two are orthogonal, not ranked against each other. - `--config` knobs become `--set` flags; `-f` is a raw values file. Helm applies `--set` - after `--values`, so a `--config` knob always wins over the same key in a `-f` file with - no extra merge logic needed. Implemented: `helm_install` uses a single code path with an - optional `values_flag` (`--values`, only when `-f` is given) alongside `extra_set_flags` - (which now includes the registry `--set`s too, gated off only in pure `-f`-only mode) — - so `--config` and `-f` can be passed together, matching how `extra_set_flags` already - layered on top of `-f` for versions/components/otel/ingress even before this change. - -### Phase 4 — Pre-install validators (fail fast) — done - -Implemented as designed, with one addition and one scope note: - -- **Helm CLI version check added:** the original draft only listed the K8s version floor - as blocking. Since §6 also resolved a Helm CLI floor (>= 4.1) for the same docs page, - `validate_helm_version()` was added as a third blocking check alongside K8s version and - StorageClass — same shape, same rationale (docs-level requirement, not chart-enforced). -- **Node capacity scoped to RAM + storage, no CPU floor:** the bullet above says - "CPU/mem above a documented floor," but the only actual floor resolved in §6 is "≥8Gi - RAM / 8Gi storage" — no CPU number exists in the docs to check against. Implemented - `validate_node_capacity()` against the RAM/storage floor only. -- **Aggregate-then-report, not fail-at-first-check:** the three blocking checks - (`validate_k8s_version`, `validate_helm_version`, `validate_storage_class`) `return 1` - instead of calling `exit 1` directly, so `run_validators()` runs every check (blocking - and warning) and reports all problems in one pass before exiting 1 — a deliberate, - one-off deviation from the rest of the script's inline `log_error; exit 1` style, chosen - so a user fixing pre-flight issues doesn't have to re-run the installer once per problem. -- **Bug found via live testing against `docker-desktop`:** `validate_node_capacity()`'s - quantity parser only handled `Ki`-suffixed values (memory's format); this cluster's real - `.status.allocatable.ephemeral-storage` is a bare byte integer with no suffix, so the - check silently read it as 0Gi. Fixed with a small `_allocatable_to_ki()` helper that - handles `Ki`/`Mi`/`Gi`/`Ti` suffixes and the bare-byte-integer form. -- **Supporting fix in `create_registry_secret`:** its `username`/`password`/`server` were - locals invisible outside the function once resolved interactively (only - `REGISTRY_USERNAME_VALUE` existed as a side-channel export). Added the same-shaped - `REGISTRY_PASSWORD_VALUE`/`REGISTRY_SERVER_VALUE` so `validate_registry_auth` can - actually see what was entered, regardless of source (env, `REGISTRY_PASSWORD_FILE`, or - prompt). - -Checks, as implemented: - -> **Superseded:** the two version floors below were later realigned to the chart's own -> prerequisites — Helm >= 3.6 blocking, and no Kubernetes floor at all (the check is -> informational). See `scripts/AGENTS.md` "Version floors realigned" and -> `docs/configuration.md` "Version floors". The rest of this section still holds. - -- **K8s version** ≥ 1.34 (blocking) -- **Helm CLI version** ≥ 4.1 (blocking) -- **Default StorageClass** exists (blocking) -- **Registry auth** — best-effort `docker login` with provided creds (warning; skipped - under `--local-registry` or when no credentials were resolved yet, e.g. `-f`-only mode) -- **NodePort conflicts** — chart's fixed NodePorts (30010/20/40/50/60/70, 30093/94, 30100, 30110) - not already bound by a Service outside the target namespace (warning) -- **Node capacity** — allocatable RAM/ephemeral-storage above 8Gi/8Gi (warning) - -New flag/env: `--skip-validators` / `SKIP_VALIDATORS`, mirroring `--skip-secret`'s exact -pattern. `run_validators()` is called from `main()` unconditionally (in every mode, -including pure `-f`-only) right before `helm_install`. - -### Phase 5 — dropped (folded into `--enable-ingress` directly) - -The original idea was a UI-ingress toggle plus a controller story. Revisiting after Phase -4 shipped: `--enable-ingress` already flips `mlrun.ui.ingress.enabled` (and -`jupyterNotebook`/`nuclio.dashboard`/`mlrun.api` Ingress alongside it) — there was no -separate chart-value toggle left to add. The only real gap was that `--enable-ingress` -used to `helm install` the actual `ingress-nginx` controller itself -(`install_ingress_controller()`) — installing a third-party controller is more than this -installer should be doing on a user's behalf. Fixed directly on the existing flag instead -of as a new phase: - -- **Removed** `install_ingress_controller()` and its call in `main()`. `--enable-ingress` - is now BYO-controller-only — it configures the chart's Ingress resources via `--set`, - nothing else. -- **Added** `validate_ingress_controller()` to Phase 4's `run_validators` (warning-only, - no-op when `--enable-ingress` isn't used): checks `kubectl get ingressclass - ` and warns — doesn't block, doesn't install — if it's missing, so the - Ingress resources not resolving is a known-cause warning instead of a silent mystery. -- **Not built:** `print_ui_ingress_url()` (reading the created Ingress back and printing - its URL in the final access table) and `installer.ingress.*` ce-config.yaml keys (the - CLI/env flag already covers the same knobs). Out of scope unless requested separately. - -### Phase 6 — CI samples (Jenkins + GitHub Actions) -CI does its own `git checkout` (via `actions/checkout` / Jenkins SCM) — the installer never -fetches. To test a `ce` PR, CI checks out that ref and passes `--chart-path`. - -- **`.github/workflows/ce-install.yml`** — `workflow_dispatch` with inputs `ce_ref` - (branch/PR ref to checkout) and `dry_run`; steps: checkout this repo → checkout `mlrun/ce` - at `ce_ref` → `azure/setup-helm` → install `yq` → `helm/kind-action` → run `install.sh` - with `CI=true`, `REGISTRY_*` from `secrets`, and `--chart-path` to the checked-out chart. - A separate `pull_request` job runs `bash -n` + `shellcheck` only. -- **`Jenkinsfile`** — parameters (CE_REF, DRY_RUN), `withCredentials` for the registry, - `post { failure { sh './install.sh --uninstall || true' } }` teardown. - ---- - -## 4. Files touched / added - -| File | Change | -|---|---| -| `install.sh` | Phases 1-4: new vars, `resolve_chart_source`, `load_config` (+existence/empty-field validation), `run_validators`; edits to `parse_args`, `prompt_or_env`, `helm_install`, `main`, `usage` | -| `ce-config.yaml.example` | New — documented sample config | -| `.github/workflows/ce-install.yml` | New — GitHub Actions | -| `Jenkinsfile` | New — Jenkins pipeline | -| `README.md` | Document `--config`, `--chart-path`, validators, CI usage | - -## 5. Verification - -1. **Static:** `bash -n install.sh` + `shellcheck install.sh`. -2. **Help:** `./install.sh --help` lists all new flags. -3. **Config validation:** - - `./install.sh --config missing.yaml` → clear "file not found" error, exit 1. - - Config with an empty required field → error naming the empty field(s), exit 1. -4. **Dry-run (needs kind/minikube/docker-desktop):** - - `./install.sh --dry-run` — renders published chart, no deploy. - - `./install.sh --ce-version 0.11.0 --dry-run` — confirms `--version` is passed. - - `./scripts/install.sh --chart-path ./charts/mlrun-ce --dry-run` — `helm dependency update` + - renders from the local chart directory. - - `./install.sh --config ce-config.yaml.example --dry-run` — `installer:` block consumed - and stripped; registry falls back to YAML when not passed on the CLI. -5. **Ingress (opt-in, BYO controller):** `./install.sh --enable-ingress` on a cluster with - an existing IngressClass → no warning, `kubectl get ingress` shows `mlrun-ui` etc. On a - cluster with no matching IngressClass → `validate_ingress_controller` warns but the - install still proceeds (no controller ever gets installed by this script). -6. **Non-interactive:** `CI=true ./install.sh` with a required var missing → clean exit 1 (no hang). -7. **CI:** trigger the GH Actions `workflow_dispatch` with a `ce_ref`; run the parameterized Jenkins build. - -## 6. Open questions for reviewers — resolved - -- **Is `yq` an acceptable dependency for the config path?** Yes, with conditions. `yq` - (mikefarah/yq, Go) makes no network calls at runtime, so runtime CVEs are limited to - YAML-parsing bugs — but the *installer* (or a CI image) fetching the `yq` binary is a - supply-chain vector like any curl-installed tool. Mitigate: pin an exact released - version, verify its checksum (GitHub releases publish `checksums`), and prefer an - already-present `yq` (package manager / CI base image) over installing one at runtime. - `load_config()` should hard-require `yq` only when `--config` is passed — no new - dependency for users who don't use the config file (matches existing "quick wins first, - no behavior change" principle). -- **Minimum supported K8s version to enforce in the validator (Phase 4)?** Per the - official install docs (docs.mlrun.org, `install-mlrun-ce/kubernetes-install.md`): - **Kubernetes >= 1.34**, **Helm >= 4.1** CLI, a default StorageClass, and >= 8Gi RAM / - 8Gi storage available. The chart itself sets no `kubeVersion` in `Chart.yaml` — the - version floor is a docs-level requirement, not chart-enforced, so `run_validators()` - is the only place it's actually checked today. -- **Which fields count as required (non-empty) in `ce-config.yaml`?** Cross-checked - against what `install.sh` already hard-enforces (`create_registry_secret`, - `gather_install_params` in the current `main`, ~lines 369-467): - - `installer.registry.url` — required, unless `installer.local` registry mode is used - (mirrors `REGISTRY_URL` today, install.sh:461-464). - - `installer.registry.secret.username` — required (mirrors `REGISTRY_USERNAME`, - install.sh:395-398). - - `installer.registry.secret.password` — required, but **never read from the file** — - only from `REGISTRY_PASSWORD` env or the interactive prompt (already a design - decision in §3; the password is checked non-empty the same way the username is). - - `installer.chartSource.chartPath` — required only when `installer.chartSource.kind: - path`. - - Everything else in the schema (`registry.secret.server` — has a working default; - `registry.secret.email`; `chartVersion`; `kubeContext`; `externalHostAddress`; - `ingress.*`; `components.*`) is optional, matching that these either have defaults - or are opt-in features today. -- **Any additional NodePorts/components to validate beyond the chart defaults listed in - Phase 4?** No — no additional ports/components identified; the list in Phase 4 stands - as-is. \ No newline at end of file diff --git a/scripts/install.sh b/scripts/install.sh index d53023dd..218c14c0 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -1140,7 +1140,7 @@ do_uninstall() { fi } -# Chart's fixed NodePorts (not configurable via values.yaml) — see docs/design-proposal.md §6. +# Chart's fixed NodePorts (not configurable via values.yaml). REQUIRED_NODEPORTS=(30010 30020 30040 30050 30060 30070 30093 30094 30100 30110) # The Helm floor mirrors charts/mlrun-ce/README.md's "Helm >=3.6" — the chart's own stated From cd31ef830b56a9accdc1c68ddab4f6cbd23a6a58 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 14:35:24 +0300 Subject: [PATCH 08/15] Trim scripts/AGENTS.md to installer reference material MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the Phase 1-6 status list and the Phase 3 pre-work notes: they record how the installer was built rather than how it works, which belongs in the local design doc, not the repo. Two genuinely reusable pieces are kept and restated without the phase framing — the value precedence rule (flag > env > config > --values > chart defaults, and why --config and -f compose) now sits under the install flow it describes, and the version floors read as current policy instead of a "realigned from X" note. De-specify the bug entries: they cited a named internal lab cluster, and one paragraph pinned live state on it, including an SSH command with an internal hostname and node IP. The findings hold for any remote cluster reached via --kube-context, so they now say that instead, and the teardown command is given with a placeholder context. Co-authored-by: Cursor --- scripts/AGENTS.md | 235 +++++++--------------------------------------- 1 file changed, 35 insertions(+), 200 deletions(-) diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index eb55b5ca..e5b1a51a 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -22,201 +22,41 @@ the repo-root `AGENTS.md`/`CONTRIBUTING.md`. This file covers the installer only 5. `deploy_local_registry` — only if `--local-registry` 6. `create_registry_secret` — skipped via `--skip-secret`, or when `-f VALUES_FILE` is given **and** `--config` is absent (pure `-f`-only mode keeps its old self-contained behavior; `-f` + `--config` together still creates the secret). In `--dry-run` only logs, doesn't touch the cluster 7. `gather_install_params` — resolves `REGISTRY_URL` (same `-f`-only skip condition as above) -8. `run_validators` — skippable via `--skip-validators`; includes `validate_ingress_controller` (warns if `--enable-ingress`'s IngressClass isn't found — see Phase 4/5 notes below) +8. `run_validators` — skippable via `--skip-validators`; includes `validate_ingress_controller`, which warns (never blocks) if `--enable-ingress`'s IngressClass isn't found 9. `helm_install` → `resolve_chart_source` (published repo vs `--chart-path`) → `helm install/upgrade`, with `--values`/`--set` composed per the precedence rule below -`install_ingress_controller` (which used to `helm install` the `ingress-nginx` chart when -`--enable-ingress` was passed) was **removed** — see "Phase 5" below. - -## Phase status - -- **Phase 1 (done):** `--ce-version` fix, `--dry-run`, `--non-interactive` + `CI=true` - auto-detect. Originally shipped as `--helm-version`/`HELM_VERSION`; renamed (hard - rename, no alias) once it became clear the flag pins the **mlrun-ce chart** version, - not the Helm CLI binary — `check_requirements`/`validate_helm_version` (Phase 4) are - the ones actually about the Helm binary, and shared the word "helm" confusingly with - this one. -- **Phase 2 (done):** `--chart-path DIR` / `resolve_chart_source` — install from a locally cloned chart dir instead of the published repo -- **Phase 3 (done):** `--config`/`CONFIG_FILE` + `load_config()` — reads the `installer:` - block of a `ce-config.yaml` (requires `yq`, only when `--config` is used) and fills in - registry url/username/server/email, `externalHostAddress`, `chartSource.{kind,chartVersion,chartPath}` - and the new `KUBE_CONTEXT` as defaults, at flag/env > config-file > default precedence. - `KUBE_CONTEXT` is threaded through every `kubectl`/`helm` call via wrapper functions - (install.sh top-of-file) rather than editing each call site — `check_requirements` uses - `type -P` instead of `command -v` for helm/kubectl so those wrappers don't cause a false - "installed" detection. `installer.ingress.*` from the original design draft was **not** - implemented (deferred; use existing `--enable-ingress` instead) — at the time this - triggered real infra (nginx-ingress install) beyond a `--set`; `--enable-ingress` no - longer installs anything (see "Phase 5" below), but `installer.ingress.*` config-file - keys remain unimplemented since `--enable-ingress`/`--enable-ingress CLASS` already - cover the same knobs via flag/env. - `installer.components.{monitoring,spark,mpi,modelMonitoring}` **was** added in a - follow-up pass — mirrors the `--disable-*` flags' exact `--set` effects 1:1 (pure value - resolution, `false` disables, flag/env always wins and is never re-enabled by the - file); `--local-registry`'s infra-deploying behavior was explicitly scoped out the same - way as `ingress.*`. OpenTelemetry support was added in a later follow-up pass as its - own `installer.otel.{operator,collector,namespaceLabel,instrumentation}` block (not - under `components.*`, since it's 4 independent knobs, not one flat bool) — the reverse - direction from `components.*`: those 4 chart values (`opentelemetry-operator.enabled`, - `opentelemetry.collector.enabled`, `opentelemetry.namespaceLabel.enabled`, - `opentelemetry.instrumentation.enabled`) all ship `false` by default (unlike - `kube-prometheus-stack`/`spark-operator`/etc., which ship `true`), so each key opts IN - via the matching `--enable-otel-{operator,collector,namespace-label,instrumentation}`/ - `ENABLE_OTEL_*` flag/env pair (a bare `--enable-otel` is a convenience that sets all - four). Kept as 4 independent toggles rather than one bundled flag deliberately — - `namespaceLabel` auto-instruments every pod in the release namespace once - `instrumentation` is also on, a materially bigger blast radius than `operator`/ - `collector` alone, so a user should be able to pick just the latter two. A flag/env-set - `ENABLE_OTEL_*=true` still always wins and is never unset by the file, same "add, - never override" rule as `components.*`. Later given an optional MODE argument — - `--enable-otel [off|collector|full]` (bare = `full`, matching the original behavior - exactly) — as a convenience on top of, not a replacement for, the 4 granular flags - (which still work individually and combine with it). `collector` exists as a named - middle ground: operator+collector only (a metrics pipeline), without opting into - `namespaceLabel`'s bigger blast radius. - **`--config` and `-f`/`--values` compose** (a later follow-up reverted an earlier - deviation): the original design draft had `--config` knobs layer as `--set` overrides - on top of `-f`'s `--values` (helm applies `--set` after `--values`, so `--config` - always wins, no merge logic needed) — the first implementation pass never actually - wired that up (silently dead instead) and was "fixed" at the time by making the two - mutually exclusive (`main()` exit 1). That exclusivity was itself the wrong fix and was - removed: `main()` now runs `create_registry_secret`/`gather_install_params` whenever - `--config` is given (even alongside `-f`) rather than only when `-f` is absent, and - `helm_install` moved the 3 registry `--set`s (`global.registry.{url,secretName}`, - `global.externalHostAddress`) into the same `extra_set_flags` array the other - `--set`s already used, gated off only in pure `-f`-only mode (no `--config`) — so - every config-resolved field now actually reaches helm as a `--set` layered on top of - `--values` in every combination. `-f` used alone (no `--config`) keeps its exact prior - self-contained behavior unchanged (no secret creation, values file must reference an - existing secret) — only adding `--config` changes that. Precedence documented in - README as: flag > env > `ce-config.yaml` (→ `--set`) > `-f`/`--values` (raw) > chart - defaults. Added `installer.versions.{mlrun,nuclio}` / `MLRUN_VERSION`/`NUCLIO_VERSION` - env vars (→ `--set mlrun.{api,ui}.image.tag`, `nuclio.{controller,dashboard}.image.tag`) - for per-service version pins, independent of `--ce-version`/`chartSource.chartVersion` - which pins the mlrun-ce umbrella chart as a whole. Also added - `verify_existing_registry_secret()`: `--skip-secret` now exits 1 immediately if the - named secret doesn't exist in the namespace, instead of failing later via an opaque - `helm --wait` timeout. `REGISTRY_PASSWORD_FILE` was added as a third way to supply the - registry password (path to a local file, trailing newline stripped, never echoed) — - same "never settable via `ce-config.yaml`" rule as `REGISTRY_PASSWORD`; precedence is - `REGISTRY_PASSWORD` env > `REGISTRY_PASSWORD_FILE` > interactive masked prompt. -- **Phase 4 (done):** `run_validators()` — a pre-install dispatcher, called from `main()` - right after the `-f`/`--config` secret-creation branch and right before `helm_install`, - skippable via `--skip-validators`/`SKIP_VALIDATORS` (mirrors `--skip-secret`'s exact - flag/env/`main()` pattern). Runs unconditionally in every mode, including pure - `-f`-only (the cluster-level checks don't depend on registry resolution; the - registry-auth check self-skips when nothing's resolved yet). Six checks, all read-only - (no `--dry-run` gating needed): - - **Blocking** (`validate_helm_version`, `validate_storage_class`): Helm CLI >= 3.6 - (parsed from `helm version --short`) and a default StorageClass exists - (`is-default-class` annotation). These `return 1` instead of calling `exit 1` directly - (the one deliberate deviation from the rest of the script's inline - `log_error; exit 1` style) so `run_validators` can run every check and report *all* - failures in one pass, then exit 1 once at the end — rather than stopping at the first - problem found. - - **Warning-only** (`validate_k8s_version`, `validate_registry_auth`, - `validate_nodeport_conflicts`, `validate_node_capacity`): the cluster's Kubernetes - version, read via `kubectl get nodes` `.status.nodeInfo.kubeletVersion` (the same - jsonpath-on-nodes style `resolve_external_host` already uses — avoids depending on - `kubectl version` supporting `-o jsonpath`), reported always and compared only against - an explicitly set `MIN_K8S_VERSION`; best-effort `docker login` with the resolved registry - creds (skipped, not warned, under `--local-registry` or when nothing's resolved yet); - the chart's fixed NodePorts (`30010/20/40/50/60/70`, `30093/94`, `30100`, `30110`) - already bound by a Service outside the target namespace (excluding the target - namespace so `helm upgrade` of the same release never self-flags); total cluster - allocatable RAM/ephemeral-storage under the documented 8Gi/8Gi floor (no CPU floor - exists in the docs to check against, despite the original proposal bullet loosely - saying "CPU/mem"). - - **Supporting fix needed along the way:** `create_registry_secret`'s - `username`/`password`/`server` are locals — `prompt_or_env` returns a value via - stdout, it never sets the named env var globally, so in the (common) interactive-prompt - case the real entered password/server were invisible outside the function. Only - `REGISTRY_USERNAME_VALUE` existed as a side-channel export (for `gather_install_params`'s - suggested-URL default). Added the same-shaped `REGISTRY_PASSWORD_VALUE` and - `REGISTRY_SERVER_VALUE` so `validate_registry_auth` can see what was actually entered, - regardless of whether it came from env, `REGISTRY_PASSWORD_FILE`, or an interactive - prompt. -- **Phase 5 — dropped, folded into existing `--enable-ingress` instead of a new phase:** - the chart's UI Ingress toggle (`mlrun.ui.ingress.enabled`) already ships as part of the - pre-existing `--enable-ingress` flag (alongside `jupyterNotebook`/`nuclio.dashboard`/ - `mlrun.api` Ingress — install.sh `helm_install`'s `extra_set_flags`), so there was no - separate toggle left to add. The one real gap — `--enable-ingress` used to `helm - install` the actual `ingress-nginx` controller itself, a real infra dependency the - installer had no business installing — was fixed directly on that flag: removed - `install_ingress_controller()` entirely, and added `validate_ingress_controller()` as a - warning-only check in `run_validators` (Phase 4) that looks for an existing IngressClass - matching `--enable-ingress`'s class and warns (doesn't block, doesn't install) if none - is found. `--enable-ingress` is now BYO-controller-only. `print_ui_ingress_url()` (the - other original Phase 5 idea — reading the created Ingress back and printing its URL in - the final access table) was not built; out of scope unless asked for separately. -- **Phase 6 (done, as real CI rather than the originally planned samples):** - `.github/workflows/installer-ci.yaml` runs `make installer-lint` (`bash -n` + - shellcheck) and `make installer-test` (the bats suite) via the Makefile targets rather - than duplicating the commands, so CI and local can't drift. It runs on **every** PR: an - earlier `paths: scripts/**` filter was dropped because the job takes about a minute and - a filtered job lets the suite rot unnoticed between installer changes. Note the unit - tests are hermetic (they stub `kubectl`/`helm`/`docker`), so running them on chart PRs - does *not* catch chart/installer drift — only the kind job would, and that's - `workflow_dispatch`-only, installing with `--chart-path ./charts/mlrun-ce - --local-registry`, for the same reason `ci.yaml`'s `test:` job is commented out (pulling - the full image set is too slow per-PR). It's a separate workflow file rather than a job - in `ci.yaml` so it reports as an independent status check. No Jenkinsfile — this repo is - GitHub Actions only. -## Phase 3 pre-work (resolved) +Value precedence, highest first: flag > env > `ce-config.yaml` (applied as `--set`) > +`-f`/`--values` (passed raw) > chart defaults. `--config` and `-f` compose rather than +conflict — helm applies `--set` after `--values`, so config-resolved values always win +without any merge logic. `-f` used alone (no `--config`) keeps its self-contained +behaviour: no secret is created and the values file must reference an existing one. -Before implementing Phase 3 (`ce-config.yaml` + `yq`), these open questions were -resolved by checking the official MLRun docs and the chart itself (`../charts/mlrun-ce`, -then still a separate repo): - -- **`yq` dependency:** acceptable. No network calls at runtime, so runtime CVE exposure - is limited to YAML parsing. Require it only when `--config` is passed (no new dep for - existing flows); pin an exact release version and verify its checksum rather than an - unpinned install. -- **K8s/Helm version floor** (for Phase 4's validator): originally taken from the official - install docs (**Kubernetes >= 1.34**, **Helm >= 4.1**) and enforced as blocking. **This was - later reversed** — see "Version floors realigned" below. Neither is enforced by the chart - itself (no `kubeVersion` in `Chart.yaml`). -- **Mandatory `ce-config.yaml` fields** (cross-checked against what `install.sh` already - hard-enforces in `create_registry_secret`/`gather_install_params`): - `installer.registry.url` (unless `--local-registry`), `installer.registry.secret.username`, - `installer.registry.secret.password` (required but **never read from the file** — env/ - prompt only), `installer.chartSource.chartPath` (only when `chartSource.kind: path`). - Everything else (`registry.secret.server`, `registry.secret.email`, `chartVersion`, - `kubeContext`, `externalHostAddress`, `ingress.*`, `components.*`) is optional — - documented in full in `docs/configuration.md`'s "Config file (`ce-config.yaml`)" section. -- **NodePorts/components for Phase 4:** no additions beyond the ports already - covered by `REQUIRED_NODEPORTS` in `install.sh`. - -## Version floors realigned (supersedes the Phase 3 pre-work finding above) +`install_ingress_controller` (which used to `helm install` the `ingress-nginx` chart when +`--enable-ingress` was passed) was **removed**: installing a cluster's ingress controller +is not the installer's job. `--enable-ingress` is now BYO-controller-only — it sets the +chart's Ingress toggles and `validate_ingress_controller` warns if no matching +IngressClass exists. -The blocking **K8s >= 1.34 / Helm >= 4.1** floors, sourced from docs.mlrun.org, were replaced -with the chart's own stated requirement: +## Version floors -- **Helm >= 3.6**, blocking — mirrors `charts/mlrun-ce/README.md`'s prerequisites, so the - installer can't refuse a Helm the chart itself supports. Helm 4.1 as a *minimum* excluded - every Helm 3 user for a chart that renders fine on Helm 3. -- **No Kubernetes floor.** `validate_k8s_version` is now informational: it reports the - detected version, returns 0 on every path, and is no longer in `run_validators`' - `|| failed=1` group. `MIN_K8S_VERSION` defaults to empty and only produces a *warning* - when set. The chart declares no `kubeVersion` and the README states no cluster version, - so there was no requirement to enforce — 1.34 as a hard minimum rejected nearly every - supported managed cluster, including the local `docker-desktop` (1.30.5) used for testing. +The installer's floors track the chart's own prerequisites, not the product install docs: -An intermediate step (before this realignment) kept the strict floors but made them -overridable via `MIN_K8S_VERSION`/`MIN_HELM_VERSION`. Those env vars survive, but their -purpose inverted: they now exist to *tighten* rather than loosen. `MIN_HELM_VERSION` is the -only hard floor; `MIN_K8S_VERSION` never blocks. +- **Helm >= 3.6, blocking** — mirrors `charts/mlrun-ce/README.md`'s prerequisites, so the + installer can't refuse a Helm the chart itself supports. `MIN_HELM_VERSION` raises it. +- **No Kubernetes floor.** `validate_k8s_version` is informational: it reports the detected + version, returns 0 on every path, and is not in `run_validators`' `|| failed=1` group. + The chart declares no `kubeVersion` and the README states no cluster version, so there is + no requirement to enforce. `MIN_K8S_VERSION` defaults to empty and only *warns* when set. -Knock-on effect: `.github/workflows/installer-ci.yaml`'s kind job had pinned -`kubectl_version`/`node_image` to `v1.34.0` and Helm to `v4.1.1` purely to satisfy those -floors. Those pins were removed — `helm/kind-action@v1.10.0` bundles a kind release -predating K8s 1.34, so that node image likely wasn't even published for it. +Both env vars exist to tighten, never to loosen. Keep `.github/workflows/installer-ci.yaml`'s +kind job unpinned for the same reason — with no floor to satisfy, the action's own default +node image is the safest choice. ## Known non-bugs - **CE does not officially support upgrades**, so a `helm upgrade` over an existing release is out of scope as a supported path. The concrete symptom seen live (0.11.0 → - 0.12.0-rc.11 on the `vmdev137` lab): the Kafka broker crash-loops with + 0.12.0-rc.11 on a real cluster): the Kafka broker crash-loops with `Invalid cluster.id in: /var/lib/kafka/data/kafka-log0/meta.properties. Expected ByHirbmSVDCwP7YDBt3V2A, but read `. Commit 19fc711 pins `kafka.clusterId: "ByHirbmSVDCwP7YDBt3V2A"` in `values.yaml` so *re-installs* reuse @@ -252,7 +92,7 @@ predating K8s 1.34, so that node image likely wasn't even published for it. ## Fixed bugs - **`helm_install`'s `--wait` had no `--timeout`, so a slow image pull failed the release** - (found via live testing against the `vmdev137` lab): both helm invocations in + (found via live testing against a real remote cluster): both helm invocations in `helm_install` (the progress-UI branch and the plain branch) passed `--wait` without `--timeout`, silently inheriting helm's **5 minute** default. A single cold pull of `quay.io/mlrun/jupyter` (4.2Gi) took **5m40s** on that cluster, so helm gave up mid-pull @@ -270,14 +110,14 @@ predating K8s 1.34, so that node image likely wasn't even published for it. `source` returns and `set -u` then trips inside `helm_install`. - **`do_hard_clean()`'s force-delete fallback could hang indefinitely** (found via live - testing against the `vmdev137` lab cluster — a real `--hard-clean` run sat blocked for + testing against a real remote cluster — a `--hard-clean` run sat blocked for 18+ hours): both the PVC and PV delete loops fall back to `kubectl delete ... --force --grace-period=0` when the graceful `--timeout 60s` delete fails, but neither fallback passed `--wait=false` — by default `kubectl delete` still blocks waiting for the object to actually disappear from the API, `--force` only skips *graceful* deletion of the underlying pod, not the wait. When a PVC has a lingering `kubernetes.io/pvc-protection` finalizer (the exact orphaned-Strimzi-Kafka scenario in - "Known non-bugs" below), nothing ever removes that finalizer, so the fallback hung just + "Known non-bugs" above), nothing ever removes that finalizer, so the fallback hung just as long as the primary attempt — defeating the point of having a fallback at all, worse still under `run_in_background` where a silently hung command gives no signal anything is wrong. Fix: added `--wait=false` to both fallback commands, so they return @@ -313,9 +153,9 @@ predating K8s 1.34, so that node image likely wasn't even published for it. SSH tunnel to the target cluster). - **`resolve_external_host()`'s generic fallback (no heuristic matched) now suggests `localhost` instead of a node-IP lookup.** Only the truly generic case changed — the - `KUBE_CONTEXT` branch (node IP; needed for e.g. the `vmdev137` lab pattern, where the - node's real internal IP is reachable on the corporate network but `localhost` would - resolve to nothing since only the API server port is SSH-tunneled) and the minikube/ + `KUBE_CONTEXT` branch (node IP; needed for the SSH-tunnelled remote-cluster pattern, + where the node's real internal IP is reachable on the private network but `localhost` + would resolve to nothing since only the API server port is tunnelled) and the minikube/ docker-desktop heuristics are untouched. The generic case (no `KUBE_CONTEXT`, not minikube, not docker-desktop — e.g. kind/k3d) previously did a node-IP lookup that's frequently unreachable for those tools, which typically NodePort-map to `localhost` @@ -354,19 +194,14 @@ predating K8s 1.34, so that node image likely wasn't even published for it. `localhost:` (works cleanly when the cert's SANs already include `localhost`/`127.0.0.1`, true for kubeadm/rke2 defaults). `--config` + `-f` composition and `REGISTRY_PASSWORD_FILE` were both confirmed working through - such a tunnel against a live `rke2` lab cluster, in addition to local - `docker-desktop` runs. The remote run reached a fully healthy state (26/26 - containers ready, `helm status` → `deployed`) — notably including `mlrun-ui`, + such a tunnel against a live `rke2` cluster, in addition to local + `docker-desktop` runs. The remote run reached a fully healthy state (every + container ready, `helm status` → `deployed`) — notably including `mlrun-ui`, which fails on local Apple Silicon `docker-desktop` runs only because that - image has no `linux/arm64` build; this lab cluster is x86_64. + image has no `linux/arm64` build; the remote cluster was x86_64. - **Not yet torn down** (deliberately, as of this writing): the `mlrun-ce` - release from this verification is still `deployed` in the `mlrun` namespace - on the shared `vmdev137` lab cluster, and the local SSH tunnel - (`ssh -f -N -L 16443:192.168.236.51:6443 iguazio@app1.vmdev137ig4.lab.iguaz.io`, - backing the local `vmdev137` kubeconfig context) is still running. Tear down - with `KUBE_CONTEXT=vmdev137 ./scripts/install.sh --uninstall --hard-clean - --non-interactive` (also deletes its PVCs) when done needing it — that + Tear a verification release down with `KUBE_CONTEXT= ./scripts/install.sh + --uninstall --hard-clean --non-interactive` (also deletes its PVCs). That command is destructive enough against shared remote infra that it's worth running deliberately rather than as a matter of course. From 80d713c8f5372e4b714237aecfef85effd0510d1 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 14:37:21 +0300 Subject: [PATCH 09/15] Use generic fixtures in the KUBE_CONTEXT test The test hardcoded a named internal lab cluster and its node IP. Neither means anything to a reader of this repo, and the address is a real private one. Swap in a placeholder context name and 192.0.2.10, from RFC 5737's documentation range, so the fixture is self-evidently fake. Co-authored-by: Cursor --- tests/install_tests.bats | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/install_tests.bats b/tests/install_tests.bats index 7a773a14..c7e3fb0f 100644 --- a/tests/install_tests.bats +++ b/tests/install_tests.bats @@ -1059,19 +1059,19 @@ EOF if [[ \"\$1\" == config && \"\$2\" == current-context ]]; then echo docker-desktop elif [[ \"\$1\" == get && \"\$2\" == node ]]; then - echo '192.168.236.51' + echo '192.0.2.10' fi ;; minikube) return 1 ;; esac } - KUBE_CONTEXT=vmdev137 + KUBE_CONTEXT=remote-cluster NON_INTERACTIVE=true resolve_external_host echo \"host=\$EXTERNAL_HOST_ADDRESS\" " [ "$status" -eq 0 ] - [[ "$output" == *"host=192.168.236.51"* ]] + [[ "$output" == *"host=192.0.2.10"* ]] [[ "$output" != *"host=host.docker.internal"* ]] } From 039e8bd16c45daae3ba12f94b39e56672afdadf2 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 14:51:29 +0300 Subject: [PATCH 10/15] Version the installer off the chart, and pin the documented URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The installer had no version at all, so a user couldn't say which script they ran and a run couldn't be reproduced. Add --version/-v, reading the version from charts/mlrun-ce/Chart.yaml beside the script rather than storing a copy: bumping the chart bumps the installer, with nothing to carry forward by hand. Run standalone (curl | bash, or copied to a bin directory) there's no chart to read and nothing recording where the script came from, so it reports unknown instead of inventing a number. Tying the version to the chart rather than giving the installer its own is deliberate. The script encodes chart internals — the fixed NodePort list, and the --set value paths it writes — so an installer and a chart from the same tag are the only pairing guaranteed to agree, and a renamed value path would otherwise fail silently as a --set that does nothing. The documented curl URLs pointed at .../development/scripts/install.sh, which is a 404 today (scripts/ isn't on development yet) and would be a moving target once it isn't. Use a pinned mlrun-ce- tag as the primary form, since chart-releaser already tags every release and those tags contain this script; keep development documented as the rolling alternative. No new release workflow is needed as a result. Co-authored-by: Cursor --- .claude/skills/bump/SKILL.md | 2 ++ .claude/skills/run-tests/SKILL.md | 5 +-- scripts/AGENTS.md | 25 ++++++++++++++- scripts/README.md | 40 +++++++++++++++++++++-- scripts/docs/parameters.md | 1 + scripts/install.sh | 30 +++++++++++++++-- tests/install_tests.bats | 53 +++++++++++++++++++++++++++++++ 7 files changed, 147 insertions(+), 9 deletions(-) diff --git a/.claude/skills/bump/SKILL.md b/.claude/skills/bump/SKILL.md index bce805b2..3ec6c4cf 100644 --- a/.claude/skills/bump/SKILL.md +++ b/.claude/skills/bump/SKILL.md @@ -22,4 +22,6 @@ Steps: 5. Remind the user: version bumps must be committed before opening a PR, and the PR title must follow `[Scope] description` format. 6. Update the MLRun CE version under Version Matrix in `charts/mlrun-ce/README.md`. +`scripts/install.sh` needs no edit — it reads its version from `Chart.yaml` at runtime, so bumping the chart bumps the installer too. + If no argument is given, show the current version and list the three options with the resulting version for each, then ask which to apply. diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md index edd4b383..3d88c78b 100644 --- a/.claude/skills/run-tests/SKILL.md +++ b/.claude/skills/run-tests/SKILL.md @@ -33,7 +33,7 @@ make installer-test bats tests/install_tests.bats ``` -Expected output: `1..98` followed by `ok N ` for every test. +Expected output: `1..102` followed by `ok N ` for every test. Keep the count in this file in sync when you add tests — it's the quickest way to notice a test silently failing to register. @@ -88,10 +88,11 @@ Tests that exercise the validators individually stub `kubectl`/`helm`/`docker` a shell functions instead, echoing whatever the check parses (a `kubeletVersion`, a `helm version --short` string, an allocatable quantity, and so on). -## Current coverage — 98 tests +## Current coverage — 102 tests | Phase / area | Tests | |--------------|-------| +| **Versioning** — `installer_version` / `--version` | reads the version from the chart beside the script, tracks it when the chart version changes, reports `unknown` when run standalone, `-v` short form | | **Phase 1** — `--ce-version` parsing | stores value, rejects missing arg, respects env var, defaults empty | | **Phase 1** — `--dry-run` / `--non-interactive` | each sets its var, each defaults false, flag consumed cleanly | | **Phase 1** — `prompt_or_env` non-interactive | returns default, exits 1 with no default, env var wins over default | diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index e5b1a51a..e38bed07 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -37,6 +37,29 @@ is not the installer's job. `--enable-ingress` is now BYO-controller-only — it chart's Ingress toggles and `validate_ingress_controller` warns if no matching IngressClass exists. +## Versioning and releases + +`installer_version()` (printed by `--version`) reads `version:` out of +`charts/mlrun-ce/Chart.yaml` next to the script, so bumping the chart bumps the installer +and there's no second copy to carry forward. Running standalone — `curl | bash`, or copied +to a bin directory — there's no chart to read and nothing in the script recording its +origin, so it reports `unknown` rather than inventing a number; that's the case pinning by +release tag exists to answer. + +They're coupled because the installer encodes chart internals: `REQUIRED_NODEPORTS` is the +chart's fixed NodePort list, and `helm_install` writes chart-specific `--set` paths +(`global.registry.*`, `mlrun.{api,ui}.image.tag`, the `opentelemetry.*` and `components.*` +keys). An installer and a chart from the same tag are the only pairing guaranteed to +agree; a renamed value path would otherwise become a `--set` that silently does nothing. + +There is no separate installer release. `.github/workflows/release.yml` runs +chart-releaser on every push to `development`/`X.Y.x`, tagging `mlrun-ce-`, and +that tag's tree contains `scripts/install.sh` — which is what the pinned +`raw.githubusercontent.com/mlrun/ce//scripts/install.sh` URLs resolve against. +Shipping an installer change is merging it with a chart version bump. The published chart +tarball packages `charts/mlrun-ce` only, so the installer ships via the git tag, not the +`.tgz`. + ## Version floors The installer's floors track the chart's own prerequisites, not the product install docs: @@ -164,7 +187,7 @@ node image is the safest choice. ## Testing -- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 98 tests, no cluster needed (sources +- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 102 tests, no cluster needed (sources `install.sh` with `INSTALL_SH_SOURCE_ONLY=true`, stubs external binaries). - **A green local run on macOS does not mean a green CI run.** bats aborts a test on the first failed assertion via `set -e`, and under macOS's system bash (3.2) diff --git a/scripts/README.md b/scripts/README.md index 744ed34f..e5f76151 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -28,16 +28,25 @@ OpenTelemetry, precedence) · [FAQ](docs/faq.md) (known gotchas) ### Run directly (no download) +Pin to a release tag. Pick one from [the releases page](https://github.com/mlrun/ce/releases) +— the installer from tag `mlrun-ce-X` is the one tested against chart `X`: + ```bash -curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh | bash +CE_TAG=mlrun-ce-0.12.0-rc.12 +curl -sSL https://raw.githubusercontent.com/mlrun/ce/${CE_TAG}/scripts/install.sh | bash ``` +Substituting `development` for the tag always gets the newest script, but it moves with +every merge, so two runs a day apart can differ. Prefer a tag anywhere reproducibility +matters, CI especially. + ### Install as a named command ```bash -curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh \ +CE_TAG=mlrun-ce-0.12.0-rc.12 +curl -sSL https://raw.githubusercontent.com/mlrun/ce/${CE_TAG}/scripts/install.sh \ -o /usr/local/bin/mlrun-install && chmod +x /usr/local/bin/mlrun-install -mlrun-install +mlrun-install --version ``` ### From a clone of this repo @@ -51,6 +60,31 @@ chart from your working tree instead. --- +## Versioning and releases + +The installer has no version of its own. It ships with the chart and is released by the +same tag, so `install.sh --version` reads the version straight out of +`charts/mlrun-ce/Chart.yaml` beside it — bumping the chart bumps the installer, with no +second copy to keep in step. Run standalone (`curl | bash`, or copied to a bin directory) +there is no chart to read and nothing recording where the script came from, so it reports +`unknown`; that's what pinning to a release tag answers. + +They're coupled on purpose. The installer encodes chart internals — the chart's fixed +NodePorts, and the `--set` value paths it writes — so an installer and a chart from the +same tag are the only pairing guaranteed to agree. Independent versions would invite a +mismatch whose failure mode is silent: a renamed value path becomes a `--set` that +quietly does nothing. + +Releasing follows from that. A push to `development` or a `X.Y.x` branch runs +chart-releaser, which tags `mlrun-ce-` and cuts a GitHub Release; that +tag's tree contains this script, which is what the pinned `raw.githubusercontent.com` +URLs above resolve against. So shipping an installer change is just merging it with a +chart version bump — there's no separate installer release to cut. Note the chart +tarball published to the Helm repo packages `charts/mlrun-ce` only, so the installer is +available from the git tag rather than from inside the `.tgz`. + +--- + ## Installation flows ### Interactive install diff --git a/scripts/docs/parameters.md b/scripts/docs/parameters.md index 4ce1b780..44ee5d12 100644 --- a/scripts/docs/parameters.md +++ b/scripts/docs/parameters.md @@ -16,6 +16,7 @@ Usage: install.sh [options] Options: -h, --help Show help + -v, --version Print the installer version (read from the chart beside it) --uninstall Uninstall the MLRun CE Helm release --hard-clean Use with --uninstall: delete all PVCs and PVs (data loss!) --skip-secret Skip creating the Docker registry secret diff --git a/scripts/install.sh b/scripts/install.sh index 218c14c0..b79fbcc5 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -17,11 +17,12 @@ # Creates a Docker registry secret from your credentials, then installs the # chart with your local URL and registry URL. # -# Run as a command (no download, no ./ or sh): -# curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh | bash +# Run as a command (no download, no ./ or sh). Pin to a release tag — see +# https://github.com/mlrun/ce/releases; "development" also works but moves with every merge: +# curl -sSL https://raw.githubusercontent.com/mlrun/ce/mlrun-ce-0.12.0-rc.12/scripts/install.sh | bash # # Or install as a named command, then run from any directory: -# curl -sSL https://raw.githubusercontent.com/mlrun/ce/development/scripts/install.sh -o /usr/local/bin/mlrun-install && chmod +x /usr/local/bin/mlrun-install +# curl -sSL https://raw.githubusercontent.com/mlrun/ce/mlrun-ce-0.12.0-rc.12/scripts/install.sh -o /usr/local/bin/mlrun-install && chmod +x /usr/local/bin/mlrun-install # mlrun-install # # From a clone of this repo (installs the published chart): @@ -89,6 +90,24 @@ log_error() { printf '%s\n' "${RED}[ERROR]${NC} $1" >&2; } kubectl() { command kubectl ${KUBE_CONTEXT:+--context "${KUBE_CONTEXT}"} "$@"; } helm() { command helm ${KUBE_CONTEXT:+--kube-context "${KUBE_CONTEXT}"} "$@"; } +# The installer has no version of its own — it ships with the chart and is released by the +# same tag — so read the version off the chart beside it rather than keeping a copy here +# that has to be carried forward by hand. Served via `curl | bash` or copied to a bin +# directory there is no chart to read, and the version is genuinely unknown: nothing in a +# standalone script records which commit it came from. Pin by release tag to know. +installer_version() { + local script_path script_dir chart_yaml + script_path="${BASH_SOURCE[0]:-$0}" + script_dir="$(cd "$(dirname "${script_path}")" 2>/dev/null && pwd)" || script_dir="" + chart_yaml="${script_dir}/../charts/mlrun-ce/Chart.yaml" + + if [[ -n "${script_dir}" && -f "${chart_yaml}" ]]; then + awk '/^version:/ {print $2; exit}' "${chart_yaml}" + else + printf 'unknown (standalone script — pin by release tag to identify it)' + fi +} + usage() { cat < "$fake_repo/charts/mlrun-ce/Chart.yaml" + run bash "$fake_repo/scripts/install.sh" --version + [ "$status" -eq 0 ] + [ "$output" = "mlrun-ce installer 9.9.9-rc.1" ] +} + +# curl | bash, or copied to /usr/local/bin: no chart to read, and nothing in the script +# records where it came from, so say so rather than inventing a version. +@test "installer_version reports unknown when running standalone" { + local standalone + standalone="$BATS_TMPDIR/standalone" + rm -rf "$standalone" + mkdir -p "$standalone" + cp "$SCRIPT" "$standalone/install.sh" + run bash "$standalone/install.sh" --version + [ "$status" -eq 0 ] + [[ "$output" == *"unknown"* ]] +} + +@test "-v is accepted as a short form of --version" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args -v + " + [ "$status" -eq 0 ] + [[ "$output" == *"mlrun-ce installer"* ]] +} + # --------------------------------------------------------------------------- # --ce-version parsing # --------------------------------------------------------------------------- From f045614e891d8ea5509a807ecd447044e34b1abe Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 15:07:06 +0300 Subject: [PATCH 11/15] Give the installer commands: mlrun-ce-installer install|uninstall|version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The modes were encoded as flags — --uninstall, and install as the unnamed default — which reads as a script rather than a CLI. Add a verb in front: install, uninstall, version, help, dispatched by parse_command so parse_args stays a pure flag parser. Nothing that worked before stops working. A leading flag, or no arguments at all, still means install, so every documented invocation and the curl | bash one-liner are unchanged, and `uninstall` and --uninstall are the same thing. A bare word that isn't a known command is an error rather than an install: `mlrun-ce-installer unistall` should not deploy a cluster on a typo. Two details worth knowing. main() expands COMMAND_ARGS through the ${a[@]+"${a[@]}"} guard because bash < 4.4 — including the macOS system bash this is developed on — treats an empty array as unset under set -u, so an argument-less run would otherwise abort; there's a test pinning that case. And the uninstall assignment is a full if rather than [[ ]] && x=y, which would return 1 and take errexit with it whenever the command wasn't uninstall. Also suppress color when stdout isn't a terminal or NO_COLOR is set. The log helpers previously emitted escape bytes unconditionally, which landed in CI logs and any redirected output. The installed command is now mlrun-ce-installer rather than mlrun-install. Co-authored-by: Cursor --- .claude/skills/run-tests/SKILL.md | 6 +- scripts/AGENTS.md | 8 ++- scripts/README.md | 24 +++++++- scripts/docs/parameters.md | 20 ++++++- scripts/install.sh | 85 ++++++++++++++++++++++++--- tests/install_tests.bats | 96 +++++++++++++++++++++++++++++++ 6 files changed, 223 insertions(+), 16 deletions(-) diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md index 3d88c78b..b035814d 100644 --- a/.claude/skills/run-tests/SKILL.md +++ b/.claude/skills/run-tests/SKILL.md @@ -33,7 +33,7 @@ make installer-test bats tests/install_tests.bats ``` -Expected output: `1..102` followed by `ok N ` for every test. +Expected output: `1..111` followed by `ok N ` for every test. Keep the count in this file in sync when you add tests — it's the quickest way to notice a test silently failing to register. @@ -88,10 +88,12 @@ Tests that exercise the validators individually stub `kubectl`/`helm`/`docker` a shell functions instead, echoing whatever the check parses (a `kubeletVersion`, a `helm version --short` string, an allocatable quantity, and so on). -## Current coverage — 102 tests +## Current coverage — 111 tests | Phase / area | Tests | |--------------|-------| +| **Commands** — `parse_command` | `install`/`uninstall` consume the verb and keep their flags, `version`/`help` print and exit 0, a leading flag or no arguments at all still means install (the empty-array case that trips `set -u` on bash 3.2), an unknown word exits 1 instead of installing | +| **Output** — color handling | no escape sequences when stdout isn't a TTY; `NO_COLOR` honored | | **Versioning** — `installer_version` / `--version` | reads the version from the chart beside the script, tracks it when the chart version changes, reports `unknown` when run standalone, `-v` short form | | **Phase 1** — `--ce-version` parsing | stores value, rejects missing arg, respects env var, defaults empty | | **Phase 1** — `--dry-run` / `--non-interactive` | each sets its var, each defaults false, flag consumed cleanly | diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index e38bed07..14494de8 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -15,6 +15,12 @@ the repo-root `AGENTS.md`/`CONTRIBUTING.md`. This file covers the installer only ## Install flow (`main()`, install.sh ~1319-1378) +0. `parse_command` — pulls an optional leading verb (`install`/`uninstall`/`version`/`help`) + off the front, leaving the rest in `COMMAND_ARGS`. Kept out of `parse_args` so that stays + a pure flag parser. No verb (or a leading flag) means `install`, which is what every + invocation predating commands relied on; an unrecognised bare word is an error rather + than an install, so a typo can't deploy. `main()` expands `COMMAND_ARGS` with the + `${a[@]+"${a[@]}"}` guard — bash < 4.4 treats an empty array as unset under `set -u` 1. `parse_args` — flags/env, precedence flag > env > default 2. `check_requirements` — helm, kubectl, docker present and reachable 3. `ensure_namespace` — creates `NAMESPACE`; in `--dry-run` only logs what it would do (no cluster mutation) @@ -187,7 +193,7 @@ node image is the safest choice. ## Testing -- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 102 tests, no cluster needed (sources +- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 111 tests, no cluster needed (sources `install.sh` with `INSTALL_SH_SOURCE_ONLY=true`, stubs external binaries). - **A green local run on macOS does not mean a green CI run.** bats aborts a test on the first failed assertion via `set -e`, and under macOS's system bash (3.2) diff --git a/scripts/README.md b/scripts/README.md index e5f76151..b893dbcc 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -45,8 +45,8 @@ matters, CI especially. ```bash CE_TAG=mlrun-ce-0.12.0-rc.12 curl -sSL https://raw.githubusercontent.com/mlrun/ce/${CE_TAG}/scripts/install.sh \ - -o /usr/local/bin/mlrun-install && chmod +x /usr/local/bin/mlrun-install -mlrun-install --version + -o /usr/local/bin/mlrun-ce-installer && chmod +x /usr/local/bin/mlrun-ce-installer +mlrun-ce-installer version ``` ### From a clone of this repo @@ -60,6 +60,26 @@ chart from your working tree instead. --- +## Commands + +```bash +mlrun-ce-installer install [options] # the default when no command is given +mlrun-ce-installer uninstall [--hard-clean] +mlrun-ce-installer version +mlrun-ce-installer help +``` + +The command is optional. Flags on their own mean `install`, so `--dry-run` and +`install --dry-run` are the same thing and anything written before commands existed still +works. `uninstall` is equivalent to the older `--uninstall` flag. A word that isn't one of +the four is rejected rather than treated as an install, so a typo like `unistall` can't +deploy a cluster by accident. + +Colored output is suppressed automatically when stdout isn't a terminal, and when +`NO_COLOR` is set, so piped and redirected runs stay readable. + +--- + ## Versioning and releases The installer has no version of its own. It ships with the chart and is released by the diff --git a/scripts/docs/parameters.md b/scripts/docs/parameters.md index 44ee5d12..34dc4308 100644 --- a/scripts/docs/parameters.md +++ b/scripts/docs/parameters.md @@ -9,11 +9,27 @@ Every flag has an environment-variable equivalent (for CI / non-interactive use) --- -## Flags +## Commands + +``` +Usage: mlrun-ce-installer [options] + install Install MLRun CE (the default when no command is given) + uninstall Uninstall the MLRun CE Helm release + version Print the installer version + help Show help ``` -Usage: install.sh [options] +The command is optional: flags passed on their own are an `install`, so invocations +written before commands existed still work. `uninstall` and the older `--uninstall` flag +do the same thing. A word that isn't one of the four is an error rather than an install, +so a typo like `unistall` can't deploy by accident. + +--- + +## Flags + +``` Options: -h, --help Show help -v, --version Print the installer version (read from the chart beside it) diff --git a/scripts/install.sh b/scripts/install.sh index b79fbcc5..cd578404 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -22,8 +22,11 @@ # curl -sSL https://raw.githubusercontent.com/mlrun/ce/mlrun-ce-0.12.0-rc.12/scripts/install.sh | bash # # Or install as a named command, then run from any directory: -# curl -sSL https://raw.githubusercontent.com/mlrun/ce/mlrun-ce-0.12.0-rc.12/scripts/install.sh -o /usr/local/bin/mlrun-install && chmod +x /usr/local/bin/mlrun-install -# mlrun-install +# curl -sSL https://raw.githubusercontent.com/mlrun/ce/mlrun-ce-0.12.0-rc.12/scripts/install.sh -o /usr/local/bin/mlrun-ce-installer && chmod +x /usr/local/bin/mlrun-ce-installer +# mlrun-ce-installer install +# +# Commands: install (the default), uninstall, version, help. Flags may be passed with no +# command at all, so every pre-command invocation below still means the same thing. # # From a clone of this repo (installs the published chart): # ./scripts/install.sh @@ -38,6 +41,9 @@ set -o errexit set -o nounset set -o pipefail +SUBCOMMAND="install" +COMMAND_ARGS=() + NAMESPACE="${NAMESPACE:-mlrun}" RELEASE_NAME="${RELEASE_NAME:-mlrun-ce}" REGISTRY_SECRET_NAME="${REGISTRY_SECRET_NAME:-registry-credentials}" @@ -75,11 +81,20 @@ KUBE_CONTEXT="${KUBE_CONTEXT:-}" MLRUN_VERSION="${MLRUN_VERSION:-}" NUCLIO_VERSION="${NUCLIO_VERSION:-}" -# Colors for output (use $'...' so escape sequences are actual bytes, not literal \033) -RED=$'\033[0;31m' -GREEN=$'\033[0;32m' -YELLOW=$'\033[1;33m' -NC=$'\033[0m' +# Colors for output (use $'...' so escape sequences are actual bytes, not literal \033). +# Suppressed when stdout isn't a terminal or NO_COLOR is set (https://no-color.org), so a +# piped or redirected run — CI logs, `| tee install.log` — reads as text instead of escapes. +if [[ -t 1 && -z "${NO_COLOR:-}" ]]; then + RED=$'\033[0;31m' + GREEN=$'\033[0;32m' + YELLOW=$'\033[1;33m' + NC=$'\033[0m' +else + RED="" + GREEN="" + YELLOW="" + NC="" +fi log_info() { printf '%s\n' "${GREEN}[INFO]${NC} $1"; } log_warn() { printf '%s\n' "${YELLOW}[WARN]${NC} $1"; } @@ -110,7 +125,13 @@ installer_version() { usage() { cat < [options] + +Commands: + install Install MLRun CE (default when no command is given) + uninstall Uninstall the MLRun CE Helm release + version Print the installer version + help Show this help message Interactive installer for MLRun CE on your local Kubernetes cluster. Creates a Docker registry secret (unless skipped) and installs the Helm chart @@ -1384,8 +1405,54 @@ run_validators() { log_info "Pre-install validation passed." } +# Verb dispatch, kept in front of parse_args rather than inside it so the flag parser stays +# a pure flag parser. Anything that isn't a known command — a flag, or nothing at all — is +# an install, which is what every invocation documented before commands existed relied on. +# A bare unknown word is rejected rather than silently installed: `mlrun-ce-installer +# unistall` should not wipe a cluster's worth of PVCs on a typo. +parse_command() { + SUBCOMMAND="install" + COMMAND_ARGS=() + + if [[ $# -eq 0 ]]; then + return 0 + fi + + case "$1" in + install|uninstall) + SUBCOMMAND="$1" + shift + ;; + version) + printf 'mlrun-ce installer %s\n' "$(installer_version)" + exit 0 + ;; + help) + usage + exit 0 + ;; + -*) + ;; + *) + log_error "Unknown command '$1'. Expected one of: install, uninstall, version, help." + log_info "Flags may be passed without a command, e.g. '--dry-run' is the same as 'install --dry-run'." + exit 1 + ;; + esac + + COMMAND_ARGS=("$@") +} + main() { - parse_args "$@" + parse_command "$@" + # bash < 4.4 treats an empty array as unset under `set -u`, so an argument-less run + # would abort here without the ${a[@]+...} guard. + parse_args ${COMMAND_ARGS[@]+"${COMMAND_ARGS[@]}"} + + # The `uninstall` command and the older --uninstall flag are the same thing. + if [[ "${SUBCOMMAND}" == "uninstall" ]]; then + UNINSTALL="true" + fi # Auto-enable non-interactive when running inside a CI environment if [[ "${CI:-}" == "true" ]]; then diff --git a/tests/install_tests.bats b/tests/install_tests.bats index 8289725a..01076571 100644 --- a/tests/install_tests.bats +++ b/tests/install_tests.bats @@ -25,6 +25,102 @@ _empty_bin() { printf '%s' "$dir" } +# --------------------------------------------------------------------------- +# Subcommands +# --------------------------------------------------------------------------- + +@test "install consumes the verb and still parses the flags after it" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_command install --dry-run + parse_args \${COMMAND_ARGS[@]+\"\${COMMAND_ARGS[@]}\"} + echo \"\$SUBCOMMAND \$DRY_RUN\" + " + [ "$status" -eq 0 ] + [ "$output" = "install true" ] +} + +@test "uninstall sets UNINSTALL the same way the --uninstall flag does" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_command uninstall --hard-clean + parse_args \${COMMAND_ARGS[@]+\"\${COMMAND_ARGS[@]}\"} + [[ \"\$SUBCOMMAND\" == uninstall ]] && UNINSTALL=true + echo \"\$UNINSTALL \$HARD_CLEAN\" + " + [ "$status" -eq 0 ] + [ "$output" = "true true" ] +} + +@test "version prints the version and exits 0" { + run bash "$SCRIPT" version + [ "$status" -eq 0 ] + [[ "$output" == *"mlrun-ce installer"* ]] +} + +@test "help prints usage and exits 0" { + run bash "$SCRIPT" help + [ "$status" -eq 0 ] + [[ "$output" == *"Usage: mlrun-ce-installer"* ]] +} + +# Every invocation documented before commands existed passed flags with no verb. +@test "a leading flag with no command is still an install" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_command --dry-run + parse_args \${COMMAND_ARGS[@]+\"\${COMMAND_ARGS[@]}\"} + echo \"\$SUBCOMMAND \$DRY_RUN\" + " + [ "$status" -eq 0 ] + [ "$output" = "install true" ] +} + +# bash < 4.4 treats an empty array as unset under `set -u`, so this is the case that +# breaks first if the ${COMMAND_ARGS[@]+...} guard in main() is ever dropped. +@test "no arguments at all defaults to install without tripping set -u" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_command + parse_args \${COMMAND_ARGS[@]+\"\${COMMAND_ARGS[@]}\"} + echo \"\$SUBCOMMAND \${#COMMAND_ARGS[@]}\" + " + [ "$status" -eq 0 ] + [ "$output" = "install 0" ] +} + +# A typo'd verb must not fall through to install: `unistall` would otherwise deploy. +@test "an unknown command exits 1 instead of falling through to install" { + run bash "$SCRIPT" unistall + [ "$status" -eq 1 ] + [[ "$output" == *"Unknown command 'unistall'"* ]] +} + +# --------------------------------------------------------------------------- +# Colored output +# --------------------------------------------------------------------------- + +# `run` captures through a pipe, so stdout is never a TTY here — the escape-free +# branch is the one under test, which is also what CI logs and `| tee` see. +@test "log output carries no escape sequences when stdout is not a terminal" { + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + log_info hello + " + [ "$status" -eq 0 ] + [ "$output" = "[INFO] hello" ] +} + +@test "NO_COLOR is honored" { + run bash -c " + NO_COLOR=1 + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + log_warn careful + " + [ "$status" -eq 0 ] + [ "$output" = "[WARN] careful" ] +} + # --------------------------------------------------------------------------- # Installer version (coupled to the chart) # --------------------------------------------------------------------------- From 6ae103d6c5c0305fff840c4d3efdbc5a661eceb3 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 15:13:24 +0300 Subject: [PATCH 12/15] update Usage: mlrun-ce-installer --- .claude/skills/run-tests/SKILL.md | 4 ++-- Makefile | 19 +++++++++++++++++++ scripts/README.md | 21 +++++++++++++++++---- scripts/install.sh | 15 ++++++++++++++- tests/install_tests.bats | 14 ++++++++++++++ 5 files changed, 66 insertions(+), 7 deletions(-) diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md index b035814d..d2a6f456 100644 --- a/.claude/skills/run-tests/SKILL.md +++ b/.claude/skills/run-tests/SKILL.md @@ -33,7 +33,7 @@ make installer-test bats tests/install_tests.bats ``` -Expected output: `1..111` followed by `ok N ` for every test. +Expected output: `1..112` followed by `ok N ` for every test. Keep the count in this file in sync when you add tests — it's the quickest way to notice a test silently failing to register. @@ -88,7 +88,7 @@ Tests that exercise the validators individually stub `kubectl`/`helm`/`docker` a shell functions instead, echoing whatever the check parses (a `kubeletVersion`, a `helm version --short` string, an allocatable quantity, and so on). -## Current coverage — 111 tests +## Current coverage — 112 tests | Phase / area | Tests | |--------------|-------| diff --git a/Makefile b/Makefile index 02fb241d..c714e5de 100644 --- a/Makefile +++ b/Makefile @@ -42,6 +42,25 @@ installer-lint: ## Syntax-check and shellcheck scripts/install.sh @bash -n scripts/install.sh @shellcheck scripts/install.sh +# Symlink rather than copy, so the command tracks the working tree and can still find the +# chart next to it (a copy has no chart, and reports its version as unknown). +INSTALLER_BIN_DIR ?= $(HOME)/.local/bin + +.PHONY: installer-link +installer-link: ## Put mlrun-ce-installer on PATH, pointing at this checkout + @mkdir -p "$(INSTALLER_BIN_DIR)" + @ln -sf "$(CURDIR)/scripts/install.sh" "$(INSTALLER_BIN_DIR)/mlrun-ce-installer" + @echo "linked $(INSTALLER_BIN_DIR)/mlrun-ce-installer -> $(CURDIR)/scripts/install.sh" + @case ":$$PATH:" in \ + *":$(INSTALLER_BIN_DIR):"*) ;; \ + *) echo "note: $(INSTALLER_BIN_DIR) is not on PATH — add it, or set INSTALLER_BIN_DIR" ;; \ + esac + +.PHONY: installer-unlink +installer-unlink: ## Remove the mlrun-ce-installer symlink + @rm -f "$(INSTALLER_BIN_DIR)/mlrun-ce-installer" + @echo "removed $(INSTALLER_BIN_DIR)/mlrun-ce-installer" + .PHONY: helm-lint helm-lint: helm-repo-add ## Lint Helm Chart @helm lint charts/mlrun-ce diff --git a/scripts/README.md b/scripts/README.md index b893dbcc..7e7b9aef 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -58,6 +58,18 @@ mlrun-ce-installer version Still installs the published chart. Add `--chart-path ./charts/mlrun-ce` to install the chart from your working tree instead. +To get the `mlrun-ce-installer` command while working on a clone, symlink it onto your +PATH: + +```bash +make installer-link # links into ~/.local/bin +make installer-link INSTALLER_BIN_DIR=/usr/local/bin # or somewhere else on PATH +``` + +The link points back at your working tree, so edits take effect immediately and +`mlrun-ce-installer version` reports the chart version rather than `unknown`. `make +installer-unlink` removes it. + --- ## Commands @@ -83,11 +95,12 @@ Colored output is suppressed automatically when stdout isn't a terminal, and whe ## Versioning and releases The installer has no version of its own. It ships with the chart and is released by the -same tag, so `install.sh --version` reads the version straight out of +same tag, so `mlrun-ce-installer version` reads the version straight out of `charts/mlrun-ce/Chart.yaml` beside it — bumping the chart bumps the installer, with no -second copy to keep in step. Run standalone (`curl | bash`, or copied to a bin directory) -there is no chart to read and nothing recording where the script came from, so it reports -`unknown`; that's what pinning to a release tag answers. +second copy to keep in step. Symlinks are resolved first, so a link onto your PATH still +finds the chart in the checkout it points at. Run standalone (`curl | bash`, or copied to +a bin directory) there is no chart to read and nothing recording where the script came +from, so it reports `unknown`; that's what pinning to a release tag answers. They're coupled on purpose. The installer encodes chart internals — the chart's fixed NodePorts, and the `--set` value paths it writes — so an installer and a chart from the diff --git a/scripts/install.sh b/scripts/install.sh index cd578404..69f02514 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -111,8 +111,21 @@ helm() { command helm ${KUBE_CONTEXT:+--kube-context "${KUBE_CONTEXT}"} "$@"; } # directory there is no chart to read, and the version is genuinely unknown: nothing in a # standalone script records which commit it came from. Pin by release tag to know. installer_version() { - local script_path script_dir chart_yaml + local script_path target script_dir chart_yaml script_path="${BASH_SOURCE[0]:-$0}" + + # Follow symlinks by hand: `readlink -f` is GNU-only, and the common way to put this + # on PATH during development is a symlink into a checkout, where the link's own + # directory has no chart in it. Loop rather than resolve once, since a link can chain. + while [[ -L "${script_path}" ]]; do + target="$(readlink "${script_path}")" + if [[ "${target}" == /* ]]; then + script_path="${target}" + else + script_path="$(dirname "${script_path}")/${target}" + fi + done + script_dir="$(cd "$(dirname "${script_path}")" 2>/dev/null && pwd)" || script_dir="" chart_yaml="${script_dir}/../charts/mlrun-ce/Chart.yaml" diff --git a/tests/install_tests.bats b/tests/install_tests.bats index 01076571..eae4cbe0 100644 --- a/tests/install_tests.bats +++ b/tests/install_tests.bats @@ -152,6 +152,20 @@ _empty_bin() { [ "$output" = "mlrun-ce installer 9.9.9-rc.1" ] } +# Symlinking onto PATH is how you'd run this as a command during development, and the +# link's own directory has no chart in it — resolve to the real file before looking. +@test "installer_version follows a symlink back to the chart" { + local linkdir + linkdir="$BATS_TMPDIR/linkbin" + rm -rf "$linkdir" + mkdir -p "$linkdir" + ln -s "$(cd "$(dirname "$SCRIPT")" && pwd)/$(basename "$SCRIPT")" \ + "$linkdir/mlrun-ce-installer" + run bash "$linkdir/mlrun-ce-installer" version + [ "$status" -eq 0 ] + [[ "$output" != *"unknown"* ]] +} + # curl | bash, or copied to /usr/local/bin: no chart to read, and nothing in the script # records where it came from, so say so rather than inventing a version. @test "installer_version reports unknown when running standalone" { From ccca666e3ca9c383e88b2cbc198f9b91bde98505 Mon Sep 17 00:00:00 2001 From: royischoss Date: Thu, 3 Sep 2026 17:00:18 +0300 Subject: [PATCH 13/15] fix co-pilot review --- .claude/skills/run-tests/SKILL.md | 4 +-- scripts/AGENTS.md | 12 ++++++--- scripts/docs/parameters.md | 2 +- scripts/install.sh | 13 ++++++++-- tests/install_tests.bats | 42 +++++++++++++++++++++++++++++++ 5 files changed, 65 insertions(+), 8 deletions(-) diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md index d2a6f456..dc9c7f5a 100644 --- a/.claude/skills/run-tests/SKILL.md +++ b/.claude/skills/run-tests/SKILL.md @@ -33,7 +33,7 @@ make installer-test bats tests/install_tests.bats ``` -Expected output: `1..112` followed by `ok N ` for every test. +Expected output: `1..115` followed by `ok N ` for every test. Keep the count in this file in sync when you add tests — it's the quickest way to notice a test silently failing to register. @@ -88,7 +88,7 @@ Tests that exercise the validators individually stub `kubectl`/`helm`/`docker` a shell functions instead, echoing whatever the check parses (a `kubeletVersion`, a `helm version --short` string, an allocatable quantity, and so on). -## Current coverage — 112 tests +## Current coverage — 115 tests | Phase / area | Tests | |--------------|-------| diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 14494de8..5a6469fa 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -45,9 +45,11 @@ IngressClass exists. ## Versioning and releases -`installer_version()` (printed by `--version`) reads `version:` out of +`installer_version()` (printed by the `version` command) reads `version:` out of `charts/mlrun-ce/Chart.yaml` next to the script, so bumping the chart bumps the installer -and there's no second copy to carry forward. Running standalone — `curl | bash`, or copied +and there's no second copy to carry forward. It walks symlinks to the real file first — +`make installer-link` puts the command on PATH as a link into the checkout, and the link's +own directory has no chart in it. Running standalone — `curl | bash`, or copied to a bin directory — there's no chart to read and nothing in the script recording its origin, so it reports `unknown` rather than inventing a number; that's the case pinning by release tag exists to answer. @@ -138,6 +140,10 @@ node image is the safest choice. `VAR=x source install.sh` prefix form, since bash discards that prefix assignment when `source` returns and `set -u` then trips inside `helm_install`. + Follow-up: `do_uninstall` kept its literal `960s` and so ignored the new variable — + same default, but a raised `HELM_TIMEOUT` didn't reach uninstall. It now passes + `--timeout "${HELM_TIMEOUT}"` too. + - **`do_hard_clean()`'s force-delete fallback could hang indefinitely** (found via live testing against a real remote cluster — a `--hard-clean` run sat blocked for 18+ hours): both the PVC and PV delete loops fall back to @@ -193,7 +199,7 @@ node image is the safest choice. ## Testing -- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 111 tests, no cluster needed (sources +- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 115 tests, no cluster needed (sources `install.sh` with `INSTALL_SH_SOURCE_ONLY=true`, stubs external binaries). - **A green local run on macOS does not mean a green CI run.** bats aborts a test on the first failed assertion via `set -e`, and under macOS's system bash (3.2) diff --git a/scripts/docs/parameters.md b/scripts/docs/parameters.md index 34dc4308..7bf27331 100644 --- a/scripts/docs/parameters.md +++ b/scripts/docs/parameters.md @@ -88,7 +88,7 @@ Options: | `EXTERNAL_HOST_ADDRESS`| — | Host address the cluster is reachable at (see [FAQ](faq.md) for the autodetect fallback chain) | | `SKIP_REGISTRY_SECRET` | `false` | Set to `true` to skip secret creation | | `SKIP_VALIDATORS` | `false` | Set to `true` to skip the pre-install validators | -| `HELM_TIMEOUT` | `960s` | Timeout for helm's `--wait` on install/upgrade. Raise it on slow networks — a cold pull of the 4.2Gi jupyter image alone can take ~6 minutes | +| `HELM_TIMEOUT` | `960s` | Timeout for helm's `--wait` on install/upgrade, and for `uninstall`. Raise it on slow networks — a cold pull of the 4.2Gi jupyter image alone can take ~6 minutes | | `MIN_K8S_VERSION` | — (no floor) | Kubernetes version to warn below (`MAJOR.MINOR`); never blocks the install | | `MIN_HELM_VERSION` | `3.6` | Helm CLI version floor the blocking validator enforces (`MAJOR.MINOR`) | | `DISABLE_SYSTEM_MONITORING` | `false` | Set to `true` to disable the Grafana/Prometheus stack | diff --git a/scripts/install.sh b/scripts/install.sh index 69f02514..017f46f4 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -1076,6 +1076,11 @@ parse_args() { collector) ENABLE_OTEL_OPERATOR="true" ENABLE_OTEL_COLLECTOR="true" + # Set, not just left alone: a mode names a complete state, so + # `collector` has to mean "no auto-instrumentation" even when the + # env vars or an earlier granular flag turned those two on. + ENABLE_OTEL_NAMESPACE_LABEL="false" + ENABLE_OTEL_INSTRUMENTATION="false" ;; full) ENABLE_OTEL_OPERATOR="true" @@ -1189,7 +1194,7 @@ do_uninstall() { log_warn "Release '${RELEASE_NAME}' not found in namespace '${NAMESPACE}' (already uninstalled?)." else log_info "Uninstalling MLRun CE release '${RELEASE_NAME}' from namespace '${NAMESPACE}'..." - helm uninstall "${RELEASE_NAME}" --namespace "${NAMESPACE}" --timeout 960s + helm uninstall "${RELEASE_NAME}" --namespace "${NAMESPACE}" --timeout "${HELM_TIMEOUT}" log_info "Uninstall complete." fi @@ -1285,7 +1290,11 @@ validate_helm_version() { # Blocking: cluster must have a default StorageClass (chart's PVCs rely on one). validate_storage_class() { local default_sc - default_sc="$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{"="}{.metadata.annotations.storageclass\.kubernetes\.io/is-default-class}{"\n"}{end}' 2>/dev/null | grep '=true$' || true)" + # Both annotations: Kubernetes still honours the deprecated beta key, and clusters + # provisioned years ago can carry only that one. Missing it would fail the install + # over a StorageClass that does in fact default. Rows are name==, so + # match =true in either field rather than only at end of line. + default_sc="$(kubectl get storageclass -o jsonpath='{range .items[*]}{.metadata.name}{"="}{.metadata.annotations.storageclass\.kubernetes\.io/is-default-class}{"="}{.metadata.annotations.storageclass\.beta\.kubernetes\.io/is-default-class}{"\n"}{end}' 2>/dev/null | grep -E '=true(=|$)' || true)" if [[ -z "$default_sc" ]]; then log_error " No default StorageClass found in the cluster. MLRun CE requires a default StorageClass for its PVCs." return 1 diff --git a/tests/install_tests.bats b/tests/install_tests.bats index eae4cbe0..a55add5c 100644 --- a/tests/install_tests.bats +++ b/tests/install_tests.bats @@ -835,6 +835,18 @@ EOF [[ "$output" == *"--timeout 1800s"* ]] } +@test "HELM_TIMEOUT also applies to uninstall" { + run bash -c " + export HELM_TIMEOUT=120s + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + check_requirements() { :; } + helm() { [[ \"\$1\" == status ]] && return 0; echo \"HELM_ARGS: \$*\"; } + do_uninstall + " + [ "$status" -eq 0 ] + [[ "$output" == *"--timeout 120s"* ]] +} + @test "load_config maps installer.components.* to DISABLE_* the same as the --disable-* flags" { local cfg="$BATS_TMPDIR/cfg_components.yaml" cat > "$cfg" <<'EOF' @@ -912,6 +924,17 @@ EOF [[ "$output" == *"op=true col=true ns=false inst=false"* ]] } +@test "--enable-otel collector turns namespaceLabel/instrumentation back off" { + run bash -c " + export ENABLE_OTEL_NAMESPACE_LABEL=true ENABLE_OTEL_INSTRUMENTATION=true + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + parse_args --enable-otel collector + echo \"op=\$ENABLE_OTEL_OPERATOR col=\$ENABLE_OTEL_COLLECTOR ns=\$ENABLE_OTEL_NAMESPACE_LABEL inst=\$ENABLE_OTEL_INSTRUMENTATION\" + " + [ "$status" -eq 0 ] + [[ "$output" == *"op=true col=true ns=false inst=false"* ]] +} + @test "--enable-otel off sets all 4 false" { run bash -c " INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' @@ -1398,6 +1421,25 @@ EOF [[ "$output" == *"Default StorageClass: fast"* ]] } +@test "validate_storage_class accepts the deprecated beta default-class annotation" { + # The stub answers according to the jsonpath it is handed, so this fails if the query + # stops asking for the beta annotation — a stub that echoed a fixed row would pass + # either way. + run bash -c " + INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' + kubectl() { + if [[ \"\$*\" == *beta*is-default-class* ]]; then + printf 'standard==\nlegacy==true\n' + else + printf 'standard=\nlegacy=\n' + fi + } + validate_storage_class + " + [ "$status" -eq 0 ] + [[ "$output" == *"Default StorageClass: legacy"* ]] +} + @test "validate_ingress_controller skips when --enable-ingress is not used" { run bash -c " INSTALL_SH_SOURCE_ONLY=true source '$SCRIPT' From 6b445faee6e6da7ba6542c6853ebe337d094511d Mon Sep 17 00:00:00 2001 From: royischoss Date: Mon, 7 Sep 2026 13:10:43 +0300 Subject: [PATCH 14/15] Make --local-registry honour --dry-run deploy_local_registry() had no DRY_RUN guard, so it ran kubectl apply unconditionally. On a cluster without the namespace the apply failed and errexit aborted the run, making --local-registry --dry-run unusable. On a cluster where the namespace existed the apply succeeded, so a run advertised as rendering-only really deployed a registry Deployment and Service and reported success. The guard returns early under DRY_RUN, placed after the LOCAL_REGISTRY_URL assignment so the URL still reaches the rendered --set flags. Verified live: the dry run renders it into nuclio's registry_url ConfigMap and mlrun's api chief/worker deployments while creating nothing. CI missed this because the kind-install job uses --local-registry for a real install, never with --dry-run. Co-authored-by: Cursor --- .claude/skills/run-tests/SKILL.md | 4 +-- scripts/AGENTS.md | 17 ++++++++++++- scripts/install.sh | 9 +++++++ tests/install_tests.bats | 42 +++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/.claude/skills/run-tests/SKILL.md b/.claude/skills/run-tests/SKILL.md index dc9c7f5a..cb3aff7f 100644 --- a/.claude/skills/run-tests/SKILL.md +++ b/.claude/skills/run-tests/SKILL.md @@ -33,7 +33,7 @@ make installer-test bats tests/install_tests.bats ``` -Expected output: `1..115` followed by `ok N ` for every test. +Expected output: `1..118` followed by `ok N ` for every test. Keep the count in this file in sync when you add tests — it's the quickest way to notice a test silently failing to register. @@ -88,7 +88,7 @@ Tests that exercise the validators individually stub `kubectl`/`helm`/`docker` a shell functions instead, echoing whatever the check parses (a `kubeletVersion`, a `helm version --short` string, an allocatable quantity, and so on). -## Current coverage — 115 tests +## Current coverage — 118 tests | Phase / area | Tests | |--------------|-------| diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 5a6469fa..9cb6f544 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -167,6 +167,21 @@ node image is the safest choice. and the warning read "~0Gi" instead of the real ~52Gi. Fix: added `_allocatable_to_ki()`, a small quantity parser that handles `Ki`/`Mi`/`Gi`/`Ti` suffixes and a bare byte-integer form, used by both the memory and ephemeral-storage loops. +- **`deploy_local_registry()` ignored `--dry-run` entirely** (found while rehearsing a demo + of `--local-registry` on `docker-desktop`): the function had no `DRY_RUN` guard, so it ran + `kubectl apply` unconditionally. Two failure modes, one loud and one quiet. On a cluster + without the namespace — the normal case for a first dry run — the apply failed with a raw + `Error from server (NotFound): namespaces "mlrun" not found`, `errexit` aborted, and the + run exited 1, so `--local-registry --dry-run` was simply unusable. On a cluster where the + namespace already existed, the apply *succeeded*: a run advertised as rendering-only + really deployed a `local-registry` Deployment and Service, and reported success. CI never + caught it because the `kind-install` job uses `--local-registry` for a real install, never + with `--dry-run`. Fix: an early `return 0` under `DRY_RUN`, placed *after* the + `LOCAL_REGISTRY_URL` assignment so the URL still reaches the rendered `--set` flags — + verified live, the dry run renders the URL into nuclio's `registry_url` ConfigMap and + mlrun's api chief/worker deployments while creating nothing. Three tests cover it: no + apply under dry-run, the URL still resolving, and a real run still applying. + - **`resolve_external_host()`'s docker-desktop/minikube autodetect ignored `KUBE_CONTEXT`** (install.sh:602, found via live testing against a remote `--kube-context`): the `kubectl config current-context` check always reports the kubeconfig's *ambient* @@ -199,7 +214,7 @@ node image is the safest choice. ## Testing -- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 115 tests, no cluster needed (sources +- Unit: `make installer-test` (`bats tests/install_tests.bats`) — 118 tests, no cluster needed (sources `install.sh` with `INSTALL_SH_SOURCE_ONLY=true`, stubs external binaries). - **A green local run on macOS does not mean a green CI run.** bats aborts a test on the first failed assertion via `set -e`, and under macOS's system bash (3.2) diff --git a/scripts/install.sh b/scripts/install.sh index 017f46f4..d1f962f2 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -366,6 +366,15 @@ deploy_local_registry() { LOCAL_REGISTRY_URL="local-registry.${NAMESPACE}.svc.cluster.local:5000" fi + # Guard goes after the URL is resolved, not before: the URL still has to reach the + # rendered --set flags for the dry-run to represent the real install. Everything below + # this point mutates the cluster, which a dry run must not do — and on a cluster where + # the namespace does exist, an unguarded apply would quietly deploy a real registry. + if [[ "${DRY_RUN}" == "true" ]]; then + log_info "Dry-run: would deploy local registry at '${LOCAL_REGISTRY_URL}'" + return 0 + fi + log_info "Deploying local Docker registry..." kubectl apply -f - --namespace "${NAMESPACE}" < Date: Mon, 7 Sep 2026 13:45:55 +0300 Subject: [PATCH 15/15] Enhance uninstall command timeout information Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- scripts/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/README.md b/scripts/README.md index 7e7b9aef..57f1371c 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -188,7 +188,7 @@ Pre-install validators still run in `--dry-run` — see [Configuration](docs/con ./scripts/install.sh --uninstall ``` -This runs `helm uninstall` with a 960s timeout. The namespace and CRDs are **not** deleted. For deleting persistent data too, see the [FAQ](docs/faq.md#deleting-everything-including-the-namespace). +This runs `helm uninstall` with a timeout controlled by `HELM_TIMEOUT` (default `960s`). The namespace and CRDs are **not** deleted. For deleting persistent data too, see the [FAQ](docs/faq.md#deleting-everything-including-the-namespace). ---