Add quality of service to launch plan/execution spec #68

katrogan · 2020-06-08T23:24:54Z

TL;DR

This PR introduces a QualityOfService designation for workflow executions. This is primarily used to designate toleration for queueing time but can potentially be leveraged for other server-side execution configuration.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

https://docs.google.com/document/d/1rYqr01x0XL6n1AdmEOUbTIGeS9Z2hJCcnG5NMy8aQ68/edit#

Tracking Issue

flyteorg/flyte#343

Follow-up issue

NA

migueltol22 · 2020-06-08T23:40:46Z

Now that we are adopting quality of service should we keep https://github.com/lyft/flyteidl/blob/42929ee56bc48fc887ed372b468929d797f39643/protos/flyteidl/core/workflow.proto#L132

katrogan · 2020-06-08T23:42:36Z

Actually I think we want to keep queueing budget because admin will still need to set it after interpreting the specified quality of service.

EngHabu

I think it's great you started this work and I feel we might need to do a bit of a design here to make sure we get it right, otherwise we will end up with an unusable bit that's only used for categorical queuing budgets' values :)
A few questions to answer:

Why 3? why not more or less?
Should it be an absolute number to the service? or relative to your other workflows in the project? or somewhere in between?
How to resolve conflicts between that attribute and the queuing budget/interruptible that you can set on the WF? does one win over the other?
If it's relative to the other WFs in the project, will/should people be able to define what each of them mean for themselves? like my Service Quality High means 10min queuing budget but yours mean 1hr... for example
If the values are absolute, we need to plan around how to ensure we have enough capacity to guarantee these classes for everyone... we don't need to answer this particular one now but we need to define the right indicators to use and what oncall should do... etc.

Maybe start a doc for defining what these mean?

EngHabu · 2020-06-09T17:29:52Z

protos/flyteidl/admin/common.proto

@@ -245,3 +245,18 @@ message AuthRole {
  }
 }

