Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*Chaos: add pre-finalizer and record injecting information in the .Status.Experiment.PodRecords #2509

Merged
merged 10 commits into from
Nov 22, 2021
Merged

*Chaos: add pre-finalizer and record injecting information in the .Status.Experiment.PodRecords #2509

merged 10 commits into from
Nov 22, 2021

Conversation

YangKeao
Copy link
Member

In this PR, the original finalizer was replaced by the .Status.Experiment.PodRecords. The Recover process will iterate the injected pods according to this field, but not the finalizers.

@YangKeao
use pod-records to assert chaos
031f194 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
set log level as info instead of error
4d57037 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 10, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Hexilee
  • cwen0

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-commenter
Copy link

codecov-commenter commented Nov 10, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (release-1.2@f0adeb5). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             release-1.2    #2509   +/-   ##
==============================================
  Coverage               ?   50.64%           
==============================================
  Files                  ?       86           
  Lines                  ?     6360           
  Branches               ?        0           
==============================================
  Hits                   ?     3221           
  Misses                 ?     2844           
  Partials               ?      295

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 f0adeb5...bba7e8e. Read the comment docs.

@YangKeao
fix records for all kinds of chaos
48c8915 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
Copy link
Member Author

/run-e2e-tests

1 similar comment
@YangKeao
Copy link
Member Author

/run-e2e-tests

@YangKeao
fix error handling in the network and io chaos
a365cff 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
Copy link
Member Author

/run-e2e-tests

Hexilee
Hexilee approved these changes Nov 11, 2021
View reviewed changes
Copy link
Member

@Hexilee Hexilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

controllers/twophase/types.go Outdated Show resolved Hide resolved
controllers/twophase/types.go Outdated Show resolved Hide resolved
@Hexilee Hexilee mentioned this pull request Nov 11, 2021
Closed
5 tasks
@YangKeao
remove retry on conflict in twophase controller
bd47dc5 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
use k8s error to assert conflict
aefc3db 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
use updatedClient to fix cache problem
e1943fc 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
Merge remote-tracking branch 'origin/fix-deletion-while-apply' into f…
ca93658 
…ix-deletion-while-apply
@YangKeao
update license
358e177 
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
Copy link
Member Author

/run-e2e-tests

2 similar comments
@YangKeao
Copy link
Member Author

/run-e2e-tests

@YangKeao
Copy link
Member Author

/run-e2e-tests

@YangKeao YangKeao mentioned this pull request Nov 17, 2021
Merged
cwen0
cwen0 reviewed Nov 22, 2021
View reviewed changes
Copy link
Member

@cwen0 cwen0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

controllers/twophase/types.go

chaos.GetMeta().SetFinalizers(finalizer.RemoveFromFinalizer(chaos.GetMeta().GetFinalizers(), common.PreFinalizer))

updateError := r.Update(ctx, chaos)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not handle conflict errors here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it fails, it will be retried by the controller-runtime automatically, as pointed out by @Hexilee here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!

cwen0
cwen0 approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@cwen0 cwen0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cwen0
Copy link
Member

cwen0 commented Nov 22, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: bba7e8e

@ti-chi-bot ti-chi-bot merged commit ec3235b into chaos-mesh:release-1.2 Nov 22, 2021
@cwen0 cwen0 mentioned this pull request Nov 23, 2021
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwen0 cwen0 cwen0 approved these changes

@Hexilee Hexilee Hexilee approved these changes

@Gallardot Gallardot Awaiting requested review from Gallardot

Assignees
No one assigned
Labels
size/XL status/can-merge status/LGT2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@YangKeao @ti-chi-bot @codecov-commenter @cwen0 @Hexilee