Fixes ReconcileVA for non-migratable drivers #244

yuga711 · 2020-08-06T05:26:07Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

This PR fixes an issue in the volume attachment reconciler. Currently, the reconciler calls a method (unconditionally) that is supposed to be called only when the corresponding volume driver implements the in-tree plugin.
The PR also fixes a few existing broken tests.

Which issue(s) this PR fixes:

Fixes #243

Special notes for your reviewer:

Tested the fix with csi-mock driver. The driver does not implement in-tree plugin.
Without the fix, the reconciler failed as below:

W0806 02:35:26.595059       1 csi_handler.go:159] Failed to repair volume handle  for driver io.kubernetes.storage.mock: could not find In-Tree driver name for CSI plugin io.kubernetes.storage.mock

With the fix, no such failure was observed.

Does this PR introduce a user-facing change?:

Fixes an issue in volume attachment reconciler when the CSI driver supports LIST_VOLUMES_PUBLISHED_NODES but does not implement CSI migration.

k8s-ci-robot · 2020-08-06T05:26:15Z

Welcome @yuga711!

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-08-06T05:26:15Z

Hi @yuga711. 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.

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.

pkg/controller/csi_handler.go

jsafrane · 2020-08-06T09:38:55Z

/ok-to-test

vendor/modules.txt

jsafrane · 2020-08-06T09:55:01Z

Can you please check the unit tests? They test with "com.test.foo" driver. Why did they succeed? Should they be fixed? Or can you please add a new unit test that covers your code change?

msau42

Can you please check the unit tests? They test with "com.test.foo" driver. Why did they succeed? Should they be fixed? Or can you please add a new unit test that covers your code change?

I was curious about this too. We seem to already have unit tests for ReconcileVA. I think it's because the fake for RepairVolumeHandle always succeeds. I wonder though, why do we need a fake translator? Can we use the real one?

pkg/controller/csi_handler.go

msau42 · 2020-08-06T16:42:42Z

Can you update the PR title and description to be more specific to the issue, something like "Fixes ReconcileVA for non-migratable drivers"

yuga711 · 2020-08-12T17:45:11Z

Can you please check the unit tests? They test with "com.test.foo" driver. Why did they succeed? Should they be fixed? Or can you please add a new unit test that covers your code change?

I was curious about this too. We seem to already have unit tests for ReconcileVA. I think it's because the fake for RepairVolumeHandle always succeeds. I wonder though, why do we need a fake translator? Can we use the real one?

Switched to real translator. Added new checks. Fixed several broken existing tests.

msau42

I haven't taken a deep look at the test changes yet, will do that tomorrow

pkg/controller/csi_handler.go

pkg/controller/framework_test.go

msau42 · 2020-08-13T01:56:09Z

pkg/controller/csi_handler_test.go

@@ -34,6 +34,7 @@ import (
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
 	core "k8s.io/client-go/testing"
+	csitranslator "k8s.io/csi-translation-lib"


Can we add a test case for ReconcileVA using a volume type that does support migration?

These are the ones that truly support in the actual translator. Do you suggest unit tests based on this? (or) we could setup a fake translator that migration support. If it's not simple work, can we do this separately?

I think adding a unit test using one of the supported translated types would be good.

msau42 · 2020-08-13T01:58:06Z

Did you try running the fixed tests without your fix to ReconcileVA, to see if the test fails as expected?

yuga711 · 2020-08-13T02:49:41Z

Did you try running the fixed tests without your fix to ReconcileVA, to see if the test fails as expected?

Yes, the tests fail as expected when I ran them without fix. ie the tests complain saying the expected number of csi calls were not run...

pkg/controller/framework_test.go

pkg/controller/csi_handler_test.go

jsafrane · 2020-08-13T08:30:49Z

pkg/controller/csi_handler_test.go

+						va(false /*attached*/, fin, nil))),
+				core.NewPatchSubresourceAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
+					types.MergePatchType, patch(va(false /*attached*/, "", nil),
+						va(false, "", nil)), "status"),


This change is odd:

The patch is empty. My tests show that the attacher indeed executes empty PATCH {}. Is there a bug in the attacher?

While the resulting patch is empty, the way it's constructed is wrong - it expects that the volume gets detached (va(false, ...) - should it be true? The volume was attached successfully.

Haven't found if there is a bug executing empty patch.
Fixed the patch construction logic in the test.

pkg/controller/csi_handler_test.go

jsafrane · 2020-08-18T07:15:08Z

pkg/controller/csi_handler_test.go

+		/* TBD
+		{
+			name:           "GCE PD va attached actual state not attached",


In general, the patch looks good, is this the only thing missing?

I wanted to add a test with one of the supported translator types. Now planning to do that separately. I have created a github issue and added a TODO here.

In general, the patch looks good, is this the only thing missing?
I wanted to add a test with one of the supported translator types. Now planning to do that separately. I have created a github issue and added a TODO here.

jsafrane · 2020-08-19T07:44:08Z

/lgtm
/approve

k8s-ci-robot · 2020-08-19T07:44:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, yuga711

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:

~~OWNERS~~ [jsafrane]

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

b54c1ba4 Merge pull request kubernetes-csi#246 from xing-yang/go_1.21 5436c81e Change go version to 1.21.5 267b40e9 Merge pull request kubernetes-csi#244 from carlory/sig-storage b42e5a2d nominate self (carlory) as kubernetes-csi reviewer a17f536f Merge pull request kubernetes-csi#210 from sunnylovestiramisu/sidecar 011033de Use set -x instead of die 5deaf667 Add wrapper script for sidecar release git-subtree-dir: release-tools git-subtree-split: b54c1ba49469d4d5d1b5d75285e8868ffe3d328f

dc4d0ae2 Merge pull request kubernetes-csi#249 from jsafrane/use-go-version e681b170 Use .go-version to get Kubernetes go version b54c1ba4 Merge pull request kubernetes-csi#246 from xing-yang/go_1.21 5436c81e Change go version to 1.21.5 267b40e9 Merge pull request kubernetes-csi#244 from carlory/sig-storage b42e5a2d nominate self (carlory) as kubernetes-csi reviewer a17f536f Merge pull request kubernetes-csi#210 from sunnylovestiramisu/sidecar 011033de Use set -x instead of die 5deaf667 Add wrapper script for sidecar release f8c8cc4c Merge pull request kubernetes-csi#237 from msau42/prow b36b5bfd Merge pull request kubernetes-csi#240 from dannawang0221/upgrade-go-version adfddcc9 Merge pull request kubernetes-csi#243 from pohly/git-subtree-pull-fix c4650889 pull-test.sh: avoid "git subtree pull" error 7b175a1e Update csi-test version to v5.2.0 987c90cc Update go version to 1.21 to match k/k 2c625d41 Add script to generate patch release notes f9d5b9c0 Merge pull request kubernetes-csi#236 from mowangdk/feature/bump_csi-driver-host-path_version b01fd537 Bump csi-driver-host-path version up to v1.12.0 984feece Merge pull request kubernetes-csi#234 from siddhikhapare/csi-tools 1f7e6059 fixed broken links of testgrid dashboard git-subtree-dir: release-tools git-subtree-split: dc4d0ae20a3dcce17fbfc745fb1f1e3b10cd9644

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 Aug 6, 2020

k8s-ci-robot requested review from msau42 and saad-ali August 6, 2020 05:26

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 6, 2020

yuga711 commented Aug 6, 2020

View reviewed changes

pkg/controller/csi_handler.go Outdated Show resolved Hide resolved

yuga711 mentioned this pull request Aug 6, 2020

Add CSI LIST_VOLUMES_PUBLISHED_NODES to the controller in mock kubernetes-csi/csi-test#280

Merged

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 Aug 6, 2020

jsafrane reviewed Aug 6, 2020

View reviewed changes

vendor/modules.txt Outdated Show resolved Hide resolved

msau42 reviewed Aug 6, 2020

View reviewed changes

pkg/controller/csi_handler.go Outdated Show resolved Hide resolved

yuga711 force-pushed the repair branch from c7cbd33 to 869dc86 Compare August 12, 2020 17:45

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 12, 2020

yuga711 changed the title ~~Fixes a failure in volume attachment reconciler~~ Fixes ReconcileVA for non-migratable drivers Aug 12, 2020

msau42 reviewed Aug 13, 2020

View reviewed changes

jsafrane reviewed Aug 13, 2020

View reviewed changes

yuga711 force-pushed the repair branch from 869dc86 to de12d3e Compare August 17, 2020 16:41

jsafrane reviewed Aug 18, 2020

View reviewed changes

Fixes ReconcileVA for non-migratable drivers

a6b77a5

yuga711 force-pushed the repair branch from de12d3e to a6b77a5 Compare August 18, 2020 17:44

k8s-ci-robot assigned jsafrane Aug 19, 2020

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

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

k8s-ci-robot merged commit be7d057 into kubernetes-csi:master Aug 19, 2020

croomes mentioned this pull request Feb 5, 2021

Use CSI attacher v3 for k8s 1.17+ storageos/cluster-operator#302

Merged

yuga711 mentioned this pull request Feb 17, 2021

REQUEST: New membership for yuga711 kubernetes/org#2507

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes ReconcileVA for non-migratable drivers #244

Fixes ReconcileVA for non-migratable drivers #244

yuga711 commented Aug 6, 2020 •

edited by msau42

Loading

k8s-ci-robot commented Aug 6, 2020

k8s-ci-robot commented Aug 6, 2020

jsafrane commented Aug 6, 2020

jsafrane commented Aug 6, 2020

msau42 left a comment

msau42 commented Aug 6, 2020

yuga711 commented Aug 12, 2020

msau42 left a comment

msau42 Aug 13, 2020

yuga711 Aug 13, 2020 •

edited

Loading

msau42 Aug 13, 2020

msau42 commented Aug 13, 2020

yuga711 commented Aug 13, 2020

jsafrane Aug 13, 2020

yuga711 Aug 17, 2020

jsafrane Aug 18, 2020

yuga711 Aug 18, 2020 •

edited

Loading

yuga711 Aug 18, 2020

jsafrane commented Aug 19, 2020

k8s-ci-robot commented Aug 19, 2020

Fixes ReconcileVA for non-migratable drivers #244

Fixes ReconcileVA for non-migratable drivers #244

Conversation

yuga711 commented Aug 6, 2020 • edited by msau42 Loading

k8s-ci-robot commented Aug 6, 2020

k8s-ci-robot commented Aug 6, 2020

jsafrane commented Aug 6, 2020

jsafrane commented Aug 6, 2020

msau42 left a comment

Choose a reason for hiding this comment

msau42 commented Aug 6, 2020

yuga711 commented Aug 12, 2020

msau42 left a comment

Choose a reason for hiding this comment

msau42 Aug 13, 2020

Choose a reason for hiding this comment

yuga711 Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

msau42 Aug 13, 2020

Choose a reason for hiding this comment

msau42 commented Aug 13, 2020

yuga711 commented Aug 13, 2020

jsafrane Aug 13, 2020

Choose a reason for hiding this comment

yuga711 Aug 17, 2020

Choose a reason for hiding this comment

jsafrane Aug 18, 2020

Choose a reason for hiding this comment

yuga711 Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

yuga711 Aug 18, 2020

Choose a reason for hiding this comment

jsafrane commented Aug 19, 2020

k8s-ci-robot commented Aug 19, 2020

yuga711 commented Aug 6, 2020 •

edited by msau42

Loading

yuga711 Aug 13, 2020 •

edited

Loading

yuga711 Aug 18, 2020 •

edited

Loading