Added cluster resource attribute support #65

pmahindrakar-oss · 2021-05-11T12:32:24Z

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

TL;DR

This PR specifically addresses supporting cluster resource attributes through flytectl.

GET

Retrieves cluster resource attributes for given project,domain combination or additionally with workflow name.

Here the command get cluster resource attributes for project flytectldemo and development domain.

flytectl get cluster-resource-attribute -p flytectldemo -d development

eg : O/P

{"project":"flytectldemo","domain":"development","attributes":{"buzz":"lightyear","foo":"bar"}}

Writing the cluster resource attribute to a file. If there are no cluster resource attributes , command would return an error
Here the command gets cluster resource attributes and writes the config file to tra.yaml
eg: content of tra.yaml

flytectl get cluster-resource-attribute --attrFile cra.yaml

.. code-block:: yaml

	domain: development
	project: flytectldemo
	attributes:
	  foo: "bar"
	  buzz: "lightyear"

UPDATE
Updates cluster resource attributes for given project and domain combination or additionally with workflow name.

Updating the cluster resource attribute is only available from config file
Here the command updates takes the input for cluster resource attributes from the config file cra.yaml

eg: content of tra.yaml
flytectl update cluster-resource-attribute -attrFile cra.yaml

DELETE
Deletes cluster resource attributes for given project and domain combination or additionally with workflow name.
Deletes cluster resource attribute for project and domain
Here the command delete cluster resource attributes for project flytectldemo and development domain.

flytectl delete cluster-resource-attribute -p flytectldemo -d development

Deleting cluster resource attribute using config file
Here the command deletes cluster resource attributes from the config file cra.yaml
eg: content of cra.yaml which will use the project domain and workflow name for deleting the resource

flytectl delete cluster-resource-attribute --attrFile cra.yaml

.. code-block:: yaml

	domain: development
	project: flytectldemo
	attributes:
	  foo: "bar"
	  buzz: "lightyear"

Deleting cluster resource attribute for a workflow
Here the command deletes cluster resource attributes for a workflow

flytectl delete cluster-resource-attribute -p flytectldemo -d development core.control_flow.run_merge_sort.merge_sort

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Tracking Issue

flyteorg/flyte#992

Follow-up issue

NA

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

codecov · 2021-05-12T07:18:31Z

Codecov Report

Merging #65 (1573ec6) into master (b511cef) will increase coverage by 2.27%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   55.67%   57.94%   +2.27%     
==========================================
  Files          58       68      +10     
  Lines        1455     1548      +93     
==========================================
+ Hits          810      897      +87     
- Misses        590      596       +6     
  Partials       55       55

Flag	Coverage Δ
unittests	`57.94% <80.00%> (+2.27%)`	⬆️

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

Impacted Files	Coverage Δ
...nd/taskresourceattribute/attrdeleteconfig_flags.go	`20.00% <20.00%> (ø)`
...nd/taskresourceattribute/attrupdateconfig_flags.go	`20.00% <20.00%> (ø)`
cmd/config/subcommand/workflow/config_flags.go	`25.00% <25.00%> (ø)`
...subcommand/clusterresourceattribute/file_config.go	`80.00% <80.00%> (ø)`
...ig/subcommand/taskresourceattribute/file_config.go	`80.00% <80.00%> (ø)`
...clusterresourceattribute/attrdeleteconfig_flags.go	`20.00% <100.00%> (ø)`
.../clusterresourceattribute/attrfetchconfig_flags.go	`20.00% <100.00%> (ø)`
...clusterresourceattribute/attrupdateconfig_flags.go	`20.00% <100.00%> (ø)`
...and/taskresourceattribute/attrfetchconfig_flags.go	`20.00% <100.00%> (ø)`
cmd/delete/delete.go	`100.00% <100.00%> (ø)`
... and 24 more

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 b511cef...1573ec6. Read the comment docs.

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

katrogan

What do you think about adding a link to the documentation https://docs.flyte.org/en/latest/howto/managing_customizable_resources.html#cluster-resources in the command Usage blocks?

katrogan · 2021-05-13T00:56:59Z

cmd/get/matchable_attribute_util.go

+		if err != nil {
+			return err
+		}
+		// Update the shadow config with the fetched taskResourceAttribute which can then be written to a file which can then be called for an update.


sorry not quite following - why do you undecorate in this call?

The shadow config of TaskResourceAttrFileConfig contains taskResourceAttributes and not the decorated matchable attribute.

type TaskResourceAttrFileConfig struct { Project string `json:"project"` Domain string `json:"domain"` Workflow string `json:"workflow,omitempty"` *admin.TaskResourceAttributes }

This i decided to do , inorder to not have the config file like this.

domain: development project: flytectldemo matching_attributes: Target: taskResourceAttributes: defaults: cpu: "1" memory: 150Mi limits: cpu: "2" memory: 350Mi

And Instead have it like this

domain: development project: flytectldemo defaults: cpu: "1" memory: 150Mi limits: cpu: "2" memory: 350Mi

Now when fetching from the admin we get MatchableAttribute in ProjectDomainAttribute/Workflowattribute
but which one to undecorate depends on what was the resource type passed in the call
And depending on that

The corr. config undecorate would call.
matchingAttribute.GetTaskResourceAttributes()
Or
matchingAttribute.GetClusterResourceAttributes()

This also avoids different structure of ProjectDomainAttributes / WorkflowAttributes as the same struct TaskResourceAttrFileConfig eg . can now be used as both contain TaskResourceAttributes

Let me know what you think

katrogan · 2021-05-13T00:59:20Z

cmd/update/matchable_cluster_resource_attribute.go

+
+Updating to the cluster resource attribute is only available from a generated file. See the get section for generating this file.
+Here the command updates takes the input for cluster resource attributes from the config file cra.yaml
+eg:  content of tra.yaml


Suggested change

eg: content of tra.yaml

eg: content of cra.yaml

katrogan · 2021-05-13T01:00:19Z

cmd/update/matchable_cluster_resource_attribute.go

+
+ flytectl update cluster-resource-attribute -attrFile cra.yaml
+
+Updating cluster resource attribute for project and domain and workflow combination. This will take precedence over any other


might be nice to call it that will completely overwrite any existing custom project and domain and workflow combination attributes

Sure . Let me add this in my followup MR of execution atrributes.

Added this.
Also this will completely overwrite any existing custom project and domain and workflow combination attributes. Would be preferable to do get and generate an attribute file if there is an existing attribute already set and then update it to have new values

pmahindrakar-oss · 2021-05-13T14:07:14Z

What do you think about adding a link to the documentation https://docs.flyte.org/en/latest/howto/managing_customizable_resources.html#cluster-resources in the command Usage blocks?

Yeah makes sense
Added the link in this PR flyteorg/flyte#1026

pmahindrakar-oss requested review from EngHabu, kumare3 and wild-endeavor as code owners May 11, 2021 12:32

pmahindrakar-oss requested a review from katrogan May 11, 2021 12:32

pmahindrakar-oss mentioned this pull request May 11, 2021

Matchable resource support in flytectl for task resource attributes #63

Merged

8 tasks

Base automatically changed from pmahindrakar/customizable-resources to master May 11, 2021 16:55

pmahindrakar-oss force-pushed the pmahindrakar/matchable-cluster-resources branch from 60a8ba2 to ea6b40e Compare May 11, 2021 17:29

Adding commands for task resource attributes support

4efa296

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

pmahindrakar-oss force-pushed the pmahindrakar/matchable-cluster-resources branch 2 times, most recently from 0d6eca9 to 28f0bfd Compare May 12, 2021 06:36

Added cluster resource attribute support

c4a6262

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

pmahindrakar-oss force-pushed the pmahindrakar/matchable-cluster-resources branch from 1f67ba2 to c4a6262 Compare May 12, 2021 11:46

Fixed update command to use cluster default config

1573ec6

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

katrogan approved these changes May 13, 2021

View reviewed changes

pmahindrakar-oss merged commit 451c142 into master May 13, 2021

pmahindrakar-oss deleted the pmahindrakar/matchable-cluster-resources branch May 13, 2021 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added cluster resource attribute support #65

Added cluster resource attribute support #65

pmahindrakar-oss commented May 11, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading

katrogan left a comment

katrogan May 13, 2021

pmahindrakar-oss May 13, 2021 •

edited

Loading

katrogan May 13, 2021

pmahindrakar-oss May 13, 2021

katrogan May 13, 2021

pmahindrakar-oss May 13, 2021

pmahindrakar-oss May 13, 2021

pmahindrakar-oss commented May 13, 2021


		flytectl update cluster-resource-attribute -attrFile cra.yaml

		Updating cluster resource attribute for project and domain and workflow combination. This will take precedence over any other

Added cluster resource attribute support #65

Added cluster resource attribute support #65

Conversation

pmahindrakar-oss commented May 11, 2021 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

codecov bot commented May 12, 2021 • edited Loading

Codecov Report

katrogan left a comment

Choose a reason for hiding this comment

katrogan May 13, 2021

Choose a reason for hiding this comment

pmahindrakar-oss May 13, 2021 • edited Loading

Choose a reason for hiding this comment

katrogan May 13, 2021

Choose a reason for hiding this comment

pmahindrakar-oss May 13, 2021

Choose a reason for hiding this comment

katrogan May 13, 2021

Choose a reason for hiding this comment

pmahindrakar-oss May 13, 2021

Choose a reason for hiding this comment

pmahindrakar-oss May 13, 2021

Choose a reason for hiding this comment

pmahindrakar-oss commented May 13, 2021

pmahindrakar-oss commented May 11, 2021 •

edited

Loading

codecov bot commented May 12, 2021 •

edited

Loading

pmahindrakar-oss May 13, 2021 •

edited

Loading