Add WriteVolumeCache API #84

jingxu97 · 2020-10-03T23:25:43Z

add writevolumecache api to csi-proxy

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add WriteVolumeCache API

andyzhangx

so this API is for specific file to flush memory to disk, right?

/lgtm

andyzhangx · 2020-10-04T01:07:43Z

internal/os/volume/api.go

@@ -47,6 +47,17 @@ func (VolAPIImplementor) FormatVolume(volumeID string) (err error) {
 	return nil
 }

+// WriteVolumeCache - Writes the file system cache to disk with given file path


nit: Writes the file system cache to disk where given file path is located

msau42 · 2020-10-05T18:46:02Z

client/api/volume/v1beta1/api.proto

@@ -24,6 +24,8 @@ service Volume {
    rpc GetVolumeDiskNumber(VolumeDiskNumberRequest) returns (VolumeDiskNumberResponse) {}
    // GetVolumeIDFromMount gets the volume id for a given mount
    rpc GetVolumeIDFromMount(VolumeIDFromMountRequest) returns (VolumeIDFromMountResponse) {}
+    // WriteVolumeCache write volume cache to disk
+    rpc WriteVolumeCache(WriteVolumeCacheRequest) returns (WriteVolumeCacheResponse) {}


I think we need a v1beta2 if we're going to modify the API since csi-proxy versions are not tied to k8s versions

jingxu97 · 2020-10-09T02:38:23Z

so this API is for specific file to flush memory to disk, right?

/lgtm

actually it mainly for volume to flush cache data before calling unstagevolume. I am considering to use volumeId too. For certain plugins like gce pd, we don't keep volume unique id, instead, it uses mount path. Get-volume could accept either volumeId or file path. But all other volume API currently use uniqueId, so maybe it makes more sense to use volumeId instead of path.
kubernetes-sigs/azuredisk-csi-driver#381 (comment)
@ddebroy

ddebroy

One comment to fix a comment and switch to volume_id

ddebroy · 2020-10-09T19:59:19Z

client/api/volume/v1beta2/api.proto

+}
+
+message WriteVolumeCacheRequest {
+    // Volume device ID of the volume to format


nit: please fix the comment above referring to format. Also please use volume_id as you mentioned and we discussed.

updated. PTAL

ddebroy

One more nit around volumeID. Otherwise lgtm.

ddebroy · 2020-10-09T21:25:14Z

internal/server/volume/server.go

+	klog.V(4).Infof("calling WriteVolumeCache with request: %+v", request)
+	response := &internal.WriteVolumeCacheResponse{}
+
+	filePath := request.FilePath


This needs to be. Since the type is string, the compiler did not catch it.

volumeID := request.VolumeId if volumeID == "" { klog.Errorf("volume id empty") return response, fmt.Errorf("volume id empty") } err := s.hostAPI. WriteVolumeCache(volumeID)

thanks for catching it. updated. PTAL

add writevolumecache api to csi-proxy v1beta2 volume API group

ddebroy

/lgtm
/approve

k8s-ci-robot · 2020-10-10T00:25:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, jingxu97

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:

~~OWNERS~~ [ddebroy,jingxu97]

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 added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 3, 2020

k8s-ci-robot requested review from andyzhangx and ddebroy October 3, 2020 23:25

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2020

jingxu97 force-pushed the oct/cache branch 2 times, most recently from dbfb884 to ac1c54b Compare October 3, 2020 23:33

andyzhangx approved these changes Oct 4, 2020

View reviewed changes

k8s-ci-robot assigned andyzhangx Oct 4, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2020

msau42 reviewed Oct 5, 2020

View reviewed changes

jingxu97 force-pushed the oct/cache branch from ac1c54b to 506d1fd Compare October 9, 2020 02:33

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 9, 2020

jingxu97 force-pushed the oct/cache branch from 506d1fd to 376a0cc Compare October 9, 2020 02:40

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

jingxu97 force-pushed the oct/cache branch 5 times, most recently from 934cf70 to b99c4b9 Compare October 9, 2020 03:20

ddebroy reviewed Oct 9, 2020

View reviewed changes

jingxu97 force-pushed the oct/cache branch from b99c4b9 to 966aa6d Compare October 9, 2020 20:15

ddebroy reviewed Oct 9, 2020

View reviewed changes

jingxu97 force-pushed the oct/cache branch 6 times, most recently from 4d30344 to 34e4770 Compare October 9, 2020 22:40

Add WriteVolumeCache API to v1beta2 Volume API group

500dc47

add writevolumecache api to csi-proxy v1beta2 volume API group

jingxu97 force-pushed the oct/cache branch from 34e4770 to 500dc47 Compare October 9, 2020 22:55

ddebroy reviewed Oct 10, 2020

View reviewed changes

k8s-ci-robot assigned ddebroy Oct 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2020

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 12, 2020

k8s-ci-robot merged commit b3cbf0c into kubernetes-csi:master Oct 12, 2020

jingxu97 mentioned this pull request Oct 14, 2020

Revert "Add WriteVolumeCache API" #93

Closed

andyzhangx mentioned this pull request Dec 15, 2020

Snapshot restore PVC cannot mount kubernetes-sigs/azuredisk-csi-driver#597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WriteVolumeCache API #84

Add WriteVolumeCache API #84

jingxu97 commented Oct 3, 2020 •

edited by ddebroy

Loading

andyzhangx left a comment

andyzhangx Oct 4, 2020

msau42 Oct 5, 2020

jingxu97 Oct 9, 2020

jingxu97 commented Oct 9, 2020 •

edited

Loading

ddebroy left a comment

ddebroy Oct 9, 2020

jingxu97 Oct 9, 2020

ddebroy left a comment

ddebroy Oct 9, 2020

jingxu97 Oct 9, 2020

ddebroy left a comment

k8s-ci-robot commented Oct 10, 2020

Add WriteVolumeCache API #84

Add WriteVolumeCache API #84

Conversation

jingxu97 commented Oct 3, 2020 • edited by ddebroy Loading

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx Oct 4, 2020

Choose a reason for hiding this comment

msau42 Oct 5, 2020

Choose a reason for hiding this comment

jingxu97 Oct 9, 2020

Choose a reason for hiding this comment

jingxu97 commented Oct 9, 2020 • edited Loading

ddebroy left a comment

Choose a reason for hiding this comment

ddebroy Oct 9, 2020

Choose a reason for hiding this comment

jingxu97 Oct 9, 2020

Choose a reason for hiding this comment

ddebroy left a comment

Choose a reason for hiding this comment

ddebroy Oct 9, 2020

Choose a reason for hiding this comment

jingxu97 Oct 9, 2020

Choose a reason for hiding this comment

ddebroy left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 10, 2020

jingxu97 commented Oct 3, 2020 •

edited by ddebroy

Loading

jingxu97 commented Oct 9, 2020 •

edited

Loading