Enable CSI based external volumes for agents - #552
Conversation
c447f89 to
f07fb92
Compare
Michelle Au (msau42)
left a comment
Michelle Au (msau42)
left a comment
There was a problem hiding this comment.
Is there a way to squash the dynamic plugin registration commit with the earlier commits that used a binary flag? It would probably be less confusing for other reviewers that go commit by commit because the binary flag changes get completely replaced later on.
Also in the PR description can you provide a high level summary of the driver registration approach?
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
General comment: Avoid unpinned external references. They're a security risk and they make tests etc unreproducible.
- container images (refer to by @ sha256 digest)
- git repos (pin a git commit by hash or find an alternative)
- install manifests (consider e.g. checking in a copy under a third_party directory)
f07fb92 to
2bc2eed
Compare
3cbbf55 to
8cc2997
Compare
| hostPath: | ||
| path: /var/lib/ateom-gvisor | ||
| type: DirectoryOrCreate | ||
| - name: kubelet-plugins |
There was a problem hiding this comment.
Double check if k8s volume reconstruction feature looks at this directory or not.
There was a problem hiding this comment.
IIUC it primarily uses /var/lib/kubelet/pods/ for volume discovery. Should I add that as well?
8c54c5a to
a9df0ea
Compare
a9df0ea to
7291838
Compare
Michelle Au (msau42)
left a comment
Michelle Au (msau42)
left a comment
There was a problem hiding this comment.
Can you also update the PR description to give a high level overview of the CSI driver registration and communication approach?
| DriverName string `json:"driverName"` | ||
|
|
||
| // ControllerEndpoint is the gRPC endpoint for the CSI Controller service. | ||
| // Must be a valid URI (e.g. unix:///var/run/csi.sock or dns:///csi-service:9000). |
There was a problem hiding this comment.
I would say remove it for now and we can consider adding it back if we need it.
|
|
||
| // NewService creates a service. instruments may be nil; the record helpers no-op. | ||
| // PluginRegistry defines the interface for dynamic CSI plugin resolution. | ||
| type PluginRegistry interface { |
There was a problem hiding this comment.
I would probably rename this to VolumePluginRegistry
| # 1. Ensure driver is cloned and checked out to pinned commit | ||
| if [ ! -d "${DRIVER_DIR}" ]; then | ||
| echo "CSI Hostpath Driver not found at ${DRIVER_DIR}. Cloning..." | ||
| git clone https://github.com/kubernetes-csi/csi-driver-host-path.git "${DRIVER_DIR}" |
There was a problem hiding this comment.
I think Ben was suggesting that we copy the manifests directly in third_party directory so we don't introduce a dependency on another repo.
| hostPath: | ||
| path: /var/lib/ateom-gvisor | ||
| type: DirectoryOrCreate | ||
| - name: kubelet-plugins |
There was a problem hiding this comment.
Add a comment to explain what this directory is used for
| func NewPlugin(client *Client) *Plugin { | ||
| return &Plugin{ | ||
| client: client, | ||
| stagingDirPrefix: "/var/lib/ateom-gvisor/staging", |
There was a problem hiding this comment.
Use ateompath helper to get the path
| STATUS_DELETING = 3; | ||
| } | ||
| Status status = 4; | ||
| // volume_context contains metadata returned by the CSI driver during volume |
There was a problem hiding this comment.
Does the CSI spec have any restrictions on syntax that we can add validation on?
| // Matches the StorageClass referenced in ActorTemplate volume definitions. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 |
There was a problem hiding this comment.
You can reuse csidriver name validation that I'm adding in #678
| // Must be a valid URI (e.g. unix:///var/run/csi.sock or dns:///csi-service:9000). | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:Pattern=`^(unix|tcp|dns)://.+$` |
There was a problem hiding this comment.
I think we can harden the validation to be stricter for grpc endpoints. (can be done as a followup)
4 participants
#232
Integrate the with the volume interface created in #405, to enable CSI managed external volumes throughout the agent lifecycle.