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

Port virtualbox scripts to VBoxManage CLI #625

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
233 changes: 197 additions & 36 deletions virtualbox/vbox-export-snapshots.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@

import os
import hashlib
import virtualbox
from virtualbox.library import VirtualSystemDescriptionType as DescType
from virtualbox.library import NetworkAttachmentType as NetType
from virtualbox.library import ExportOptions as ExportOps
import re
import subprocess
from datetime import datetime
import time

# Base name of the exported VMs
EXPORTED_VM_NAME = "FLARE-VM"

# Name of the VM to export the snapshots from
VM_NAME = f"{EXPORTED_VM_NAME}.testing"

Expand All @@ -32,56 +32,217 @@
("FLARE-VM.EDU", ".EDU", "Windows 10 VM with FLARE-VM default configuration installed + FLARE-EDU teaching materials"),
]


def sha256_file(filename):
with open(filename, "rb") as f:
return hashlib.file_digest(f, "sha256").hexdigest()


def change_network_adapters(vm, max_adapters):
for i in range(max_adapters):
adapter = vm.get_network_adapter(i)
adapter.attachment_type = NetType.host_only
vm.save_settings()

# cmd is an array of string arguments to pass
def run_vboxmanage(cmd):
Copy link
Member

Choose a reason for hiding this comment

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

We should extract all common functions (the ones used in several scripts) into a file we can re-use to avoid duplication.

"""Runs a VBoxManage command and returns the output."""
try:
result = subprocess.run(["VBoxManage"] + cmd, capture_output=True, text=True, check=True)
return result.stdout
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")
Comment on lines +45 to +48
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.
Suggested change
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")

Copy link
Member

Choose a reason for hiding this comment

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

Also, if we re-raise the exception, I think we should use a more concrete typ of exception like RuntimeError.


def get_vm_uuid(vm_name):
"""Gets the machine UUID for a given VM name using 'VBoxManage list vms'."""
try:
vms_output = run_vboxmanage(["list", "vms"])
# regex VM name and extract the GUID
match = re.search(rf'"{vm_name}" \{{(.*?)\}}', vms_output)
if match:
uuid = "{" + match.group(1) + "}"
return uuid
else:
raise Exception(f"Could not find VM '{vm_name}'")
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

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

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. πŸ€”

print(f"Error getting machine UUID: {e}")
raise Exception(f"Could not find VM '{vm_name}'")

def get_vm_state(machine_guid):
"""Gets the VM state using 'VBoxManage showvminfo'."""
vminfo = run_vboxmanage(["showvminfo", machine_guid, "--machinereadable"])
for line in vminfo.splitlines():
if line.startswith("VMState"):
return line.split("=")[1].strip('"')
raise Exception(f"Could not start VM '{machine_guid}'")

def ensure_vm_running(machine_guid):
"""Checks if the VM is running and starts it if it's not.
Waits up to 1 minute for the VM to transition to the 'running' state.
"""
try:
vm_state = get_vm_state(machine_guid)
if vm_state != "running":
print(f"VM {machine_guid} is not running (state: {vm_state}). Starting VM...")
run_vboxmanage(["startvm", machine_guid, "--type", "gui"])

# Wait for VM to start (up to 1 minute)
timeout = 60 # seconds
check_interval = 5 # seconds
start_time = time.time()
while time.time() - start_time < timeout:
vm_state = get_vm_state(machine_guid)
if vm_state == "running":
print(f"VM {machine_guid} started.")
time.sleep(5) # wait a bit to be careful and avoid any weird races
return
print(f"Waiting for VM (state: {vm_state})")
time.sleep(check_interval)
print("Timeout waiting for VM to start. Exiting...")
raise TimeoutError(f"VM did not start within the timeout period {timeout}s.")
else:
print("VM is already running.")
return
except Exception as e:
print(f"Error checking VM state: {e}")
raise Exception(f"Could not ensure '{machine_guid}' running")

def ensure_vm_shutdown(machine_guid):
"""Checks if the VM is running and shuts it down if it is."""
try:
vm_state = get_vm_state(machine_guid)
if vm_state != "poweroff":
print(f"VM {machine_guid} is not powered off. Shutting down VM...")
run_vboxmanage(["controlvm", machine_guid, "poweroff"])

