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

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

Merged

Conversation

@cwdsuzhou
Copy link
Member

commented Jul 9, 2019

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
@cwdsuzhou

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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 cwdsuzhou:July/block_not_support_attach branch 2 times, most recently from 1d0e7c3 to 35c6af2 Jul 9, 2019
klog.Error(log("blockMapper.SetupDevice failed to get volume attachment [id=%v]: %v", attachID, err))
return "", err
skip = false
klog.Error(log("Failed to check CSIDriver for %s: %s", driverName, err))

This comment has been minimized.

Copy link
@jsafrane

jsafrane Jul 12, 2019

Member

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

This comment has been minimized.

Copy link
@cwdsuzhou

cwdsuzhou Jul 15, 2019

Author Member

thanks, done

Add unit test

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

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 15, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

[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

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 merged commit 10a1d1f into kubernetes:master Jul 15, 2019
23 checks passed
23 checks passed
cla/linuxfoundation cwdsuzhou authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@cwdsuzhou

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

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

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

@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
…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
…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 cwdsuzhou:July/block_not_support_attach branch Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.