Repository navigation
feat: mount an existing volume per actor at a sub-path - #234
Merged
Merged
Conversation
DeepEqual handed a slice of proto messages to reflect.DeepEqual, which compares each message's internal state as well as its fields. The RPC logger's marshal fills the size cache of a request's messages, so a repeated message field and its clone compared unequal, and declarative validation's unchanged-value shortcut then ran the field's update checks: an immutable repeated field failed every create, because the create validates the stored object as an update of the request. A slice of messages is now compared element by element with proto.Equal, a nil and an empty one being the same field value. Signed-off-by: Timo Derstappen <teemow@gmail.com>
Signed-off-by: Timo Derstappen <teemow@gmail.com>
Actors that work in one shared workspace need part of a volume that exists outside Substrate and outlives them: their own directory read-write and another directory of the same volume read-only. External volumes were created per actor, attached single-node-writer and deleted with the actor, and a mount had no sub-path or read-only flag. An ActorTemplate now declares an existing volume by name (Volume.existing_volume), and CreateActor supplies it per actor (Actor.existing_volumes: name, CSI driver, volume handle, access mode READ_WRITE_MANY or READ_ONLY_MANY, and the sub-path this actor sees as the volume's root). A VolumeMount gains sub_path and read_only, so one volume may be mounted at several paths. CreateActor refuses a reference to a volume the template does not declare as existing, a driver without a CSIDriverConfig, a handle no PersistentVolume of the driver holds and an access mode the PersistentVolume does not permit; the PersistentVolume's volume attributes are passed to the driver at resume. An existing volume the actor does not supply contributes neither the volume nor its mounts. Substrate never creates, deletes or detaches an existing volume: pause, resume and delete only unmount and mount it. A volume of a multi-node mode is staged once per target, so one actor's unmount never unstages it under another actor on the node. ateom, which sees the published volume and may mount, binds each sub-path before the sandbox starts and releases it when the sandbox stops, on both runtimes: from a descriptor opened beneath the volume's root without following symbolic links, so a link another actor wrote on the volume fails the mount rather than redirecting it, and a missing directory fails it too. An actor with existing volumes boots from its image instead of the template's golden snapshot, which was captured without their mounts, and cannot be created from a tag. The existingvolumes e2e suite runs on kind with the CSI NFS driver. Signed-off-by: Timo Derstappen <teemow@gmail.com>
Records the existing-volume change, the upstream issue it follows and when it leaves the line. Signed-off-by: Timo Derstappen <teemow@gmail.com>
Signed-off-by: Timo Derstappen <teemow@gmail.com>
1 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Actors that work in one shared workspace need part of a volume that exists outside Substrate and outlives them: their own directory read-write, and another directory of the same volume read-only. On this line an external volume is created per actor, attached single-node-writer and deleted with the actor; a
VolumeMounthas no sub-path and no read-only flag; anActorcarries no volume of its own.Proposed solution
The shape proposed upstream in agent-substrate#1637 (agent-substrate#1637 (comment)), on top of the access modes of agent-substrate#1988:
Template:
Volume.existing_volume(an empty marker) declares a volume each actor supplies. Mounts of it may set the newVolumeMount.sub_pathandread_only. Templates still refuse a mount of an undeclared volume, andsub_path/read_onlyon any other kind of volume.Actor:
CreateActortakesActor.existing_volumes(immutable). EachExistingVolumehas a name, a CSI driver, a volume handle, an access mode (READ_WRITE_MANYorREAD_ONLY_MANY, the shape of volume: support configurable access modes agent-substrate/substrate#1988'sVolumeAccessMode), and asub_path: the directory this actor sees as the volume's root. Template mount sub-paths are relative to it, so one template serves every actor, each in a directory of its own (sessions/<id>read-write,mirrorsread-only, both from one handle).Refused at create, with the reason:
CSIDriverConfig;ate-api-server reads PersistentVolumes (a new ClusterRole rule). The PersistentVolume's
volumeAttributesreach the driver at resume.Unsupplied volumes: an existing volume the actor does not supply contributes neither the volume nor its mounts. The create request of such an actor is unchanged.
Lifecycle: Substrate never creates, deletes or detaches an existing volume (other actors on the node may be using it). Pause, resume (on another node too) and delete only unmount and mount it.
CSI: a multi-node volume is published with
MULTI_NODE_MULTI_WRITERorMULTI_NODE_READER_ONLY. It is staged once per target, so one actor's unmount never unstages it under another actor on the node.Sub-paths on the node: ateom (which sees the volume the CSI node plugin published and may mount; atelet may do neither) binds each sub-path, on both runtimes (
internal/volumebind), before the sandbox starts and releases it when the sandbox stops. It binds from a descriptor opened beneath the volume's root withopenat2(RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS | RESOLVE_NO_XDEV), then remounts it read-only where asked. A symbolic link another actor wrote on the volume therefore fails the mount instead of redirecting it, and so does a missing directory: nothing is auto-created. The OCI spec binds the per-mount directory, read-only when asked. On the microvm runtime the bind is made before the volumes directory is shared into the guest, so the guest sees it too.Snapshots and tags: an actor with existing volumes boots from its image instead of restoring the template's golden snapshot, which was captured without their mounts. It cannot be created from a tag, since a tag does not record the mounts its guest state was captured with.
Gate: upstream puts this behind the Preview gate of Add a "preview gate" concept, use it for external volumes agent-substrate/substrate#1994, which this line does not carry, so it is ungated here.
Commits:
FORK.mdrow with the upstream issue and the exit. The repeated-message equality fix is carried as its own commit with its own row.resources.DeepEqualcompared repeated message fields withreflect.DeepEqual, so an immutable repeated field (hereexisting_volumes) failed everyCreateActorwith "field is immutable".Design notes:
ValidateVolumeCapabilities(the NFS CSI driver does), so the check is the cluster's own record: a PersistentVolume of the driver with that handle, permitting the access mode.NodeUnstageVolumetouches only its own staging. Existing volumes are neverControllerUnpublishVolumed. Neither needs a per-node count of the actors using the volume.VolumeAccessModeenum is mirrored. Its own prerequisites (CSI volume support: follow-up improvements agent-substrate/substrate#1729, csi: add publishContext to actor volumes agent-substrate/substrate#1578) are therefore not needed here.Replaces #231 (the snapshot-seeded per-actor volume, superseded by this design) and #233 (this change with the ateom bind as a second commit; the tree is the same, CI green there). On this rebase-merged line the feature lands as one commit.
Acceptance criteria
Proof at head 6572232:
TestExistingVolumespassed on every e2e lane, the helm-e2e job (https://github.com/giantswarm/substrate/actions/runs/37956501224/job/113908245803) and both runtimes of the E2E job (https://github.com/giantswarm/substrate/actions/runs/37956500963/job/113908872436); the unit tests inrun-testsof the same run.sessions/aread-write andmirrorsread-only at the declared paths (TestExistingVolumes/MountsSubPathsAtDeclaredPaths, PASS on all three lanes)ActorsWriteOnlyTheirOwnSubPath, PASS on all three lanes)PauseResumeDeleteLeaveTheVolume, PASS on all three lanes)TestWorkloadSpecExistingVolumes/without_a_reference_the_request_is_unchanged; e2eNoReferenceNoMounts(PASS on all three lanes)TestValidateExistingVolumes; e2eRefusesBadReferencesAtCreate/{unknown_driver,missing_volume}(PASS on all three lanes)FORK.mdrow naming the upstream issue it follows (rows in 5421c8e and 6572232,fork-ledgergreen); released as a line release candidate: the release that follows the mergeCloses #227.
This pull request was written by an agent.