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

Add another client per API that can communicate with a custom named pipe #124

Merged
merged 7 commits into from
Apr 8, 2021

Conversation

mauriciopoppe
Copy link
Member

@mauriciopoppe mauriciopoppe commented Apr 6, 2021

What type of PR is this?

/kind feature

What this PR does / why we need it:

There are two scenarios that we might see while deploying the CSI proxy:

  • PD CSI driver imports the v1 CSI Proxy client, the node has the beta CSI Proxy server
  • PD CSI driver imports the v1 CSI Proxy client, the node has the v1 CSI Proxy server

Instead of handling multiple imports in PD CSI driver we can just use the v1 client with additional startup code to identify which of these cases should attempt to solve.

This PR adds another way to initialize a client by sending a named pipe, the idea is that the user of the client can import the v1 client and still be able to call the v1beta named pipes for compatibility

The client is:

func NewClientWithPipePath(pipePath string) (*Client, error)

The consumers of the client should send the pipePath they want to use, for example GCE PD CSI driver is using the following:

import (
	csiproxyclient "github.com/kubernetes-csi/csi-proxy/client"
	"github.com/kubernetes-csi/csi-proxy/client/apiversion"

	diskapi "github.com/kubernetes-csi/csi-proxy/client/api/disk/v1"
	diskclient "github.com/kubernetes-csi/csi-proxy/client/groups/disk/v1"
)

func getDiskClient() (*diskclient.Client, error) {
	var err error
	versionv1beta2 := apiversion.NewVersionOrPanic("v1beta2")
	diskAPIVersions := []apiversion.Version{diskClient.Version /* v1 */, versionv1beta2}

	// attempt to connect to one of the clients in order
	for _, apiVersion := range diskAPIVersions {
		pipePath := csiproxyclient.PipePath(diskclient.GroupName, apiVersion)
		klog.V(4).Infof("Attempting to connect to csi-proxy diskapi at path=%s", pipePath)
		diskClient, err := diskclient.NewClientWithPipePath(pipePath)
		if err == nil {
			klog.V(4).Infof("Connected to csi-proxy diskapi at path=%s", pipePath)
			return diskClient, nil
		}
		klog.V(4).Infof("Connection to csi-proxy diskapi at path=%s failed with error %v, might retry with a different version", pipePath, err)
	}
	return nil, fmt.Errorf("Couldn't connect to any csi-proxy diskapi server, last error %v", err)
}

func NewCSIProxyMounter() (*CSIProxyMounter, error) {
	diskClient, err := getDiskClient()
	if err != nil {
		return nil, err
	}

	fsClient, err := fsclient.NewClient()
	if err != nil {
		return nil, err
	}
	volumeClient, err := volumeclient.NewClient()
	if err != nil {
		return nil, err
	}
	return &CSIProxyMounter{
		FsClient:     fsClient,
		DiskClient:   diskClient,
		VolumeClient: volumeClient,
	}, nil
}

In my cluster I have CSI Proxy server v0.2, I've deployed GCE PD CSI driver with the v1 client (this change) and it could successfully avoid connecting to the v1 api because it doesn't exist, the logs:

I0407 05:51:58.024148    4288 safe-mounter_windows.go:63] Attempting to connect to csi-proxy diskapi at path=\\.\\pipe\\csi-proxy-disk-v1
I0407 05:51:58.024148    4288 safe-mounter_windows.go:69] Connection to csi-proxy diskapi at path=\\.\\pipe\\csi-proxy-disk-v1 failed with error=open \\.\\pipe\\csi-proxy-disk-v1: The system cannot find the file specified., might retry with a different version
I0407 05:51:58.025117    4288 safe-mounter_windows.go:63] Attempting to connect to csi-proxy diskapi at path=\\.\\pipe\\csi-proxy-disk-v1beta2
I0407 05:51:58.025117    4288 safe-mounter_windows.go:66] Connected to csi-proxy diskapi at path=\\.\\pipe\\csi-proxy-disk-v1beta2
I0407 05:51:58.025117    4288 safe-mounter_windows.go:81] Attempting to connect to csi-proxy fsapi at path=\\.\\pipe\\csi-proxy-filesystem-v1
I0407 05:51:58.025117    4288 safe-mounter_windows.go:87] Connection to csi-proxy fsapi at path=\\.\\pipe\\csi-proxy-filesystem-v1 failed with error=open \\.\\pipe\\csi-proxy-filesystem-v1: The system cannot find the file specified., might retry with a different version
I0407 05:51:58.025117    4288 safe-mounter_windows.go:81] Attempting to connect to csi-proxy fsapi at path=\\.\\pipe\\csi-proxy-filesystem-v1beta1
I0407 05:51:58.026099    4288 safe-mounter_windows.go:84] Connected to csi-proxy fsapi at path=\\.\\pipe\\csi-proxy-filesystem-v1beta1
I0407 05:51:58.026099    4288 safe-mounter_windows.go:99] Attempting to connect to csi-proxy volumeapi at path=\\.\\pipe\\csi-proxy-volume-v1
I0407 05:51:58.026099    4288 safe-mounter_windows.go:105] Connection to csi-proxy volumeapi at path=\\.\\pipe\\csi-proxy-volume-v1 failed with error=open \\.\\pipe\\csi-proxy-volume-v1: The system cannot find the file specified., might retry with a different version
I0407 05:51:58.026099    4288 safe-mounter_windows.go:99] Attempting to connect to csi-proxy volumeapi at path=\\.\\pipe\\csi-proxy-volume-v1beta1
I0407 05:51:58.026099    4288 safe-mounter_windows.go:102] Connected to csi-proxy volumeapi at path=\\.\\pipe\\csi-proxy-volume-v1beta1

