[pve-devel] [PATCH v1 pve-esxi-import-tools 0/5] Improve listvms.py

[pve-devel] [PATCH v1 pve-esxi-import-tools 0/5] Improve listvms.py

[Max Carrara]

This series adds a bunch of improvements for listvms.py, most notably
better typing (and thus better linting support) as well as parsing
arguments via the Python STL's `argparse` [0]. For more information,
please see the individual patches.

All patches were additionally tested in order to ensure that the JSON
output on successful invocations remains unchanged. This was done as
follows:

  # on master
  ./listvms.py $ARGS | jq > ref.json
  # after each patch
  ./listvms.py $ARGS | jq > output.json
  diff -u ref.json output.json

Furthermore, I built the repo's package and installed it on my local
system, and re-added my virtual ESXi host in the storage settings. The
plugin worked as expected - all my VMs on the ESXi hosts showed up and
were able to be live-imported. 

[0]: https://docs.python.org/3.11/library/argparse.html

Max Carrara (5):
  listvms: remove unused import and variable
  listvms: reorder imports
  listvms: improve typing and add dataclasses to represent dicts
  listvms: add arg parser, context manager for connections, fetch helper
  listvms: run formatter

 listvms.py | 296 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 225 insertions(+), 71 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH v1 pve-esxi-import-tools 1/5] listvms: remove unused import and variable

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 listvms.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/listvms.py b/listvms.py
index 97bb368..8666742 100755
--- a/listvms.py
+++ b/listvms.py
@@ -2,7 +2,6 @@
 
 from typing import List, Dict, Optional
 import json
-import os
 import ssl
 import sys
 from pyVim.connect import SmartConnect, Disconnect
@@ -100,7 +99,6 @@ def main():
         sys.exit(1)
 
     try:
-        datacenters = get_all_datacenters(si)
         vms = list_vms(si)
         data = {}
         for vm in vms:
-- 
2.39.2

[pve-devel] [PATCH v1 pve-esxi-import-tools 2/5] listvms: reorder imports

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 listvms.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/listvms.py b/listvms.py
index 8666742..cc3209f 100755
--- a/listvms.py
+++ b/listvms.py
@@ -1,9 +1,11 @@
 #!/usr/bin/python3
 
-from typing import List, Dict, Optional
 import json
 import ssl
 import sys
+
+from typing import List, Dict, Optional, Tuple
+
 from pyVim.connect import SmartConnect, Disconnect
 from pyVmomi import vim
 
-- 
2.39.2

[pve-devel] [PATCH v1 pve-esxi-import-tools 3/5] listvms: improve typing and add dataclasses to represent dicts

[Max Carrara]

This commit replaces some of the explicitly imported types from the
`typing` module with their inbuilt counterparts, e.g. `typing.List`
becomes `list`. This is supported since Python 3.9 [0].

Additionally, file paths are now represented as `pathlib.Path` [1],
which also checks whether the given string is actually a valid path
when constructed.

Furthermore, the `dict`s with values of mixed types are now
represented as dataclasses [2] instead, in order to make them more
type-safe (--> allow for better linting).

Because dataclasses and `pathlib.Path`s are not JSON-serializable by
default however, a helper function is added, which allows for more
fine-grained control regarding how those objects are serialized.

[0]: https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections
[1]: https://docs.python.org/3.11/library/pathlib.html
[2]: https://docs.python.org/3.11/library/dataclasses.html

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 listvms.py | 99 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 26 deletions(-)

diff --git a/listvms.py b/listvms.py
index cc3209f..cdea95a 100755
--- a/listvms.py
+++ b/listvms.py
@@ -1,26 +1,69 @@
 #!/usr/bin/python3
 
+import dataclasses
 import json
 import ssl
 import sys
 
-from typing import List, Dict, Optional, Tuple
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
 
 from pyVim.connect import SmartConnect, Disconnect
 from pyVmomi import vim
 
 
-def get_datacenter_of_vm(vm: vim.VirtualMachine) -> Optional[vim.Datacenter]:
+@dataclass
+class VmVmxInfo:
+    datastore: str
+    path: Path
+    checksum: str
+
+
+@dataclass
+class VmDiskInfo:
+    datastore: str
+    path: Path
+    capacity: int
+
+
+@dataclass
+class VmInfo:
+    config: VmVmxInfo
+    disks: list[VmDiskInfo]
+    power: str
+
+
+def json_dump_helper(obj: Any) -> Any:
+    """Converts otherwise unserializable objects to types that can be
+    serialized as JSON.
+
+    Raises:
+        TypeError: If the conversion of the object is not supported.
+    """
+    if dataclasses.is_dataclass(obj):
+        return dataclasses.asdict(obj)
+
+    match obj:
+        case Path():
+            return str(obj)
+
+    raise TypeError(
+        f"Can't make object of type {type(obj)} JSON-serializable: {repr(obj)}"
+    )
+
+
+def get_datacenter_of_vm(vm: vim.VirtualMachine) -> vim.Datacenter | None:
     """Find the Datacenter object a VM belongs to."""
     current = vm.parent
     while current:
         if isinstance(current, vim.Datacenter):
             return current
         current = current.parent
