Encapsulate Recover of controllers #1014

Yiyiyimu · 2020-10-02T08:32:05Z

Signed-off-by: yiyiyimu wosoyoung@gmail.com

What problem does this PR solve?

fix #961, currently only encapsulate chaos w/o managers.

What is changed and how does it work?

Add a common CleanFinalizersAndRecover which implemented for a new struct.

Checklist

Tests

Unit test
E2E test
Manual test (add detailed scripts or steps below)
No code

Does this PR introduce a user-facing change?

NONE

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

… into refine-recover

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Yiyiyimu · 2020-10-02T12:22:02Z

It would be a bit hard for encapsulation for controllers with managers, like IoChaos and NetworkChaos. They are quite similar but still there is some chaos-specific logic, for example #788 and #892 is made for networkChaos only.

~~Actually I'm not so sure if it would be a good idea to encapsulate this part.~~
Not like api which every chaos type has the same API, encapsulating code here might make it hard to implement a change of certain chaos type, like the PR I listed. But I'm not sure about this inference, maybe certain change could work on different types but right now only one type is changed, so encapsulation would make it effective. @YangKeao Could you give me some suggestions?

No that does not effect the encapsulation, #788 didn't make difference, and the difference resulting from #892 is coming from only change one of managers but forget the other one.

YangKeao · 2020-10-10T09:31:07Z

Maybe I should also try to make a subresourcemanager to replace podiochaosmanager and podnetworkchaosmanager... But these changes are blocked until #940 get merged. The code generator in #940 could provide a more convinient way to add getter and setter, and obviously I need SetName, SetNamespace in subresourcemanager 😢 .

Yiyiyimu · 2020-10-10T11:52:00Z

Maybe I should also try to make a subresourcemanager to replace podiochaosmanager and podnetworkchaosmanager... But these changes are blocked until #940 get merged. The code generator in #940 could provide a more convinient way to add getter and setter, and obviously I need SetName, SetNamespace in subresourcemanager 😢 .

Sure I think we could wait until that time to update this part

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

codecov-io · 2020-11-28T07:09:37Z

Codecov Report

Merging #1014 (ea13fca) into master (7e9ff3f) will decrease coverage by 3.62%.
The diff coverage is 52.77%.

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
- Coverage   55.78%   52.15%   -3.63%     
==========================================
  Files          68       78      +10     
  Lines        4383     4918     +535     
==========================================
+ Hits         2445     2565     +120     
- Misses       1768     2095     +327     
- Partials      170      258      +88

Impacted Files	Coverage Δ
api/v1alpha1/common_types.go	`0.00% <0.00%> (ø)`
api/v1alpha1/common_webhook.go	`100.00% <ø> (ø)`
api/v1alpha1/dnschaos_type.go	`0.00% <0.00%> (ø)`
api/v1alpha1/dnschaos_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/httpchaos_types.go	`0.00% <0.00%> (ø)`
api/v1alpha1/iochaos_types.go	`0.00% <ø> (-40.00%)`	⬇️
api/v1alpha1/jvmchaos_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/kernelchaos_types.go	`0.00% <ø> (-20.00%)`	⬇️
api/v1alpha1/kernelchaos_webhook.go	`100.00% <ø> (+14.81%)`	⬆️
api/v1alpha1/kinds.go	`27.27% <ø> (+0.60%)`	⬆️
... and 128 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 0f817aa...ea13fca. Read the comment docs.

Yiyiyimu · 2020-11-28T07:23:18Z

Hi @YangKeao is the current implementation what we want

Yiyiyimu · 2020-11-28T07:23:25Z

/run-e2e-tests

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Yiyiyimu · 2020-12-15T14:09:15Z

Hi @YangKeao PTAL

YangKeao · 2020-12-16T03:52:52Z

Actually, I don't think generating all codes through a generator is a good idea. Only small parts of them (such as some getter or setter) should be done by code generation.

We can analyze the problem like this:

To implement Recover, we require an endpoint with cleanFinalizersAndRecover and Event methods. To implement cleanFinalizersAndRecover for an endpoint, we need a getter and setter for the Finalizers, a Client, and the most important part: recoverPod.

Only some of these requirements should be generated through the code generator (e.g. the Finalizers getter and setter for the custom resource).

Yiyiyimu · 2020-12-16T07:59:33Z

Yeah it's actually also weird for me, to "generate" this large block of code that contains some logic. But since we could easily get/set finalizer so just only generate this part could not remove much duplicated code.

