add more status for record #3170

xlgao-zju · 2022-04-21T05:17:26Z

What problem does this PR solve?

Close #3127

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

CHANGELOG

I have updated the CHANGELOG.md
I have labeled this PR with "no-need-update-changelog"

Tests

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

create experiment, pause and resume it

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

ti-chi-bot · 2022-04-21T05:17:28Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

STRRL
YangKeao

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.

codecov · 2022-04-21T05:31:09Z

Codecov Report

Merging #3170 (653c3e5) into master (22a1cf5) will decrease coverage by 0.05%.
The diff coverage is 60.00%.

❗ Current head 653c3e5 differs from pull request most recent head a8721fb. Consider uploading reports for the commit a8721fb to get more accurate results

@@            Coverage Diff             @@
##           master    #3170      +/-   ##
==========================================
- Coverage   41.04%   40.98%   -0.06%     
==========================================
  Files         161      164       +3     
  Lines       13640    13794     +154     
==========================================
+ Hits         5598     5653      +55     
- Misses       7629     7715      +86     
- Partials      413      426      +13

Impacted Files	Coverage Δ
api/v1alpha1/common_types.go	`0.00% <0.00%> (ø)`
controllers/common/step.go	`0.00% <0.00%> (ø)`
controllers/common/fx.go	`59.04% <77.77%> (+5.88%)`	⬆️
controllers/statuscheck/controller.go	`79.26% <0.00%> (-4.88%)`	⬇️
pkg/workflow/controllers/abort_node_reconciler.go	`15.11% <0.00%> (-3.49%)`	⬇️
pkg/selector/generic/mode.go	`25.64% <0.00%> (-2.57%)`	⬇️
.../workflow/controllers/workflow_entry_reconciler.go	`54.05% <0.00%> (-2.17%)`	⬇️
...g/workflow/controllers/parallel_node_reconciler.go	`60.12% <0.00%> (-1.90%)`	⬇️
pkg/workflow/controllers/serial_node_reconciler.go	`59.58% <0.00%> (-1.56%)`	⬇️
pkg/workflow/controllers/statuscheck_reconciler.go	`7.57% <0.00%> (-1.52%)`	⬇️
... and 6 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 6111432...a8721fb. Read the comment docs.

xlgao-zju · 2022-04-21T08:22:40Z

/cc @cwen0 @STRRL @YangKeao @Hexilee
for review

STRRL

rest LGTM!

api/v1alpha1/common_types.go

STRRL · 2022-04-22T04:20:24Z

after discussing with @xlgao-zju, the capacity of events might be a problem now

There are two reasons that would make `events increase fast:

"one-to-many" chaos-records relation; when a chaos experiment uses a selector for selecting a big mount of targets, there would be many "records" in the status
uncontrolled retry/reconcile when failed to Apply/Recover; we would always requeue the reconcile request if the operation failed, and only using the default rate limiter of controller-runtime. So there would be many events with "similar" messages appearing in a short time.

Maybe we need an aggregation or rotate policy in the future. I still stay positive about the effect of this problem, I think we do not need to consider it in this PR.

How do you think about it? @YangKeao @Hexilee @xlgao-zju

xlgao-zju · 2022-04-24T10:04:52Z

at the beginning, I append the events, when we apply failed. and then I get too many error messages. and the reason is:

apply failed -> update the status -> the update will let the object be reconciled -> apply failed

since this is circle, so we will get so so many record.events 😂

in order to avoid thi bug, I did those:

make the common CRD's status as a sub resource(or modify the status will increase the generation of the resource)
for common CRDs, use WithEventFilter to filter the events, pick up the event only when label/annotation/spec changes
for the CRDs controlled by common CRDs, there is no filter. since when the child CR status changes, we need to reconcile the according parent CR
add another condition step after record step, since if we will not reconcile the object, if only the status changes, the conditions calculated before record step may not be correct after the record step

the PR is ready for review
@STRRL @YangKeao @Hexilee

xlgao-zju · 2022-05-06T03:11:55Z

kindly ping @STRRL @YangKeao @Hexilee for review

STRRL · 2022-05-06T03:27:00Z

PTAL @YangKeao , this PR introduces Status Subresources for all the chaos CRD. But I forgot why do we NOT use status subresources before. Maybe after navirna we could use subresrouce without any blocking?🤔

STRRL · 2022-05-06T03:35:28Z

after discussing with @xlgao-zju, the capacity of events might be a problem now

There are two reasons that would make `events increase fast:

