petri: use Trace-CimMethodExecution in powershell commands #2149
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances PowerShell command error handling in the petri testing framework by adding WMI error tracing and improving robustness of VM cleanup operations. The changes focus on better error visibility and handling intermittent failures gracefully.
Key changes:
- Added
Trace-CimMethodExecutionto PowerShell CIM method calls for better error visibility - Made VM restart configuration non-fatal to handle intermittent state transition failures
- Renamed PowerShell function and variables for better consistency and clarity
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| petri/src/vm/hyperv/vm.rs | Modified wait_for_halt to ignore errors from run_set_turn_off_on_guest_restart with explanatory comment |
| petri/src/vm/hyperv/hyperv.psm1 | Added error tracing to CIM method calls and renamed function/variables for consistency |
petri/src/vm/hyperv/hyperv.psm1
Outdated
|
|
||
| $vmid = $Vm.Id | ||
| $msvm_ComputerSystem = Get-CimInstance -namespace $ROOT_HYPER_V_NAMESPACE -query "select * from Msvm_ComputerSystem where Name = '$vmid'" | ||
| $msvmComputerSystem = Get-CimInstance -namespace $ROOT_HYPER_V_NAMESPACE -query "select * from msvm_ComputerSystem where Name = '$vmid'" |
There was a problem hiding this comment.
nit: why change the case of the WMI class name here?
There was a problem hiding this comment.
Oops didn't mean to change the class name, just the variable name to standardize the naming convention.
| // If we aren't expecting a restart, tell the VM to turn off if the | ||
| // guest unexpectedly restarts. This command may fail if the VM is | ||
| // transitioning between states. In that case, the VM will be shut off | ||
| // and destroyed later if necessary. | ||
| _ = powershell::run_set_turn_off_on_guest_restart(&self.vmid, &self.ps_mod, !allow_reset) | ||
| .await; | ||
|
|
There was a problem hiding this comment.
nit: ideally, this would be a separate PR since it is a separate logical change.
There was a problem hiding this comment.
you may also want to consider adding some mechanism to write the output as warnings instead of errors in the event that this does fail. e.g. errors in the log that aren't failures can be confusing for folks not intimately familiar with looking at the petri logs
There was a problem hiding this comment.
This isn't really a separate logical change; before we were ignoring this error at the powershell layer, now we are ignoring it more explicitly in rust (along with not ignoring any other possible powershell failures). Today the output of these commands gets logged as debug (not failure or warning) whether it is an error or not. Maybe I should log it as warning always if the return code is not ok.
3 participants
This should help catch any wmi errors that would have otherwise been silent. Unfortunately, I had to ignore the output of one of the commands since it fails intermittently. However, it is ok since run_set_turn_off_on_guest_restart just helps kill rouge VMs faster, they should still get cleaned up eventually.