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

Node E2E: add termination message test #23969

Merged
merged 1 commit into from
Jun 20, 2016
Merged

Node E2E: add termination message test #23969

merged 1 commit into from
Jun 20, 2016

Conversation

Random-Liu
Copy link
Member

Based on #23658.

This PR:

  1. Cleans up the ConformanceContainer a bit
  2. Add termination message test

This test proves #23639, without #23658, the test could not pass.

@liangchenye @kubernetes/sig-node

@Random-Liu Random-Liu added area/test area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Apr 7, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 7, 2016
@Random-Liu Random-Liu assigned pwittrock and unassigned dchen1107 Apr 7, 2016
@Random-Liu
Copy link
Member Author

@k8s-bot test this issue #21367

@Random-Liu Random-Liu force-pushed the add-termination-message-test branch from ec563e3 to f456cd1 Compare April 7, 2016 08:10
@pwittrock
Copy link
Member

@Random-Liu Would you check this test failure test/e2e_node/container.go:51: undefined: nodeName

@Random-Liu
Copy link
Member Author

@pwittrock Just wanna to ask you about that. @chenyeliang met this before. The nodeName is just in the same directory.

@pwittrock
Copy link
Member

Symbols aren't exported from _test.go files :( If you move it to a non-test file I suspect it will work.

@Random-Liu
Copy link
Member Author

Thanks! Good to know that! :)

@Random-Liu Random-Liu force-pushed the add-termination-message-test branch from f456cd1 to 763d29d Compare April 7, 2016 19:46
@Random-Liu
Copy link
Member Author

@pwittrock Fixed, :)

@Random-Liu Random-Liu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 7, 2016
pwittrock
pwittrock reviewed Apr 8, 2016
View reviewed changes
test/e2e_node/conformance_test.go
Name: "termination-message-container",
Command: []string{"/bin/sh", "-c"},
Args: []string{"sleep 60 && /bin/echo -n " + terminationMessage + " > " + terminationMessagePath},
TerminationMessagePath: terminationMessagePath,
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to sleep 60 seconds? This is going to extend the runtime of the tests by a good bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, I didn't remember where I copied this line from. Thanks! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will correct the time soon.

@pwittrock
Copy link
Member

Review pass done.

@pwittrock pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2016
@Random-Liu Random-Liu force-pushed the add-termination-message-test branch from 763d29d to 1d915d9 Compare April 8, 2016 00:37
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2016
@Random-Liu
Copy link
Member Author

Will rebase and self apply LGTM after #23658 gets merged.

@pwittrock
Copy link
Member

SGTM

@Random-Liu Random-Liu mentioned this pull request Apr 8, 2016
Closed
@Random-Liu Random-Liu force-pushed the add-termination-message-test branch from 1d915d9 to fa93d74 Compare April 11, 2016 17:34
@Random-Liu
Copy link
Member Author

Random-Liu commented Apr 20, 2016 •
edited
Loading

@pwittrock I'm waiting for #24329, :) @liangchenye is refactoring the container conformance test in #24329 , and I'll rebase all my PRs based on his refactoring PR.

@pwittrock
Copy link
Member

Makes sense

@pwittrock
Copy link
Member

@Random-Liu Let me know when I should TAL. I assume you are busy for the next couple weeks with 1.3 stuff

@Random-Liu
Copy link
Member Author

@pwittrock Will rebase these PRs after code freeze. :)

@Random-Liu Random-Liu force-pushed the add-termination-message-test branch from fa93d74 to 6bbe063 Compare June 13, 2016 21:38
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2016
@Random-Liu
Copy link
Member Author

@pwittrock Rebased the PR. Sorry for the delay. :)

@Random-Liu Random-Liu changed the title Node E2E: Cleanup ConformanceContainer and add termination message test Add termination message test Jun 13, 2016
@Random-Liu Random-Liu changed the title Add termination message test Node E2E: add termination message test Jun 13, 2016
@pwittrock pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2016
@Random-Liu Random-Liu force-pushed the add-termination-message-test branch from 6bbe063 to 19f5a90 Compare June 17, 2016 20:43
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2016
@Random-Liu
Copy link
Member Author

I just did a simple rebase, apply lgtm again.

@Random-Liu Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2016
@Random-Liu Random-Liu added this to the v1.3 milestone Jun 17, 2016
@Random-Liu
Copy link
Member Author

It's nice to have this test coverage in 1.3, so mark 1.3.

@goltermann
Copy link
Contributor

E2E passed put check still listed as yellow and not in queue

@k8s-bot e2e test this issue: #IGNORE

@Random-Liu
Copy link
Member Author

@k8s-bot test this issue #27663

@goltermann
Copy link
Contributor

@k8s-bot node e2e test this

@goltermann
Copy link
Contributor

@k8s-bot e2e test this issue: #IGNORE

@k8s-bot
Copy link

k8s-bot commented Jun 20, 2016

GCE e2e build/test passed for commit 19f5a90.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 20, 2016

GCE e2e build/test passed for commit 19f5a90.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit bd9ac8d into kubernetes:master Jun 20, 2016
@Random-Liu Random-Liu deleted the add-termination-message-test branch June 20, 2016 04:51
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Oct 16, 2019
@k8s-publishing-bot
Merge pull request kubernetes#23969 from bertinatto/csi_perf
6f371ea 
Bug 1745773: UPSTREAM: 83747: Improve efficiency of csiMountMgr.GetAttributes

Origin-commit: a8701aabbac3d841addf8913f8dc58ba0209a9e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pwittrock pwittrock

Labels
area/kubelet area/test 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. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
8 participants
@Random-Liu @pwittrock @liangchenye @goltermann @k8s-bot @k8s-github-robot @googlebot @dchen1107