Port virtualbox scripts to VBoxManage CLI #625
Conversation
There was a problem hiding this comment.
Thanks for the work @stevemk14ebr! I need to still test the code locally, but I have added some questions and improvement suggestions already. It is good to see what we can do with VBoxManage and how it allows us to remove the virtualbox dependency. The disadvantage is that it is less flexible, as it does not export everything in the API (for example, it seems it is not possible to access the max number of adapters which would allow us to write simpler code as in the previous version) and that we need to create a subprocess everytime we want to run a command. The new code using VBoxManage also looks longer and more complicated, but we may be able to simplify it a bit.
What about keeping both the version using the virtualbox library and the new one using VBoxManage until we have tested and migrated everything else?
Also, I think we need some documentation in /virtualbox/README.md.
| except subprocess.CalledProcessError as e: | ||
| # exit code is an error | ||
| print(f"Error running VBoxManage command: {e} ({e.stderr})") | ||
| raise Exception(f"Error running VBoxManage command") |
There was a problem hiding this comment.
why is it needed to catch the exception to print and error and re-reise it? I see the same pattern in other functions as well.
There was a problem hiding this comment.
Style choice, this throws a pretty error to the top level main to print out. I can change if you think there's a more pythonic style
There was a problem hiding this comment.
I have seen in other Python code that an exception is re-triggered to add extra details or format the exception differently, but without a print that duplicates similar information. The print apart from duplicating the information, can make the output difficult to digest in this case, as {e.stderr} is rendering the output that is likely to be the long help message from VBoxManage. I think we should remove the try-catch, as the re-triggered exception is almost the same:
- Original exception:
Command '['VBoxManage', 'list2', 'list', 'hostonlyifs']' returned non-zero exit status 2 - Retriggered exception:
Error running VBoxManage command: Command '['VBoxManage', 'list2', 'list', 'hostonlyifs']' returned non-zero exit status 2.
| except subprocess.CalledProcessError as e: | |
| # exit code is an error | |
| print(f"Error running VBoxManage command: {e} ({e.stderr})") | |
| raise Exception(f"Error running VBoxManage command") |
There was a problem hiding this comment.
Also, if we re-raise the exception, I think we should use a more concrete typ of exception like RuntimeError.
| vm_uuid, | ||
| "--ovf10", # Maybe change to ovf20 | ||
| f"--output={filename}", | ||
| "--vsys=0", # we have normal vms with only 1 vsys |
There was a problem hiding this comment.
[nitpick] I needed to check the documentation to understand what this is doing, I think we can improve the comment to clarify why this parameter is needed:
| "--vsys=0", # we have normal vms with only 1 vsys | |
| "--vsys=0", # We need to specify the index of the VM, 0 as we only export 1 VM |
There was a problem hiding this comment.
This took me a bit to figure out actually, it is a necessary parameter, but appears to be almost never used by anyone. There exists a concept of multiple virtual systems in a single VM. We don't ever use this, and a normal VM shouldn't have more than 1 virtual system, but it's a necessary parameter so I have had to include it.
There was a problem hiding this comment.
You can export several VMs in the same appliance. I was just purposing to add more details to the comment to clarify it. 😉
There was a problem hiding this comment.
I may need you to educate me honestly, I don't know more than what I commented about vsys
There was a problem hiding this comment.
I think it is not clear what I meant. My suggestion is just to change your comment by We need to specify the index of the VM, 0 as we only export 1 VM:
| "--vsys=0", # we have normal vms with only 1 vsys | |
| "--vsys=0", # We need to specify the index of the VM, 0 as we only export 1 VM |
| session.unlock_machine() | ||
| print(f"Restored '{snapshot_name}' and changed its adapter(s) to host-only") | ||
|
|
||
| vm_uuid = get_vm_uuid(VM_NAME) |
There was a problem hiding this comment.
do we need to get the UUID? It seems like the commands work with the VM_NAME (we may need to enclose the entire name in double quotes to avoid issues with spaces), or am I missing something?
There was a problem hiding this comment.
We could rely on the VM_NAME alone, but I use the UUID so that we can support multiple VMs of the same name and be sure we refer to the same VM consistently for all operations
we can, the vminfo command lists all 8 adapters (the max) and any unset adapters have the value 'none'. The code doesn't need to check the max adapters because it lists all of them, even if they're unset, so we always loop all 8 adapters.
I have no issues with not merging these PRs (I will send more for the other two scripts) until we are ready to drop the |
stevemk14ebr
changed the title
There was a problem hiding this comment.
I have tested vbox-export-snapshots.py and I it fails because the network interface does not have a name. I though this happened in the previous version when exporting the VM, but it seems like setting it could be the issue and because you start the VM (what I was not doing in the previous version) it fails even before exporting it:
Starting operations on FLARE-VM
VM {40138663-f254-412b-8776-10a7cc08daea} is already shut down (state: poweroff).
Restored 'FLARE-VM'
VM {40138663-f254-412b-8776-10a7cc08daea} is already shut down (state: poweroff).
Found existing hostonlyif vboxnet0
Changed nic1
Nic configuration verified correct
Power cycling before export...
VM {40138663-f254-412b-8776-10a7cc08daea} is not running (state: poweroff). Starting VM...
Error running VBoxManage command: Command '['VBoxManage', 'startvm', '{40138663-f254-412b-8776-10a7cc08daea}', '--type', 'gui']' returned non-zero exit status 1. (VBoxManage: error: Nonexistent host networking interface, name '' (VERR_INTERNAL_ERROR)
VBoxManage: error: Details: code NS_ERROR_FAILURE (0x80004005), component ConsoleWrap, interface IConsole
)
Error checking VM state: Error running VBoxManage command
Unexpectedly failed doing operations on FLARE-VM. Exiting...
Done. Exiting...
I reported what I think was a bug in https://www.virtualbox.org/ticket/22158. But what really confuses me is that it seems it does work for you. 😕
|
|
||
| try: | ||
| hostonly_ifname = ensure_hostonlyif_exists() | ||
| dynamic_machine_guids = get_dynamic_vm_uuids() |
There was a problem hiding this comment.
The original version was checking the status of all VMs, but only modified the ones with .dynamic in the name. This version checks only the .dynamic VMs. I think it is useful to be able to display the status of all of them (this could be under an argument).
| if get_vm_state(machine_guid) == "poweroff": | ||
| run_vboxmanage(["modifyvm", machine_guid, f"--{nic}", DISABLED_ADAPTER_TYPE]) | ||
| print(f"Set VM {nic} to hostonly") | ||
| else: | ||
| run_vboxmanage(["controlvm", machine_guid, nic, "hostonly", hostonly_ifname]) | ||
| print(f"Set VM {nic} to hostonly") |
There was a problem hiding this comment.
We can remove the duplicated print, allowing us to keep the message consist in all cases. I also think we should include the VM name in the message as at the moment I find it difficult to understand in the output:
| if get_vm_state(machine_guid) == "poweroff": | |
| run_vboxmanage(["modifyvm", machine_guid, f"--{nic}", DISABLED_ADAPTER_TYPE]) | |
| print(f"Set VM {nic} to hostonly") | |
| else: | |
| run_vboxmanage(["controlvm", machine_guid, nic, "hostonly", hostonly_ifname]) | |
| print(f"Set VM {nic} to hostonly") | |
| if get_vm_state(machine_guid) == "poweroff": | |
| run_vboxmanage(["modifyvm", machine_guid, f"--{nic}", DISABLED_ADAPTER_TYPE]) | |
| else: | |
| run_vboxmanage(["controlvm", machine_guid, nic, "hostonly", hostonly_ifname]) | |
| print(f"Set VM {vm_name} adaper {nic} to hostonly") |
| vminfo = run_vboxmanage(["showvminfo", machine_guid, "--machinereadable"]) | ||
| for nic_number, nic_value in re.findall("^nic(\d+)=\"(\S+)\"", vminfo, flags=re.M): | ||
| if nic_value not in ALLOWED_ADAPTER_TYPES: | ||
| nics_with_internet.append(f"nic{nic_number}") |
There was a problem hiding this comment.
I think it is clear for the user to use the adapter number without prepending nic:
| nics_with_internet.append(f"nic{nic_number}") | |
| nics_with_internet.append(nic_number) |
|
|
||
| # modify the invalid adapters if allowed | ||
| if nics_with_internet: | ||
| for nic in nics_with_internet: |
There was a problem hiding this comment.
This displays a notification per enabled adapter, which is IMO confusing when several adapters are enabled. The previous version was concatenating them and displaying a single notification, what is also the idea behind saving the adapters with internet into a list.
| for line in hostonlyifs_output.splitlines(): | ||
| if line.startswith("Name:"): | ||
| hostonlyif_name = line.split(":")[1].strip() | ||
| print(f"Found existing hostonlyif {hostonlyif_name}") |
There was a problem hiding this comment.
I would remove this message, I think it is confusing for the user.
| print(f"Found existing hostonlyif {hostonlyif_name}") |
| hostonlyifs_output = run_vboxmanage(["list", "hostonlyifs"]) # Get the updated list | ||
| for line in hostonlyifs_output.splitlines(): | ||
| if line.startswith("Name:"): | ||
| hostonlyif_name = line.split(":")[1].strip() | ||
| print(f"Created hostonlyif {hostonlyif_name}") | ||
| return |
There was a problem hiding this comment.
this code is repeated twice, please move it to a function to remove the code duplication
| except subprocess.CalledProcessError as e: | ||
| # exit code is an error | ||
| print(f"Error running VBoxManage command: {e} ({e.stderr})") | ||
| raise Exception(f"Error running VBoxManage command") |
There was a problem hiding this comment.
I have seen in other Python code that an exception is re-triggered to add extra details or format the exception differently, but without a print that duplicates similar information. The print apart from duplicating the information, can make the output difficult to digest in this case, as {e.stderr} is rendering the output that is likely to be the long help message from VBoxManage. I think we should remove the try-catch, as the re-triggered exception is almost the same:
- Original exception:
Command '['VBoxManage', 'list2', 'list', 'hostonlyifs']' returned non-zero exit status 2 - Retriggered exception:
Error running VBoxManage command: Command '['VBoxManage', 'list2', 'list', 'hostonlyifs']' returned non-zero exit status 2.
| except subprocess.CalledProcessError as e: | |
| # exit code is an error | |
| print(f"Error running VBoxManage command: {e} ({e.stderr})") | |
| raise Exception(f"Error running VBoxManage command") |
| except subprocess.CalledProcessError as e: | ||
| # exit code is an error | ||
| print(f"Error running VBoxManage command: {e} ({e.stderr})") | ||
| raise Exception(f"Error running VBoxManage command") |
There was a problem hiding this comment.
Also, if we re-raise the exception, I think we should use a more concrete typ of exception like RuntimeError.
| return uuid | ||
| else: | ||
| raise Exception(f"Could not find VM '{vm_name}'") | ||
| except Exception as e: |
There was a problem hiding this comment.
I am not sure if it is a good idea to catch all types of exceptions or we should be more specific and use something like RuntimeError. 🤔
| vm.save_settings() | ||
|
|
||
| # cmd is an array of string arguments to pass | ||
| def run_vboxmanage(cmd): |
There was a problem hiding this comment.
We should extract all common functions (the ones used in several scripts) into a file we can re-use to avoid duplication.
Ports to VBoxManage CLI, identical logic otherwise. Errors handled gracefully for the most part. Output: