-
Notifications
You must be signed in to change notification settings - Fork 252
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
Add LCOW rootfs as pmem/vhd as option; Config for no scsi and number of VPMem #240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments are more about the future so you can just file work items if you agree. Great start
functional/lcow_test.go
Outdated
OperatingSystem: "linux", | ||
ID: "uvm", | ||
VPMemDeviceCount: 1, | ||
NoSCSI: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we expose it this way? When we support multiple controllers a single boolean wont suffice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will update to *int where nil means 1 by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I like that
functional/lcow_test.go
Outdated
t.Fatal(err) | ||
} | ||
defer lcowUVM.Terminate() | ||
out, err := exec.Command(`hcsdiag`, `exec`, `-uvm`, lcowUVM.ID(), `dmesg`).Output() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know this infra very well yet. Why are we using hcsdiag and not use the HCS CreateProcess call ourselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a temporary hack. I'll add a TODO here.
} | ||
|
||
fmt.Printf("- Creating %s...\n", destFile) | ||
if _, err := exec.Command(`powershell`, `-command`, fmt.Sprintf(`New-VHD %s -SizeBytes %dMB -fixed`, destFile, c.Int("s"))).Output(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wont work on all machines. Should we do this via the storage api's instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, temporary hack. I'll add a TODO here. Ideally I should extend go-winio to be able to create fixed disks under 1GB.
internal/uvm/create.go
Outdated
type PreferredRootFSType int | ||
|
||
const ( | ||
PreferredRootFSTypeDefault = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like code like this. Default means you have to have some IF that transforms the code into initrd or vhd based on some case (by default). Why not just start InitRd = 0 so it is the default and if you want VHD you have to specify it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's updated now.
internal/uvm/create.go
Outdated
EnableGraphicsConsole bool // If true, enable a graphics console for the utility VM | ||
ConsolePipe string // The named pipe path to use for the serial console. eg \\.\pipe\vmpipe | ||
VPMemDeviceCount int32 // Number of VPMem devices. Limit at 128. If booting UVM from VHD, device 0 is taken. | ||
NoSCSI bool // The utility VM does not have any SCSI controllers. Useful in some service VM scenarios. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as my comment above. How about SCSICount: 0, 1, 2 etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's updated now.
internal/uvm/create.go
Outdated
hcsDocument.VirtualMachine.Devices.VPMem.Devices = make(map[string]schema2.VirtualMachinesResourcesStorageVpmemDeviceV2) | ||
imageFormat := "VHD1" | ||
if strings.ToLower(filepath.Ext(opts.RootFSFile)) == "vhdx" { | ||
imageFormat = "VHD2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema doesnt look like it even supports VHD2 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. It's Default
😕
Signed-off-by: John Howard <jhoward@microsoft.com>
b93bdea
to
d3cc9bb
Compare
Signed-off-by: John Howard <jhoward@microsoft.com>
d3cc9bb
to
7addede
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Some of the tests are commented out I assume you will later turn them on but seems ok for now.
Signed-off-by: John Howard jhoward@microsoft.com
Fixes #205. @jstarks @jterry75
build.ps1
in the Microsoft/opengcs repo into a rootfs.vhd file.createuvm.go