-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
Fixed GCE PD tests to cleanup pods and disk properly after testing #68581
Fixed GCE PD tests to cleanup pods and disk properly after testing #68581
Conversation
3486701
to
f206833
Compare
/kind bug |
// - Force detach and delete. | ||
pod, err := f.PodClient().Get(config.Prefix+"-client", metav1.GetOptions{}) | ||
Expect(err).NotTo(HaveOccurred(), "Failed getting pod %q.", config.Prefix+"-client") | ||
detachAndDeletePDs(volumeName, []types.NodeName{types.NodeName(pod.Spec.NodeName)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any other tests using this detach method? Can we remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are other tests using it, but I don't think they will encounter the same problem as we're seeing in the parent issue. The tests all wait for detach before running their defer with the detach method, it seems like its just "insurance" in the defer to cleanup after the test. I believe its out of scope of this PR to remove that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can you open an issue to clean it up later? I dislike the e2e doing the detach because it requires the e2e to know the node that the test pod got scheduled to, and deleting the pod should effectively do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f206833
to
7bffc1b
Compare
@msau42 changed the PR and comments addressed |
…orce detach PD's from nodes
7bffc1b
to
e38aff2
Compare
ALL these changes should be cherrypicked to
|
/lgtm |
/assign @MaciekPytel |
/retest |
Thanks for fixing this so quickly! |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidz627, msau42, shyamjvs 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 |
…581-upstream-release-1.11 Automated cherry pick of #68581: Fixed GCE PD tests to wait for pod deletion after usage, and
See Title
Might solve: #68570
ALL these changes should be cherrypicked to 1.11.
VolumeTestCleanup changes should be cherrypicked to 1.11, 1.12, and master
/assign @msau42