-    return None
+    return
 
 
-def list_vms(service_instance: vim.ServiceInstance) -> List[vim.VirtualMachine]:
+def list_vms(service_instance: vim.ServiceInstance) -> list[vim.VirtualMachine]:
     """List all VMs on the ESXi/vCenter server."""
     content = service_instance.content
     vm_view = content.viewManager.CreateContainerView(
@@ -32,39 +75,36 @@ def list_vms(service_instance: vim.ServiceInstance) -> List[vim.VirtualMachine]:
     vm_view.Destroy()
     return vms
 
-def parse_file_path(path):
+def parse_file_path(path) -> tuple[str, Path]:
     """Parse a path of the form '[datastore] file/path'"""
     datastore_name, relative_path = path.split('] ', 1)
     datastore_name = datastore_name.strip('[')
-    return (datastore_name, relative_path)
+    return (datastore_name, Path(relative_path))
 
-def get_vm_vmx_info(vm: vim.VirtualMachine) -> Dict[str, str]:
+def get_vm_vmx_info(vm: vim.VirtualMachine) -> VmVmxInfo:
     """Extract VMX file path and checksum from a VM object."""
     datastore_name, relative_vmx_path = parse_file_path(vm.config.files.vmPathName)
-    return {
-        'datastore': datastore_name,
-        'path': relative_vmx_path,
-        'checksum': vm.config.vmxConfigChecksum.hex() if vm.config.vmxConfigChecksum else 'N/A'
-    }
 
-def get_vm_disk_info(vm: vim.VirtualMachine) -> Dict[str, int]:
+    return VmVmxInfo(
+        datastore=datastore_name,
+        path=relative_vmx_path,
+        checksum=vm.config.vmxConfigChecksum.hex() if vm.config.vmxConfigChecksum else 'N/A'
+    )
+
+def get_vm_disk_info(vm: vim.VirtualMachine) -> list[VmDiskInfo]:
     disks = []
     for device in vm.config.hardware.device:
-        if type(device).__name__ == 'vim.vm.device.VirtualDisk':
+        if isinstance(device, vim.vm.device.VirtualDisk):
             try:
                 (datastore, path) = parse_file_path(device.backing.fileName)
                 capacity = device.capacityInBytes
-                disks.append({
-                    'datastore': datastore,
-                    'path': path,
-                    'capacity': capacity,
-                })
+                disks.append(VmDiskInfo(datastore, path, capacity))
             except Exception as err:
                 # if we can't figure out the disk stuff that's fine...
                 print("failed to get disk information for esxi vm: ", err, file=sys.stderr)
     return disks
 
-def get_all_datacenters(service_instance: vim.ServiceInstance) -> List[vim.Datacenter]:
+def get_all_datacenters(service_instance: vim.ServiceInstance) -> list[vim.Datacenter]:
     """Retrieve all datacenters from the ESXi/vCenter server."""
     content = service_instance.content
     dc_view = content.viewManager.CreateContainerView(content.rootFolder, [vim.Datacenter], True)
@@ -107,18 +147,25 @@ def main():
             name = 'vm ' + vm.name
             try:
                 dc = get_datacenter_of_vm(vm)
-                vm_info = {
-                    'config': get_vm_vmx_info(vm),
-                    'disks': get_vm_disk_info(vm),
-                    'power': vm.runtime.powerState,
-                }
+                if dc is None:
+                    print(
+                        f"Failed to get datacenter for {name}",
+                        file=sys.stderr
+                    )
+
+                vm_info = VmInfo(
+                    config=get_vm_vmx_info(vm),
+                    disks=get_vm_disk_info(vm),
+                    power=vm.runtime.powerState,
+                )
+
                 datastore_info = {ds.name: ds.url for ds in vm.config.datastoreUrl}
                 data.setdefault(dc.name, {}).setdefault('vms', {})[vm.name] = vm_info
                 data.setdefault(dc.name, {}).setdefault('datastores', {}).update(datastore_info)
             except Exception as err:
                 print("failed to get info for", name, ':', err, file=sys.stderr)
 
-        print(json.dumps(data, indent=2))
+        print(json.dumps(data, indent=2, default=json_dump_helper))
     finally:
         Disconnect(si)
 
-- 
2.39.2

[pve-devel] [PATCH v1 pve-esxi-import-tools 4/5] listvms: add arg parser, context manager for connections, fetch helper

[Max Carrara]

In order to make the CLI interface more friendly to humans, Python's
`argparse` [0] module from the standard library is used to parse the
arguments provided to the script. Each option and positional argument
also contain a short help text that is shown when running the script
with either "-h" or "--help".

Additionally, this commit also adds a context manager [1] for
establishing connections to an ESXi host. The context manager ensures
that the connection is closed in its inner `finally` block.

The inner part of the VM-data-fetching loop in `main()` is factored
out into a separate helper function, which now raises a `RuntimeError`
if the datacenter of a VM cannot be looked up.

In general, should any exception be thrown inside the loop, its output
is subsequently logged to stderr. The loop then just continues like
before.

Any exception that is not caught inside of `main()` is now printed to
stderr, followed by exiting with `1`.

Overall, the script's behaviour and output on successful operations
remains the same, except regarding unsuccessful argument parsing and
displaying error messages. In other words, invocations prior to this
patch should result in the same JSON output (if successful).

This was tested by piping the outputs of this script before and after
this commit through `jq` and then comparing the outputs with `diff`.

[0]: https://docs.python.org/3.11/library/argparse.html
[1]: https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 listvms.py | 189 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 140 insertions(+), 49 deletions(-)

diff --git a/listvms.py b/listvms.py
index cdea95a..9f85bf9 100755
--- a/listvms.py
+++ b/listvms.py
@@ -1,10 +1,13 @@
 #!/usr/bin/python3
 
+import argparse
 import dataclasses
 import json
 import ssl
 import sys
+import textwrap
 
+from contextlib import contextmanager
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Any
@@ -13,6 +16,97 @@ from pyVim.connect import SmartConnect, Disconnect
 from pyVmomi import vim
 
 
+def parse_args() -> argparse.Namespace:
+    parser = argparse.ArgumentParser(
+        prog="listvms",
+        description="List VMs on an ESXi host.",
+    )
+
+    def _squeeze_and_wrap(text: str) -> str:
+        """Makes it easier to write help text using multiline strings."""
+
+        text = text.replace("\n", " ").strip()
+
+        # squeeze recurring spaces
+        while "  " in text:
+            text = text.replace("  ", " ")
+
+        return "\n".join(textwrap.wrap(text, 60, break_on_hyphens=False))
+
+    parser.add_argument(
+        "--skip-cert-verification",
+        help=_squeeze_and_wrap(
+            """Skip the verification of TLS certs, e.g. to allow self-signed
+            certs."""
+        ),
+        action="store_true",
+    )
+
+    parser.add_argument(
+        "hostname",
+        help=_squeeze_and_wrap("""The name or address of the ESXi host."""),
+    )
+
+    parser.add_argument(
+        "username",
+        help=_squeeze_and_wrap("""The name of the user to connect with."""),
+    )
+
+    parser.add_argument(
+        "password_file",
+        help=_squeeze_and_wrap(
+            """The file which contains the password for the provided
+            username."""
+        ),
+        type=Path,
+    )
+
+    return parser.parse_args()
+
+
+@dataclass
+class EsxiConnectonArgs:
+    hostname: str
+    username: str
+    password_file: Path
+    skip_cert_verification: bool = False
+
+
+@contextmanager
+def connect_to_esxi_host(args: EsxiConnectonArgs) -> vim.ServiceInstance:
+    """Opens a connection to an ESXi host with the given username and password
+    contained in the password file.
+    """
+    ssl_context = (
+        ssl._create_unverified_context()
+        if args.skip_cert_verification
+        else None
+    )
+
+    with open(args.password_file) as pw_file:
+        password = pw_file.read().strip()
+
+    connection = None
+
+    try:
+        connection = SmartConnect(
+            host=args.hostname,
+            user=args.username,
+            pwd=password,
+            sslContext=ssl_context,
+        )
+
+        yield connection
+
+    except OSError as err:
+        print(f"Failed to connect: {err}", file=sys.stderr)
+        raise
+
+    finally:
+        if connection is not None:
+            Disconnect(connection)
+
+
 @dataclass
 class VmVmxInfo:
     datastore: str
@@ -112,62 +206,59 @@ def get_all_datacenters(service_instance: vim.ServiceInstance) -> list[vim.Datac
     dc_view.Destroy()
     return datacenters
 
+
+def fetch_and_update_vm_data(vm: vim.VirtualMachine, data: dict[Any, Any]):
+    """Fetches all required VM, datastore and datacenter information, and
+    then updates the given `dict`.
+
+    Raises:
+        RuntimeError: If looking up the datacenter for the given VM fails.
+    """
+    datacenter = get_datacenter_of_vm(vm)
+    if datacenter is None:
+        raise RuntimeError(f"Failed to lookup datacenter for VM {vm.name}")
+
+    data.setdefault(datacenter.name, {})
+
+    vms = data[datacenter.name].setdefault("vms", {})
+    datastores = data[datacenter.name].setdefault("datastores", {})
+
+    vms[vm.name] = VmInfo(
+        config=get_vm_vmx_info(vm),
+        disks=get_vm_disk_info(vm),
+        power=str(vm.runtime.powerState),
+    )
+
+    datastores.update({ds.name: ds.url for ds in vm.config.datastoreUrl})
+
+
 def main():
-    if sys.argv[1] == '--skip-cert-verification':
-        del sys.argv[1]
-        ssl_context = ssl._create_unverified_context()
-    else:
-        ssl_context = None
-
-    esxi_host = sys.argv[1]
-    esxi_user = sys.argv[2]
-    esxi_password_file = sys.argv[3]
-
-    esxi_password = ''
-    with open(esxi_password_file) as f:
-        esxi_password = f.read()
-        if esxi_password.endswith('\n'):
-            esxi_password = esxi_password[:-1]
+    args = parse_args()
 
-    try:
-        si = SmartConnect(
-            host=esxi_host,
-            user=esxi_user,
-            pwd=esxi_password,
-            sslContext=ssl_context,
-        )
-    except OSError as err:
-        print(f"failed to connect: {err}")
-        sys.exit(1)
+    connection_args = EsxiConnectonArgs(
+        hostname=args.hostname,
+        username=args.username,
+        password_file=args.password_file,
+        skip_cert_verification=args.skip_cert_verification,
+    )
 
-    try:
-        vms = list_vms(si)
+    with connect_to_esxi_host(connection_args) as connection:
         data = {}
-        for vm in vms:
-            name = 'vm ' + vm.name
+        for vm in list_vms(connection):
             try:
-                dc = get_datacenter_of_vm(vm)
-                if dc is None:
-                    print(
-                        f"Failed to get datacenter for {name}",
-                        file=sys.stderr
-                    )
-
-                vm_info = VmInfo(
-                    config=get_vm_vmx_info(vm),
-                    disks=get_vm_disk_info(vm),
-                    power=vm.runtime.powerState,
+                fetch_and_update_vm_data(vm, data)
+            except Exception as err:
+                print(
+                    f"Failed to get info for VM {vm.name}: {err}",
+                    file=sys.stderr,
                 )
 
-                datastore_info = {ds.name: ds.url for ds in vm.config.datastoreUrl}
-                data.setdefault(dc.name, {}).setdefault('vms', {})[vm.name] = vm_info
-                data.setdefault(dc.name, {}).setdefault('datastores', {}).update(datastore_info)
-            except Exception as err:
-                print("failed to get info for", name, ':', err, file=sys.stderr)
+    print(json.dumps(data, indent=2, default=json_dump_helper))
 
-        print(json.dumps(data, indent=2, default=json_dump_helper))
-    finally:
-        Disconnect(si)
 
 if __name__ == "__main__":
-    main()
+    try:
+        main()
+    except Exception as err:
+        print(f"Encountered unexpected error: {err}", file=sys.stderr)
+        sys.exit(1)
-- 
2.39.2

[pve-devel] [PATCH v1 pve-esxi-import-tools 5/5] listvms: run formatter

[Max Carrara]

This commit formats the script using `black -l 80` [0], even though we
don't have an official style guide for Python.

[0]: https://github.com/psf/black

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 listvms.py | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/listvms.py b/listvms.py
index 9f85bf9..a3cb35c 100755
--- a/listvms.py
+++ b/listvms.py
@@ -169,22 +169,29 @@ def list_vms(service_instance: vim.ServiceInstance) -> list[vim.VirtualMachine]:
     vm_view.Destroy()
     return vms
 
+
 def parse_file_path(path) -> tuple[str, Path]:
     """Parse a path of the form '[datastore] file/path'"""
-    datastore_name, relative_path = path.split('] ', 1)
-    datastore_name = datastore_name.strip('[')
+    datastore_name, relative_path = path.split("] ", 1)
+    datastore_name = datastore_name.strip("[")
     return (datastore_name, Path(relative_path))
 
+
 def get_vm_vmx_info(vm: vim.VirtualMachine) -> VmVmxInfo:
     """Extract VMX file path and checksum from a VM object."""
-    datastore_name, relative_vmx_path = parse_file_path(vm.config.files.vmPathName)
+    datastore_name, relative_vmx_path = parse_file_path(
+        vm.config.files.vmPathName
+    )
 
     return VmVmxInfo(
         datastore=datastore_name,
         path=relative_vmx_path,
-        checksum=vm.config.vmxConfigChecksum.hex() if vm.config.vmxConfigChecksum else 'N/A'
+        checksum=vm.config.vmxConfigChecksum.hex()
+        if vm.config.vmxConfigChecksum
+        else "N/A",
     )
 
+
 def get_vm_disk_info(vm: vim.VirtualMachine) -> list[VmDiskInfo]:
     disks = []
     for device in vm.config.hardware.device:
@@ -195,13 +202,22 @@ def get_vm_disk_info(vm: vim.VirtualMachine) -> list[VmDiskInfo]:
                 disks.append(VmDiskInfo(datastore, path, capacity))
             except Exception as err:
                 # if we can't figure out the disk stuff that's fine...
-                print("failed to get disk information for esxi vm: ", err, file=sys.stderr)
+                print(
+                    "failed to get disk information for esxi vm: ",
+                    err,
+                    file=sys.stderr,
+                )
     return disks
 
-def get_all_datacenters(service_instance: vim.ServiceInstance) -> list[vim.Datacenter]:
+
+def get_all_datacenters(
+    service_instance: vim.ServiceInstance,
+) -> list[vim.Datacenter]:
     """Retrieve all datacenters from the ESXi/vCenter server."""
     content = service_instance.content
-    dc_view = content.viewManager.CreateContainerView(content.rootFolder, [vim.Datacenter], True)
+    dc_view = content.viewManager.CreateContainerView(
+        content.rootFolder, [vim.Datacenter], True
+    )
     datacenters = dc_view.view
     dc_view.Destroy()
     return datacenters
-- 
2.39.2

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 3/5] listvms: improve typing and add dataclasses to represent dicts

[Lukas Wagner]

On  2024-03-19 16:32, Max Carrara wrote:
> This commit replaces some of the explicitly imported types from the
> `typing` module with their inbuilt counterparts, e.g. `typing.List`
> becomes `list`. This is supported since Python 3.9 [0].
> 
> Additionally, file paths are now represented as `pathlib.Path` [1],
> which also checks whether the given string is actually a valid path
> when constructed.
> 
> Furthermore, the `dict`s with values of mixed types are now
> represented as dataclasses [2] instead, in order to make them more
> type-safe (--> allow for better linting).
> 
> Because dataclasses and `pathlib.Path`s are not JSON-serializable by
> default however, a helper function is added, which allows for more
> fine-grained control regarding how those objects are serialized.
> 
> [0]: https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections
> [1]: https://docs.python.org/3.11/library/pathlib.html
> [2]: https://docs.python.org/3.11/library/dataclasses.html
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  listvms.py | 99 ++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 73 insertions(+), 26 deletions(-)
> 
> diff --git a/listvms.py b/listvms.py
> index cc3209f..cdea95a 100755
> --- a/listvms.py
> +++ b/listvms.py
> @@ -1,26 +1,69 @@
>  #!/usr/bin/python3
>  
> +import dataclasses
>  import json
>  import ssl
>  import sys
>  
> -from typing import List, Dict, Optional, Tuple
> +from dataclasses import dataclass
> +from pathlib import Path
> +from typing import Any
>  
>  from pyVim.connect import SmartConnect, Disconnect
>  from pyVmomi import vim
>  
>  
> -def get_datacenter_of_vm(vm: vim.VirtualMachine) -> Optional[vim.Datacenter]:
> +@dataclass
> +class VmVmxInfo:
> +    datastore: str
> +    path: Path
> +    checksum: str
> +
> +
> +@dataclass
> +class VmDiskInfo:
> +    datastore: str
> +    path: Path
> +    capacity: int
> +
> +
> +@dataclass
> +class VmInfo:
> +    config: VmVmxInfo
> +    disks: list[VmDiskInfo]
> +    power: str
> +
> +
> +def json_dump_helper(obj: Any) -> Any:
> +    """Converts otherwise unserializable objects to types that can be
> +    serialized as JSON.
> +
> +    Raises:
> +        TypeError: If the conversion of the object is not supported.
> +    """
> +    if dataclasses.is_dataclass(obj):
> +        return dataclasses.asdict(obj)
> +
> +    match obj:
> +        case Path():
> +            return str(obj)
> +
> +    raise TypeError(
> +        f"Can't make object of type {type(obj)} JSON-serializable: {repr(obj)}"
> +    )
> +
> +
> +def get_datacenter_of_vm(vm: vim.VirtualMachine) -> vim.Datacenter | None:
>      """Find the Datacenter object a VM belongs to."""
>      current = vm.parent
>      while current:
>          if isinstance(current, vim.Datacenter):
>              return current
>          current = current.parent
> -    return None
> +    return

mypy does not seem to like this change :)

listvms.py:157: error: Return value expected  [return-value]


>  
>  
> -def list_vms(service_instance: vim.ServiceInstance) -> List[vim.VirtualMachine]:
> +def list_vms(service_instance: vim.ServiceInstance) -> list[vim.VirtualMachine]:
>      """List all VMs on the ESXi/vCenter server."""
>      content = service_instance.content
>      vm_view = content.viewManager.CreateContainerView(
> @@ -32,39 +75,36 @@ def list_vms(service_instance: vim.ServiceInstance) -> List[vim.VirtualMachine]:
>      vm_view.Destroy()
>      return vms
>  
> -def parse_file_path(path):
> +def parse_file_path(path) -> tuple[str, Path]:
>      """Parse a path of the form '[datastore] file/path'"""
>      datastore_name, relative_path = path.split('] ', 1)
>      datastore_name = datastore_name.strip('[')
> -    return (datastore_name, relative_path)
> +    return (datastore_name, Path(relative_path))
>  
> -def get_vm_vmx_info(vm: vim.VirtualMachine) -> Dict[str, str]:
> +def get_vm_vmx_info(vm: vim.VirtualMachine) -> VmVmxInfo:
>      """Extract VMX file path and checksum from a VM object."""
>      datastore_name, relative_vmx_path = parse_file_path(vm.config.files.vmPathName)
> -    return {
> -        'datastore': datastore_name,
> -        'path': relative_vmx_path,
> -        'checksum': vm.config.vmxConfigChecksum.hex() if vm.config.vmxConfigChecksum else 'N/A'
> -    }
>  
> -def get_vm_disk_info(vm: vim.VirtualMachine) -> Dict[str, int]:
> +    return VmVmxInfo(
> +        datastore=datastore_name,
> +        path=relative_vmx_path,
> +        checksum=vm.config.vmxConfigChecksum.hex() if vm.config.vmxConfigChecksum else 'N/A'
> +    )
> +
> +def get_vm_disk_info(vm: vim.VirtualMachine) -> list[VmDiskInfo]:
>      disks = []
>      for device in vm.config.hardware.device:
> -        if type(device).__name__ == 'vim.vm.device.VirtualDisk':
> +        if isinstance(device, vim.vm.device.VirtualDisk):
>              try:
>                  (datastore, path) = parse_file_path(device.backing.fileName)
>                  capacity = device.capacityInBytes
> -                disks.append({
> -                    'datastore': datastore,
> -                    'path': path,
> -                    'capacity': capacity,
> -                })
> +                disks.append(VmDiskInfo(datastore, path, capacity))
>              except Exception as err:
>                  # if we can't figure out the disk stuff that's fine...
>                  print("failed to get disk information for esxi vm: ", err, file=sys.stderr)
>      return disks
>  
> -def get_all_datacenters(service_instance: vim.ServiceInstance) -> List[vim.Datacenter]:
> +def get_all_datacenters(service_instance: vim.ServiceInstance) -> list[vim.Datacenter]:
>      """Retrieve all datacenters from the ESXi/vCenter server."""
>      content = service_instance.content
>      dc_view = content.viewManager.CreateContainerView(content.rootFolder, [vim.Datacenter], True)
> @@ -107,18 +147,25 @@ def main():
>              name = 'vm ' + vm.name
>              try:
>                  dc = get_datacenter_of_vm(vm)
> -                vm_info = {
> -                    'config': get_vm_vmx_info(vm),
> -                    'disks': get_vm_disk_info(vm),
> -                    'power': vm.runtime.powerState,
> -                }
> +                if dc is None:
> +                    print(
> +                        f"Failed to get datacenter for {name}",
> +                        file=sys.stderr
> +                    )
> +
> +                vm_info = VmInfo(
> +                    config=get_vm_vmx_info(vm),
> +                    disks=get_vm_disk_info(vm),
> +                    power=vm.runtime.powerState,
> +                )
> +
>                  datastore_info = {ds.name: ds.url for ds in vm.config.datastoreUrl}
>                  data.setdefault(dc.name, {}).setdefault('vms', {})[vm.name] = vm_info
>                  data.setdefault(dc.name, {}).setdefault('datastores', {}).update(datastore_info)
>              except Exception as err:
>                  print("failed to get info for", name, ':', err, file=sys.stderr)
>  
> -        print(json.dumps(data, indent=2))
> +        print(json.dumps(data, indent=2, default=json_dump_helper))

I guess this could be 

`json.dump(data, sys.stdout, ...)` 

That'd not add a newline in the end, not sure if that is important here.
But it really does not matter that much, I guess. ;)

>      finally:
>          Disconnect(si)
>  

-- 
- Lukas

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 4/5] listvms: add arg parser, context manager for connections, fetch helper