+// This general categorization specifies how much queue time an execution can tolerate. 
+// This can be extended in the future to make other execution-time decisions.
+enum QualityOfService {


I think we adopted this model instead:

Suggested change

enum QualityOfService {

message QualityOfService {

enum Values {

UNDEFINED = 0;

...

}

}

EngHabu · 2020-06-09T17:31:47Z

protos/flyteidl/admin/execution.proto

@@ -237,6 +237,8 @@ message ExecutionSpec {
    // Optional: auth override to apply this execution.
    AuthRole auth_role        = 16;

+    // Indicates the amount of queueing a launched workflow execution can tolerate.


Is it only about queuing? does it also control interruptible bit?

update doc?

anandswaminathan · 2020-06-09T19:44:25Z

protos/flyteidl/admin/common.proto

+    // Guarantees that this execution will begin as soon as requested and incur no queueing time.
+    QUALITY_OF_SERVICE_HIGH = 1;
+
+    // This execution may incur some queueing delay (e.g. 30 minutes) and is medium priority.


Do not indicate time here in comments. The range currently is huge.

katrogan · 2020-06-09T23:22:19Z

@EngHabu thanks for the suggestions. I started a doc here to capture a few different implementation proposals and open questions.

katrogan · 2020-06-26T22:28:17Z

@EngHabu @migueltol22 @anandswaminathan mind taking another look? updated to reflect design decisions/discussions

katrogan · 2020-07-01T21:05:37Z

ping @EngHabu @migueltol22 @anandswaminathan

EngHabu · 2020-07-01T22:58:39Z

protos/flyteidl/admin/execution.proto

@@ -237,6 +237,8 @@ message ExecutionSpec {
    // Optional: auth override to apply this execution.
    AuthRole auth_role        = 16;

+    // Indicates the amount of queueing a launched workflow execution can tolerate.


update doc?

EngHabu · 2020-07-01T23:00:23Z

protos/flyteidl/admin/quality_of_service.proto

+
+message QualityOfServiceSpec {
+  // Indicates how much queueing delay an execution can tolerate.
+  uint32 queueing_budget_mins = 1;


Should we use this instead?

originally that's what I had but people commented that anything beyond minutes was too much granularity and unnecessary

one option is we could expose it as queueing_budget_mins in the SDK and then store that as a duration in the proto field but that can still be manipulated by non-SDK users. what do you think @EngHabu ?

will go with the second option, updated to use duration and we can expose it as a field in mins only in the sdk cc @migueltol22

migueltol22 · 2020-07-06T15:06:11Z

protos/flyteidl/admin/launch_plan.proto

@@ -91,6 +92,9 @@ message LaunchPlanSpec {
    Auth auth                             = 8 [deprecated=true];

    AuthRole auth_role                    = 9;
+
+    // Indicates the amount of queueing a launched workflow execution can tolerate.
+    QualityOfService quality_of_service   = 16;


Since this will be part of the launchplan should we also remove this?

I believe we'll want to resolve the queueing budget based on the quality of service on the admin side before creating the workflow CRD so we should still keep the queueing_budget field

thank you for the catch! updated to use QualityOfService in the workflow metadata

migueltol22

LGTM. Will leave final approval to @EngHabu

Add quality of service to launch plan/execution spec

b3763f7

katrogan requested review from EngHabu, migueltol22 and anandswaminathan June 8, 2020 23:25

katrogan added 3 commits June 8, 2020 16:48

Deprecate queueing budget

b528545

undefined as default

1080df1

matchable resource too

152fa97

migueltol22 previously approved these changes Jun 9, 2020

View reviewed changes

missed new generated files oops

ed7c5e8

katrogan dismissed migueltol22’s stale review via ed7c5e8 June 9, 2020 16:43

EngHabu reviewed Jun 9, 2020

View reviewed changes

anandswaminathan reviewed Jun 9, 2020

View reviewed changes

update

9396d20

EngHabu reviewed Jul 1, 2020

View reviewed changes

migueltol22 reviewed Jul 6, 2020

View reviewed changes

katrogan added 3 commits July 6, 2020 11:25

also workflow metadata

8b76e6c

Merge branch 'master' into quality-of-service

dd83d74

version

78fd448

migueltol22 reviewed Jul 7, 2020

View reviewed changes

katrogan added 2 commits July 7, 2020 14:19

sigh

8843e9d

rename too

7f8b4d8

katrogan requested a review from EngHabu July 8, 2020 17:41

use duration instead

c3fd659

EngHabu approved these changes Jul 8, 2020

View reviewed changes

katrogan merged commit e9727af into master Jul 8, 2020

eapolinario pushed a commit that referenced this pull request Sep 8, 2023

Add quality of service to launch plan/execution spec (#68)

6d1f69b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quality of service to launch plan/execution spec #68

Add quality of service to launch plan/execution spec #68

katrogan commented Jun 8, 2020 •

edited

Loading

migueltol22 commented Jun 8, 2020

katrogan commented Jun 8, 2020 •

edited

Loading

EngHabu left a comment

EngHabu Jun 9, 2020

EngHabu Jun 9, 2020

EngHabu Jul 1, 2020

anandswaminathan Jun 9, 2020

katrogan commented Jun 9, 2020

katrogan commented Jun 26, 2020

katrogan commented Jul 1, 2020

EngHabu Jul 1, 2020

EngHabu Jul 1, 2020

katrogan Jul 6, 2020 •

edited

Loading

katrogan Jul 8, 2020

migueltol22 Jul 6, 2020

katrogan Jul 6, 2020

katrogan Jul 6, 2020

migueltol22 left a comment

-enum QualityOfService {
+message QualityOfService {
+    enum Values {
+        UNDEFINED = 0;
+        ...
+    }
+}

Add quality of service to launch plan/execution spec #68

Add quality of service to launch plan/execution spec #68

Conversation

katrogan commented Jun 8, 2020 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

migueltol22 commented Jun 8, 2020

katrogan commented Jun 8, 2020 • edited Loading

EngHabu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan commented Jun 9, 2020

katrogan commented Jun 26, 2020

katrogan commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrogan Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migueltol22 left a comment

Choose a reason for hiding this comment

katrogan commented Jun 8, 2020 •

edited

Loading

katrogan commented Jun 8, 2020 •

edited

Loading

katrogan Jul 6, 2020 •

edited

Loading