"one-to-many" chaos-records relation; when a chaos experiment uses a selector for selecting a big mount of targets, there would be many "records" in the status

uncontrolled retry/reconcile when failed to Apply/Recover; we would always requeue the reconcile request if the operation failed, and only using the default rate limiter of controller-runtime. So there would be many events with "similar" messages appearing in a short time.

Maybe we need an aggregation or rotate policy in the future. I still stay positive about the effect of this problem, I think we do not need to consider it in this PR.

How do you think about it? @YangKeao @Hexilee @xlgao-zju

it seems this issue is NOT resolved properly, I could still reproduce it with these manifests:

---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx
spec:
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - image: nginx:latest
        imagePullPolicy: Always
        name: nginx
        resources: {}
---
apiVersion: chaos-mesh.org/v1alpha1
kind: IOChaos
metadata:
  name: io-delay-example
spec:
  action: latency
  mode: all
  selector:
    labelSelectors:
      app: nginx
  volumePath: /not-existed
  path: /not-existed/**/*
  delay: "10ms"
  percent: 10

STRRL · 2022-05-06T03:42:21Z

It seems the predicates DO work. But the soo many events appended and resoruceVersion increased near to thousands. I am still trying to figure it out with logs. 🤔

controllers/common/fx.go

xlgao-zju · 2022-05-09T10:59:36Z

@STRRL @YangKeao I have remove the "sub resource" part. and add a predicate to skip the events which ONLY object.status.experiment.records[].events changed

PTAL

STRRL

rest LGTM!

PTAL @xlgao-zju

controllers/common/step.go

controllers/common/fx.go

xlgao-zju · 2022-05-10T07:34:37Z

@STRRL updated

YangKeao

Rest LGTM

controllers/common/fx.go

Signed-off-by: xianglingao <xianglingao@tencent.com>

xlgao-zju · 2022-05-10T08:07:56Z

@YangKeao Updated

YangKeao

LGTM

STRRL

LGTM!

STRRL · 2022-05-11T03:31:37Z

/merge

ti-chi-bot · 2022-05-11T03:31:39Z

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

Commit hash: a8721fb

ti-chi-bot added contribution size/XXL labels Apr 21, 2022

ti-chi-bot requested review from YangKeao and yeya24 April 21, 2022 05:17

xlgao-zju force-pushed the more-status branch from a1d58fa to 279b18f Compare April 21, 2022 07:51

ti-chi-bot requested review from cwen0, Hexilee and STRRL April 21, 2022 08:22

xlgao-zju force-pushed the more-status branch 2 times, most recently from be46497 to 28e9c92 Compare April 21, 2022 11:27

STRRL reviewed Apr 22, 2022

View reviewed changes

api/v1alpha1/common_types.go Outdated Show resolved Hide resolved

api/v1alpha1/common_types.go Outdated Show resolved Hide resolved

api/v1alpha1/common_types.go Outdated Show resolved Hide resolved

STRRL mentioned this pull request Apr 22, 2022

Backoff with the failure of chaos apply and recover #3176

Open

xlgao-zju force-pushed the more-status branch 5 times, most recently from 212bdd5 to 5315634 Compare April 24, 2022 09:42

xlgao-zju requested a review from STRRL April 24, 2022 13:29

ti-chi-bot added the needs-rebase label Apr 27, 2022

xlgao-zju force-pushed the more-status branch from 5315634 to cecd1f8 Compare May 5, 2022 02:59

ti-chi-bot removed the needs-rebase label May 5, 2022

ti-chi-bot added the size/XXL label May 9, 2022

sonatype-lift bot reviewed May 9, 2022

View reviewed changes

controllers/common/fx.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed May 9, 2022

