Made tracing of calls and container lifecycle steps in FakeDockerClient optional #39826

shyamjvs · 2017-01-12T21:52:35Z

Slightly refactored the FakeDockerClient code and made tracing optional (but enabled by default).

k8s-reviewable · 2017-01-12T21:52:42Z

This change is

shyamjvs · 2017-01-12T21:55:46Z

@yujuhong There are also some unused functions like AssertCreated, AssertStarted, AssertStopped, etc. Should we just let them stay there as available features?

Random-Liu · 2017-01-12T21:54:59Z

pkg/kubelet/dockertools/fake_docker_client.go

 }

-func NewFakeDockerClientWithClock(c clock.Clock) *FakeDockerClient {
-	return newClientWithVersionAndClock(fakeDockerVersion, minimumDockerAPIVersion, c)
+func (f *FakeDockerClient) WithClock(c clock.Clock) *FakeDockerClient {


We need to lock here and the following.

Nice catch!

Random-Liu · 2017-01-12T21:57:32Z

pkg/kubelet/dockertools/fake_docker_client.go

 }

-func NewFakeDockerClientWithVersion(version, apiVersion string) *FakeDockerClient {


There are some other places using this function. You also need to update them.
For example

kubernetes/pkg/kubelet/dockertools/images_test.go

Line 27 in 8fd4145

fakeDockerClient := NewFakeDockerClientWithVersion("1.2.3", "1.2")

.

Oops! Sorry for the blooper.

Random-Liu · 2017-01-12T21:57:59Z

pkg/kubelet/dockertools/fake_docker_client.go

+	return f
+}
+
+func (f *FakeDockerClient) AppendCalledWith(callDetail calledDetail) {


AppendCalled

Random-Liu · 2017-01-12T21:58:08Z

pkg/kubelet/dockertools/fake_docker_client.go

+	}
+}
+
+func (f *FakeDockerClient) AppendPulledWith(pull string) {


AppendPulled

Random-Liu · 2017-01-12T21:58:25Z

pkg/kubelet/dockertools/fake_docker_client.go

+	}
+}
+
+func (f *FakeDockerClient) AppendContainerTraceWithEntry(traceCategory string, containerName string) {


AppendContainerTrace

Random-Liu · 2017-01-12T21:58:49Z

pkg/kubelet/dockertools/fake_docker_client.go

+}
+
+func (f *FakeDockerClient) AppendContainerTraceWithEntry(traceCategory string, containerName string) {
+	if f.EnableTrace == false {


nit: !f.EnableTrace.

Random-Liu · 2017-01-12T21:59:09Z

pkg/kubelet/dockertools/fake_docker_client.go

+}
+
+func (f *FakeDockerClient) AppendPulledWith(pull string) {
+	if f.EnableTrace == true {


nit: if f.EnableTrace {

k8s-ci-robot · 2017-01-12T22:40:33Z

Jenkins Bazel Build failed for commit 194cc24234aed82776618264888123fa47ea4b37. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake 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.

k8s-ci-robot · 2017-01-12T23:04:38Z

Jenkins unit/integration failed for commit 194cc24234aed82776618264888123fa47ea4b37. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake 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.

shyamjvs · 2017-01-12T23:07:20Z

@Random-Liu There is a very weird reason why the unit tests are failing. So in this line in container_gc_test.go file, we do a new(FakeDockerClient) which initializes the struct with all fields set to their zero value. So this disables tracing and hence these tests fail later on, when they assert non-empty expected traces to be same as these actual empty traces.

Should we change the field in FakeDockerClient to DisableTrace or just change the line from fakeDocker := new(FakeDockerClient) to fakeDocker := NewFakeDockerClient()? Second option seems more reasonable but first one might be safer for future.

Random-Liu · 2017-01-13T19:23:36Z

@shyamjvs Just change this line should be fine. I don't think the original code is right. We should always use initializer to create the object.

shyamjvs · 2017-01-17T07:11:24Z

@Random-Liu Can you PTAL now? Fixed the issue.

Random-Liu

LGTM with one nit

Random-Liu · 2017-01-17T16:56:27Z

pkg/kubelet/dockertools/fake_docker_client.go

+	return f
+}
+
+func (f *FakeDockerClient) AppendCalled(callDetail calledDetail) {


Use private function, because this function should only be used internally.

Done. Thanks for pointing.

Random-Liu · 2017-01-17T16:56:34Z

pkg/kubelet/dockertools/fake_docker_client.go

+	}
+}
+
+func (f *FakeDockerClient) AppendPulled(pull string) {


Random-Liu · 2017-01-17T16:56:40Z

pkg/kubelet/dockertools/fake_docker_client.go

+	}
+}
+
+func (f *FakeDockerClient) AppendContainerTrace(traceCategory string, containerName string) {


…nt optional

shyamjvs · 2017-01-17T17:58:18Z

Adding lgtm label based on @Random-Liu 's comment.

Random-Liu · 2017-01-18T02:07:38Z

@k8s-bot test this please.

k8s-github-robot · 2017-01-18T05:11:36Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 12, 2017

k8s-github-robot assigned lavalamp Jan 12, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jan 12, 2017

Random-Liu reviewed Jan 12, 2017

View reviewed changes

Random-Liu assigned Random-Liu and unassigned lavalamp Jan 12, 2017

shyamjvs force-pushed the fake-docker-client-fix branch from df90817 to 194cc24 Compare January 12, 2017 22:32

Random-Liu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jan 13, 2017

shyamjvs force-pushed the fake-docker-client-fix branch from 194cc24 to e6f8a4b Compare January 16, 2017 22:19

Random-Liu reviewed Jan 17, 2017

View reviewed changes

Made tracing of calls and container lifecycle steps in FakeDockerClie…

814c850

…nt optional

shyamjvs force-pushed the fake-docker-client-fix branch from e6f8a4b to 814c850 Compare January 17, 2017 17:56

shyamjvs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2017

k8s-github-robot merged commit 180936f into kubernetes:master Jan 18, 2017

shyamjvs deleted the fake-docker-client-fix branch January 18, 2017 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made tracing of calls and container lifecycle steps in FakeDockerClient optional #39826

Made tracing of calls and container lifecycle steps in FakeDockerClient optional #39826

shyamjvs commented Jan 12, 2017

k8s-reviewable commented Jan 12, 2017

shyamjvs commented Jan 12, 2017

Random-Liu Jan 12, 2017

shyamjvs Jan 12, 2017

shyamjvs Jan 12, 2017

Random-Liu Jan 12, 2017

shyamjvs Jan 12, 2017

Random-Liu Jan 12, 2017

shyamjvs Jan 12, 2017

Random-Liu Jan 12, 2017

shyamjvs Jan 12, 2017

Random-Liu Jan 12, 2017

shyamjvs Jan 12, 2017

Random-Liu Jan 12, 2017

shyamjvs Jan 12, 2017

Random-Liu Jan 12, 2017

shyamjvs Jan 12, 2017

k8s-ci-robot commented Jan 12, 2017

k8s-ci-robot commented Jan 12, 2017

shyamjvs commented Jan 12, 2017 •

edited

Random-Liu commented Jan 13, 2017

shyamjvs commented Jan 17, 2017

Random-Liu left a comment

Random-Liu Jan 17, 2017

shyamjvs Jan 17, 2017

Random-Liu Jan 17, 2017

shyamjvs Jan 17, 2017

Random-Liu Jan 17, 2017

shyamjvs Jan 17, 2017

shyamjvs commented Jan 17, 2017

Random-Liu commented Jan 18, 2017

k8s-github-robot commented Jan 18, 2017

		}

		func NewFakeDockerClientWithVersion(version, apiVersion string) *FakeDockerClient {

Made tracing of calls and container lifecycle steps in FakeDockerClient optional #39826

Made tracing of calls and container lifecycle steps in FakeDockerClient optional #39826

Conversation

shyamjvs commented Jan 12, 2017

k8s-reviewable commented Jan 12, 2017

shyamjvs commented Jan 12, 2017

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

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

k8s-ci-robot commented Jan 12, 2017

k8s-ci-robot commented Jan 12, 2017

shyamjvs commented Jan 12, 2017 • edited

Random-Liu commented Jan 13, 2017

shyamjvs commented Jan 17, 2017

Random-Liu left a 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

shyamjvs commented Jan 17, 2017

Random-Liu commented Jan 18, 2017

k8s-github-robot commented Jan 18, 2017

shyamjvs commented Jan 12, 2017 •

edited