feat: add framework-level validation for conflicting env vars across …

[krishdef7]

What this PR does / why we need it

Adds framework-level validation to detect conflicts between environment variables managed by different runtime plugins (MPI, Torch), and between plugin-reserved envs and user-provided envs in a TrainJob.

Changes

• Add MPIRunReservedEnvNames constant for OpenMPI reserved env vars
• Add user-env conflict validation to MPI plugin (mirrors existing Torch pattern)
• Add EnvVarsReserverPlugin interface for plugins to expose reserved env var names
• Implement ReservedEnvVarNames() on MPI and Torch plugins
• Add cross-plugin env conflict detection in RunCustomValidationPlugins
• Add unit tests for MPI env validation

Which issue(s) this PR fixes

Closes #3147

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jeffwan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

🎉 Welcome to the Kubeflow Trainer! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
• Our team will review your PR soon! cc @kubeflow/kubeflow-trainer-team

Join the community:

• Slack: Join our #kubeflow-trainer Slack channel.
• Meetings: Attend the Kubeflow AutoML and Training Working Group bi-weekly meetings.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[Copilot]

Pull request overview

This PR adds framework-level validation to detect conflicts between environment variables managed by different runtime plugins (MPI, Torch) and between plugin-reserved envs and user-provided envs in a TrainJob.

Changes:

• Introduces EnvVarsReserverPlugin interface for plugins to expose reserved environment variable names
• Implements the interface on MPI and Torch plugins to return their respective reserved env var sets
• Adds cross-plugin env conflict detection in the framework's validation logic
• Adds user-env conflict validation to the MPI plugin (mirroring existing Torch pattern)
• Adds comprehensive test coverage for MPI env validation

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/runtime/framework/interface.go	Defines new EnvVarsReserverPlugin interface for exposing reserved env var names
pkg/runtime/framework/core/framework.go	Implements cross-plugin env conflict detection in RunCustomValidationPlugins
pkg/runtime/framework/plugins/mpi/mpi.go	Implements EnvVarsReserverPlugin and adds user-env validation
pkg/runtime/framework/plugins/torch/torch.go	Implements EnvVarsReserverPlugin interface
pkg/constants/constants.go	Defines MPIRunReservedEnvNames set with OpenMPI reserved env vars
pkg/runtime/framework/plugins/mpi/mpi_test.go	Adds tests for MPI reserved env validation

[krishdef7]

Hi @jinchihe @kuizhiqing,

Adding a quick concrete example to clarify the validation:

If a user configures:

• MPI plugin (reserves OMPI_* envs)
• Torch plugin
• and manually sets OMPI_MCA_orte_default_hostfile in spec.trainer.env

This currently passes validation but can lead to runtime issues.

With this PR, validation fails early with a clear error indicating the conflicting env.

Happy to adjust the validation scope or placement if needed.

[krishdef7]

Hi @jinchihe @kuizhiqing, happy to make any changes based on your feedback. Let me know if the approach or placement needs adjustment.

[krishdef7]

@jinchihe @kuizhiqing

Addressed Copilot feedback and added concrete MPI/Torch conflict example above.

Let me know if any changes are needed, otherwise happy for /lgtm.

[krishdef7]

@jinchihe @kuizhiqing

Rebased onto latest master and resolved merge conflicts.
Updated MPI validation logic to align with the new NumProcPerNode type.

CI checks are passing. Let me know if any adjustments are needed.

[krishdef7]

@jinchihe @kuizhiqing, Rebased to fix DCO sign-off and removed an accidentally included commit from another branch. The PR now contains only the env conflict validation changes across 6 files. DCO is green. Happy for review when you get a chance

[krishdef7]

@andreyvelich @akshaychitneni, this PR implements the env conflict validation from the TODO you left in #2308. Cleaned up a dead NumProcPerNode validation block in this latest push. All CI passing, happy for review when you get a chance.