fix: decouple pull and mount from kubelet RPC deadlines#36
Open
tjandy98 wants to merge 2 commits intomodelpack:mainfrom
Open
fix: decouple pull and mount from kubelet RPC deadlines#36tjandy98 wants to merge 2 commits intomodelpack:mainfrom
tjandy98 wants to merge 2 commits intomodelpack:mainfrom
Conversation
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
Signed-off-by: tjandy98 <3953059+tjandy98@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request decouples model pulling and mounting operations from the parent context's deadline to prevent interruptions caused by kubelet timeouts. Specifically, it introduces a 30-second timeout for mounting in node_static_inline.go and decouples the pull operation in worker.go. Feedback was provided regarding a potential context leak in worker.go due to a missing call to the cancellation function.
| // when kubelet times out and retries | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithCancel(ctx) | ||
| ctx, cancel = context.WithCancel(context.WithoutCancel(ctx)) |
There was a problem hiding this comment.
The cancel function returned by context.WithCancel should be called to release resources as soon as the operation is complete. Since this context is decoupled from the parent and has no deadline, failing to call cancel will result in a context leak until the garbage collector cleans it up. Adding a defer cancel() ensures that resources are released regardless of how the function exits.
Suggested change
| ctx, cancel = context.WithCancel(context.WithoutCancel(ctx)) | |
| ctx, cancel = context.WithCancel(context.WithoutCancel(ctx)) | |
| defer cancel() |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📊 Code Coverage Report
📦 Per-package breakdown |
imeoer
reviewed
May 7, 2026
Collaborator
imeoer
left a comment
imeoer
left a comment
There was a problem hiding this comment.
Thanks for this PR, but it introduces an issue: if pulling model is no longer subject to kubelet’s 2min mount volume timeout, the model volume directory may be mounted into the pod prematurely, before the model weight files are fully downloaded.
| // when kubelet times out and retries | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithCancel(ctx) | ||
| ctx, cancel = context.WithCancel(context.WithoutCancel(ctx)) |
Collaborator
There was a problem hiding this comment.
Thanks for the PR! Should we only use WithoutCancel for the s.worker.PullModel of nodePublishVolumeStaticInlineVolume?
Author
Thanks for the feedback. From my understanding mount only runs if |
Collaborator
Okay, so will the pod likely see an empty directory initially and then see the model contents later? If so, we need a clear mechanism for the application process in the pod to know when the model directory is ready, but this is no different from using dynamic API mounting, the business logic must explicitly adapt: |
Author
The pod status will not be marked as ready as long as all the layers are not pulled completely yet |
Collaborator
Do you mean the inference engine process will initially fail and then eventually reach the ready state via the container’s restart backoff retry mechanism? That seems viable, could we control this behavior via a parameter, e.g., |
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
2 participants
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.
The CSI driver's NodePublishVolume was canceling in-flight model pulls whenever kubelet's RPC deadline expired, making it impossible to mount any model large enough to exceed that budget.