feat(localmodel): replace hostPath with PVC-backed volumes in LocalModel node agent - #3
Draft
tmvfb wants to merge 13 commits into
Draft
feat(localmodel): replace hostPath with PVC-backed volumes in LocalModel node agent#3tmvfb wants to merge 13 commits into
tmvfb wants to merge 13 commits into
Conversation
…volumes Implement a new controller that watches LocalModelNodeGroup CRs and dynamically creates a PV, PVC, and DaemonSet per node group. The DaemonSet uses PVC-backed volumes instead of raw hostPath, eliminating Pod Security Standards (PSS) violations. - New LocalModelNodeGroupReconciler in pkg/controller/v1alpha1/localmodelnodegroup/ - Add 6 agent config fields to LocalModelConfig (image, pull policy, resources) - Wire up the new controller in cmd/localmodel/main.go - PSS-compliant security context on agent containers - Finalizer for orderly cleanup on CR deletion - Semantic equality checks to avoid unnecessary DaemonSet updates - Node affinity derived from PV spec (no kserve/localmodel label needed) Closes: #2
…arts Remove the static localmodelnode-agent DaemonSet that was deployed via kustomize and Helm. The new LocalModelNodeGroup controller now creates DaemonSets dynamically per node group. - Delete config/localmodelnodes/manager.yaml (static DaemonSet) - Delete config/localmodelnodes/localmodelnode_agent_image_patch.yaml - Delete charts/kserve-localmodel-resources/files/daemonset-patch.yaml - Simplify config/localmodelnodes/kustomization.yaml to only reference RBAC - Update config/rbac/localmodel/role.yaml with DaemonSet/PV/PVC/finalizer perms - Update Helm chart RBAC, values, and configmap templates for new agent fields - Update Makefile RBAC generation path to include localmodelnodegroup
Remove all references to the static localmodelnode-agent DaemonSet from test/development overlays, CI workflows, and developer scripts. - Remove localmodelnode_agent_image_patch.yaml from test overlay - Remove localmodelnode_image_patch.yaml reference from development overlay - Remove 'Create model root directory' step from e2e-test.yml - Remove 'Label worker nodes for modelcache' step from e2e-test.yml - Remove sed line for agent patch from update-test-overlays.sh - Remove ko resolve block for DaemonSet from hack/image_patch_dev.sh
Add Ginkgo/Gomega envtest-based tests for the new controller: - suite_test.go: test suite setup with envtest - controller_test.go: tests for PV/PVC/DaemonSet creation, label verification, PSS-compliant security context, PVC volume usage, node affinity propagation, finalizer lifecycle, and deletion cleanup
There was a problem hiding this comment.
Pull request overview
Replaces the static LocalModel node agent DaemonSet (previously using hostPath) with a new LocalModelNodeGroup controller that provisions a PV + PVC + DaemonSet per node group, and wires configuration/RBAC/chart changes to support the new model.
Changes:
- Added
LocalModelNodeGroupReconcilerplus envtest coverage to create PV/PVC/DaemonSet per node group. - Extended
LocalModelConfig(ConfigMap-backed) with agent image/pull policy/resource fields and added defaults in manifests/charts. - Removed static localmodelnode-agent DaemonSet + related overlays/CI/dev script patching; updated RBAC and build generation.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
test/scripts/gh-actions/update-test-overlays.sh |
Stops patching a now-removed static localmodel node agent image patch in GH Actions. |
pkg/controller/v1alpha1/localmodelnodegroup/suite_test.go |
New Ginkgo envtest suite bootstrap for the LocalModelNodeGroup controller tests. |
pkg/controller/v1alpha1/localmodelnodegroup/controller_test.go |
New envtest-based tests for PV/PVC/DaemonSet creation and finalizer behavior. |
pkg/controller/v1alpha1/localmodelnodegroup/controller.go |
New controller that reconciles PV/PVC and a PVC-backed DaemonSet per LocalModelNodeGroup. |
pkg/apis/serving/v1beta1/configmap.go |
Adds new LocalModel agent config fields and validates image pull policy. |
hack/image_patch_dev.sh |
Removes localmodel node agent DaemonSet image patching; normalizes heredoc formatting. |
config/rbac/localmodel/role.yaml |
Grants localmodel manager permissions for daemonsets and localmodelnodegroup patch/update + finalizers. |
config/overlays/test/localmodelnode_agent_image_patch.yaml |
Deletes obsolete static DaemonSet image patch. |
config/overlays/test/kustomization.yaml |
Removes reference to deleted localmodel node agent patch. |
config/overlays/development/kustomization.yaml |
Removes reference to deleted localmodelnode DaemonSet patch. |
config/localmodelnodes/manager.yaml |
Deletes the static localmodel node agent DaemonSet manifest. |
config/localmodelnodes/localmodelnode_agent_image_patch.yaml |
Deletes obsolete DaemonSet image patch. |
config/localmodelnodes/kustomization.yaml |
Removes static DaemonSet resource and patch wiring (leaving RBAC resource). |
config/configmap/inferenceservice.yaml |
Adds default values for new LocalModel agent config fields. |
cmd/localmodel/main.go |
Registers the new LocalModelNodeGroup controller with the localmodel manager. |
charts/kserve-resources/values.yaml |
Updates Helm values: removes old scheduling/hostPath knobs; adds agent pull policy and resources. |
charts/kserve-resources/files/common/configmap.yaml |
Adds new LocalModel agent config defaults into the chart’s configmap content. |
charts/kserve-resources/files/common/configmap-patch.yaml |
Templates new LocalModel agent config fields from Helm values into the configmap. |
charts/kserve-localmodel-resources/values.yaml |
Removes values for the deleted static localmodelnode DaemonSet. |
charts/kserve-localmodel-resources/files/resources.yaml |
Removes static DaemonSet from rendered resources; extends RBAC for new controller needs. |
charts/kserve-localmodel-resources/files/daemonset-patch.yaml |
Deletes obsolete DaemonSet patch template. |
Makefile |
Includes localmodelnodegroup controller path in RBAC generation for the localmodel manager role. |
.github/workflows/e2e-test.yml |
Removes node labeling and /models directory setup steps tied to the old hostPath DaemonSet. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…up controller - Validate agent CPU/memory resource quantities in NewLocalModelConfig using resource.ParseQuantity, following the GetStorageInitializerConfigs pattern in the same file - Replace resource.MustParse with resource.ParseQuantity in createLocalModelAgentDaemonSet to avoid panics on invalid config - Guard against nil NodeAffinity/Required before dereferencing PV spec - Add Owns(PVC) and Owns(PV) to SetupWithManager for self-healing on accidental deletion, consistent with the localmodel controller - Pass explicit timeout/interval to the initializeManager Eventually block
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+175
to
+192
| // Parse resource quantities safely instead of using MustParse | ||
| cpuRequest, err := resource.ParseQuantity(localModelConfig.LocalModelAgentCpuRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse localModelAgentCpuRequest %q: %w", localModelConfig.LocalModelAgentCpuRequest, err) | ||
| } | ||
| memoryRequest, err := resource.ParseQuantity(localModelConfig.LocalModelAgentMemoryRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse localModelAgentMemoryRequest %q: %w", localModelConfig.LocalModelAgentMemoryRequest, err) | ||
| } | ||
| cpuLimit, err := resource.ParseQuantity(localModelConfig.LocalModelAgentCpuLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse localModelAgentCpuLimit %q: %w", localModelConfig.LocalModelAgentCpuLimit, err) | ||
| } | ||
| memoryLimit, err := resource.ParseQuantity(localModelConfig.LocalModelAgentMemoryLimit) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse localModelAgentMemoryLimit %q: %w", localModelConfig.LocalModelAgentMemoryLimit, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
createLocalModelAgentDaemonSet unconditionally parses LocalModelAgentCpuRequest/MemoryRequest/CpuLimit/MemoryLimit and uses LocalModelAgentImage from LocalModelConfig. If these fields are omitted (e.g., older inferenceservice-config during upgrade, or a custom config that didn’t add the new keys), reconciliation will fail on empty strings (quantity parse error) or produce an invalid DaemonSet (empty image), leaving node groups permanently unreconciled. Consider applying defaults in NewLocalModelConfig (matching the manifest defaults) and/or only parsing/applying resources when the fields are non-empty (with a clear error if image is empty).
Suggested change
| // Parse resource quantities safely instead of using MustParse | |
| cpuRequest, err := resource.ParseQuantity(localModelConfig.LocalModelAgentCpuRequest) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentCpuRequest %q: %w", localModelConfig.LocalModelAgentCpuRequest, err) | |
| } | |
| memoryRequest, err := resource.ParseQuantity(localModelConfig.LocalModelAgentMemoryRequest) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentMemoryRequest %q: %w", localModelConfig.LocalModelAgentMemoryRequest, err) | |
| } | |
| cpuLimit, err := resource.ParseQuantity(localModelConfig.LocalModelAgentCpuLimit) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentCpuLimit %q: %w", localModelConfig.LocalModelAgentCpuLimit, err) | |
| } | |
| memoryLimit, err := resource.ParseQuantity(localModelConfig.LocalModelAgentMemoryLimit) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentMemoryLimit %q: %w", localModelConfig.LocalModelAgentMemoryLimit, err) | |
| } | |
| // Parse resource quantities conditionally; empty strings are treated as "no value" | |
| var ( | |
| cpuRequest resource.Quantity | |
| memoryRequest resource.Quantity | |
| cpuLimit resource.Quantity | |
| memoryLimit resource.Quantity | |
| err error | |
| ) | |
| if localModelConfig.LocalModelAgentCpuRequest != "" { | |
| cpuRequest, err = resource.ParseQuantity(localModelConfig.LocalModelAgentCpuRequest) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentCpuRequest %q: %w", localModelConfig.LocalModelAgentCpuRequest, err) | |
| } | |
| } | |
| if localModelConfig.LocalModelAgentMemoryRequest != "" { | |
| memoryRequest, err = resource.ParseQuantity(localModelConfig.LocalModelAgentMemoryRequest) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentMemoryRequest %q: %w", localModelConfig.LocalModelAgentMemoryRequest, err) | |
| } | |
| } | |
| if localModelConfig.LocalModelAgentCpuLimit != "" { | |
| cpuLimit, err = resource.ParseQuantity(localModelConfig.LocalModelAgentCpuLimit) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentCpuLimit %q: %w", localModelConfig.LocalModelAgentCpuLimit, err) | |
| } | |
| } | |
| if localModelConfig.LocalModelAgentMemoryLimit != "" { | |
| memoryLimit, err = resource.ParseQuantity(localModelConfig.LocalModelAgentMemoryLimit) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to parse localModelAgentMemoryLimit %q: %w", localModelConfig.LocalModelAgentMemoryLimit, err) | |
| } | |
| } | |
| // Ensure a valid container image is configured | |
| if localModelConfig.LocalModelAgentImage == "" { | |
| return nil, fmt.Errorf("localModelAgentImage in LocalModelConfig must not be empty") | |
| } |
Comment on lines
+372
to
+398
| // Step 1 - Checks if the CR is in the deletion process | ||
| if nodeGroup.DeletionTimestamp.IsZero() { | ||
| // The object is not being deleted, so if it does not have our finalizer, | ||
| // then lets add the finalizer and update the object. This is equivalent | ||
| // registering our finalizer. | ||
| if !utils.Includes(nodeGroup.GetFinalizers(), finalizerName) { | ||
| patch := client.MergeFrom(nodeGroup.DeepCopy()) | ||
| nodeGroup.SetFinalizers(append(nodeGroup.GetFinalizers(), finalizerName)) | ||
| if err := r.Patch(ctx, nodeGroup, patch); err != nil { | ||
| r.Log.Error(err, "Unable to patch LocalModelNodeGroup with finalizer") | ||
| return ctrl.Result{}, err | ||
| } | ||
| } | ||
| } else { | ||
| // The object is being deleted, so if it has our finalizer, then lets | ||
| // remove it and update the object. This is equivalent to unregistering | ||
| // our finalizer. | ||
| if utils.Includes(nodeGroup.GetFinalizers(), finalizerName) { | ||
| patch := client.MergeFrom(nodeGroup.DeepCopy()) | ||
| nodeGroup.SetFinalizers(utils.RemoveString(nodeGroup.GetFinalizers(), finalizerName)) | ||
| if err := r.Patch(ctx, nodeGroup, patch); err != nil { | ||
| r.Log.Error(err, "Unable to patch LocalModelNodeGroup without finalizer") | ||
| return ctrl.Result{}, err | ||
| } | ||
| } | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
The reconciler adds a finalizer on create, but on deletion it immediately removes the finalizer without performing any cleanup or ordering (no explicit deletion/waiting for PV/PVC/DaemonSet). As written, the finalizer is effectively a no-op and doesn’t provide the “orderly deletion” described in the PR summary. Either implement the intended cleanup sequence before removing the finalizer, or remove the finalizer entirely and rely on ownerReferences/GC.
Add the 6 new localModelAgent config fields to all test ConfigMap fixtures in localmodel/controller_test.go and localmodelnode/controller_test.go. These fields are now validated by NewLocalModelConfig() and their absence causes test failures.
…stale TODOs Replace local MountPath constant with constants.DefaultModelLocalMountPath to avoid duplication. Remove 3 stale TODO comments that were leftover from the original upstream review.
Port unit tests from upstream PR kserve#4431 for createPV, createPVC, createLocalModelAgentDaemonSet, and reconcileDaemonSet. Adapted to handle our error-returning createLocalModelAgentDaemonSet signature (uses ParseQuantity instead of MustParse). Added an extra test case for invalid resource quantities.
Add tests for ImagePullPolicy validation (Always, IfNotPresent, Never, invalid, empty, missing) and resource quantity validation (valid values, invalid CPU request, invalid memory limit, empty values).
Add the 6 localModelAgent config fields to both the _example and functional localModel sections in the test overlay ConfigMap. Tests running against this overlay will now get a complete config that passes NewLocalModelConfig() validation.
The controller-managed DaemonSets use app.kubernetes.io/component labels instead of control-plane labels. Update the selector from control-plane=kserve-localmodelnode-agent to app.kubernetes.io/component=localmodelnode-agent so CI correctly finds agent pods.
Add ko_resolve_localmodelnode_agent file for the development overlay and update image_patch_dev.sh to resolve the localmodel agent image and patch it into the localModel ConfigMap section during local development.
…Config agent fields Regenerate openapi_generated.go, swagger.json, and the Python SDK model to reflect the 6 new LocalModelConfig fields: localModelAgentImage, localModelAgentImagePullPolicy, localModelAgentCpuRequest, localModelAgentMemoryRequest, localModelAgentCpuLimit, and localModelAgentMemoryLimit.
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.
Summary
Replace the static localmodelnode-agent DaemonSet (which used raw
hostPathvolumes) with a newLocalModelNodeGroupcontroller that dynamically creates a PV, PVC, and DaemonSet per node group with PVC-backed volumes.LocalModelNodeGroupReconcilerwatchesLocalModelNodeGroupCRs and reconciles PV + PVC + DaemonSet per node groupLocalModelConfigConfigMap (6 new fields)PersistentVolumeSpec.NodeAffinity— nokserve/localmodel=workerlabel neededLocalModelNodeGroupis removedChanges
New files
pkg/controller/v1alpha1/localmodelnodegroup/controller.go— the controllerpkg/controller/v1alpha1/localmodelnodegroup/suite_test.go— test suitepkg/controller/v1alpha1/localmodelnodegroup/controller_test.go— envtest-based testsModified files
pkg/apis/serving/v1beta1/configmap.go— 6 newLocalModelConfigfieldsconfig/configmap/inferenceservice.yaml— defaults for new fieldscmd/localmodel/main.go— register new reconcilerconfig/rbac/localmodel/role.yaml— DaemonSet, PV, PVC, finalizer permsMakefile— RBAC generation pathkserve-resources,kserve-localmodel-resources) — values, configmap, RBACDeleted files
config/localmodelnodes/manager.yaml— static DaemonSetconfig/localmodelnodes/localmodelnode_agent_image_patch.yamlcharts/kserve-localmodel-resources/files/daemonset-patch.yamlconfig/overlays/test/localmodelnode_agent_image_patch.yamlCloses #2