Add RmdirContents API in the Fileystem v2alpha1 API Group #186

mauriciopoppe · 2022-01-06T23:28:52Z

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 feature

What this PR does / why we need it:
Adds the new Filesystem API, RmdirContents, it removes the contents of a directory (but not the directory itself).

Does this PR introduce a user-facing change?:

Add Filesystem API RmdirContents to remove the contents of a directory.

/cc @jingxu97 @ddebroy

ddebroy

Recommend starting a v2beta1 API group in filesystem for the new changes.

client/api/filesystem/v1/api.proto

mauriciopoppe · 2022-01-07T22:02:42Z

/hold

Needs #187 to be merged first.

@ddebroy please take a look again, I generated the files on top of a new api version v2alpha1.

ddebroy · 2022-01-08T01:00:18Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2022-01-08T01:00:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

client/api/filesystem/v2alpha1/api.proto

jingxu97 · 2022-01-11T01:30:26Z

pkg/os/filesystem/api.go

+	if err != nil {
+		return err
+	}
+	for _, file := range files {


you didn't implement force option here?

thanks, fixed

mauriciopoppe · 2022-01-11T18:18:10Z

/hold

#187 was merged and I'll try with the prow integration test job too.

mauriciopoppe · 2022-01-26T23:42:11Z

/test pull-kubernetes-csi-csi-proxy-integration

mauriciopoppe · 2022-01-27T19:09:19Z

/remove-hold
@jingxu97 this is ready for review, please take a look :)

jingxu97 · 2022-01-27T20:20:52Z

client/api/filesystem/v2alpha1/api.proto

+    string path = 1;
+
+    // Force remove all contents under path (if any).
+    bool force = 2;


why do we want to have this option? If force is false, it can only remove files, or empty dirs?
Does RmdirContents mean we want to remove everything under it, not just empty dirs?

good observation, from LVP it's always called with force = true and if it's false then it's like you say for, this is because I copied the message from the existing Rmdir message

I removed the field

mauriciopoppe · 2022-01-27T22:56:44Z

/test pull-kubernetes-csi-csi-proxy-integration

mauriciopoppe · 2022-01-27T22:58:30Z

/test pull-kubernetes-csi-csi-proxy-integration

jingxu97 · 2022-01-28T00:00:42Z

client/api/filesystem/v2alpha1/api.proto

@@ -15,6 +15,9 @@ service Filesystem {
    // This may be used for unlinking a symlink created through CreateSymlink.
    rpc Rmdir(RmdirRequest) returns (RmdirResponse) {}

+    // RmdirContents removes the contents of a directory in the host filesystem.


maybe add a few more words to mention contents are all the files and sub dirs under the dir. The directory itself will not be deleted.

added:

// Unlike Rmdir it won't delete the requested path, it'll only delete its contents.

jingxu97 · 2022-01-28T00:16:07Z

pkg/os/filesystem/api.go

@@ -78,6 +80,29 @@ func (filesystemAPI) Rmdir(path string, force bool) error {
 	return os.Remove(path)
 }

+// RmdirContents removes the contents of a directory with `os.RemoveAll`
+func (filesystemAPI) RmdirContents(path string) error {
+	dir, err := os.Open(path)


https://stackoverflow.com/a/52685448

a reference of implementation

I took the same implementation as LVP for linux

mauriciopoppe · 2022-01-28T00:18:00Z

/test pull-kubernetes-csi-csi-proxy-integration

jingxu97 · 2022-01-28T00:22:12Z

/lgtm

k8s-ci-robot requested review from ddebroy and jingxu97 January 6, 2022 23:28

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch 3 times, most recently from 50caaff to f1d1bf8 Compare January 6, 2022 23:57

ddebroy reviewed Jan 7, 2022

View reviewed changes

client/api/filesystem/v1/api.proto Outdated Show resolved Hide resolved

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from f1d1bf8 to 3af8237 Compare January 7, 2022 21:02

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 Jan 7, 2022

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch 3 times, most recently from 3544eec to 1842fce Compare January 7, 2022 21:15

mauriciopoppe changed the title ~~Add RmdirContents API in the Fileystem API Group~~ Add RmdirContents API in the Fileystem v2alpha1 API Group Jan 7, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2022

k8s-ci-robot assigned ddebroy Jan 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2022

ddebroy reviewed Jan 8, 2022

View reviewed changes

client/api/filesystem/v2alpha1/api.proto Outdated Show resolved Hide resolved

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from 1842fce to 78eb1b3 Compare January 8, 2022 05:27

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 8, 2022

jingxu97 reviewed Jan 11, 2022

View reviewed changes

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from 3571adc to fd5fdae Compare January 11, 2022 18:16

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2022

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from c4e5b93 to 8c10065 Compare January 11, 2022 19:00

mauriciopoppe mentioned this pull request Jan 18, 2022

Scripts to run the integration tests with a real kubernetes VM #190

Merged

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from 8c10065 to 84ffc81 Compare January 26, 2022 23:41

mauriciopoppe mentioned this pull request Jan 26, 2022

[WIP] Add the Lsdir API to the Filesystem v1 API Group #183

Closed

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch 3 times, most recently from 38089c9 to a9faf8e Compare January 27, 2022 01:20

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2022

mauriciopoppe mentioned this pull request Jan 27, 2022

skip resizing volume when extent is less than the minimum of 1MB #196

Closed

jingxu97 reviewed Jan 27, 2022

View reviewed changes

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from a9faf8e to f16a115 Compare January 27, 2022 22:55

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from f16a115 to 897656b Compare January 27, 2022 22:57

jingxu97 reviewed Jan 28, 2022

View reviewed changes

Generate code with protobuf and csi-proxy-api-gen for RmdirContents

811515c

jingxu97 reviewed Jan 28, 2022

View reviewed changes

RmdirContents server implementation

b5442d7

mauriciopoppe force-pushed the rmdircontents-filesystem-api branch from 897656b to b5442d7 Compare January 28, 2022 00:16

k8s-ci-robot assigned jingxu97 Jan 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2022

k8s-ci-robot merged commit 50d3996 into kubernetes-csi:master Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RmdirContents API in the Fileystem v2alpha1 API Group #186

Add RmdirContents API in the Fileystem v2alpha1 API Group #186

mauriciopoppe commented Jan 6, 2022

ddebroy left a comment

mauriciopoppe commented Jan 7, 2022

ddebroy commented Jan 8, 2022

k8s-ci-robot commented Jan 8, 2022

jingxu97 Jan 11, 2022

mauriciopoppe Jan 11, 2022

mauriciopoppe commented Jan 11, 2022

mauriciopoppe commented Jan 26, 2022

mauriciopoppe commented Jan 27, 2022

jingxu97 Jan 27, 2022

mauriciopoppe Jan 27, 2022

mauriciopoppe commented Jan 27, 2022

mauriciopoppe commented Jan 27, 2022

jingxu97 Jan 28, 2022

mauriciopoppe Jan 28, 2022

jingxu97 Jan 28, 2022

mauriciopoppe Jan 28, 2022

mauriciopoppe commented Jan 28, 2022

jingxu97 commented Jan 28, 2022

Add RmdirContents API in the Fileystem v2alpha1 API Group #186

Add RmdirContents API in the Fileystem v2alpha1 API Group #186

Conversation

mauriciopoppe commented Jan 6, 2022

ddebroy left a comment

Choose a reason for hiding this comment

mauriciopoppe commented Jan 7, 2022

ddebroy commented Jan 8, 2022

k8s-ci-robot commented Jan 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciopoppe commented Jan 11, 2022

mauriciopoppe commented Jan 26, 2022

mauriciopoppe commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciopoppe commented Jan 27, 2022

mauriciopoppe commented Jan 27, 2022

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

mauriciopoppe commented Jan 28, 2022

jingxu97 commented Jan 28, 2022