Add quality of service logic to admin #107
Conversation
katrogan
requested review from
anandswaminathan,
EngHabu,
kumare3,
matthewphsmith and
wild-endeavor
as code owners
katrogan
requested review from
migueltol22 and
anandswaminathan
and removed request for
matthewphsmith,
kumare3 and
anandswaminathan
Codecov Report
@@ Coverage Diff @@
## master #107 +/- ##
==========================================
- Coverage 62.60% 62.52% -0.08%
==========================================
Files 102 104 +2
Lines 7418 7587 +169
==========================================
+ Hits 4644 4744 +100
- Misses 2228 2286 +58
- Partials 546 557 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
|
||
| // If nothing in the hierarchy has set the quality of service, see if an override exists in the matchable attributes | ||
| // resource table. | ||
| resource, err := q.resourceManager.GetResource(ctx, interfaces.ResourceRequest{ |
There was a problem hiding this comment.
Should we only do this if qualityOfServiceTier == UNDEFINED ?
There was a problem hiding this comment.
good call, thanks! refactored this before opening the PR and missed the check here
|
friendly ping @EngHabu @migueltol22 |
| @@ -159,3 +159,16 @@ cluster_resources: | |||
| valueFrom: | |||
| env: SHELL | |||
| refresh: 3s | |||
| qualityOfService: | |||
There was a problem hiding this comment.
Not a fan of this name - I just don't like the connotation of a low quality of service. Is it too late to change this?
There was a problem hiding this comment.
it's not used so technically i can go make a breaking change :) any suggestions instead? basic/baseline?
There was a problem hiding this comment.
actually we've had bad luck with supposedly no-op breaking changes so I would rather not (this is checked into flyteidl, unfortunately)
| // Config values should always be vetted so there's no need to check the error from conversion. | ||
| duration, _ := ptypes.Duration(executionValues.QueueingBudget) | ||
|
|
||
| return QualityOfServiceSpec{ |
There was a problem hiding this comment.
can we add a debug log line to each case so we can more easily understand what was used?
| @@ -0,0 +1,26 @@ | |||
| package mocks | |||
There was a problem hiding this comment.
why doesn't this file have those generated file comments? is this not generated?
There was a problem hiding this comment.
nope, see flyteorg/flyte#149
| return &workflowengineInterfaces.ExecutionInfo{ | ||
| Cluster: testCluster, | ||
| }, nil | ||
| }) | ||
| //qosProvider := runtimeMocks.NewMockQualityOfServiceProvider() |
TL;DR
This PR implements translation from a user-specified quality of service to a queueing budget used for cost optimization.
Users can specify the quality of service for an execution (in order of decreasing specificity)
System administrators can specify default quality of service specs for different tiers and default tiers for different domains.
Type
Are all requirements met?
Complete description
https://docs.google.com/document/d/1rYqr01x0XL6n1AdmEOUbTIGeS9Z2hJCcnG5NMy8aQ68/edit#
Tracking Issue
flyteorg/flyte#343
Follow-up issue
NA