[Lukas Wagner]

On  2024-03-19 16:32, Max Carrara wrote:

> +    def _squeeze_and_wrap(text: str) -> str:
> +        """Makes it easier to write help text using multiline strings."""
> +
> +        text = text.replace("\n", " ").strip()
> +
> +        # squeeze recurring spaces
> +        while "  " in text:
> +            text = text.replace("  ", " ")
> +

How about `" ".join(text.split())`? :)

-- 
- Lukas

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 0/5] Improve listvms.py

[Lukas Wagner]

On  2024-03-19 16:32, Max Carrara wrote:
> This series adds a bunch of improvements for listvms.py, most notably
> better typing (and thus better linting support) as well as parsing
> arguments via the Python STL's `argparse` [0]. For more information,
> please see the individual patches.
> 
> All patches were additionally tested in order to ensure that the JSON
> output on successful invocations remains unchanged. This was done as
> follows:
> 
>   # on master
>   ./listvms.py $ARGS | jq > ref.json
>   # after each patch
>   ./listvms.py $ARGS | jq > output.json
>   diff -u ref.json output.json
> 
> Furthermore, I built the repo's package and installed it on my local
> system, and re-added my virtual ESXi host in the storage settings. The
> plugin worked as expected - all my VMs on the ESXi hosts showed up and
> were able to be live-imported. 
> 

Looks good to me.

Since this is type-hinted Python code, maybe the following `mypy.ini` would make sense?

```
[mypy]

[mypy-pyVmomi]
ignore_missing_imports = True

[mypy-pyVim.*]
ignore_missing_imports = True
```

Otherwise `mypy` complains about missing type stubs for those two modules :)


Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

-- 
- Lukas

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 3/5] listvms: improve typing and add dataclasses to represent dicts

[Max Carrara]

On Wed Mar 20, 2024 at 10:38 AM CET, Lukas Wagner wrote:
>
>
> On  2024-03-19 16:32, Max Carrara wrote:
> > This commit replaces some of the explicitly imported types from the
> > `typing` module with their inbuilt counterparts, e.g. `typing.List`
> > becomes `list`. This is supported since Python 3.9 [0].
> > 
> > Additionally, file paths are now represented as `pathlib.Path` [1],
> > which also checks whether the given string is actually a valid path
> > when constructed.
> > 
> > Furthermore, the `dict`s with values of mixed types are now
> > represented as dataclasses [2] instead, in order to make them more
> > type-safe (--> allow for better linting).
> > 
> > Because dataclasses and `pathlib.Path`s are not JSON-serializable by
> > default however, a helper function is added, which allows for more
> > fine-grained control regarding how those objects are serialized.
> > 
> > [0]: https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections
> > [1]: https://docs.python.org/3.11/library/pathlib.html
> > [2]: https://docs.python.org/3.11/library/dataclasses.html
> > 
> > Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> > ---
> >  listvms.py | 99 ++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 73 insertions(+), 26 deletions(-)
> > 
> > diff --git a/listvms.py b/listvms.py
> > index cc3209f..cdea95a 100755
> > --- a/listvms.py
> > +++ b/listvms.py
> > @@ -1,26 +1,69 @@
> >  #!/usr/bin/python3
> >  
> > +import dataclasses
> >  import json
> >  import ssl
> >  import sys
> >  
> > -from typing import List, Dict, Optional, Tuple
> > +from dataclasses import dataclass
> > +from pathlib import Path
> > +from typing import Any
> >  
> >  from pyVim.connect import SmartConnect, Disconnect
> >  from pyVmomi import vim
> >  
> >  
> > -def get_datacenter_of_vm(vm: vim.VirtualMachine) -> Optional[vim.Datacenter]:
> > +@dataclass
> > +class VmVmxInfo:
> > +    datastore: str
> > +    path: Path
> > +    checksum: str
> > +
> > +
> > +@dataclass
> > +class VmDiskInfo:
> > +    datastore: str
> > +    path: Path
> > +    capacity: int
> > +
> > +
> > +@dataclass
> > +class VmInfo:
> > +    config: VmVmxInfo
> > +    disks: list[VmDiskInfo]
> > +    power: str
> > +
> > +
> > +def json_dump_helper(obj: Any) -> Any:
> > +    """Converts otherwise unserializable objects to types that can be
> > +    serialized as JSON.
> > +
> > +    Raises:
> > +        TypeError: If the conversion of the object is not supported.
> > +    """
> > +    if dataclasses.is_dataclass(obj):
> > +        return dataclasses.asdict(obj)
> > +
> > +    match obj:
> > +        case Path():
> > +            return str(obj)
> > +
> > +    raise TypeError(
> > +        f"Can't make object of type {type(obj)} JSON-serializable: {repr(obj)}"
> > +    )
> > +
> > +
> > +def get_datacenter_of_vm(vm: vim.VirtualMachine) -> vim.Datacenter | None:
> >      """Find the Datacenter object a VM belongs to."""
> >      current = vm.parent
> >      while current:
> >          if isinstance(current, vim.Datacenter):
> >              return current
> >          current = current.parent
> > -    return None
> > +    return
>
> mypy does not seem to like this change :)
>
> listvms.py:157: error: Return value expected  [return-value]

