Improve NodePublishVolume and NodeStageVolume #463

okartau · 2019-11-13T12:05:55Z

NodePublishVolume performs checks for attributes compatibility if volume already mounted.
If StagingTargetPath is not set, return FAILED_PRECONDITION.

Resolves: #462
Resolves: #465

okartau · 2019-11-14T09:08:26Z

I just realized that if Capability/mount_flags are present in NodePublishVolume request, current code never uses those in mounting.
I think in reality mount options are not set in request, at least we have not seen such yet (?)
The current PR contains log message to trace given mount option, so we will at least see and log what is given in request.
If we start comparing those mount flags, it means we also need to start to use those in mount.
The spec says these are "The mount options that can be used for the volume",
does the "can be used" actually mean "must be used" ?

okartau · 2019-11-19T16:40:21Z

Removed test-repeater part which was present only for testing, and WIP.

pkg/pmem-csi-driver/nodeserver.go

avalluri · 2019-11-20T07:21:49Z

pkg/pmem-csi-driver/nodeserver.go

@@ -42,7 +43,12 @@ type nodeServer struct {
 	nodeCaps []*csi.NodeServiceCapability
 	cs       *nodeControllerServer
 	// Driver deployed to provision only ephemeral volumes(only for Kubernetes v1.15)
-	mounter mount.Interface
+	mounter       mount.Interface
+	volReadOnly   map[string]bool


How about introducing a new structure and keeping all these new params as fields in it. Then we can maintain a single VolumeId:Params map, easy to maintain. If I remember correctly we used to have such type earlier but removed as found no real need that time.

yes, good idea. I started with that, but I realized now the creation and deletion happen in two places, so we can not just conveniently get rid of whole map entry in one place.
Another related question arises: I added map for fsType, which is there now for completeness, but is not actually used. This part is created and used in different function.
Can we perhaps leave fsType out, the we can have one map handling in single place.

creation and deletion happen in two places, so we can not just conveniently get rid of whole map entry in one place.

Yes, If I understood correctly the change, the two possible places depend on type of volume: ephemeral or persistent. So for persistent volumes the point of creation/deletion of map entry in Stage/Unstage and for ephemeral its publish/unpublish. We can even store the volume type in that map so that it's easy to check when to delete the entry from the cache and even we can avoid these workarounds for ephemerals.

avalluri · 2019-11-20T07:24:18Z

pkg/pmem-csi-driver/nodeserver.go

+	mountFlags := req.GetVolumeCapability().GetMount().GetMountFlags()
+	readOnly := req.GetReadonly()
+	fsType := req.GetVolumeCapability().GetMount().GetFsType()
+	klog.V(3).Infof("NodePublishVolume request: VolID:%s targetpath:%v sourcepath:%v readonly:%v Capab/mountflags:%v Capab/fsType:%v",


Can you change Capab/mountflags to mount flags: in the log message. Capab is not informative.

Changed in current code, just pushed

avalluri · 2019-11-20T07:44:09Z

pkg/pmem-csi-driver/nodeserver.go

+	// before storing: sort mountFlags slice and join to become single string, as this is what we use to compare
+	askedFlags := req.GetVolumeCapability().GetMount().GetMountFlags()
+	sort.Strings(askedFlags)
+	joinedAskedFlags := strings.Join(askedFlags[:], ",")


Some code duplication, that we can avoid by reusing the already prepared mount flags array here: fd89169#diff-4a0caa0c2e37db715640b7cbf97d7a1fR212-R214

The code above is inside if-block so it is not guaranteed it has created that value.

what about we move that computation above the "if" so that it happens always once?

what about we move that computation above the "if" so that it happens always once?

Yes I think, as we always store that info anyway.

avalluri · 2019-11-20T07:46:54Z

pkg/pmem-csi-driver/nodeserver.go

@@ -374,6 +413,7 @@ func (ns *nodeServer) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag
 		return nil, err
 	}

+	ns.volFstype[req.VolumeId] = ""


When exactly we clean/remove the cache cahced map(s) elements?

thanks, indeed I should use delete() instead of just overwriting with empty, added delete() calls in latest code.

avalluri · 2019-11-20T07:51:04Z

pkg/pmem-csi-driver/nodeserver.go

-			//   Means, we check only mountflags part of Capability.
-			//   Mount flags are coded as []string, so we sort and join mount flags to single string
-			//   for easier comparing.
+			//    We do not check VolumeCapability/fsType as fsType may be missing (is used in NodeStageVolume).


These remvoed comments are added part of this PR, I feel like you can squash with your earlier commit which introduced these comments. I am ok to completely sqashing this commit too.

thanks, yes I had changed in later commit that I had created in previous one. I now combined the changes of text-changing commit into previous bigger commit.

okartau · 2019-11-20T12:58:32Z

addressed recent commits, force-pushed, also e2e testing that code locally while CI is not yet functioning.

pohly · 2019-11-20T20:38:16Z

We have good CI results for this one. @avalluri, is it okay to merge?

okartau · 2019-11-21T14:57:05Z

I pushed a version where mountflags sorting for storing is made above "if" to have single instance of that code.
This PR needs more work to handle fsType values storing, based on off-line discussion we had with @avalluri, thus restoring WIP in title

okartau · 2019-11-22T08:27:41Z

pushed with added code to compare also fsType, but only if present in request.
But this now happens only in NodePublish. I wonder should we also add fsType check request vs existing in NodeStage.

okartau · 2019-11-22T11:22:20Z

pushed new version where main improvement commit is changed so that it get fsType from device instead of storing in map.

okartau · 2019-11-22T18:45:10Z

pushed a new version again, checking of existing file system is moved out from ProvisionDevice.

avalluri

Thanks, @okartau . Few things in my mind:

current code is not considering the mount flags in stage volume for repeated mounts.
I feel instead of caching mount flags, we should retrieve that information from the existing mount point as we are doing that from fsType. We should change our logic of determining the existing filesystem of a mount path. Currently, we are retrieving that information usingfile and blkid. Instead, if we can retrieve both mount flags and fstype from procfs.

I am ok to merge this now, do the changes as a separate PR.

okartau · 2019-11-25T11:56:12Z

One more thing, let's combine the change here once more with #453 and see the CI result. It used to be combined, then I removed #453 part, but then we changed it some more, so the latest state or PR change is not verified combined with repeated tests.

okartau · 2019-11-25T12:59:09Z

As this CI failure here demonstrates (and also failure on #453 after rebase), recent change to strictly forcing vendor/ state takes away method that I use for development purposes: having local changes in devel/, but only in PR, not to be merged. I combine such changes with other PRs to generate a state with those changes that are not (yet) upstreamed. Is this how we want it, (i.e. we cant put changes through CI with intentional vendor/ changes that we know of but we plan not to check in), or could we have some method to work around that check?
I understand this protects us against unintentional vendor/ changes, but what about intentional.

pohly · 2019-11-25T14:09:52Z

Olev Kartau <notifications@github.com> writes:

I understand this protects us against unintentional vendor/ changes, but what about intentional.

Remove the "make test" step in the Jenkinsfile?

okartau · 2019-11-25T15:24:41Z

Remove the "make test" step in the Jenkinsfile?

Tried that in #453 but it fails in dm-test. Does dm-test depend on something that happens in 'make test' ? It's not that obvious from output what went wrong, or is it for experienced eye...
https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-453/4/pipeline

avalluri · 2019-11-25T15:46:47Z

[2019-11-25T15:05:08.372Z] + make run_dm_tests
[2019-11-25T15:05:08.372Z] curl -L https://github.com/govm-project/govm/releases/download/0.9-alpha/govm_0.9-alpha_Linux_x86_64.tar.gz -o /mnt/workspace/pmem-csi_PR-453/_work/govm_0.9-alpha_Linux_amd64.tar.gz
[2019-11-25T15:05:08.372Z]   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
[2019-11-25T15:05:08.372Z]                                  Dload  Upload   Total   Spent    Left  Speed
[2019-11-25T15:05:08.372Z] 
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100   625    0   625    0     0   2192      0 --:--:-- --:--:-- --:--:--  2192
[2019-11-25T15:05:08.372Z] Warning: Failed to create the file 
[2019-11-25T15:05:08.372Z] Warning: /mnt/workspace/pmem-csi_PR-453/_work/govm_0.9-alpha_Linux_amd64.tar.gz
[2019-11-25T15:05:08.372Z] Warning: : No such file or directory

faile at the govm installation step.

okartau · 2019-11-25T15:51:20Z

you mean failure was independent of skip of 'make test' step ? and it could work when retried?

avalluri · 2019-11-25T16:07:43Z

you mean failure was independent of skip of 'make test' step ? and it could work when retried?

It should but, _work/govm_$(GOVM_VERSION)_Linux_amd64.tar.gz: target is not creating _work dir. This was not visible as make test(maybe setup-ca.sh) crates that.

pohly · 2019-11-25T16:11:40Z

Olev Kartau <notifications@github.com> writes:

Tried that in #453 but it fails in dm-test. Does dm-test depend on something that happens in 'make test' ?

Let's just get rid of it, okay? See #473

okartau · 2019-11-26T11:58:03Z

CI success in combination with repeated tests, removing WIP and repeating-dev-debug commits, force pushed

A log message has gone missing in some previous commit.

Return FailedPrecondition if stagingtargetpath not set

If a volume is already mounted, we must check are the parameteres compatible, and return OK if they are. For having something to compare, we have to store params on Publish. Use sorted and converted to single string mount options in compare. Start with mount options given in request, instead of empty, to take given options into use if present. fsType is not stored in settings, but detected from device.

There may be mount options present in request, so let's start with that set instead of empty.

This check is needed only if called from StageVolume, so lets make it in StageVolume, giving possibility to return API error codes that follow spec better.

pohly · 2019-11-29T07:29:51Z

Is this ready for merging or does it need another round of review by @avalluri?

avalluri · 2019-11-29T08:25:12Z

Is this ready for merging or does it need another round of review by @avalluri?

It can be merged., but needs a follow-up PR to cover the points mentioned here #463 (review)

okartau · 2019-11-29T09:33:10Z

but needs a follow-up PR

I created issue #483, will merge this.

okartau force-pushed the improve-nodepublish branch from ad3d2d6 to 921dd63 Compare November 13, 2019 13:20

okartau force-pushed the improve-nodepublish branch from 921dd63 to c49f571 Compare November 14, 2019 13:30

okartau changed the title ~~WIP: Improve NodePublishVolume return codes~~ WIP: Improve NodePublishVolume and NodeStageVolume + #453 for testing repeated ops Nov 14, 2019

okartau force-pushed the improve-nodepublish branch 7 times, most recently from f345e91 to 7e61623 Compare November 19, 2019 16:38

okartau changed the title ~~WIP: Improve NodePublishVolume and NodeStageVolume + #453 for testing repeated ops~~ Improve NodePublishVolume and NodeStageVolume Nov 19, 2019

okartau requested a review from avalluri November 19, 2019 16:40

okartau force-pushed the improve-nodepublish branch from 7e61623 to aaa4343 Compare November 20, 2019 06:14

avalluri reviewed Nov 20, 2019

View reviewed changes

pkg/pmem-csi-driver/nodeserver.go Show resolved Hide resolved

avalluri suggested changes Nov 20, 2019

View reviewed changes

okartau force-pushed the improve-nodepublish branch from aaa4343 to 452ea5a Compare November 20, 2019 12:51

okartau force-pushed the improve-nodepublish branch from 452ea5a to c7e5a7c Compare November 21, 2019 14:54

okartau changed the title ~~Improve NodePublishVolume and NodeStageVolume~~ WIP: Improve NodePublishVolume and NodeStageVolume Nov 21, 2019

okartau force-pushed the improve-nodepublish branch from c7e5a7c to 44fa0b4 Compare November 22, 2019 08:24

okartau force-pushed the improve-nodepublish branch from 44fa0b4 to e071b3b Compare November 22, 2019 11:08

okartau force-pushed the improve-nodepublish branch from e071b3b to 40337da Compare November 22, 2019 15:51

avalluri approved these changes Nov 25, 2019

View reviewed changes

okartau changed the title ~~WIP: Improve NodePublishVolume and NodeStageVolume~~ Improve NodePublishVolume and NodeStageVolume Nov 25, 2019

okartau changed the title ~~Improve NodePublishVolume and NodeStageVolume~~ WIP: Improve NodePublishVolume and NodeStageVolume Nov 25, 2019

okartau force-pushed the improve-nodepublish branch from b87a6f9 to 3830935 Compare November 25, 2019 20:52

okartau changed the title ~~WIP: Improve NodePublishVolume and NodeStageVolume~~ Improve NodePublishVolume and NodeStageVolume Nov 26, 2019

okartau force-pushed the improve-nodepublish branch from 3830935 to a7819fe Compare November 26, 2019 11:57

okartau force-pushed the improve-nodepublish branch from a7819fe to d445884 Compare November 26, 2019 20:23

okartau added 7 commits November 28, 2019 20:43

NodeStageVolume: restore log msg

aba1cca

A log message has gone missing in some previous commit.

NodeStageVolume: minor correction to fsType use

1282ee0

Correct NodePublishVolume return code

1e5e9a7

Return FailedPrecondition if stagingtargetpath not set

NodeStageVolume: start with mount options in request

94f277c

There may be mount options present in request, so let's start with that set instead of empty.

NodeUnpublishVolume: avoid use of local variable for VolumeID

04cbb0f

nodeserver: Take check for existing fsType out of ProvisionDevice

d13ec74

This check is needed only if called from StageVolume, so lets make it in StageVolume, giving possibility to return API error codes that follow spec better.

okartau force-pushed the improve-nodepublish branch from d445884 to d13ec74 Compare November 28, 2019 18:43

okartau mentioned this pull request Nov 29, 2019

Follow-up improvement of NodeStage handling of mount flags #483

Closed

okartau merged commit 5acdbaf into intel:devel Nov 29, 2019

okartau mentioned this pull request Nov 29, 2019

NodeStageVolume violates CSI spec idempotency req. #386

Closed

okartau deleted the improve-nodepublish branch March 16, 2021 14:51

Improve NodePublishVolume and NodeStageVolume #463

Improve NodePublishVolume and NodeStageVolume #463

Uh oh!

Conversation

okartau commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

okartau commented Nov 14, 2019

Uh oh!

okartau commented Nov 19, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avalluri Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okartau commented Nov 20, 2019

Uh oh!

pohly commented Nov 20, 2019

Uh oh!

okartau commented Nov 21, 2019

Uh oh!

okartau commented Nov 22, 2019

Uh oh!

okartau commented Nov 22, 2019

Uh oh!

okartau commented Nov 22, 2019

Uh oh!

avalluri left a comment

Choose a reason for hiding this comment

Uh oh!

okartau commented Nov 25, 2019

Uh oh!

okartau commented Nov 25, 2019

Uh oh!

pohly commented Nov 25, 2019 via email

Uh oh!

okartau commented Nov 25, 2019

Uh oh!

avalluri commented Nov 25, 2019

Uh oh!

okartau commented Nov 25, 2019

Uh oh!

avalluri commented Nov 25, 2019

Uh oh!

pohly commented Nov 25, 2019 via email

Uh oh!

okartau commented Nov 26, 2019

Uh oh!

pohly commented Nov 29, 2019

Uh oh!

avalluri commented Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

okartau commented Nov 29, 2019

Uh oh!

okartau commented Nov 13, 2019 •

edited

Loading

avalluri Nov 21, 2019 •

edited

Loading

avalluri commented Nov 29, 2019 •

edited

Loading