Implement StatusCheck in Workflow #3067

iguoyr · 2022-03-28T04:45:28Z

Signed-off-by: SiyuChen ryougi201@gmail.com

What problem does this PR solve?

Close #3048

What's changed and how it works?

Related changes

Need to update chaos-mesh/website
Need to update Dashboard UI
Need to cheery-pick to release branches
- release-2.1
- release-2.0

Checklist

Tests

Unit test
E2E test
No code
Manual test (add steps below)

Side effects

Breaking backward compatibility

Release note

Please add a release note.

You can safely ignore this section if you don't think this PR needs a release note.

DCO

If you find the DCO check fails, please run commands like below (Depends on the actual situations. For example, if the failed commit isn't the most recent) to fix it:

git commit --amend --signoff
git push --force

Signed-off-by: SiyuChen <ryougi201@gmail.com>

ti-chi-bot · 2022-03-28T04:45:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

STRRL
WangXiangUSTC

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: SiyuChen <ryougi201@gmail.com>

…rkflow Signed-off-by: SiyuChen <ryougi201@gmail.com>

codecov · 2022-04-01T06:07:16Z

Codecov Report

Merging #3067 (483f8a1) into master (4cfc1ea) will decrease coverage by 0.67%.
The diff coverage is 23.89%.

@@            Coverage Diff             @@
##           master    #3067      +/-   ##
==========================================
- Coverage   37.10%   36.42%   -0.68%     
==========================================
  Files         106      109       +3     
  Lines        9634    10044     +410     
==========================================
+ Hits         3575     3659      +84     
- Misses       5736     6052     +316     
- Partials      323      333      +10

Impacted Files	Coverage Δ
pkg/workflow/controllers/statuscheck_reconciler.go	`7.57% <7.57%> (ø)`
controllers/utils/recorder/workflow.go	`15.38% <12.19%> (-1.14%)`	⬇️
pkg/workflow/controllers/abort_node_reconciler.go	`15.11% <15.11%> (ø)`
.../workflow/controllers/abort_workflow_reconciler.go	`48.97% <48.97%> (ø)`
.../workflow/controllers/workflow_entry_reconciler.go	`49.18% <66.66%> (-3.73%)`	⬇️
pkg/workflow/controllers/bootstrap.go	`76.19% <74.28%> (-0.74%)`	⬇️
pkg/workflow/controllers/new_node.go	`69.15% <100.00%> (+0.58%)`	⬆️
pkg/workflow/controllers/utils.go	`86.04% <100.00%> (+0.33%)`	⬆️
pkg/selector/generic/mode.go	`25.64% <0.00%> (-2.57%)`	⬇️
... and 1 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 4cfc1ea...483f8a1. Read the comment docs.

api/v1alpha1/statuscheck_types.go

api/v1alpha1/workflownode_types.go

pkg/workflow/controllers/abort_node_reconciler.go

WangXiangUSTC · 2022-04-08T08:51:49Z

pkg/workflow/controllers/abort_node_reconciler.go

+// Reconcile watches `WorkflowNodes`, if:
+// 1. the abort condition is `False`, just return.
+// 2. the abort condition is `True`, the node is not `TypeStatusCheck`, it will propagate abort condition to children nodes.
+// 3. the abort condition is `True`, the node is `TypeStatusCheck`, it will add abort annotation to the parent workflow.


Does the StatusCheck's parent continue to add abort annotation to its parent?

No, the abort 'flow' is top-down, only the parent node will abort the children nodes.

So I add another reconciler (AbortWorkflowReconciler), it will set the abort condition of the entry node to True.

pkg/workflow/controllers/abort_workflow_reconciler.go

Signed-off-by: SiyuChen <ryougi201@gmail.com>

WangXiangUSTC · 2022-04-13T03:21:47Z

pkg/workflow/controllers/statuscheck_reconciler.go

+		for _, item := range statusChecks[1:] {
+			statusCheckToRemove = append(statusCheckToRemove, item.GetName())
+		}
+		it.logger.Info("removing duplicated StatusCheck",


why exist some duplicate status checks?

It's the same reason with #3112 🤣.

maybe we can update duplicated to redundant later

api/v1alpha1/workflownode_types.go

Signed-off-by: SiyuChen <ryougi201@gmail.com>

STRRL

IMO, the abort should recover all the injected chaos. How do you think about it ?

STRRL · 2022-04-12T09:23:53Z

api/v1alpha1/workflow_types.go

+	// Abort describe whether to abort the workflow when the failure threshold of StatusCheck is exceeded. Only used when Type is TypeStatusCheck.
+	// +optional
+	Abort bool `json:"abort,omitempty"`


Suggested change

// Abort describe whether to abort the workflow when the failure threshold of StatusCheck is exceeded. Only used when Type is TypeStatusCheck.

// +optional

Abort bool `json:"abort,omitempty"`

// Abort describe whether to abort the workflow when the failure threshold of StatusCheck is exceeded. Only used when Type is TypeStatusCheck.

// +optional

AbortWithStatusCheck bool `json:"abortWithStatusCheck,omitempty"`

I suggest using more words as the name of this field. Only one word in a plain workflow.yaml without these comments might bring confusion.

Updated! PTAL

api/v1alpha1/workflownode_types.go

STRRL · 2022-04-14T03:29:30Z

IMO, the abort should recover all the injected chaos. How do you think about it ?

Sorry, I missed changes on WorkflowNodeFinished in pkg/workflow/controllers/utils.go. This behavior has been built by it.

Signed-off-by: SiyuChen <ryougi201@gmail.com>

WangXiangUSTC

LGTM

STRRL

LGTM!

STRRL · 2022-04-14T09:18:33Z

/merge

ti-chi-bot · 2022-04-14T09:18:35Z

This pull request has been accepted and is ready to merge.

Commit hash: c769b4b

chore: add status check on workflow

3012f92

Signed-off-by: SiyuChen <ryougi201@gmail.com>

ti-chi-bot added the do-not-merge/work-in-progress label Mar 28, 2022

ti-chi-bot requested review from cwen0 and YangKeao March 28, 2022 04:45

ti-chi-bot added size/XXL needs-rebase labels Mar 28, 2022

chore: status check in workflow

48ae859

Signed-off-by: SiyuChen <ryougi201@gmail.com>

iguoyr marked this pull request as ready for review April 1, 2022 04:54

ti-chi-bot removed the do-not-merge/work-in-progress label Apr 1, 2022

Merge remote-tracking branch 'upstream/master' into statuscheck-in-wo…

0f553f3

…rkflow Signed-off-by: SiyuChen <ryougi201@gmail.com>

ti-chi-bot removed the needs-rebase label Apr 1, 2022

iguoyr requested review from a team April 2, 2022 03:14

WangXiangUSTC reviewed Apr 8, 2022

View reviewed changes

iguoyr added 2 commits April 12, 2022 14:59

chore: handle error

e001e28

Signed-off-by: SiyuChen <ryougi201@gmail.com>

chore: fix

04f45e4

Signed-off-by: SiyuChen <ryougi201@gmail.com>

WangXiangUSTC reviewed Apr 13, 2022

View reviewed changes

chore: typo

8a172af

Signed-off-by: SiyuChen <ryougi201@gmail.com>

iguoyr requested a review from a team April 13, 2022 07:23

STRRL reviewed Apr 13, 2022

View reviewed changes

chore: add comments

c769b4b

Signed-off-by: SiyuChen <ryougi201@gmail.com>

WangXiangUSTC approved these changes Apr 14, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label Apr 14, 2022

STRRL approved these changes Apr 14, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Apr 14, 2022

ti-chi-bot added the status/can-merge label Apr 14, 2022

Merge branch 'master' into statuscheck-in-workflow

483f8a1

ti-chi-bot merged commit 92c0311 into chaos-mesh:master Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement StatusCheck in Workflow #3067

Implement StatusCheck in Workflow #3067

iguoyr commented Mar 28, 2022 •

edited

ti-chi-bot commented Mar 28, 2022 •

edited

codecov bot commented Apr 1, 2022 •

edited

WangXiangUSTC Apr 8, 2022

iguoyr Apr 11, 2022

iguoyr Apr 11, 2022

WangXiangUSTC Apr 13, 2022

iguoyr Apr 13, 2022

WangXiangUSTC Apr 14, 2022

STRRL left a comment

STRRL Apr 12, 2022

iguoyr Apr 14, 2022

STRRL commented Apr 14, 2022

WangXiangUSTC left a comment

STRRL left a comment

STRRL commented Apr 14, 2022

ti-chi-bot commented Apr 14, 2022

Implement StatusCheck in Workflow #3067

Implement StatusCheck in Workflow #3067

Conversation

iguoyr commented Mar 28, 2022 • edited

What problem does this PR solve?

What's changed and how it works?

Related changes

Checklist

Release note

DCO

ti-chi-bot commented Mar 28, 2022 • edited

codecov bot commented Apr 1, 2022 • edited

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

Choose a reason for hiding this comment

STRRL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

STRRL commented Apr 14, 2022

WangXiangUSTC left a comment

Choose a reason for hiding this comment

STRRL left a comment

Choose a reason for hiding this comment

STRRL commented Apr 14, 2022

ti-chi-bot commented Apr 14, 2022

iguoyr commented Mar 28, 2022 •

edited

ti-chi-bot commented Mar 28, 2022 •

edited

codecov bot commented Apr 1, 2022 •

edited