Interesting! I got `rufflsp`, which doesn't seem to warn me here. Thanks
for pointing that out, will add that in v2.

>
>
> >  
> >  
> > -def list_vms(service_instance: vim.ServiceInstance) -> List[vim.VirtualMachine]:
> > +def list_vms(service_instance: vim.ServiceInstance) -> list[vim.VirtualMachine]:
> >      """List all VMs on the ESXi/vCenter server."""
> >      content = service_instance.content
> >      vm_view = content.viewManager.CreateContainerView(
> > @@ -32,39 +75,36 @@ def list_vms(service_instance: vim.ServiceInstance) -> List[vim.VirtualMachine]:
> >      vm_view.Destroy()
> >      return vms
> >  
> > -def parse_file_path(path):
> > +def parse_file_path(path) -> tuple[str, Path]:
> >      """Parse a path of the form '[datastore] file/path'"""
> >      datastore_name, relative_path = path.split('] ', 1)
> >      datastore_name = datastore_name.strip('[')
> > -    return (datastore_name, relative_path)
> > +    return (datastore_name, Path(relative_path))
> >  
> > -def get_vm_vmx_info(vm: vim.VirtualMachine) -> Dict[str, str]:
> > +def get_vm_vmx_info(vm: vim.VirtualMachine) -> VmVmxInfo:
> >      """Extract VMX file path and checksum from a VM object."""
> >      datastore_name, relative_vmx_path = parse_file_path(vm.config.files.vmPathName)
> > -    return {
> > -        'datastore': datastore_name,
> > -        'path': relative_vmx_path,
> > -        'checksum': vm.config.vmxConfigChecksum.hex() if vm.config.vmxConfigChecksum else 'N/A'
> > -    }
> >  
> > -def get_vm_disk_info(vm: vim.VirtualMachine) -> Dict[str, int]:
> > +    return VmVmxInfo(
> > +        datastore=datastore_name,
> > +        path=relative_vmx_path,
> > +        checksum=vm.config.vmxConfigChecksum.hex() if vm.config.vmxConfigChecksum else 'N/A'
> > +    )
> > +
> > +def get_vm_disk_info(vm: vim.VirtualMachine) -> list[VmDiskInfo]:
> >      disks = []
> >      for device in vm.config.hardware.device:
> > -        if type(device).__name__ == 'vim.vm.device.VirtualDisk':
> > +        if isinstance(device, vim.vm.device.VirtualDisk):
> >              try:
> >                  (datastore, path) = parse_file_path(device.backing.fileName)
> >                  capacity = device.capacityInBytes
> > -                disks.append({
> > -                    'datastore': datastore,
> > -                    'path': path,
> > -                    'capacity': capacity,
> > -                })
> > +                disks.append(VmDiskInfo(datastore, path, capacity))
> >              except Exception as err:
> >                  # if we can't figure out the disk stuff that's fine...
> >                  print("failed to get disk information for esxi vm: ", err, file=sys.stderr)
> >      return disks
> >  
> > -def get_all_datacenters(service_instance: vim.ServiceInstance) -> List[vim.Datacenter]:
> > +def get_all_datacenters(service_instance: vim.ServiceInstance) -> list[vim.Datacenter]:
> >      """Retrieve all datacenters from the ESXi/vCenter server."""
> >      content = service_instance.content
> >      dc_view = content.viewManager.CreateContainerView(content.rootFolder, [vim.Datacenter], True)
> > @@ -107,18 +147,25 @@ def main():
> >              name = 'vm ' + vm.name
> >              try:
> >                  dc = get_datacenter_of_vm(vm)
> > -                vm_info = {
> > -                    'config': get_vm_vmx_info(vm),
> > -                    'disks': get_vm_disk_info(vm),
> > -                    'power': vm.runtime.powerState,
> > -                }
> > +                if dc is None:
> > +                    print(
> > +                        f"Failed to get datacenter for {name}",
> > +                        file=sys.stderr
> > +                    )
> > +
> > +                vm_info = VmInfo(
> > +                    config=get_vm_vmx_info(vm),
> > +                    disks=get_vm_disk_info(vm),
> > +                    power=vm.runtime.powerState,
> > +                )
> > +
> >                  datastore_info = {ds.name: ds.url for ds in vm.config.datastoreUrl}
> >                  data.setdefault(dc.name, {}).setdefault('vms', {})[vm.name] = vm_info
> >                  data.setdefault(dc.name, {}).setdefault('datastores', {}).update(datastore_info)
> >              except Exception as err:
> >                  print("failed to get info for", name, ':', err, file=sys.stderr)
> >  
> > -        print(json.dumps(data, indent=2))
> > +        print(json.dumps(data, indent=2, default=json_dump_helper))
>
> I guess this could be 
>
> `json.dump(data, sys.stdout, ...)` 
>
> That'd not add a newline in the end, not sure if that is important here.
> But it really does not matter that much, I guess. ;)

I guess it does not in this case, but since you mentioned it, I'll just
include it in v2 as well. I don't think the newline is important, but
will test nevertheless. Thanks for the tip!

>
> >      finally:
> >          Disconnect(si)
> >  

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 4/5] listvms: add arg parser, context manager for connections, fetch helper

[Max Carrara]

On Wed Mar 20, 2024 at 10:39 AM CET, Lukas Wagner wrote:
>
>
> On  2024-03-19 16:32, Max Carrara wrote:
>
> > +    def _squeeze_and_wrap(text: str) -> str:
> > +        """Makes it easier to write help text using multiline strings."""
> > +
> > +        text = text.replace("\n", " ").strip()
> > +
> > +        # squeeze recurring spaces
> > +        while "  " in text:
> > +            text = text.replace("  ", " ")
> > +
>
> How about `" ".join(text.split())`? :)

That would indeed also work here - I tend to automatically resort to the
while-loop because `str.split()` also splits on `\n`, `\t` and some
other whitespace characters, which is what I *usually* want to avoid,
but in this case it's unnecessary.

Will use your suggestion instead in v2, thanks!

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 0/5] Improve listvms.py

[Max Carrara]

On Wed Mar 20, 2024 at 10:42 AM CET, Lukas Wagner wrote:
> On  2024-03-19 16:32, Max Carrara wrote:
> > This series adds a bunch of improvements for listvms.py, most notably
> > better typing (and thus better linting support) as well as parsing
> > arguments via the Python STL's `argparse` [0]. For more information,
> > please see the individual patches.
> > 
> > All patches were additionally tested in order to ensure that the JSON
> > output on successful invocations remains unchanged. This was done as
> > follows:
> > 
> >   # on master
> >   ./listvms.py $ARGS | jq > ref.json
> >   # after each patch
> >   ./listvms.py $ARGS | jq > output.json
> >   diff -u ref.json output.json
> > 
> > Furthermore, I built the repo's package and installed it on my local
> > system, and re-added my virtual ESXi host in the storage settings. The
> > plugin worked as expected - all my VMs on the ESXi hosts showed up and
> > were able to be live-imported. 
> > 
>
> Looks good to me.
>
> Since this is type-hinted Python code, maybe the following `mypy.ini` would make sense?
>
> ```
> [mypy]
>
> [mypy-pyVmomi]
> ignore_missing_imports = True
>
> [mypy-pyVim.*]
> ignore_missing_imports = True
> ```
>
> Otherwise `mypy` complains about missing type stubs for those two modules :)

