[Housekeeping] Remove storage as a task resource option #4116
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Signed-off-by: [karthik] <[[email protected]]>
Kota-Karthik
changed the title
Kota-Karthik
changed the title
Signed-off-by: [karthik] <[[email protected]]>
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4116 +/- ##
==========================================
- Coverage 58.98% 57.50% -1.48%
==========================================
Files 618 393 -225
Lines 52708 28904 -23804
==========================================
- Hits 31088 16621 -14467
+ Misses 19140 10598 -8542
+ Partials 2480 1685 -795
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -61,7 +61,7 @@ helm install gateway bitnami/contour -n flyte | |||
| | cloud_events.eventsPublisher.topicName | string | `"arn:aws:sns:us-east-2:123456:123-my-topic"` | | | |||
| | cloud_events.type | string | `"aws"` | | | |||
| | cluster_resource_manager | object | `{"config":{"cluster_resources":{"customData":[{"production":[{"projectQuotaCpu":{"value":"5"}},{"projectQuotaMemory":{"value":"4000Mi"}}]},{"staging":[{"projectQuotaCpu":{"value":"2"}},{"projectQuotaMemory":{"value":"3000Mi"}}]},{"development":[{"projectQuotaCpu":{"value":"4"}},{"projectQuotaMemory":{"value":"3000Mi"}}]}],"refreshInterval":"5m","standaloneDeployment":false,"templatePath":"/etc/flyte/clusterresource/templates"}},"enabled":true,"podAnnotations":{},"service_account_name":"flyteadmin","standaloneDeployment":false,"templates":[{"key":"aa_namespace","value":"apiVersion: v1\nkind: Namespace\nmetadata:\n name: {{ namespace }}\nspec:\n finalizers:\n - kubernetes\n"},{"key":"ab_project_resource_quota","value":"apiVersion: v1\nkind: ResourceQuota\nmetadata:\n name: project-quota\n namespace: {{ namespace }}\nspec:\n hard:\n limits.cpu: {{ projectQuotaCpu }}\n limits.memory: {{ projectQuotaMemory }}\n"}]}` | Configuration for the Cluster resource manager component. This is an optional component, that enables automatic cluster configuration. This is useful to set default quotas, manage namespaces etc that map to a project/domain | | |||
| | cluster_resource_manager.config | object | `{"cluster_resources":{"customData":[{"production":[{"projectQuotaCpu":{"value":"5"}},{"projectQuotaMemory":{"value":"4000Mi"}}]},{"staging":[{"projectQuotaCpu":{"value":"2"}},{"projectQuotaMemory":{"value":"3000Mi"}}]},{"development":[{"projectQuotaCpu":{"value":"4"}},{"projectQuotaMemory":{"value":"3000Mi"}}]}],"refreshInterval":"5m","standaloneDeployment":false,"templatePath":"/etc/flyte/clusterresource/templates"}}` | Configmap for ClusterResource parameters | | |||
| | cluster_resource_manager.config | object | `{"cluster_":{"customData":[{"production":[{"projectQuotaCpu":{"value":"5"}},{"projectQuotaMemory":{"value":"4000Mi"}}]},{"staging":[{"projectQuotaCpu":{"value":"2"}},{"projectQuotaMemory":{"value":"3000Mi"}}]},{"development":[{"projectQuotaCpu":{"value":"4"}},{"projectQuotaMemory":{"value":"3000Mi"}}]}],"refreshInterval":"5m","standaloneDeployment":false,"templatePath":"/etc/flyte/clusterresource/templates"}}` | Configmap for ClusterResource parameters | | |||
There was a problem hiding this comment.
Why is this file modified?
|
@Kota-Karthik, thanks for creating the PR! Could you modify the PR description to specify the changes you made? I don't think you added all those flags you mentioned, correct? They are already present in the code. Could you just describe the changes you introduced? |
|
@Kota-Karthik, could you review my comments? Let me know if you aren't able to resolve them within the next two days 'cause a couple of others are also interested in resolving this issue. |
|
@Kota-Karthik, please let me know if you're currently working on incorporating my suggestions. If not, I'll plan to close the issue within a week. Thank you for creating the PR, though. |
Tracking issue
closes #3955
Describe your changes
I made changes to the
ApplyResourceOverridesfunction to ensure that user-specified resource requests and limits are respected without being overridden by default values, except when explicitly requested. Here are the key changes I made:Introduced a new flag
assignIfUnsetto control whether default values should be assigned. This flag is used to determine whether the code should apply default values for resources.Modified the behavior inside the function to check the
assignIfUnsetflag before applying default values. WhenassignIfUnsetis set totrue, default values will be assigned only if the user hasn't already specified resource requests and limits.Added conditional checks for each resource type (e.g., CPU, memory, ephemeral storage, GPU) to ensure that the default values are applied only if the user has not already set resource requirements.
Removed the assignment of default values for storage resources (
v1.ResourceStorage) based on the flagassignIfUnset. This allows users to set their own storage requirements without being overridden by defaults.Introduced the
shouldAdjustGPUflag to determine whether GPU resource adjustments are necessary. GPU resources are now adjusted based on user-specified values, and thegpuResourceNameis used for customization.Updated the comments to reflect the changes made and provide context for the
assignIfUnsetflag and resource adjustments.These changes ensure that users can set their desired resource requirements, and the code will respect their choices without applying default values unless explicitly requested by setting
assignIfUnsettotrue. This provides greater flexibility and control to users when specifying resource requirements for their tasks.Check all the applicable boxes
Note to reviewers
@samhita-alla
As per your instruction, I have created the Pull Request.
I kindly request you to review it at your earliest convenience.