Repository navigation
ateapi: check mounted volumes are attachable and test assigned_node detach on crash - #2611
Conversation
| ref := &ateapipb.ObjectRef{Atespace: actor.GetMetadata().GetAtespace(), Name: actor.GetMetadata().GetName()} | ||
| return getMountedActorVolumes(ctx, ref, actor.GetStatus().GetExternalVolumes(), tmpl) | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: generally helper functions improve readability, but this is sort of a wrapper for another helper function. I'd recommend keeping it as it was
| func checkVolumesAttachable(volumes []*ateapipb.ExternalVolume) error { | ||
| for _, vol := range volumes { | ||
| switch { | ||
| case vol.GetStatus() != ateapipb.ExternalVolume_STATUS_CREATED: | ||
| return apierror.FailedPrecondition("volume %q is %s, want %s before it is attached", vol.GetName(), vol.GetStatus(), ateapipb.ExternalVolume_STATUS_CREATED) | ||
| case vol.GetStorageVolumeId() == "": | ||
| return apierror.FailedPrecondition("volume %q has no storage volume ID to attach", vol.GetName()) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
We are actually getting rid of volume status in #2456. I don't think you need to rebase on the change, just use the StorageVolumeID != "" check.
58095e4 to
5567567
Compare
a88d3b4 to
47d2b5c
Compare
| if assignedWorker.GetNodeName() == "" && len(getMountedActorVolumes(ctx, ref, actor.GetStatus().GetExternalVolumes(), actorTemplate)) > 0 { | ||
| return nil, nil, apierror.FailedPrecondition("worker %s has no node name to attach the actor's volumes to", assignedWorker.GetMetadata().GetName()) | ||
| } |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Gating this check on mounted volumes still lets actors without volumes book a nodeless worker and fail right after in ensureVolumesAttached (cmd/ateapi/internal/controlapi/workflow_resume.go:732). Every actor needs a node name (Worker.node_name and WorkerAssignment.node_name are required, and ensureAteletRestored dials atelet by node). Returning here after workerHoldingStaleClaim also leaves a stale claim unreleased. Reject an empty node name in scheduling.Applies (or unconditionally here) instead of only when volumes are mounted, and drop the wantErr: false case in TestAssignWorkerAttempt_WorkerWithoutNodeRejectsVolumes.
There was a problem hiding this comment.
Addressed
| mockVolume("u", "id-u"), | ||
| mockVolume("o", "id-o"), |
There was a problem hiding this comment.
🤖 nit 🟢 – Seeding u and o with non-empty storage IDs means "all provisioned" would still pass if checkVolumesAttachable checked actor.GetStatus().GetExternalVolumes() instead of only mounted volumes. Give u and o empty storage IDs here so the test also verifies unmounted and undeclared volumes are ignored.
There was a problem hiding this comment.
addressed
| if storageClass != "" { | ||
| requireAssignedToWorkerNode(ctx, t, clients, actorRef) | ||
| } |
There was a problem hiding this comment.
🤖 nit 🟢 – assigned_node is set on every resume and cleared on every suspend and revert, whether the actor mounts an external volume or not. Drop the storageClass != "" guard here and on lines 428 and 470 so clusters without a CSI StorageClass still verify assigned_node.
There was a problem hiding this comment.
addressed
Bowei Du (bowei)
added
area/api-machinery
area/storage
area/tests
6f25f23 to
c1b3ec1
Compare
|
Can you double check the failing tests? |
Just look into it, I think it's some ephemerall runner GitHub stall rather than a code issue |
55e5822 to
3102795
Compare
…its it, following PR#2879 merging
3102795 to
26316c6
Compare
|
PR#2961 fixed the e2e ingressauth test, I also updated the capacity volume to conform to PR#2769 |
…s assigned_node to be retained after synchronous suspend
Some storage backends (e.g. Cloud Filestore IP export rules) restrict a volume to the node it is published to via CSI
ControllerPublishVolume/ControllerUnpublishVolume, so every publish must be matched by an unpublish to the same node. Since #2222 , detach targetsActorStatus.assigned_node, which survives crashes and worker loss. This PR adds checks before attach and test coverage for the paths where detach used to be skipped.Follows #2094, which covers the publish side.
Changes
Worker without a node name:
assignWorkerAttemptrejects such a worker with when the actor mounts external volumes. Previously the resume failed inensureVolumesAttached, after the worker was booked.Volume checks before attach:
ensureVolumesAttachedchecks that every mounted volume isSTATUS_CREATEDand has astorage_volume_idbefore any attach call, so one bad volume can't leave earlier volumes published.Unit tests, functional tests
E2E:
assigned_nodeequals the worker'snode_namewhile RUNNING and is cleared after suspend and revert. The OSS NFS driver doesn't implementControllerPublishVolume.Tests pass