cherry-pick service-name env variable config from upstream KFP #233
Conversation
Signed-off-by: alyssacgoins <[email protected]>
WalkthroughThis pull request extends the backend system to support configurable ML Pipeline server address and port parameters. New configuration constants, accessor functions, and function signatures are introduced, with driver invocations updated to include ML pipeline server endpoint details. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alyssacgoins
changed the title
|
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-233
spec:
dspVersion: v2
apiServer:
image: "quay.io/opendatahub/ds-pipelines-api-server:pr-233"
argoDriverImage: "quay.io/opendatahub/ds-pipelines-driver:pr-233"
argoLauncherImage: "quay.io/opendatahub/ds-pipelines-launcher:pr-233"
persistenceAgent:
image: "quay.io/opendatahub/ds-pipelines-persistenceagent:pr-233"
scheduledWorkflow:
image: "quay.io/opendatahub/ds-pipelines-scheduledworkflow:pr-233"
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-233"
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 [email protected]:opendatahub-io/data-science-pipelines.git
cd data-science-pipelines/
git fetch origin pull/233/head
git checkout -b pullrequest 58951527b1071d7563977f7c8ec3b22405a3e70b
oc apply -f dspa.pr-233.yamlMore instructions here on how to deploy and test a Data Science Pipelines Application. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test_data/compiled-workflows/mixed_parameters.yaml (1)
1-449: I'll examine the three files identified as missing the configuration to determine if they should have been updated as part of this change.Let me examine the structure of the files missing the
ml_pipeline_server_addressconfiguration:Let me inspect the structure of one of the files identified as missing the configuration:
Based on the shell script execution results, I found that the review comment raises a valid concern about consistency across compiled workflow test files. The script identified three YAML files with driver invocations that lack the
ml_pipeline_server_addressconfiguration that is present inmixed_parameters.yaml:
test_data/compiled-workflows/arguments-parameters.yamltest_data/compiled-workflows/hello-world.yamltest_data/compiled-workflows/arguments.pipeline.yamlThese files contain driver templates but are missing the ML pipeline server configuration that the updated
mixed_parameters.yamlnow includes. Since this appears to be part of a KFP v2 driver update for proper integration with ML Pipeline servers, these other files should be updated as well to maintain consistency across the test data fixtures.Update the three identified compiled workflow files to include ml_pipeline_server_address and ml_pipeline_server_port parameters in their driver templates.
These files have driver invocations but are missing the ml_pipeline_server configuration added to mixed_parameters.yaml:
- test_data/compiled-workflows/arguments-parameters.yaml
- test_data/compiled-workflows/hello-world.yaml
- test_data/compiled-workflows/arguments.pipeline.yaml
Each should include
--ml_pipeline_server_address ml-pipeline.kubeflowand--ml_pipeline_server_port "8887"in their system-container-driver and system-dag-driver templates for consistency and proper functionality.backend/src/v2/component/launcher_v2.go (1)
1-1050: Run golangci-lint on backend Go changes to catch logic and style issuesPer the backend Go guidelines, golangci-lint should be run on this package. Manual verification reveals a critical bug that linting would catch:
Bug at line 701 in
downloadArtifacts: Inside thecloseNonDefaultBucketsclosure,err.Error()is logged instead ofcloseBucketErr.Error(). The local error variable is shadowed by the outer scope error, causing wrong error context to be logged and potential nil pointer dereference.if closeBucketErr := bucket.Close(); closeBucketErr != nil { glog.Warningf("failed to close bucket %q: %q", name, err.Error()) // Should be closeBucketErr.Error() }Fix: Replace
err.Error()withcloseBucketErr.Error()at line 701.backend/src/v2/cacheutils/cache.go (1)
97-97: Fix misleading error message.The error message says
"metadata.NewClient() failed"but this is actually the cache client initialization. This appears to be copy-pasted from metadata/client.go.Apply this fix:
- return nil, fmt.Errorf("metadata.NewClient() failed: %w", err) + return nil, fmt.Errorf("cache.NewClient() failed: %w", err)
🧹 Nitpick comments (3)
backend/src/v2/component/launcher_v2.go (1)
47-64: LauncherV2Options struct extension looks safe; consider whether ml pipeline fields need validationAdding
MLPipelineServerAddressandMLPipelineServerPorttoLauncherV2Optionscleanly preserves backward compatibility and doesn’t affect existing behavior, since they’re only stored on the options struct here. The currentvalidate()method still only enforces MLMD address/port and core pod identity; that’s fine if ML pipeline address/port are expected to have defaults from config/flags, but if they are meant to be strictly required it might be worth adding symmetric checks later. I wouldn’t block this PR on that, especially for a cherry-pick.Also applies to: 279-299
backend/src/v2/component/launcher_v2_test.go (1)
323-323: Consider extracting hardcoded ML Pipeline server values to test constants.The hardcoded values
"ml-pipeline.kubeflow"and"8887"appear throughout the codebase. For better maintainability, consider defining these as package-level test constants.Example:
+const ( + testMLPipelineServerAddress = "ml-pipeline.kubeflow" + testMLPipelineServerPort = "8887" +) + func Test_NewLauncherV2(t *testing.T) { var testCmdArgs = []string{"sh", "-c", "echo \"hello world\""} - disabledCacheClient, _ := cacheutils.NewClient("ml-pipeline.kubeflow", "8887", true, &tls.Config{}) + disabledCacheClient, _ := cacheutils.NewClient(testMLPipelineServerAddress, testMLPipelineServerPort, true, &tls.Config{})backend/src/v2/cacheutils/cache.go (1)
88-89: Consider adding input validation for server configuration.The endpoint construction assumes valid inputs but doesn't validate them. Consider adding checks for empty or malformed address/port values to provide clearer error messages.
Example:
func NewClient(mlPipelineServerAddress string, mlPipelineServerPort string, cacheDisabled bool, tlsCfg *tls.Config) (Client, error) { if cacheDisabled { return &disabledCacheClient{}, nil } + + if mlPipelineServerAddress == "" || mlPipelineServerPort == "" { + return nil, fmt.Errorf("mlPipelineServerAddress and mlPipelineServerPort must not be empty") + } creds := insecure.NewCredentials() if tlsCfg != nil { creds = credentials.NewTLS(tlsCfg) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
backend/src/apiserver/common/config.go(2 hunks)backend/src/apiserver/common/const.go(1 hunks)backend/src/v2/cacheutils/cache.go(2 hunks)backend/src/v2/cacheutils/cache_test.go(3 hunks)backend/src/v2/client_manager/client_manager.go(3 hunks)backend/src/v2/cmd/driver/main.go(2 hunks)backend/src/v2/cmd/launcher-v2/main.go(3 hunks)backend/src/v2/compiler/argocompiler/container.go(2 hunks)backend/src/v2/compiler/argocompiler/dag.go(2 hunks)backend/src/v2/compiler/argocompiler/importer.go(1 hunks)backend/src/v2/component/launcher_v2.go(1 hunks)backend/src/v2/component/launcher_v2_test.go(1 hunks)backend/src/v2/config/env.go(4 hunks)backend/src/v2/metadata/env.go(1 hunks)backend/test/v2/integration/cache_test.go(1 hunks)test_data/compiled-workflows/add_numbers.yaml(2 hunks)test_data/compiled-workflows/arguments_parameters.yaml(2 hunks)test_data/compiled-workflows/artifact_cache.yaml(2 hunks)test_data/compiled-workflows/artifact_crust.yaml(2 hunks)test_data/compiled-workflows/artifacts_complex.yaml(2 hunks)test_data/compiled-workflows/artifacts_simple.yaml(2 hunks)test_data/compiled-workflows/collected_artifacts.yaml(2 hunks)test_data/compiled-workflows/collected_parameters.yaml(2 hunks)test_data/compiled-workflows/component_with_metadata_fields.yaml(2 hunks)test_data/compiled-workflows/component_with_optional_inputs.yaml(2 hunks)test_data/compiled-workflows/component_with_pip_index_urls.yaml(2 hunks)test_data/compiled-workflows/component_with_pip_install.yaml(2 hunks)test_data/compiled-workflows/component_with_pip_install_in_venv.yaml(2 hunks)test_data/compiled-workflows/components_with_optional_artifacts.yaml(2 hunks)test_data/compiled-workflows/concat_message.yaml(2 hunks)test_data/compiled-workflows/conditional_producer_and_consumers.yaml(2 hunks)test_data/compiled-workflows/container_component_with_no_inputs.yaml(2 hunks)test_data/compiled-workflows/container_io.yaml(2 hunks)test_data/compiled-workflows/container_no_input.yaml(2 hunks)test_data/compiled-workflows/container_with_artifact_output.yaml(2 hunks)test_data/compiled-workflows/container_with_concat_placeholder.yaml(2 hunks)test_data/compiled-workflows/container_with_if_placeholder.yaml(2 hunks)test_data/compiled-workflows/container_with_placeholder_in_fstring.yaml(2 hunks)test_data/compiled-workflows/containerized_python_component.yaml(2 hunks)test_data/compiled-workflows/create_pod_metadata_complex.yaml(2 hunks)test_data/compiled-workflows/cross_loop_after_topology.yaml(2 hunks)test_data/compiled-workflows/dict_input.yaml(2 hunks)test_data/compiled-workflows/embedded_artifact.yaml(2 hunks)test_data/compiled-workflows/env-var.yaml(2 hunks)test_data/compiled-workflows/fail_v2.yaml(2 hunks)test_data/compiled-workflows/flip_coin.yaml(2 hunks)test_data/compiled-workflows/hello_world.yaml(2 hunks)test_data/compiled-workflows/identity.yaml(2 hunks)test_data/compiled-workflows/if_elif_else_complex.yaml(2 hunks)test_data/compiled-workflows/if_elif_else_with_oneof_parameters.yaml(2 hunks)test_data/compiled-workflows/if_else_with_oneof_artifacts.yaml(2 hunks)test_data/compiled-workflows/if_else_with_oneof_parameters.yaml(2 hunks)test_data/compiled-workflows/input_artifact.yaml(2 hunks)test_data/compiled-workflows/iris_pipeline_compiled.yaml(2 hunks)test_data/compiled-workflows/lightweight_python_functions_pipeline.yaml(2 hunks)test_data/compiled-workflows/lightweight_python_functions_with_outputs.yaml(2 hunks)test_data/compiled-workflows/log_streaming_compiled.yaml(2 hunks)test_data/compiled-workflows/long-running.yaml(2 hunks)test_data/compiled-workflows/loop_consume_upstream.yaml(2 hunks)test_data/compiled-workflows/metrics_visualization_v2.yaml(2 hunks)test_data/compiled-workflows/mixed_parameters.yaml(2 hunks)test_data/compiled-workflows/modelcar.yaml(2 hunks)test_data/compiled-workflows/multiple_artifacts_namedtuple.yaml(2 hunks)test_data/compiled-workflows/multiple_parameters_namedtuple.yaml(2 hunks)test_data/compiled-workflows/nested_pipeline_opt_input_child_level_compiled.yaml(2 hunks)test_data/compiled-workflows/nested_pipeline_opt_inputs_nil_compiled.yaml(2 hunks)test_data/compiled-workflows/nested_pipeline_opt_inputs_parent_level_compiled.yaml(2 hunks)test_data/compiled-workflows/nested_return.yaml(2 hunks)test_data/compiled-workflows/nested_with_parameters.yaml(2 hunks)test_data/compiled-workflows/notebook_component_mixed.yaml(2 hunks)test_data/compiled-workflows/notebook_component_simple.yaml(2 hunks)test_data/compiled-workflows/output_metrics.yaml(2 hunks)test_data/compiled-workflows/parallel_for_after_dependency.yaml(2 hunks)test_data/compiled-workflows/parameter_cache.yaml(2 hunks)test_data/compiled-workflows/parameter_oneof.yaml(2 hunks)test_data/compiled-workflows/parameters_complex.yaml(2 hunks)test_data/compiled-workflows/parameters_simple.yaml(2 hunks)test_data/compiled-workflows/pipeline_as_exit_task.yaml(2 hunks)test_data/compiled-workflows/pipeline_in_pipeline.yaml(2 hunks)test_data/compiled-workflows/pipeline_in_pipeline_complex.yaml(2 hunks)test_data/compiled-workflows/pipeline_in_pipeline_loaded_from_yaml.yaml(2 hunks)test_data/compiled-workflows/pipeline_producer_consumer.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_after.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_artifact_upload_download.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_concat_placeholder.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_condition.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_condition_dynamic_task_output_custom_training_job.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_dynamic_importer_metadata.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_dynamic_task_output_custom_training_job.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_env.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_exit_handler.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_google_artifact_type.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_importer.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_importer_and_gcpc_types.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_input_status_state.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_loops.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_loops_and_conditions.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_metadata_fields.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_metrics_outputs.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_multiple_exit_handlers.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_nested_conditions.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_nested_conditions_yaml.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_nested_loops.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_only_display_name.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_outputs.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_parallelfor_parallelism.yaml(2 hunks)test_data/compiled-workflows/pipeline_with_params_containing_format.yaml(2 hunks)
⛔ Files not processed due to max files limit (35)
- test_data/compiled-workflows/pipeline_with_placeholders.yaml
- test_data/compiled-workflows/pipeline_with_pod_metadata.yaml
- test_data/compiled-workflows/pipeline_with_retry.yaml
- test_data/compiled-workflows/pipeline_with_reused_component.yaml
- test_data/compiled-workflows/pipeline_with_secret_as_env.yaml
- test_data/compiled-workflows/pipeline_with_secret_as_volume.yaml
- test_data/compiled-workflows/pipeline_with_semaphore.yaml
- test_data/compiled-workflows/pipeline_with_string_machine_fields_pipeline_input.yaml
- test_data/compiled-workflows/pipeline_with_string_machine_fields_task_output.yaml
- test_data/compiled-workflows/pipeline_with_submit_request.yaml
- test_data/compiled-workflows/pipeline_with_task_final_status.yaml
- test_data/compiled-workflows/pipeline_with_task_final_status_yaml.yaml
- test_data/compiled-workflows/pipeline_with_task_using_ignore_upstream_failure.yaml
- test_data/compiled-workflows/pipeline_with_utils.yaml
- test_data/compiled-workflows/pipeline_with_various_io_types.yaml
- test_data/compiled-workflows/pipeline_with_volume.yaml
- test_data/compiled-workflows/pipeline_with_volume_no_cache.yaml
- test_data/compiled-workflows/pipeline_with_workspace.yaml
- test_data/compiled-workflows/placeholder_with_if_placeholder_none_input_value.yaml
- test_data/compiled-workflows/preprocess.yaml
- test_data/compiled-workflows/producer_consumer_param_pipeline.yaml
- test_data/compiled-workflows/pvc_mount.yaml
- test_data/compiled-workflows/pythonic_artifact_with_single_return.yaml
- test_data/compiled-workflows/pythonic_artifacts_test_pipeline.yaml
- test_data/compiled-workflows/pythonic_artifacts_with_list_of_artifacts.yaml
- test_data/compiled-workflows/pythonic_artifacts_with_multiple_returns.yaml
- test_data/compiled-workflows/ray_integration_compiled.yaml
- test_data/compiled-workflows/sequential_v1.yaml
- test_data/compiled-workflows/sequential_v2.yaml
- test_data/compiled-workflows/take_nap_compiled.yaml
- test_data/compiled-workflows/take_nap_pipeline_root_compiled.yaml
- test_data/compiled-workflows/two_step_pipeline.yaml
- test_data/compiled-workflows/two_step_pipeline_containerized.yaml
- test_data/compiled-workflows/upload_download_compiled.yaml
- test_data/compiled-workflows/xgboost_sample_pipeline.yaml
🧰 Additional context used
📓 Path-based instructions (1)
backend/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Run golangci-lint on Go backend code to enforce Go linting rules
Files:
backend/src/apiserver/common/const.gobackend/test/v2/integration/cache_test.gobackend/src/v2/component/launcher_v2_test.gobackend/src/v2/compiler/argocompiler/dag.gobackend/src/v2/cacheutils/cache_test.gobackend/src/v2/metadata/env.gobackend/src/v2/compiler/argocompiler/container.gobackend/src/v2/client_manager/client_manager.gobackend/src/v2/cacheutils/cache.gobackend/src/v2/config/env.gobackend/src/v2/compiler/argocompiler/importer.gobackend/src/v2/cmd/driver/main.gobackend/src/v2/component/launcher_v2.gobackend/src/apiserver/common/config.gobackend/src/v2/cmd/launcher-v2/main.go
🧬 Code graph analysis (13)
backend/test/v2/integration/cache_test.go (2)
backend/src/v2/metadata/testutils/test_utils.go (1)
NewTestMlmdClient(13-30)backend/src/v2/metadata/env.go (1)
GetMetadataConfig(14-19)
backend/src/v2/component/launcher_v2_test.go (1)
backend/src/v2/cacheutils/cache.go (1)
NewClient(79-103)
backend/src/v2/compiler/argocompiler/dag.go (2)
backend/src/v2/config/env.go (1)
GetMLPipelineServerConfig(195-200)backend/src/v2/metadata/env.go (1)
GetMetadataConfig(14-19)
backend/src/v2/cacheutils/cache_test.go (1)
backend/src/v2/cacheutils/cache.go (1)
NewClient(79-103)
backend/src/v2/metadata/env.go (2)
backend/src/v2/config/env.go (1)
ServerConfig(66-69)backend/src/apiserver/common/config.go (2)
GetMetadataServiceName(124-126)GetPodNamespace(116-118)
backend/src/v2/compiler/argocompiler/container.go (2)
backend/src/v2/config/env.go (1)
GetMLPipelineServerConfig(195-200)backend/src/v2/metadata/env.go (1)
GetMetadataConfig(14-19)
backend/src/v2/client_manager/client_manager.go (2)
backend/test/config/flags.go (1)
CaCertPath(37-37)backend/src/v2/cacheutils/cache.go (2)
Client(31-42)NewClient(79-103)
backend/src/v2/cacheutils/cache.go (1)
backend/src/v2/metadata/client.go (2)
NewClient(119-144)Client(113-116)
backend/src/v2/config/env.go (2)
backend/src/v2/metadata/env.go (1)
ServerConfig(9-12)backend/src/apiserver/common/config.go (2)
GetMLPipelineServiceName(120-122)GetPodNamespace(116-118)
backend/src/v2/compiler/argocompiler/importer.go (1)
backend/src/v2/metadata/env.go (1)
GetMetadataConfig(14-19)
backend/src/v2/cmd/driver/main.go (1)
backend/src/v2/cacheutils/cache.go (1)
NewClient(79-103)
backend/src/apiserver/common/config.go (1)
backend/src/apiserver/common/const.go (2)
DefaultMLPipelineServiceName(88-88)DefaultMetadataServiceName(89-89)
backend/src/v2/cmd/launcher-v2/main.go (1)
backend/test/config/flags.go (2)
Namespace(23-23)CaCertPath(37-37)
🔇 Additional comments (94)
test_data/compiled-workflows/pipeline_with_concat_placeholder.yaml (2)
63-66: YAML syntax and consistency look good.The new ML pipeline server configuration arguments are properly formatted and consistently placed alongside existing proxy arguments in the
system-container-drivertemplate. Verify that the driver executable supports these new CLI flags.
268-271: Consistency across driver templates confirmed.The same server configuration arguments are correctly applied to the
system-dag-drivertemplate with identical values, maintaining consistency across the workflow definition.test_data/compiled-workflows/components_with_optional_artifacts.yaml (2)
86-89: Added ML Pipeline server configuration to system-container-driver template.The arguments are correctly formatted, properly placed in the args list, and use appropriate values for the test data context. Values are consistent with the parallel changes in the system-dag-driver template.
380-383: Added ML Pipeline server configuration to system-dag-driver template.The arguments mirror the changes in the system-container-driver template with matching address and port values, maintaining consistency across the workflow specification.
test_data/compiled-workflows/notebook_component_simple.yaml (1)
168-171: ML Pipeline server configuration added consistently across driver templates.The changes introduce
--ml_pipeline_server_addressand--ml_pipeline_server_portarguments to both thesystem-container-driver(lines 168–171) andsystem-dag-driver(lines 373–376) templates. The hardcoded values (ml-pipeline.kubeflowand8887) align with Kubeflow defaults and follow the same pattern as the existingmlmd_server_address/portconfiguration. The additions are syntactically consistent with the surrounding command arguments.Please verify that the hardcoded server endpoint values are appropriate for the test environment and that this change aligns with the upstream KFP cherry-pick (PR kubeflow#12463). If this is a generated/compiled workflow file, consider confirming that the source compiler or DSL template has been updated accordingly.
Also applies to: 373-376
test_data/compiled-workflows/pipeline_with_artifact_upload_download.yaml (1)
91-94: Changes follow established patterns and appear correct.The additions of
--ml_pipeline_server_addressand--ml_pipeline_server_portto both driver templates (container-driver and dag-driver) are properly placed, follow the existing argument structure, and maintain consistency across templates. The pattern mirrors the existing metadata server configuration.Please verify that the hardcoded values (
ml-pipeline.kubeflowand8887) match the defaults in the upstream KFP cherry-pick (PR kubeflow#12463). Since this is a compiled workflow fixture, confirm it's regenerated from the higher-level DSL/configuration rather than manually edited.Also applies to: 321-324
test_data/compiled-workflows/iris_pipeline_compiled.yaml (1)
123-126: Consistent ML pipeline server configuration added in both driver templates.The new command-line arguments for
ml_pipeline_server_addressandml_pipeline_server_portare correctly added in both thesystem-container-driver(lines 123–126) andsystem-dag-driver(lines 378–381) templates, with identical values positioned before the MLMD server arguments.Since this is a test data file (auto-generated compiled workflow), verify that the hardcoded defaults (
ml-pipeline.kubeflowand8887) match the configuration defaults established elsewhere in the codebase and align with the upstream KFP PR kubeflow#12463 that this cherry-pick is based on.Also applies to: 378-381
test_data/compiled-workflows/pipeline_producer_consumer.yaml (2)
123-126: LGTM!The new arguments are properly formatted and consistently placed alongside existing configuration parameters. The default values (
ml-pipeline.kubeflowand port8887) are appropriate for standard Kubeflow deployments.
328-331: Consistent across driver templates.The same configuration is properly replicated in the
system-dag-drivertemplate, maintaining uniformity between both driver invocations.test_data/compiled-workflows/nested_pipeline_opt_inputs_parent_level_compiled.yaml (1)
153-156: Changes verified as consistent across all compiled workflow test data files.Verification confirms this change is part of a systematic update across 127 test data files in the directory. Spot-checks of add_numbers.yaml and arguments_parameters.yaml show identical argument placement, naming, and hardcoded values (ml-pipeline.kubeflow address). The arguments are properly formatted, consistently positioned in both driver templates, and correctly specified. No issues found.
test_data/compiled-workflows/mixed_parameters.yaml (2)
87-90: ML pipeline server configuration added to system-container-driver.The command-line arguments for ML pipeline server address and port are correctly added before the MLMD server arguments. The placement and formatting are consistent with the existing argument pattern.
292-295: ML pipeline server configuration added to system-dag-driver.The command-line arguments for ML pipeline server address and port mirror the additions in
system-container-driver(lines 87-90), ensuring consistency across both driver templates.test_data/compiled-workflows/pipeline_with_metrics_outputs.yaml (3)
75-78: Verify this is not an auto-generated test fixture that should be regenerated instead.This appears to be compiled test data showing expected Argo Workflow output. If the source workflow template generator was modified to add these new driver arguments, the test data should be regenerated from that source rather than manually updated. Manually maintained compiled fixtures can drift from their source.
Please clarify:
- Is this test data file manually maintained or auto-generated from templates?
- If auto-generated, has the source generator been updated (e.g., in KFP's workflow generation code)?
- Are there other compiled workflow test files that should receive the same updates?
Also applies to: 280-283
75-78: Verify the hardcoded server address and port are appropriate defaults.The changes add hardcoded values
ml-pipeline.kubeflowand"8887"to both driver templates. Confirm:
- These are the correct default values that should be baked into test workflows
- These values align with the upstream KFP changes being cherry-picked
- Whether runtime overrides or environment variables should provide flexibility instead
To verify alignment with upstream, you might cross-check against the original PR: kubeflow#12463
Also applies to: 280-283
38-82: All driver invocations have been updated consistently.Verification confirms:
system-container-driver(lines 38–82) includes the new ML pipeline server arguments (--ml_pipeline_server_addressat line 75,--mlmd_server_addressat line 79)system-dag-driver(lines 245–287) includes the new ML pipeline server arguments (--ml_pipeline_server_addressat line 280,--mlmd_server_addressat line 284)- No other driver templates exist in this file
- Sampled other test data files show consistent application of the same arguments across all driver invocations
test_data/compiled-workflows/modelcar.yaml (1)
95-98: ML pipeline server configuration arguments added consistently across templates.Both system-container-driver and system-dag-driver templates now include hardcoded ML pipeline server address and port with values
ml-pipeline.kubeflowand8887. This aligns with the expected defaults from the backend configuration layer. All test data files reviewed show identical values and placement.Also applies to: 394-397
test_data/compiled-workflows/if_else_with_oneof_parameters.yaml (1)
94-97: ML pipeline server configuration consistently added to driver templates.Both
system-container-driverandsystem-dag-drivertemplates have been updated with the new--ml_pipeline_server_addressand--ml_pipeline_server_portarguments. Placement and values are consistent across both locations.Also applies to: 333-336
test_data/compiled-workflows/loop_consume_upstream.yaml (1)
118-121: Consistent ML pipeline server configuration applied.Both driver templates properly updated with matching argument placement and values.
Also applies to: 350-353
test_data/compiled-workflows/lightweight_python_functions_pipeline.yaml (1)
125-128: Consistent ML pipeline server configuration applied.Both driver templates properly updated with matching argument placement and values.
Also applies to: 355-358
test_data/compiled-workflows/conditional_producer_and_consumers.yaml (1)
92-95: Consistent ML pipeline server configuration applied.Both driver templates properly updated with matching argument placement and values.
Also applies to: 329-332
test_data/compiled-workflows/pipeline_with_input_status_state.yaml (1)
88-91: Consistent ML pipeline server configuration applied.Both driver templates properly updated with matching argument placement and values.
Also applies to: 325-328
test_data/compiled-workflows/pipeline_with_after.yaml (1)
64-67: Consistent ML pipeline server configuration applied.Both driver templates properly updated with matching argument placement and values.
Also applies to: 322-325
test_data/compiled-workflows/pipeline_with_parallelfor_parallelism.yaml (1)
168-171: Consistent ML pipeline server configuration applied.Both driver templates properly updated with matching argument placement and values.
Also applies to: 373-376
test_data/compiled-workflows/component_with_pip_install.yaml (1)
72-75: Consistent ML pipeline server configuration applied.Both driver templates properly updated with matching argument placement and values.
Also applies to: 277-280
test_data/compiled-workflows/pipeline_with_google_artifact_type.yaml (1)
153-156: Consistent pattern across test data: ML pipeline server address and port added to driver templates.These changes add configurable ML pipeline server endpoint parameters to both
system-container-driverandsystem-dag-drivertemplates, aligning with the PR objective to support ML pipeline server name/address configuration. The values (ml-pipeline.kubeflow:8887) are placed consistently after proxy settings and before MLMD settings.All 8 provided test data files follow the identical pattern, suggesting these are generated artifacts. This consistency is a positive signal, but verify that:
- The upstream commit (kubeflow#12463) includes corresponding backend code to consume these parameters.
- The hardcoded defaults here align with the backend configuration constants introduced elsewhere in the PR.
Also applies to: 395-398
test_data/compiled-workflows/env-var.yaml (1)
72-75: Test data aligned with configurable ML pipeline server changes.The additions of
--ml_pipeline_server_addressand--ml_pipeline_server_portto both the system-container-driver and system-dag-driver templates correctly reflect the backend configuration changes. Placement after proxy settings and before MLMD server configuration is semantically correct.Also applies to: 277-280
test_data/compiled-workflows/notebook_component_mixed.yaml (1)
299-302: Test data consistent with ML pipeline server configuration propagation.Both driver template instances correctly include the new ML pipeline server configuration arguments. Values and placement align with the broader set of test data updates in this PR.
Also applies to: 554-557
test_data/compiled-workflows/create_pod_metadata_complex.yaml (1)
114-117: ML pipeline server configuration arguments correctly integrated.The additions maintain consistency with the broader test data updates. Both driver templates include the new configuration knobs in the expected locations.
Also applies to: 619-622
test_data/compiled-workflows/pipeline_with_dynamic_task_output_custom_training_job.yaml (1)
148-151: ML pipeline server configuration properly integrated into custom training job workflow.Both driver templates contain the new configuration arguments in correct positions.
Also applies to: 426-429
test_data/compiled-workflows/pipeline_with_importer_and_gcpc_types.yaml (1)
65-68: ML pipeline server configuration arguments correctly added.The changes align with the test data update pattern across the PR.
Also applies to: 340-343
test_data/compiled-workflows/pipeline_with_params_containing_format.yaml (1)
90-93: Configuration arguments properly integrated for format parameter workflow.Changes follow established pattern across all test data updates.
Also applies to: 296-299
test_data/compiled-workflows/nested_return.yaml (1)
71-74: ML pipeline server configuration correctly added to nested return workflow.Both driver templates include the new arguments in the expected positions.
Also applies to: 276-279
test_data/compiled-workflows/parallel_for_after_dependency.yaml (1)
73-76: ML pipeline server configuration arguments properly integrated.The changes maintain consistency with all other test data updates across this PR.
Also applies to: 278-281
test_data/compiled-workflows/container_with_placeholder_in_fstring.yaml (1)
61-64: LGTM! ML pipeline server configuration added correctly.The addition of
--ml_pipeline_server_addressand--ml_pipeline_server_portflags to both driver templates aligns with the PR's objective to introduce configurable ML pipeline server endpoints.Also applies to: 266-269
test_data/compiled-workflows/container_no_input.yaml (1)
61-64: LGTM! Consistent with ML pipeline server configuration pattern.The flags are correctly positioned and use the expected default values.
Also applies to: 266-269
test_data/compiled-workflows/pipeline_with_outputs.yaml (1)
77-80: LGTM! ML pipeline server endpoints properly configured.Also applies to: 307-310
backend/test/v2/integration/cache_test.go (1)
50-50: LGTM! Accessor rename applied correctly.The change from
metadata.DefaultConfig().Porttometadata.GetMetadataConfig().Portcorrectly uses the renamed accessor function. The functionality remains unchanged.test_data/compiled-workflows/pipeline_as_exit_task.yaml (1)
107-110: LGTM! ML pipeline server configuration applied consistently.Also applies to: 313-316
test_data/compiled-workflows/containerized_python_component.yaml (1)
61-64: LGTM! Configuration flags added as expected.Also applies to: 266-269
test_data/compiled-workflows/parameters_simple.yaml (1)
91-94: LGTM! ML pipeline server parameters properly integrated.Also applies to: 296-299
backend/src/v2/metadata/env.go (1)
14-19: LGTM! Configuration refactor improves flexibility.The rename from
DefaultConfig()toGetMetadataConfig()better reflects the function's purpose, and usingcommon.GetMetadataServiceName()instead of a hardcoded string enables the service name to be configured via environment variables, aligning with the PR's objectives.backend/src/v2/compiler/argocompiler/importer.go (1)
82-83: LGTM - Clean config accessor refactoring.The change from
metadata.DefaultConfig()tometadata.GetMetadataConfig()is a straightforward refactoring that provides better encapsulation of metadata server configuration. The new accessor properly constructs the fully-qualified domain name using the service name and namespace.test_data/compiled-workflows/pipeline_with_nested_conditions.yaml (1)
91-94: The test configuration is correct; port 8887 is the standard gRPC port for Kubeflow Pipelines.The original review comment claims port 8887 is "non-standard", but this is incorrect. Based on the codebase analysis:
- Port 8888 is used for HTTP/REST API traffic
- Port 8887 is the standard gRPC port
The backend source code confirms this: both launcher-v2 and driver default to port 8887 for the ml_pipeline_server_port, and the manifest files consistently show port 8888 for HTTP and port 8887 for gRPC in the ml-pipeline service definition.
The test_data files at lines 91-94 and 321-324 are correctly configured with port "8887" for gRPC communication, matching the upstream Kubeflow Pipelines service architecture. No changes are needed.
Likely an incorrect or invalid review comment.
test_data/compiled-workflows/pipeline_with_env.yaml (2)
76-79: Consistent with upstream KFP configuration.ML pipeline server endpoint arguments added with expected defaults. Placement after proxy configuration and before metadata server configuration follows logical organization.
305-308: Consistent argument propagation across driver templates.The same ML pipeline server configuration arguments are correctly applied to the DAG driver template, ensuring both container and DAG execution paths have the necessary configuration.
test_data/compiled-workflows/artifacts_simple.yaml (2)
100-103: ML pipeline server configuration in system-container-driver.Consistent with other test workflows; configuration arguments properly positioned.
305-308: ML pipeline server configuration in system-dag-driver.Configuration arguments correctly replicated in DAG driver template.
test_data/compiled-workflows/container_with_if_placeholder.yaml (2)
64-67: ML pipeline server configuration in system-container-driver.Consistent with repository-wide pattern; arguments properly placed.
269-272: ML pipeline server configuration in system-dag-driver.Configuration arguments correctly applied in DAG driver template.
test_data/compiled-workflows/flip_coin.yaml (2)
127-130: ML pipeline server configuration in system-container-driver.Endpoint and port arguments added with correct defaults per upstream.
368-371: ML pipeline server configuration in system-dag-driver.Configuration correctly mirrored in DAG driver template.
test_data/compiled-workflows/pipeline_with_dynamic_importer_metadata.yaml (2)
135-138: ML pipeline server configuration in system-container-driver.Consistent application with other workflows in this PR.
364-367: ML pipeline server configuration in system-dag-driver.Configuration properly applied in DAG driver.
test_data/compiled-workflows/identity.yaml (2)
70-73: ML pipeline server configuration in system-container-driver.Endpoint and port arguments correctly added.
275-278: ML pipeline server configuration in system-dag-driver.Configuration consistently applied across driver templates.
test_data/compiled-workflows/cross_loop_after_topology.yaml (2)
91-94: ML pipeline server configuration in system-container-driver.Consistent with repository pattern.
328-331: ML pipeline server configuration in system-dag-driver.Configuration properly replicated.
test_data/compiled-workflows/pipeline_with_condition_dynamic_task_output_custom_training_job.yaml (2)
186-189: ML pipeline server configuration in system-container-driver.Consistent application per PR objectives.
391-394: ML pipeline server configuration in system-dag-driver.Configuration correctly applied.
test_data/compiled-workflows/multiple_artifacts_namedtuple.yaml (1)
91-94: ML pipeline server flags are correctly added and orderedThe new
--ml_pipeline_server_address/--ml_pipeline_server_portflags and values are consistent with other workflows and correctly inserted alongside the existing proxy and MLMD flags for both container and DAG drivers.Also applies to: 296-299
test_data/compiled-workflows/collected_parameters.yaml (1)
116-119: Consistent ml pipeline server configuration in driver argsBoth container and DAG driver templates correctly add
--ml_pipeline_server_address ml-pipeline.kubeflowand--ml_pipeline_server_port "8887"in the same position relative to proxy and MLMD flags as other compiled workflows.Also applies to: 346-349
test_data/compiled-workflows/nested_pipeline_opt_inputs_nil_compiled.yaml (1)
101-104: Nested pipeline drivers wired to ml pipeline serverThe added ml pipeline server address/port flags in both the system container driver and DAG driver blocks match the established pattern and keep configuration consistent for nested executions.
Also applies to: 354-357
test_data/compiled-workflows/collected_artifacts.yaml (1)
181-184: Collected-artifacts workflow updated with ml pipeline server args
--ml_pipeline_server_addressand--ml_pipeline_server_portare added consistently to both driver templates and positioned before the MLMD server flags, matching the rest of the suite.Also applies to: 386-389
test_data/compiled-workflows/pipeline_with_loops_and_conditions.yaml (1)
150-153: Loop/condition drivers receive ml pipeline server configurationThe additional ml pipeline server address/port flags are correctly threaded into both driver arg lists without affecting control-flow structure, and they remain adjacent to the existing proxy and MLMD options.
Also applies to: 419-422
test_data/compiled-workflows/pipeline_with_loops.yaml (1)
104-107: Loop-only workflow drivers aligned with new ml pipeline server flagsBoth the container and DAG drivers now include the ml pipeline server address/port flags in the standard position; this keeps the compiled expectations in sync with the new driver CLI.
Also applies to: 469-472
test_data/compiled-workflows/component_with_optional_inputs.yaml (1)
74-77: Optional-input component workflow updated consistentlyThe ml pipeline server address/port flags are added to both driver arg lists with the same values and ordering as the rest of the compiled workflows, so tests for this simple component stay aligned.
Also applies to: 279-282
backend/src/v2/compiler/argocompiler/container.go (1)
211-214: Verification confirms config functions properly handle edge cases—approved.The refactoring to config-based server endpoints is sound. Script verification confirms:
GetPodNamespace()handles missing namespace viaDefaultPodNamespacefallbackGetMLPipelineServiceName()andGetMetadataServiceName()both useGetStringConfigWithDefault()with their respective defaults- Address format differences are correctly implemented: ML pipeline uses
serviceName.namespace, metadata usesserviceName.namespace.svc.cluster.local- No initialization issues or unhandled edge cases
test_data/compiled-workflows/component_with_metadata_fields.yaml (1)
85-88: I need to verify one more detail—whether "kubeflow" is defined as a default namespace constant in the backend code:Hardcoded test values are correctly aligned with backend configuration.
The ml-pipeline service is deployed in the kubeflow namespace by default, and verification confirms:
- Service name
ml-pipelinematchesDefaultMLPipelineServiceName(backend/src/apiserver/common/const.go:88)- Port
8887matchesmlPipelineGrpcServicePort(backend/src/v2/config/env.go:48)- Address
ml-pipeline.kubeflowcorrectly combines the service name and namespaceThe test workflow values at lines 85-88 and 290-293 are accurate and consistent with backend defaults.
test_data/compiled-workflows/component_with_pip_index_urls.yaml (1)
72-75: Test data update: ML pipeline server config arguments consistently added.Both system-container-driver and system-dag-driver templates now include the new
--ml_pipeline_server_addressand--ml_pipeline_server_portarguments. Placement and values are consistent with the PR pattern across all test workflows.Verify that the hardcoded test values (
ml-pipeline.kubeflowand"8887") match the backend defaults defined inbackend/src/apiserver/common/const.goandbackend/src/v2/config/env.go.Also applies to: 277-280
test_data/compiled-workflows/artifact_cache.yaml (1)
89-92: Consistent with PR pattern.Same argument additions in identical locations as component_with_pip_index_urls.yaml.
Also applies to: 294-297
test_data/compiled-workflows/pipeline_with_exit_handler.yaml (1)
88-91: Consistent with PR pattern.Same argument additions in identical locations.
Also applies to: 351-354
test_data/compiled-workflows/if_else_with_oneof_artifacts.yaml (1)
108-111: Consistent with PR pattern.Same argument additions in identical locations.
Also applies to: 345-348
test_data/compiled-workflows/arguments_parameters.yaml (1)
61-64: Consistent with PR pattern.Same argument additions in identical locations.
Also applies to: 266-269
test_data/compiled-workflows/embedded_artifact.yaml (1)
111-114: Consistent with PR pattern.Same argument additions in identical locations.
Also applies to: 340-343
test_data/compiled-workflows/if_elif_else_with_oneof_parameters.yaml (1)
116-119: Consistent with PR pattern.Same argument additions in identical locations.
Also applies to: 387-390
test_data/compiled-workflows/add_numbers.yaml (1)
70-73: Consistent with PR pattern across all 8 test workflow files.Same argument additions in identical locations. All test data fixtures have been systematically updated with the new ML pipeline server configuration parameters.
Also applies to: 275-278
test_data/compiled-workflows/container_with_artifact_output.yaml (2)
266-269: Same DNS format concern for system-dag-driver.The ML Pipeline server address here has the same potential incompleteness as noted in the system-container-driver template (line 62). Please ensure consistency with the metadata server address format on line 271.
61-64: DNS format inconsistency confirmed, but appears systematically intentional.The verification reveals that
ml-pipeline.kubeflowandmetadata-grpc-service.kubeflow.svc.cluster.localuse different DNS formats consistently across all ~160 compiled workflow test files. While the review comment correctly identifies the format inconsistency (short name vs. full FQDN), the uniformity suggests this is intentional configuration rather than an error.Both Kubernetes DNS name forms are valid within a cluster. However, for consistency and best practices, both services should ideally use the same format. The question of whether this needs correction depends on the actual Kubeflow deployment configuration and whether these services are intentionally configured with different accessibility scopes.
test_data/compiled-workflows/long-running.yaml (1)
61-64: Consistent with DNS format concern noted in other workflows.These additions follow the same pattern as other workflow files. Please apply the same verification regarding the ML Pipeline server address format (see comments on container_with_artifact_output.yaml).
Also applies to: 291-294
test_data/compiled-workflows/pipeline_with_metadata_fields.yaml (1)
101-104: Consistent with DNS format concern noted in other workflows.These additions follow the same pattern. Please apply the same verification regarding the ML Pipeline server address format.
Also applies to: 331-334
backend/src/apiserver/common/const.go (1)
87-90: LGTM! Clean constant definitions.The new service name constants are well-defined and align with Kubeflow naming conventions. These provide good defaults for the configurable service names introduced in this PR.
test_data/compiled-workflows/pipeline_in_pipeline_loaded_from_yaml.yaml (1)
92-95: Consistent with DNS format concern noted in other workflows.These additions follow the same pattern. Please apply the same verification regarding the ML Pipeline server address format.
Also applies to: 322-325
test_data/compiled-workflows/parameters_complex.yaml (1)
120-123: New ML pipeline server args are consistent with driver flags and config.The added
--ml_pipeline_server_address/--ml_pipeline_server_portentries look correct and are ordered consistently before the existing MLMD flags in both driver templates.Also applies to: 325-328
backend/src/v2/compiler/argocompiler/dag.go (1)
22-24: DAG driver now correctly sources ML pipeline & MLMD endpoints from config.Importing
configand wiring--ml_pipeline_server_address/portand--mlmd_server_address/portthroughGetMLPipelineServerConfig()andGetMetadataConfig()aligns the DAG driver template with the new centralized server configuration.Also applies to: 558-580
test_data/compiled-workflows/hello_world.yaml (1)
61-68: Workflow driver arguments match the new ML pipeline/MLMD flag scheme.Both container and DAG driver templates now include
ml_pipeline_server_*before the existingmlmd_server_*flags with consistent values; this matches the updated driver binary interface.Also applies to: 266-273
backend/src/v2/client_manager/client_manager.go (1)
31-39: ClientManager options and cache client wiring align with the new cache client API.Extending
Optionswith ML pipeline and MLMD endpoints and routing them throughinitCacheClienttocacheutils.NewClientis consistent and keeps initialization centralized alongside the existing metadata client.Also applies to: 64-88, 107-109
backend/src/v2/cacheutils/cache_test.go (1)
221-223: Tests correctly use the newNewClienthost/port signature.All three tests now construct the cache client with explicit ML pipeline address/port plus TLS config, matching the updated
NewClientAPI and the rest of the wiring in this PR.Also applies to: 342-344, 412-413
backend/src/apiserver/common/config.go (1)
25-44: New service-name config keys and getters are consistent with existing config patterns.
MLPipelineServiceName/MetadataServiceNameand their getters reuseGetStringConfigWithDefaultwith sensible defaults, matching how other config values are exposed and enabling env-based overrides for service discovery.Also applies to: 120-126
backend/src/v2/cmd/driver/main.go (1)
71-75: Driver correctly introduces ML pipeline server flags and passes them to the cache client.The new
ml_pipeline_server_*andmlmd_server_*flags align with the driver’s CLI usage in compiled workflows, andcacheutils.NewClientnow receives the ML pipeline address/port and cacheDisabled flag in one place, keeping connection configuration explicit.Also applies to: 185-197
test_data/compiled-workflows/if_elif_else_complex.yaml (1)
161-164: ML pipeline server args are wired consistently in this workflow as well.The added
ml_pipeline_server_address/ml_pipeline_server_portflags in both driver templates follow the same pattern as other workflows and match the driver binary’s expectations.Also applies to: 367-370
backend/src/v2/cmd/launcher-v2/main.go (4)
87-88: LGTM! Clean wiring of new configuration fields.The new ML pipeline server address and port fields are correctly propagated to
LauncherV2Options.
117-118: LGTM! Consistent with existing MLMD server configuration pattern.The new fields correctly mirror the pattern used for MLMD server configuration, maintaining consistency in the codebase.
43-44: Confirm default values match upstream KFP code.The ml-pipeline service in Kubeflow exposes ports 8888 and 8887, confirming port 8887 is a valid Kubeflow Pipelines port. The service name "ml-pipeline.kubeflow" corresponds to the ml-pipeline service in the kubeflow namespace and is standard Kubernetes DNS naming.
However, I could not locate the specific details of upstream KFP PR kubeflow#12463 to conclusively verify these defaults match that cherry-pick. Please manually confirm that the values in
backend/src/v2/cmd/launcher-v2/main.golines 43-44 match the source PR before merging.
31-52: Code passes linting standards.Golangci-lint verification completed with 0 issues. The code meets the repository's Go linting standards as required by the coding guidelines.
| func GetMLPipelineServerConfig() *ServerConfig { | ||
| return &ServerConfig{ | ||
| Address: common.GetMLPipelineServiceName() + "." + common.GetPodNamespace(), | ||
| Port: mlPipelineGrpcServicePort, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Incomplete Kubernetes service DNS name construction.
The constructed address common.GetMLPipelineServiceName() + "." + common.GetPodNamespace() produces an incomplete DNS name like ml-pipeline.kubeflow. The canonical Kubernetes service DNS format is <service>.<namespace>.svc.cluster.local.
Check whether this shortened format is intentional and functional in your environment, or if it should be updated to match the full DNS format used elsewhere (e.g., see how metadata server address is constructed).
🏁 Script executed:
#!/bin/bash
# Compare with metadata service address construction
rg -n "GetMetadataServiceName|metadata-grpc-service" backend/src/v2/metadata/ -A 3 -B 3
rg -n "svc\.cluster\.local" backend/src/v2/ -C 3Length of output: 887
🌐 Web query:
Does Kubernetes service DNS resolution work with shortened service.namespace format or does it require the full service.namespace.svc.cluster.local format?
💡 Result:
Shortened names work. Inside a Pod’s namespace you can use just "service"; from other namespaces "service.namespace" also resolves (or use the full FQDN "service.namespace.svc.cluster.local"). Resolution depends on the Pod’s DNS search path and ndots behavior; Windows pods have additional limitations (they generally need the namespace suffix or full FQDN). [1][2]
Sources:
[1] Kubernetes: DNS for Services and Pods.
[2] Kubernetes service-discovery / resolv.conf explanation (search suffixes and ndots).
Update ML pipeline service address to use full Kubernetes DNS format.
The verification confirms an actual inconsistency in your codebase. The metadata service uses the full FQDN format (service.namespace.svc.cluster.local), while the ML pipeline service uses the shortened format (service.namespace).
While shortened names like "service.namespace" do resolve in Kubernetes via DNS search paths, this approach is less portable and depends on DNS configuration. Windows pods generally need the namespace suffix or full FQDN, making the full format more reliable across environments.
Update backend/src/v2/config/env.go:196 to match the metadata service pattern:
Address: common.GetMLPipelineServiceName() + "." + common.GetPodNamespace() + ".svc.cluster.local",🤖 Prompt for AI Agents
In backend/src/v2/config/env.go around lines 195 to 200, the ML pipeline service
Address uses the shortened DNS form (service.namespace); update it to the full
Kubernetes DNS format to match the metadata service by appending
".svc.cluster.local" (i.e., construct Address using
common.GetMLPipelineServiceName() + "." + common.GetPodNamespace() +
".svc.cluster.local"), ensuring consistency and portability across environments.
| - --ml_pipeline_server_address | ||
| - ml-pipeline.kubeflow | ||
| - --ml_pipeline_server_port | ||
| - "8887" |
There was a problem hiding this comment.
🧩 Analysis chain
Good—test data updated consistently with new driver arguments.
The changes properly add --ml_pipeline_server_address and --ml_pipeline_server_port arguments to both driver templates (system-container-driver and system-dag-driver). The formatting, placement, and symmetry across both locations look correct.
However, please verify that:
- The KFP driver binary actually accepts these new command-line arguments and that the defaults provided here (
ml-pipeline.kubeflowand8887) are appropriate for the test environment. - Other compiled workflow test data files in the repository follow the same pattern and have been updated similarly.
You can run the following script to check if other workflow test files need the same updates:
Also applies to: 267-270
🏁 Script executed:
#!/bin/bash
# Description: Find other compiled workflow YAML files in test_data and check if they have the new ml_pipeline arguments
# Search for other workflow files that reference system-container-driver or system-dag-driver
fd -e yaml test_data/compiled-workflows/ -x grep -l "system-container-driver\|system-dag-driver" {} \;
# For each file found, check if it already has ml_pipeline_server_address
echo ""
echo "Checking for presence of ml_pipeline_server_address in driver templates:"
fd -e yaml test_data/compiled-workflows/ -x bash -c 'echo "File: {}"; grep -c "ml_pipeline_server_address" {} 2>/dev/null || echo "Not found"' \;Length of output: 917
🏁 Script executed:
#!/bin/bash
# Description: Find all YAML files in test_data/compiled-workflows and check for ml_pipeline arguments
echo "All YAML files in test_data/compiled-workflows:"
fd . test_data/compiled-workflows/ -e yaml
echo ""
echo "Checking for ml_pipeline_server_address in each file:"
fd . test_data/compiled-workflows/ -e yaml -x bash -c 'echo "File: {}"; if grep -q "ml_pipeline_server_address" {}; then echo "✓ Has ml_pipeline_server_address"; else echo "✗ Missing ml_pipeline_server_address"; fi' \;
echo ""
echo "Files that have system-container-driver or system-dag-driver but no ml_pipeline_server_address:"
fd . test_data/compiled-workflows/ -e yaml -x bash -c 'if (grep -q "system-container-driver\|system-dag-driver" {} 2>/dev/null) && ! grep -q "ml_pipeline_server_address" {} 2>/dev/null; then echo "{}"; fi' \;Length of output: 21303
🏁 Script executed:
#!/bin/bash
# Verify that the 3 files without ml_pipeline_server_address actually have driver templates
echo "Checking hello-world.yaml:"
grep -A5 "system-container-driver\|system-dag-driver" test_data/compiled-workflows/hello-world.yaml | head -20
echo ""
echo "Checking arguments-parameters.yaml:"
grep -A5 "system-container-driver\|system-dag-driver" test_data/compiled-workflows/arguments-parameters.yaml | head -20
echo ""
echo "Checking arguments.pipeline.yaml:"
grep -A5 "system-container-driver\|system-dag-driver" test_data/compiled-workflows/arguments.pipeline.yaml | head -20Length of output: 1628
Update three additional test data files to include the new ml_pipeline server arguments.
Verification found that three other compiled workflow test files have system-container-driver and system-dag-driver templates but are missing the --ml_pipeline_server_address and --ml_pipeline_server_port arguments added in this PR:
- test_data/compiled-workflows/hello-world.yaml
- test_data/compiled-workflows/arguments-parameters.yaml
- test_data/compiled-workflows/arguments.pipeline.yaml
These files should be updated with the same arguments in the same locations (before --mlmd_server_address) to maintain consistency across the test suite. The other 145 compiled workflow files already have these arguments.
🤖 Prompt for AI Agents
In test_data/compiled-workflows/container_with_concat_placeholder.yaml around
lines 62 to 65, the PR added --ml_pipeline_server_address and
--ml_pipeline_server_port arguments but three other compiled workflow test files
(test_data/compiled-workflows/hello-world.yaml,
test_data/compiled-workflows/arguments-parameters.yaml,
test_data/compiled-workflows/arguments.pipeline.yaml) are missing these args;
update those three files by inserting the two arguments
(--ml_pipeline_server_address <value> and --ml_pipeline_server_port "<port>") in
the same locations as in container_with_concat_placeholder.yaml, i.e., add both
arguments immediately before the existing --mlmd_server_address entry inside the
system-container-driver and system-dag-driver templates so the argument order
matches the rest of the test suite.
There was a problem hiding this comment.
@alyssacgoins Could you update the missing arguments in the test data files that coderabbit pointed out?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@VaniHaripriya Those test data files don't appear to be run in the workflow compiler test (compiler test passes locally and in CI) or any test currently running in CI - I'm hesitant to update a test file that my current changes aren't being tested on. What are your thoughts?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HumairAK The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 2de7aae
into
opendatahub-io:master
4 participants
Description of your changes:
Cherry-pick this commit from upstream: kubeflow#12463
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.