phd: add smoke test for VCR replacement#872
Merged
Merged
Conversation
hawkw
approved these changes
Feb 24, 2025
hawkw
left a comment
hawkw
left a comment
Member
There was a problem hiding this comment.
this is pretty straightforward! i had a couple teensy nitpicks.
iximeow
reviewed
Feb 25, 2025
iximeow
left a comment
iximeow
left a comment
Member
There was a problem hiding this comment.
+1 to Eliza's comments, plus maybe bonus wrinkle in the new test?
Contributor
Author
|
Thanks for the reviews! I'll rebase and merge this later today; should have a PR with a fix for #841 up shortly after. |
Add a smoke test for Crucible VCR replacement:
- Add a `CrucibleDisk` function to get a disk's current VCR.
- Add a `TestVm` framework to send a VCR replacement request.
- Fix a couple of bugs in Crucible disk setup:
- The `id` field in the VCR's `CrucibleOpts` needs to be the same in
the old and new VCRs, so use the disk ID for it instead of
calling `Uuid::new_v4()` every time the VCR is generated.
- When using a `Blank` disk source, properly account for the fact
that the disk source size is given in bytes, not gibibytes.
Also add a couple of bells and whistles to allow this test to be
transformed into a test of VCR replacement during VM startup:
- Make PHD's `VmSpec` type a public type and amend `Framework` to allow
tests to create a VM from a spec. This gives tests a way to access
a config's Crucible disks before actually launching a VM (and sending
an instance spec to Propolis).
- Reorganize the `CrucibleDisk` types to wrap the disk innards in a
`Mutex`, allowing them to be mutated through the `Arc` references that
tests get. This will eventually be used to allow tests to override the
downstairs addresses in a disk's VCRs before launching a VM that uses
that disk, which will be used to test #841. In the meantime, use the
mutex to protect the VCR generation number, which no longer needs to
be an `AtomicU64`.
gjcolombo
force-pushed
the
gjcolombo/phd-vcr-smoke-test
branch
from
1f04b2f to
ca88843
Compare
leftwo
approved these changes
Feb 26, 2025
| read_only: false, | ||
| }, | ||
| gen, | ||
| r#gen: self.generation, |
Contributor
There was a problem hiding this comment.
This is what I wanted to see!
|
|
||
| // Create a blank data disk on which to perform VCR replacement. This is | ||
| // necessary because Crucible doesn't permit VCR replacements for volumes | ||
| // whose read-only parents are local files (which is true for artifact-based |
Contributor
There was a problem hiding this comment.
We should update that. Crucible does support replacement of read only parents now, so perhaps we need to update the file based backend?
Though, testing on a data disk is also needed, so not like this is bad or wrong.
Contributor
Author
There was a problem hiding this comment.
We're tracking this with oxidecomputer/crucible#1658.
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
4 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.
Add a smoke test for Crucible VCR replacement:
CrucibleDiskfunction to get a disk's current VCR.TestVmframework to send a VCR replacement request.idfield in the VCR'sCrucibleOptsneeds to be the same in the old and new VCRs, so use the disk ID for it instead of callingUuid::new_v4()every time the VCR is generated.Blankdisk source, properly account for the fact that the disk source size is given in bytes, not gibibytes.Also add a couple of bells and whistles to allow this test to be transformed into a test of VCR replacement during VM startup:
VmSpectype a public type and amendFrameworkto allow tests to create a VM from a spec. This gives tests a way to access a config's Crucible disks before actually launching a VM (and sending an instance spec to Propolis).CrucibleDisktypes to wrap the disk innards in aMutex, allowing them to be mutated through theArcreferences that tests get. This will eventually be used to allow tests to override the downstairs addresses in a disk's VCRs before launching a VM that uses that disk, which will be used to test fixes for want Crucible VCR replacement to be able to proceed before a VM has fully started #841. In the meantime, use the mutex to protect the VCR generation number, which no longer needs to be anAtomicU64.