Added execution config changes #378

pmahindrakar-oss · 2022-03-23T12:17:55Z

Signed-off-by: Prafulla Mahindrakar [email protected]

TL;DR

Testing Done

Using only the values.yaml changes

      flyteadmin:
        roleNameKey: "iam.amazonaws.com/role"
        profilerPort: 10254
        metricsScope: "flyte:"
        labels:
          "FlyteLabelKey" : "FlyteLabelValue"
        annotations:
          "FlyteAnnotationKey" : "FlyteAnnotationValue"
        metadataStoragePrefix:
          - "metadata"
          - "admin"
        eventVersion: 2
        testing:
          host: http://flyteadmin

Launched pod

 kubectl get pods  -n flytesnacks-development f5e21f3c6ec3048d8b80-n0-0 -o yaml
apiVersion: v1
kind: Pod
metadata:
  annotations:
    cluster-autoscaler.kubernetes.io/safe-to-evict: "false"
    flyteannotationkey: FlyteAnnotationValue
  creationTimestamp: "2022-03-24T14:57:29Z"
  labels:
    domain: development
    execution-id: f5e21f3c6ec3048d8b80
    flytelabelkey: FlyteLabelValue
    interruptible: "false"
    node-id: n0
    project: flytesnacks
    shard-key: "15"
    task-name: core-flyte-basics-lp-greet
    workflow-name: core-flyte-basics-lp-go-greet

With overriden project domain attributes for WORKFLOW_EXECUTION_CONFIG

lytectl get workflow-execution-config -p flytesnacks -d development
{
  "project": "flytesnacks",
  "domain": "development",
  "max_parallelism": 10,
  "security_context": {
    "run_as": {
      "k8s_service_account": "default"
    }
  },
  "raw_output_data_config": {
    "output_location_prefix": "cliOutputLocationPrefix"
  },
  "labels": {
    "values": {
      "cliLabelKey": "cliLabelValue"
    }
  },
  "annotations": {
    "values": {
      "cliAnnotationKeyNew": "cliAnnotationValue"
    }
  }
}%

kubectl get pods  -n flytesnacks-development fe1c87252016c4cf6942-n0-0 -o yaml
apiVersion: v1
kind: Pod
metadata:
  annotations:
    cliAnnotationKeyNew: cliAnnotationValue
    cluster-autoscaler.kubernetes.io/safe-to-evict: "false"
  creationTimestamp: "2022-03-24T14:58:37Z"
  labels:
    cliLabelKey: cliLabelValue
    domain: development
    execution-id: fe1c87252016c4cf6942
    interruptible: "false"
    node-id: n0
    project: flytesnacks
    shard-key: "8"
    task-name: core-flyte-basics-lp-greet
    workflow-name: core-flyte-basics-lp-go-greet

Worflow level attributes have a bug and hence couldn't be tested and also the current changes don't use workflow level execution config.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#2070

Follow-up issue

NA

pmahindrakar-oss · 2022-03-23T12:18:48Z

pkg/runtime/application_config_provider.go

+	Labels:                map[string]string{"defaultFlyteLabelKey": "defaultFlyteLabelValue"},
+	Annotations:           map[string]string{"defaultFlyteAnnotationKey": "defaultFlyteAnnotationValue"},
+	OutputLocationPrefix:  "defaultFlyteOutputLocationPrefix",
+	K8SServiceAccount:     "default",
+	AssumableIamRole:      "default",


Are we ok with these defaults @katrogan

Hm, besides setting the K8SServiceAccount are the rest of these actually useful for a flyte execution? maybe we can omit the rest

codecov · 2022-03-23T12:25:14Z

Codecov Report

Merging #378 (ccffd3d) into master (cc40d26) will increase coverage by 0.23%.
The diff coverage is 82.19%.

❗ Current head ccffd3d differs from pull request most recent head 3e18bf8. Consider uploading reports for the commit 3e18bf8 to get more accurate results

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
+ Coverage   60.39%   60.62%   +0.23%     
==========================================
  Files         150      150              
  Lines       10708    10739      +31     
==========================================
+ Hits         6467     6511      +44     
+ Misses       3543     3528      -15     
- Partials      698      700       +2

