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

Fix subpath tests not to fail in namespace deletion #68074

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

mkimuram
Copy link
Contributor

What this PR does / why we need it:
This PR fixes below subpath test not to fail in namespace deletion

  • subPath should support restarting containers using directory as subpath
  • subPath should support restarting containers using file as subpath

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes: #68073

Special notes for your reviewer:
/sig storage
/sig testing

Release note:

NONE

This commit fixes below subpath test not to fail in namespace deletion

 - subPath should support restarting containers using directory as subpath
 - subPath should support restarting containers using file as subpath

Fixes: kubernetes#68073
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 30, 2018
@mkimuram
Copy link
Contributor Author

@msau42

PTAL

@davidz627
Copy link
Contributor

/ok-to-test
/assign

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 30, 2018
@davidz627
Copy link
Contributor

/assign @msau42

@davidz627
Copy link
Contributor

/test pull-kubernetes-e2e-gce

@tpepper
Copy link
Member

tpepper commented Aug 30, 2018

/milestone v1.12
/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Aug 30, 2018
@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Aug 30, 2018
@msau42
Copy link
Member

msau42 commented Aug 30, 2018

/lgtm

This is fine for quickly fixing the release blocking tests.

Let's still investigate why the AfterEach cleanup is not working.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mkimuram, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2018
@davidz627
Copy link
Contributor

davidz627 commented Aug 30, 2018

I looked up the error message we're seeing and found in the code:

// but they were all undergoing deletion (kubelet is probably culprit, check NodeLost)
return fmt.Errorf("namespace %v was not deleted with limit: %v, pods remaining: %v", namespace, err, remainingPods)

meaning the pods already had deletion timestamp but they were not deleted in time.

@msau42
Copy link
Member

msau42 commented Aug 30, 2018

That's from the namespace deletion that happens at the very end of the tests. But in the case of nfs and gluster, where the storage backends are pods themselves, this is too late because the storage backend pods get deleted at the end of the test. So if the client Pod gets deleted after the storage backend Pod, then the client Pod deletion will hang forever.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 67368, 59930, 68074). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit 90fed8e into kubernetes:master Aug 30, 2018
@mkimuram
Copy link
Contributor Author

@davidz627 @msau42

Let's still investigate why the AfterEach cleanup is not working.

I confirmed that after #68569 is applied, even deleting this commit doesn't make error.
So, the root cause of this issue seems failure in updating resource that will be used in AfterEach. And it is fixed there.

Although this commit isn't necessarily needed now, but it isn't harmful and is consistent with other test functions. Therefore, I think that we can keep this commit as it is.

I think that we can regard that issue #68073 is now resolved completely.

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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subpath tests fails in namespace deletion due to remaining pod
6 participants