feat: OTel collector bootstrap as operator startup Runnable - #376
Conversation
4da3a43 to
6528868
Compare
pdettori
left a comment
There was a problem hiding this comment.
Well-structured PR — operator-native startup logic with proper feature gating (off by default), idempotent create/update, leader-election guard, and solid test coverage (16 tests).
One blocking issue: both commits use Co-authored-by: Cursor which violates the repo's attribution policy (CLAUDE.md requires Assisted-By: instead of Co-authored-by: to avoid inflating GitHub contributor stats). Please amend or interactive-rebase to fix the trailers.
Two non-blocking suggestions inline.
| if len(parts) >= 2 { | ||
| info.workspaceNS = parts[1] | ||
| } | ||
| info.tracesURL = fmt.Sprintf("%s://%s/v1/traces", parsed.Scheme, parsed.Host) |
There was a problem hiding this comment.
suggestion: If parsed.Scheme is empty (e.g., a URL like //host:port/path which url.Parse accepts without error), this produces an invalid "://host:port/v1/traces". Consider guarding:
if parsed.Scheme == "" || parsed.Host == "" {
log.Info("MLflow address URL missing scheme or host, falling back", "url", cr.Status.Address.URL)
// fall through to Status.URL fallback
} else {
info.tracesURL = fmt.Sprintf("%s://%s/v1/traces", parsed.Scheme, parsed.Host)
...
}|
|
||
| // --- MLflow service discovery tests --- | ||
|
|
||
| func TestMLflowCRDPresent_DiscoversCRNamespace(t *testing.T) { |
There was a problem hiding this comment.
suggestion: Missing negative test cases for mlflowInfoFromCR — an empty Status.Address.URL and a malformed URL (e.g. "://badurl") would exercise the fallback logic and protect against regressions.
Implement OtelBootstrapRunnable that replaces the Helm-based otel-ingress-ca-job and collectorConfig helper with operator-native startup logic: - Step 1: Project OpenShift ingress CA into kagenti-system (OCP only) - Step 2: Assemble OTel collector ConfigMap from detected components (Phoenix, MLflow) with dynamic CRD/service discovery Key behaviours: - OCP detection via config.openshift.io API group discovery - MLflow CRD-missing graceful degradation with restart guidance - MLflow service backoff retry when CRD present but CR not ready - Idempotent: no Deployment restart when ConfigMap unchanged - RHOAI bearer token auth on OCP, OAuth2 client auth elsewhere New files: internal/bootstrap/otel.go - Runnable implementation internal/bootstrap/presets.go - Ported Helm preset configs internal/bootstrap/otel_test.go - 16 unit tests Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: Bobbins228 <mcampbel@redhat.com>
The bootstrap runnable was using the manager's cached client which hadn't synced at startup, causing AlreadyExists errors for ConfigMaps created by the Helm chart. Switch all bootstrap reads to APIReader (direct API server) and add AlreadyExists fallback handling. Also fix MLflow endpoint discovery: the MLflow CRD is cluster-scoped so cr.Namespace is always empty. Derive the in-cluster traces endpoint and workspace namespace from status.address.url instead of relying on the hardcoded preset endpoint. Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: Bobbins228 <mcampbel@redhat.com>
Guard against empty scheme/host when parsing MLflow address URL, falling back to Status.URL. Add three negative test cases for mlflowInfoFromCR exercising empty, malformed, and scheme-less URLs. Assisted-By: Cursor <noreply@cursor.com> Signed-off-by: Bobbins228 <mcampbel@redhat.com>
6528868 to
44016d4
Compare
pdettori
left a comment
There was a problem hiding this comment.
All review comments addressed:
- Empty scheme/host guard —
mlflowInfoFromCRnow checksparsed.Scheme != "" && parsed.Host != ""before constructing the traces endpoint (commit 44016d4). - Negative test cases — Three tests added: empty URL, malformed URL (
://badurl), and scheme-less URL (//host:port) all exercise the fallback logic. - Attribution trailers — All commits now use
Assisted-by:instead ofCo-authored-by:.
LGTM — well-structured fix commit on top of an already solid PR.
Summary
Adds an
OtelBootstrapRunnable(internal/bootstrap/otel.go) that runs once at operator startup to handle OTel collector infrastructure setup. This replaces the Helm-basedotel-ingress-ca-job.yamlJob and thekagenti.otel.collectorConfigHelm helper with operator-managed equivalents that can dynamically discover available components.Step 1 — Ingress CA trust (replaces
otel-ingress-ca-job.yaml):openshift-config-managed/default-ingress-certand root CA fromopenshift-config/kube-root-ca.crt.kagenti-system/otel-ingress-ca.Step 2 — OTel collector ConfigMap assembly (replaces
kagenti.otel.collectorConfig):status.address.url(the CRD is cluster-scoped socr.Namespaceis always empty).values.yaml).Changes
internal/bootstrap/otel.go—OtelBootstrapRunnableimplementingmanager.Runnableinternal/bootstrap/presets.go— OTel collector YAML presets as Go constants (ported from kagenti-deps Helm chart)internal/bootstrap/otel_test.go— unit tests covering all acceptance criteriainternal/mlflow/types.go— addedAddressfield toMLflowStatusto capturestatus.address.urlcmd/main.go—--enable-otel-bootstrapflag, registers runnable viamgr.Add()charts/kagenti-operator/values.yaml—otelBootstrap.enabletogglecharts/kagenti-operator/templates/manager/manager.yaml— passes flag to container argscharts/kagenti-operator/templates/rbac/role.yaml— addsendpointspermissionAll bootstrap reads use
APIReader(direct API server) rather than the manager's cached client to avoid race conditions during early startup.Test Plan
go test ./internal/bootstrap/...otelBootstrap.enable=truehttps://mlflow.redhat-ods-applications.svc:8443/v1/traces)Made with Cursor