Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHD: add guest adapter for WS2022 #607

Merged
merged 4 commits into from
Jan 17, 2024
Merged

Conversation

gjcolombo
Copy link
Contributor

The adapter assumes that

  • sysprep has been run such that the guest OS is generalized and, at first boot, its hostname will be set to "PHD-WINDOWS"
  • Cygwin is installed to C:\cygwin
  • the local administrator account is enabled with the appropriate password

A suitable image is available on catacomb. This image passes nproc_test, but currently fails several other tests in ways that appear to be unrelated to the correctness of the adapter:

  • The Cygwin version in the test image doesn't include a pciutils package in-box, so the lspci lifecycle test doesn't work as expected. There are Windows ports of pciutils available that could resolve this, but a more flexible approach (which I will probably take in a future PR) is to allow tests to ask the framework what "flavor" of user space the current guest provides so that they can tailor their commands accordingly.
  • The Crucible tests unconditionally specify that the guest disk size should be 10 GiB, which is smaller than the guest image, which (unsurprisingly) leads to a very unhappy guest. This is a general problem with the way Crucible disk sizes work and will be fixed in a separate PR.

Tests: local test run of the nproc smoke test with a WS2022 image.

Related to #601.

The adapter assumes that

- sysprep has been run such that the guest OS is generalized and, at first boot,
  its hostname will be set to "PHD-WINDOWS"
- Cygwin is installed to C:\cygwin
- the local administrator account is enabled with the appropriate password

A suitable image is available on catacomb. This image passes `nproc_test`, but
currently fails several other tests in ways that appear to be unrelated to the
correctness of the adapter:

- The Cygwin version in the test image doesn't include a `pciutils` package
  in-box, so the `lspci` lifecycle test doesn't work as expected. There are
  Windows ports of `pciutils` available that could resolve this, but a more
  flexible approach (which I will probably take in a future PR) is to allow
  tests to ask the framework what "flavor" of user space the current guest
  provides so that they can tailor their commands accordingly.
- The Crucible tests unconditionally specify that the guest disk size should be
  10 GiB, which is smaller than the guest image, which (unsurprisingly) leads to
  a very unhappy guest. This is a general problem with the way Crucible disk
  sizes work and will be fixed in a separate PR.

Tests: local test run of the `nproc` smoke test with a WS2022 image.
@gjcolombo gjcolombo requested a review from hawkw January 16, 2024 21:19
CommandSequenceEntry::WaitFor("Domain :"),
CommandSequenceEntry::WriteStr(""),
CommandSequenceEntry::WaitFor("Password:"),
CommandSequenceEntry::WriteStr("0xide#1Fan"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to be encoding more expectations about image login credentials, it might make sense to have some top-level markdown doc listing them so that folks don't need to go fishing in the source for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, done in 40aed6a.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a couple questions and some very minor nitpicks, but this looks good to me overall!

impl GuestOs for WindowsServer2022 {
fn get_login_sequence(&self) -> CommandSequence {
CommandSequence(vec![
// The image is generalized and needs to be specialized. Let it boot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity: what does it mean for the image to be "generalized"/"specialized", exactly? and why does booting the image twice do what we need?

phd-tests/framework/src/test_vm/mod.rs Outdated Show resolved Hide resolved
@hawkw hawkw added the testing Related to testing and/or the PHD test framework. label Jan 16, 2024
Comment on lines +15 to +20
/// - The image has been generalized (by running `sysprep /generalize`) and is
/// configured so that on first boot it will skip the out-of-box experience
/// (OOBE) and initialize the local administrator account with the appropriate
/// password. See [MSDN's Windows Setup
/// documentation](https://learn.microsoft.com/en-us/windows-hardware/manufacture/desktop/generalize?view=windows-11)
/// for more details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is lovely, thank you for expanding on it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My pleasure! One additional detail here (I mentioned this in DMs, but adding here as well for posterity) is that the (WIP) Windows image builder currently generates generalized images (so that we don't have to worry about any specialization conflicts if they're used as the read-only base images for multiple VMs), and I leveraged that here to create the test image I used to test this adapter. It might be possible to create an image that's technically been specialized already (so that Setup doesn't have to run and we don't have to wait for this reboot) but can still be reused across multiple VMs, but I'd have to do some research to figure out how.

@gjcolombo gjcolombo merged commit 6e41165 into master Jan 17, 2024
10 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/phd/ws2022-adapter branch January 17, 2024 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Related to testing and/or the PHD test framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants