Fix the issue that io delay injected does not take effect #121

cwen0 · 2020-01-10T16:22:55Z

What problem does this PR solve?

Fix the issue that io delay injected does not take effect
Fix the config watcher can't restart
Update IOChaos document
Use the data directory as the mountpoint of the fuse server
No longer allow to inject chaosfs container dynamically and only support inject chaosfs container before the application created.
If the application was created, the data directory of the application was not empty, we can't use 'fusermount' to mount it and we must rename it to fuse-data, but If we rename the data directory, there is a risk that the application will not start properly because the data directory cannot be found when the pod was restarted.

Check List

Tests

Manual test (add detailed scripts or steps below)

2020-01-10T15:56:31.578Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:32.579Z        INFO    fuse-server     Inject fault    {"method": "release", "path": "last_tikv.toml"}
2020-01-10T15:56:32.579Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:32.580Z        INFO    fuse-server     Inject fault    {"method": "getattr", "path": "MAILPATH"}
2020-01-10T15:56:32.580Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:38.694Z        INFO    fuse-server     Inject fault    {"method": "getattr", "path": "snap"}
2020-01-10T15:56:38.694Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:39.696Z        INFO    fuse-server     Inject fault    {"method": "opendir", "path": "snap"}
2020-01-10T15:56:39.696Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:40.706Z        INFO    fuse-server     Inject fault    {"method": "getattr", "path": "db"}
2020-01-10T15:56:40.706Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:41.708Z        INFO    fuse-server     Inject fault    {"method": "statfs", "path": "db"}
2020-01-10T15:56:41.708Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:50.706Z        INFO    fuse-server     Inject fault    {"method": "getattr", "path": "db"}
2020-01-10T15:56:50.706Z        INFO    fuse-server     Inject fault    {"context": {}}
2020-01-10T15:56:51.441Z        INFO    fuse-server     recover all fault

Does this PR introduce a user-facing change?:

NONE 

No longer allow to inject `chaosfs` container dynamically and only support inject `chaosfs ` container before the application created.

Signed-off-by: cwen0 <cwenyin0@gmail.com>

zhouqiang-cl · 2020-01-10T19:02:54Z

This pr need queey's test. can you help to test it @mahjonp

ethercflow · 2020-01-11T11:43:05Z

controllers/iochaos/fs/types.go

-	if err := utils.UnsetIoInjection(ctx, r.Client, pod, iochaos); err != nil {
-		r.Log.Error(err, "failed to unset I/O injection",
+		return true, nil
+	}, cctx.Done())


When network is not ok, Poll loop and cctx timeout, wait.PollUntil will return timeout err or nil?

timeout err

Signed-off-by: cwen0 <cwenyin0@gmail.com>

zhouqiang-cl

LGTM

mahjonp · 2020-01-12T07:40:02Z

scripts/init.sh

