Separate staging/publish and unstaging/unpublish logics for block #74026

mkimuram · 2019-02-13T17:02:52Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR changes staging/publish and unstaging/unpublish logics for block to be called from separate methods. Also, publish path is changed to per pod path from per volume path.

[Before]

stageVolumeForBlock: called from SetupDevice
publishVolumeForBlock: called from SetupDevice
unstageVolumeForBlock: called from TearDownDevice
unpublishVolumeForBlock: called from TearDownDevice
publish path: plugins/kubernetes.io/csi/volumeDevices/publish/{pvname}

[After]

stageVolumeForBlock: called from SetupDevice
publishVolumeForBlock: called from MapDevice
unstageVolumeForBlock: called from TearDownDevice
unpublishVolumeForBlock: called from UnmapDevice
publish path: plugins/kubernetes.io/csi/volumeDevices/publish/{pvname}/{podUid}

Summary of discussion why this change needed is commented here

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

Special notes for your reviewer:
/sig storage
@bswartz @msau42 @wongma7 @vladimirvivien

Does this PR introduce a user-facing change?:

All nodes need to be drained before upgrading Kubernetes cluster, because paths used for block volumes are changed in this release, so on-line upgrade of nodes aren't allowed.

k8s-ci-robot · 2019-02-13T17:03:00Z

Hi @mkimuram. Thanks for your PR.

I'm waiting for a kubernetes 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.

mkimuram · 2019-02-13T17:09:48Z

pkg/volume/util/operationexecutor/operation_generator.go

-			return volumeToMount.GenerateError("MapVolume.EvalHostSymlinks failed", err)
+		// For in-tree drivers, actual device is attached either in WaitForAttach or SetUpDevice,
+		// not in MapDevice below, so devicePath could be checked here.
+		if !isCSI {


@msau42 @wongma7

Do you have any better ideas to handle this, so that operation_generator code doesn't need to care whether it is CSI?
We might make SetUpDevice for CSI return dummy device paht and make all driver resolve symlink from volumeMapPath below, but I'm not sure if it is better than current one.

for csi, is the devicePath not the staging path returned by SetUpDevice (NodeStage)?

There won't be a device in staging path, for csi drivers that don't implement staging and for csi drivers that put other things than a device in staging path. (CSI v1.0 spec allows these two cases.)
The actual device will be available in publish path after MapDevice is called, by this PR.

wongma7 · 2019-02-13T19:58:42Z

/ok-to-test

wongma7 · 2019-02-13T20:08:15Z

pkg/volume/util/operationexecutor/operation_generator.go

-		// Execute tear down device
-		unmapErr := blockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath)
+		// Execute unmap device
+		unmapErr := blockVolumeUnmapper.UnmapDevice(globalMapPath, deviceToDetach.DevicePath)


should we call UnmapDevice in the per-pod UnmapVolume instead? Then what if we had all the in-tree plugins called og.blkUtil.UnmapDevice in their plugin.UnmapDevice, and CSI calls Unpublish in its plugin.UnmapDevice

Thank you for pointing it out.
This will be a big change, so I wanted to make an agreement before go ahead.

I will try implementing this way and update the commit.

wongma7 · 2019-02-13T20:09:34Z

pkg/volume/csi/csi_block.go

 	// Call NodePublishVolume
+	stagingPath := m.getStagingPath()


should this be getPublishPath? currently getPublishPath is unused

Staging path is used here for publish call, it is due to CSI spec.

It's confusing, so I agree that we should change publishVolumeForBlock not to take stagingPath as an argument, and get it inside the function, as it is done for publishPath.

wongma7 · 2019-02-13T20:16:50Z

pkg/volume/util/operationexecutor/operation_generator.go

