Enable CSI based external volumes for agents - #552
Conversation
1f35e1d to
c447f89
Compare
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?
| @@ -0,0 +1,58 @@ | |||
| // Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
Put these under internal/volumes since we don't expect consumers of substrate to need this.
|
|
||
| resp, err := p.client.ControllerPublishVolume(ctx, req) | ||
| if err != nil { | ||
| if status.Code(err) == codes.Unimplemented { |
There was a problem hiding this comment.
Add a followup item to determine this ahead of time by querying the csi driver's controller capabilities. The log message will be very spammy. Same thing with all the volume operations
| return fmt.Errorf("CSI ControllerPublishVolume failed: %w", err) | ||
| } | ||
|
|
||
| // NOTE: CSI ControllerPublishVolume returns PublishContext (metadata needed for mounting). |
There was a problem hiding this comment.
Add a followup item for it
| } | ||
|
|
||
| // Helper to provide standard capabilities for general volume operations. | ||
| func getStandardCapabilities() []*csi.VolumeCapability { |
There was a problem hiding this comment.
Add followup item that we may need to expose access modes
| if err := s.volumePlugin.DeleteVolume(ctx, vol.GetStorageVolumeId()); err != nil { | ||
| plugin, ok := s.volumePlugins[vol.GetVolumeType()] | ||
| if !ok { | ||
| slog.ErrorContext(ctx, "No volume plugin found for type during cleanup", slog.String("volume_type", vol.GetVolumeType()), slog.String("volume_id", vol.GetStorageVolumeId())) |
There was a problem hiding this comment.
I think this should be a blocking error otherwise we will leak disks
| ControllerEndpoint string `json:"controllerEndpoint"` | ||
|
|
||
| // NodeSocketOverride is an optional override for the CSI Node service socket | ||
| // on the worker nodes. If empty, ATE defaults to the standard path. |
There was a problem hiding this comment.
What is the standard path?
|
|
||
| # Define paths | ||
| DRIVER_DIR="${ROOT}/bin/csi-driver-host-path" | ||
| DEPLOY_DIR="${DRIVER_DIR}/deploy/kubernetes-1.27" |
There was a problem hiding this comment.
This is a very old version of hostpath driver
| @@ -0,0 +1,121 @@ | |||
| # Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
Add "-kind" suffix since this looks specific to kind clusters
| capabilities: | ||
| drop: | ||
| - ALL | ||
| privileged: true |
There was a problem hiding this comment.
What needs this?
| @@ -0,0 +1,92 @@ | |||
| # Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
The manifests are not strictly for e2e testing, so I would move them to the driver registration commit
| @@ -0,0 +1,121 @@ | |||
| # Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
Do we want to invoke these scripts as part of install-ate-kind.sh? Maybe optional?
2 participants
#232
Integrate the with the volume interface created in #405, to enable CSI managed external volumes throughout the agent lifecycle.