Ensure admin never sets task resource request > limit #126
Conversation
katrogan
requested review from
anandswaminathan,
EngHabu,
kumare3,
matthewphsmith and
wild-endeavor
as code owners
katrogan
requested review from
bnsblue
and removed request for
wild-endeavor,
matthewphsmith,
kumare3 and
anandswaminathan
Codecov Report
@@ Coverage Diff @@
## master #126 +/- ##
==========================================
- Coverage 62.26% 61.27% -1.00%
==========================================
Files 105 105
Lines 7845 7267 -578
==========================================
- Hits 4885 4453 -432
+ Misses 2385 2247 -138
+ Partials 575 567 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -263,6 +265,41 @@ func assignResourcesIfUnset(ctx context.Context, identifier *core.Identifier, | |||
| return resourceEntries | |||
| } | |||
|
|
|||
| func resolveTaskLimitsAndPlatformRequestDefaults(ctx context.Context, identifier *core.Identifier, | |||
There was a problem hiding this comment.
I don't see the code updating the limits (as the name suggests?) am I missing something?
There was a problem hiding this comment.
nope, I just struggled with naming. is considerTaskLimitsAndPlatformRequestDefaults or reconcileTaskLimitsAndPlatformRequestDefaults any better? other suggestions welcomed
There was a problem hiding this comment.
the limit is not supposed to change though... the request is.
|
friendly ping @EngHabu @wild-endeavor @bnsblue |
| } | ||
| if quantity.Cmp(resource.MustParse(limitValue)) == 1 { | ||
| // The quantity is greater than the limit! Course correct below. | ||
| logger.Debugf(ctx, "Updating requested value for task [%+v] resource [%s]. Overriding to [%s] from [%s]", |
There was a problem hiding this comment.
nit: can you add what causes this override to the log so that it's clearer when reading the logs?
There was a problem hiding this comment.
Also I feel the debug level should be warning/info? Not quite sure 🤔
There was a problem hiding this comment.
updated & upgraded
| }, | ||
| }, | ||
| }, resources)) | ||
| }) |
There was a problem hiding this comment.
Can you add a test that tests the logic here? https://github.com/lyft/flyteadmin/pull/126/files#diff-fc047e54b9dd82ca7c89ac9b32cb07b3R282-R288
Specifically, can you add a test where one of the resource does not have the limit specified?
There was a problem hiding this comment.
at the stage where the method is called all values will have already been substituted with the application defaults. (https://github.com/lyft/flyteadmin/pull/126/files#diff-fc047e54b9dd82ca7c89ac9b32cb07b3R342) i only added that check as a defensive safeguard
There was a problem hiding this comment.
oh interesting. So the control flow should never enter this block?
https://github.com/lyft/flyteadmin/pull/126/files#diff-fc047e54b9dd82ca7c89ac9b32cb07b3R283-R288
There was a problem hiding this comment.
unless the user is specifying a resource we don't provide defaults for. but in that case we don't find ourselves in the original conundrum where the substituted request > limit since there will be no request default value which leads to a permissible situation for scheduling
There was a problem hiding this comment.
From a unit-test point of view I think that it would still be nice to have a test for it since we don't know if in the future this function would be invoked through other code paths where the default limits were not pre-filled. But as you said it is not critical here.
|
LGTM. Just some nits. |
TL;DR
In the case where a user specifies a task resource limit but not the request value, admin will substitute the request value with the platform-configured default. In some cases this default can exceed the user-specified limit which is non-sensical and prevents kubernetes from ever scheduling the task.
Type
Are all requirements met?
Complete description
Fixes user-reported bug.
Tracking Issue
flyteorg/flyte#264
Follow-up issue
NA