# Wait for VM to shut down (up to 1 minute)
timeout = 60 # seconds
check_interval = 5 # seconds
start_time = time.time()
while time.time() - start_time < timeout:
vm_state = get_vm_state(machine_guid)
if vm_state == "poweroff":
print(f"VM {machine_guid} is shut down (status: {vm_state}).")
time.sleep(5) # wait a bit to be careful and avoid any weird races
return
time.sleep(check_interval)
print("Timeout waiting for VM to shut down. Exiting...")
raise TimeoutError("VM did not shut down within the timeout period.")
else:
print(f"VM {machine_guid} is already shut down (state: {vm_state}).")
return
except Exception as e:
print(f"Error checking VM state: {e}")
raise Exception(f"Could not ensure '{machine_guid}' shutdown")

def ensure_hostonlyif_exists():
"""Gets the name of, or creates a new hostonlyif"""
try:
# Find existing hostonlyif
hostonlyifs_output = run_vboxmanage(["list", "hostonlyifs"])
for line in hostonlyifs_output.splitlines():
if line.startswith("Name:"):
hostonlyif_name = line.split(":")[1].strip()
print(f"Found existing hostonlyif {hostonlyif_name}")
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this message, I think it is confusing for the user.

Suggested change
print(f"Found existing hostonlyif {hostonlyif_name}")

return

# No host-only interface found, create one
print("No host-only interface found. Creating one...")
run_vboxmanage(["hostonlyif", "create"]) # Create a host-only interface
Copy link
Member

Choose a reason for hiding this comment

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

[nitpick] I think the comment here is not needed, there is a similar comment above and the command is self explanatory:

Suggested change
run_vboxmanage(["hostonlyif", "create"]) # Create a host-only interface
run_vboxmanage(["hostonlyif", "create"])

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
Comment on lines +146 to +151
Copy link
Member

Choose a reason for hiding this comment

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

this code is repeated twice, please move it to a function to remove the code duplication

print("Failed to create new hostonlyif. Exiting...")
raise Exception("Failed to create new hostonlyif.")
except Exception as e:
print(f"Error getting host-only interface name: {e}")
raise Exception("Failed to verify host-only interface exists")
Comment on lines +132 to +156
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand why we need to check if there is a host-only interface. Does something fails if it does not exist? If something does fail, I would suggest to allow the error to reach the user to avoid introducing an extra function. I think creating the Host-Only interface does not need to be part of this script.

Copy link
Author

Choose a reason for hiding this comment

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

The command will silently do nothing if there is no host only adapter. It would be caught by the check I added in the change adapter function, but I chose to ensure the VM state is as we expect. If I did not do this there would need to be a manual step to ensure a host only adapter exists which seemed not necessary, why not automate it.


def change_network_adapters_to_hostonly(machine_guid):
"""Changes all active network adapters to Host-Only. Must be poweredoff"""
ensure_hostonlyif_exists()
try:
foundOne = False
# change any existing enabled nic to hostonly
vminfo = run_vboxmanage(["showvminfo", machine_guid, "--machinereadable"])
for line in vminfo.splitlines():
# Match lines exactly in the format 'nicN="value"'
match = re.match(r"nic(\d+)=\"(.*?)\"", line)
if match:
nic_number = match.group(1)
nic_value = match.group(2)
stevemk14ebr marked this conversation as resolved.
Show resolved Hide resolved
if nic_value != "none": # Ignore NICs with value "none"
run_vboxmanage(["modifyvm", machine_guid, f"--nic{nic_number}", "hostonly"])
stevemk14ebr marked this conversation as resolved.
Show resolved Hide resolved
print(f"Changed nic{nic_number} to hostonly")
foundOne = True

# If no nic was enabled / configured, set the first to hostonly
if not foundOne:
run_vboxmanage(["modifyvm", machine_guid, f"--nic1", "hostonly"])

