Validates the scheduler #461

Gallardot · 2020-05-02T09:08:47Z

What problem does this PR solve?

#373

What is changed and how does it work?

Check List

Tests

Unit test

Code changes

Has Go code change

Side effects

None

Related changes

None

Does this PR introduce a user-facing change?:

NONE

codecov-io · 2020-05-02T10:51:16Z

Codecov Report

Merging #461 into master will increase coverage by 2.23%.
The diff coverage is 55.94%.

@@            Coverage Diff             @@
##           master     #461      +/-   ##
==========================================
+ Coverage   58.57%   60.81%   +2.23%     
==========================================
  Files          60       68       +8     
  Lines        3626     4410     +784     
==========================================
+ Hits         2124     2682     +558     
- Misses       1333     1537     +204     
- Partials      169      191      +22

Impacted Files	Coverage Δ
api/v1alpha1/common_webhook.go	`100.00% <ø> (ø)`
api/v1alpha1/iochaos_types.go	`60.00% <0.00%> (+27.44%)`	⬆️
api/v1alpha1/kernelchaos_types.go	`33.33% <0.00%> (+28.68%)`	⬆️
api/v1alpha1/networkchaos_types.go	`26.86% <0.00%> (+7.92%)`	⬆️
api/v1alpha1/podchaos_types.go	`55.55% <0.00%> (+22.99%)`	⬆️
controllers/stresschaos_controller.go	`0.00% <0.00%> (ø)`
controllers/timechaos_controller.go	`0.00% <0.00%> (ø)`
pkg/chaosdaemon/netem_linux.go	`26.66% <0.00%> (-17.78%)`	⬇️
pkg/chaosdaemon/netlink_linux.go	`0.00% <ø> (ø)`
pkg/chaosdaemon/stress_server.go	`0.00% <0.00%> (ø)`
... and 42 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 15ff111...a09f725. Read the comment docs.

zhouqiang-cl · 2020-05-05T13:12:02Z

/run-e2e-tests

Gallardot · 2020-05-06T02:35:24Z

/run-e2e-tests

cwen0 · 2020-05-06T03:10:54Z

api/v1alpha1/common_validation.go

 		}
+	} else if (duration == nil && scheduler != nil) || (duration != nil && scheduler == nil) {


I think duration==nil && scheduler != nil is ok, sometimes we just want to start to chaos experiment at a specific time and last to the end time.

I am not sure about that.
But I find the controllers except podchaos all need scheduler == nil && duration == nil OR scheduler != nil && duration != nil

https://github.com/pingcap/chaos-mesh/blob/8e769a704563d2becd5bd9269961783bca912952/controllers/iochaos/types.go#L47-L51

https://github.com/pingcap/chaos-mesh/blob/8e769a704563d2becd5bd9269961783bca912952/controllers/stresschaos/types.go#L59-L63

https://github.com/pingcap/chaos-mesh/blob/8e769a704563d2becd5bd9269961783bca912952/controllers/kernelchaos/types.go#L66-L70

https://github.com/pingcap/chaos-mesh/blob/8e769a704563d2becd5bd9269961783bca912952/controllers/timechaos/types.go#L58-L62

But podschaos has a different logic

https://github.com/pingcap/chaos-mesh/blob/8e769a704563d2becd5bd9269961783bca912952/controllers/podchaos/types.go#L48-L52

I think the main reason for this difference is that PodKillAction and ContainerKillAction have different behaviors. We have addressed this difference in webhook of podschaos.

This can be simplified to else if !(duration == nil && scheduler == nil)

cwen0 · 2020-05-06T03:12:08Z

api/v1alpha1/common_validation.go

+		cronField := schedulerField.Child("cron")
+		allErrs = append(validateCron(spec.Cron, cronField))
+
+		scheduler, _ := cronv3.ParseStandard(spec.Cron)


why ignore this error?

validateCron had validated the cron.

If ParseStandard causes error, the scheduler will be nil

So I think we can ignore this error.

I don't suggest ignoring error here. The logic could be changed in future and we may miss the root cause when error happens.

I've already fixed it. PTAL
@cwen0 @Yisaer

yeya24 · 2020-05-06T03:58:47Z

/run-e2e-tests

Gallardot · 2020-05-09T02:39:37Z

/run-e2e-test

yeya24 · 2020-05-09T03:15:42Z

/run-e2e-test

cwen0 · 2020-05-09T05:47:52Z

/run-e2e-test

cwen0

LGTM

cwen0 · 2020-05-09T09:50:38Z

/run-e2e-test

yeya24 · 2020-05-09T14:53:58Z

/run-e2e-test

yeya24

I checked the e2e test and tested this PR in my local environment. Seems the continuous failure of e2e is caused by this PR itself. @Gallardot

Tested with the time chaos example.

apiVersion: pingcap.com/v1alpha1
kind: TimeChaos
metadata:
  name: time-shift-example
  namespace: chaos-testing
spec:
  mode: one
  selector:
    labelSelectors:
      app: io
  timeOffset: "-1h"
  duration: "9m"
  scheduler:
    cron: "@every 10m"

Seems the nextRecover is not correct.

yeya24 · 2020-05-09T16:24:02Z

api/v1alpha1/common_validation.go

 		}
