Make scheduler and duration optional for apis #172

Yisaer · 2020-01-23T05:24:42Z

What problem does this PR solve?

#171
Make scheduler and duration optional for chaos APIs, then we can support this feature for each kind of chaos in parallel.

What is changed and how does it work?

Make scheduler and duration optional for chaos APIs, then we can support this feature for each kind of chaos in parallel. chaos-manager would reject the reconciliation if the scheduler and duration are not both defined currently.

Check List

Tests

Manual test (add detailed scripts or steps below)

Code changes

Has Go code change

Side effects

None

Does this PR introduce a user-facing change?:

NONE

codecov-io · 2020-01-23T05:27:55Z

Codecov Report

Merging #172 into master will increase coverage by 0.19%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
+ Coverage   39.27%   39.47%   +0.19%     
==========================================
  Files          17       17              
  Lines         611      608       -3     
==========================================
  Hits          240      240              
+ Misses        340      337       -3     
  Partials       31       31

Impacted Files	Coverage Δ
api/v1alpha1/iochaos_types.go	`32.55% <0%> (+0.73%)`	⬆️
api/v1alpha1/networkchaos_types.go	`14.43% <0%> (+0.14%)`	⬆️
api/v1alpha1/podchaos_types.go	`32.55% <0%> (+0.73%)`	⬆️

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 cdb7532...2cbf319. Read the comment docs.

zhouqiang-cl · 2020-01-23T08:56:21Z

api/v1alpha1/podchaos_types.go

