Support duration chaos for Network Chaos #198

Yisaer · 2020-02-05T13:13:42Z

What problem does this PR solve?

#171
Make Network chaos support duration chaos feature.

What is changed and how does it work?

Making each kind of Networkchaos implement common controller interface. I have tested netem and partiton chaos locally in kind and works fine.

Check List

Tests

Manual test (add detailed scripts or steps below)

Code changes

Has Go code change

Side effects

None

Related changes

Need to update the documentation

Does this PR introduce a user-facing change?:

Support duration chaos for network chaos by omitting the scheduler and duration.

codecov-io · 2020-02-05T13:15:38Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3aee6ba). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #198   +/-   ##
=========================================
  Coverage          ?   43.64%           
=========================================
  Files             ?       18           
  Lines             ?      685           
  Branches          ?        0           
=========================================
  Hits              ?      299           
  Misses            ?      353           
  Partials          ?       33

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 3aee6ba...3379f38. Read the comment docs.

Yisaer · 2020-02-05T14:22:38Z

controllers/common/types.go

@@ -80,6 +80,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 			r.Log.Error(err, "failed to recover chaos")
 			return ctrl.Result{Requeue: true}, nil
 		}
+		return ctrl.Result{}, nil


This is a bug during handling the recover logic in common chaos controller, I fixed in this request when I found it.

do we need r.Update(ctx, chaos) here?

do we need r.Update(ctx, chaos) here?

good catch. I adjust the common reconciler.

zhouqiang-cl · 2020-02-06T01:54:59Z

controllers/common/networkchaos/netem/types.go

+import (
+	"context"
+
+	"github.com/pingcap/chaos-mesh/controllers/networkchaos/netem"


group this import?

I execute make groupimports locally and there is no change. Is this something wrong?

It is wield 😯

It seems to be default behavior golang/go#27365 (comment) and golang/go#26846 (comment) you need to put imports together so goimport will treat them as a group and sort within the group. Otherwise it assumes you put them into different group for some reason, e.g. third party, protobuf etc.

zhouqiang-cl · 2020-02-06T02:00:03Z

controllers/networkchaos/types.go

+		r.Log.Error(err, fmt.Sprintf("unable to get podchaos[%s/%s]'s duration", networkchaos.Namespace, networkchaos.Name))
+		return ctrl.Result{}, nil
+	}
+	if scheduler == nil && duration == nil {


what if duration != nil

@zhouqiang-cl Networkchaos controller only handle the situation when scheduler and duration are both defined or omitted. For the other situation, it would log the error and return the empty response.

More over, I also think Adding validation admission webhook for each chaos kind to check the scheduler and duration situation is necessary as I mentioned in #171

zhouqiang-cl · 2020-02-06T04:11:45Z

@YangKeao @cwen0 PTAL

zhouqiang-cl · 2020-02-06T08:15:39Z

@at15 Can you take a look at other part of this PR too

at15

@zhouqiang-cl I added some nitpicking, I am not familiar with the code base, just came across the PR in slack ...

at15 · 2020-02-06T08:48:42Z

controllers/networkchaos/types.go

 	}
 }
