Restore accelerated-inputs volume mount in v2 flytek8s pod construction - #7790
Open
pvditt wants to merge 1 commit into
Open
Restore accelerated-inputs volume mount in v2 flytek8s pod construction#7790pvditt wants to merge 1 commit into
pvditt wants to merge 1 commit into
Conversation
The accelerated-inputs pod wiring was commented out when flytek8s was ported to v2 because it read config from the v1 propeller config package, which v2 does not depend on. Give the config a v2-native home on K8sPluginConfig (plugins.k8s.accelerated-inputs, same fields and defaults as the v1 propeller section) and restore ApplyAcceleratedInputsSpec in ApplyFlytePodConfiguration. The function matches v1 behavior for freshly built pod specs, with one addition: the volume and the primary-container mount are checked independently before adding, so callers applying it to pre-populated pod templates (fasttask environment builders) are idempotent and fill in whichever piece is missing. Flag entries are hand-added in the generated pflags files following the generator's output format, since the pflags tool is not vendored here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsS7fjndEwkicFKLcguRpi Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores “accelerated inputs” support in the v2 flytek8s pod construction path by reintroducing the hostPath volume + primary-container mount needed for node-local dataset access, and wiring the feature to the v2 K8sPluginConfig config surface (including flags and tests).
Changes:
- Gate accelerated-inputs injection on
config.GetK8sPluginConfig().AcceleratedInputs.Enabledand apply the mount/volume to the primary container. - Introduce
K8sPluginConfig.AcceleratedInputs(with defaults) and add corresponding pflags + flag round-trip tests. - Add/restore unit tests covering both
ToK8sPodSpecbehavior andApplyAcceleratedInputsSpecidempotency.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go | Restores accelerated-inputs hostPath volume + primary mount injection during pod construction. |
| flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper_test.go | Restores/extends tests for accelerated-inputs volume/mount behavior and idempotent application. |
| flyteplugins/go/tasks/pluginmachinery/flytek8s/config/k8spluginconfig_flags.go | Adds CLI flags for the new accelerated-inputs config fields. |
| flyteplugins/go/tasks/pluginmachinery/flytek8s/config/k8spluginconfig_flags_test.go | Adds flag round-trip tests for accelerated-inputs entries. |
| flyteplugins/go/tasks/pluginmachinery/flytek8s/config/config.go | Adds AcceleratedInputs config struct and default values under K8sPluginConfig. |
Files not reviewed (2)
- flyteplugins/go/tasks/pluginmachinery/flytek8s/config/k8spluginconfig_flags.go: Generated file
- flyteplugins/go/tasks/pluginmachinery/flytek8s/config/k8spluginconfig_flags_test.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1174
to
+1187
| hasMount := false | ||
| for _, mount := range cont.VolumeMounts { | ||
| if mount.Name == acceleratedInputsVolumeName { | ||
| hasMount = true | ||
| break | ||
| } | ||
| } | ||
| if !hasMount { | ||
| spec.Containers[i].VolumeMounts = append(cont.VolumeMounts, v1.VolumeMount{ | ||
| Name: acceleratedInputsVolumeName, | ||
| ReadOnly: true, | ||
| MountPath: cfg.LocalPathPrefix, | ||
| }) | ||
| } |
Comment on lines
+318
to
+321
| Enabled bool `json:"enabled" pflag:",Enabled accelerated inputs feature which overwrites remote artifacts path to local disk paths"` | ||
| RemotePathPrefix string `json:"remote-path-prefix" pflag:",Remote path prefix that should be replaced with local path prefix"` | ||
| LocalPathPrefix string `json:"local-path-prefix" pflag:",Path to locally mounted directory k8s pod"` | ||
| VolumePath string `json:"volume-path" pflag:",Path to locally mounted directory on k8s host node"` |
Comment on lines
+1151
to
+1169
| hasVolume := false | ||
| for _, vol := range spec.Volumes { | ||
| if vol.Name == acceleratedInputsVolumeName { | ||
| hasVolume = true | ||
| break | ||
| } | ||
| } | ||
| if !hasVolume { | ||
| hostPathType := v1.HostPathDirectory | ||
| spec.Volumes = append(spec.Volumes, v1.Volume{ | ||
| Name: acceleratedInputsVolumeName, | ||
| VolumeSource: v1.VolumeSource{ | ||
| HostPath: &v1.HostPathVolumeSource{ | ||
| Path: cfg.VolumePath, | ||
| Type: &hostPathType, | ||
| }, | ||
| }, | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Accelerated inputs (node-local dataset mirror) was half-wired in v2: leaseworker injects the
_F_PATH_REWRITEenv var telling the SDK to read inputs locally, but the hostPath volume mount that makes the local path exist was commented out (TODO @pvditt) when flytek8s was ported — so the SDK's mount check always failed and every task silently fell back to S3. This restores the mount.What
flytepropeller/pkg/controller/config, a v1 dependency v2 deliberately severed. The config now lives onK8sPluginConfig(plugins.k8s.accelerated-inputs) with the same four fields and defaults as the v1 propeller section, so a working v1 config block ports value-for-value.ApplyAcceleratedInputsSpecrestored inApplyFlytePodConfiguration: read-only mount of theunion-persistent-data-volumehostPath into the primary container, gated onEnabled.Testing
AcceleratedInputsEnabledtest throughToK8sPodSpec(volume, mount path, read-only).TestApplyAcceleratedInputsSpec: pre-existing volume without a primary mount gets the mount added without duplicating the volume; repeated application stays at one volume/one mount.Consumer-side wiring (leaseworker feeds this config; fasttask builder injection) lands in the cloud repo.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MsS7fjndEwkicFKLcguRpi