Support Volcano scheduler integration by enabling PodGroup creation for podTask in Propeller #6457
Conversation
… podTask in propeller Signed-off-by: zhuo-zhi <[email protected]>
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
|
Hi reviewers, could you please help me add the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6457 +/- ##
==========================================
+ Coverage 58.47% 58.50% +0.02%
==========================================
Files 940 940
Lines 71584 71658 +74
==========================================
+ Hits 41861 41920 +59
- Misses 26540 26551 +11
- Partials 3183 3187 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Without looking to deeply at this I feel that the plugin manager code probably shouldn't be doing exceptional work for a third party Kubernetes scheduler. My intuition is that you would want to create a new plugin for Volcano and configure your Propeller instance to use that as the default plugin. |
@Sovietaced Thanks for the review. All the changes introduced in this PR are gated behind a configuration flag, which is disabled by default. This ensures that the plugin manager will not perform this functionality unless explicitly enabled. This feature is primarily designed to support the current k8s Pod plugin, and more broadly, any future plugins that launch Pod resources. If I understand correctly, the current code structure assumes each plugin is responsible for managing only one k8s object. This assumption is why a new plugin doesn't work because additional logic is required in the plugin manager to create PodGroups before launching Pods. For other plugins such as TFJob, PyTorchJob, and MPIJob, their corresponding operators are already integrated with Volcano and can create PodGroups on their own. |
Signed-off-by: zhuo-zhi <[email protected]>
| } | ||
| } | ||
|
|
||
| err = backOffHandler.Handle(ctx, func() error { |
There was a problem hiding this comment.
As before, this error will still be handled by the logic a few lines below.
I understand. My main concern is that core flyte propeller internals shouldn't have references to third party elements like volcano. You're using a feature flag to branch the logic in the generic plugin manager code to do something very specific for volcano. I will wait for more of the Flyte core maintainers to look at this but I'm thinking it might make more sense to rearchitect Flyte to allow plugins to build multiple resources. |
Signed-off-by: zhuo-zhi <[email protected]>
I see your point about making the plugin manager code more generic. I'm not sure how much effort it would take, or whether there are concrete plans to rearchitect the Flyte plugins to support building multiple resources. In the meantime, would it be possible to include this change in an earlier release so users can opt in earlier if needed? It could also help validate the effectiveness of a potential future rearchitecture. More feedback from the Flyte core maintainers would also be greatly appreciated. |
Signed-off-by: zhuo-zhi <[email protected]>
@davidmirror-ops Thanks for the review. I've created a separate PR to document the preparation steps: unionai/unionai-docs#372 |
| return podRequestedResources | ||
| } | ||
|
|
||
| // getRequestResource will get the actual request resource. |
There was a problem hiding this comment.
I feel this module is not the right place to add this.
I agree with everything @Sovietaced said, I also feel we cannot have such specialized logic in the plugin manager itself, even with a feature flag :/ |
@fg91 @Sovietaced Thanks for the insights. I understand your concern. I’m wondering if there are any actionable steps you’d suggest I take to help move the PR forward? Rearchitecting Flyte to support plugins that build multiple resources appears to be a significant change, as it would likely require refactoring all existing plugins along with the plugin manager. If that’s the only viable path for integration, would it be possible for me to get some support on this? |
I would defer to the Union folks or people that understand the Propeller architecture deeply. |
|
I agree that re-architecting the plugin interface to allow for "extra resources" would be a bigger effort. I don't see a clean way to integrate the creation of resources like PodGroup into Flyte without this though. The adapted plugin interface could in theory look like this: type Plugin interface {
// Defines a func to create a query object (typically just object and type meta portions) that's used to query k8s
// resources.
BuildIdentityResource(ctx context.Context, taskCtx pluginsCore.TaskExecutionMetadata) (client.Object, error)
// Defines a func to create the full resource object that will be posted to k8s.
BuildResource(ctx context.Context, taskCtx pluginsCore.TaskExecutionContext) (client.Object, error)
// Analyses the k8s resource and reports the status as TaskPhase. This call is expected to be relatively fast,
// any operations that might take a long time (limits are configured system-wide) should be offloaded to the
// background.
GetTaskPhase(ctx context.Context, pluginContext PluginContext, resource client.Object) (pluginsCore.PhaseInfo, error)
// Properties desired by the plugin
GetProperties() PluginProperties
// New
// Defines a func to create query objects for the extra resources e.g. for aborting
BuildExtraIdentityResources(...) ([]client.Object, error)
// Defines a func to create the full extra resource objects that will be posted to k8s.
BuildExtraResources(...) ([]client.Object, error)
}Which extra resources would be created would depend on the user configuration for the plugin returned by Flytepropeller would not do any state monitoring for those extra resources, just create them along the creation of the main task resource. And it would delete them on abort. In theory we could omit the I don't think the existing plugins would create that much work because their respective All this is just brainstorming though, something like this would definitely require an RFC (I could help with the process) and it also definitely would need early input from Union folks like @eapolinario @wild-endeavor, @kumare3. Alternative approach: If your main goal was not officially integrating Flyte with Volcano but you/your organization being able to use Flyte pod tasks with Volcano, I think a much lower hanging fruit (albeit not an official integration into flyte) would be to handle the PodGroup creation in a (mutating) admission webhook that gets triggered when flytepropeller creates the pods. You could deploy such a service along side Flyte. I've done something like this in the past for other resources. |
|
Cc @troychiu this was the one |
5 participants
Why are the changes needed?
Support Volcano scheduler integration by enabling PodGroup creation for podTask in Propeller. Volcano is a Kubernetes-native batch scheduler optimized for high-performance, AI/ML, and big data workloads.
Before this change, all podTasks associated with a FlyteWorkflow shared a single PodGroup, which was automatically created just once by the Volcano controller when the first podTask was launched. Subsequent podTasks reused this PodGroup, which introduced limitations. Specifically, different podTasks could not be scheduled with different Volcano queues or job priorities, since the shared PodGroup enforced a single queue and priority for all.
This change enables Propeller to create individual Volcano PodGroups for each podTask, allowing them to specify distinct queues and job priorities for scheduling.
What changes were proposed in this pull request?
enable-create-pod-group-for-podin the config to control whether this feature is enabled. The default value is false to ensure backward compatibility.How was this patch tested?
Unit tests added
Used internally at Linkedin.
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This pull request enhances the Volcano scheduler support by allowing the creation of individual PodGroups for each podTask in Propeller, improving scheduling flexibility with distinct queues and job priorities. A new configuration option ensures backward compatibility, and relevant unit tests have been added to validate and verify the new functionality.