-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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]]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ApplyResourceOverrides
function 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
assignIfUnset
to 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
assignIfUnset
flag before applying default values. WhenassignIfUnset
is 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
shouldAdjustGPU
flag to determine whether GPU resource adjustments are necessary. GPU resources are now adjusted based on user-specified values, and thegpuResourceName
is used for customization.Updated the comments to reflect the changes made and provide context for the
assignIfUnset
flag 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
assignIfUnset
totrue
. 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.