View reviewed changes

controllers/common/fx.go Outdated Show resolved Hide resolved

xlgao-zju force-pushed the more-status branch from 653c3e5 to 29baafa Compare May 9, 2022 09:41

xlgao-zju requested a review from STRRL May 9, 2022 10:55

xlgao-zju force-pushed the more-status branch from 29baafa to 397b963 Compare May 9, 2022 11:01

STRRL reviewed May 10, 2022

View reviewed changes

controllers/common/step.go Outdated Show resolved Hide resolved

controllers/common/fx.go Outdated Show resolved Hide resolved

xlgao-zju force-pushed the more-status branch from 397b963 to b585de5 Compare May 10, 2022 07:03

xlgao-zju requested a review from STRRL May 10, 2022 07:05

YangKeao reviewed May 10, 2022

View reviewed changes

controllers/common/fx.go Outdated Show resolved Hide resolved

more status

a8721fb

Signed-off-by: xianglingao <xianglingao@tencent.com>

xlgao-zju force-pushed the more-status branch from b585de5 to a8721fb Compare May 10, 2022 08:03

YangKeao approved these changes May 10, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label May 10, 2022

STRRL approved these changes May 11, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels May 11, 2022

ti-chi-bot added the status/can-merge label May 11, 2022

ti-chi-bot merged commit 10df92f into chaos-mesh:master May 11, 2022

xlgao-zju deleted the more-status branch May 11, 2022 03:54

xlgao-zju mentioned this pull request May 26, 2022

display the experiment records details on dashboard #3303

Open

YangKeao mentioned this pull request May 31, 2022

Promote "Xianglin Gao" to committer #3317

Merged

Andrewmatilde mentioned this pull request Jun 1, 2022

How to determine the completed status of an experiment #3316

Closed

STRRL mentioned this pull request Jun 24, 2022

Whether chaos-mesh events can be collected by Prometheus #3383

Closed

STRRL mentioned this pull request Nov 8, 2022

chaos-controller-manager: memory keeps increasing #3761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more status for record #3170

add more status for record #3170

xlgao-zju commented Apr 21, 2022 •

edited

ti-chi-bot commented Apr 21, 2022 •

edited

codecov bot commented Apr 21, 2022 •

edited

xlgao-zju commented Apr 21, 2022

STRRL left a comment

STRRL commented Apr 22, 2022

xlgao-zju commented Apr 24, 2022

xlgao-zju commented May 6, 2022

STRRL commented May 6, 2022

STRRL commented May 6, 2022

STRRL commented May 6, 2022 •

edited

xlgao-zju commented May 9, 2022

STRRL left a comment

xlgao-zju commented May 10, 2022

YangKeao left a comment

xlgao-zju commented May 10, 2022

YangKeao left a comment

STRRL left a comment

STRRL commented May 11, 2022

ti-chi-bot commented May 11, 2022

add more status for record #3170

add more status for record #3170

Conversation

xlgao-zju commented Apr 21, 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 Apr 21, 2022 • edited

codecov bot commented Apr 21, 2022 • edited

Codecov Report

xlgao-zju commented Apr 21, 2022

STRRL left a comment

Choose a reason for hiding this comment

STRRL commented Apr 22, 2022

xlgao-zju commented Apr 24, 2022

xlgao-zju commented May 6, 2022

STRRL commented May 6, 2022

STRRL commented May 6, 2022

STRRL commented May 6, 2022 • edited

xlgao-zju commented May 9, 2022

STRRL left a comment

Choose a reason for hiding this comment

xlgao-zju commented May 10, 2022

YangKeao left a comment

Choose a reason for hiding this comment

xlgao-zju commented May 10, 2022

YangKeao left a comment

Choose a reason for hiding this comment

STRRL left a comment

Choose a reason for hiding this comment

STRRL commented May 11, 2022

ti-chi-bot commented May 11, 2022

xlgao-zju commented Apr 21, 2022 •

edited

ti-chi-bot commented Apr 21, 2022 •

edited

codecov bot commented Apr 21, 2022 •

edited

STRRL commented May 6, 2022 •

edited