add implement for CSI plugin #7
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
|
/assign @gnufied |
pkg/resizer/csi_resizer.go
Outdated
|
|
||
| ctx, cancel := timeoutCtx(r.timeout) | ||
| defer cancel() | ||
| // TODO: Get secrets used for expansion operation. |
There was a problem hiding this comment.
you can get the secret here and remove the TODO :)
There was a problem hiding this comment.
I'm not sure how to get these secrets. The external-attacher got publish/stage secrets from the PV object directly, and the external-snapshotter got snapshot secrets from VolumeSnapshotClass.Parameters. So which way should we adopt? Add a ControllerExpandSecretRef field to CSIPersistentVolumeSource or just fetch these secrets from StorageClass.Parameters?
cc @gnufied
There was a problem hiding this comment.
resizer needs admin credentials, not publish/stage credentials (used by attacher), in most cases.
There was a problem hiding this comment.
Lets do what external-provisioner does. It also fetches secrets from SC. We could use same secret that are being used for provisioning. cc @msau42
There was a problem hiding this comment.
Which secret needs the be used? Same as provision? Or do we need a new secret for resize?
There was a problem hiding this comment.
PV source still refers to CSIPersistentVolumeSource right? So no - I do not think we need resizing secrets there.
There was a problem hiding this comment.
It's where we put all the other non provisioning secrets. So you don't need to reference storageclass.
There was a problem hiding this comment.
OK, I will fetch secrets from SC with a separate name.
There was a problem hiding this comment.
Instead of fetching secrets directly from the storageclass, can it be added as a parameter to the CSIPersistentVolumeSource? That way all the secret fetching/templating logic can be confined to just the external-provisioner.
| // New creates a new CSI client. | ||
| func New(address string, timeout time.Duration) (Client, error) { | ||
| conn, err := newGRPCConnection(address, timeout) | ||
| if err != nil { |
There was a problem hiding this comment.
Lets use connection interface from kubernetes-csi/csi-lib-utils#11 ?
There was a problem hiding this comment.
OK, these util functions make sense to me, I will update this later.
pkg/resizer/csi_resizer.go
Outdated
| ) | ||
|
|
||
| // NewCSIResizer creates a new resizer responsible for resizing CSI volumes. | ||
| func NewCSIResizer(address string, timeout time.Duration) (Resizer, error) { |
There was a problem hiding this comment.
It is bit odd that all these functions take timeout parameter but convert this into a context anyways. Shouldn't we just be passing Context here in the first place?
There was a problem hiding this comment.
WithTimeout returns WithDeadline(parent, time.Now().Add(timeout)), so I think we need to create the context just before each CSI operation. If we passed a Context to NewCSIResizer method, it means the total time consumed by all CSI operations(waitDriverReady, getDriverName, etc.) should not exceed timeout. This is not what we want.
pkg/resizer/csi_resizer.go
Outdated
|
|
||
| ctx, cancel := timeoutCtx(r.timeout) | ||
| defer cancel() | ||
| // TODO: Get secrets used for expansion operation. |
There was a problem hiding this comment.
Lets do what external-provisioner does. It also fetches secrets from SC. We could use same secret that are being used for provisioning. cc @msau42
| return nil | ||
| } | ||
|
|
||
| func getDriverName(client csi.Client, timeout time.Duration) (string, error) { |
There was a problem hiding this comment.
I would preferably convert these functions to directly accept contexts rather than converting them around.
|
@mlmhl can you update this PR with fetching secret from SC and then we can mostly merge this. |
|
@gnufied I've updated this PR to fetch secrets from ps: We should add these keys to external-provisioner too, as it needs to remove these keys from parameters before passing to the driver, see this function. I will post a PR to do this after this PR merged. cc @msau42 What's more, I haven't introduce the connection interface in https://github.com/kubernetes-csi/csi-lib-utils due to the dependency conflict: csi-lib-utils requires https://github.com/container-storage-interface/spec |
|
I can help update CSI lib utils to use the master CSI spec |
|
Thanks @msau42 ! |
|
@saad-ali should we consider tagging a new CSI release since we need it for resizing? |
|
@gnufied Yes. Please please start a thread (propose cutting a CSI 1.1 and a timeline) and add it it to the agenda of the next CSI meeting. |
|
lets merge this for now rather than being blocked on csi tag or csi-lib being updated. Also, for alpha release using secrets from SC is fine, we will update this code to fetch from "new place", once that decision is finalized in kubernetes-csi/external-provisioner#233 /lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, mlmhl 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 |
k8s-ci-robot
added
the
approved
Rebase to v0.4.0
This PR adds an initial implement of CSI resize plugin. Currently we can test this by the csi-test tool with PR kubernetes-csi/csi-test#160