Update VolumeAttachment.Finalizers as separate subresource in markAsDetatched by mattcary · Pull Request #227 · kubernetes-csi/external-attacher

mattcary · 2020-04-22T19:14:22Z

/kind bug

What this PR does / why we need it:
The update of the VolumeAttachment version from v1beta to v1 changed its status to be a subresource. markAsDetached() updated both the status and the finalizers in a single patch, and the update to v1 blindly put in a "status" subresource restriction, causing the finalizer removal to be silently ignored. This change removes the finalizer in a second patch.

Which issue(s) this PR fixes:
Fixes #225

NONE

(release note edited by jsafrane, this bug has not been in any released external-attacher)

k8s-ci-robot · 2020-04-22T19:14:30Z

Welcome @mattcary!

It looks like this is your first PR to kubernetes-csi/external-attacher 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-attacher has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-04-22T19:14:30Z

Hi @mattcary. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /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 by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

msau42 · 2020-04-22T19:18:53Z

/ok-to-test

msau42 · 2020-04-22T20:45:28Z

Test cases under https://github.com/kubernetes-csi/external-attacher/blob/master/pkg/controller/csi_handler_test.go need to be updated with the additional patch action for removing the finalizer.

I think we can also look into this as a separate followup, but I don't see the https://github.com/kubernetes-csi/external-attacher/blob/master/pkg/controller/framework_test.go checking for volumattachment object deletion. A unit test checking for deletion probably could have caught this issue. cc @jsafrane

mattcary · 2020-04-22T22:22:25Z

Tests are fixed.

For correctly checking that the volumes are detached, would it be enough to just check through the vaInformer that there are no such resources (modulo adding a flag or something for test cases that we expect to leave VAs hanging about)?

msau42 · 2020-04-22T23:34:51Z

I'm not sure the fake client/informer is going to handle deletion of the object after we remove all the finalizers. Maybe something we can at least check for in some test case, is that the finalizer is cleared from the object?

msau42 · 2020-04-22T23:39:42Z

+					types.MergePatchType, patch(deleted(va(false, "", ann)),
+						deleted(vaWithDetachError(va(false, "", ann), "persistentvolume \"pv1\" not found"))),
+					"status"),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,


This is odd. This test case is supposed to fail the call the first two times, and then succeed on the 3rd.

Weird indeed. The test passes with >= 1 actions and only fails when the first action is removed. I'm trying to puzzle out what's going on.

Ok, I figured out what was going on with the test. Turns out it wasn't actually testing what it said it was (wrong verb in the reactor), and even then the expected actions weren't behaving as they probably were expected to. I've clarified it all in a comment (or tried to).

Ok, the test case itself is kind of strange, but I can see the benefit for adding an additional call to verify that we do retry.

My one last nit is that there's actually 5 loops but the comment says 4.

mattcary · 2020-04-24T05:35:58Z

I'm not sure the fake client/informer is going to handle deletion of the object after we remove all the finalizers. Maybe something we can at least check for in some test case, is that the finalizer is cleared from the object?

So the actions/fake clientset don't actually change any of the resource AFAIU. In order to check that the finalizer is cleared, we'd have to manually apply the patch. However, the Patch Action doesn't seem to expose any subresources, so we couldn't accurately test the main thing we want to test. At least that I can tell. sigh. The idea is close; framework_test.go does exactly that for Updates.

I'm open to further ideas if you have them.

msau42 · 2020-04-24T16:17:04Z

Yeah I don't have any great ideas. Opened #228 to track investigating possible solutions.

One more last thing, can you squash your commits to 1?

This also fixes a pre-existing bug in "detach unknown PV -> error + error saving the error" which had an incorrect reactor verb and wasn't testing what it thought it was.

mattcary · 2020-04-24T17:27:53Z

Squashed.

msau42 · 2020-04-24T17:39:48Z

/lgtm
/approve
Thanks!

k8s-ci-robot · 2020-04-24T17:39:52Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [msau42]

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

1df23dba Merge pull request kubernetes-csi#230 from msau42/prow 1f92b7e7 Add ginkgo timeout to e2e tests to help catch any stuck tests c10b6780 Merge pull request kubernetes-csi#227 from coulof/check-sidecar-supported-versions b0555351 Header bd0a10b6 typo c39d73c3 Add comments f6491af0 Script to verify EOL sidecar version git-subtree-dir: release-tools git-subtree-split: 1df23dba61da5d4e52ae79e6e1571f9d1d94311d

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 22, 2020

k8s-ci-robot requested review from jsafrane and msau42 April 22, 2020 19:14

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 22, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 22, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 22, 2020

msau42 reviewed Apr 22, 2020

View reviewed changes

Update finalizer in separate subresource in detatch

f0cede9

This also fixes a pre-existing bug in "detach unknown PV -> error + error saving the error" which had an incorrect reactor verb and wasn't testing what it thought it was.

mattcary force-pushed the finalizer-fix branch from 62b43e7 to f0cede9 Compare April 24, 2020 17:26

k8s-ci-robot assigned msau42 Apr 24, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2020

k8s-ci-robot merged commit b2caf3d into kubernetes-csi:master Apr 24, 2020

mattcary mentioned this pull request May 14, 2020

REQUEST: New membership for mattcary kubernetes/org#1877

Closed

2 tasks

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 29, 2020

Conversation

mattcary commented Apr 22, 2020 • edited by jsafrane Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 22, 2020

Uh oh!

k8s-ci-robot commented Apr 22, 2020

Uh oh!

msau42 commented Apr 22, 2020

Uh oh!

msau42 commented Apr 22, 2020

Uh oh!

mattcary commented Apr 22, 2020

Uh oh!

msau42 commented Apr 22, 2020

Uh oh!

msau42 Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

mattcary Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

mattcary Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

msau42 Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

mattcary Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

mattcary commented Apr 24, 2020

Uh oh!

msau42 commented Apr 24, 2020

Uh oh!

mattcary commented Apr 24, 2020

Uh oh!

msau42 commented Apr 24, 2020

Uh oh!

k8s-ci-robot commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattcary commented Apr 22, 2020 •

edited by jsafrane

Loading