Resource manager overhaul #552
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,6 @@ import ( | |
|
|
||
| "github.com/flyteorg/flyteadmin/plugins" | ||
|
|
||
| "github.com/flyteorg/flyteadmin/auth" | ||
|
|
||
| "github.com/flyteorg/flyteadmin/pkg/manager/impl/resources" | ||
|
|
@@ -184,12 +182,49 @@ func (m *ExecutionManager) addPluginOverrides(ctx context.Context, executionID * | |
| return nil, nil | ||
| } | ||
|
|
||
| // defaults should be a coalesce of task defaults, and platform defaults. | ||
| // task limits should be the limits from the task, coalesced with the defaults from step one | ||
| // then both should be limited by any platform limits. | ||
| // anything 0 or empty is not set. | ||
| // if both requests and limits end up empty, return nil. if one is empty, return nil for it | ||
| func (m *ExecutionManager) getResources(ctx context.Context, taskResources *core.Resources, platformResources workflowengineInterfaces.TaskResources) *core.Resources { | ||
|
|
||
| // requests: coalesce(task request, platform default) | ||
| // limits: coalesce(task limits, task requests) | ||
|
There was a problem hiding this comment. IMHO this only makes sense for non-compressible resources like memory and storage. |
||
| // check that defaults and limits are both below platform limit | ||
| var requestSet runtimeInterfaces.TaskResourceSet | ||
| var limitSet runtimeInterfaces.TaskResourceSet | ||
| if taskResources != nil && taskResources.GetRequests() != nil { | ||
| requestSet = util.GetTaskResourcesAndCoalesce(ctx, taskResources.GetRequests(), platformResources.Defaults) | ||
| } else { | ||
| requestSet = platformResources.Defaults | ||
| } | ||
| if taskResources != nil && taskResources.GetLimits() != nil { | ||
| limitSet = util.GetTaskResourcesAndCoalesce(ctx, taskResources.GetLimits(), requestSet) | ||
| } else { | ||
| limitSet = requestSet | ||
| } | ||
| adjustedRequestSet := util.ConstrainTaskResourceSet(ctx, requestSet, platformResources.Limits) | ||
| adjustedLimitSet := util.ConstrainTaskResourceSet(ctx, limitSet, platformResources.Limits) | ||
|
|
||
| // convert the sets back to core.Resources | ||
| requestEntries := util.ConvertTaskResourceSetToCoreResources(adjustedRequestSet) | ||
| limitEntries := util.ConvertTaskResourceSetToCoreResources(adjustedLimitSet) | ||
| if len(requestEntries) == 0 && len(limitEntries) == 0 { | ||
| return nil | ||
| } | ||
| res := core.Resources{} | ||
|
There was a problem hiding this comment. In the code this replaces (current lines 210-213) we init the There was a problem hiding this comment. i think we solve this with more testing. looked through the code and i don't think it's used. Also ran locally with no admin config, no matchable resources, and no task resources, and things seem to be running almost correctly. The pod is still being created with but the fly crd has node resources and has execution config task resources: which is correct because these are non-nullable. and resources doesn't show up at all in the task template part of the task definition. if downstream code can't handle a nil for some reason it should handle it there i feel. |
||
| if len(requestEntries) > 0 { | ||
| res.Requests = requestEntries | ||
| } | ||
| if len(limitEntries) > 0 { | ||
| res.Limits = limitEntries | ||
| } | ||
|
|
||
| return &res | ||
| } | ||
|
|
||
| // Assumes input contains a compiled task with a valid container resource execConfig. | ||
| func (m *ExecutionManager) setCompiledTaskDefaults(ctx context.Context, task *core.CompiledTask, | ||
| platformTaskResources workflowengineInterfaces.TaskResources) { | ||
|
|
||
|
|
@@ -203,100 +238,8 @@ func (m *ExecutionManager) setCompiledTaskDefaults(ctx context.Context, task *co | |
| logger.Debugf(ctx, "Not setting default resources for task [%+v], no container resources found to check", task) | ||
| return | ||
| } | ||
| task.Template.GetContainer().Resources = m.getResources(ctx, task.Template.GetContainer().Resources, platformTaskResources) | ||
|
|
||
|
Comment on lines
-248
to
-250
There was a problem hiding this comment. Do we have to check for cases where a user manually sets a resource limit to 0 to override the default limits? For example - flyteadmin configurations default limit to 5G Memory, if a user wants a task with unbounded Memory can they set There was a problem hiding this comment. yeah not sure. So as of this PR, "0", is not treated specially until the very end. It's like any other number. It's at |
||
| } | ||
|
|
||
| // Fetches inherited execution metadata including the parent node execution db model id and the source execution model id | ||
|
|
@@ -1623,7 +1566,7 @@ func NewExecutionManager(db repositoryInterfaces.Repository, pluginRegistry *plu | |
| "size in bytes of serialized execution outputs"), | ||
| } | ||
|
|
||
| resourceManager := resources.NewResourceManager(db, config) | ||
| return &ExecutionManager{ | ||
| db: db, | ||
| config: config, | ||
|
|
||
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.
This logic seems sound to me, but would love to see this simplified. For example, can we convert to a similar type and use a function like: