diff --git a/api/v1/weightsandbiases_conversion_clickhouse_replication_test.go b/api/v1/weightsandbiases_conversion_clickhouse_replication_test.go new file mode 100644 index 00000000..02c00669 --- /dev/null +++ b/api/v1/weightsandbiases_conversion_clickhouse_replication_test.go @@ -0,0 +1,189 @@ +package v1 + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" + + appsv2 "github.com/wandb/operator/api/v2" +) + +// externalClickHouseValues is a minimal v1 external ClickHouse block, so +// conversion has a connection to attach the replication flag to. +func externalClickHouseValues(extra map[string]interface{}) map[string]interface{} { + global := map[string]interface{}{ + "clickhouse": map[string]interface{}{ + "install": false, + "host": "clickhouse-wandb.clickhouse.svc.cluster.local", + "port": int64(8123), + "database": "weave", + "user": "weave", + }, + } + values := map[string]interface{}{"global": global} + for k, v := range extra { + if k != "global" { + values[k] = v + continue + } + // One level deeper, so adding global.clickhouse.replicated doesn't wipe out + // the connection fields the flag needs something to attach to. + for gk, gv := range v.(map[string]interface{}) { + existing, isMap := global[gk].(map[string]interface{}) + incoming, alsoMap := gv.(map[string]interface{}) + if isMap && alsoMap { + for ik, iv := range incoming { + existing[ik] = iv + } + continue + } + global[gk] = gv + } + } + return values +} + +func convertedClickHouse(t *testing.T, values map[string]interface{}) *appsv2.ClickHouseConnection { + t.Helper() + dst := &appsv2.WeightsAndBiases{} + require.NoError(t, newV1(values).ConvertTo(dst)) + return dst.Spec.ClickHouse[appsv2.DefaultInstanceName].ExternalClickHouse +} + +// convertedClickHousePending decodes the clickhouse-pending annotation. Conversion +// leaves the connection literals it can't express as selectors there, plus the +// structured global.clickhouse.replicated flag, for the reconciler to drain into +// the connection Secret. +func convertedClickHousePending(t *testing.T, values map[string]interface{}) map[string]interface{} { + t.Helper() + dst := &appsv2.WeightsAndBiases{} + require.NoError(t, newV1(values).ConvertTo(dst)) + + raw, found := dst.Annotations[ClickHousePendingAnnotation] + if !found { + return map[string]interface{}{} + } + var pending map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(raw), &pending)) + return pending +} + +func legacyOverrideEnvNames(t *testing.T, values map[string]interface{}) map[string][]string { + t.Helper() + dst := &appsv2.WeightsAndBiases{} + require.NoError(t, newV1(values).ConvertTo(dst)) + + out := map[string][]string{} + for key, override := range dst.Spec.Wandb.LegacyOverrides { + for _, env := range override.Env { + out[key] = append(out[key], env.Name) + } + } + return out +} + +// The structured global.clickhouse.replicated flag is written into the +// clickhouse-pending annotation for the reconciler to materialize. Env-var +// replication (WF_CLICKHOUSE_REPLICATED[_CLUSTER]) is handled separately at +// reconcile from legacyOverrides. +func TestConvertTo_ClickHouseReplicatedFlagToPending(t *testing.T) { + pending := convertedClickHousePending(t, externalClickHouseValues(map[string]interface{}{ + "global": map[string]interface{}{ + "clickhouse": map[string]interface{}{"replicated": true}, + }, + })) + + require.Equal(t, "true", pending["replicated"]) + require.NotContains(t, pending, "replicatedCluster", "the structured flag carries no cluster name") +} + +// The flag merges with the connection literals mapClickHouse already wrote, +// rather than clobbering them. +func TestConvertTo_ClickHouseReplicatedFlagMergesWithLiterals(t *testing.T) { + pending := convertedClickHousePending(t, externalClickHouseValues(map[string]interface{}{ + "global": map[string]interface{}{ + "clickhouse": map[string]interface{}{"replicated": true}, + }, + })) + + require.Equal(t, "clickhouse-wandb.clickhouse.svc.cluster.local", pending["host"]) + require.Equal(t, "weave", pending["database"]) + require.Equal(t, "weave", pending["user"]) + require.Equal(t, "8123", pending["port"], "a numeric v1 port must survive the merge as written") + require.Equal(t, "true", pending["replicated"]) +} + +// No flag: the connection converts but publishes no replication key. +func TestConvertTo_ClickHouseReplicatedFlagAbsent(t *testing.T) { + conn := convertedClickHouse(t, externalClickHouseValues(nil)) + require.NotNil(t, conn) + + pending := convertedClickHousePending(t, externalClickHouseValues(nil)) + require.NotContains(t, pending, "replicated") +} + +// A non-boolean flag is treated as absent (nestedBoolLenient) rather than failing +// conversion; the value can't make a v1 object unservable. +func TestConvertTo_ClickHouseReplicatedFlagNonBooleanIgnored(t *testing.T) { + pending := convertedClickHousePending(t, externalClickHouseValues(map[string]interface{}{ + "global": map[string]interface{}{ + "clickhouse": map[string]interface{}{"replicated": "yes-please"}, + }, + })) + + require.NotContains(t, pending, "replicated") +} + +// Managed ClickHouse (install=true) creates no external connection, so there is +// nothing to attach the replication flag to. +func TestConvertTo_ClickHouseInstallTrueLeavesNoExternal(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(map[string]interface{}{ + "global": map[string]interface{}{ + "clickhouse": map[string]interface{}{"install": true, "replicated": true}, + }, + }) + require.NoError(t, src.ConvertTo(dst)) + + require.Empty(t, dst.Spec.ClickHouse, + "install=true must leave ClickHouse to the defaulter, which makes it managed") +} + +// The replication env vars now flow through into legacyOverrides verbatim; the +// reconciler maps and removes them. Previously conversion stripped them here. +func TestConvertTo_ClickHouseReplicationEnvPassesThrough(t *testing.T) { + names := legacyOverrideEnvNames(t, externalClickHouseValues(map[string]interface{}{ + "global": map[string]interface{}{ + "extraEnv": map[string]interface{}{ + "ENABLE_REGISTRY_UI": "true", + "WF_CLICKHOUSE_REPLICATED": "true", + "WF_CLICKHOUSE_REPLICATED_CLUSTER": "weavecluster", + }, + }, + })) + + global := names[appsv2.LegacyOverridesGlobalKey] + require.Contains(t, global, "WF_CLICKHOUSE_REPLICATED") + require.Contains(t, global, "WF_CLICKHOUSE_REPLICATED_CLUSTER") + require.Contains(t, global, "ENABLE_REGISTRY_UI") +} + +// The connection must still round-trip through the raw v1 annotation untouched, +// so the original env survives even though the reconciler will map it. +func TestConvertTo_ClickHouseReplicationPreservesV1Annotation(t *testing.T) { + dst := &appsv2.WeightsAndBiases{} + src := newV1(externalClickHouseValues(map[string]interface{}{ + "global": map[string]interface{}{ + "extraEnv": map[string]interface{}{"WF_CLICKHOUSE_REPLICATED": "true"}, + }, + })) + require.NoError(t, src.ConvertTo(dst)) + + var decoded map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(dst.Annotations[v1ValuesAnnotation]), &decoded)) + global := decoded["global"].(map[string]interface{}) + extra := global["extraEnv"].(map[string]interface{}) + require.Equal(t, "true", extra["WF_CLICKHOUSE_REPLICATED"], + "the v1-values annotation must keep the original env for round-tripping") +} diff --git a/api/v1/weightsandbiases_conversion_mapping.go b/api/v1/weightsandbiases_conversion_mapping.go index 1a581361..da627d61 100644 --- a/api/v1/weightsandbiases_conversion_mapping.go +++ b/api/v1/weightsandbiases_conversion_mapping.go @@ -596,6 +596,18 @@ func mapClickHouse(globalMap map[string]interface{}, dst *appsv2.WeightsAndBiase } } + // The structured replicated flag travels as a pending literal, so the + // reconciler materializes it into the connection Secret. It applies only when + // a connection exists (managed ClickHouse derives its own topology), and the + // env var WF_CLICKHOUSE_REPLICATED can still override it at reconcile. + if sawField { + if flag, ok, flagErr := nestedBoolLenient(chMap, "replicated"); flagErr != nil { + return fmt.Errorf("spec.values.global.clickhouse.replicated: %w", flagErr) + } else if ok { + remaining[clickHousePendingReplicatedKey] = strconv.FormatBool(flag) + } + } + if !sawField { return nil } @@ -610,6 +622,30 @@ func mapClickHouse(globalMap map[string]interface{}, dst *appsv2.WeightsAndBiase return nil } +// clickHousePendingReplicatedKey carries the structured global.clickhouse.replicated +// flag in the clickhouse-pending annotation; migrateLegacyClickHouse turns it into a +// key of the converted connection Secret. The WF_CLICKHOUSE_REPLICATED[_CLUSTER] env +// vars are mapped at reconcile from spec.wandb.legacyOverrides, not here. +const clickHousePendingReplicatedKey = "replicated" + +// nestedBoolLenient reads a bool that v1 may have stringly-typed, treating an +// uninterpretable value as absent so it can't make a v1 object unservable. +func nestedBoolLenient(values map[string]interface{}, path ...string) (bool, bool, error) { + raw, found, err := unstructured.NestedFieldNoCopy(values, path...) + if err != nil || !found { + return false, false, nil + } + s, isScalar := scalarToString(raw) + if !isScalar { + return false, false, nil + } + parsed, parseErr := strconv.ParseBool(s) + if parseErr != nil { + return false, false, nil + } + return parsed, true, nil +} + // redisFields maps each v1 global.redis. to a *RedisConnection setter. var redisFields = []struct { v1Key string diff --git a/api/v2/weightsandbiases_types.go b/api/v2/weightsandbiases_types.go index 67053400..b5ebaf21 100644 --- a/api/v2/weightsandbiases_types.go +++ b/api/v2/weightsandbiases_types.go @@ -821,6 +821,12 @@ type ClickHouseConnection struct { Password corev1.SecretKeySelector `json:"password,omitempty"` URL corev1.SecretKeySelector `json:"url,omitempty"` + + // Replicated tells applications whether to create ReplicatedMergeTree tables. + Replicated corev1.SecretKeySelector `json:"replicated,omitempty"` + + // CLUSTER. Only meaningful when Replicated is true. + ClusterName corev1.SecretKeySelector `json:"clusterName,omitempty"` } type ClickHouseConfig struct { diff --git a/api/v2/zz_generated.deepcopy.go b/api/v2/zz_generated.deepcopy.go index db93560a..8aebbfdb 100644 --- a/api/v2/zz_generated.deepcopy.go +++ b/api/v2/zz_generated.deepcopy.go @@ -296,6 +296,8 @@ func (in *ClickHouseConnection) DeepCopyInto(out *ClickHouseConnection) { in.Username.DeepCopyInto(&out.Username) in.Password.DeepCopyInto(&out.Password) in.URL.DeepCopyInto(&out.URL) + in.Replicated.DeepCopyInto(&out.Replicated) + in.ClusterName.DeepCopyInto(&out.ClusterName) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClickHouseConnection. diff --git a/config/crd/bases/apps.wandb.com_weightsandbiases.yaml b/config/crd/bases/apps.wandb.com_weightsandbiases.yaml index 8854518a..468409c0 100644 --- a/config/crd/bases/apps.wandb.com_weightsandbiases.yaml +++ b/config/crd/bases/apps.wandb.com_weightsandbiases.yaml @@ -525,6 +525,19 @@ spec: properties: externalClickhouse: properties: + clusterName: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic database: properties: key: @@ -577,6 +590,19 @@ spec: - key type: object x-kubernetes-map-type: atomic + replicated: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic tcpPort: properties: key: @@ -4612,6 +4638,19 @@ spec: type: array connection: properties: + clusterName: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic database: properties: key: @@ -4664,6 +4703,19 @@ spec: - key type: object x-kubernetes-map-type: atomic + replicated: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic tcpPort: properties: key: diff --git a/docs/design/wandb_v2/legacy_env_var_mapping.md b/docs/design/wandb_v2/legacy_env_var_mapping.md new file mode 100644 index 00000000..fef86c35 --- /dev/null +++ b/docs/design/wandb_v2/legacy_env_var_mapping.md @@ -0,0 +1,439 @@ +# Legacy env-var → CR field mapping: from conversion special-case to a reconcile-time registry + +**Status:** Proposal (not yet implemented) +**Related:** [legacy_overrides.md](legacy_overrides.md) (the `spec.wandb.legacyOverrides` mechanism this builds on) + +## Problem + +v1 users configured server behavior by setting raw environment variables +(`env` / `extraEnv` in helm values). v2 models many of those knobs as typed CR +fields. Today the v1 → v2 conversion webhook special-cases exactly **two** env +vars — `WF_CLICKHOUSE_REPLICATED` and `WF_CLICKHOUSE_REPLICATED_CLUSTER` — to +turn them into typed ClickHouse topology fields. Every new env var we discover +would need the same bespoke machinery bolted into the webhook. + +That does not scale, and the webhook is the wrong place for it: + +- **It is stateless.** The conversion webhook cannot create Secrets, so every + literal it can't express as a selector is stashed in a + `legacy.operator.wandb.com/*-pending` annotation and drained later by the + reconciler. Env-var mapping inherits that whole two-hop dance. +- **It resolves the server manifest at admission time** (with a failure-cooldown + cache) just to know which values sections are applications. +- **The mapping is bound to the v2 CRD shape**, which the reconciler owns. Field + layout, instance keying, and managed-vs-external rules live on the reconcile + side; expressing them in `api/v1` couples the spoke version to the hub's + internals. + +## Key insight + +`spec.wandb.legacyOverrides` **already carries every other env var verbatim** +into the CR. `mapLegacyOverrides` walks the global and per-application sections +and copies their `env`/`extraEnv` into +`spec.wandb.legacyOverrides[].Env` as `[]corev1.EnvVar`. The only +reason the reconciler can't already see the two ClickHouse vars is that +`legacyEnvFromSection` explicitly skips them (`isClickHouseReplicationEnv`). + +Remove that one exclusion and **all** env vars flow into `legacyOverrides`. The +reconciler already holds the CR, a `client.Client` (so it can materialize +Secrets), and the resolved manifest. So the env → CR mapping is a natural +reconcile-time step, driven by a **hardcoded registry** bound to the CRD. + +## Current architecture + +Two independent mechanisms exist today. + +### Mechanism A — `legacyOverrides` passthrough (generic, no typing) + +``` +v1 values (env/extraEnv) + └─ conversion: mapLegacyOverrides → legacyEnvFromSection → legacyEnvVar + · env beats extraEnv; sorted by name; helm-template + non-scalar dropped + · isClickHouseReplicationEnv(k) → SKIP ← the special case + → spec.wandb.legacyOverrides[].Env ([]corev1.EnvVar) + +reconcile: reconcileApplications → per-app env pipeline + └─ applyLegacyOverrideEnv(...) applied LAST (beats manifest + injected env) + · global layer, then per-app layer (per-app wins) +``` + +Env vars land as raw pod env. They are never promoted to typed fields. +`validateLegacyOverrides` only *logs* keys that map to no manifest application. + +Files: [weightsandbiases_conversion_overrides.go](../../../api/v1/weightsandbiases_conversion_overrides.go), +[legacy_overrides.go](../../../internal/controller/reconciler/legacy_overrides.go). + +### Mechanism B — ClickHouse replication special-case (typed, bespoke) + +``` +v1 values + └─ conversion: + legacyEnvFromSection EXCLUDES the two vars (isClickHouseReplicationEnv) + mapClickHouseReplication → harvestClickHouseReplication + · scans every section's env/extraEnv (env beats extraEnv) + · per-app beats global; apps that disagree → hard error + · fallback: structured global.clickhouse.replicated (env beats flag) + · only attaches when an ExternalClickHouse connection exists + (managed derives its own topology → drop) + → writeAnnotation(ClickHousePendingAnnotation, {replicated, replicatedCluster}) + +reconcile: + migrateLegacyClickHouse drains the pending annotation + → -clickhouse-converted Secret (keys replicated / replicatedCluster) + → sets ExternalClickHouse.Replicated / .ClusterName selectors (only-fill-if-zero) + +pod build (read side): + resolveEnvvars, manifest source {type: clickhouse, field: replicated|replicated-cluster} + → SecretKeyRef into the connection Secret + → SKIPS the env var when the selector is unpublished (pods.go:283-294) +``` + +Files: [weightsandbiases_conversion_mapping.go](../../../api/v1/weightsandbiases_conversion_mapping.go) +(lines ~618-802), [migrate_legacy.go](../../../internal/controller/reconciler/migrate_legacy.go) +(lines ~197-260), [pods.go](../../../internal/controller/reconciler/pods.go) (lines ~259-301). + +### The load-bearing fact + +Every field of every `*Connection` struct (`ClickHouseConnection`, +`MysqlConnection`, `RedisConnection`, `ObjectStoreConnection`) and the four +OIDC credential fields are `corev1.SecretKeySelector`. There are **no plain +`string`/`*bool` connection fields.** `Replicated` and `ClusterName` are +`SecretKeySelector`, not `*bool`/`string`. So "map an env var into a connection +field" always means **materialize a Secret value + point a selector at it** — +never "assign a Go string." Only a handful of `spec.wandb.*` / `spec.global.*` +scalars (`OidcSpec.SessionLength`, `Wandb.License`, `Wandb.BucketProxy`, …) are +plain fields. + +The round-trip only works because the manifest **re-injects** the value from the +mapped field on the read side. Removing the env var from `legacyOverrides` is +safe *only* when a manifest source reads it back out of the field we wrote (for +replication, the `clickhouse` source does, and skips it when unpublished). This +is a hard constraint on the registry (see [Round-trip safety](#round-trip-safety)). + +## Proposed design + +### Division of responsibility + +| Input | Owner | Why | +|-------|-------|-----| +| **Raw env vars** (`env`/`extraEnv`) | **Reconciler**, via the registry | Env values already ride into `legacyOverrides`; the reconciler can materialize Secrets and knows the CRD shape. | +| **Structured helm values** (`global.clickhouse.host`, …) | Conversion webhook (unchanged) | These are already typed and unambiguous; `mapClickHouse` / `clickHouseFields` etc. stay as-is. | + +The conversion webhook stops intercepting env vars. A new reconcile step maps +registered env vars from `legacyOverrides` into typed fields and removes them. + +### The registry + +A hardcoded, declarative table — one entry per mappable env var — living in a new +`internal/controller/reconciler/legacy_env_mapping.go`. It generalizes the +existing `clickHouseFields` `setRef`-closure pattern +([weightsandbiases_conversion_mapping.go:532](../../../api/v1/weightsandbiases_conversion_mapping.go)). + +```go +// legacyEnvMappings is the hardcoded registry of v1 env vars the reconciler +// promotes into typed v2 fields, then removes from legacyOverrides. Bound to +// the CRD; grows as we model more knobs. Only add an env var here when a +// manifest source re-injects its value from the target field (see round-trip +// safety) — otherwise removal silently drops it from the pod. +var legacyEnvMappings = []legacyEnvMapping{ + { + env: "WF_CLICKHOUSE_REPLICATED", + scope: scopeDatastore, // one value for the datastore; sources must agree + onExist: overrideConversionDerived, + apply: externalClickHouseSelector("replicated"), + }, + { + env: "WF_CLICKHOUSE_REPLICATED_CLUSTER", + scope: scopeDatastore, + onExist: overrideConversionDerived, + apply: externalClickHouseSelector("clusterName"), + }, +} + +type legacyEnvMapping struct { + env string + scope mappingScope // scopeDatastore | scopeGlobal | scopePerApp + onExist conflictPolicy // keepCR (default) | overrideConversionDerived + apply applyFn +} + +// applyFn owns instance resolution, the managed-vs-external guard, Secret +// materialization / selector repointing, and setting the field. It receives the +// full scope-resolved EnvVar (literal Value OR ValueFrom). It returns remove=true +// when the env should be stripped from legacyOverrides (mapped, or intentionally +// dropped); remove=false leaves it as a raw-env passthrough (a valueFrom shape the +// Secret-only target can't represent). +type applyFn func(ctx context.Context, c client.Client, w *v2.WeightsAndBiases, env corev1.EnvVar) (remove bool, err error) +``` + +`scope`: +- **`scopeDatastore`** — a property of the datastore, not a workload. Collect the + env from every `legacyOverrides` section; per-app beats global; sections that + disagree are a hard error (preserves today's conflict semantics). One resolved + value. +- **`scopeGlobal`** / **`scopePerApp`** — future shapes for workload-scoped env + that maps to per-instance or global fields. + +`onExist`: +- **`keepCR`** (default, and the rule for all *future* generic entries) — + CR value is source of truth; if the target field is already set, don't + overwrite, just drop the env. Mirrors migrate_legacy's `fill` (only-fill-zero). +- **`overrideConversionDerived`** — for datastore-topology entries whose only + pre-set source is the conversion webhook's structured-flag mapping. The v1 env + var is the most explicit v1 signal, so it overrides. Safe because a v2-native + user does not carry a v1 env in `legacyOverrides` (the map is conversion-owned). + +### The reconcile step + +Add `mapLegacyEnvToCR(ctx, client, wandb)` and wire it in **immediately after +`migrateLegacyAnnotations`** and before the manifest load, reusing the same +short-circuit idiom ([reconcile_v2.go:167-169](../../../internal/controller/reconciler/reconcile_v2.go)): + +```go +// Migrate legacy v1 conversion annotations into typed spec fields +if res, err := migrateLegacyAnnotations(ctx, client, wandb); err != nil || res.RequeueAfter > 0 { + return res, err +} + +// NEW: promote known legacy env vars into typed fields, drop them from overrides +if res, err := mapLegacyEnvToCR(ctx, client, wandb); err != nil || res.RequeueAfter > 0 { + return res, err +} + +// Fetch manifest early so infra sizing can be applied before provisioning +manifest, err := serverManifest.GetServerManifest(...) +``` + +**Placement rationale.** Because `migrateLegacyAnnotations` short-circuits and +requeues on any change, `mapLegacyEnvToCR` only runs on passes where migration is +already a no-op — i.e. after every `*-pending` annotation is drained and the +external connection (which `scopeDatastore` topology attaches to) already exists. +The step does not need the manifest (it matches on env name against +`legacyOverrides` keys — global plus application names), so keeping it pre-manifest +groups all spec-normalizing mutations together, before any workload env is built. +If a future entry needs the manifest, move the call after `ApplyInfraSizing` +([reconcile_v2.go:184](../../../internal/controller/reconciler/reconcile_v2.go)). + +**Algorithm (per pass):** + +1. If `len(wandb.Spec.Wandb.LegacyOverrides) == 0`, return no-op. +2. For each registry entry, resolve the env value across sections per `scope` + (`scopeDatastore`: per-app beats global, disagreement → error). +3. If found, call `entry.apply(...)` with the full `EnvVar`, honoring `onExist` + and the source shape (see [Source shapes](#source-shapes-literal-vs-valuefrom)): + - literal `Value` → materialize into the converted Secret (merge — see below). + - `ValueFrom.SecretKeyRef` → point the selector directly at the user's Secret. + - other `ValueFrom` → passthrough (`remove=false`, leave the env in place). + - `keepCR` skips the write when the field is already set; managed ClickHouse + drops (no write, but still removed). +4. **Remove the env var from every `legacyOverrides` section** when `remove=true` + (mapped, guard-dropped, or user-field-wins) — the field/manifest is now + authoritative. Passthrough (`remove=false`) leaves it. Prune emptied sections; + prune the map when it empties. +5. If anything changed, `client.Update(ctx, wandb)` + return + `ctrl.Result{RequeueAfter: time.Second}`; else no-op. (Exactly + `migrateLegacyAnnotations`' persistence shape — there is no shared helper, so + this is hand-written the same way.) + +### Source shapes: literal vs ValueFrom + +`legacyOverrides` carries each env var as a full `corev1.EnvVar`, so a mapped var +may be a literal `Value` **or** a `ValueFrom` (`legacyEnvVar` preserves the whole +body). The mapper must handle both — the original harvest read only scalars and +silently dropped `valueFrom`-sourced replication env, a latent bug this fixes. + +Both shapes reach the pod through the same consolidation: external ClickHouse +`WriteState` runs every spec selector through `external.ResolveFields` → +`ResolveSecretKey`, which dereferences whatever Secret each selector points at +(operator-owned **or** the user's) and copies the values into the unified +`wandb-clickhouse-connection` Secret +([clickhouse.go:37-60](../../../internal/controller/infra/external/clickhouse/clickhouse.go)). +So: + +- **literal `Value`** → materialize into `-clickhouse-converted`, point the + selector there (needs the merge below). +- **`ValueFrom.SecretKeyRef`** → set the selector **directly** to the user's + `{Name, Key}` — no Secret write at all; `ResolveFields` reads it. +- **`ValueFrom.ConfigMapKeyRef` / other** → not representable in a Secret-only + selector → passthrough (`remove=false`): leave the env in `legacyOverrides`, + where it still reaches the pod as raw env (the read side skips the manifest + binding when the field is unpublished, so the raw override supplies it). No loss. + +### Secret materialization (merge, don't replace) + +For the **literal** case the value must live in a Secret the selector can point +at. **Reuse the `-clickhouse-converted` Secret, but merge rather than +replace.** `migrateLegacyClickHouse` builds the full data map and +`CreateOrUpdate`s, *replacing* `secret.Data` +([migrate_legacy.go:453](../../../internal/controller/reconciler/migrate_legacy.go); +test `TestMigrateLegacyClickHouse_PreExistingSecretOverwritten`). A naive +`CreateOrUpdate` from `mapLegacyEnvToCR` on the same Secret would clobber the +host/port/user literals migration wrote. Add a small merge helper +(read existing → set the one key → update). The `ValueFrom.SecretKeyRef` case +needs no Secret write. + +### Managed-vs-external guard + +Replication attaches only when `ClickHouse[default].ExternalClickHouse != nil` +(managed ClickHouse derives its own topology). For managed, `apply` writes nothing +and returns `remove=true`, so the env is still removed — preserving +`TestConvertTo_ClickHouseReplicationDroppedWhenManaged`. + +### Round-trip safety + +Removing an env var from `legacyOverrides` only preserves pod behavior if a +manifest source re-injects the value from the field we wrote. For replication the +manifest `{type: clickhouse, field: replicated|replicated-cluster}` source does +exactly that and *skips* the env when the selector is unpublished +([pods.go:283-294](../../../internal/controller/reconciler/pods.go)). + +**Registry contract:** only register an env var whose value the manifest +re-injects from the target field. This is an author rule, enforced by per-version +fixture tests, not by code (the binding is manifest-version-specific). A generic +`custom-resource` dotted-path source already exists +([pods.go:417-434](../../../internal/controller/reconciler/pods.go)) as the mirror +for scalar `spec.*` targets that have no dedicated source type. + +### Precedence summary (after this change) + +For a mapped (registered) env var, effective precedence at the pod becomes: + +1. Explicit v2 CR field set by a user → wins (`keepCR` entries). +2. v1 env var via `legacyOverrides` → the reconcile mapping + (`overrideConversionDerived` beats the conversion-derived structured flag). +3. v1 structured flag via the conversion webhook → the field, when no env maps. +4. Manifest default. + +Unmapped env vars are unchanged: they stay in `legacyOverrides` and +`applyLegacyOverrideEnv` still applies them **last**, beating manifest env. + +## What changes + +### Remove from the conversion webhook (`api/v1`) + +- **`isClickHouseReplicationEnv` exclusion** in `legacyEnvFromSection` + ([overrides.go:252-257](../../../api/v1/weightsandbiases_conversion_overrides.go)) + — *the pivotal change*: lets both env vars flow into `legacyOverrides`. +- `mapClickHouseReplication`, `harvestClickHouseReplication`, + `readClickHouseReplicationEnv`, `resolveClickHouseEnvFinding`, the + `clickHouseEnvFinding` struct, `readClickHousePendingAnnotation` (now dead), the + `clickHousePendingReplicatedKey`/`clickHousePendingClusterKey` constants, and the + `mapClickHouseReplication` call in `applyValueMappings`. +- `envClickHouseReplicated`/`envClickHouseReplicatedCluster` and + `isClickHouseReplicationEnv` move to (or are re-declared in) the reconciler + package as the registry's env names. +- **Keep** `mapClickHouse` and `clickHouseFields` (structured connection literals). + Under the recommended option (A below), add a small **structured-only** + read of `global.clickhouse.replicated` (+ cluster) into the pending + annotation's `replicated`/`replicatedCluster` keys, replacing the env-aware + harvest with a plain typed mapping. + +### Add to the reconciler (`internal/controller/reconciler`) + +- New `legacy_env_mapping.go`: the registry, `mapLegacyEnvToCR`, target + constructors (`externalClickHouseSelector`), scope resolution (the datastore + conflict/precedence check), the Secret-merge helper, the `onExist` guard, and + env removal + map pruning. +- Wire `mapLegacyEnvToCR` into `Reconcile` after `migrateLegacyAnnotations`. +- `migrateLegacyClickHouse` keeps draining `replicated`/`replicatedCluster` from + the pending annotation (now sourced only from the **structured flag**); the env + mapper overrides on a later pass when the env is present. + +## Invariants to preserve + +These are pinned by existing tests and must survive the move (source: the +behavior audit of `legacy_overrides_test.go`, +`weightsandbiases_conversion_clickhouse_replication_test.go`, +`weightsandbiases_conversion_overrides_test.go`, `migrate_legacy_test.go`, +`clickhouse_replication_test.go`). + +Replication mapping (re-expressed against `legacyOverrides` in the reconciler): +- [ ] Env resolved from any section's `env`/`extraEnv`; **env beats extraEnv**. +- [ ] **Per-app beats global**; apps that **disagree → hard error** ("every + application must agree"); identical values are not a conflict. +- [ ] Non-boolean `WF_CLICKHOUSE_REPLICATED` → hard error ("is not a boolean"). +- [ ] Helm-template (`{{ }}`) values ignored (already dropped by + `legacyEnvVar` before they reach `legacyOverrides`). +- [ ] Managed ClickHouse → replication dropped (no external connection to attach). +- [ ] Values land in `-clickhouse-converted` under keys + `replicated`/`replicatedCluster`; selectors point there; **only-fill-zero / + CR-value-wins** except the `overrideConversionDerived` topology entries. +- [ ] The env vars **do not remain** in `legacyOverrides` after mapping + (today's `require.NotContains`); the raw v1-values annotation still + round-trips them untouched. +- [ ] Read side: apps read topology from the connection Secret; the env var is + **dropped when the selector is unpublished** (unchanged — no code change). + +Generic `legacyOverrides` behavior (unchanged, must not regress): +- [ ] `overrideEnvVars` replace-in-place-then-append; empty names skipped; last + duplicate wins; nil overrides are a no-op. +- [ ] `applyLegacyOverrideEnv` precedence manifest < global < per-app; an app + with no entry still gets the global layer. +- [ ] `validateLegacyOverrides` remains non-mutating (log-only). + +### Test migration + +- `api/v1/weightsandbiases_conversion_clickhouse_replication_test.go` — **moves** + into `internal/controller/reconciler/` and is rewritten to seed + `spec.wandb.legacyOverrides[...].Env` and assert Secret/selector results plus + env removal (instead of asserting the pending-annotation payload). +- `api/v1/weightsandbiases_conversion_overrides_test.go` — **stays** as a + conversion test but loses the "replication vars stripped from overrides" case + (that behavior moves to the reconciler); the two env vars now pass through + verbatim, so add/adjust a passthrough assertion. +- `internal/controller/reconciler/migrate_legacy_test.go` — the ClickHouse + replication-drain cases narrow to the structured-flag path; env-driven + replication assertions move to the new mapper's test. +- `internal/controller/reconciler/legacy_overrides_test.go` and + `clickhouse_replication_test.go` — **unchanged.** +- New `internal/controller/reconciler/legacy_env_mapping_test.go`. + +## Open decision: the structured `global.clickhouse.replicated` fallback + +Today the harvest also honors the **structured** v1 flag +`global.clickhouse.replicated` as a fallback when no env var set replication, with +**env-beats-flag** precedence (`TestConvertTo_ClickHouseReplicationEnvWinsOverFlag`, +`...FromGlobalClickhouseFlag`). That flag is not an env var and is not carried in +`legacyOverrides`, so a purely env-driven reconcile mapper won't see it. + +- **(A) Recommended — flag mapped in conversion; env overrides in reconcile.** + `mapClickHouse` maps the *structured* flag into the field (its normal + structured job, no env harvesting). The reconcile env mapper uses + `overrideConversionDerived` for the topology entries, so a present env var + overrides the flag-derived field (preserves env-beats-flag) and the flag value + survives when no env maps. Preserves **every** current invariant; keeps the + reconciler purely `legacyOverrides`-driven (no v1-values parsing). Cost: a v2 + user who both set the typed field *and* left a stale v1 env would be overridden + — but that combination cannot arise from conversion (the map is + conversion-owned) and is documented. +- **(B) Simpler — drop the structured-flag fallback (env-only).** Uniform + `keepCR` everywhere; removes *all* replication logic from conversion. Cost: a + v1 deployment that set `global.clickhouse.replicated: true` structurally but + never set the env var loses replication — a real behavior change with an + existing test. Low risk in practice (the chart typically propagates the flag to + the env var anyway), but it is a fidelity loss. + +Recommend **(A)** for zero behavior change; choose **(B)** if the team accepts the +narrow fidelity loss for a smaller conversion webhook. + +## Sequencing + +1. API/registry scaffolding in the reconciler (`legacy_env_mapping.go`) + the + Secret-merge helper; unit-test the mapper in isolation with hand-built + `legacyOverrides`. +2. Wire `mapLegacyEnvToCR` into `Reconcile`; add the two replication entries. +3. Move the structured-flag mapping into `mapClickHouse` (option A) and delete the + env harvest + exclusion from the conversion webhook. +4. Migrate/rewrite the affected tests; update + [docs/infra-connection-settings.md](../../infra-connection-settings.md) to note + replication env vars are now promoted at reconcile. +5. `make manifests generate sync-crd-embed`, `make lint`, `make test`. + +## Non-goals + +- Changing the manifest read side (`resolveEnvvars`) — it already re-injects + mapped values and is the mechanism that makes removal safe. +- Modeling new env vars beyond the two replication seeds. The env-candidate audit + (ClickHouse connection vars, `GORILLA_OIDC_*`, `GORILLA_SESSION_LENGTH`, + bucket/`WF_FILE_STORAGE_*`, Kafka) confirms the registry will grow, but each new + entry is its own change gated on a manifest re-injection binding. diff --git a/docs/design/wandb_v2/legacy_env_var_mapping_plan.md b/docs/design/wandb_v2/legacy_env_var_mapping_plan.md new file mode 100644 index 00000000..db8a7858 --- /dev/null +++ b/docs/design/wandb_v2/legacy_env_var_mapping_plan.md @@ -0,0 +1,490 @@ +# Implementation plan: reconcile-time legacy env-var → CR mapping (Option A) + +Companion to [legacy_env_var_mapping.md](legacy_env_var_mapping.md). Implements +**Option A**: the structured `global.clickhouse.replicated` flag is mapped to the +typed field in the conversion webhook (a plain structured mapping); the raw env +vars flow through `legacyOverrides` and are promoted — and allowed to override the +conversion-derived value — by a new reconcile-time registry. + +Branch: `danielpanzella/envvar-to-cr-value`. + +## End-state behavior (what "done" looks like) + +- The conversion webhook no longer knows about `WF_CLICKHOUSE_*` env vars. All env + vars — including the two replication vars — land in + `spec.wandb.legacyOverrides[...].Env` verbatim. +- `mapClickHouse` maps the **structured** `global.clickhouse.replicated` flag into + the clickhouse-pending annotation's `replicated` key (only when a ClickHouse + connection is present). +- A new reconciler step `mapLegacyEnvToCR` promotes registered env vars into typed + fields (materializing/merging the `-clickhouse-converted` Secret), removes + the mapped env vars from `legacyOverrides`, and persists + requeues. +- Precedence at the pod: explicit user field > v1 env var > v1 structured flag > + manifest default. Unmapped env vars are untouched. + +--- + +## Two source shapes: literal `Value` vs `ValueFrom` + +`legacyOverrides` carries each env var as a full `corev1.EnvVar`, which may hold +either a literal `Value` **or** a `ValueFrom` (e.g. `secretKeyRef`) — `legacyEnvVar` +preserves the whole body during conversion. The mapper must handle both. (The +original harvest only read scalars via `scalarToString` and silently dropped any +`valueFrom`-sourced replication env — a latent bug this design fixes.) + +Both shapes reach the pod through the **same** consolidation: external ClickHouse +`WriteState` runs every spec selector — `spec.Replicated`, `spec.ClusterName`, … — +through `external.ResolveFields` → `ResolveSecretKey`, which dereferences whatever +secret each selector points at and copies the values into the unified +`wandb-clickhouse-connection` secret ([clickhouse.go:37-60](../../../internal/controller/infra/external/clickhouse/clickhouse.go)). +So a selector may point at an operator-owned Secret **or a user's Secret** — the +value is resolved either way. That gives three source cases: + +| Source env shape | How it maps to `conn.Replicated` (a `SecretKeySelector`) | +|---|---| +| literal `Value` (e.g. `"true"`) | materialize the value into `-clickhouse-converted`, point the selector there | +| `ValueFrom.SecretKeyRef` | point the selector **directly** at the user's `{Name, Key}` — no copy | +| `ValueFrom.ConfigMapKeyRef` / `FieldRef` / … | not representable in a Secret-only selector → **passthrough**: leave the env in `legacyOverrides` (still injected as raw pod env), don't touch the field | + +## The override policy (the other subtle rule) + +For the two representable shapes, `onExist` distinguishes conversion-derived from +user-set by **where the current selector points**: + +| Current `conn.Replicated` selector | Action (representable source) | +|---|---| +| unset (`.Name == ""`) | write (materialize or repoint); set selector | +| points at `-clickhouse-converted` (conversion-derived: migrate_legacy's flag drain, or a prior env-map pass) | override — env beats the structured flag | +| points at a **user-supplied** Secret (and not the converted one) | leave field untouched (CR/user wins) | + +`removed from legacyOverrides` is gated on representability, **not** on whether a +write happened: + +- literal / `SecretKeyRef` source (mapped, or skipped because user-owned) → **removed** + (the field is authoritative; the manifest re-injects from it). +- managed / unconfigured ClickHouse → **removed** and not written (managed derives + its own topology — matches the original drop). +- unrepresentable `valueFrom` → **kept** (passthrough), so no data is lost. + +Cluster (`WF_CLICKHOUSE_REPLICATED_CLUSTER`) has no structured source, so it is +`keepCR` (only-fill-if-unset), then removed under the same rules. + +--- + +## Steps + +Each step is independently compilable/testable. Steps 1–5 add the new path +(dormant except for the two seed entries); step 6 removes the old path; steps 7–9 +clean up, migrate tests, and regenerate. + +### Step 1 — Registry scaffolding (new file, no wiring) + +**File:** `internal/controller/reconciler/legacy_env_mapping.go` (new) + +Add the types and the seed registry. No behavior yet beyond pure helpers. + +```go +package reconciler + +const ( + envClickHouseReplicated = "WF_CLICKHOUSE_REPLICATED" + envClickHouseReplicatedCluster = "WF_CLICKHOUSE_REPLICATED_CLUSTER" + + // Data keys inside the operator-owned -clickhouse-converted Secret. + convertedClickHouseReplicatedKey = "replicated" + convertedClickHouseClusterKey = "replicatedCluster" +) + +type mappingScope int + +const ( + // scopeDatastore: one value for the whole datastore. Collected across all + // legacyOverrides sections; per-app beats global; disagreement is an error. + scopeDatastore mappingScope = iota +) + +type conflictPolicy int + +const ( + keepCR conflictPolicy = iota // don't overwrite a field already set + overrideConversionDerived // overwrite when unset or operator-owned (see policy table) +) + +// applyFn owns instance resolution, the managed-vs-external guard, Secret +// materialization / selector repointing, and setting the field. It receives the +// full scope-resolved EnvVar (Value or ValueFrom). It returns remove=true when +// the env var should be stripped from legacyOverrides (mapped, or intentionally +// dropped); remove=false leaves it in place as a raw-env passthrough (a valueFrom +// shape the target can't represent). +type applyFn func(ctx context.Context, c ctrlClient.Client, w *apiv2.WeightsAndBiases, env corev1.EnvVar) (remove bool, err error) + +type legacyEnvMapping struct { + env string + scope mappingScope + onExist conflictPolicy + apply applyFn +} + +var legacyEnvMappings = []legacyEnvMapping{ + {env: envClickHouseReplicated, scope: scopeDatastore, onExist: overrideConversionDerived, + apply: externalClickHouseSelector(convertedClickHouseReplicatedKey, overrideConversionDerived)}, + {env: envClickHouseReplicatedCluster, scope: scopeDatastore, onExist: keepCR, + apply: externalClickHouseSelector(convertedClickHouseClusterKey, keepCR)}, +} +``` + +**Verify:** `go build ./...` (unused symbols are fine at this stage if referenced +by the test added in later steps; otherwise add the resolver from Step 3 in the +same commit). + +### Step 2 — Secret upsert (merge) helper + +**File:** `internal/controller/reconciler/migrate_legacy.go` (add near +`materializeConvertedSecret`) + +`materializeConvertedSecret` **replaces** `secret.Data` +([migrate_legacy.go:453](../../../internal/controller/reconciler/migrate_legacy.go)), +which would clobber the host/port keys migration wrote. Add a merge variant that +preserves existing keys: + +```go +// upsertConvertedSecretKeys merges data into an existing (or new) opaque Secret +// without dropping keys other writers set. Used by the env mapper, which adds +// keys to the same -*-converted Secret migrateLegacy* already populated. +func upsertConvertedSecretKeys(ctx context.Context, c ctrlClient.Client, w *apiv2.WeightsAndBiases, name string, data map[string][]byte) error { + if len(data) == 0 { + return nil + } + secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: w.Namespace}} + if _, err := ctrl.CreateOrUpdate(ctx, c, secret, func() error { + secret.Type = corev1.SecretTypeOpaque + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + for k, v := range data { + secret.Data[k] = v // overwrite our keys, keep the rest + } + return nil + }); err != nil { + return fmt.Errorf("upsert %s: %w", name, err) + } + return nil +} +``` + +(`CreateOrUpdate` populates `secret` with live state before the mutate runs, so +`secret.Data` already holds migration's keys.) + +**Verify:** a unit test that seeds a Secret with `{host}`, upserts `{replicated}`, +asserts both keys survive. + +### Step 3 — The ClickHouse apply function + scope resolver + +**File:** `internal/controller/reconciler/legacy_env_mapping.go` + +```go +const clickHouseConvertedSecretSuffix = "-clickhouse-converted" + +func externalClickHouseSelector(dataKey string, policy conflictPolicy) applyFn { + return func(ctx context.Context, c ctrlClient.Client, w *apiv2.WeightsAndBiases, env corev1.EnvVar) (bool, error) { + spec, ok := w.Spec.ClickHouse[apiv2.DefaultInstanceName] + if !ok || spec.ExternalClickHouse == nil { // managed / unconfigured → drop (remove, no write) + return true, nil + } + conn := spec.ExternalClickHouse + target := &conn.Replicated + if dataKey == convertedClickHouseClusterKey { + target = &conn.ClusterName + } + secretName := w.Name + clickHouseConvertedSecretSuffix + + // Only literal Value and ValueFrom.SecretKeyRef are representable in a + // Secret-only selector. Other valueFrom shapes → passthrough (keep env). + isLiteral := env.ValueFrom == nil && env.Value != "" + hasSecretRef := env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil + if !isLiteral && !hasSecretRef { + logx.GetSlog(ctx).Warn("legacy env has an unrepresentable valueFrom; leaving it as a raw override", + "env", env.Name) + return false, nil // remove=false: passthrough + } + + // onExist: leave a user-supplied selector alone; overwrite when unset or + // operator-owned. keepCR only fills when unset. Either way the env is removed. + userOwned := target.Name != "" && target.Name != secretName + if (policy == keepCR && target.Name != "") || (policy == overrideConversionDerived && userOwned) { + return true, nil + } + + if hasSecretRef { + *target = *env.ValueFrom.SecretKeyRef // point directly at the user's Secret + return true, nil + } + if err := upsertConvertedSecretKeys(ctx, c, w, secretName, + map[string][]byte{dataKey: []byte(env.Value)}); err != nil { + return false, err + } + *target = secretSelector(secretName, dataKey) + return true, nil + } +} +``` + +`secretSelector` already exists +([migrate_legacy.go:482](../../../internal/controller/reconciler/migrate_legacy.go)). +No map write-back is needed: `w.Spec.ClickHouse[default].ExternalClickHouse` is a +`*ClickHouseConnection`, and the `spec` copy shares that pointer, so `conn := +spec.ExternalClickHouse` and `*target = …` mutate the connection the map entry +already points at. (Only a *newly created* connection would need the +`setExternalInstance` write-back idiom from `migrateLegacyClickHouse`; the guard +above returns early when `ExternalClickHouse == nil`, so we never create one here.) + +Scope resolver (datastore precedence + conflict), re-expressing +`resolveClickHouseEnvFinding` against `legacyOverrides`: + +```go +// resolveDatastoreEnv finds one env var across legacyOverrides sections and +// returns the winning EnvVar (Value or ValueFrom): per-app beats "global"; apps +// that disagree are an error. Agreement compares the whole body, so a literal and +// a secretKeyRef for the same var are a genuine conflict. +func resolveDatastoreEnv(overrides map[string]apiv2.LegacyOverrides, name string) (env corev1.EnvVar, found bool, err error) { + var globalEnv corev1.EnvVar + var globalSet bool + var appEnv corev1.EnvVar + var appSet bool + var appSrc string + for _, key := range sortedKeys(overrides) { + for _, e := range overrides[key].Env { + if e.Name != name || (e.Value == "" && e.ValueFrom == nil) { + continue + } + if key == apiv2.LegacyOverridesGlobalKey { + if !globalSet { + globalEnv, globalSet = e, true + } + continue + } + if appSet && !sameEnvBody(e, appEnv) { + return corev1.EnvVar{}, false, fmt.Errorf( + "legacyOverrides: %s at %s conflicts with %s; ClickHouse replication "+ + "is a property of the datastore, so every application must agree", + name, appSrc, key) + } + appEnv, appSet, appSrc = e, true, key + } + } + switch { + case appSet: + return appEnv, true, nil // per-app beats global + case globalSet: + return globalEnv, true, nil + default: + return corev1.EnvVar{}, false, nil + } +} + +// sameEnvBody compares the payload (not the name) of two EnvVars. +func sameEnvBody(a, b corev1.EnvVar) bool { + return a.Value == b.Value && apiequality.Semantic.DeepEqual(a.ValueFrom, b.ValueFrom) +} +``` + +Both literal `Value` and `ValueFrom` entries are considered — a v1 user who +sourced the var from a Secret keeps it (the original harvest dropped these). +Helm-template `Value`s never reach `legacyOverrides` (`legacyEnvVar` drops them +during conversion), so no template handling is needed here. + +**Verify:** table tests for `resolveDatastoreEnv` (global-only, per-app-wins, +agreeing apps, conflicting apps → error) and for `externalClickHouseSelector` +(unset → writes; operator-owned → overwrites value; user-owned → skipped; managed +→ not applied). + +### Step 4 — Orchestration + env removal + persist/requeue + +**File:** `internal/controller/reconciler/legacy_env_mapping.go` + +```go +// mapLegacyEnvToCR promotes registered legacy env vars into typed spec fields, +// then removes them from legacyOverrides (the field is now authoritative). +// Mirrors migrateLegacyAnnotations: mutate spec → Update → requeue → short-circuit. +func mapLegacyEnvToCR(ctx context.Context, c ctrlClient.Client, w *apiv2.WeightsAndBiases) (ctrl.Result, error) { + if len(w.Spec.Wandb.LegacyOverrides) == 0 { + return ctrl.Result{}, nil + } + changed := false + for _, m := range legacyEnvMappings { + env, found, err := resolveDatastoreEnv(w.Spec.Wandb.LegacyOverrides, m.env) // scopeDatastore only for now + if err != nil { + return ctrl.Result{}, err + } + if !found { + continue + } + remove, err := m.apply(ctx, c, w, env) + if err != nil { + return ctrl.Result{}, err + } + if remove && removeLegacyEnv(w, m.env) { // strip from every section; prune empties + changed = true + } + } + if !changed { + return ctrl.Result{}, nil + } + if err := c.Update(ctx, w); err != nil { + return ctrl.Result{}, fmt.Errorf("update CR after legacy env mapping: %w", err) + } + return ctrl.Result{RequeueAfter: time.Second}, nil +} +``` + +`removeLegacyEnv` filters the named var out of every section's `Env`, drops a +section whose `Env` empties **and** has nil `Resources`, and deletes the map when +it empties (so `legacyOverrides` disappears cleanly, matching the "absent, not +empty map" invariant). Returns whether anything was removed. + +**Note on `changed`:** gate the requeue on actual env **removal**, not on whether +a field was written. A guard-dropped var (managed ClickHouse) or a +user-field-wins skip still returns `remove=true` and must persist. An +unrepresentable `valueFrom` returns `remove=false` (passthrough) and persists +nothing for that entry. The `apply` Secret write is idempotent, so re-running +before the removal persists is safe. + +### Step 5 — Wire into `Reconcile` + +**File:** `internal/controller/reconciler/reconcile_v2.go` (after line 169) + +```go +if res, migErr := migrateLegacyAnnotations(ctx, client, wandb); migErr != nil || res.RequeueAfter > 0 { + return res, migErr +} + +// Promote known legacy env vars into typed fields, then drop them from overrides. +if res, mapErr := mapLegacyEnvToCR(ctx, client, wandb); mapErr != nil || res.RequeueAfter > 0 { + return res, mapErr +} +``` + +Runs on passes where `migrateLegacyAnnotations` is a no-op, so the external +ClickHouse connection already exists. + +### Step 6 — Remove the env harvest from the conversion webhook, add the structured mapping + +**File:** `api/v1/weightsandbiases_conversion_mapping.go` + +- Delete the `mapClickHouseReplication` call in `applyValueMappings` (line ~104). +- Delete `mapClickHouseReplication`, `harvestClickHouseReplication`, + `readClickHouseReplicationEnv`, `resolveClickHouseEnvFinding`, + `clickHouseEnvFinding`, `readClickHousePendingAnnotation`, the + `envClickHouseReplicated`/`envClickHouseReplicatedCluster`/`isClickHouseReplicationEnv` + symbols, and `clickHousePendingClusterKey` (cluster is now env-only). +- **Keep** `nestedBoolLenient` and `clickHousePendingReplicatedKey`. +- In `mapClickHouse`, after the `clickHouseFields` loop and `passwordSecret` block, + before the `if !sawField` gate — map the structured flag **only when a + connection field was already seen** (preserves "replication needs an external + connection; managed drops it"): + +```go +if sawField { + if flag, ok, err := nestedBoolLenient(chMap, "replicated"); err != nil { + return fmt.Errorf("spec.values.global.clickhouse.replicated: %w", err) + } else if ok { + remaining[clickHousePendingReplicatedKey] = strconv.FormatBool(flag) + } +} +``` + +`remaining` is the clickhouse-pending payload, and `migrateLegacyClickHouse` +already decodes a `replicated` key into `conn.Replicated` — so no reconciler +change is needed for the flag path. + +**File:** `api/v1/weightsandbiases_conversion_overrides.go` + +- Delete the `isClickHouseReplicationEnv(k)` exclusion (lines 252-257) — now the + two env vars pass through into `legacyOverrides`. This is the pivotal change. + +### Step 7 — migrate_legacy cluster cleanup (small) + +**File:** `internal/controller/reconciler/migrate_legacy.go` + +Cluster no longer travels through the pending annotation. Remove +`ReplicatedCluster` from `legacyClickHousePayload` and its +`fill(&conn.ClusterName, "replicatedCluster", …)` line. Keep the `Replicated` +field + its `fill` (fed by the structured flag). The env mapper now owns +`conn.ClusterName` and overrides of `conn.Replicated`. + +### Step 8 — Tests + +- **Move + rewrite** `api/v1/weightsandbiases_conversion_clickhouse_replication_test.go` + → `internal/controller/reconciler/legacy_env_mapping_test.go`. Re-express its + cases against `spec.wandb.legacyOverrides` input and Secret/selector output: + env from app/global env & extraEnv, env-beats-extraEnv (already resolved by + conversion, so assert passthrough ordering upstream), per-app-beats-global, + conflicting-apps → error, non-boolean tolerated as a Secret string (the "is not + a boolean" check was a webhook concern — decide whether to keep it; see Open + items), managed → dropped, literal values → converted Secret keys, **env removed + from overrides**, override-of-flag, user-owned-selector-respected. **New cases:** + a `valueFrom.secretKeyRef` source → `conn.Replicated` points at the user's + Secret and the env is removed; an unrepresentable `valueFrom` (e.g. + `configMapKeyRef`) → field untouched and env **left** in `legacyOverrides`. +- **Trim** `api/v1/weightsandbiases_conversion_overrides_test.go`: drop the + "replication vars stripped" expectation; add a passthrough assertion that the + two vars now appear in `legacyOverrides`. +- **Adjust** `internal/controller/reconciler/migrate_legacy_test.go`: the + ClickHouse replication-drain cases now cover only the structured-flag path + (`replicated`, no `replicatedCluster`). +- **Add** a conversion test: `global.clickhouse{host, replicated:true}` → + pending `replicated:"true"`. +- **Unchanged:** `legacy_overrides_test.go`, `clickhouse_replication_test.go`. + +### Step 9 — Regenerate, doc, lint, test + +- No `*_types.go` change, so codegen is a no-op — but run + `make manifests generate sync-crd-embed` to be safe (CI gates on it). +- Update [docs/infra-connection-settings.md](../../infra-connection-settings.md): + note the two replication env vars are now promoted to typed fields at reconcile. +- `make lint && make test`. + +--- + +## Invariant → test coverage map + +| Invariant (from the audit) | Where enforced after the change | +|---|---| +| env beats extraEnv | conversion `legacyEnvFromSection` (unchanged) + passthrough test | +| per-app beats global; apps disagree → error | `resolveDatastoreEnv` (Step 3) + table test | +| helm-template values dropped | conversion `legacyEnvVar` (unchanged) | +| managed ClickHouse → replication dropped | `externalClickHouseSelector` guard (Step 3) | +| literal values → `-clickhouse-converted` keys `replicated`/`replicatedCluster` | `externalClickHouseSelector` + `upsertConvertedSecretKeys` | +| `valueFrom.secretKeyRef` source preserved (field points at user Secret) | `externalClickHouseSelector` (Step 3) + new test | +| unrepresentable `valueFrom` kept as raw override (no data loss) | `externalClickHouseSelector` passthrough (Step 3) + new test | +| env vars removed from `legacyOverrides` | `removeLegacyEnv` (Step 4) | +| structured flag fallback (env beats flag) | flag → pending in `mapClickHouse`; env override in Step 3 | +| read side skips unpublished topology | `resolveEnvvars` `clickhouse` source (unchanged) | +| generic override/precedence/validate behavior | `legacy_overrides_test.go` (unchanged) | + +## Open items to confirm during implementation + +1. **Non-boolean `WF_CLICKHOUSE_REPLICATED`.** Today conversion errors ("is not a + boolean"). At reconcile the value is just a Secret string; the app parses it — + and for a `valueFrom.secretKeyRef` source the value isn't even in hand + (it lives in the user's Secret), so a strict check couldn't run uniformly + anyway. Recommend **dropping the hard error** (store the string, let the app + decide). Flag if you want to keep it for the literal case only. +2. **Two-pass latency.** Flag drain (migrate_legacy) and env override land on + separate reconcile passes. Both are ~1s requeues; acceptable and consistent + with existing migration behavior. No change needed. +3. **`envClickHouseReplicated` constant location.** Moves from `api/v1` to + `reconciler`. If any other package imported the v1 constants (grep says no), + update accordingly. + +## Suggested PR breakdown + +- **PR 1** (Steps 1–5): add `mapLegacyEnvToCR` + registry + helpers + wiring, with + new reconciler tests. The old conversion path still runs, so the two vars are + still excluded from `legacyOverrides` — the new step is a no-op in prod but fully + unit-tested. *(Optional: land the machinery before flipping the source.)* +- **PR 2** (Steps 6–9): remove the conversion harvest + exclusion, add the + structured-flag mapping, migrate tests, docs, codegen. This is the behavior flip. + +Landing as one PR is also fine given the shared test migration; split only if you +want the machinery reviewed before the flip. diff --git a/internal/controller/infra/external/clickhouse/clickhouse.go b/internal/controller/infra/external/clickhouse/clickhouse.go index 79e5b2bd..cea9be9a 100644 --- a/internal/controller/infra/external/clickhouse/clickhouse.go +++ b/internal/controller/infra/external/clickhouse/clickhouse.go @@ -32,14 +32,18 @@ func WriteState( ) []metav1.Condition { logger := ctrl.LoggerFrom(ctx) + // Unset selectors resolve to "" and are dropped by ResolveFields, so an + // external ClickHouse that declares no topology simply has no such keys. fields := map[string]corev1.SecretKeySelector{ - "url": spec.URL, - "Host": spec.Host, - "HTTPPort": spec.HTTPPort, - "TCPPort": spec.TCPPort, - "User": spec.Username, - "Password": spec.Password, - "Database": spec.Database, + "url": spec.URL, + "Host": spec.Host, + "HTTPPort": spec.HTTPPort, + "TCPPort": spec.TCPPort, + "User": spec.Username, + "Password": spec.Password, + "Database": spec.Database, + "Replicated": spec.Replicated, + "ClusterName": spec.ClusterName, } data, err := external.ResolveFields(ctx, c, wandb.Namespace, fields) @@ -64,13 +68,13 @@ func ReadState( newConditions []metav1.Condition, ) ([]metav1.Condition, *apiv2.ClickHouseConnection) { nsName := types.NamespacedName{Namespace: wandb.Namespace, Name: connectionSecretName(key)} - _, conditions, found := external.ReadConnectionSecret(ctx, c, nsName, newConditions) + secret, conditions, found := external.ReadConnectionSecret(ctx, c, nsName, newConditions) if !found { return conditions, nil } localRef := corev1.LocalObjectReference{Name: nsName.Name} - return conditions, &apiv2.ClickHouseConnection{ + conn := &apiv2.ClickHouseConnection{ URL: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "url", Optional: ptr.To(false)}, Host: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "Host", Optional: ptr.To(false)}, HTTPPort: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "HTTPPort", Optional: ptr.To(false)}, @@ -79,6 +83,18 @@ func ReadState( Password: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "Password", Optional: ptr.To(false)}, Database: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "Database", Optional: ptr.To(false)}, } + + // Topology is optional for an external ClickHouse: the selector is left unset + // when the user declared nothing, so consumers fall back to their own default + // instead of mounting a key that isn't there. + if _, ok := secret.Data["Replicated"]; ok { + conn.Replicated = corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "Replicated", Optional: ptr.To(false)} + } + if _, ok := secret.Data["ClusterName"]; ok { + conn.ClusterName = corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "ClusterName", Optional: ptr.To(false)} + } + + return conditions, conn } func DeleteConnectionSecret(ctx context.Context, c client.Client, wandb *apiv2.WeightsAndBiases, key string) error { diff --git a/internal/controller/infra/external/clickhouse/clickhouse_test.go b/internal/controller/infra/external/clickhouse/clickhouse_test.go new file mode 100644 index 00000000..794f724b --- /dev/null +++ b/internal/controller/infra/external/clickhouse/clickhouse_test.go @@ -0,0 +1,179 @@ +package clickhouse + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + apiv2 "github.com/wandb/operator/api/v2" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +const sourceSecretName = "external-clickhouse" + +func sourceSel(key string) corev1.SecretKeySelector { + return corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: sourceSecretName}, + Key: key, + } +} + +func externalTestWandb(conn *apiv2.ClickHouseConnection) *apiv2.WeightsAndBiases { + return &apiv2.WeightsAndBiases{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps.wandb.com/v2", Kind: "WeightsAndBiases"}, + ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "default"}, + Spec: apiv2.WeightsAndBiasesSpec{ + ClickHouse: map[string]apiv2.ClickHouseSpec{ + apiv2.DefaultInstanceName: {ExternalClickHouse: conn}, + }, + }, + } +} + +// externalTestClient seeds the user's source Secret with the given keys, plus any +// extra objects. +func externalTestClient(t *testing.T, wandb *apiv2.WeightsAndBiases, sourceData map[string][]byte, extra ...client.Object) client.Client { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, apiv2.AddToScheme(scheme)) + + source := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: sourceSecretName, Namespace: "default"}, + Data: sourceData, + } + objects := append([]client.Object{source, wandb}, extra...) + return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() +} + +// withConnectionSecret seeds an already-written connection Secret in Data form, +// the way a GET returns it: the apiserver folds StringData into Data on write, +// and never returns StringData. +func withConnectionSecret(data map[string][]byte) client.Object { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: ConnectionSecretName, Namespace: "default"}, + Data: data, + } +} + +func connectionSecret(t *testing.T, c client.Client) *corev1.Secret { + t.Helper() + secret := &corev1.Secret{} + require.NoError(t, c.Get(context.Background(), + types.NamespacedName{Namespace: "default", Name: ConnectionSecretName}, secret)) + return secret +} + +func fullyDeclaredConnection() *apiv2.ClickHouseConnection { + return &apiv2.ClickHouseConnection{ + Host: sourceSel("Host"), + HTTPPort: sourceSel("HTTPPort"), + TCPPort: sourceSel("TCPPort"), + Username: sourceSel("User"), + Password: sourceSel("Password"), + Database: sourceSel("Database"), + Replicated: sourceSel("Replicated"), + ClusterName: sourceSel("ClusterName"), + } +} + +func baseSourceData() map[string][]byte { + return map[string][]byte{ + "Host": []byte("clickhouse.example.com"), + "HTTPPort": []byte("8123"), + "TCPPort": []byte("9000"), + "User": []byte("wandb"), + "Password": []byte("secret"), + "Database": []byte("wandb"), + } +} + +// Replication is connection info, so a declared topology has to land in the +// connection Secret next to host and database — that Secret is the only thing +// applications read. +func TestWriteStateCopiesDeclaredTopologyIntoConnectionSecret(t *testing.T) { + wandb := externalTestWandb(fullyDeclaredConnection()) + data := baseSourceData() + data["Replicated"] = []byte("true") + data["ClusterName"] = []byte("weavecluster") + c := externalTestClient(t, wandb, data) + + WriteState(context.Background(), c, wandb, apiv2.DefaultInstanceName, fullyDeclaredConnection()) + + secret := connectionSecret(t, c) + require.Equal(t, "true", secret.StringData["Replicated"]) + require.Equal(t, "weavecluster", secret.StringData["ClusterName"]) +} + +// An external ClickHouse that declares no topology must keep working: the +// selectors are unset, so the keys are simply absent. +func TestWriteStateOmitsUndeclaredTopology(t *testing.T) { + declared := fullyDeclaredConnection() + declared.Replicated = corev1.SecretKeySelector{} + declared.ClusterName = corev1.SecretKeySelector{} + + wandb := externalTestWandb(declared) + c := externalTestClient(t, wandb, baseSourceData()) + + WriteState(context.Background(), c, wandb, apiv2.DefaultInstanceName, declared) + + secret := connectionSecret(t, c) + require.NotContains(t, secret.StringData, "Replicated") + require.NotContains(t, secret.StringData, "ClusterName") + require.Equal(t, "clickhouse.example.com", secret.StringData["Host"], + "the rest of the connection must still be written") +} + +// A declared "false" is not the same as an absent value: applications need to be +// able to be told explicitly not to replicate. +func TestWriteStateCopiesExplicitlyFalseReplicated(t *testing.T) { + wandb := externalTestWandb(fullyDeclaredConnection()) + data := baseSourceData() + data["Replicated"] = []byte("false") + data["ClusterName"] = []byte("") + c := externalTestClient(t, wandb, data) + + WriteState(context.Background(), c, wandb, apiv2.DefaultInstanceName, fullyDeclaredConnection()) + + secret := connectionSecret(t, c) + require.Equal(t, "false", secret.StringData["Replicated"]) +} + +func TestReadStatePublishesTopologySelectorsWhenPresent(t *testing.T) { + wandb := externalTestWandb(fullyDeclaredConnection()) + data := baseSourceData() + data["Replicated"] = []byte("true") + data["ClusterName"] = []byte("weavecluster") + c := externalTestClient(t, wandb, data, withConnectionSecret(data)) + + _, conn := ReadState(context.Background(), c, wandb, apiv2.DefaultInstanceName, nil) + + require.NotNil(t, conn) + require.Equal(t, ConnectionSecretName, conn.Replicated.Name) + require.Equal(t, "Replicated", conn.Replicated.Key) + require.Equal(t, ConnectionSecretName, conn.ClusterName.Name) + require.Equal(t, "ClusterName", conn.ClusterName.Key) +} + +// Pointing a container at a key that isn't in the Secret keeps it from starting, +// so an absent topology must leave the selector unset. +func TestReadStateLeavesTopologySelectorsUnsetWhenAbsent(t *testing.T) { + declared := fullyDeclaredConnection() + declared.Replicated = corev1.SecretKeySelector{} + declared.ClusterName = corev1.SecretKeySelector{} + + wandb := externalTestWandb(declared) + c := externalTestClient(t, wandb, baseSourceData(), withConnectionSecret(baseSourceData())) + + _, conn := ReadState(context.Background(), c, wandb, apiv2.DefaultInstanceName, nil) + + require.NotNil(t, conn) + require.Empty(t, conn.Replicated.Name) + require.Empty(t, conn.ClusterName.Name) + require.Equal(t, "Host", conn.Host.Key, "the rest of the connection must still resolve") +} diff --git a/internal/controller/infra/managed/clickhouse/altinity/conn.go b/internal/controller/infra/managed/clickhouse/altinity/conn.go index 1a64641f..40f769ea 100644 --- a/internal/controller/infra/managed/clickhouse/altinity/conn.go +++ b/internal/controller/infra/managed/clickhouse/altinity/conn.go @@ -17,13 +17,15 @@ import ( ) type clickhouseConnInfo struct { - Host string - TCPPort string - HTTPPort string - User string - Password string - Database string - Tls bool + Host string + TCPPort string + HTTPPort string + User string + Password string + Database string + Tls bool + Replicated bool + ClusterName string } func (c *clickhouseConnInfo) toURL() string { @@ -90,13 +92,15 @@ func writeClickHouseConnInfo( }, Type: corev1.SecretTypeOpaque, StringData: map[string]string{ - urlKey: connInfo.toURL(), - "Host": connInfo.Host, - "TCPPort": connInfo.TCPPort, - "HTTPPort": connInfo.HTTPPort, - "User": connInfo.User, - "Password": connInfo.Password, - "Database": connInfo.Database, + urlKey: connInfo.toURL(), + "Host": connInfo.Host, + "TCPPort": connInfo.TCPPort, + "HTTPPort": connInfo.HTTPPort, + "User": connInfo.User, + "Password": connInfo.Password, + "Database": connInfo.Database, + "Replicated": strconv.FormatBool(connInfo.Replicated), + "ClusterName": connInfo.ClusterName, }, } @@ -113,5 +117,8 @@ func writeClickHouseConnInfo( Username: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "User", Optional: ptr.To(false)}, Password: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "Password", Optional: ptr.To(false)}, Database: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "Database", Optional: ptr.To(false)}, + // The operator provisions the cluster, so these keys always exist. + Replicated: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "Replicated", Optional: ptr.To(false)}, + ClusterName: corev1.SecretKeySelector{LocalObjectReference: localRef, Key: "ClusterName", Optional: ptr.To(false)}, }, nil } diff --git a/internal/controller/infra/managed/clickhouse/altinity/conn_test.go b/internal/controller/infra/managed/clickhouse/altinity/conn_test.go new file mode 100644 index 00000000..0c19cec5 --- /dev/null +++ b/internal/controller/infra/managed/clickhouse/altinity/conn_test.go @@ -0,0 +1,116 @@ +package altinity + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + apiv2 "github.com/wandb/operator/api/v2" + chiv1 "github.com/wandb/operator/pkg/vendored/altinity-clickhouse/clickhouse.altinity.com/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +var _ = Describe("ClickHouse connection topology", func() { + chiWithReplicas := func(replicas int) *chiv1.ClickHouseInstallation { + return &chiv1.ClickHouseInstallation{ + ObjectMeta: metav1.ObjectMeta{Name: "wandb-chi", Namespace: "wandb"}, + Spec: chiv1.ChiSpec{ + Configuration: &chiv1.Configuration{ + Clusters: []*chiv1.Cluster{{ + Name: chiClusterName, + Layout: &chiv1.ChiClusterLayout{ShardsCount: ShardsCount, ReplicasCount: replicas}, + }}, + }, + }, + Status: &chiv1.Status{Endpoint: "clickhouse.wandb.svc"}, + } + } + + // Weave only creates ReplicatedMergeTree tables when told the cluster is + // replicated, and the topology comes off the live CHI: a spec change hasn't + // reached ClickHouse until the CHI carries it. + DescribeTable("derives replication from the live CHI layout", + func(replicas int, expected bool) { + connInfo := readConnectionDetails(chiWithReplicas(replicas)) + Expect(connInfo).ToNot(BeNil()) + Expect(connInfo.Replicated).To(Equal(expected)) + }, + Entry("a single replica is not replicated", 1, false), + Entry("two replicas are replicated", 2, true), + Entry("three replicas are replicated", 3, true), + ) + + It("publishes the CHI cluster name, not the Service-name derivation", func() { + connInfo := readConnectionDetails(chiWithReplicas(2)) + Expect(connInfo.ClusterName).To(Equal(CHIClusterName())) + Expect(connInfo.ClusterName).ToNot(Equal(ClusterName("wandb-chi"))) + }) + + It("reports no topology for a CHI with no cluster layout", func() { + chi := chiWithReplicas(2) + chi.Spec.Configuration.Clusters[0].Layout = nil + + connInfo := readConnectionDetails(chi) + Expect(connInfo.Replicated).To(BeFalse()) + Expect(connInfo.ClusterName).To(BeEmpty()) + }) + + Describe("writeClickHouseConnInfo", func() { + var ( + ctx context.Context + nsnBuilder *NsNameBuilder + owner *apiv2.WeightsAndBiases + ) + + BeforeEach(func() { + ctx = context.Background() + nsnBuilder = createNsNameBuilder(types.NamespacedName{Name: "wandb-chi", Namespace: "wandb"}) + owner = &apiv2.WeightsAndBiases{ + TypeMeta: metav1.TypeMeta{APIVersion: apiv2.GroupVersion.String(), Kind: "WeightsAndBiases"}, + ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "wandb", UID: "wandb-uid"}, + } + }) + + newClient := func() *fake.ClientBuilder { + scheme := runtime.NewScheme() + Expect(corev1.AddToScheme(scheme)).To(Succeed()) + Expect(apiv2.AddToScheme(scheme)).To(Succeed()) + return fake.NewClientBuilder().WithScheme(scheme) + } + + // The connection Secret is the only thing applications read, so topology + // has to travel in it alongside host and database. + It("writes topology into the connection Secret and returns selectors", func() { + c := newClient().Build() + + conn, err := writeClickHouseConnInfo(ctx, c, owner, nsnBuilder, readConnectionDetails(chiWithReplicas(3))) + Expect(err).ToNot(HaveOccurred()) + + secret := &corev1.Secret{} + Expect(c.Get(ctx, nsnBuilder.ConnectionNsName(), secret)).To(Succeed()) + Expect(secret.StringData).To(HaveKeyWithValue("Replicated", "true")) + Expect(secret.StringData).To(HaveKeyWithValue("ClusterName", chiClusterName)) + + Expect(conn.Replicated.Name).To(Equal(nsnBuilder.ConnectionNsName().Name)) + Expect(conn.Replicated.Key).To(Equal("Replicated")) + Expect(conn.ClusterName.Key).To(Equal("ClusterName")) + }) + + // An unreplicated cluster must say so explicitly: applications need to + // distinguish "not replicated" from "nothing published". + It("writes an explicit false for a single-replica cluster", func() { + c := newClient().Build() + + _, err := writeClickHouseConnInfo(ctx, c, owner, nsnBuilder, readConnectionDetails(chiWithReplicas(1))) + Expect(err).ToNot(HaveOccurred()) + + secret := &corev1.Secret{} + Expect(c.Get(ctx, nsnBuilder.ConnectionNsName(), secret)).To(Succeed()) + Expect(secret.StringData).To(HaveKeyWithValue("Replicated", "false")) + }) + }) +}) diff --git a/internal/controller/infra/managed/clickhouse/altinity/naming.go b/internal/controller/infra/managed/clickhouse/altinity/naming.go index 9ffe32e5..3d8238cf 100644 --- a/internal/controller/infra/managed/clickhouse/altinity/naming.go +++ b/internal/controller/infra/managed/clickhouse/altinity/naming.go @@ -75,6 +75,7 @@ func (n *NsNameBuilder) ConnectionNsName() types.NamespacedName { func createNsNameBuilder(baseNsName types.NamespacedName) *NsNameBuilder { return CreateNsNameBuilder(baseNsName) } +func CHIClusterName() string { return chiClusterName } const ( // chiClusterName is the single cluster the CHI defines. diff --git a/internal/controller/infra/managed/clickhouse/altinity/naming_test.go b/internal/controller/infra/managed/clickhouse/altinity/naming_test.go index 2f8a02f2..5c5ad660 100644 --- a/internal/controller/infra/managed/clickhouse/altinity/naming_test.go +++ b/internal/controller/infra/managed/clickhouse/altinity/naming_test.go @@ -1,15 +1,36 @@ package altinity import ( + "context" "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" apiv2 "github.com/wandb/operator/api/v2" + "github.com/wandb/operator/pkg/wandb/manifest" "k8s.io/apimachinery/pkg/util/validation" ) var _ = Describe("managed ClickHouse naming", func() { + Describe("CHIClusterName", func() { + It("matches the cluster the CHI declares", func() { + wandb := clickHouseWandb() + spec := wandb.Spec.ClickHouse[apiv2.DefaultInstanceName].ManagedClickHouse + + chi, err := ToClickHouseVendorSpec( + context.Background(), wandb, spec, clickHouseScheme(), + testObjectStorageConn(), testObjectStorageEndpoint, true, manifest.Manifest{}, + ) + Expect(err).NotTo(HaveOccurred()) + Expect(chi.Spec.Configuration.Clusters).To(HaveLen(1)) + Expect(chi.Spec.Configuration.Clusters[0].Name).To(Equal(CHIClusterName())) + }) + + It("is not the Service-name derivation", func() { + Expect(CHIClusterName()).NotTo(Equal(ClusterName("wandb-clickhouse-chi"))) + }) + }) + Describe("KeeperNsName", func() { It("pairs the Keeper with the installation via the shared base name", func() { spec := &apiv2.ManagedClickHouseSpec{Name: "wandb-legacy-overrides-v1-chi", Namespace: "wandb"} diff --git a/internal/controller/infra/managed/clickhouse/altinity/read.go b/internal/controller/infra/managed/clickhouse/altinity/read.go index c5c3ac42..092f04d2 100644 --- a/internal/controller/infra/managed/clickhouse/altinity/read.go +++ b/internal/controller/infra/managed/clickhouse/altinity/read.go @@ -24,14 +24,36 @@ func readConnectionDetails(actual *chiv1.ClickHouseInstallation) *clickhouseConn clickhouseHTTPPort := strconv.Itoa(ClickHouseHTTPPort) clickhouseTCPPort := strconv.Itoa(ClickHouseNativePort) + replicated, clusterName := readClusterTopology(actual) + return &clickhouseConnInfo{ - Host: clickhouseHost, - HTTPPort: clickhouseHTTPPort, - TCPPort: clickhouseTCPPort, - User: ClickHouseUser, - Password: ClickHousePassword, - Database: ClickHouseDatabase, + Host: clickhouseHost, + HTTPPort: clickhouseHTTPPort, + TCPPort: clickhouseTCPPort, + User: ClickHouseUser, + Password: ClickHousePassword, + Database: ClickHouseDatabase, + Replicated: replicated, + ClusterName: clusterName, + } +} + +// readClusterTopology reports whether applications should use +// ReplicatedMergeTree, and the cluster their DDL runs ON CLUSTER. +func readClusterTopology(actual *chiv1.ClickHouseInstallation) (bool, string) { + if actual.Spec.Configuration == nil { + return false, "" + } + for _, cluster := range actual.Spec.Configuration.Clusters { + if cluster == nil || cluster.Name != chiClusterName { + continue + } + if cluster.Layout == nil { + return false, "" + } + return cluster.Layout.ReplicasCount > 1, cluster.Name } + return false, "" } func ReadState( diff --git a/internal/controller/reconciler/clickhouse_replication_test.go b/internal/controller/reconciler/clickhouse_replication_test.go new file mode 100644 index 00000000..0f1ca9fb --- /dev/null +++ b/internal/controller/reconciler/clickhouse_replication_test.go @@ -0,0 +1,142 @@ +package reconciler + +import ( + "context" + "testing" + + apiv2 "github.com/wandb/operator/api/v2" + serverManifest "github.com/wandb/operator/pkg/wandb/manifest" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +const clickHouseConnSecret = "wandb-clickhouse-connection" + +func clickHouseSel(key string) corev1.SecretKeySelector { + return corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: clickHouseConnSecret}, + Key: key, + } +} + +// wandbWithClickHouseConnection publishes a connection whose topology selectors +// are whatever the infra layer resolved +func wandbWithClickHouseConnection(conn apiv2.ClickHouseConnection) *apiv2.WeightsAndBiases { + return &apiv2.WeightsAndBiases{ + ObjectMeta: metav1.ObjectMeta{Name: "wandb", Namespace: "default"}, + Status: apiv2.WeightsAndBiasesStatus{ + ClickHouseStatus: map[string]apiv2.ClickHouseInfraStatus{ + apiv2.DefaultInstanceName: {Connection: conn}, + }, + }, + } +} + +func fullClickHouseConnection() apiv2.ClickHouseConnection { + return apiv2.ClickHouseConnection{ + URL: clickHouseSel("url"), + Host: clickHouseSel("Host"), + HTTPPort: clickHouseSel("HTTPPort"), + TCPPort: clickHouseSel("TCPPort"), + Username: clickHouseSel("User"), + Password: clickHouseSel("Password"), + Database: clickHouseSel("Database"), + Replicated: clickHouseSel("Replicated"), + ClusterName: clickHouseSel("ClusterName"), + } +} + +// resolveClickHouseEnv resolves a single manifest env var backed by a clickhouse +// source field. +func resolveClickHouseEnv(t *testing.T, wandb *apiv2.WeightsAndBiases, name, field string) (corev1.EnvVar, bool) { + t.Helper() + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("failed adding corev1 to scheme: %v", err) + } + client := fake.NewClientBuilder().WithScheme(scheme).Build() + + envs := []serverManifest.EnvVar{ + {Name: name, Sources: []serverManifest.EnvSource{{ + Type: "clickhouse", + Name: apiv2.DefaultInstanceName, + Field: field, + }}}, + } + resolved, err := resolveEnvvars(context.Background(), client, wandb, serverManifest.Manifest{}, nil, envs) + if err != nil { + t.Fatalf("resolveEnvvars returned error: %v", err) + } + for _, env := range resolved { + if env.Name == name { + return env, true + } + } + return corev1.EnvVar{}, false +} + +// Weave reads replication out of the connection Secret, the same way it reads +// host and database. +func TestResolveEnvvarsClickHouseReplicationFromConnectionSecret(t *testing.T) { + wandb := wandbWithClickHouseConnection(fullClickHouseConnection()) + + for _, tc := range []struct { + field string + key string + }{ + {"replicated", "Replicated"}, + {"replicated-cluster", "ClusterName"}, + } { + t.Run(tc.field, func(t *testing.T) { + env, found := resolveClickHouseEnv(t, wandb, "WF_CLICKHOUSE_TEST", tc.field) + if !found { + t.Fatalf("expected an env var for field %q", tc.field) + } + if env.ValueFrom == nil || env.ValueFrom.SecretKeyRef == nil { + t.Fatalf("expected a secret-backed env var, got %+v", env) + } + if got := env.ValueFrom.SecretKeyRef.Name; got != clickHouseConnSecret { + t.Errorf("expected secret %q, got %q", clickHouseConnSecret, got) + } + if got := env.ValueFrom.SecretKeyRef.Key; got != tc.key { + t.Errorf("expected key %q, got %q", tc.key, got) + } + }) + } +} + +// An external ClickHouse that declared no topology publishes no selector. +// Mounting a key that isn't in the Secret keeps the container from starting, so +// the env var has to be dropped and the application left on its own default. +func TestResolveEnvvarsClickHouseReplicationSkippedWhenUnpublished(t *testing.T) { + conn := fullClickHouseConnection() + conn.Replicated = corev1.SecretKeySelector{} + conn.ClusterName = corev1.SecretKeySelector{} + wandb := wandbWithClickHouseConnection(conn) + + for _, field := range []string{"replicated", "replicated-cluster"} { + t.Run(field, func(t *testing.T) { + if _, found := resolveClickHouseEnv(t, wandb, "WF_CLICKHOUSE_TEST", field); found { + t.Error("expected no env var when the connection publishes no topology") + } + }) + } +} + +// The rest of the connection must keep resolving whether or not topology is +// published. +func TestResolveEnvvarsClickHouseHostUnaffectedByTopology(t *testing.T) { + conn := fullClickHouseConnection() + conn.Replicated = corev1.SecretKeySelector{} + conn.ClusterName = corev1.SecretKeySelector{} + + env, found := resolveClickHouseEnv(t, wandbWithClickHouseConnection(conn), "WF_CLICKHOUSE_HOST", "host") + if !found { + t.Fatal("expected WF_CLICKHOUSE_HOST to resolve") + } + if env.ValueFrom.SecretKeyRef.Key != "Host" { + t.Errorf("expected key Host, got %q", env.ValueFrom.SecretKeyRef.Key) + } +} diff --git a/internal/controller/reconciler/legacy_env_mapping.go b/internal/controller/reconciler/legacy_env_mapping.go new file mode 100644 index 00000000..16701865 --- /dev/null +++ b/internal/controller/reconciler/legacy_env_mapping.go @@ -0,0 +1,265 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reconciler + +import ( + "context" + "fmt" + "reflect" + "sort" + "time" + + corev1 "k8s.io/api/core/v1" + ctrl "sigs.k8s.io/controller-runtime" + ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" + + apiv2 "github.com/wandb/operator/api/v2" + "github.com/wandb/operator/internal/logx" +) + +// Legacy env vars promoted into typed v2 fields at reconcile. v1 users set these +// as raw pod env; conversion carries them verbatim into legacyOverrides, and this +// step maps the ones we model into typed fields, then drops them. +const ( + envClickHouseReplicated = "WF_CLICKHOUSE_REPLICATED" + envClickHouseReplicatedCluster = "WF_CLICKHOUSE_REPLICATED_CLUSTER" + + // Data keys inside the operator-owned -clickhouse-converted Secret. + convertedClickHouseReplicatedKey = "replicated" + convertedClickHouseClusterKey = "replicatedCluster" +) + +type mappingScope int + +const ( + // scopeDatastore: one value for the whole datastore. Collected across every + // legacyOverrides section; per-app beats global; disagreement is an error. + scopeDatastore mappingScope = iota +) + +type conflictPolicy int + +const ( + keepCR conflictPolicy = iota // never overwrite a field already set + overrideConversionDerived // overwrite when unset or operator-owned +) + +// applyFn owns the managed-vs-external guard, Secret materialization / selector +// repointing, and setting the field from the resolved EnvVar. remove=true means +// strip the env from legacyOverrides (mapped, or intentionally dropped); +// remove=false leaves it as a raw-env passthrough (a valueFrom shape the +// Secret-only target can't represent). +type applyFn func(ctx context.Context, c ctrlClient.Client, w *apiv2.WeightsAndBiases, env corev1.EnvVar) (remove bool, err error) + +type legacyEnvMapping struct { + env string + scope mappingScope + apply applyFn +} + +// legacyEnvMappings is the hardcoded registry of v1 env vars the reconciler +// promotes into typed v2 fields. Bound to the CRD; grows as we model more knobs. +// Only add an env var here when a manifest source re-injects its value from the +// target field — otherwise removal silently drops it from the pod. +var legacyEnvMappings = []legacyEnvMapping{ + { + env: envClickHouseReplicated, + scope: scopeDatastore, + apply: externalClickHouseSelector(convertedClickHouseReplicatedKey, + func(c *apiv2.ClickHouseConnection) *corev1.SecretKeySelector { return &c.Replicated }, + overrideConversionDerived), + }, + { + env: envClickHouseReplicatedCluster, + scope: scopeDatastore, + apply: externalClickHouseSelector(convertedClickHouseClusterKey, + func(c *apiv2.ClickHouseConnection) *corev1.SecretKeySelector { return &c.ClusterName }, + keepCR), + }, +} + +// mapLegacyEnvToCR promotes registered legacy env vars into typed spec fields, +// then removes them from legacyOverrides (the field is now authoritative). +// Mirrors migrateLegacyAnnotations: mutate spec -> Update -> requeue, and the +// caller short-circuits the rest of reconcile on a non-zero RequeueAfter. +func mapLegacyEnvToCR(ctx context.Context, c ctrlClient.Client, w *apiv2.WeightsAndBiases) (ctrl.Result, error) { + if len(w.Spec.Wandb.LegacyOverrides) == 0 { + return ctrl.Result{}, nil + } + + changed := false + for _, m := range legacyEnvMappings { + env, found, err := resolveLegacyEnv(m, w.Spec.Wandb.LegacyOverrides) + if err != nil { + return ctrl.Result{}, err + } + if !found { + continue + } + remove, err := m.apply(ctx, c, w, env) + if err != nil { + return ctrl.Result{}, err + } + if remove && removeLegacyEnv(w, m.env) { + changed = true + } + } + if !changed { + return ctrl.Result{}, nil + } + if err := c.Update(ctx, w); err != nil { + return ctrl.Result{}, fmt.Errorf("update CR after legacy env mapping: %w", err) + } + return ctrl.Result{RequeueAfter: time.Second}, nil +} + +func resolveLegacyEnv(m legacyEnvMapping, overrides map[string]apiv2.LegacyOverrides) (corev1.EnvVar, bool, error) { + switch m.scope { + case scopeDatastore: + return resolveDatastoreEnv(overrides, m.env) + default: + return corev1.EnvVar{}, false, fmt.Errorf("legacy env %q: unsupported mapping scope %d", m.env, m.scope) + } +} + +// resolveDatastoreEnv finds one env var across legacyOverrides sections and +// returns the winning EnvVar (Value or ValueFrom): per-app beats "global"; apps +// that disagree are an error. Agreement compares the whole body, so a literal and +// a secretKeyRef for the same var are a genuine conflict. +func resolveDatastoreEnv(overrides map[string]apiv2.LegacyOverrides, name string) (corev1.EnvVar, bool, error) { + var globalEnv corev1.EnvVar + var globalSet bool + var appEnv corev1.EnvVar + var appSet bool + var appSrc string + for _, key := range sortedLegacyKeys(overrides) { + for _, e := range overrides[key].Env { + if e.Name != name || (e.Value == "" && e.ValueFrom == nil) { + continue + } + if key == apiv2.LegacyOverridesGlobalKey { + if !globalSet { + globalEnv, globalSet = e, true + } + continue + } + if appSet && !sameEnvBody(e, appEnv) { + return corev1.EnvVar{}, false, fmt.Errorf( + "legacyOverrides: %s at %q conflicts with %q; ClickHouse replication is a "+ + "property of the datastore, so every application must agree", + name, appSrc, key) + } + appEnv, appSet, appSrc = e, true, key + } + } + switch { + case appSet: + return appEnv, true, nil + case globalSet: + return globalEnv, true, nil + default: + return corev1.EnvVar{}, false, nil + } +} + +// sameEnvBody compares the payload (not the name) of two EnvVars. +func sameEnvBody(a, b corev1.EnvVar) bool { + return a.Value == b.Value && reflect.DeepEqual(a.ValueFrom, b.ValueFrom) +} + +// externalClickHouseSelector maps a resolved env var into an external +// ClickHouseConnection SecretKeySelector field. A literal value is written into +// the operator-owned converted Secret; a secretKeyRef source repoints the field +// at the user's Secret; any other valueFrom shape passes through untouched. +func externalClickHouseSelector( + dataKey string, + pick func(*apiv2.ClickHouseConnection) *corev1.SecretKeySelector, + policy conflictPolicy, +) applyFn { + return func(ctx context.Context, c ctrlClient.Client, w *apiv2.WeightsAndBiases, env corev1.EnvVar) (bool, error) { + spec, ok := w.Spec.ClickHouse[apiv2.DefaultInstanceName] + if !ok || spec.ExternalClickHouse == nil { + // Managed or unconfigured: managed derives its own topology, so drop. + return true, nil + } + target := pick(spec.ExternalClickHouse) + + isLiteral := env.ValueFrom == nil && env.Value != "" + hasSecretRef := env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil + if !isLiteral && !hasSecretRef { + logx.GetSlog(ctx).Warn("legacy env has an unrepresentable valueFrom; leaving it as a raw override", + "env", env.Name) + return false, nil // passthrough: leave the env as raw pod env + } + + secretName := clickHouseConvertedSecretName(w) + userOwned := target.Name != "" && target.Name != secretName + if (policy == keepCR && target.Name != "") || (policy == overrideConversionDerived && userOwned) { + return true, nil // field already authoritative; just drop the env + } + + if hasSecretRef { + *target = *env.ValueFrom.SecretKeyRef // point directly at the user's Secret + return true, nil + } + if err := upsertConvertedSecretKeys(ctx, c, w, secretName, map[string][]byte{dataKey: []byte(env.Value)}); err != nil { + return false, err + } + *target = secretSelector(secretName, dataKey) + return true, nil + } +} + +// removeLegacyEnv strips name from every legacyOverrides section, pruning a +// section whose Env empties and carries no Resources, and the map when it +// empties. Returns whether anything was removed. +func removeLegacyEnv(w *apiv2.WeightsAndBiases, name string) bool { + overrides := w.Spec.Wandb.LegacyOverrides + removed := false + for key, ov := range overrides { + var kept []corev1.EnvVar + for _, e := range ov.Env { + if e.Name == name { + removed = true + continue + } + kept = append(kept, e) + } + if len(kept) == len(ov.Env) { + continue + } + ov.Env = kept + if len(ov.Env) == 0 && ov.Resources == nil { + delete(overrides, key) + continue + } + overrides[key] = ov + } + if len(overrides) == 0 { + w.Spec.Wandb.LegacyOverrides = nil + } + return removed +} + +func sortedLegacyKeys(m map[string]apiv2.LegacyOverrides) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/internal/controller/reconciler/legacy_env_mapping_test.go b/internal/controller/reconciler/legacy_env_mapping_test.go new file mode 100644 index 00000000..6cc99c0d --- /dev/null +++ b/internal/controller/reconciler/legacy_env_mapping_test.go @@ -0,0 +1,299 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package reconciler + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" + + apiv2 "github.com/wandb/operator/api/v2" +) + +// newEnvMapFixture builds a WeightsAndBiases with the given legacyOverrides and +// (optionally) an external ClickHouse connection, plus a fake client seeded with +// it and any extra objects. +func newEnvMapFixture( + t *testing.T, + overrides map[string]apiv2.LegacyOverrides, + chConn *apiv2.ClickHouseConnection, + seed ...ctrlClient.Object, +) (ctrlClient.Client, *apiv2.WeightsAndBiases) { + t.Helper() + return newMigrationFixture(t, nil, func(w *apiv2.WeightsAndBiases) { + w.Spec.Wandb.LegacyOverrides = overrides + if chConn != nil { + w.Spec.ClickHouse = map[string]apiv2.ClickHouseSpec{ + apiv2.DefaultInstanceName: {ExternalClickHouse: chConn}, + } + } + }, seed...) +} + +func litEnv(name, value string) corev1.EnvVar { + return corev1.EnvVar{Name: name, Value: value} +} + +func externalCH(w *apiv2.WeightsAndBiases) *apiv2.ClickHouseConnection { + return w.Spec.ClickHouse[apiv2.DefaultInstanceName].ExternalClickHouse +} + +func TestMapLegacyEnvToCR_LiteralReplicatedFromGlobal(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + }, &apiv2.ClickHouseConnection{}) + + res, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + require.NotZero(t, res.RequeueAfter) + + secret, err := getClickHouseConvertedSecret(t, client) + require.NoError(t, err) + require.Equal(t, []byte("true"), secret.Data[convertedClickHouseReplicatedKey]) + + conn := externalCH(wandb) + require.Equal(t, "wandb-clickhouse-converted", conn.Replicated.Name) + require.Equal(t, convertedClickHouseReplicatedKey, conn.Replicated.Key) + + // Env stripped from legacyOverrides (map pruned to nil since it emptied). + require.Nil(t, wandb.Spec.Wandb.LegacyOverrides) +} + +func TestMapLegacyEnvToCR_ClusterFromApp(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + "parquet": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicatedCluster, "weavecluster")}}, + }, &apiv2.ClickHouseConnection{}) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + + secret, err := getClickHouseConvertedSecret(t, client) + require.NoError(t, err) + require.Equal(t, []byte("weavecluster"), secret.Data[convertedClickHouseClusterKey]) + + conn := externalCH(wandb) + require.Equal(t, "wandb-clickhouse-converted", conn.ClusterName.Name) + require.Equal(t, convertedClickHouseClusterKey, conn.ClusterName.Key) + require.Nil(t, wandb.Spec.Wandb.LegacyOverrides) +} + +func TestMapLegacyEnvToCR_PerAppBeatsGlobal(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + "parquet": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "false")}}, + }, &apiv2.ClickHouseConnection{}) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + + secret, err := getClickHouseConvertedSecret(t, client) + require.NoError(t, err) + require.Equal(t, []byte("false"), secret.Data[convertedClickHouseReplicatedKey]) +} + +func TestMapLegacyEnvToCR_ConflictingAppsError(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + "parquet": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + "weave": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "false")}}, + }, &apiv2.ClickHouseConnection{}) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.Error(t, err) + require.Contains(t, err.Error(), "every application must agree") +} + +func TestMapLegacyEnvToCR_AgreeingAppsOK(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + "parquet": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + "weave": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + }, &apiv2.ClickHouseConnection{}) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + + secret, err := getClickHouseConvertedSecret(t, client) + require.NoError(t, err) + require.Equal(t, []byte("true"), secret.Data[convertedClickHouseReplicatedKey]) +} + +// A secretKeyRef source points the connection field directly at the user's +// Secret; no converted Secret is written. +func TestMapLegacyEnvToCR_SecretKeyRefRepointsField(t *testing.T) { + userRef := &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "user-ch"}, + Key: "replicated", + } + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{{ + Name: envClickHouseReplicated, + ValueFrom: &corev1.EnvVarSource{SecretKeyRef: userRef}, + }}}, + }, &apiv2.ClickHouseConnection{}) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + + conn := externalCH(wandb) + require.Equal(t, "user-ch", conn.Replicated.Name) + require.Equal(t, "replicated", conn.Replicated.Key) + + _, err = getClickHouseConvertedSecret(t, client) + require.Error(t, err, "no converted Secret should be written for a secretKeyRef source") + require.Nil(t, wandb.Spec.Wandb.LegacyOverrides) +} + +// An unrepresentable valueFrom (configMapKeyRef) cannot fill a Secret-only +// selector, so the env is left in place as a raw pod-env override. +func TestMapLegacyEnvToCR_UnrepresentableValueFromPassthrough(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{{ + Name: envClickHouseReplicated, + ValueFrom: &corev1.EnvVarSource{ConfigMapKeyRef: &corev1.ConfigMapKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "cm"}, + Key: "replicated", + }}, + }}}, + }, &apiv2.ClickHouseConnection{}) + + res, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + require.Zero(t, res.RequeueAfter, "passthrough persists nothing") + + require.Empty(t, externalCH(wandb).Replicated.Name) + require.Len(t, wandb.Spec.Wandb.LegacyOverrides[apiv2.LegacyOverridesGlobalKey].Env, 1) +} + +// Managed (no external connection) drops the value but still removes the env, so +// managed ClickHouse's own topology is authoritative. +func TestMapLegacyEnvToCR_ManagedDropsAndRemoves(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + }, nil) + + res, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + require.NotZero(t, res.RequeueAfter) + + _, err = getClickHouseConvertedSecret(t, client) + require.Error(t, err) + require.Nil(t, wandb.Spec.Wandb.LegacyOverrides) +} + +// overrideConversionDerived overwrites a value the conversion flag drain wrote +// into the operator-owned converted Secret (env beats the structured flag). +func TestMapLegacyEnvToCR_OverridesConversionDerivedValue(t *testing.T) { + conn := &apiv2.ClickHouseConnection{ + Replicated: secretSelector("wandb-clickhouse-converted", convertedClickHouseReplicatedKey), + } + seed := &corev1.Secret{} + seed.Name = "wandb-clickhouse-converted" + seed.Namespace = "default" + seed.Data = map[string][]byte{convertedClickHouseReplicatedKey: []byte("true")} + + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "false")}}, + }, conn, seed) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + + secret, err := getClickHouseConvertedSecret(t, client) + require.NoError(t, err) + require.Equal(t, []byte("false"), secret.Data[convertedClickHouseReplicatedKey], "env overrides the flag value") +} + +// A user-owned selector (points at the user's own Secret) is left untouched, but +// the env is still removed — the typed field wins. +func TestMapLegacyEnvToCR_UserOwnedSelectorRespected(t *testing.T) { + conn := &apiv2.ClickHouseConnection{ + Replicated: secretSelector("user-ch", "flag"), + } + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + }, conn) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + + require.Equal(t, "user-ch", externalCH(wandb).Replicated.Name, "user field untouched") + require.Equal(t, "flag", externalCH(wandb).Replicated.Key) + require.Nil(t, wandb.Spec.Wandb.LegacyOverrides, "env still removed") +} + +// keepCR (cluster) does not overwrite a field the user already set. +func TestMapLegacyEnvToCR_ClusterKeepCRWhenSet(t *testing.T) { + conn := &apiv2.ClickHouseConnection{ + ClusterName: secretSelector("user-ch", "cluster"), + } + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv(envClickHouseReplicatedCluster, "override")}}, + }, conn) + + _, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + + require.Equal(t, "user-ch", externalCH(wandb).ClusterName.Name) + require.Equal(t, "cluster", externalCH(wandb).ClusterName.Key) + require.Nil(t, wandb.Spec.Wandb.LegacyOverrides) +} + +func TestMapLegacyEnvToCR_NoOverridesNoOp(t *testing.T) { + client, wandb := newEnvMapFixture(t, nil, &apiv2.ClickHouseConnection{}) + + res, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + require.Zero(t, res.RequeueAfter) +} + +// Unrelated env vars are left untouched (only registered names are promoted). +func TestMapLegacyEnvToCR_UnrelatedEnvUntouched(t *testing.T) { + client, wandb := newEnvMapFixture(t, map[string]apiv2.LegacyOverrides{ + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv("SOME_OTHER_VAR", "x")}}, + }, &apiv2.ClickHouseConnection{}) + + res, err := mapLegacyEnvToCR(context.Background(), client, wandb) + require.NoError(t, err) + require.Zero(t, res.RequeueAfter) + require.Len(t, wandb.Spec.Wandb.LegacyOverrides[apiv2.LegacyOverridesGlobalKey].Env, 1) +} + +func TestRemoveLegacyEnv_PrunesEmptyPreservesResourcesAndOthers(t *testing.T) { + quantity := corev1.ResourceRequirements{} + wandb := &apiv2.WeightsAndBiases{} + wandb.Spec.Wandb.LegacyOverrides = map[string]apiv2.LegacyOverrides{ + // Emptied section with no resources -> pruned. + apiv2.LegacyOverridesGlobalKey: {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}}, + // Emptied Env but has Resources -> section kept, Env cleared. + "weave": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true")}, Resources: &quantity}, + // Other env survives. + "parquet": {Env: []corev1.EnvVar{litEnv(envClickHouseReplicated, "true"), litEnv("KEEP", "1")}}, + } + + removed := removeLegacyEnv(wandb, envClickHouseReplicated) + require.True(t, removed) + + overrides := wandb.Spec.Wandb.LegacyOverrides + require.NotContains(t, overrides, apiv2.LegacyOverridesGlobalKey) + require.Contains(t, overrides, "weave") + require.Empty(t, overrides["weave"].Env) + require.NotNil(t, overrides["weave"].Resources) + require.Len(t, overrides["parquet"].Env, 1) + require.Equal(t, "KEEP", overrides["parquet"].Env[0].Name) +} diff --git a/internal/controller/reconciler/migrate_legacy.go b/internal/controller/reconciler/migrate_legacy.go index c3557bd5..735d9b85 100644 --- a/internal/controller/reconciler/migrate_legacy.go +++ b/internal/controller/reconciler/migrate_legacy.go @@ -202,6 +202,10 @@ type legacyClickHousePayload struct { Database string `json:"database,omitempty"` User string `json:"user,omitempty"` Password string `json:"password,omitempty"` + // Replicated carries the structured global.clickhouse.replicated flag. The + // WF_CLICKHOUSE_REPLICATED[_CLUSTER] env vars are mapped at reconcile from + // legacyOverrides (mapLegacyEnvToCR), which can override this. + Replicated string `json:"replicated,omitempty"` } // migrateLegacyClickHouse drains the clickhouse-pending annotation into a @@ -223,7 +227,7 @@ func migrateLegacyClickHouse( return false, fmt.Errorf("decode %s: %w", apiv1.ClickHousePendingAnnotation, err) } - secretName := fmt.Sprintf("%s-clickhouse-converted", wandb.Name) + secretName := clickHouseConvertedSecretName(wandb) conn := wandb.Spec.ClickHouse[apiv2.DefaultInstanceName].ExternalClickHouse if conn == nil { conn = &apiv2.ClickHouseConnection{} @@ -243,6 +247,7 @@ func migrateLegacyClickHouse( fill(&conn.Database, "database", payload.Database) fill(&conn.Username, "username", payload.User) fill(&conn.Password, "password", payload.Password) + fill(&conn.Replicated, "replicated", payload.Replicated) if err := materializeConvertedSecret(ctx, c, wandb, secretName, data); err != nil { return false, err @@ -426,6 +431,45 @@ func migrateLegacyOIDC( return true, nil } +func clickHouseConvertedSecretName(wandb *apiv2.WeightsAndBiases) string { + return fmt.Sprintf("%s-clickhouse-converted", wandb.Name) +} + +// upsertConvertedSecretKeys merges data into an existing (or new) opaque Secret +// without dropping keys other writers set — unlike materializeConvertedSecret, +// which replaces Data. The env mapper adds keys to the same -*-converted +// Secret migrateLegacy* already populated. +func upsertConvertedSecretKeys( + ctx context.Context, + c ctrlClient.Client, + wandb *apiv2.WeightsAndBiases, + secretName string, + data map[string][]byte, +) error { + if len(data) == 0 { + return nil + } + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: wandb.Namespace, + }, + } + if _, err := ctrl.CreateOrUpdate(ctx, c, secret, func() error { + secret.Type = corev1.SecretTypeOpaque + if secret.Data == nil { + secret.Data = map[string][]byte{} + } + for k, v := range data { + secret.Data[k] = v + } + return nil + }); err != nil { + return fmt.Errorf("upsert %s: %w", secretName, err) + } + return nil +} + // materializeConvertedSecret CreateOrUpdates an opaque Secret with data, // no-op when empty. Safe to call on partial-migration retries. func materializeConvertedSecret( diff --git a/internal/controller/reconciler/migrate_legacy_test.go b/internal/controller/reconciler/migrate_legacy_test.go index 6a16d569..9cb8c1bd 100644 --- a/internal/controller/reconciler/migrate_legacy_test.go +++ b/internal/controller/reconciler/migrate_legacy_test.go @@ -888,6 +888,55 @@ func TestMigrateLegacyClickHouse_FullLiteralPayload(t *testing.T) { require.Empty(t, conn.URL.Name) } +// Replication travels with the connection, so the drain has to land it in the +// converted Secret alongside host and database — that Secret is what the +// applications read. +// The structured global.clickhouse.replicated flag drains into the connection +// Secret. Cluster (WF_CLICKHOUSE_REPLICATED_CLUSTER) is env-only now, mapped at +// reconcile from legacyOverrides, so it never rides the pending annotation. +func TestMigrateLegacyClickHouse_DrainsReplicatedFlag(t *testing.T) { + payload := `{"host":"clickhouse.example.com","port":8123,"replicated":"true"}` + client, wandb := newMigrationFixture(t, map[string]string{ + apiv1.ClickHousePendingAnnotation: payload, + }, nil) + + _, err := migrateLegacyAnnotations(context.Background(), client, wandb) + require.NoError(t, err) + + secret, err := getClickHouseConvertedSecret(t, client) + require.NoError(t, err) + require.Equal(t, []byte("true"), secret.Data["replicated"]) + require.NotContains(t, secret.Data, "replicatedCluster") + + conn := wandb.Spec.ClickHouse[apiv2.DefaultInstanceName].ExternalClickHouse + require.NotNil(t, conn) + require.Equal(t, "wandb-clickhouse-converted", conn.Replicated.Name) + require.Equal(t, "replicated", conn.Replicated.Key) + require.Empty(t, conn.ClusterName.Name, "cluster is env-only, mapped at reconcile") +} + +// A payload with no replication leaves the selectors unset, so the connection +// publishes no topology and applications keep their own default. +func TestMigrateLegacyClickHouse_LeavesTopologyUnsetWhenAbsent(t *testing.T) { + payload := `{"host":"clickhouse.example.com","port":8123}` + client, wandb := newMigrationFixture(t, map[string]string{ + apiv1.ClickHousePendingAnnotation: payload, + }, nil) + + _, err := migrateLegacyAnnotations(context.Background(), client, wandb) + require.NoError(t, err) + + secret, err := getClickHouseConvertedSecret(t, client) + require.NoError(t, err) + require.NotContains(t, secret.Data, "replicated") + require.NotContains(t, secret.Data, "replicatedCluster") + + conn := wandb.Spec.ClickHouse[apiv2.DefaultInstanceName].ExternalClickHouse + require.NotNil(t, conn) + require.Empty(t, conn.Replicated.Name) + require.Empty(t, conn.ClusterName.Name) +} + func TestMigrateLegacyClickHouse_PartialPayload(t *testing.T) { payload := `{"host":"clickhouse.example.com","password":"shh"}` client, wandb := newMigrationFixture(t, map[string]string{ diff --git a/internal/controller/reconciler/pods.go b/internal/controller/reconciler/pods.go index ab4e24b3..6251b527 100644 --- a/internal/controller/reconciler/pods.go +++ b/internal/controller/reconciler/pods.go @@ -280,6 +280,18 @@ func resolveEnvvars(ctx context.Context, client ctrlClient.Client, wandb *v2.Wei selector.Key = "Database" case "url": selector.Key = "url" + case "replicated", "replicated-cluster": + // Topology is published for managed ClickHouse, and for an + // external one only when its connection declares it. Skip the + // env var rather than mount a key that may not be there. + topology := status.Connection.Replicated + if src.Field == "replicated-cluster" { + topology = status.Connection.ClusterName + } + if topology.Name == "" || topology.Key == "" { + continue + } + selector.Key = topology.Key default: // Unrecognized field; skip continue diff --git a/internal/controller/reconciler/reconcile_v2.go b/internal/controller/reconciler/reconcile_v2.go index dee5419a..2b588b9d 100644 --- a/internal/controller/reconciler/reconcile_v2.go +++ b/internal/controller/reconciler/reconcile_v2.go @@ -169,6 +169,13 @@ func Reconcile( return res, migErr } + ///////////////////////// + // Promote known legacy env vars from legacyOverrides into typed spec fields, + // then drop them from legacyOverrides (the CR field is the source of truth) + if res, mapErr := mapLegacyEnvToCR(ctx, client, wandb); mapErr != nil || res.RequeueAfter > 0 { + return res, mapErr + } + ///////////////////////// // Fetch manifest early so infra sizing can be applied before provisioning. // Local file:// manifests need no registry credentials, so a missing pull diff --git a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml index 6df9d6b8..cb6fa8e7 100644 --- a/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml +++ b/internal/crdinstaller/crds/operator/apps.wandb.com_weightsandbiases.yaml @@ -525,6 +525,19 @@ spec: properties: externalClickhouse: properties: + clusterName: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic database: properties: key: @@ -577,6 +590,19 @@ spec: - key type: object x-kubernetes-map-type: atomic + replicated: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic tcpPort: properties: key: @@ -4592,6 +4618,19 @@ spec: type: array connection: properties: + clusterName: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic database: properties: key: @@ -4644,6 +4683,19 @@ spec: - key type: object x-kubernetes-map-type: atomic + replicated: + properties: + key: + type: string + name: + default: "" + type: string + optional: + type: boolean + required: + - key + type: object + x-kubernetes-map-type: atomic tcpPort: properties: key: