Added support for execution queue attribute #66

pmahindrakar-oss · 2021-05-12T13:54:42Z

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

TL;DR

This PR specifically addresses supporting execution queue attributes through flytectl.

GET

Retrieves execution queue attributes for given project,domain combination or additionally with workflow name.

Here the command get execution queue attributes for project flytectldemo and development domain.

flytectl get execution-queue-attribute -p flytectldemo -d development

eg : O/P

{"project":"flytectldemo","domain":"development","tags":["buzz","lightyear","foo","bar"]}

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

flytectl get execution-queue-attribute --attrFile era.yaml

.. code-block:: yaml

	domain: development
	project: flytectldemo
	tags:
	  - foo
	  - buzz
	  - lightyear

UPDATE
Updates execution queue attributes for given project and domain combination or additionally with workflow name.

Updating the execution queue attribute is only available from config file
Here the command updates takes the input for execution queue attributes from the config file era.yaml

eg: content of era.yaml
flytectl update execution-queue-attribute -attrFile era.yaml

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

flytectl delete execution-queue-attribute -p flytectldemo -d development

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

flytectl delete execution-queue-attribute --attrFile era.yaml

.. code-block:: yaml

	domain: development
	project: flytectldemo
	tags:
	  - foo
	  - buzz
	  - lightyear

Deleting execution queue attribute for a workflow
Here the command deletes execution queue attributes for a workflow

flytectl delete execution-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

katrogan · 2021-05-12T16:27:13Z

cmd/config/subcommand/executionqueueattribute/file_config.go

+
+// UnDecorate to uncover ExecutionQueueAttributes.
+func (a *AttrFileConfig) UnDecorate(matchingAttribute *admin.MatchingAttributes) {
+	if matchingAttribute == nil {


when does this happen?

This will happen in case where admin doesn't return an error of empty matching attribute and we try to unwrap the response to get the executionQueueAttribute which wont exist.
This is check is safeguard against it.

This is what is passed to undecorate
workflowAttr.GetAttributes().GetMatchingAttributes()
Where workflowAttr is response from the admin

Ideally it would be preferable to do null checks for both workflowAttr.GetAttributes() and then workflowAttr.GetAttributes().GetMatchingAttributes()

Since the idl can return nil for both

func (m *WorkflowAttributesGetResponse) GetAttributes() *WorkflowAttributes { if m != nil { return m.Attributes } return nil } ... func (m *WorkflowAttributes) GetMatchingAttributes() *MatchingAttributes { if m != nil { return m.MatchingAttributes } return nil }```

oh interesting, so there are cases where we don't return not found but just an empty response? that seems wrong

We might be avoiding this in code path, but the idl supports returning nil hence added a guard .
I haven't seen it during my testing since if there are no attributes its always an error response and if thats the case then we don't call Undecorate.

If the resource is not found we do return an from the ORM layer
https://github.com/flyteorg/flyteadmin/blob/db32bb3810e7490e04ff693eadd26414aa294482/pkg/repositories/gormimpl/resource_repo.go#L114

And which is propogated back to client
https://github.com/flyteorg/flyteadmin/blob/db32bb3810e7490e04ff693eadd26414aa294482/pkg/manager/impl/resources/resource_manager.go#L220

Hence i don't see how we can end up in this state of no error and empty response.

great, just wanted to confirm you hadn't actually seen this behavior! protecting against it since it's technically allowed by the response structure sounds fine

katrogan · 2021-05-12T16:31:31Z

cmd/delete/matchable_execution_queue_attribute.go

+ flytectl delete execution-queue-attribute -p flytectldemo -d development 
+
+
+Deleting execution queue attribute using config file which was used for creating it.


Suggested change

Deleting execution queue attribute using config file which was used for creating it.

Deletes execution queue attribute using config file which was used for creating it.

Thanks done

katrogan · 2021-05-12T16:32:02Z

cmd/delete/matchable_execution_queue_attribute.go

+
+
+Deleting execution queue attribute using config file which was used for creating it.
+Here the command deletes execution queue attributes from the config file cra.yaml


nit: why is this called cra? (copy paste from cluster resources attributes?)

yup . thanks for pointing it out. Fixed it.
Would need a code generator for comments for such cases.

katrogan · 2021-05-12T16:32:42Z

cmd/delete/matchable_execution_queue_attribute.go

+		- lightyear
+
+Deleting execution queue attribute for a workflow
+Here the command execution queue attributes for a workflow


this looks like it's missing a second half?

katrogan · 2021-05-12T16:40:44Z

cmd/get/matchable_execution_queue_attribute.go

+
+ flytectl get execution-queue-attribute -p flytectldemo -d development 
+
+eg : O/P


what is O/P?

Its the output

Reworded it
eg : output from the command

katrogan · 2021-05-12T16:41:43Z

cmd/get/matchable_execution_queue_attribute.go

+Here the command get execution queue attributes for  project flytectldemo and development domain.
+::
+
+ flytectl get execution-queue-attribute -p flytectldemo -d development 


can you add an example with a workflow name too?

Done. Added for this and other attributes aswell.

katrogan · 2021-05-12T16:46:39Z

docs/source/gen/flytectl_delete_execution-queue-attribute.rst

+
+
+Deleting execution queue attribute using config file which was used for creating it.
+Here the command deletes execution queue attributes from the config file cra.yaml


Suggested change

Here the command deletes execution queue attributes from the config file cra.yaml

Here the command deletes execution queue attributes from the config file era.yaml

Done. Thanks

katrogan · 2021-05-12T16:47:04Z

docs/source/gen/flytectl_delete_execution-queue-attribute.rst

+
+Deleting execution queue attribute using config file which was used for creating it.
+Here the command deletes execution queue 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


Can we note that the tags are optional in the file (since they're unread)?

Done.
Tags are optional in the file as they are unread during the delete command but can be kept as the same file can be used for get, update or delete
Added to other commands aswell

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

codecov · 2021-05-13T06:29:43Z

Codecov Report

Merging #66 (de5efe9) into master (451c142) will increase coverage by 0.37%.
The diff coverage is 63.80%.

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   57.94%   58.31%   +0.37%     
==========================================
  Files          68       75       +7     
  Lines        1548     1653     +105     
==========================================
+ Hits          897      964      +67     
- Misses        596      633      +37     
- Partials       55       56       +1

Flag	Coverage Δ
unittests	`58.31% <63.80%> (+0.37%)`	⬆️

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

Impacted Files	Coverage Δ
cmd/delete/matchable_cluster_resource_attribute.go	`100.00% <ø> (ø)`
cmd/delete/matchable_task_resource_attribute.go	`100.00% <ø> (ø)`
cmd/get/matchable_cluster_resource_attribute.go	`100.00% <ø> (ø)`
cmd/get/matchable_task_resource_attribute.go	`100.00% <ø> (ø)`
cmd/update/matchable_cluster_resource_attribute.go	`100.00% <ø> (ø)`
cmd/update/matchable_task_resource_attribute.go	`100.00% <ø> (ø)`
.../executionqueueattribute/attrdeleteconfig_flags.go	`20.00% <20.00%> (ø)`
...d/executionqueueattribute/attrfetchconfig_flags.go	`20.00% <20.00%> (ø)`
.../executionqueueattribute/attrupdateconfig_flags.go	`20.00% <20.00%> (ø)`
.../subcommand/executionqueueattribute/file_config.go	`80.00% <80.00%> (ø)`
... and 13 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 451c142...de5efe9. Read the comment docs.

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

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

pmahindrakar-oss force-pushed the pmahindrakar/matchable-execution-queue-attributes branch from 34f46fe to 0f2030b Compare May 12, 2021 14:00

pmahindrakar-oss requested a review from katrogan May 12, 2021 14:29

katrogan previously approved these changes May 12, 2021

View reviewed changes

Base automatically changed from pmahindrakar/matchable-cluster-resources to master May 13, 2021 04:56

pmahindrakar-oss added 5 commits May 13, 2021 10:28

Adding commands for task resource attributes support

e18a85b

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

Added cluster resource attribute support

44eb8dc

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

Added support for execution queue attribute

c30eb1f

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

Addressed review comments

f02592f

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

Rebased and fixed tests

175ed83

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

pmahindrakar-oss dismissed katrogan’s stale review via 175ed83 May 13, 2021 06:25

pmahindrakar-oss force-pushed the pmahindrakar/matchable-execution-queue-attributes branch from 0f2030b to 175ed83 Compare May 13, 2021 06:26

Fixed docs

de5efe9

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

pmahindrakar-oss requested a review from katrogan May 13, 2021 06:56

katrogan approved these changes May 13, 2021

View reviewed changes

pmahindrakar-oss merged commit f0e6770 into master May 14, 2021

pmahindrakar-oss deleted the pmahindrakar/matchable-execution-queue-attributes branch May 14, 2021 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for execution queue attribute #66

Added support for execution queue attribute #66

pmahindrakar-oss commented May 12, 2021 •

edited

Loading

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

katrogan May 13, 2021

pmahindrakar-oss May 14, 2021 •

edited

Loading

katrogan May 14, 2021

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

pmahindrakar-oss May 13, 2021

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

katrogan May 12, 2021

pmahindrakar-oss May 13, 2021

codecov bot commented May 13, 2021 •

edited

Loading

		flytectl delete execution-queue-attribute -p flytectldemo -d development


		Deleting execution queue attribute using config file which was used for creating it.

	Deleting execution queue attribute using config file which was used for creating it.
	Deletes execution queue attribute using config file which was used for creating it.



		Deleting execution queue attribute using config file which was used for creating it.
		Here the command deletes execution queue attributes from the config file cra.yaml


		flytectl get execution-queue-attribute -p flytectldemo -d development

		eg : O/P

Added support for execution queue attribute #66

Added support for execution queue attribute #66

Conversation

pmahindrakar-oss commented May 12, 2021 • edited Loading

TL;DR

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

Choose a reason for hiding this comment

pmahindrakar-oss May 14, 2021 • 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

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

codecov bot commented May 13, 2021 • edited Loading

Codecov Report

pmahindrakar-oss commented May 12, 2021 •

edited

Loading

pmahindrakar-oss May 14, 2021 •

edited

Loading

codecov bot commented May 13, 2021 •

edited

Loading