+	} else if (duration == nil && scheduler != nil) || (duration != nil && scheduler == nil) {


This can be simplified to else if !(duration == nil && scheduler == nil)

Gallardot · 2020-05-09T17:08:32Z

I checked the e2e test and tested this PR in my local environment. Seems the continuous failure of e2e is caused by this PR itself. @Gallardot

Tested with the time chaos example.
apiVersion: pingcap.com/v1alpha1
kind: TimeChaos
metadata:
  name: time-shift-example
  namespace: chaos-testing
spec:
  mode: one
  selector:
    labelSelectors:
      app: io
  timeOffset: "-1h"
  duration: "9m"
  scheduler:
    cron: "@every 10m"
Seems the nextRecover is not correct.

I'm terribly sorry about that. I’ll fix it right away. @yeya24

yeya24 · 2020-05-09T17:36:58Z

I checked the e2e test and tested this PR in my local environment. Seems the continuous failure of e2e is caused by this PR itself. @Gallardot
Tested with the time chaos example.
apiVersion: pingcap.com/v1alpha1
kind: TimeChaos
metadata:
  name: time-shift-example
  namespace: chaos-testing
spec:
  mode: one
  selector:
    labelSelectors:
      app: io
  timeOffset: "-1h"
  duration: "9m"
  scheduler:
    cron: "@every 10m"
Seems the nextRecover is not correct.
I'm terribly sorry about that. I’ll fix it right away. @yeya24

No worries. Thanks for your awesome work! 🚀

…to validate

Gallardot · 2020-05-12T07:53:30Z

/run-e2e-test

Gallardot · 2020-05-12T08:57:58Z

I have fix the continuous failure.
I will migrate nextStart and nextRecover to status in another PR

@yeya24 @cwen0 PTAL

yeya24 · 2020-05-12T22:27:20Z

/run-e2e-test

yeya24 · 2020-05-13T17:35:25Z

/run-e2e-test

yeya24

LGTM

cwen0

LGTM

validates the scheduler

e4ac30b

sre-bot added the contribution label May 2, 2020

Gallardot added 3 commits May 2, 2020 17:11

validates the scheduler

44240aa

fix

34a510a

fix

d543fa9

Gallardot added the status/WIP label May 2, 2020

fix

bfc44a4

fix

7723815

Gallardot added status/PTAL and removed status/WIP labels May 2, 2020

cwen0 reviewed May 6, 2020

View reviewed changes

Gallardot added 2 commits May 7, 2020 23:40

fix

aa0f4a2

fix

3038e9e

Gallardot linked an issue May 7, 2020 that may be closed by this pull request

Validate duration and cron schedule conflict in webhook #373

Closed

fix

5f70105

Merge branch 'master' into validate

1952a39

Merge branch 'master' into validate

a09f725

cwen0 previously approved these changes May 9, 2020

View reviewed changes

Merge branch 'master' into validate

6a64475

yeya24 requested changes May 9, 2020

View reviewed changes

Gallardot added status/WIP and removed status/PTAL labels May 9, 2020

fix

6835596

Gallardot dismissed cwen0’s stale review via 6835596 May 12, 2020 07:24

Gallardot added 3 commits May 12, 2020 15:27

Merge branch 'master' into validate

4870c08

fix

17da2e2

Merge branch 'validate' of https://github.com/Gallardot/chaos-mesh in…

0d9c892

…to validate

Gallardot requested review from Yisaer, cwen0 and yeya24 May 12, 2020 08:58

Gallardot added status/PTAL and removed status/WIP labels May 12, 2020

Merge branch 'master' into validate

7a08543

yeya24 approved these changes May 13, 2020

View reviewed changes

cwen0 approved these changes May 14, 2020

View reviewed changes

cwen0 merged commit 4d2efcd into chaos-mesh:master May 14, 2020

Gallardot deleted the validate branch May 14, 2020 03:48

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

Validates the scheduler (chaos-mesh#461)

401efb6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validates the scheduler #461

Validates the scheduler #461

Gallardot commented May 2, 2020

codecov-io commented May 2, 2020 •

edited

Loading

zhouqiang-cl commented May 5, 2020

Gallardot commented May 6, 2020

cwen0 May 6, 2020

Gallardot May 6, 2020

yeya24 May 9, 2020

cwen0 May 6, 2020

Gallardot May 6, 2020

Yisaer May 6, 2020

Gallardot May 7, 2020

yeya24 commented May 6, 2020

Gallardot commented May 9, 2020

yeya24 commented May 9, 2020

cwen0 commented May 9, 2020

cwen0 left a comment

cwen0 commented May 9, 2020

yeya24 commented May 9, 2020

yeya24 left a comment

yeya24 May 9, 2020

Gallardot commented May 9, 2020

yeya24 commented May 9, 2020

Gallardot commented May 12, 2020

Gallardot commented May 12, 2020

yeya24 commented May 12, 2020

yeya24 commented May 13, 2020

yeya24 left a comment

cwen0 left a comment

		}
		} else if (duration == nil && scheduler != nil) \|\| (duration != nil && scheduler == nil) {

Validates the scheduler #461

Validates the scheduler #461

Conversation

Gallardot commented May 2, 2020

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 May 2, 2020 • edited Loading

Codecov Report

zhouqiang-cl commented May 5, 2020

Gallardot commented May 6, 2020

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

yeya24 commented May 6, 2020

Gallardot commented May 9, 2020

yeya24 commented May 9, 2020

cwen0 commented May 9, 2020

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 commented May 9, 2020

yeya24 commented May 9, 2020

yeya24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gallardot commented May 9, 2020

yeya24 commented May 9, 2020

Gallardot commented May 12, 2020

Gallardot commented May 12, 2020

yeya24 commented May 12, 2020

yeya24 commented May 13, 2020

yeya24 left a comment

Choose a reason for hiding this comment

cwen0 left a comment

Choose a reason for hiding this comment

codecov-io commented May 2, 2020 •

edited

Loading