# ensure changes applied
vminfo = run_vboxmanage(["showvminfo", machine_guid, "--machinereadable"])
for line in vminfo.splitlines():
# Match lines exactly in the format 'nicN="value"'
match = re.match(r"nic(\d+)=\"(.*?)\"", line)
if match:
nic_number = match.group(1)
nic_value = match.group(2)
if nic_value == "hostonly":
print("Verified hostonly nic configuration correct")
return
stevemk14ebr marked this conversation as resolved.
Show resolved Hide resolved
except Exception as e:
print(f"Error changing network adapters: {e}")
print("Failed to change VM network adapters to hostonly")
raise Exception("Failed to change VM network adapters to hostonly")

if __name__ == "__main__":
date = datetime.today().strftime("%Y%m%d")

vbox = virtualbox.VirtualBox()
vm = vbox.find_machine(VM_NAME)
max_adapters = vbox.system_properties.get_max_network_adapters(vm.chipset_type)

for snapshot_name, extension, description in SNAPSHOTS:
print(f"Starting operations on {snapshot_name}")
try:
# Restore snapshot
session = vm.create_session()
snapshot = session.machine.find_snapshot(snapshot_name)
progress = session.machine.restore_snapshot(snapshot)
progress.wait_for_completion(-1)
change_network_adapters(session.machine, max_adapters)
session.unlock_machine()
print(f"Restored '{snapshot_name}' and changed its adapter(s) to host-only")

vm_uuid = get_vm_uuid(VM_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

# Shutdown machine
ensure_vm_shutdown(vm_uuid)

# Restore snapshot (must be shutdown)
run_vboxmanage(["snapshot", vm_uuid, "restore", snapshot_name])
print(f"Restored '{snapshot_name}'")
stevemk14ebr marked this conversation as resolved.
Show resolved Hide resolved

# change all adapters to hostonly (must be shutdown)
change_network_adapters_to_hostonly(vm_uuid)
stevemk14ebr marked this conversation as resolved.
Show resolved Hide resolved

# do a power cycle to ensure everything is good
print("Power cycling before export...")
ensure_vm_running(vm_uuid)
time.sleep(10)
stevemk14ebr marked this conversation as resolved.
Show resolved Hide resolved
ensure_vm_shutdown(vm_uuid)
print("Power cycling done.")

# Export .ova
exported_vm_name = f"{EXPORTED_VM_NAME}.{date}{extension}"
export_directory = os.path.expanduser(f"~/{EXPORT_DIR_NAME}")
os.makedirs(export_directory, exist_ok=True)
filename = os.path.join(export_directory, f"{exported_vm_name}.ova")
appliance = vbox.create_appliance()
sys_description = vm.export_to(appliance, exported_vm_name)
sys_description.set_final_value(DescType.name, exported_vm_name)
sys_description.set_final_value(DescType.description, description)
progress = appliance.write("ovf-1.0", [ExportOps.create_manifest], filename)

print(f"Exporting {filename} (this will take some time, go for an 🍦!)")
progress.wait_for_completion(-1)

run_vboxmanage(
[
"export",
vm_uuid,
"--ovf10", # Maybe change to ovf20
stevemk14ebr marked this conversation as resolved.
Show resolved Hide resolved
f"--output={filename}",
"--vsys=0", # we have normal vms with only 1 vsys
Copy link
Member

Choose a reason for hiding this comment

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

[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:

Suggested change
"--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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

You can export several VMs in the same appliance. I was just purposing to add more details to the comment to clarify it. πŸ˜‰

Copy link
Author

Choose a reason for hiding this comment

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

I may need you to educate me honestly, I don't know more than what I commented about vsys

Copy link
Member

Choose a reason for hiding this comment

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

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:

Suggested change
"--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

f"--vmname={exported_vm_name}",
f"--description={description}",
]
)

# Generate file with SHA256
with open(f"{filename}.sha256", "w") as f:
f.write(sha256_file(filename))

print(f"Exported {filename}! πŸŽ‰")

except Exception as e:
print(f"ERROR exporting {snapshot_name}: {e}")

print(f"Unexpectedly failed doing operations on {snapshot_name}. Exiting...")
break
print(f"All operations on {snapshot_name} successful βœ…")
print("Done. Exiting...")