Flag	Coverage Δ
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/rpc/adminservice/base.go	`3.88% <ø> (+0.17%)`	⬆️
pkg/runtime/application_config_provider.go	`12.50% <ø> (ø)`
pkg/repositories/database.go	`46.72% <50.00%> (+8.34%)`	⬆️
pkg/manager/impl/execution_manager.go	`73.97% <87.23%> (+0.75%)`	⬆️
pkg/manager/impl/util/shared.go	`67.11% <100.00%> (+2.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc40d26...3e18bf8. Read the comment docs.

katrogan · 2022-03-23T17:41:18Z

pkg/manager/impl/execution_manager.go

+// WorkflowExecutionConfigInterface is used as common interface for capturing the common behavior catering to the needs
+// of fetching the WorkflowExecutionConfig across LaunchPlanSpec, ExecutionCreateRequest
+// MatchableResource_WORKFLOW_EXECUTION_CONFIG and ApplicationConfig
+type WorkflowExecutionConfigInterface interface {


why does this need to be an interface?

It becomes very convenient now to use the same merge function

mergeIntoExecConfig(workflowExecConfig *admin.WorkflowExecutionConfig, spec WorkflowExecutionConfigInterface)

for launchplan spec, executionCreateRequest, matchable resorce and application config.
Without these we will have to define individual functions for each of these object types.

Abstracting this common functionality across all these make it easier to manage the code.
Let me know if you think otherwise

I see, maybe one day flyteorg/flyte#2242 will simplify this!

Yeah that would help simplify a lot. I couldn't find a way to do this with current protobuf and faced the same issue with another service we wrote where we had to duplicate the message fields .

katrogan · 2022-03-23T17:48:59Z

pkg/runtime/application_config_provider.go

+	Labels:                map[string]string{"defaultFlyteLabelKey": "defaultFlyteLabelValue"},
+	Annotations:           map[string]string{"defaultFlyteAnnotationKey": "defaultFlyteAnnotationValue"},
+	OutputLocationPrefix:  "defaultFlyteOutputLocationPrefix",
+	K8SServiceAccount:     "default",
+	AssumableIamRole:      "default",


Hm, besides setting the K8SServiceAccount are the rest of these actually useful for a flyte execution? maybe we can omit the rest

Signed-off-by: Prafulla Mahindrakar <[email protected]>

pmahindrakar-oss · 2022-03-24T15:25:11Z

@katrogan i have added default values only for k8ServiceAccount for now . Please have a look at the description along with the mentioned bug for workflow attribute

katrogan

mind updating the PR title before merging?

go.mod

katrogan · 2022-03-24T22:20:11Z

pkg/manager/impl/execution_manager.go

+// WorkflowExecutionConfigInterface is used as common interface for capturing the common behavior catering to the needs
+// of fetching the WorkflowExecutionConfig across LaunchPlanSpec, ExecutionCreateRequest
+// MatchableResource_WORKFLOW_EXECUTION_CONFIG and ApplicationConfig
+type WorkflowExecutionConfigInterface interface {


I see, maybe one day flyteorg/flyte#2242 will simplify this!

katrogan · 2022-03-24T22:23:17Z

pkg/manager/impl/execution_manager.go

+	if workflowExecConfig.GetSecurityContext() == nil && spec.GetSecurityContext() != nil {
+		workflowExecConfig.SecurityContext = spec.GetSecurityContext()
+	}
+	// Launchplan spec has label, annotation and rawOutputDataConfig initialized with empty values.


ah, is this from flytekit serialization? thank you for documenting this

Yes that right

labels=entity.labels or _common_models.Labels({}), annotations=entity.annotations or _common_models.Annotations({}), auth_role=entity._auth_role or _common_models.AuthRole(), raw_output_data_config=entity.raw_output_data_config or _common_models.RawOutputDataConfig(""),

https://github.com/flyteorg/flytekit/blob/b08e2b6d4cd42f6dc9be86bcec3044b3138e329c/flytekit/tools/translator.py#L224

katrogan · 2022-03-24T22:24:13Z

pkg/manager/impl/execution_manager.go

+
+	workflowExecConfig := &admin.WorkflowExecutionConfig{}
+	// merge the request spec into workflowExecConfig
+	mergeIntoExecConfig(workflowExecConfig, request.Spec)


why this call instead of initializing the workflowExecConfig struct inline with value from the request.Spec?

Two reasons

It allows us to not make changes in merge aswell getExecutionConfig whenever a new field is introduced.

Since we now want old behavior where if any field is set in request spec then return that execution config immediately instead of checking for other overrides.

So i have kept the function with its return value checked. Let me know what you think

pkg/manager/impl/util/shared.go

katrogan · 2022-03-24T22:32:45Z

pkg/manager/impl/execution_manager.go

+		workflowExecConfig.Annotations = spec.GetAnnotations()
+	}
+}
+
 // Produces execution-time attributes for workflow execution.
 // Defaults to overridable execution values set in the execution create request, then looks at the launch plan values
 // (if any) before defaulting to values set in the matchable resource db and further if matchable resources don't
 // exist then defaults to one set in application configuration
 func (m *ExecutionManager) getExecutionConfig(ctx context.Context, request *admin.ExecutionCreateRequest,


what if a user specifies some fields in the execution config in the request spec, but not all?

I think we should then treat that execution config as finalized, and the omitted fields as intentionally excluded.

currently, we keep proceeding with the fallbacks regardless of whether the request spec is populated, or barring that, if the launch plan is then populated. i'm not sure this is right, what do you think? if one of these levels has any non-empty values then should we treat that execution config as the final execution config we apply?

I wanted this to look like how we do override for go variables and scopes actually define what that level defines the value for that variable
https://go.dev/play/p/jdkv14fHY8N

But we don't have a way to tell protobuf fields that they are set but those are explicitly being set to nil or empty.
i have seen where some libraries provide a behavior of tristate

i.e set (having value/ having no value) / unset(uninitialized)
Something like this tristateBoolean having isUndef https://docs.oracle.com/cd/E26098_01/apirefs.1112/e17493/oracle/ide/util/TriStateBoolean.html#isUndef__

That way it easier to distinguish what the user wanted to do.

Also I think in the current state that issue comes even for maxParallelism = 0 in execution create request and launch plan spec having the same value and it will fall back to other overrides though the user intended to set maxParallelism = 0 in execution create Request

Having a tristate behavior solves such cases and make it easier to reason across overrides and avoid using custom logic IMO.

To keep logic similar to previous one , i am adding a isChanged bool flag in merge function which is checked after the merge from execution create request and launchplan spec and if it gets set then we return that config object.

Let me know what you think.

re: the tri-state, we do something funky with one-ofs for notification overrides to permit a user to explicitly disable notifications as opposed to interpreting the optional field as omitted. we could consider updating the execution config in the proto definitions to have some kind of bool flag for "is set" so that even an empty execution config is considered explicitly set

isChanged looks good for now too!

pkg/manager/impl/util/shared_test.go

katrogan · 2022-03-24T22:34:48Z

pkg/runtime/interfaces/application_configuration.go

+
+	// Optional: security context override to apply this execution.
+	// iam_role references the fully qualified name of Identity & Access Management role to impersonate.
+	AssumableIamRole string `json:"assumableIamRole"`


nit: add comments to pflag?

We are not using this as pflag config and none of the other fields in this config can be passed from command line.
If we want that behavior then i would need to add a pflag generator and add pflag annotation to all.
Let me know if we need that now or in any future PR's

ah okay, nevermind then

…riding Signed-off-by: Prafulla Mahindrakar <[email protected]>

Signed-off-by: Prafulla Mahindrakar <[email protected]>

* Added execution config changes Signed-off-by: Prafulla Mahindrakar <[email protected]> * lint fixes Signed-off-by: Prafulla Mahindrakar <[email protected]> * using executionConfig data during launch Signed-off-by: Prafulla Mahindrakar <[email protected]> * resolve conflicts Signed-off-by: Prafulla Mahindrakar <[email protected]> * Removed defaults for labels and annotations Signed-off-by: Prafulla Mahindrakar <[email protected]> * added more coverage Signed-off-by: Prafulla Mahindrakar <[email protected]> * added more coverage Signed-off-by: Prafulla Mahindrakar <[email protected]> * Updating idl and lint fixes Signed-off-by: Prafulla Mahindrakar <[email protected]> * Adde missing go.sum Signed-off-by: Prafulla Mahindrakar <[email protected]> * feedback changes to return immediately if any field is set while overriding Signed-off-by: Prafulla Mahindrakar <[email protected]> * using released flyteidl Signed-off-by: Prafulla Mahindrakar <[email protected]> * using released flyteidl Signed-off-by: Prafulla Mahindrakar <[email protected]>

pmahindrakar-oss requested review from katrogan, wild-endeavor, anandswaminathan, EngHabu and kumare3 as code owners March 23, 2022 12:17

pmahindrakar-oss commented Mar 23, 2022

View reviewed changes

katrogan reviewed Mar 23, 2022

View reviewed changes

pmahindrakar-oss added 4 commits March 24, 2022 15:29

Added execution config changes

99f4e4c

Signed-off-by: Prafulla Mahindrakar <[email protected]>

lint fixes

571bb05

Signed-off-by: Prafulla Mahindrakar <[email protected]>

using executionConfig data during launch

ad6e165

Signed-off-by: Prafulla Mahindrakar <[email protected]>

resolve conflicts

c5ea342

Signed-off-by: Prafulla Mahindrakar <[email protected]>

pmahindrakar-oss force-pushed the pmahindrakar/2070-Project-Domain-Setting branch from 4ad214e to c5ea342 Compare March 24, 2022 10:02

pmahindrakar-oss added 5 commits March 24, 2022 16:32

Removed defaults for labels and annotations

ae357af

Signed-off-by: Prafulla Mahindrakar <[email protected]>

added more coverage

104b83e

Signed-off-by: Prafulla Mahindrakar <[email protected]>

added more coverage

5af9eff

Signed-off-by: Prafulla Mahindrakar <[email protected]>

Updating idl and lint fixes

59ac680

Signed-off-by: Prafulla Mahindrakar <[email protected]>

Adde missing go.sum

8b6d8f0

Signed-off-by: Prafulla Mahindrakar <[email protected]>

pmahindrakar-oss requested a review from katrogan March 24, 2022 15:09

This was referenced Mar 24, 2022

Project domain attribute for workflow Execution config flyteorg/flytectl#298

Merged

Additional execution properties as matchable entities. flyteorg/flyteidl#253

Merged

katrogan reviewed Mar 24, 2022

View reviewed changes

pmahindrakar-oss added 3 commits March 25, 2022 17:05

feedback changes to return immediately if any field is set while over…

c7acb94

…riding Signed-off-by: Prafulla Mahindrakar <[email protected]>

using released flyteidl

1927bde

Signed-off-by: Prafulla Mahindrakar <[email protected]>

using released flyteidl

3e18bf8

Signed-off-by: Prafulla Mahindrakar <[email protected]>

pmahindrakar-oss changed the title ~~WIP : Added execution config changes~~ Added execution config changes Mar 25, 2022

pmahindrakar-oss requested a review from katrogan March 25, 2022 11:42

katrogan approved these changes Mar 25, 2022

View reviewed changes

pmahindrakar-oss merged commit 56de4b1 into master Mar 25, 2022

pmahindrakar-oss deleted the pmahindrakar/2070-Project-Domain-Setting branch March 25, 2022 14:37

YmirKhang mentioned this pull request Jun 7, 2022

[BUG] Flyte spark tasks are not being executed with spark service account flyteorg/flyte#2595

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added execution config changes #378

Added execution config changes #378

pmahindrakar-oss commented Mar 23, 2022 •

edited

Loading

pmahindrakar-oss Mar 23, 2022

katrogan Mar 23, 2022

codecov bot commented Mar 23, 2022 •

edited

Loading

katrogan Mar 23, 2022

pmahindrakar-oss Mar 24, 2022

katrogan Mar 24, 2022

pmahindrakar-oss Mar 25, 2022

katrogan Mar 23, 2022

pmahindrakar-oss commented Mar 24, 2022

katrogan left a comment

katrogan Mar 24, 2022

katrogan Mar 24, 2022

pmahindrakar-oss Mar 25, 2022

katrogan Mar 24, 2022

pmahindrakar-oss Mar 25, 2022

katrogan Mar 24, 2022

pmahindrakar-oss Mar 25, 2022

katrogan Mar 25, 2022

katrogan Mar 24, 2022

pmahindrakar-oss Mar 25, 2022

katrogan Mar 25, 2022

Added execution config changes #378

Added execution config changes #378

Conversation

pmahindrakar-oss commented Mar 23, 2022 • edited Loading

TL;DR

Testing Done

Using only the values.yaml changes

With overriden project domain attributes for WORKFLOW_EXECUTION_CONFIG

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2022 • edited Loading

Codecov Report

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

pmahindrakar-oss commented Mar 24, 2022

katrogan 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmahindrakar-oss commented Mar 23, 2022 •

edited

Loading

codecov bot commented Mar 23, 2022 •

edited

Loading