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

Remove unused WaitForDetach from Detacher interface and plugins #35629

Merged

Conversation

kiall
Copy link

@kiall kiall commented Oct 26, 2016

See issue #33128 and PR #33270

We can't rely on the device name provided by OpenStack Cinder, and thus
must perform detection based on the drive serial number (aka It's cinder ID)
on the kubelet itself.

This needs to be removed now, as part of #33128, as the code can't be
updated to attempt device detection and fallback through to the Cinder
provided deviceName, as detection "fails" when the device is gone, and
if cinder has reported a deviceName that another volume has used in
relaity, then this will block forever (or until the other, unreleated,
volume has been detached)


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@grahamhayes
Copy link
Contributor

@kubernetes/sig-storage

@grahamhayes
Copy link
Contributor

@k8s-bot ok to test

@grahamhayes
Copy link
Contributor

@kubernetes/sig-openstack

@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 Oct 26, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 66fed5fcb3338ccd5a691167bac2eda7dc184106. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 66fed5fcb3338ccd5a691167bac2eda7dc184106. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 66fed5fcb3338ccd5a691167bac2eda7dc184106. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@rootfs
Copy link
Contributor

rootfs commented Oct 26, 2016

@saad-ali @jingxu97 Do you have any plan for WaitForDetach?

@jsafrane jsafrane assigned saad-ali and jingxu97 and unassigned jsafrane Oct 27, 2016
@jingxu97
Copy link
Contributor

@saad-ali, do you know the original purpose/use of WaitForDetach function?

@kiall
Copy link
Author

kiall commented Oct 27, 2016

Heh, all tests failing. there must be some dependency on some changes from within the original PR this was extracted from, I'll fixup tomorrow.

@saad-ali
Copy link
Member

Thanks for the cleanup @kiall. LGTM. Once the tests are fixed, I'll add LGTM label.

@jingxu97 It's a vestigial of previous refactors. It is not actually used anywhere in our code since the "Detach()" interface blocks until detach is completed or fails. Therefore, it is safe to remove altogether.

This has been unused since 542f2dc, and relies on deviceName, which
can no longer be relied upon (see issue kubernetes#33128).

This needs to be removed now, as part of kubernetes#33128, as the code can't be
updated to attempt device detection and fallback through to the Cinder
provided deviceName, as detection "fails" when the device is gone, and
if cinder has reported a deviceName that another volume has used in
relaity, then this will block forever (or until the other, unreleated,
volume has been detached)
@kiall kiall force-pushed the bug/33128-unused-waitfordetach branch from 66fed5f to ccb8d53 Compare November 2, 2016 11:00
@kiall
Copy link
Author

kiall commented Nov 2, 2016

@saad-ali all done, should be ready to LGTM now

@dims
Copy link
Member

dims commented Nov 3, 2016

@thockin @saad-ali : can you please add milestone 1.5 for this one? and switch the release-note-label-needed tag to release-note-none please? oh please lgtm as well :)

@saad-ali saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Nov 4, 2016
@saad-ali saad-ali added this to the v1.5 milestone Nov 4, 2016
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 33dab1d into kubernetes:master Nov 6, 2016
k8s-github-robot pushed a commit that referenced this pull request Jan 3, 2018
Automatic merge from submit-queue (batch tested with PRs 57366, 57779). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove unused command waitfordetach from flex volume driver

**What this PR does / why we need it**:
Clean unused code.

**Special notes for your reviewer**:
See #35629
Original PR is #50754 , the PR's repo has bean deleted,so I create a new PR and merge upstream for test.
**Release note**:

```release-note
NONE
```
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
…tfordetach

Automatic merge from submit-queue

Remove unused WaitForDetach from Detacher interface and plugins

See issue kubernetes#33128 and PR kubernetes#33270

We can't rely on the device name provided by OpenStack Cinder, and thus
must perform detection based on the drive serial number (aka It's cinder ID)
on the kubelet itself.

This needs to be removed now, as part of kubernetes#33128, as the code can't be
updated to attempt device detection and fallback through to the Cinder
provided deviceName, as detection "fails" when the device is gone, and
if cinder has reported a deviceName that another volume has used in
relaity, then this will block forever (or until the other, unreleated,
volume has been detached)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants