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

Jump to bottom

add kubelet tests to verify host clean up #41659

Merged
merged 4 commits into from Apr 13, 2017
Merged

add kubelet tests to verify host clean up #41659

merged 4 commits into from Apr 13, 2017

Conversation

jeffvance
Copy link
Contributor

@jeffvance jeffvance commented Feb 17, 2017 •
edited

What this PR does / why we need it:
Increasingly we're seeing more failures in persistent volume e2e tests where pv tests are run in parallel with disruptive tests. The quick solution is to tag the pv tests as Flaky. This pr addresses one cause of the flakiness and adds a disruptive kubelet test.
Once this pr is shown to not produce flakes the [Flaky] tag for the "HostCleanup" tests will be removed in a separate pr.

  • Adds volume tests to kubelet.go motivated by issues 31272 and 37657
  • Addresses reverted pr 41178 and negates the need for pr 41229

Which issue this PR fixes
Adds regression tests to cover issues: #31272 and #37657

Special notes for your reviewer:
It's possible that one of the new tests, which relies on the existence of /usr/sbin/rpc.nfsd in the nfs-server pod, will not work in the GCI container env. If this turns out to be true then I will add a SkipIfProviderIs("gke") to the It block.

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 Feb 17, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Feb 17, 2017
@jeffvance
Copy link
Contributor Author

cc @krousey @rmmh @copejon

@krousey
Copy link
Contributor

krousey commented Feb 17, 2017

/approve

@saad-ali @copejon to review

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2017
@krousey krousey assigned saad-ali and unassigned krousey Feb 17, 2017
@jeffvance
Copy link
Contributor Author

jeffvance commented Feb 17, 2017 •
edited

@copejon all tests pass. Can you review the KubeDescribe and Context text and the last It block? Thx.

@jeffvance
Copy link
Contributor Author

@saad-ali can this pr get a review (and maybe an additional assignee)? Thanks!
cc @copejon

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2017
This was referenced Feb 23, 2017
Closed
Merged
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2017
@jeffvance
Copy link
Contributor Author

jeffvance commented Feb 25, 2017 •
edited

all tests pass... @jsafrane @copejon @divyenpatel PTAL, thanks.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@jeffvance
Copy link
Contributor Author

rebased...

@jeffvance
Copy link
Contributor Author

@k8s-bot test this

@jeffvance
Copy link
Contributor Author

@k8s-bot gci gce e2e test this

@jeffvance
Copy link
Contributor Author

jeffvance commented Feb 28, 2017 •
edited

rebased, all tests pass. PTAL @jsafrane @saad-ali @krousey @divyenpatel ...someone! . Thanks

@jeffvance jeffvance changed the title add disruptive tests to verify host clean up. Issues: #31272, #37657. add disruptive tests to verify host clean up. Mar 1, 2017
@jeffvance jeffvance changed the title add disruptive tests to verify host clean up. add disruptive tests to verify host clean up and remove [Flaky] Mar 1, 2017
copejon
copejon reviewed Mar 1, 2017
View reviewed changes
test/e2e/kubelet.go Outdated
@@ -395,15 +441,19 @@ var _ = framework.KubeDescribe("kubelet", func() {
testTbl := []hostCleanupTest{
{
itDescr: "after deleting the nfs-server, the host should be cleaned-up when deleting sleeping pod which mounts an NFS vol",
Copy link
Contributor

Choose a reason for hiding this comment

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

Very small nit - this could be clearer by saying something like

"after stopping the nfs server pod, deleting the client pod should remove it's pod directory from the node.

@jeffvance
Copy link
Contributor Author

@k8s-bot gce etcd3 e2e test this

@jeffvance
Copy link
Contributor Author

@k8s-bot test this

@jeffvance
Copy link
Contributor Author

@k8s-bot cancel

@jeffvance
Copy link
Contributor Author

@saad-ali @jingxu97 can this pr get a /lgtm? Freshly rebased and all tests pass. Thanks.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2017
@jeffvance
Copy link
Contributor Author

@jingxu97 @saad-ali rebased (again) and looking for a lgtm. Thanks.

@jeffvance
Copy link
Contributor Author

@k8s-bot test this

@jeffvance jeffvance force-pushed the kubelet-wedge2 branch 2 times, most recently from 5899e93 to 0274bd9 Compare April 10, 2017 22:38
@jeffvance
Copy link
Contributor Author

@saad-ali @jingxu97 Can one of you please lgtm this pr. I submitted it almost 2 months ago and have done 4 rebases since then. If there's an issue I'm happy to fix it but I really want to get this pr unstuck. Thanks,

@saad-ali
Copy link
Member

Sorry for the delay @jeffvance. I took a look and the ...host cleanup with volume mounts... tests still look consistently red in the flaky test suites:

Am I missing something?

@jeffvance
Copy link
Contributor Author

jeffvance commented Apr 11, 2017 •
edited

@saad-ali Thanks. The test grids above are for Flaky tests but this pr removes the [Flaky] tag and would not be part of the flaky suite. Other than testing on GCE and GKE what else is needed for this pr? I will create a new commit that adds back the [Flaky] tag so that it can be proven (or not) that this pr removes the need for [Flaky].

@jeffvance jeffvance changed the title add disruptive tests to verify host clean up and remove [Flaky] tag add disruptive tests to verify host clean up Apr 11, 2017
@jeffvance
Copy link
Contributor Author

@saad-ali all tests for this pr pass. If you lgtm we can see if the red flaky tests for "HostCleanup" work. thanks

@jeffvance jeffvance changed the title add disruptive tests to verify host clean up add kubelet tests to verify host clean up Apr 12, 2017
@jeffvance
Copy link
Contributor Author

@k8s-bot kubemark e2e test this

@saad-ali
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 Apr 12, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeffvance, krousey, saad-ali

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
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b0a7241 into kubernetes:master Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@copejon copejon copejon left review comments

@jingxu97 jingxu97 jingxu97 left review comments

@msau42 msau42 msau42 left review comments

Assignees

@saad-ali saad-ali

@jingxu97 jingxu97

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@jeffvance @k8s-reviewable @krousey @saad-ali @jingxu97 @divyenpatel @k8s-github-robot @copejon @msau42 @k8s-ci-robot