feat(storage): add persistent volume snapshot/resize/persistence suite (DATASVC-XX-02/03/04)#439
Open
abegnoche wants to merge 10 commits into
Open
feat(storage): add persistent volume snapshot/resize/persistence suite (DATASVC-XX-02/03/04)#439abegnoche wants to merge 10 commits into
abegnoche wants to merge 10 commits into
Conversation
📝 WalkthroughWalkthroughAdds a storage validation suite and wiring for provider-agnostic block-storage checks, plus AWS reference scripts and my-isv demo stubs for create, snapshot, resize, persistence, and teardown flows. ChangesStorage validation suite
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
abegnoche
changed the title
cursor
Bot
changed the title
Member
Author
|
Note: currently in draft because it adds a new storage.yaml file |
Adds a provider-agnostic block-storage validation suite plus the AWS reference implementation covering the block-storage trio that share one fixture (launch instance + create/attach EBS volume): - DATASVC-XX-02 (#321) snapshot_lifecycle: CreateSnapshot -> restore to a new volume -> attach/mount -> byte-compare the sentinel data. - DATASVC-XX-03 (#322) volume_resize: ModifyVolume + in-guest growpart/resize2fs, verifying the guest sees the larger filesystem. - DATASVC-XX-04 (#323) volume_persistence: stop/start the instance and verify the volume stays attached with its data intact. Reuses existing StepSuccessCheck + FieldExistsCheck + CrudOperationsCheck (no new validation classes, no released_tests.json change). common/ebs.py centralizes the boto3 volume/snapshot lifecycle and the Nitro NVMe by-id device mapping; create_volume/teardown_volume own the shared fixture. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Adds the my-isv living-example scaffold (TODO blocks + ISVCTL_DEMO_MODE dummy success) for the five block-storage scripts and the provider config that imports the suite. Adds block-storage to MY_ISV_DOMAINS so 'make demo-test' exercises the whole pipeline end-to-end with no cloud. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Adds the block-storage suite to the contracts list and per-step field table in suites/README.md, and a block-storage/ row to the my-isv scaffold domains table. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Covers the common/ebs Nitro device mapping and modification waiter, plus happy-path and failure cases (guest setup failure, snapshot content mismatch, cleanup leak, growpart failure, volume detached after restart) for the five AWS block-storage scripts using boto3/ssh fakes. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
boto3 waiters raise WaiterError (a BotoCoreError, not a ClientError), so a waiter timeout would have propagated uncaught and skipped the JSON contract the validations depend on - the same failure mode #438 fixed for S3. Broaden the AWS script except clauses to (ClientError, BotoCoreError) and make the ebs cleanup helpers swallow BotoCoreError so best-effort teardown never raises out of a finally block. Also pass the snapshot ID to cleanup when CreateSnapshot succeeded but the completion wait failed. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
…new suite Move the DATASVC-XX-02/03/04 block-storage validations out of the standalone suites/block-storage.yaml and into suites/vm.yaml, so block storage is validated under the existing VM contract rather than adding another suite file. - vm.yaml: add fixture_volume / snapshot_lifecycle / volume_resize / volume_persistence / volume_teardown_checks. These activate only when a provider config supplies the matching volume steps; a standard VM run skips them automatically. Renamed the volume teardown check to volume_teardown_checks to avoid colliding with the VM teardown_checks, and dropped the redundant instance_launched (covered by setup_checks). - providers/aws|my-isv/config/block-storage.yaml: import suites/vm.yaml, rename the command group to vm (must match the inherited platform), and exclude the ssh-labelled VM host checks (cloud-init/GPU/OS/NIM are not block-storage concerns). - delete suites/block-storage.yaml. - docs: drop block-storage from the suites list and point the domain's contract at vm.yaml. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Replace the earlier approach of folding block-storage validations into suites/vm.yaml. Instead, add a dedicated suites/storage.yaml umbrella contract (platform: storage) that today covers persistent block storage (DATASVC-XX-02/03/04) and is the future home for object/file storage checks - avoiding both suite proliferation and the skip-noise of sharing the VM contract. - suites/storage.yaml: new umbrella contract (was the block-storage suite). - suites/vm.yaml: reverted to its original VM-only validations. - providers/aws|my-isv/config/block-storage.yaml: import suites/storage.yaml and use the 'storage' command group (matches the suite platform). - docs: list the 'storage' suite and point the block-storage domain's contract at storage.yaml. The block-storage provider domain (config/block-storage.yaml + scripts/block-storage/) stays as the concrete block-volume implementation of the storage contract. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Complete the storage-suite rename by aligning the runnable provider domain with the suite name. - providers/aws|my-isv/config/storage.yaml replace config/block-storage.yaml. - providers/aws|my-isv/scripts/storage/ replace scripts/block-storage/. - Makefile demo domain is now storage, so the focused smoke test is make demo-storage. - Tests and docs now refer to storage paths and the AWS unit test file is renamed to test_aws_storage_scripts.py. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/block-storage-trio-datasvc-35b8
branch
from
fc35116 to
25bca23
Compare
Add test_id and labels to storage.yaml checks (DATASVC02-01 through DATASVC04-01), register the storage suite in validate_suite_wiring.py, and sync the storage label into docs/test-plan.yaml. These guardrails landed on main after PR #439 was opened. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Member
Author
|
/ok to test cfe0ce8 |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-09 18:33:45 UTC | Commit: cfe0ce8 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@isvctl/configs/providers/aws/scripts/storage/create_volume.py`:
- Around line 136-154: The instance lookup in the create_volume flow can crash
before the existing error handling because describe_instances() may return an
empty Reservations list and the code indexes into
instances["Reservations"][0]["Instances"][0]. Add an explicit empty-result check
in the create_volume script before reading instance details, and surface a JSON
error path when no instance is found. Apply the same safeguard in the
corresponding DescribeInstances handling in snapshot_lifecycle, volume_resize,
and volume_persistence so they all fail gracefully instead of raising
IndexError.
In `@isvctl/configs/providers/aws/scripts/storage/volume_resize.py`:
- Line 167: The final size-verification call in the volume resize flow is
unguarded, so a transient AWS failure can crash the script after the resize work
has already completed. Update the `main` flow in `volume_resize.py` around the
`describe_volumes` verification for `ebs_grew` to wrap it in the same
`try`/`except (ClientError, BotoCoreError)` pattern used elsewhere, and return
the existing JSON error response instead of letting a traceback escape.
In `@isvctl/configs/providers/my-isv/scripts/storage/volume_resize.py`:
- Line 60: Reject non-positive resize deltas in demo mode by validating the
parsed `--grow-gib` value before `success=True` is set in `volume_resize.py`.
Update the demo-mode path in `main` (and any helper logic around
`new_size_gib`/`old_size_gib`) so it fails or exits early when `grow_gib <= 0`
or would not increase the volume size, instead of reporting success. Keep the
check close to the existing resize calculation so the script’s demo-mode
contract is enforced consistently.
In `@isvctl/configs/suites/storage.yaml`:
- Around line 78-91: The storage suite checks for fixture_volume (and the
similar cases in the other storage blocks) only validate IDs and operation
names, so they miss the real contract outcomes. Update the relevant check
definitions to require the documented result fields from the storage workflow,
using the existing FieldExistsCheck/related checks tied to the storage fixture
steps, so the suite verifies the seeded sentinel, restore content match, and
filesystem size expectations rather than just
create/attach/format/mount/write_sentinel being present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0bc78e71-1cb7-4da3-9765-b4c067d72597
📒 Files selected for processing (20)
Makefiledocs/test-plan.yamlisvctl/configs/providers/aws/config/storage.yamlisvctl/configs/providers/aws/scripts/common/ebs.pyisvctl/configs/providers/aws/scripts/storage/create_volume.pyisvctl/configs/providers/aws/scripts/storage/snapshot_lifecycle.pyisvctl/configs/providers/aws/scripts/storage/teardown_volume.pyisvctl/configs/providers/aws/scripts/storage/volume_persistence.pyisvctl/configs/providers/aws/scripts/storage/volume_resize.pyisvctl/configs/providers/my-isv/config/storage.yamlisvctl/configs/providers/my-isv/scripts/README.mdisvctl/configs/providers/my-isv/scripts/storage/create_volume.pyisvctl/configs/providers/my-isv/scripts/storage/snapshot_lifecycle.pyisvctl/configs/providers/my-isv/scripts/storage/teardown_volume.pyisvctl/configs/providers/my-isv/scripts/storage/volume_persistence.pyisvctl/configs/providers/my-isv/scripts/storage/volume_resize.pyisvctl/configs/suites/README.mdisvctl/configs/suites/storage.yamlisvctl/tests/test_aws_storage_scripts.pyscripts/validate_suite_wiring.py
Add ebs.describe_instance() and use it in all four AWS storage scripts so an empty DescribeInstances response returns JSON instead of IndexError. Wrap the final describe_volumes size check in volume_resize.py, preserve API error messages when verify_size already failed, and reject non-positive --grow-gib in the my-isv demo stub. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
Member
Author
|
/ok to test 798f788 |
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
2 participants
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.
What
Closes #321
Closes #322
Closes #323
Adds a new
storagevalidation suite covering the persistent volume trio:snapshot_lifecycle: snapshot a volume, restore it, and verify sentinel data.volume_resize: grow the volume and in-guest filesystem, then verify size.volume_persistence: stop/start the instance and verify volume/data persistence.storage.yamlis intentionally broad so future object/file storage checks can land in the same suite.Changes
suites/storage.yamlprovider-agnostic contract.providers/aws/config/storage.yamlandproviders/aws/scripts/storage/.my-isvscaffold underproviders/my-isv/config/storage.yamlandproviders/my-isv/scripts/storage/.storagetoMY_ISV_DOMAINS(make demo-storage).isvctl/tests/test_aws_storage_scripts.py.Validation
ISVTEST_INCLUDE_UNRELEASED=1 AWS_PROFILE=ncp-isv-lab uv run isvctl test run -f isvctl/configs/providers/aws/config/storage.yaml -- -v -s— AWS live run passed setup, all three storage tests, and teardown.make demo-testuvx pre-commit runSummary by CodeRabbit
New Features
Tests
Bug Fixes
Documentation