UPSTREAM: <carry>: fix: skip unstarted handlers in RetrieveUserContainerEnvVars and ApplyCustomProperties - #409
UPSTREAM: <carry>: fix: skip unstarted handlers in RetrieveUserContainerEnvVars and ApplyCustomProperties#409jira-autofix[bot] wants to merge 2 commits into
Conversation
|
@jira-autofix[bot]: This pull request references RHOAIENG-76883 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe dispatcher skips environment-variable retrieval and custom-property application for handlers whose Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
Commit Checker results: |
|
@jira-autofix[bot]: This pull request references RHOAIENG-76883 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/v2/common/plugins/dispatcher_test.go (1)
300-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
startedHandlers == nilfallback. These cases only cover handlers that went throughOnTaskStart; add one test that never callsOnTaskStartsoRetrieveUserContainerEnvVarsandApplyCustomPropertiesstill cover the backward-compatibility path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/v2/common/plugins/dispatcher_test.go` around lines 300 - 381, Add a test covering a newly constructed dispatcher whose startedHandlers state remains nil because OnTaskStart is never called. Exercise both RetrieveUserContainerEnvVars and ApplyCustomProperties, and assert they use the backward-compatible behavior for the configured handler(s), including expected environment variables and applied properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/src/v2/common/plugins/dispatcher_test.go`:
- Around line 300-381: Add a test covering a newly constructed dispatcher whose
startedHandlers state remains nil because OnTaskStart is never called. Exercise
both RetrieveUserContainerEnvVars and ApplyCustomProperties, and assert they use
the backward-compatible behavior for the configured handler(s), including
expected environment variables and applied properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 21c7dbe9-834b-40ff-a38e-d2b5d2365d4e
📒 Files selected for processing (2)
backend/src/v2/common/plugins/dispatcher.gobackend/src/v2/common/plugins/dispatcher_test.go
|
A set of new images have been built to help with testing out this PR: |
|
An OCP cluster where you are logged in as cluster admin is required. The Data Science Pipelines team recommends testing this using the Data Science Pipelines Operator. Check here for more information on using the DSPO. To use and deploy a DSP stack with these images (assuming the DSPO is deployed), first save the following YAML to a file named apiVersion: datasciencepipelinesapplications.opendatahub.io/v1
kind: DataSciencePipelinesApplication
metadata:
name: pr-409
spec:
dspVersion: v2
apiServer:
image: "quay.io/opendatahub/ds-pipelines-api-server:pr-409"
argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-409"
argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-409"
persistenceAgent:
image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-409"
scheduledWorkflow:
image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-409"
mlmd:
deploy: true # Optional component
grpc:
image: "quay.io/opendatahub/mlmd-grpc-server:latest"
envoy:
image: "registry.redhat.io/openshift-service-mesh/proxyv2-rhel8:2.3.9-2"
mlpipelineUI:
deploy: true # Optional component
image: "quay.io/opendatahub/ds-pipelines-frontend:pr-409"
objectStorage:
minio:
deploy: true
image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'Then run the following: cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines.git
cd data-science-pipelines/
git fetch origin pull/409/head
git checkout -b pullrequest 28c3686ba80b66385fff843b801931f796056747
oc apply -f dspa.pr-409.yamlMore instructions here on how to deploy and test a Data Science Pipelines Application. |
Addresses review feedback on PR #409: - Removes raw env var values from duplicate-key log message in RetrieveUserContainerEnvVars to prevent sensitive data exposure (CWE-532). Now logs only the key name and handler name. RHOAIENG-76883 Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
jira-autofix
Bot
changed the title
Commit Checker results: |
|
Change to PR detected. A new PR build was completed. |
…nerEnvVars and ApplyCustomProperties When a task plugin handler fails during OnTaskStart (e.g., MLflow handler cannot read CA bundle), it is not recorded in startedHandlers. The OnTaskEnd method already checks startedHandlers and skips handlers that did not start successfully, but RetrieveUserContainerEnvVars and ApplyCustomProperties were missing this guard. This caused a fatal error when RetrieveUserContainerEnvVars tried to retrieve env vars (like MLFLOW_RUN_ID) from handlers whose initialization had failed, and ApplyCustomProperties would do unnecessary work on unstarted handlers. Add the same startedHandlers check to RetrieveUserContainerEnvVars and ApplyCustomProperties that OnTaskEnd already uses, so handlers that failed to start are skipped consistently across all dispatcher methods. Add tests covering the single unstarted handler case and the mixed (started + unstarted) handler case for both methods. RHOAIENG-76883 Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Addresses review feedback on PR #409: - Removes raw env var values from duplicate-key log message in RetrieveUserContainerEnvVars to prevent sensitive data exposure (CWE-532). Now logs only the key name and handler name. RHOAIENG-76883 Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
7008f86 to
df2cd81
Compare
Commit Checker results: |
|
Change to PR detected. A new PR build was completed. |
2 participants
Summary
Fixes RHOAIENG-76883: Pipeline scheduling fails when MLflow handler initialization fails due to missing CA bundle.
Root cause:
RetrieveUserContainerEnvVarsandApplyCustomPropertiesindispatcher.goiterated all handlers unconditionally. When the MLflow handler'sOnTaskStartfails (e.g., missing/dsp-custom-certs/dsp-ca.crt), it is not recorded instartedHandlers, butRetrieveUserContainerEnvVarsstill called it, causing a fatal error when trying to injectMLFLOW_RUN_IDfrom an uninitialized handler.Fix: Added the same
startedHandlersguard toRetrieveUserContainerEnvVarsandApplyCustomPropertiesthatOnTaskEndalready uses. Handlers whoseOnTaskStartfailed are now skipped consistently across all dispatcher methods.Additional change: Redacted env var values from the duplicate-key log message in
RetrieveUserContainerEnvVarsto prevent sensitive data exposure (CWE-532).Files changed
backend/src/v2/common/plugins/dispatcher.go— Added startedHandlers guard + redacted log valuesbackend/src/v2/common/plugins/dispatcher_test.go— Added 4 test cases covering skip behavior for both methodsTest plan
RetrieveUserContainerEnvVarsskipping unstarted handlers (single and mixed)ApplyCustomPropertiesskipping unstarted handlers (single and mixed)Summary by CodeRabbit
Bug Fixes
Tests