Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support to merge complex commands when to inject commands #185

Merged
merged 17 commits into from
Feb 4, 2020
Merged

support to merge complex commands when to inject commands #185

merged 17 commits into from
Feb 4, 2020

Conversation

cwen0
Copy link
Member

@cwen0 cwen0 commented Feb 4, 2020 •
edited
Loading

Signed-off-by: cwen0 [email protected]

What problem does this PR solve?

fix #143

What is changed and how does it work?

  1. support to merge complex commands
  2. fix update annotations when to inject the sidecar container

eg:

inject: []string{"bash", "-c", "/check.sh"}, origin: []string{"bash", "-c", "set -ex\n/tiflash server --config-file /data/config.toml"}
merged commands: []string{"/bin/sh", "-ec", "/check.sh\nset -ex\n/tiflash server --config-file /data/config.toml\n"}

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

image

log:

kubectl logs -f --namespace app-ns tiflash-0 -c tiflash
fuse server not running, /tmp/fuse/pid not found, retry...
fuse server not running, /tmp/fuse/pid not found, retry...
fuse server not running, /tmp/fuse/pid not found, retry...
fuse server not running, /tmp/fuse/pid not found, retry...
fuse server not running, /tmp/fuse/pid not found, retry...
fuse server is started
++ hostname
+ [[ tiflash-0 =~ -([0-9]+)$ ]]
+ ordinal=0
+ set +ex
Server:         10.96.0.10
Address:        10.96.0.10#53

Name:   tiflash-0.tiflash.app-ns.svc.cluster.local
Address: 10.244.5.18

Logging trace to /logs/server.log
Logging errors to /logs/error.log

Code changes

  • Has Go code change

@codecov-io
Copy link

codecov-io commented Feb 4, 2020 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #185   +/-   ##
=========================================
  Coverage          ?   44.29%           
=========================================
  Files             ?       18           
  Lines             ?      675           
  Branches          ?        0           
=========================================
  Hits              ?      299           
  Misses            ?      343           
  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 f6f0cfb...cf2370d. Read the comment docs.

@cwen0 cwen0 changed the title [WIP] support to merge complex commands when to inject commands support to merge complex commands when to inject commands Feb 4, 2020
zhouqiang-cl
zhouqiang-cl reviewed Feb 4, 2020
View reviewed changes
pkg/utils/command.go Outdated Show resolved Hide resolved
zhouqiang-cl
zhouqiang-cl reviewed Feb 4, 2020
View reviewed changes
Copy link
Contributor

@zhouqiang-cl zhouqiang-cl left a comment

Choose a reason for hiding this comment

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

rest LGTM

mahjonp
mahjonp reviewed Feb 4, 2020
View reviewed changes
pkg/utils/command.go Outdated

scripts += cmd
scripts += "\n"
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

emm, this continue is not necessary

zhouqiang-cl
zhouqiang-cl previously approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@zhouqiang-cl zhouqiang-cl left a comment

Choose a reason for hiding this comment

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

LGTM

@cwen0
address comments
6024b87 
Signed-off-by: cwen0 <[email protected]>
@cwen0
address comments
0f415b2 
Signed-off-by: cwen0 <[email protected]>
@mahjonp
Copy link
Contributor

mahjonp commented Feb 4, 2020

seems this PR is workable, but I'm not sure. @you06 @mapleFU PTAL

@mahjonp
Copy link
Contributor

mahjonp commented Feb 4, 2020

please comments on documents? list the limitation of current implementation, Rest LGTM

mapleFU
mapleFU reviewed Feb 4, 2020
View reviewed changes
pkg/utils/command.go

func isShellScripts(cmd string) bool {
if cmd == "bash" || cmd == "sh" ||
strings.HasPrefix(cmd, "bash ") || strings.HasPrefix(cmd, "sh ") ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just strings.HasPrefix(cmd, "bash ") || strings.HasPrefix(cmd, "sh ")

Copy link
Member Author

Choose a reason for hiding this comment

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

avoid some commands with prefix bash or sh, eg: shell2 xxx

pkg/utils/command.go Outdated
}

func mergeOriginCommandsAndArgs(origin []string, args []string) string {
commands := []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

commands := append(origin, args...)

@cwen0
Copy link
Member Author

cwen0 commented Feb 4, 2020

@mahjonp @zhouqiang-cl @YangKeao @mapleFU PTAL again, thanks!

zhouqiang-cl
zhouqiang-cl previously approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@zhouqiang-cl zhouqiang-cl left a comment

Choose a reason for hiding this comment

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

LGTM

mahjonp
mahjonp previously approved these changes Feb 4, 2020
View reviewed changes
@cwen0
fix comments
d29241e 
Signed-off-by: cwen0 <[email protected]>
@cwen0 cwen0 dismissed stale reviews from mahjonp and zhouqiang-cl via d29241e February 4, 2020 13:06
zhouqiang-cl
zhouqiang-cl previously approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@zhouqiang-cl zhouqiang-cl left a comment

Choose a reason for hiding this comment

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

LGTM

@cwen0
fix typo
9a4cca7 
Signed-off-by: cwen0 <[email protected]>
mahjonp
mahjonp reviewed Feb 4, 2020
View reviewed changes
doc/io_chaos.md Outdated Show resolved Hide resolved
zhouqiang-cl
zhouqiang-cl previously approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@zhouqiang-cl zhouqiang-cl left a comment

Choose a reason for hiding this comment

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

LGTM

zhouqiang-cl
zhouqiang-cl approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@zhouqiang-cl zhouqiang-cl left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouqiang-cl zhouqiang-cl merged commit fe0b928 into chaos-mesh:master Feb 4, 2020
@cwen0 cwen0 deleted the fix_inject_scripts branch July 14, 2020 02:01
sjwsl pushed a commit to sjwsl/chaos-mesh that referenced this pull request May 6, 2021
@cwen0
support to merge complex commands when to inject commands (chaos-mesh…
176254d 
…#185)

* support to merge complex commands when to inject commands

Signed-off-by: cwen0 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mapleFU mapleFU mapleFU left review comments

@zhouqiang-cl zhouqiang-cl zhouqiang-cl approved these changes

@mahjonp mahjonp mahjonp approved these changes

@YangKeao YangKeao Awaiting requested review from YangKeao

Assignees
No one assigned
Labels
component/operator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot start tiflash when to inject iochaos
5 participants
@cwen0 @codecov-io @mahjonp @zhouqiang-cl @mapleFU