I have another idea (still a bit weird) that to build a common cleanFinalizersAndRecover, in which pass the endpoint as an argument (don't know if it is plausible), it could be like

func (e *endpoint) Recover(ctx context.Context, req ctrl.Request, chaos v1alpha1.InnerObject) error {
	stressChaos, ok := chaos.(*v1alpha1.StressChaos)
	finalizers, err := cleanFinalizersAndRecover(ctx, e, chaos, stressChaos.Finalizers, stressChaos.Annotations[common.AnnotationCleanFinalizer])
	stressChaos.Finalizers = finalizers
}
// the common one
func cleanFinalizersAndRecover(ctx context.Context, e *endpoint, chaos v1alpha1.InnerObject, finalizers []string, forceClean string) error {
	...
	err = e.recoverPod(ctx, &pod, chaos)
	...
}
// do another type assertion in recoverPod
func (e *endpoint) recoverPod(ctx context.Context, pod *v1.Pod, chaos *v1alpha1.StressChaos) error {
	stressChaos, ok := chaos.(*v1alpha1.StressChaos)
	...
}

YangKeao · 2020-12-17T03:54:44Z

@Yiyiyimu We can ignore the *manager part temporarily and encapsulate others first.

A straightforward plan could be defining a trait RecoverPod (maybe you can pick a better name), which contains one RecoverPod function.

And automatically implement cleanFinalizersAndRecover for all these traits. However, you cannot impl<T: IRecoverPod> ICleanFinalizers for T in go. But you can construct a new RecoverDelegate struct whose only field is a RecoverPod interface. Than implement Recover and cleanFinalizersAndRecover for this RecoverDelegate struct.

But how to combine it with Apply method still needs consideration. I don't know much about how to write elegant code with such a lack of language features 😭 .

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Yiyiyimu · 2020-12-25T00:57:01Z

@cwen0 it seems the test goes stable now. PTAL

Yiyiyimu · 2020-12-29T07:57:14Z

Hi @cwen0 @YangKeao could we review this pr at a faster speed? There are so many times files got conflicts.

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

cwen0 · 2020-12-29T11:56:50Z

Hi @cwen0 @YangKeao could we review this pr at a faster speed? There are so many times files got conflicts.

Sorry for our late reply. I will review this pr ASAP.

cwen0

Rest LGTM

cwen0 · 2020-12-30T03:19:52Z

controllers/dnschaos/types.go

-
-func (r *endpoint) recoverPod(ctx context.Context, pod *v1.Pod) error {
-	r.Log.Info("Try to recover pod", "namespace", pod.Namespace, "name", pod.Name)
+	r.cancelDNSServerRules(service.Spec.ClusterIP, config.ControllerCfg.DNSServicePort, chaos.Name)


We don't need to run this function in each pod.

Fixed. Thanks!

controllers/networkchaos/partition/types.go

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

cwen0

LGTM

cwen0 · 2021-01-06T03:46:26Z

@YangKeao PTAL

YangKeao

LGTM

YangKeao · 2021-01-06T06:43:25Z

/merge

ti-srebot · 2021-01-06T06:45:40Z

/run-all-tests

ti-srebot · 2021-01-06T07:00:37Z

@Yiyiyimu merge failed.

YangKeao · 2021-01-06T07:45:31Z

/merge

ti-srebot · 2021-01-06T07:45:37Z

/run-all-tests

ti-srebot · 2021-01-06T08:07:38Z

@Yiyiyimu merge failed.

WangXiangUSTC · 2021-01-06T08:30:11Z

/run-e2e-tests

YangKeao · 2021-01-06T08:58:58Z

/merge

ti-srebot · 2021-01-06T08:59:00Z

Your auto merge job has been accepted, waiting for:

1360

ti-srebot · 2021-01-06T09:16:10Z

/run-all-tests

refine controller recover

80cb7ab

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

ti-srebot added the contribution label Oct 2, 2020

Yiyiyimu added 3 commits October 2, 2020 16:32

Merge branch 'master' into refine-recover

1769d28

fix check

5d4a3c7

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Merge branch 'refine-recover' of https://github.com/yiyiyimu/chaos-mesh…

fedf41f

… into refine-recover

Yiyiyimu changed the title ~~[WIP] Encapsulate Recover of controllers~~ Encapsulate Recover of controllers Oct 2, 2020

[not working]encap for w/o managers

b44a8e5

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Yiyiyimu changed the title ~~Encapsulate Recover of controllers~~ [WIP] Encapsulate Recover of controllers Oct 2, 2020

Yiyiyimu added 3 commits November 28, 2020 12:03

merge master

ad0dd9c

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

refactor DNSChaos to keep the same

a3667c9

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

add DNSChaos to generate Recover

a9eed86

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Yiyiyimu changed the title ~~[WIP] Encapsulate Recover of controllers~~ Encapsulate Recover of controllers Nov 28, 2020

fix format for goimports

5503fef

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Yiyiyimu added 2 commits December 15, 2020 21:35

merge master

dcf5109

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

remove empty line

093b303

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

Yiyiyimu added 5 commits December 19, 2020 14:39

revert to master

a1704bb

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

fix typo

9ffcf3d

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

nasty version

b9281f3

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

refine the structure

04d7244

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

fix: remove chaos-builder/recover.go

072001e

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

merge master

b14dba6

Yiyiyimu added 2 commits December 29, 2020 16:16

merge master

5d622c6

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

fix CI

ea13fca

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

cwen0 requested review from YangKeao and WangXiangUSTC December 29, 2020 11:48

cwen0 reviewed Dec 30, 2020

View reviewed changes

fix: cancel dnschaos one time but not each pod

dbef464

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>

cwen0 self-requested a review January 4, 2021 02:34

WangXiangUSTC added the improve/Refine label Jan 5, 2021

cwen0 approved these changes Jan 6, 2021

View reviewed changes

YangKeao approved these changes Jan 6, 2021

View reviewed changes

ti-srebot added the status/can-merge label Jan 6, 2021

Merge branch 'master' into refine-recover

552f2c5

Merge branch 'master' into refine-recover

347e231

ti-srebot merged commit 6d04530 into chaos-mesh:master Jan 6, 2021

Yiyiyimu mentioned this pull request Jan 6, 2021

feat: make a common pod manager to replace podio(network)chaosmanager #1364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate Recover of controllers #1014

Encapsulate Recover of controllers #1014

Yiyiyimu commented Oct 2, 2020 •

edited

Loading

Yiyiyimu commented Oct 2, 2020 •

edited

Loading

YangKeao commented Oct 10, 2020

Yiyiyimu commented Oct 10, 2020

codecov-io commented Nov 28, 2020 •

edited

Loading

Yiyiyimu commented Nov 28, 2020

Yiyiyimu commented Nov 28, 2020

Yiyiyimu commented Dec 15, 2020

YangKeao commented Dec 16, 2020

Yiyiyimu commented Dec 16, 2020

YangKeao commented Dec 17, 2020 •

edited

Loading

Yiyiyimu commented Dec 25, 2020

Yiyiyimu commented Dec 29, 2020

cwen0 commented Dec 29, 2020

cwen0 left a comment

cwen0 Dec 30, 2020

Yiyiyimu Dec 30, 2020

cwen0 left a comment

cwen0 commented Jan 6, 2021

YangKeao left a comment

YangKeao commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

YangKeao commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

WangXiangUSTC commented Jan 6, 2021

YangKeao commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

Encapsulate Recover of controllers #1014

Encapsulate Recover of controllers #1014

Conversation

Yiyiyimu commented Oct 2, 2020 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Checklist

Does this PR introduce a user-facing change?

Yiyiyimu commented Oct 2, 2020 • edited Loading

YangKeao commented Oct 10, 2020

Yiyiyimu commented Oct 10, 2020

codecov-io commented Nov 28, 2020 • edited Loading

Codecov Report

Yiyiyimu commented Nov 28, 2020

Yiyiyimu commented Nov 28, 2020

Yiyiyimu commented Dec 15, 2020

YangKeao commented Dec 16, 2020

Yiyiyimu commented Dec 16, 2020

YangKeao commented Dec 17, 2020 • edited Loading

Yiyiyimu commented Dec 25, 2020

Yiyiyimu commented Dec 29, 2020

cwen0 commented Dec 29, 2020

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 Dec 30, 2020

Choose a reason for hiding this comment

Yiyiyimu Dec 30, 2020

Choose a reason for hiding this comment

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 commented Jan 6, 2021

YangKeao left a comment

Choose a reason for hiding this comment

YangKeao commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

YangKeao commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

WangXiangUSTC commented Jan 6, 2021

YangKeao commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

ti-srebot commented Jan 6, 2021

Yiyiyimu commented Oct 2, 2020 •

edited

Loading

Yiyiyimu commented Oct 2, 2020 •

edited

Loading

codecov-io commented Nov 28, 2020 •

edited

Loading

YangKeao commented Dec 17, 2020 •

edited

Loading