Repository navigation
test(volume): verify node payload and mock driver publishContext round-trip. - #2094
Conversation
3d6ecf5 to
1e3c7f2
Compare
Vu Le (lekhoavu2909)
changed the title
1e3c7f2 to
a0be897
Compare
Vu Le (lekhoavu2909)
changed the title
Vu Le (lekhoavu2909)
changed the title
|
Thanks for adding publishContext coverage. Before this goes in, I'd suggest building on the lifecycle tests Sneha added in #1581 rather than adding parallel ones, since most of the new setup here already exists there. Functional test
Proposal:
That gives one lifecycle test with strictly more coverage than either and no duplicated setup. Unit test
|
Vu Le (lekhoavu2909)
changed the title
|
Address hajiler 's comment above |
Vu Le (lekhoavu2909)
changed the title
15393f2 to
2c05799
Compare
e4ea563 to
b9eba6b
Compare
…d-trip Assert that ControllerPublishVolume and ControllerUnpublishVolume receive the target NodeId in CSI plugin unit tests. Update MockVolumePlugin.AttachVolume to return realistic publish context metadata reflecting the target node. Add TestExternalVolume_PublishContextLifecycle to verify complete volume attachment, node migration, and clean detachment across resume, pause, suspend, and delete, asserting on fakeAtelet wire delivery. Add TestEnsureVolumesAttached_NodeMigration verifying in-memory publish context delivery on worker node migration.
b9eba6b to
dd4ca23
Compare
Vu Le (lekhoavu2909)
changed the title
|
/LGTM |
3aca92f to
8aea45e
Compare
| } | ||
|
|
||
| // 5. Clean up by suspending and deleting the actor. | ||
| // 5. Suspend to detach from node1 and clear local snapshot, then migrate to node2. |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The merge from main reverted the rename and doc comment at cmd/ateapi/internal/controlapi/functionaltest/actor_test.go:2692-2704. The function is still named TestPauseActor_VolumeLifecycle_DetachAndResumeAttach, and its header comment still describes the old 5-step cleanup instead of the 6-step migration and delete flow tested here.
There was a problem hiding this comment.
Addressed
| driver.controllerUnpublishVolumeFunc = func(ctx context.Context, req *csi.ControllerUnpublishVolumeRequest) (*csi.ControllerUnpublishVolumeResponse, error) { | ||
| if req.GetNodeId() != "node-1" { | ||
| t.Errorf("ControllerUnpublishVolume received NodeId %q, want %q", req.GetNodeId(), "node-1") | ||
| } | ||
| if req.GetVolumeId() != "test-vol" { | ||
| t.Errorf("ControllerUnpublishVolume received VolumeId %q, want %q", req.GetVolumeId(), "test-vol") | ||
| } | ||
| return &csi.ControllerUnpublishVolumeResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Unlike AttachVolume (where the test checks resp.PublishContext), DetachVolume returns no response payload. If DetachVolume skips the RPC, controllerUnpublishVolumeFunc never runs and both checks in this test pass silently. Record a called counter and assert it ran after each DetachVolume call.
There was a problem hiding this comment.
Addressed
8aea45e to
7ff961b
Compare
9b5cf5c to
7d77fe6
Compare
…etach on crash (agent-substrate#2611) 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 agent-substrate#2222 , detach targets `ActorStatus.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 agent-substrate#2094, which covers the publish side. - *ateapi: check mounted volumes are attachable before publishing* - *test(ateapi): cover volume detach via assigned_node after crash or worker loss* #### Changes - Worker without a node name: `assignWorkerAttempt` rejects such a worker with when the actor mounts external volumes. Previously the resume failed in `ensureVolumesAttached`, after the worker was booked. - Volume checks before attach: `ensureVolumesAttached` checks that every mounted volume is `STATUS_CREATED` and has a `storage_volume_id` before any attach call, so one bad volume can't leave earlier volumes published. - Unit tests, functional tests - E2E: `assigned_node` equals the worker's `node_name` while RUNNING and is cleared after suspend and revert. The OSS NFS driver doesn't implement `ControllerPublishVolume`. - [x] Tests pass
Some external storage backends (such as Cloud Filestore IP export rules) scope volume access to the Kubernetes worker node currently hosting an actor via CSI
ControllerPublishVolumeandControllerUnpublishVolume.This PR verifies end-to-end NodeId and PublishContext forwarding across the CSI plugin, control plane resume workflow, and functional actor lifecycle (including cross-node migration).
Changes
CSI plugin:
TestPlugin_AttachVolume/TestPlugin_DetachVolumeassert theNodeIdandVolumeIdsent on publish and unpublish, including theUnimplementedpath.Mock drivers:
MockVolumePluginand the functional test'sattachFailVolumePluginreturn a publish context naming the target node and volume, so existing lifecycle tests
now carry a non-empty
publishContextthrough Restore.Lifecycle test: builds on the enhance tests to validate volume lifecycle for actor state changes #1581 pause lifecycle test, renamed
TestPauseActor_VolumeLifecycle_DetachResumeAndMigrate. It checks the Restore request'spublishContextafter each resume, adds a migration leg (suspend → worker on node2 →resume → attach to node2), and verifies detach, volume deletion, and actor removal on delete.
lastRestoreVolumePublishContextreplaces the per-test lookup of the Restore request.Unit test:
TestEnsureVolumesAttached_ReturnsPublishContextalso asserts thatAttachVolumeis called once, for the mounted volume, with the worker's node.Tests pass