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

FileSystem APIs #11

Merged
merged 2 commits into from Nov 18, 2019
Merged

FileSystem APIs #11

merged 2 commits into from Nov 18, 2019

Conversation

@ddebroy
Copy link
Contributor

ddebroy commented Oct 4, 2019

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:
Initial structure and APIs for supporting host file system calls

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Very much WIP - just the initial structure

Does this PR introduce a user-facing change?:

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 4, 2019

Welcome @ddebroy!

It looks like this is your first PR to kubernetes-csi/csi-proxy 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/csi-proxy has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@ddebroy

This comment has been minimized.

Copy link
Contributor Author

ddebroy commented Oct 4, 2019

cc @wk8

@ddebroy ddebroy added this to In progress in Windows: Alpha Oct 7, 2019

// Windows API's documentation lists hexadecimal error codes; this is the hexadecimal
// representation of the error code. For additional details, please refer
// to https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/705fb797-2175-4a90-b5a3-3918024b10b8

This comment has been minimized.

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Oct 11, 2019

what is the use case of this hexadecimal_code?

uint32 code = 2;

// Windows API's documentation lists hexadecimal error codes; this is the hexadecimal
// representation of the error code. For additional details, please refer

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

Could mention that'll be a string of the form 0x[0-9A-F]{8} ?

service FileSystem {
// PathExists checks if the given path exists on the host.
// PathExists checks if the parameter path exists on the host filesystem.

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

nit: host's filesystem?

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Oct 11, 2019

if the requested path exists...?

rpc PathExists(PathExistsRequest) returns (PathExistsResponse) {}

// Mkdir creates a directory at the requested path on the host filesystem.

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

Same nit (and same for other occurrences below)

)

var (
kubeletCSIPluginsPath = flag.String("kubelet-csi-plugins-path", "C:\\var\\lib\\kubelet\\plugins", "Absolute path of the Kubelet plugin directory in the host file system")

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

Nit: using backticks for strings containing backslashes is more readable IMO.

}

message PathExistsResponse {
bool success = 1;
// The Windows API error code observed (if any)
api.WindowsApiError api_error = 2;

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

The way I imagined it, CmdletError would be used when the RPC call mapped to (and was being run as) a powershell cmdlet, and WindowsApiError when the RPC call mapped to a Windows API procedure (e.g. https://docs.microsoft.com/en-us/windows/win32/api/iscsidsc/nf-iscsidsc-addiscsiconnectionw).

Not sure PathExists maps to either of these though; presumably that would be just calling os.Stat in go? So maybe a simple string error = 1 would be enough here?

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

And in any case this should be field # 1 , no one out there is running the mock API that's currently in master :)


message MkdirResponse {
// The Windows API error code observed (if any)
api.WindowsApiError api_error = 1;

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

Same remark as above, not sure this should be a WindowsApiError; maybe a CmdletError if the implementation shells out to New-Item, or just a string error message if using os.MkdirAll?


message RmdirResponse {
// The Windows API error code observed (if any)
api.WindowsApiError api_error = 1;

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

Same remark.

bool exists = 4;
message LinkPathResponse {
// The Windows API error code observed (if any)
api.WindowsApiError api_error = 1;

This comment has been minimized.

Copy link
@wk8

wk8 Oct 8, 2019

Member

And same :)

@ddebroy ddebroy force-pushed the ddebroy:filesystem1 branch from a1b4db8 to b6bac96 Oct 25, 2019
@ddebroy ddebroy force-pushed the ddebroy:filesystem1 branch from b6bac96 to 4a1dda6 Oct 25, 2019
@ddebroy ddebroy force-pushed the ddebroy:filesystem1 branch 2 times, most recently from 5943810 to 9567ff8 Oct 26, 2019
@ddebroy

This comment has been minimized.

Copy link
Contributor Author

ddebroy commented Oct 26, 2019

Execution of unit tests:

/go/src/github.com/kubernetes-csi/csi-proxy # GO111MODULE=on go test github.com/kubernetes-csi/csi-proxy/internal/server/filesystem
ok  	github.com/kubernetes-csi/csi-proxy/internal/server/filesystem	0.007s
Signed-off-by: Deep Debroy <ddebroy@docker.com>
@ddebroy ddebroy force-pushed the ddebroy:filesystem1 branch from 9567ff8 to 45d3a58 Oct 26, 2019
if err != nil {
return &internal.MkdirResponse{
Error: err.Error(),
}, nil

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 28, 2019

Author Contributor

Returning nil for the overall error is intentional here to distinguish between [1] actual GRPC layer failures or failures in dispatching the API request to the right API handler and [2] errors in executing the API by the handler. We can optionally use https://godoc.org/google.golang.org/grpc/status to directly return rich error data through API handlers (which is the pattern in CSI plugins).

// or as kubelet-pod-path parameters of csi-proxy.
// If a relative path is passed, depending on the context parameter of this
// function, the path will be considered relative to the path specified either as
// kubelet-csi-plugins-path or as kubelet-pod-path parameters of csi-proxy.

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Oct 28, 2019

In what case kubelet-csi-plugins-path will be used? and what case for kubelet-pod-path?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 29, 2019

Author Contributor

I will add that in the comments. For NodeStageVolume calls, kubelet-csi-plugins-path is expected to be used to create C:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\<pv-name>\globalmount. For NodePublishVolume calls,, kubelet-pod-path is expected to be used to create C:\var\lib\kubelet\pods\<pod-uuid>\volumes\kubernetes.io~csi\<pvc-name>\mount.

}

func (APIImplementor) Rmdir(path string) error {
err := os.RemoveAll(path)

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Oct 28, 2019

RemoveAll removes path and any children it contains. But I thought we only remove empty dir which should be os.Remove(path)?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 31, 2019

Author Contributor

I will parameterize this and call the appropriate API.

func (APIImplementor) LinkPath(tgt string, src string) error {
var err error
if runtime.GOOS == "windows" {
_, err = exec.Command("cmd", "/c", "mklink", "/D", tgt, src).CombinedOutput()

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Oct 28, 2019

Will os.Symlink() work here?
I do see an issue opened related to this golang/go#22874
not sure what is the conclusion

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 31, 2019

Author Contributor

os.Symlink() appears to work pretty well! I will switch to it. I was trying to mirror k/k code at https://github.com/kubernetes/kubernetes/blob/7d40536c8184d7b70e4dc3c27d9acc42b5b32d8e/pkg/util/mount/mount_windows.go#L106 but I guess that predates symlink support for Windows in golang. Maybe that can be switched too.

}

func (s *Server) LinkPath(ctx context.Context, request *internal.LinkPathRequest, version apiversion.Version) (*internal.LinkPathResponse, error) {
err := s.validatePathWindows(internal.CONTAINER, request.SourcePath)

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Oct 28, 2019

why here to use internal.CONTAINER directly instead of request.Context?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 29, 2019

Author Contributor

In general we want to constrain the operations within FS contexts as much as possible (as commented in KEP). For Mkdir/Rmdir, we need to allow both contexts as explained above. LinkPath however is only needed for linking the node-global staging paths under PLUGIN context to pod-local staging paths under CONTAINER context. Since no other FS path context is supported (or deemed necessary at the v1alpha1 stage), we do not have a field to specify it in LinkPathRequest at all. I can add a note clarifying this in the comments.

Copy link
Member

wk8 left a comment

A few questions/nits :)

client/api/filesystem/v1alpha1/api.proto Show resolved Hide resolved
if os.IsNotExist(err) {
return false, nil
}
return true, err

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

Why true?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 31, 2019

Author Contributor

good catch. Will return false here.


func (APIImplementor) Mkdir(path string) error {
err := os.MkdirAll(path, 0755)
return err

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

Nit: why not just return os.MkdirAll ?


func (APIImplementor) LinkPath(tgt string, src string) error {
var err error
if runtime.GOOS == "windows" {

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

This is never meant to run anywhere else though, so not sure I see the value of that if?

}

func TestMkdirWindows(t *testing.T) {
v1alpha1, nil := apiversion.NewVersion("v1alpha1")

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

Wasn't aware golang had some pattern matching ;)

This only compiles because you shadow nil, then use that on L114. Please remove?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 31, 2019

Author Contributor

heh - can't believe that compiled. Will remove :-)

}

func NewServer(hostOS string, kubeletCSIPluginsPath string, kubeletPodPath string, hostAPI API) (*Server, error) {
if hostOS != "windows" {

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

I'm sorry, I'm not sure I get the idea behind this; could you please elaborate a mind what you have in mind? It seems it never gets called with anything other than windows, even for tests, which I thought was the idea in the 1st place?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 31, 2019

Author Contributor

I agree the OS check is useless. It's an artifact of something else I was trying and will remove.

}

func isDriveLetterWindows(c uint8) bool {
return ('a' <= c && c <= 'z' || 'A' <= c && c <= 'Z')

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

Nit: parens not needed?

internal/server/filesystem/server.go Show resolved Hide resolved
hostAPI API
}

const MAX_PATH_LENGTH_WINDOWS = 260

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

Nit: this will certainly be used in many other places in this repo, might be worth putting it into an internal/utils.go file or similar?

@@ -10,20 +10,21 @@ import (
"google.golang.org/grpc"

"github.com/kubernetes-csi/csi-proxy/client"
srvtypes "github.com/kubernetes-csi/csi-proxy/internal/server/types"

This comment has been minimized.

Copy link
@wk8

wk8 Oct 30, 2019

Member

Need to amend the auto-gen to account for this, will do as this PR gets merged.

This comment has been minimized.

Copy link
@ddebroy

ddebroy Oct 31, 2019

Author Contributor
return fmt.Errorf("Path length %d exceeds maximum characters: %d", pathlen, MAX_PATH_LENGTH_WINDOWS)
}

if len(path) > 0 && (path[0] == '\\') {

This comment has been minimized.

Copy link
@kkmsft

kkmsft Nov 1, 2019

Contributor

nit: we could use pathlen here instead of len(path)

This comment has been minimized.

Copy link
@ddebroy

ddebroy Nov 1, 2019

Author Contributor

I don't see a pathlen builtin. Can you point me to the golang package that contains pathlen please @kkmsft

This comment has been minimized.

Copy link
@kkmsft

kkmsft Nov 1, 2019

Contributor

Sorry! for the confusion. I was not talking about a package, but the variable being used in the codepath - pathlen - instead of calculating len(path)

func (APIImplementor) LinkPath(tgt string, src string) error {
var err error
if runtime.GOOS == "windows" {
_, err = exec.Command("cmd", "/c", "mklink", "/D", tgt, src).CombinedOutput()

This comment has been minimized.

Copy link
@kkmsft

kkmsft Nov 1, 2019

Contributor

Wouldn't we want to use async mode in these cases -- link is not time consuming so not exactly relevant. But from the code flow from the top of the server, shouldn't we have a context which can be passed on to server (from client) to cancel the operation, which should be then passed here and then we can use exec.Start/exec.Wait and exec.Process.Kill to achieve that pattern ? WDYT ? Perhaps we don't need to incur the complexity of this as of now, but just wanted to get an idea of what the direction would be for context/cancellation in client<->server for csi-proxy in general ?

This comment has been minimized.

Copy link
@ddebroy

ddebroy Nov 1, 2019

Author Contributor

Yes we should definitely use the ctx for more the more complex/time consuming operations like formatting a partition, etc as part of the disk APIs. The ctx is passed through to the server layer (that call into the API implementors) but ignored for the FS ops for now as discussed above

return fmt.Errorf("Path length %d exceeds maximum characters: %d", pathlen, MAX_PATH_LENGTH_WINDOWS)
}

if len(path) > 0 && (path[0] == '\\') {

This comment has been minimized.

Copy link
@kkmsft

kkmsft Nov 1, 2019

Contributor
Suggested change
if len(path) > 0 && (path[0] == '\\') {
if pathlen > 0 && (path[0] == '\\') {
Signed-off-by: Deep Debroy <ddebroy@docker.com>
@ddebroy ddebroy force-pushed the ddebroy:filesystem1 branch from 3f01066 to 6d186b7 Nov 6, 2019
@ddebroy ddebroy changed the title WIP: preliminary FileSystem API structure FileSystem APIs Nov 6, 2019
@ddebroy

This comment has been minimized.

Copy link
Contributor Author

ddebroy commented Nov 6, 2019

@jingxu97 @wk8 @kkmsft this is ready for another look.

Tests:

/go/src/github.com/kubernetes-csi/csi-proxy # GO111MODULE=on GOOS=windows go build -o bin/csi-proxy.exe ./cmd/server
/go/src/github.com/kubernetes-csi/csi-proxy # GO111MODULE=on go test github.com/kubernetes-csi/csi-proxy/internal/server/filesystem
ok  	github.com/kubernetes-csi/csi-proxy/internal/server/filesystem	0.009s

Windows:

PS C:\Users\deep\csi-proxy-1\integrationtests> go test filesystem_test.go
ok      command-line-arguments  0.348s
@wk8
wk8 approved these changes Nov 18, 2019
Copy link
Member

wk8 left a comment

Couple of minor nits, LGTM :) !

/lgtm
/approve

func apiGroups() ([]srvtypes.APIGroup, error) {
fssrv, err := filesystemsrv.NewServer(*kubeletCSIPluginsPath, *kubeletPodPath, filesystemapi.New())
if err != nil {
return []srvtypes.APIGroup{}, err

This comment has been minimized.

Copy link
@wk8

wk8 Nov 18, 2019

Member

Fine for now, but going forward we might want to have a Config struct, and have a standard init func taking a Config and returning a (srvtypes.APIGroup, error) tuple, so we can daisy chain inits.

More of a note for later, good for now :)


type APIImplementor struct{}

func New() APIImplementor {

This comment has been minimized.

Copy link
@wk8

wk8 Nov 18, 2019

Member

Nit: NewImplementor ?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 18, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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 3cd6943 into kubernetes-csi:master Nov 18, 2019
2 of 3 checks passed
2 of 3 checks passed
tide Not mergeable. Retesting: pull-kubernetes-csi-csi-proxy
Details
cla/linuxfoundation ddebroy authorized
Details
pull-kubernetes-csi-csi-proxy Job succeeded.
Details
Windows: Alpha automation moved this from In progress to Done Nov 18, 2019
return false
}

func (s *Server) validatePathWindows(pathCtx internal.PathContext, path string) error {

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Dec 12, 2019

could we move these functions into util so that other apigroup can use it?

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.