-func (in *PodChaos) GetDuration() (time.Duration, error) {
-	duration, err := time.ParseDuration(in.Spec.Duration)
+//GetDuration would return the duration for chaos
+func (in *PodChaos) GetDuration() (*time.Duration, error) {


@cwen0 maybe we should refine this function, there are many repeat lines

config/crd/bases/pingcap.com_iochaos.yaml

config/crd/bases/pingcap.com_networkchaos.yaml

zhouqiang-cl · 2020-01-23T10:53:01Z

LGTM

zhouqiang-cl · 2020-01-29T04:17:16Z

@cwen0 @YangKeao @mahjonp PTAL

siddontang · 2020-01-29T05:34:24Z

api/v1alpha1/iochaos_types.go

@@ -84,7 +84,7 @@ type IoChaosSpec struct {
 	// such as "300ms", "-1.5h" or "2h45m".
 	// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
 	// +optional
-	Duration string `json:"duration"`
+	Duration *string `json:"duration,omitempty"`


why using pointer here? you can use empty to check the duration directly

Using pointer here is to make chaos API more user-friendly. If users need the chaos action always perform after they apply the chaos API, it is more reasonable that they omit the scheduler and the duration instead of leaving those as empty.

api/v1alpha1/iochaos_types.go

siddontang · 2020-01-29T05:43:06Z

api/v1alpha1/zz_generated.deepcopy.go

@@ -175,7 +175,16 @@ func (in *IoChaosList) DeepCopyObject() runtime.Object {
 func (in *IoChaosSpec) DeepCopyInto(out *IoChaosSpec) {


seem you can use https://github.com/jinzhu/copier to simplify the codes

This is auto-generated, so I suggest making it be as it generated by scripts.

mahjonp

rest lgtm

api/v1alpha1/iochaos_types.go

api/v1alpha1/networkchaos_types.go

api/v1alpha1/common_types.go

config/crd/bases/pingcap.com_iochaos.yaml

manifests/crd.yaml

api/v1alpha1/common_types.go

api/v1alpha1/iochaos_types.go

Yisaer · 2020-01-30T06:41:39Z

In this request, I also execute make yaml after I finish editing the API spec and I found that the current CRD isn't the latest which I mentioned in #173, so we can see some changes in CRD file which is not related in this request.
cc @cwen0 @mahjonp

zhouqiang-cl

LGTM

Co-Authored-By: mahjonp <junpeng.man@gmail.com>

mahjonp

lgtm

cwen0 · 2020-01-30T12:04:11Z

api/v1alpha1/common_types.go

 	// supported value: Pending / Running / Succeeded / Failed / Unknown
-	PodPhaseSelectors []string `json:"phaseSelector,omitempty"`
+	PodPhaseSelectors []string `json:"phaseSelectors,omitempty"`


json:"podPhaseSelectors, omitempty"

mahjonp

lgtm

zhouqiang-cl

LGTM

* make scheduler and duration optional for apis

Yisaer added 2 commits January 23, 2020 13:18

make scheduler and duration optional for apis

ad73df5

add comment

ab8cde3

Yisaer requested review from YangKeao, cwen0 and zhouqiang-cl January 23, 2020 05:38

zhouqiang-cl added component/operator status/PTAL type/enhancement New feature or request labels Jan 23, 2020

zhouqiang-cl reviewed Jan 23, 2020

View reviewed changes

config/crd/bases/pingcap.com_iochaos.yaml Outdated Show resolved Hide resolved

zhouqiang-cl reviewed Jan 23, 2020

View reviewed changes

config/crd/bases/pingcap.com_networkchaos.yaml Outdated Show resolved Hide resolved

zhouqiang-cl reviewed Jan 23, 2020

View reviewed changes

config/crd/bases/pingcap.com_networkchaos.yaml Outdated Show resolved Hide resolved

zhouqiang-cl added the reviews/need-LGT3 label Jan 23, 2020

zhouqiang-cl requested review from you06 and mahjonp January 23, 2020 09:28

fix crd spec

cdb7532

Merge branch 'master' into make_scheduler_and_duration_optional_for_apis

b0b1c14

siddontang reviewed Jan 29, 2020

View reviewed changes

api/v1alpha1/iochaos_types.go Show resolved Hide resolved

siddontang reviewed Jan 29, 2020

View reviewed changes

mahjonp reviewed Jan 29, 2020

View reviewed changes

zhouqiang-cl added status/ReqChange and removed status/PTAL labels Jan 29, 2020

cwen0 reviewed Jan 30, 2020

View reviewed changes

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

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

Yisaer added 2 commits January 30, 2020 14:43

format comment

7d9ffbd

fix by comment

18ad845

fix PodPhaseSelectors

602ba9e

Yisaer requested review from mahjonp, cwen0, zhouqiang-cl and siddontang January 30, 2020 06:58

zhouqiang-cl added status/PTAL and removed status/ReqChange labels Jan 30, 2020

zhouqiang-cl reviewed Jan 30, 2020

View reviewed changes

Yisaer and others added 2 commits January 30, 2020 17:57

Update api/v1alpha1/common_types.go

4ac5300

Co-Authored-By: mahjonp <junpeng.man@gmail.com>

make crd latest

ecb8828

mahjonp previously approved these changes Jan 30, 2020

View reviewed changes

cwen0 reviewed Jan 30, 2020

View reviewed changes

fix by comment

2cbf319

Yisaer dismissed mahjonp’s stale review via 2cbf319 January 31, 2020 08:07

Yisaer requested review from mahjonp, zhouqiang-cl and cwen0 January 31, 2020 08:11

mahjonp approved these changes Jan 31, 2020

View reviewed changes

zhouqiang-cl approved these changes Jan 31, 2020

View reviewed changes

Yisaer merged commit 3837ade into chaos-mesh:master Jan 31, 2020

zhouqiang-cl mentioned this pull request Jan 31, 2020

Make scheduler and duration optional for apis #171

Closed

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

Make scheduler and duration optional for apis (chaos-mesh#172)

fb903d0

* make scheduler and duration optional for apis

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make scheduler and duration optional for apis #172

Make scheduler and duration optional for apis #172

Yisaer commented Jan 23, 2020 •

edited

codecov-io commented Jan 23, 2020 •

edited

zhouqiang-cl Jan 23, 2020

zhouqiang-cl commented Jan 23, 2020

zhouqiang-cl commented Jan 29, 2020

siddontang Jan 29, 2020

Yisaer Jan 30, 2020

siddontang Jan 29, 2020

Yisaer Jan 30, 2020

mahjonp left a comment

Yisaer commented Jan 30, 2020 •

edited

zhouqiang-cl left a comment

mahjonp left a comment

cwen0 Jan 30, 2020

Yisaer Jan 31, 2020

mahjonp left a comment

zhouqiang-cl left a comment

		@@ -175,7 +175,16 @@ func (in *IoChaosList) DeepCopyObject() runtime.Object {
		func (in IoChaosSpec) DeepCopyInto(out IoChaosSpec) {

Make scheduler and duration optional for apis #172

Make scheduler and duration optional for apis #172

Conversation

Yisaer commented Jan 23, 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 Jan 23, 2020 • edited

Codecov Report

zhouqiang-cl Jan 23, 2020

Choose a reason for hiding this comment

zhouqiang-cl commented Jan 23, 2020

zhouqiang-cl commented Jan 29, 2020

siddontang Jan 29, 2020

Choose a reason for hiding this comment

Yisaer Jan 30, 2020

Choose a reason for hiding this comment

siddontang Jan 29, 2020

Choose a reason for hiding this comment

Yisaer Jan 30, 2020

Choose a reason for hiding this comment

mahjonp left a comment

Choose a reason for hiding this comment

Yisaer commented Jan 30, 2020 • edited

zhouqiang-cl left a comment

Choose a reason for hiding this comment

mahjonp left a comment

Choose a reason for hiding this comment

cwen0 Jan 30, 2020

Choose a reason for hiding this comment

Yisaer Jan 31, 2020

Choose a reason for hiding this comment

mahjonp left a comment

Choose a reason for hiding this comment

zhouqiang-cl left a comment

Choose a reason for hiding this comment

Yisaer commented Jan 23, 2020 •

edited

codecov-io commented Jan 23, 2020 •

edited

Yisaer commented Jan 30, 2020 •

edited