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.

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

Summary Test looks at pods that have containers that restart. #46308

Merged

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented May 23, 2017

Occasionally, the node can report extra containers that had been restarted through the summary API.
This test change tests a pod that restarts, and hopefully should allow us to reproduce and debug this behavior.

/assign @dchen1107

/release-note-none

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 23, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 23, 2017
@dashpole
Copy link
Contributor Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 23, 2017
@dashpole
Copy link
Contributor Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@k8s-ci-robot
Copy link
Contributor

@dashpole: The following test(s) failed:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce 1a6572f link @k8s-bot pull-kubernetes-federation-e2e-gce test this

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.

@dashpole
Copy link
Contributor Author

@dchen1107 this looks like it is ready for review. The federation test doesnt seem to pass for whatever reason

createSummaryTestPods(f, pod0, pod1)
// Wait for cAdvisor to collect 2 stats points
time.Sleep(15 * time.Second)
pods := getSummaryTestPods(f, pod0, pod1)
Copy link
Member

Choose a reason for hiding this comment

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

Can you point to me where we ensure there is no duplicate stats for a single container within your test pod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To match the containers in a pod, we use the MatchAllElements method of gstruct. This matches elements in a list, with one match allowed for each. See the tests for gstruct. So we were already checking for duplicate elements before. We just never tested on a restarted container.

@dchen1107
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46429, 46308, 46395, 45867, 45492)

@k8s-github-robot k8s-github-robot merged commit 54a47a6 into kubernetes:master May 26, 2017
@gmarek
Copy link
Contributor

gmarek commented May 26, 2017

@dashpole @dchen1107 This broke cross-build.

cc @luxas

@dashpole dashpole deleted the summary_container_restart branch May 26, 2017 15:29
@dashpole
Copy link
Contributor Author

@gmarek can you show me a failure? What broke it?

@dashpole
Copy link
Contributor Author

finally managed to run make cross
test/e2e_node/summary_test.go:138: constant 100000000000 overflows int is the error. Ill put up a fix for it.

k8s-github-robot pushed a commit that referenced this pull request May 31, 2017
Automatic merge from submit-queue

Fix Cross-Build, and reduce test to 1 restart to reduce flakyness

In response to #46308 (comment)

This fixes the error: `test/e2e_node/summary_test.go:138: constant 100000000000 overflows int` from the cross build.

This [recent flake](https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-node-kubelet/4179) was because the container restarted during the period where the test expected to Continually see the container in the Summary API.

/assign @dchen1107 
cc @gmarek @luxas 

/release-note-none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants