search

BOINC / boinc

Open-source software for volunteer computing and grid computing.
https://boinc.berkeley.edu
GNU Lesser General Public License v3.0
2k stars 445 forks source link

Vboxwrapper - handling of outdated vboxmanage command options #5587

Closed computezrmle closed 5 months ago

computezrmle commented 5 months ago

Certain Vbox tasks from LHC@home recently failed with log entries like this:

2024-04-13 16:06:31 (7532): Detected: VirtualBox VboxManage Interface (Version: Unknown)
.
.
.
2024-04-13 16:06:43 (7532): Error in add storage controller (fixed disk) for VM: -108
Command:
VBoxManage -q storagectl "boinc_2f92bf95dc688693" --name "Hard Disk Controller" --add "sata" --controller "IntelAHCI" --hostiocache off --sataportcount 3
Output:
Oracle VM VirtualBox Command Line Management Interface Version 7.0.8
Copyright (C) 2005-2023 Oracle and/or its affiliates

VBoxManage.exe: error: Unknown option: --sataportcount
Usage - Manage a storage controller:

  VBoxManage storagectl <uuid | vmname> <--name=controller-name> [--add= floppy
      | ide | pcie | sas | sata | scsi | usb ] [--controller= BusLogic | I82078
      | ICH6 | IntelAhci | LSILogic | LSILogicSAS | NVMe | PIIX3 | PIIX4 | USB
      | VirtIO ] [--bootable= on | off ] [--hostiocache= on | off ]
      [--portcount=count] [--remove] [--rename=new-controller-name]

2024-04-13 16:06:43 (7532): Could not create VM
2024-04-13 16:06:43 (7532): ERROR: VM failed to start

The root cause is that (for an unknown reason) the VirtualBox version could not be detected. Vboxwrapper then used --sataportcount which is set since is_virtual_box_version_newer() returns false as default. The log also shows that the VirtualBox version used on this computer is 7.0.8.

Suggestion To give a task a chance to run is_virtual_box_version_newer() should return true as default which would set recent vboxmanage command options as default.

AenBleidd commented 5 months ago

I don't like this solution simply because this breaks the whole function logic. Instead I'd investigate more why it was not possible to detect the VBox version. Otherwise this will break the logic, and would make vboxwrapper to act wrong in the case when it works perfectly fine right now. I'd suggest to add additional logging to the is_virtualbox_version_newer function to show the input string, to identify more accurately, why it was not possible to detect the correct VBox version

computezrmle commented 5 months ago

As mentioned, the reason why the check fails is unknown. The log is not from my own computer and the computer in question does not always fail the version check. Could be a broken VirtualBox installation, but that's without evidence.

Below please find a complete list where is_virtual_box_version_newer() is called. The first location is the only critical one as it causes the task to fail, but it wouldn't do any harm to give the task a chance.

All other locations also cause no harm if true is returned, especially since all checks (but 1) are against very old VirtualBox versions that are not in use any more for years.

As for more logging: Agreed, but it may be done in get_version_information() if the version details can't be identified.

https://github.com/BOINC/boinc/blob/627637d475e10a1a7fc1a4f408dd5c0bf0a6f9b8/samples/vboxwrapper/vbox_vboxmanage.cpp#L452-L458

https://github.com/BOINC/boinc/blob/627637d475e10a1a7fc1a4f408dd5c0bf0a6f9b8/samples/vboxwrapper/vbox_vboxmanage.cpp#L352-L360

https://github.com/BOINC/boinc/blob/627637d475e10a1a7fc1a4f408dd5c0bf0a6f9b8/samples/vboxwrapper/vbox_vboxmanage.cpp#L735-L738

https://github.com/BOINC/boinc/blob/627637d475e10a1a7fc1a4f408dd5c0bf0a6f9b8/samples/vboxwrapper/vbox_vboxmanage.cpp#L874-L877

https://github.com/BOINC/boinc/blob/627637d475e10a1a7fc1a4f408dd5c0bf0a6f9b8/samples/vboxwrapper/vbox_vboxmanage.cpp#L1456-L1458

https://github.com/BOINC/boinc/blob/627637d475e10a1a7fc1a4f408dd5c0bf0a6f9b8/samples/vboxwrapper/vbox_vboxmanage.cpp#L2140-L2143

AenBleidd commented 5 months ago

Even if now there is no harm to do the change you proposed, that doesn't mean that this won't make an issue in future. I suggest to do the next:

  1. Extend logging (get_version_information() indeed is the best place for that)
  2. Release a new version of vboxwrapper and wait for more details about this issue. I prefer to fix the root cause of the issue, and not just patch the aftermaths. Thank you for understanding.
computezrmle commented 5 months ago

As for (1.): done

As for (2.): It does harm now since it forces a task to fail without any reason to do so. I understand this as a logical error in the current vboxwrapper that needs to be corrected.

davidpanderson commented 5 months ago

My feeling is: we should try to figure out why version detection fails.

But if it does fail, we should assume it's the current version.