I think adding this wouldn't hurt at all, so I'll include it in v2.
Thanks for the suggestion!

If only the VMware Python stuff wasn't so ... well, the way it is.

>
>
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

Thanks for reviewing and testing!

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 4/5] listvms: add arg parser, context manager for connections, fetch helper

[Wolfgang Bumiller]

On Tue, Mar 19, 2024 at 04:32:49PM +0100, Max Carrara wrote:
> +@contextmanager
> +def connect_to_esxi_host(args: EsxiConnectonArgs) -> vim.ServiceInstance:
> +    """Opens a connection to an ESXi host with the given username and password
> +    contained in the password file.
> +    """
> +    ssl_context = (
> +        ssl._create_unverified_context()
> +        if args.skip_cert_verification
> +        else None
> +    )
> +
> +    with open(args.password_file) as pw_file:
> +        password = pw_file.read().strip()

This strips all whitespace from both sides, which is not what we want.
(Not that I particularly care whether esxi even allows spaces in
passwords at all...)
The old code specifically only stripped a single trailing *newline*,
mainly for when you edit the file with eg. vim which defaults to adding
one...

Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 4/5] listvms: add arg parser, context manager for connections, fetch helper

[Max Carrara]

On Wed Mar 20, 2024 at 1:45 PM CET, Wolfgang Bumiller wrote:
> On Tue, Mar 19, 2024 at 04:32:49PM +0100, Max Carrara wrote:
> > +@contextmanager
> > +def connect_to_esxi_host(args: EsxiConnectonArgs) -> vim.ServiceInstance:
> > +    """Opens a connection to an ESXi host with the given username and password
> > +    contained in the password file.
> > +    """
> > +    ssl_context = (
> > +        ssl._create_unverified_context()
> > +        if args.skip_cert_verification
> > +        else None
> > +    )
> > +
> > +    with open(args.password_file) as pw_file:
> > +        password = pw_file.read().strip()
>
> This strips all whitespace from both sides, which is not what we want.
> (Not that I particularly care whether esxi even allows spaces in
> passwords at all...)
> The old code specifically only stripped a single trailing *newline*,
> mainly for when you edit the file with eg. vim which defaults to adding
> one...

Oh, thanks for pointing this out - I had assumed that, well, passwords
wouldn't contain spaces ... ever.

Mea culpa; will fix in v2.