-			return volumeToMount.GenerateError("MapVolume.EvalHostSymlinks failed", err)
+		// For in-tree drivers, actual device is attached either in WaitForAttach or SetUpDevice,
+		// not in MapDevice below, so devicePath could be checked here.
+		if !isCSI {


for csi, is the devicePath not the staging path returned by SetUpDevice (NodeStage)?

mkimuram

@wongma7

Thank you for your comment.
I will update the commit as suggested.

mkimuram · 2019-02-13T20:34:35Z

pkg/volume/util/operationexecutor/operation_generator.go

-			return volumeToMount.GenerateError("MapVolume.EvalHostSymlinks failed", err)
+		// For in-tree drivers, actual device is attached either in WaitForAttach or SetUpDevice,
+		// not in MapDevice below, so devicePath could be checked here.
+		if !isCSI {


There won't be a device in staging path, for csi drivers that don't implement staging and for csi drivers that put other things than a device in staging path. (CSI v1.0 spec allows these two cases.)
The actual device will be available in publish path after MapDevice is called, by this PR.

mkimuram · 2019-02-13T21:01:30Z

pkg/volume/util/operationexecutor/operation_generator.go

-		// Execute tear down device
-		unmapErr := blockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath)
+		// Execute unmap device
+		unmapErr := blockVolumeUnmapper.UnmapDevice(globalMapPath, deviceToDetach.DevicePath)


Thank you for pointing it out.
This will be a big change, so I wanted to make an agreement before go ahead.

I will try implementing this way and update the commit.

mkimuram · 2019-02-13T21:02:27Z

pkg/volume/csi/csi_block.go

 	// Call NodePublishVolume
+	stagingPath := m.getStagingPath()


Staging path is used here for publish call, it is due to CSI spec.

It's confusing, so I agree that we should change publishVolumeForBlock not to take stagingPath as an argument, and get it inside the function, as it is done for publishPath.

mkimuram · 2019-02-14T00:57:51Z

@wongma7

Updated as suggested. PTAL

Note that ref-counting and releasing descriptor lock codes are also moved to CSI's UnmapDevice, almost as they are. We might need to consider more to properly handle all cases including multiple pod mapping case.
(To get feed back early, I'm pushing this commit, after I only confirmed that this worked well with one pod mapping case.)

fejta-bot · 2019-06-10T18:10:27Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-07-10T18:59:07Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

mkimuram · 2019-11-01T02:34:45Z

@davidz627

I've created #84625 to separate the commits and explain the intention of the code to each commit comment. This is still work in progress, because no logic change from this PR's version, so I will continue working on to change the code per your comments.

I hope that above PR at least answer to your two comments below:

Overall comment is that this probably could benefit from being seperated into some smaller PRs.

why am I seeing so much new functionality all over the place? It's not clear to mewhat this stuff is doing

I would appreciate it if you could suggest me how we separate it to small PRs. (Should it be per commit in #84625?)

davidz627 · 2019-11-01T18:11:01Z

I don't really understand 100% what's happening enough to have suggestions on how to split this up into smaller PRs. It may be that the answer is it can't and some commit separation is helpful enough for review :) I'm just concerned since theres a lot of code moving around and it's not clear which parts are refactoring and which are net-new. The PR title says logic is being "seperated" but I see some completely new functions with significant logic. It might be worth seperating out the refactor into one PR where things just move around, and another PR for each of the issues that need to be fixed

mkimuram · 2019-11-01T19:27:01Z

@davidz627

Thank you for your comment.

I think that the last three commits in #84625 is actually separating the logic as described in the PR tittle, and other commits are prerequisite for it. So, let's split them into two, and split them more if needed.

Edit:
Currently, separated into below three PRs:

They should be merged with above order.

mkimuram · 2019-11-14T22:25:41Z

Rebased again on top of #84747 after #84660 is merged. Waiting for #84747 to be merged.

@saad-ali

Can this also be targeted to 1.17? (Although, @jsafrane 's time zone is already midnight.)

The last discussion for this PR is #84625 (comment) . I'm pushing the latest version here, so that we can compare this PR and #84625 . I believe that this PR should be ready to be merged, if others agree.)

Any comments? @davidz627 @msau42 @jingxu97

mkimuram · 2019-11-15T01:52:47Z

Now #84747 is merged. I will rebase this PR with the latest master.

This change is to allow CSI driver to publish the same volume for multipe pods on the same node.

mkimuram · 2019-11-15T02:01:54Z

@saad-ali

Rebased. Now it should be ready to be merged. PTAL

saad-ali

/lgtm
/approve
/milestone v1.17

saad-ali · 2019-11-15T02:28:47Z

/priority important-soon

k8s-ci-robot · 2019-11-15T02:28:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mkimuram, saad-ali

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:

~~pkg/volume/csi/OWNERS~~ [saad-ali]

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 requested review from abrarshivani and andyzhangx February 13, 2019 17:03

mkimuram commented Feb 13, 2019

View reviewed changes

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 Feb 13, 2019

wongma7 reviewed Feb 13, 2019

View reviewed changes

mkimuram commented Feb 13, 2019

View reviewed changes

mkimuram force-pushed the issue/73773 branch from df3a8da to 2ceee60 Compare February 14, 2019 00:59

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2019

mkimuram force-pushed the issue/73773 branch from 437d1cc to 5d7e7c5 Compare February 19, 2019 20:57

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 19, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2019

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 10, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 10, 2019

bswartz mentioned this pull request Jul 11, 2019

CSI Block storage support kubernetes/enhancements#565

Closed

This was referenced Nov 4, 2019

Refactor mapper/unmapper of block volume #84747

Merged

Refactor block volume's descriptor lock logic #84660

Merged

mkimuram force-pushed the issue/73773 branch 3 times, most recently from e1ce3fd to 136ce69 Compare November 14, 2019 22:13

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 14, 2019

mkimuram added 2 commits November 15, 2019 01:55

Change publish path for CSI block volume per pod

7caf731

This change is to allow CSI driver to publish the same volume for multipe pods on the same node.

Separate staging/publish and unstaging/unpublish logics for block

4578c6c

mkimuram force-pushed the issue/73773 branch from 136ce69 to 4578c6c Compare November 15, 2019 02:00

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 15, 2019

saad-ali approved these changes Nov 15, 2019

View reviewed changes

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

k8s-ci-robot added this to the v1.17 milestone Nov 15, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 15, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2019

k8s-ci-robot merged commit cb2684c into kubernetes:master Nov 15, 2019

ymmt2005 mentioned this pull request Jan 7, 2020

Kubernetes 1.17 cybozu-go/neco#708

Closed

5 tasks

dulltz mentioned this pull request Mar 2, 2020

PoC: try to upgrade kubelet without draining workloads having raw block volume cybozu-go/neco#830

Closed

4 tasks

mkimuram mentioned this pull request May 28, 2021

Fix for the block device bind mount caused block device not work normally after node restart #102161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate staging/publish and unstaging/unpublish logics for block #74026

Separate staging/publish and unstaging/unpublish logics for block #74026

mkimuram commented Feb 13, 2019 •

edited

Loading

k8s-ci-robot commented Feb 13, 2019

mkimuram Feb 13, 2019

wongma7 Feb 13, 2019

mkimuram Feb 13, 2019

wongma7 commented Feb 13, 2019

wongma7 Feb 13, 2019

mkimuram Feb 13, 2019

wongma7 Feb 13, 2019

mkimuram Feb 13, 2019

wongma7 Feb 13, 2019

mkimuram left a comment

mkimuram Feb 13, 2019

mkimuram Feb 13, 2019

mkimuram Feb 13, 2019

mkimuram commented Feb 14, 2019

fejta-bot commented Jun 10, 2019

fejta-bot commented Jul 10, 2019

mkimuram commented Nov 1, 2019

davidz627 commented Nov 1, 2019

mkimuram commented Nov 1, 2019 •

edited

Loading

mkimuram commented Nov 14, 2019

mkimuram commented Nov 15, 2019

mkimuram commented Nov 15, 2019

saad-ali left a comment

saad-ali commented Nov 15, 2019

k8s-ci-robot commented Nov 15, 2019

Separate staging/publish and unstaging/unpublish logics for block #74026

Separate staging/publish and unstaging/unpublish logics for block #74026

Conversation

mkimuram commented Feb 13, 2019 • edited Loading

k8s-ci-robot commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wongma7 commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram commented Feb 14, 2019

fejta-bot commented Jun 10, 2019

fejta-bot commented Jul 10, 2019

mkimuram commented Nov 1, 2019

davidz627 commented Nov 1, 2019

mkimuram commented Nov 1, 2019 • edited Loading

mkimuram commented Nov 14, 2019

mkimuram commented Nov 15, 2019

mkimuram commented Nov 15, 2019

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali commented Nov 15, 2019

k8s-ci-robot commented Nov 15, 2019

mkimuram commented Feb 13, 2019 •

edited

Loading

mkimuram commented Nov 1, 2019 •

edited

Loading