agent-substrate / agent-substrate/substrate
CSI volume support: follow-up improvements from PR #552
- Linguagem predominante
- Go
- Estrelas
- 1.8k
- Forks
- 316
- Merge médio
- 2d 43min
- PRs com merge (30d)
- 287
Descrição
PR #552 ("CSI volume support") introduced the initial CSI integration—a `CSIDriverConfig` CRD, an `internal/volume/csi` plugin, and control-plane/worker-plane wiring. It landed with seven `TODO` comments in non-vendored code. This issue tracks them so they aren't lost.
### 1. Correctness / driver compatibility
- [ ] **Propagate `PublishContext` from `ControllerPublishVolume` to the node mount path**
— https://github.com/agent-substrate/substrate/blob/d7629ed42eef637bacb9ff29c75428e3ec9904b7/internal/volume/csi/plugin.go#L125
Current behavior: `Plugin.AttachVolume` discards the response (`_ = resp.GetPublishContext()`) because the Substrate `VolumePlugin` interface has nowhere to put it.
Proposed fix: extend `VolumePlugin` so `AttachVolume` returns the publish context, persist it alongside the volume's assignment state, and pass it into `NodeStageVolume`/`NodePublishVolume` on the worker.
Why it matters: drivers that require it (e.g. AWS EBS) cannot successfully mount today, so this gates any real cloud driver beyond the hostpath test driver.
- [ ] **Support configurable volume access modes instead of hardcoding `SINGLE_NODE_WRITER`**
— https://github.com/agent-substrate/substrate/blob/d7629ed42eef637bacb9ff29c75428e3ec9904b7/internal/volume/csi/plugin.go#L236
Current behavior: `getStandardCapabilities()` returns one hardcoded `SINGLE_NODE_WRITER` + `Mount` capability, used for every create, attach, and mount call.
Proposed fix: add an access-mode field to the volume API type, map it to the corresponding `csi.VolumeCapability_AccessMode` enum, and validate the requested mode against the driver's reported capabilities. Unblocks `ReadWriteMany` / `ReadOnlyMany` volumes shared across actors.
### 2. Efficiency and log noise
- [ ] **Query CSI driver capabilities at plugin initialization**
— https://github.com/agent-substrate/substrate/blob/d7629ed42eef637bacb9ff29c75428e3ec9904b7/internal/volume/csi/plugin.go#L113
Current behavior: `AttachVolume` optimistically calls `ControllerPublishVolume`, catches `codes.Unimplemented`, and logs a warning on every attach for drivers without `PUBLISH_UNPUBLISH_VOLUME` (including the hostpath driver used in e2e).
Proposed fix: call `ControllerGetCapabilities` once during plugin init, cache the capability set on the `Plugin`, and skip attach/detach entirely when unsupported.
- [ ] **Reconsider the shared informer used solely for the `CSIDriverConfig` lister in atelet**
— https://github.com/agent-substrate/substrate/blob/d7629ed42eef637bacb9ff29c75428e3ec9904b7/cmd/atelet/main.go#L221
Current behavior: atelet spins up a full `SharedInformerFactory` (resync `0`) on every worker node just to obtain `Api().V1alpha1().CSIDriverConfigs().Lister()`, caching all CSIDriverConfigs cluster-wide per node.
Proposed fix: given the lister is read infrequently, evaluate a direct client `Get` with a small TTL cache, or a field/label-filtered (or metadata-only) informer, to reduce per-node memory and API-server watch load.
### 3. Consistency and API hardening
- [ ] **Extract a shared volume-plugin lookup helper for control plane and worker plane**
— control plane: https://github.com/agent-substrate/substrate/blob/d7629ed42eef637bacb9ff29c75428e3ec9904b7/cmd/ateapi/internal/controlapi/volumes.go#L145
— worker plane: https://github.com/agent-substrate/substrate/blob/d7629ed42eef637bacb9ff29c75428e3ec9904b7/cmd/atelet/volumes.go#L69
Current behavior: `deleteActorVolumes` (`registry.GetPlugin`) and `unmountExternalVolumes` (`s.getPlugin`) each duplicate plugin resolution and error wrapping with slightly different message formats.
Proposed fix: add one helper in `internal/volume` that resolves a plugin by volume type and returns a consistently wrapped error. Constraint: it must preserve the underlying gRPC status code—the worker path relies on `status.Code(err) == codes.NotFound` to treat an unmount as already completed.
- [ ] **Harden `CSIDriverConfig.ControllerEndpoint` validation**
— https://github.com/agent-substrate/substrate/blob/d7629ed42eef637bacb9ff29c75428e3ec9904b7/pkg/api/v1alpha1/csidriverconfig_types.go#L34
Current behavior: validated only by `+kubebuilder:validation:Pattern=` + "`^(tcp|dns)://.+$`", which accepts effectively any string after the scheme and has no length bound.
Proposed fix: tighten the CEL/regex validation to a real host:port (or `dns:///` target) form, add a `MaxLength`, and consider a validating webhook for anything the schema can't express. This field determines where the control plane dials, so a permissive value is a request-forgery vector.
Guia de contribuição
Avaliação
Esta issue ainda não foi avaliada.