agent-substrate / agent-substrate/substrate

CSI volume support: follow-up improvements from PR #552

Đang mở
#1,077 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
area/storage kind/feature
Ngôn ngữ chính
Go
Star
1.8k
Fork
316
Merge trung bình
2 ngày 43 phút
Pull request đã merge (30 ngày)
287

Mô tả

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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.