+
+func (r *Reconciler) defaultResponse(networkchaos *v1alpha1.NetworkChaos) ctrl.Result {


Maybe rename it to invalidAction or ignoreInvalidAction because it is logging error for invalid input (that should get caught in admission webhook?) instead of doing default chaos.

at15 · 2020-02-06T08:53:15Z

controllers/twophase/networkchaos/netem/types.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package netem


It would be better to have package level go doc, i.e. https://pkg.go.dev/github.com/pingcap/chaos-mesh does not tell much when you look at it. Also some package name might need some explanation like netem is for ... network event manager? ...

Nice advice, I think it should be recorded as an new issue.

controllers/twophase/networkchaos/netem/types.go

at15 · 2020-02-06T09:34:27Z

controllers/networkchaos/types.go


 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/pingcap/chaos-mesh/api/v1alpha1"
+	commonNetem "github.com/pingcap/chaos-mesh/controllers/common/networkchaos/netem"


I find the common and twwphase wrapping and renaming a bit duplicated. Is it possible to eliminate them? I am not very familiar with the code base ... it seems we have two set of interfaces under common and twophase. And the wrappers are almost same (except for the last parameter)

// under common/types // InnerCommonReconcile used in common chaos reconcile type InnerCommonReconcile interface { Apply(ctx context.Context, req ctrl.Request, chaos InnerCommonObject) error Recover(ctx context.Context, req ctrl.Request, chaos InnerCommonObject) error Object() InnerCommonObject } // under twophase/types type InnerReconciler interface { Apply(ctx context.Context, req ctrl.Request, chaos InnerObject) error Recover(ctx context.Context, req ctrl.Request, chaos InnerObject) error Object() InnerObject } // under common/podfailure // CommonReconciler is reconciler for podfailure type CommonReconciler struct { *podfailure.Reconciler } // Apply would perform common chaos for podchaos func (r *CommonReconciler) Apply(ctx context.Context, req ctrl.Request, chaos common.InnerCommonObject) error { return r.Perform(ctx, req, chaos) } // under twophase/podfailure // TwoPhaseReconciler reconcile the networkchaos type TwoPhaseReconciler struct { *podfailure.Reconciler } // Apply implement InnerReconciler.Apply func (r *TwoPhaseReconciler) Apply(ctx context.Context, req ctrl.Request, chaos twophase.InnerObject) error { return r.Perform(ctx, req, chaos) }

Instead of having a layout where we duplicate package names under common and twophase

- podfailure - netem - common - podfailure - netem - twophase - podfaillure - netem

We can define those wrappers in podfailure and netem directly with new factory func like NewCommonReconciler, to avoid having two extra packages for each chaos and use netem.NewCommonReconciler instead of commonNetem.NewReconciler. This removes subpackage from common and twophase packages and solves the renaming problem.

// podfailure // NewReconciler would create new reconciler for podfailure chaos func NewReconciler(c client.Client, log logr.Logger, req ctrl.Request) *Reconciler { return &Reconciler{Client: c, Log: log} } func NewCommonReconciler(...) *CommonReconciler { } func NewTwoPhaseReconciler(...) *TwoPhaseReconciler { }

btw: the wrappers should be generated

Deeply agree with you.

fixed in #201

controllers/networkchaos/types.go

zhouqiang-cl · 2020-02-06T10:22:35Z

@at15 thank you very much❤️

Yisaer · 2020-02-06T12:03:43Z

Thanks a lot for the code review.
Current layout is a bit of complex and I can't agree it anymore. As @at15 's advice, I would refactor the current layout first to make the whole controller structure easier.

The Duration Networkchaos feature is suspended until #201 is merged.

zhouqiang-cl · 2020-02-07T09:18:17Z

@Yisaer please solve the conflict

…chaos_for_network_chaos

zhouqiang-cl · 2020-02-07T10:52:39Z

@cwen0 @YangKeao PTAL

Yisaer · 2020-02-07T10:55:21Z

controllers/common/types.go

+	} else {
+		// Start failure action
+		r.Log.Info("Performing Action")


Adjust the common reconciler, make sure the chaos object would be udated after Performing or Recovering.

mahjonp

LGTM.

controllers/common/types.go

cwen0 · 2020-02-07T12:21:00Z

controllers/common/types.go

-	err = r.Apply(ctx, req, chaos)
-	if err != nil {
-		r.Log.Error(err, "failed to apply chaos action")
+			updateError := retry.RetryOnConflict(retry.DefaultRetry, func() error {


If apply action is failed, we should probably not update the chaos object and recover it. @YangKeao what do you think? Maybe we need a separate pr to fix this issue.

cwen0

LGTM

mahjonp · 2020-02-07T13:00:33Z

controllers/common/types.go

@@ -63,33 +63,32 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 		err = r.Recover(ctx, req, chaos)
 		if err != nil {
 			r.Log.Error(err, "failed to recover chaos")
-			return ctrl.Result{Requeue: true}, nil
+			return ctrl.Result{}, nil


emm, so we won't retry if the recovery fails? I think it will cause more unrecovered chaos actions.

I agreed. What's your opinion? @cwen0

And could we set a field like Status, let user be aware of it when observer chaos result?

yes, requeue is better and if recover action was failed, we also can block this delete action. @mahjonp There is already a status field in chaos object, but we have not used it well.

so, this pr's handle is ok?

Yisaer added 2 commits February 5, 2020 21:09

support duration chaos for networkchaos

c2a0e45

fix duration

6998afd

add comment

21737d0

cwen0 added status/WIP component/operator labels Feb 5, 2020

fix common controller logic

9a8e552

Yisaer commented Feb 5, 2020

View reviewed changes

Yisaer changed the title ~~[WIP] Support duration chaos for Network Chaos~~ Support duration chaos for Network Chaos Feb 5, 2020

Yisaer added status/PTAL and removed status/WIP labels Feb 5, 2020

Yisaer requested review from YangKeao, cwen0 and zhouqiang-cl February 5, 2020 14:28

zhouqiang-cl reviewed Feb 6, 2020

View reviewed changes

fix controller

e82aaa2

Yisaer requested a review from zhouqiang-cl February 6, 2020 03:17

Merge branch 'master' into support_duratiom_chaos_for_network_chaos

1f30064

at15 reviewed Feb 6, 2020

View reviewed changes

Update types.go

ee4b9ee

Yisaer removed the status/PTAL label Feb 6, 2020

Yisaer mentioned this pull request Feb 6, 2020

Refactor controller layout #201

Merged

Merge branch 'master' into support_duratiom_chaos_for_network_chaos

3aee6ba

zhouqiang-cl added this to In progress in Weekly Plan 2020-02-08 ~ 2020-02-15 via automation Feb 7, 2020

Yisaer added 3 commits February 7, 2020 18:32

Merge remote-tracking branch 'upstream/master' into support_duratiom_…

d9b0d45

…chaos_for_network_chaos

fix by comment

032431f

adjust common reconciler

bda5f52

Yisaer commented Feb 7, 2020

View reviewed changes

Yisaer requested a review from mahjonp February 7, 2020 10:56

Yisaer added 2 commits February 7, 2020 18:57

fmt imports

db1502f

fmt imports

aefefc1

mahjonp previously approved these changes Feb 7, 2020

View reviewed changes

cwen0 reviewed Feb 7, 2020

View reviewed changes

Update types.go

e0cdff2

Yisaer dismissed mahjonp’s stale review via e0cdff2 February 7, 2020 12:34

Weekly Plan 2020-02-08 ~ 2020-02-15 automation moved this from In progress to Review in progress Feb 7, 2020

Update types.go

0deef22

cwen0 previously approved these changes Feb 7, 2020

View reviewed changes

Yisaer requested a review from mahjonp February 7, 2020 12:55

mahjonp reviewed Feb 7, 2020

View reviewed changes

Update types.go

3379f38

Yisaer dismissed cwen0’s stale review via 3379f38 February 7, 2020 14:41

cwen0 approved these changes Feb 7, 2020

View reviewed changes

mahjonp approved these changes Feb 7, 2020

View reviewed changes

Weekly Plan 2020-02-08 ~ 2020-02-15 automation moved this from Review in progress to Reviewer approved Feb 7, 2020

cwen0 merged commit f380702 into chaos-mesh:master Feb 7, 2020

Weekly Plan 2020-02-08 ~ 2020-02-15 automation moved this from Reviewer approved to Done Feb 7, 2020

sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021

Support duration chaos for Network Chaos (chaos-mesh#198)

4cadc63

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support duration chaos for Network Chaos #198

Support duration chaos for Network Chaos #198

Yisaer commented Feb 5, 2020 •

edited

codecov-io commented Feb 5, 2020 •

edited

Yisaer Feb 5, 2020

mahjonp Feb 6, 2020

Yisaer Feb 7, 2020

zhouqiang-cl Feb 6, 2020

Yisaer Feb 6, 2020

zhouqiang-cl Feb 6, 2020

at15 Feb 6, 2020

zhouqiang-cl Feb 6, 2020

Yisaer Feb 6, 2020

zhouqiang-cl Feb 6, 2020

zhouqiang-cl commented Feb 6, 2020

zhouqiang-cl commented Feb 6, 2020

at15 left a comment

at15 Feb 6, 2020

at15 Feb 6, 2020

Yisaer Feb 6, 2020

at15 Feb 6, 2020

mahjonp Feb 6, 2020

Yisaer Feb 7, 2020

zhouqiang-cl commented Feb 6, 2020

Yisaer commented Feb 6, 2020 •

edited

zhouqiang-cl commented Feb 7, 2020

zhouqiang-cl commented Feb 7, 2020

Yisaer Feb 7, 2020

mahjonp left a comment

cwen0 Feb 7, 2020

cwen0 left a comment

mahjonp Feb 7, 2020

Yisaer Feb 7, 2020

mahjonp Feb 7, 2020

cwen0 Feb 7, 2020

zhouqiang-cl Feb 7, 2020

Yisaer Feb 7, 2020

Support duration chaos for Network Chaos #198

Support duration chaos for Network Chaos #198

Conversation

Yisaer commented Feb 5, 2020 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

codecov-io commented Feb 5, 2020 • 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

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

zhouqiang-cl commented Feb 6, 2020

zhouqiang-cl commented Feb 6, 2020

at15 left a 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

zhouqiang-cl commented Feb 6, 2020

Yisaer commented Feb 6, 2020 • edited

zhouqiang-cl commented Feb 7, 2020

zhouqiang-cl commented Feb 7, 2020

Choose a reason for hiding this comment

mahjonp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwen0 left a 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

Yisaer commented Feb 5, 2020 •

edited

codecov-io commented Feb 5, 2020 •

edited

Yisaer commented Feb 6, 2020 •

edited