Incremental improvements to kubelet e2e tests #24426
Conversation
pwittrock
commented
Apr 18, 2016
pwittrock
commented
- Add keep-alive to ssh connection
- Don't try to stop services on image-based runs
- Increase jenkins ci timeout to 90 minutes to accomadate unpredictable go build times
- Remove spammy log statement
- Add keep-alive to ssh connection - Don't try to stop services on image-based runs - Increase jenkins ci timeout to 90 minutes to accomadate unpredictable go build times - Remove spammy log statement
k8s-github-robot
added
size/S
| @@ -151,7 +150,6 @@ func (config *PrivilegedPodTestConfig) dialFromContainer(containerIP string, con | |||
| var output map[string]string | |||
| err = json.Unmarshal([]byte(stdout), &output) | |||
| Expect(err).NotTo(HaveOccurred(), fmt.Sprintf("Could not unmarshal curl response: %s", stdout)) | |||
| glog.Infof("Deserialized output is %v", output) | |||
There was a problem hiding this comment.
This output was just not useful at all?
There was a problem hiding this comment.
cc @yujuhong
The tests that call this log the map if it does not match expectations. I don't think we want to output this when the tests pass since it clutters up the build log.
There was a problem hiding this comment.
I copied this from the original test. The output seems pretty meaningful. E.g., "privileged_test.go:154] Deserialized output is map[output:RTNETLINK answers: Operation not permitted"
Do we have any general rules for logging in a node e2e suite? A cluster e2e test logs quite a lot at the info level, but I've just noticed that it's very different in the node e2e suite.
There was a problem hiding this comment.
I expect that message is ignored unless the test fails, in which case it should be printed out by the assertion. That output is actually expected, but can make debugging unrelated failures hard because it looks like an error message even though it is printed at info level.
My opinion would be that anything besides the test results themselves are just debug information that we should only assume will be seen if there is a test failure. To the extent possible, debug information should only be printed if it is related to a failed tests.
There was a problem hiding this comment.
I agree in general that we should keep the output to the minimum, but fwiw, comparing the logs of a passed test against those of a failed test is helpful at times. I guess we won't need that if every test is well-written :)
|
GCE e2e build/test passed for commit 90d2f9a. |
eparis
added
lgtm
pwittrock
added
priority/important-soon
|
Added p1 label since this is causing flaky failures and should recieve higher priority |
|
is the flake fix just the timeout extension? |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
@k8s-bot test this issue: #IGNORE Tests have been pending for 24 hours |
|
GCE e2e build/test passed for commit 90d2f9a. |
|
Automatic merge from submit-queue |
