Add support for stracing containers #1253

mrunalp · 2018-01-12T00:39:06Z

Signed-off-by: Mrunal Patel [email protected]

- What I did
Added support to enable strace on containers.

- How I did it
Added an annotation which when passed over the API is taken as a signal to start a strace on the container.

- How to verify it
Set the annotation/label on a container and check that strace process is started for it.

- Description for the changelog

TomSweeneyRedHat

Change LGTM, but Travis isn't happy.

rhatdan · 2018-01-12T14:30:44Z

oci/oci.go

+
+	if enableStrace {
+		go func() {
+			straceCmd := exec.Command("strace", "-f", "-o", fmt.Sprintf("/tmp/%v", c.id), "-p", fmt.Sprintf("%d", c.state.Pid))


This will have to be added as a requirement for CRI-O.

Added a README.

rhatdan · 2018-01-12T14:31:15Z

oci/oci.go

+			straceCmd := exec.Command("strace", "-f", "-o", fmt.Sprintf("/tmp/%v", c.id), "-p", fmt.Sprintf("%d", c.state.Pid))
+			_, err := straceCmd.CombinedOutput()
+			if err != nil {
+				logrus.Infof("Failed to execute strace: %v", err)


Shouldn't this be Errorf?

mrunalp · 2018-01-17T00:31:03Z

Updated and added a commit to allow this when daemon has enabled it explicitly.

Signed-off-by: Mrunal Patel <[email protected]>

mrunalp · 2018-01-17T20:07:24Z

/test all

openshift-ci-robot · 2018-01-17T22:52:30Z

@mrunalp: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/e2e_rhel	`74f624b`	link	`/test e2e_rhel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking · 2018-01-18T22:38:11Z

lib/config.go

@@ -168,6 +168,10 @@ type RuntimeConfig struct {
 	// ContainerExitsDir is the directory in which container exit files are
 	// written to by conmon.
 	ContainerExitsDir string `toml:"container_exits_dir"`
+
+	// AllowStrace determinates whether the cri-o strace annotation takes effect
+	// or not.


I think the “not” case could use more specifics. Perhaps “or not” → “or is silently ignored”? We'll also want similar text in a docs/crio.8.md entry.

wking · 2018-01-18T22:42:37Z

server/container_create.go

@@ -737,6 +737,11 @@ func (s *Server) createSandboxContainer(ctx context.Context, containerID string,
 		}
 	}

+	// Only allow strace annotation if cri-o daemon allows it
+	if !s.config.Config.AllowStrace {
+		specgen.RemoveAnnotation(annotations.Strace)


I'm not sure if the server config is available in CreateContainer or not. But this approach only covers annotations and does not guard against the setting coming in through labels. It would be nice if we could move this guard into CreateContainer somehow to cover both injection paths. As a side benefit, it would allow us to guard against unwanted strace invocations without having to adjust user-supplied annotations (more on leaving annotations alone in opencontainers/runtime-spec#946).

This flag controls whether strace is started for containers with the cri-o strace annotation/label "io.kubernetes.cri-o.Strace". Signed-off-by: Mrunal Patel <[email protected]>

Signed-off-by: Mrunal Patel <[email protected]>

mrunalp · 2018-01-19T00:32:35Z

Updated.

wking · 2018-01-19T04:57:32Z

docs/crio.8.md

@@ -120,6 +120,8 @@ crio [GLOBAL OPTIONS] config [OPTIONS]

 **--cpu-profile**: Set the CPU profile file path

+**--allow-strace**: Allow strace for containers when the strace annotation/label "io.kubernetes.cri-o.Strace=true" is set


Except for the last few of these, the options in this man page seem to be ordered alphabetically. There are a number of them, so I think we may want to continue with that alphabetization to keep them easy to find.

wking · 2018-01-19T05:01:40Z

oci/container.go

@@ -36,6 +36,7 @@ type Container struct {
 	stdinOnce       bool
 	privileged      bool
 	trusted         bool
+	allowStrace     bool


Do we want to poke these holes one at a time? Or do we want to poke a big hole and add:

server *server.Config

so the container can use c.server.AllowStrace? A bigger hole would make the NewContainer interface more stable if we need additional server configs passed through later on.

mrunalp · 2018-11-28T01:37:40Z

Closing as we need to chase down issues in runc to support this effectively.

mrunalp requested a review from runcom as a code owner January 12, 2018 00:39

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes labels Jan 12, 2018

runcom approved these changes Jan 12, 2018

View reviewed changes

TomSweeneyRedHat reviewed Jan 12, 2018

View reviewed changes

rhatdan reviewed Jan 12, 2018

View reviewed changes

mrunalp force-pushed the debug_strace branch from 8b18c8d to 1eda5e7 Compare January 17, 2018 00:30

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 17, 2018

mrunalp changed the title ~~WIP: Add support for stracing containers~~ Add support for stracing containers Jan 17, 2018

Add support for stracing containers

effe1c4

Signed-off-by: Mrunal Patel <[email protected]>

mrunalp force-pushed the debug_strace branch from 1eda5e7 to 74f624b Compare January 17, 2018 19:22

wking reviewed Jan 18, 2018

View reviewed changes

mrunalp added 2 commits January 18, 2018 16:28

Add a flag for --allow-strace

740b28f

This flag controls whether strace is started for containers with the cri-o strace annotation/label "io.kubernetes.cri-o.Strace". Signed-off-by: Mrunal Patel <[email protected]>

Add docs for --allow-strace

bff62b1

Signed-off-by: Mrunal Patel <[email protected]>

mrunalp force-pushed the debug_strace branch from 74f624b to bff62b1 Compare January 19, 2018 00:30

wking reviewed Jan 19, 2018

View reviewed changes

mrunalp closed this Nov 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for stracing containers #1253

Add support for stracing containers #1253

mrunalp commented Jan 12, 2018 •

edited

Loading

TomSweeneyRedHat left a comment

rhatdan Jan 12, 2018

mrunalp Jan 17, 2018

rhatdan Jan 12, 2018

mrunalp commented Jan 17, 2018

mrunalp commented Jan 17, 2018

openshift-ci-robot commented Jan 17, 2018 •

edited

Loading

wking Jan 18, 2018

wking Jan 18, 2018

mrunalp commented Jan 19, 2018

wking Jan 19, 2018

wking Jan 19, 2018

mrunalp commented Nov 28, 2018

		@@ -120,6 +120,8 @@ crio [GLOBAL OPTIONS] config [OPTIONS]

		--cpu-profile: Set the CPU profile file path

		--allow-strace: Allow strace for containers when the strace annotation/label "io.kubernetes.cri-o.Strace=true" is set

Add support for stracing containers #1253

Add support for stracing containers #1253

Conversation

mrunalp commented Jan 12, 2018 • edited Loading

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

rhatdan Jan 12, 2018

Choose a reason for hiding this comment

mrunalp Jan 17, 2018

Choose a reason for hiding this comment

rhatdan Jan 12, 2018

Choose a reason for hiding this comment

mrunalp commented Jan 17, 2018

mrunalp commented Jan 17, 2018

openshift-ci-robot commented Jan 17, 2018 • edited Loading

wking Jan 18, 2018

Choose a reason for hiding this comment

wking Jan 18, 2018

Choose a reason for hiding this comment

mrunalp commented Jan 19, 2018

wking Jan 19, 2018

Choose a reason for hiding this comment

wking Jan 19, 2018

Choose a reason for hiding this comment

mrunalp commented Nov 28, 2018

mrunalp commented Jan 12, 2018 •

edited

Loading

openshift-ci-robot commented Jan 17, 2018 •

edited

Loading