Adds shared memory into extended resources #6193
Conversation
Signed-off-by: Thomas J. Fan <[email protected]>
Code Review Agent Run #6a4c77Actionable Suggestions - 6
Additional Suggestions - 3
Review Details
AI Code Review powered by |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6193 +/- ##
==========================================
+ Coverage 36.85% 36.86% +0.01%
==========================================
Files 1318 1318
Lines 134543 134647 +104
==========================================
+ Hits 49590 49644 +54
- Misses 80632 80681 +49
- Partials 4321 4322 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Thomas J. Fan <[email protected]>
Changelist by BitoThis pull request implements the following key changes.
|
| var quantity resource.Quantity | ||
| if test.sharedVolume.GetSizeLimit() != "" { | ||
| quantity, err = resource.ParseQuantity(test.sharedVolume.GetSizeLimit()) | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
Consider initializing quantity to a zero value before the conditional block. Currently if GetSizeLimit() returns an empty string, quantity remains uninitialized when used in the assertion.
Code suggestion
Check the AI-generated fix before applying
| var quantity resource.Quantity | |
| if test.sharedVolume.GetSizeLimit() != "" { | |
| quantity, err = resource.ParseQuantity(test.sharedVolume.GetSizeLimit()) | |
| assert.NoError(t, err) | |
| } | |
| var quantity resource.Quantity | |
| quantity = resource.Quantity{} | |
| if test.sharedVolume.GetSizeLimit() != "" { | |
| quantity, err = resource.ParseQuantity(test.sharedVolume.GetSizeLimit()) | |
| assert.NoError(t, err) | |
| } |
Code Review Run #6a4c77
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| // Size limit for shared memory. If not set, then the shared memory is equal | ||
| // to the allocated memory. | ||
| // +optional | ||
| string size_limit = 3; |
There was a problem hiding this comment.
Consider using a more specific type like k8s.io/apimachinery/pkg/api/resource.Quantity for size_limit instead of string to ensure proper validation of memory size values.
Code suggestion
Check the AI-generated fix before applying
| string size_limit = 3; | |
| k8s.io.apimachinery.pkg.api.resource.Quantity size_limit = 3; |
Code Review Run #6a4c77
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| if extendedResources.GetSharedMemory() != nil { | ||
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) |
There was a problem hiding this comment.
Consider adding validation for SharedMemory fields before applying the override. The MountPath and MountName fields should be validated to ensure they contain valid values.
Code suggestion
Check the AI-generated fix before applying
| if extendedResources.GetSharedMemory() != nil { | |
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) | |
| if extendedResources.GetSharedMemory() != nil { | |
| shm := extendedResources.GetSharedMemory() | |
| if shm.MountPath == "" || shm.MountName == "" { | |
| return nil, nil, fmt.Errorf("shared memory mount path and name must be specified") | |
| } | |
| if !strings.HasPrefix(shm.MountPath, "/") { | |
| return nil, nil, fmt.Errorf("shared memory mount path must be absolute") | |
| } | |
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) |
Code Review Run #6a4c77
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| if extendedResources.GetSharedMemory() != nil { | ||
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) | ||
| if err != nil { | ||
| return nil, nil, err |
There was a problem hiding this comment.
Consider checking if primaryContainerName is empty before using it in ApplySharedMemory(). An empty container name could cause issues with shared memory configuration.
Code suggestion
Check the AI-generated fix before applying
| if extendedResources.GetSharedMemory() != nil { | |
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) | |
| if err != nil { | |
| return nil, nil, err | |
| if extendedResources.GetSharedMemory() != nil { | |
| if primaryContainerName == "" { | |
| return nil, nil, fmt.Errorf("primary container name cannot be empty when configuring shared memory") | |
| } | |
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) | |
| if err != nil { | |
| return nil, nil, err |
Code Review Run #6a4c77
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| @@ -429,6 +490,14 @@ | |||
| ApplyGPUNodeSelectors(podSpec, extendedResources.GetGpuAccelerator()) | |||
| } | |||
|
|
|||
| // Shared memory volume | |||
| if extendedResources.GetSharedMemory() != nil { | |||
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) | |||
There was a problem hiding this comment.
Consider checking the return value from ApplySharedMemory() before proceeding. The error handling could be more explicit.
Code suggestion
Check the AI-generated fix before applying
| err = ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()) | |
| if err := ApplySharedMemory(podSpec, primaryContainerName, extendedResources.GetSharedMemory()); err != nil { | |
| return nil, nil, fmt.Errorf("failed to apply shared memory: %w", err) | |
| } |
Code Review Run #6a4c77
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Code Review Agent Run #5bb8beActionable Suggestions - 0Review Details
AI Code Review powered by |
…d_memory_extended_sources_v2 Signed-off-by: Thomas J. Fan <[email protected]>
Signed-off-by: Thomas J. Fan <[email protected]>
Code Review Agent Run Status
|
Tracking issue
Towards #6142
Why are the changes needed?
This PR adds shared memory as an extend resource, that is made available through
@task(shared_memory). For the simple case, you can have@task(shared_memory=True), which means: "memory backed volumes are sized to node allocatable memory". Otherwise, you can setshared_memory="2Gi"to specify the value.What changes were proposed in this pull request?
This PR adds
shared_memoryto the IDL and the implementation to get it to work.How was this patch tested?
Unit tests were added to this PR and tested with flytekit changes:
Then I used kubectl to make sure that the pod spec was correct.
Summary by Bito
This PR implements shared memory support in Flyte's extended resources by introducing a new SharedMemory message type in the IDL with configurable mount options. The implementation includes protobuf definitions and pod helper functionality for Kubernetes pods, utilizing getter methods for shared volume mount properties. The changes enable tasks to request and configure memory-backed volumes with either default node allocatable memory or custom size specifications across multiple language bindings including Go, JavaScript/TypeScript, and Python.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5