+}
+
+copy_scripts() {
+  echo $1


polish codes?

mahjonp · 2020-01-12T07:57:04Z

pkg/utils/coalescer.go

+				// input is closed, so lets signal one last time if we have any pending unsignalled events
+				if signalled {
+					// send final event, so we dont miss the trailing event after input chan close
+					output <- struct{}{}


The comment on Coalescer is inaccurate

// it signals on output chan with the last value from input chan

and why remove closing the output channel?

Closing output channel maybe better.

mahjonp · 2020-01-12T08:05:26Z

examples/chaosfs-configmap/tikv-configmap.yaml

-        - -original=/var/lib/tikv/data
-        - -mountpoint=/var/lib/tikv/fuse-data
+        - -original=/var/lib/tikv/fuse-data
+        - -mountpoint=/var/lib/tikv/data


the arg name is misleading..

@ethercflow PTAL

mahjonp · 2020-01-12T08:12:03Z

doc/sidecar_configmap.md

 This directory is usually set to the same level directory as the original directory.
+* **mountpoint**: defines the mountpoint to mount original directory.


Seems this description is contradictory with how it is used actually?

- -original=/var/lib/tikv/fuse-data - -mountpoint=/var/lib/tikv/data

yes, I will fix it.

mahjonp · 2020-01-12T08:20:56Z

cmd/controller-manager/main.go

@@ -187,6 +187,7 @@ func watchConfig(cfg *config.Config, stopCh <-chan struct{}) {
 				case <-stopCh:
 					close(sigChan)
 					return
+				default:


why add the default branch? this may cause there exist multiple configWatchers that notify the eventsCh at the same time?

No, this branch just used to restart configWatcher.Watch

I misunderstood the stopCh before, I thought it was used by configWatcher to notify the outer goroutine that it had been finished.
If configWatcher.Watch or eventsCh first received the msg from stopCh before the outer goroutine, the configWatcher.Watch will be restarted forever because case <- stopCh at line 187 won't receive msg again? how to solve this problem?

stopCh will be closed, so all receiver will receive this event.

Got, that's ok

Restarting is ok for ErrWatchChannelClosed case, is that ok for other err?

mahjonp

rest lgtm

mahjonp · 2020-01-12T08:43:19Z

What's the reason of not support inject sidecar dynamically now?

Signed-off-by: cwen0 <cwenyin0@gmail.com>

cwen0 · 2020-01-12T10:52:08Z

What's the reason of not support inject sidecar dynamically now?

If the application was created, the data directory of the application was not empty, we can't use fusermount to mount it and we must rename it to fuse-data, but If we rename the data directory, there is a risk that the application will not start properly because the data directory cannot be found when the pod was restarted.

codecov-io · 2020-01-12T15:06:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7ab71cc). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #121   +/-   ##
=========================================
  Coverage          ?   29.96%           
=========================================
  Files             ?       24           
  Lines             ?      801           
  Branches          ?        0           
=========================================
  Hits              ?      240           
  Misses            ?      530           
  Partials          ?       31

Impacted Files	Coverage Δ
pkg/utils/coalescer.go	`0% <0%> (ø)`

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 7ab71cc...9e436b2. Read the comment docs.

…chaos

zhouqiang-cl

LGTM

you06

LGTM

Signed-off-by: cwen0 <cwenyin0@gmail.com>

zhouqiang-cl

LGTM

mahjonp

lgtm

Fix the issue that io delay injected does not take effect

2442712

cwen0 requested review from YangKeao, zhouqiang-cl, ethercflow and mahjonp January 10, 2020 16:23

cwen0 added 2 commits January 11, 2020 00:39

update iochaos document

56c0e96

Signed-off-by: cwen0 <cwenyin0@gmail.com>

update link

89189f9

Signed-off-by: cwen0 <cwenyin0@gmail.com>

cwen0 added the type/bug Something isn't working label Jan 10, 2020

zhouqiang-cl requested a review from you06 January 10, 2020 18:58

ethercflow reviewed Jan 11, 2020

View reviewed changes

update iochaos document

7ab71cc

Signed-off-by: cwen0 <cwenyin0@gmail.com>

zhouqiang-cl added type/bug-fix A bug needs to be fixed. and removed type/bug Something isn't working labels Jan 12, 2020

mahjonp mentioned this pull request Jan 12, 2020

Cannot uninstall iochaos when sidecar chaosfs running failed #126

Closed

zhouqiang-cl reviewed Jan 12, 2020

View reviewed changes

zhouqiang-cl added the priority/P1 label Jan 12, 2020

mahjonp reviewed Jan 12, 2020

View reviewed changes

scripts/init.sh Outdated

}

copy_scripts() {

echo $1

Copy link

Contributor

mahjonp Jan 12, 2020

Choose a reason for hiding this comment

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

polish codes?

mahjonp reviewed Jan 12, 2020

View reviewed changes

Merge branch 'master' into fix_iochaos

a9e2259

mahjonp reviewed Jan 12, 2020

View reviewed changes

address comments

11de512

Signed-off-by: cwen0 <cwenyin0@gmail.com>

Merge branch 'master' into fix_iochaos

52d49c2

Merge branch 'fix_iochaos' of github.com:cwen0/chaos-mesh into fix_io…

1286047

…chaos

zhouqiang-cl previously approved these changes Jan 12, 2020

View reviewed changes

you06 previously approved these changes Jan 12, 2020

View reviewed changes

fix logs

9e436b2

Signed-off-by: cwen0 <cwenyin0@gmail.com>

cwen0 dismissed stale reviews from you06 and zhouqiang-cl via 9e436b2 January 13, 2020 00:06

zhouqiang-cl approved these changes Jan 13, 2020

View reviewed changes

mahjonp approved these changes Jan 13, 2020

View reviewed changes

cwen0 merged commit 60d091d into chaos-mesh:master Jan 13, 2020

cwen0 deleted the fix_iochaos branch January 13, 2020 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the issue that io delay injected does not take effect #121

Fix the issue that io delay injected does not take effect #121

cwen0 commented Jan 10, 2020 •

edited

zhouqiang-cl commented Jan 10, 2020

ethercflow Jan 11, 2020

cwen0 Jan 11, 2020

zhouqiang-cl left a comment

mahjonp Jan 12, 2020

mahjonp Jan 12, 2020

cwen0 Jan 12, 2020 •

edited by zhouqiang-cl

mahjonp Jan 12, 2020

cwen0 Jan 12, 2020

mahjonp Jan 12, 2020 •

edited

cwen0 Jan 12, 2020

mahjonp Jan 12, 2020

cwen0 Jan 12, 2020

mahjonp Jan 12, 2020

cwen0 Jan 12, 2020

mahjonp Jan 12, 2020

mahjonp Jan 12, 2020

mahjonp left a comment

mahjonp commented Jan 12, 2020

cwen0 commented Jan 12, 2020

codecov-io commented Jan 12, 2020 •

edited

zhouqiang-cl left a comment

you06 left a comment

zhouqiang-cl left a comment

mahjonp left a comment

		This directory is usually set to the same level directory as the original directory.
		* mountpoint: defines the mountpoint to mount original directory.

Fix the issue that io delay injected does not take effect #121

Fix the issue that io delay injected does not take effect #121

Conversation

cwen0 commented Jan 10, 2020 • edited

What problem does this PR solve?

Check List

Does this PR introduce a user-facing change?:

zhouqiang-cl commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouqiang-cl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwen0 Jan 12, 2020 • edited by zhouqiang-cl

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahjonp Jan 12, 2020 • edited

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

mahjonp left a comment

Choose a reason for hiding this comment

mahjonp commented Jan 12, 2020

cwen0 commented Jan 12, 2020

codecov-io commented Jan 12, 2020 • edited

Codecov Report

zhouqiang-cl left a comment

Choose a reason for hiding this comment

you06 left a comment

Choose a reason for hiding this comment

zhouqiang-cl left a comment

Choose a reason for hiding this comment

mahjonp left a comment

Choose a reason for hiding this comment

cwen0 commented Jan 10, 2020 •

edited

cwen0 Jan 12, 2020 •

edited by zhouqiang-cl

mahjonp Jan 12, 2020 •

edited

codecov-io commented Jan 12, 2020 •

edited