Edit:

Even though I could connect to the v1beta API using the v1 client I couldn't send a message to it, the error I got was:

gce-pd-driver I0407 19:48:16.583101    8696 utils.go:60] /csi.v1.Node/NodeUnstageVolume called with request: volume_id:"projects/UNSPECIFIED/zones/us-central1-b/disks/pvc-1190e7da-72f1-480f-8c9d-95209b061660" staging_target_path:"\\var\\lib\\kubelet\\pl
 ugins\\kubernetes.io\\csi\\volumeDevices\\staging\\pvc-1190e7da-72f1-480f-8c9d-95209b061660"
 gce-pd-driver E0407 19:48:16.584111    8696 utils.go:63] /csi.v1.Node/NodeUnstageVolume returned with error: rpc error: code = Internal desc = NodeUnstageVolume failed: rpc error: code = Unimplemented desc = unknown service v1.Filesystem

In PD CSI we are falling back to using both APIs at the same time, in any case, the check with winio.Dial works :)

Does this PR introduce a user-facing change?:

NONE

/assign @jingxu97

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Apr 6, 2021
@k8s-ci-robot k8s-ci-robot requested review from ddebroy and msau42 April 6, 2021 05:53
@k8s-ci-robot
Copy link
Contributor

Hi @mauriciopoppe. Thanks for your PR.

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

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 6, 2021
client/utils.go Outdated Show resolved Hide resolved
@k8s-ci-robot 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. labels Apr 7, 2021
@mauriciopoppe mauriciopoppe changed the title [WIP] Add another client per API that can communicate with a custom named pipe Add another client per API that can communicate with a custom named pipe Apr 7, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2021
@mauriciopoppe
Copy link
Member Author

/assign @ddebroy

@ddebroy
Copy link
Contributor

ddebroy commented Apr 7, 2021

/retest

@ddebroy
Copy link
Contributor

ddebroy commented Apr 7, 2021

This is super cool!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2021
@ddebroy
Copy link
Contributor

ddebroy commented Apr 7, 2021

Will approve shortly as soon as the pull-kubernetes-csi-csi-proxy reports

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2021
func NewClientWithPipePath(pipePath string) (*Client, error) {

// verify that the pipe exists
_, err := winio.DialPipe(pipePath, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering what is the difference between winio.DialPipe and the following grpc.Dial?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the first test in https://github.com/microsoft/go-winio/blob/master/pipe_test.go, grpc.Dial creates connections in the background unless WithBlock() is also sent as an option (which I also tried but got stuck in the v1 request)

@ddebroy
Copy link
Contributor

ddebroy commented Apr 8, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2021
@jingxu97
Copy link
Contributor

jingxu97 commented Apr 8, 2021

/ok-to-test

@k8s-ci-robot 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 Apr 8, 2021
@mauriciopoppe
Copy link
Member Author

/retest

@mauriciopoppe
Copy link
Member Author

/retest

@ddebroy
Copy link
Contributor

ddebroy commented Apr 8, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2021
@k8s-ci-robot
Copy link
Contributor

[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:

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 added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none Denotes a PR that doesn't merit a release note. 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 Apr 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit a0b516e into kubernetes-csi:master Apr 8, 2021
manueltellez pushed a commit to manueltellez/csi-proxy that referenced this pull request Apr 19, 2021
1d60e77 Merge pull request kubernetes-csi#131 from pohly/kubernetes-1.20-tag
9f10459 prow.sh: support building Kubernetes for a specific version
fe1f284 Merge pull request kubernetes-csi#121 from kvaps/namespace-check
8fdf0f7 Merge pull request kubernetes-csi#128 from fengzixu/master
1c94220 fix: fix a bug of csi-sanity
a4c41e6 Merge pull request kubernetes-csi#127 from pohly/fix-boilerplate
ece0f50 check namespace for snapshot-controller
dbd8967 verify-boilerplate.sh: fix path to script
9289fd1 Merge pull request kubernetes-csi#125 from sachinkumarsingh092/optional-spelling-boilerplate-checks
ad29307 Make the spelling and boilerplate checks optional
5f06d02 Merge pull request kubernetes-csi#124 from sachinkumarsingh092/fix-spellcheck-boilerplate-tests
48186eb Fix spelling and boilerplate errors
71690af Merge pull request kubernetes-csi#122 from sachinkumarsingh092/include-spellcheck-boilerplate-tests
981be3f Adding spelling and boilerplate checks.
2bb7525 Merge pull request kubernetes-csi#117 from fengzixu/master
3b6d17b Merge pull request kubernetes-csi#118 from pohly/cloud-build-timeout
9318c6c cloud build: double the timeout, now 1 hour
4ab8b15 use the tag to replace commit of csi-test
5d74e45 change the csi-test import path to v4
7dcd0a9 upgrade csi-test to v4.0.2
86ff580 Merge pull request kubernetes-csi#116 from andyzhangx/export-image-name
c3a9662 allow export image name and registry name
c6a88c6 Merge pull request kubernetes-csi#113 from xing-yang/install_snapshot_controller
45ec4c6 Fix the install of snapshot CRDs and controller
5d874cc Merge pull request kubernetes-csi#112 from xing-yang/cleanup
79bbca7 Cleanup
d437673 Merge pull request kubernetes-csi#111 from xing-yang/update_snapshot_v1_rc
57718f8 Update snapshot CRD version

git-subtree-dir: release-tools
git-subtree-split: 1d60e7792624a9938c0bd1b045211fbb89e513d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants