feat: patch kagenti-ui-config ConfigMap with MLflow dashboard URL at startup#411
Conversation
…startup Signed-off-by: ChristianZaccaria <christian.zaccaria.cz@gmail.com>
cwiklik
left a comment
There was a problem hiding this comment.
Small, well-scoped feature moving UI-config patching from setup-kagenti.sh into the operator. Clean code, correctly gated under --enable-mlflow, uses APIReader for direct reads, and NeedLeaderElection() = true to avoid multi-replica races. Strong test coverage (patch, idempotency, both skip paths, trailing-slash normalization). No must-fix issues.
Two suggestions are about robustness, not correctness — the one-shot-at-startup model (#1) is the most substantive: it works once MLflow is ready but won't self-heal if MLflow lags operator startup.
Verified the Makefile docker-buildx change is correct: kagenti-operator/Dockerfile already has FROM --platform=$BUILDPLATFORM, so removing the Dockerfile.cross sed hack is safe.
Nit: commit subject uses feat: without the emoji prefix Kagenti's commit convention favors (:sparkles: feat:) — style only, DCO is green. All 16 CI checks passing.
| log := r.Log.WithName("ui-config-bootstrap") | ||
|
|
||
| dashboardURL := r.discoverDashboardURL(ctx, log) | ||
| if dashboardURL == "" { |
There was a problem hiding this comment.
suggestion: One-shot bootstrap silently gives up if no MLflow CR is Available yet. Start() runs once; on a fresh install the MLflow Route is typically admitted after the operator boots, so discoverDashboardURL returns "", this returns nil, and the ConfigMap is never patched until the operator pod restarts. This couples correct behavior to startup ordering. Consider a watch/requeue on MLflow status (or a periodic retry) so the patch is applied whenever MLflow becomes ready — the operator handling ordering rather than depending on it.
There was a problem hiding this comment.
Thanks for the suggestion, I added a retry loop with backoff (~4 min window), previously the kagenti setup script would wait 2mins. A full controller/watch felt heavyweight for a one-shot ConfigMap patch, the retry covers the typical MLflow startup lag. If MLflow takes significantly longer, a pod restart or manual reconciliation could handle this.
| cm.Data = make(map[string]string) | ||
| } | ||
| cm.Data[mlflowDashboardURLKey] = dashboardURL | ||
| if err := r.Client.Update(ctx, cm); err != nil { |
There was a problem hiding this comment.
suggestion: The PR title says "patch" but this is a read-modify-write Update. If another writer (e.g. the UI deploy) updates the CM between Get and Update, you get a resourceVersion conflict → logged and dropped (return nil), patch lost. A client.Patch (merge/strategic) of the single MLFLOW_DASHBOARD_URL key avoids the conflict window and matches the stated intent.
There was a problem hiding this comment.
Switched to client.MergeFrom patch — now only the MLFLOW_DASHBOARD_URL key is patched without touching the rest of the ConfigMap, avoiding resourceVersion conflicts with concurrent writers.
|
|
||
| func (r *UIConfigBootstrapRunnable) discoverDashboardURL(ctx context.Context, log logr.Logger) string { | ||
| list := &mlflow.MLflowList{} | ||
| if err := r.APIReader.List(ctx, list); err != nil { |
There was a problem hiding this comment.
nit: List is cluster-wide and returns the first Available CR by list order — non-deterministic if multiple MLflow CRs exist across namespaces. If a single instance is expected, scope with client.InNamespace(r.Namespace) or document the single-instance assumption.
There was a problem hiding this comment.
Documented the single-instance assumption — Kagenti deploys one MLflow per platform, so cluster-wide list is intentional and the first Available CR is always the correct one.
Summary
Adds a startup runnable (gated by
--enable-mlflow) that patches thekagenti-ui-configConfigMap with the MLflow dashboard URL derived from the MLflow CR'sstatus.url. This removes the need forsetup-kagenti.shto handle UI config patching after MLflow Route creation.Changes
internal/bootstrap/uiconfig.go—UIConfigBootstrapRunnablethat discovers MLflow CR and patches the ConfigMap once at startupinternal/bootstrap/uiconfig_test.go— unit tests (idempotency, skip when no MLflow/ConfigMap)cmd/main.go— register the runnable under--enable-mlflowMakefile— fixdocker-buildxtarget (Dockerfile already has--platform)Test Plan
go test ./internal/bootstrap/ -run TestUIConfig)