-
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
Use CSI driver to determine unique name for migrated in-tree plugins #101737
Conversation
@Jiawei0227: GitHub didn't allow me to request PR reviews from the following users: codablock. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
t.Fatalf("Expect to get plugin \"kubernetes.io/gce-pd\", but got: %s", pluginName) | ||
} | ||
|
||
// suppose CSI migration is on for test plugin |
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.
Should we copy the entire implementation from ad-controller in test? How about something like - gnufied@8be54b4#diff-47fb572faf452565e30c9a91b7e72693d2c6e4bcaecd43aaa9ccb5277b036ef3R385 ?
In this way we just verify attachedVolumes
inside ASOW. The test should check if volume spec belongs to CSI driver and not intree driver.
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.
Done. I verified that the call of processVolumeAttachment will not mark the volume as uncertain with this fix, which indicates the fix works.
pkg/controller/volume/attachdetach/attach_detach_controller_test.go
Outdated
Show resolved
Hide resolved
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, Jiawei0227 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 |
/retest |
/lgtm |
/triage accepted |
/retest |
…101737-upstream-release-1.19 Automated cherry pick of #101737: Use CSI driver to determine unique name for migrated in-tree
…101737-upstream-release-1.20 Automated cherry pick of #101737: Use CSI driver to determine unique name for migrated in-tree
…101737-upstream-release-1.21 Automated cherry pick of #101737: Use CSI driver to determine unique name for migrated in-tree
What type of PR is this?
/kind bug
What this PR does / why we need it:
This is a forked PR from #101423
Credit to @codablock for reporting and sending the fix. This PR tries to add a unit test for it. It turns out that writing an integrated test using
Test_ADC_VolumeAttachmentRecovery
is non-trivial as the test is using a special plugin calledTestPlugin
which does not exist in csi migration. To enable it for CSI migration requires more code refactors. The unit test added in this PR tries to capture the main logic exist in the fix to verify the functionality.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig storage
/assign @gnufied
/cc @msau42 @codablock