refactor(mlflow): use external gateway URL for tracking#294
refactor(mlflow): use external gateway URL for tracking#294pdettori merged 1 commit intokagenti:mainfrom
Conversation
Use status.url (gateway) instead of status.address.url (internal service) for the MLflow tracking URI. This avoids requiring the OpenShift service-serving CA in agent containers. Remove MLFLOW_TRACKING_SERVER_CERT_PATH, DefaultCACertPath, custom CA loading, and MLflowAddressStatus type. Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
pdettori
left a comment
pdettori
left a comment
There was a problem hiding this comment.
Clean, well-scoped refactoring. Switches MLflow tracking URI from internal service URL (status.address.url) to external gateway URL (status.url), eliminating the need for the OpenShift service-serving CA certificate in agent containers. All removed constants, types, and methods are consistently cleaned up.
Areas reviewed: Go (controller, client, types), Tests
Commits: 1 commit, signed-off ✓
CI status: all 16 checks passing
| uri := cr.Status.Address.URL | ||
| logger.V(1).Info("Auto-discovered MLflow tracking URI", "uri", uri, "cr", cr.GetName()) | ||
| return uri | ||
| logger.Info("MLflow CR is Available but status.url is not set, skipping", "cr", cr.GetName()) |
There was a problem hiding this comment.
👍 Nice logic inversion — checking for presence first (URL != "") is cleaner than the old guard-and-continue pattern.
| type MLflowAddressStatus struct { | ||
| // URL is the in-cluster HTTPS URL for the managed MLflow Service. | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
| // URL is the external gateway URL for the MLflow server (e.g. via the RHOAI data-science gateway). |
There was a problem hiding this comment.
The json:"url" tag changes the serialized field from nested address.url to flat status.url. If any external consumer (e.g., the MLflow operator setting this status) still writes to the old address structure, deserialization would silently drop the value.
Since E2E tests pass, the upstream CRD likely already uses the flat status.url field — just flagging for awareness.
| c.HTTPClient = &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| Transport: &http.Transport{TLSClientConfig: tlsCfg}, | ||
| Timeout: 30 * time.Second, |
There was a problem hiding this comment.
nit: The default http.Client uses the system cert pool, which is correct for publicly-trusted gateway certs. If the gateway ever uses a private CA (e.g., air-gapped environments), this will need a configurable CA path re-added. The old TODO captured this well — consider keeping a one-line note for future readers.
github-project-automation
Bot
moved this from Backlog
to Done
in Kagenti Issue Prioritization
3 participants
Summary
Use status.url (gateway) instead of status.address.url (internal service) for the MLflow tracking URI. This avoids requiring the OpenShift service-serving CA in agent containers.
Remove MLFLOW_TRACKING_SERVER_CERT_PATH, DefaultCACertPath, custom CA loading, and MLflowAddressStatus type.
Related issue(s)
(Optional) Testing Instructions
Fixes #