Skip to content

Bugfix: csi plugin supporting raw block that does not need attach mounted failed#79920

Merged
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
cwdsuzhou:July/block_not_support_attach
Jul 15, 2019
Merged

Bugfix: csi plugin supporting raw block that does not need attach mounted failed#79920
k8s-ci-robot merged 1 commit into
kubernetes:masterfrom
cwdsuzhou:July/block_not_support_attach

Conversation

@cwdsuzhou

@cwdsuzhou cwdsuzhou commented Jul 9, 2019

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

csi plugin supporting raw block that does not need attach mounted failed.
reason: for block device, csi_block.go always gets an attachment no matter if it supports attachment.

Which issue(s) this PR fixes:

Fixes #79884

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Bugfix: csi plugin supporting raw block that does not need attach mounted failed

@k8s-ci-robot 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 9, 2019
@k8s-ci-robot k8s-ci-robot requested review from davidz627 and rootfs July 9, 2019 05:08
@cwdsuzhou

cwdsuzhou commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

/assign @saad-ali @jsafrane

@cwdsuzhou cwdsuzhou changed the title Bugfix: csi raw block that does not need attach mounted failed Bugfix: csi plugin supporting raw block that does not need attach mounted failed Jul 9, 2019
@jsafrane

jsafrane commented Jul 9, 2019

Copy link
Copy Markdown
Member

make verify is not happy about your change:

Invalid invocations of featuregatetesting.SetFeatureGateDuringTest():
./pkg/volume/csi/csi_block_test.go:360:	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIDriverRegistry, true)

Always make a deferred call to the returned function to ensure the feature gate is reset:   
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.<FeatureName>, <value>)()
--

Can you also please enhance our e2e test for VolumeAttachment with block volumes? We have some for filesystem based PVs: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/csi_mock_volume.go#L233

cc @vladimirvivien @msau42

@cwdsuzhou

cwdsuzhou commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

make verify is not happy about your change:

Invalid invocations of featuregatetesting.SetFeatureGateDuringTest():
./pkg/volume/csi/csi_block_test.go:360:	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIDriverRegistry, true)

Always make a deferred call to the returned function to ensure the feature gate is reset:   
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.<FeatureName>, <value>)()
--

Can you also please enhance our e2e test for VolumeAttachment with block volumes? We have some for filesystem based PVs: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/csi_mock_volume.go#L233

cc @vladimirvivien @msau42

I would fix it, thanks

@cwdsuzhou

cwdsuzhou commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

Can you also please enhance our e2e test for VolumeAttachment with block volumes? We have some for filesystem based PVs: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/csi_mock_volume.go#L233

I would like to enhance e2e test for VolumeAttachment with block volumes in another PR.
@jsafrane wdyt?

@cwdsuzhou cwdsuzhou force-pushed the July/block_not_support_attach branch 2 times, most recently from 1d0e7c3 to 35c6af2 Compare July 9, 2019 11:51
jsafrane
jsafrane reviewed Jul 12, 2019
View reviewed changes
Comment thread pkg/volume/csi/csi_block.go Outdated

@jsafrane jsafrane Jul 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, return err would be more useful here. Kubelet should not assume that the volume is attachable.

@cwdsuzhou cwdsuzhou Jul 15, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done

Bugfix: csi raw block that does not need attach mounted failed
0c628e1 
Add unit test

fix verify-test-featurefates failed
@cwdsuzhou cwdsuzhou force-pushed the July/block_not_support_attach branch from 35c6af2 to 0c628e1 Compare July 15, 2019 02:34
@jsafrane

jsafrane commented Jul 15, 2019

Copy link
Copy Markdown
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2019
@k8s-ci-robot

k8s-ci-robot commented Jul 15, 2019

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 10a1d1f into kubernetes:master Jul 15, 2019
This was referenced Jul 16, 2019
Merged
Merged
Closed
@cwdsuzhou

cwdsuzhou commented Jul 16, 2019

Copy link
Copy Markdown
Contributor Author

@jsafrane I have cherry picked the PR to k8s 1.15 1.14 and 1.13, PTAL, thanks

@neolit123

neolit123 commented Jul 16, 2019

Copy link
Copy Markdown
Member

@cwdsuzhou

I have cherry picked the PR to k8s 1.15 1.14 and 1.13, PTAL, thanks

the criteria for cherry picks in k8s should be "critical bug fixes only".
does this bugfix qualify?

@cwdsuzhou

cwdsuzhou commented Jul 17, 2019

Copy link
Copy Markdown
Contributor Author

@neolit123 I think it qualifies.Without this PR CSI plugins support raw block but do not need attachment can not mount block devices successfully. However I we have claimed we support CSI raw block mode.

k8s-ci-robot added a commit that referenced this pull request Aug 7, 2019
@k8s-ci-robot
Merge pull request #80196 from cwdsuzhou/automated-cherry-pick-of-#79…
9821e16 
…920-upstream-release-1.14

Automated cherry pick of #79920: Bugfix: csi raw block that does not need attach mounted failed
k8s-ci-robot added a commit that referenced this pull request Aug 7, 2019
@k8s-ci-robot
Merge pull request #80195 from cwdsuzhou/automated-cherry-pick-of-#79…
6eb69c3 
…920-upstream-release-1.15

Automated cherry pick of #79920: Bugfix: csi raw block that does not need attach mounted failed
@cwdsuzhou cwdsuzhou deleted the July/block_not_support_attach branch September 30, 2019 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsafrane jsafrane jsafrane left review comments

@davidz627 davidz627 Awaiting requested review from davidz627

@rootfs rootfs Awaiting requested review from rootfs

Assignees

@jsafrane jsafrane

@saad-ali saad-ali

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

csi plugin supporting raw block failed

5 participants

@cwdsuzhou @jsafrane @k8s-ci-robot @neolit123 @saad-ali