-
Notifications
You must be signed in to change notification settings - Fork 454
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
feat: add usb controller #2280
base: main
Are you sure you want to change the base?
feat: add usb controller #2280
Conversation
1420065
to
2ca7685
Compare
647a6a0
to
6feba9c
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.
Left some thoughts on supporting only a single USB controller for each supported version based on the configuration maximums in v7 and v8.
86290f9
to
95782f5
Compare
95782f5
to
2be836d
Compare
2be836d
to
2156d69
Compare
2156d69
to
911cae3
Compare
911cae3
to
db44d5d
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.
Minor nits, but otherwise looks good.
db44d5d
to
c687081
Compare
c687081
to
6c72e2c
Compare
Add the ability to add usb controller on build and modification of virtual machine. Signed-off-by: Jared Burns <[email protected]>
6c72e2c
to
7209433
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!
Ready for @spacegospod and @iBrandyJackson to review.
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.
Please remove the redundant power cycle.
Everything else looks good to me
keyCounter-- | ||
} | ||
|
||
// Power on the virtual machine after adding the USB controller devices. |
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.
Don't do this, the machine gets reconfigured at the end of the function.
} | ||
|
||
if device != nil { | ||
// Power off the virtual machine before removing the USB controller. |
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.
Don't do this. The machine gets reconfigured at the end of the function.
You are appending device changes but you are not actually reconfiguring the machine in your diff, doing a power cycle achieves nothing
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 tested without and it does not power off with the existing code. So not sure what is broke but this does not work without the code being added.
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 think that this might be needed for now @spacegospod due to some existing issues in the provider. I would recommend leaving this in (for now) with a // TODO
to revisit the concern and reference a tracking PR to review the reasons power on/off might not work currently as expected.
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 tested without and it does not power off with the existing code. So not sure what is broke but this does not work without the code being added.
This is a snippet with the conditions for the existing power off routine
if changed || len(spec.DeviceChange) > 0 {
// Check to see if we need to shutdown the VM for this process.
if d.Get("reboot_required").(bool) && vprops.Runtime.PowerState != types.VirtualMachinePowerStatePoweredOff {
I'm sure your code passes the first statement since you're adding to spec.DeviceChange
,
The likely culprit is reboot_required
not being set. If a reboot of the VM is indeed necessary for a USB controller change you should probably mark the new settings as "required with" reboot_required. This way a user would have to make a
conscious decision to do a power cycle of their workload.
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.
@burnsjared0415 give this a shot:
if d.HasChange("usb_controller") {
old, new := d.GetChange("usb_controller")
oldUSB := old.([]interface{})
newUSB := new.([]interface{})
keyCounter := -100
usb2ControllerPresent := false
usb3ControllerPresent := false
// Map to store existing USB controllers and their keys.
existingUSBControllers := make(map[string]int32)
// Check for existing USB controllers and store their keys.
for _, dev := range vprops.Config.Hardware.Device {
switch controller := dev.(type) {
case *types.VirtualUSBController:
usb2ControllerPresent = true
existingUSBControllers["2.0"] = controller.Key
case *types.VirtualUSBXHCIController:
usb3ControllerPresent = true
existingUSBControllers["3.x"] = controller.Key
}
}
rebootRequired := false
// Remove USB controllers that are no longer in the configuration.
for _, oldUSBControllerInterface := range oldUSB {
oldUSBController := oldUSBControllerInterface.(map[string]interface{})
oldUSBVersion := oldUSBController["version"].(string)
found := false
for _, newUSBControllerInterface := range newUSB {
newUSBController := newUSBControllerInterface.(map[string]interface{})
newUSBVersion := newUSBController["version"].(string)
if oldUSBVersion == newUSBVersion {
found = true
break
}
}
if !found {
key, exists := existingUSBControllers[oldUSBVersion]
if exists {
var device types.BaseVirtualDevice
switch oldUSBVersion {
case "2.0":
device = &types.VirtualUSBController{
VirtualController: types.VirtualController{
VirtualDevice: types.VirtualDevice{
Key: key,
},
},
}
case "3.1", "3.2":
device = &types.VirtualUSBXHCIController{
VirtualController: types.VirtualController{
VirtualDevice: types.VirtualDevice{
Key: key,
},
},
}
}
if device != nil {
spec.DeviceChange = append(spec.DeviceChange, &types.VirtualDeviceConfigSpec{
Operation: types.VirtualDeviceConfigSpecOperationRemove,
Device: device,
})
log.Printf("[INFO] Removed USB controller: %s", oldUSBVersion)
rebootRequired = true
}
}
}
}
// Add new USB controller devices.
for _, usbControllerInterface := range newUSB {
usbController := usbControllerInterface.(map[string]interface{})
usbVersion := usbController["version"].(string)
var device types.BaseVirtualDevice
switch usbVersion {
case "2.0":
if !usb2ControllerPresent {
usb2ControllerPresent = true
enabled := true // directly create a boolean variable
device = &types.VirtualUSBController{
VirtualController: types.VirtualController{
VirtualDevice: types.VirtualDevice{
Key: int32(keyCounter),
},
},
EhciEnabled: &enabled, // assign address directly
}
rebootRequired = true
}
case "3.1", "3.2":
if !usb3ControllerPresent {
usb3ControllerPresent = true
device = &types.VirtualUSBXHCIController{
VirtualController: types.VirtualController{
VirtualDevice: types.VirtualDevice{
Key: int32(keyCounter),
},
},
}
rebootRequired = true
}
}
if device != nil {
spec.DeviceChange = append(spec.DeviceChange, &types.VirtualDeviceConfigSpec{
Operation: types.VirtualDeviceConfigSpecOperationAdd,
Device: device,
})
log.Printf("[INFO] Added USB controller: %s", usbVersion)
keyCounter--
}
}
if rebootRequired {
log.Printf("[INFO] USB controller changes require a virtual machine reboot.")
_ = d.Set("reboot_required", 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.
See #2280 (comment) with possible simplification for the required reboot.
Description
Add the ability to add usb controller on build and modification of virtual machine.
Acceptance tests
Output from acceptance testing:
Release Note
Release note for CHANGELOG:
References
Closes #620