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.

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

Support force clean finalizer in networkchaos/netem #415

Merged
merged 4 commits into from
May 8, 2020

Conversation

yujunz
Copy link
Contributor

@yujunz yujunz commented Apr 11, 2020

What problem does this PR solve?

#227

What is changed and how does it work?

Force clean finalizer

Check List

Tests

  • Manual test (see examples/clean-finalizer)

Code changes

  • Has Go code change

Does this PR introduce a user-facing change?:

Support forcing to clean finalizer with annotation

@yujunz yujunz force-pushed the force-cleanup branch 2 times, most recently from f99dbe5 to d62b147 Compare April 27, 2020 12:54
@@ -29,6 +29,13 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

const (
// AnnotationCleanFinalizer key
AnnotationCleanFinalizer = `cleanFinalizer`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may need a common prefix for chaos-mesh annotations. @cwen0

Copy link
Member

@cwen0 cwen0 May 7, 2020

Choose a reason for hiding this comment

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

LGTM

@yujunz yujunz changed the title WIP: Support force cleanup in networkchaos Support force cleanup in networkchaos May 6, 2020
cwen0
cwen0 previously approved these changes May 7, 2020
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


1. Run `setup.sh` to simulate a condition of unable to clean finalizers.
2. Press `Ctrl + C` to break
3. Run `test.sh` to annotate chaos resource and force cleaning finalizers.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest demonstrating the detailed operation for force cleanup instead of executing a script. That would be much helpful for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote the example as an e2e test, not user oriented feature demonstration.

@@ -29,6 +29,13 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

const (
// AnnotationCleanFinalizer key
AnnotationCleanFinalizer = `chaos-mesh/cleanFinalizer`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we should define the annotation key prefix for chaos-mesh. What about chaosmesh.pingcap.com/ @zhouqiang-cl @cwen0

}

networkchaos.Finalizers = utils.RemoveFromFinalizer(networkchaos.Finalizers, key)
}

return nil
if networkchaos.Annotations[common.AnnotationCleanFinalizer] == common.AnnotationCleanFinalizerForced {
r.Log.Info("Force cleanup all finalizers")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the target chaos object name/namespace/type in the log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@yujunz yujunz changed the title Support force cleanup in networkchaos Support force cleanup in networkchaos/netem May 7, 2020
@yujunz yujunz changed the title Support force cleanup in networkchaos/netem Support force clean finalizer in networkchaos/netem May 7, 2020
@cwen0
Copy link
Member

cwen0 commented May 8, 2020

/run-e2e-test

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 May 8, 2020

@Yisaer PTAL

@cwen0
Copy link
Member

cwen0 commented May 8, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

@yujunz merge failed.

@yeya24
Copy link
Contributor

yeya24 commented May 8, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

@yujunz merge failed.

@yeya24
Copy link
Contributor

yeya24 commented May 8, 2020

/run-e2e-test

@yeya24
Copy link
Contributor

yeya24 commented May 8, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

@yujunz merge failed.

@yeya24
Copy link
Contributor

yeya24 commented May 8, 2020

I don't know why merge failed after 2 minutes. But all tests are passed. I will merge it now.

@yeya24 yeya24 merged commit 7df0492 into chaos-mesh:master May 8, 2020
sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021
* Support force cleanup in networkchaos

* Add example of force clean finalizer

* Amend according to review comments

Co-authored-by: pingcap-github-bot <sre-bot@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants