all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: "Max Carrara" <m.carrara@proxmox.com>
To: "Lukas Wagner" <l.wagner@proxmox.com>,
	"Proxmox VE development discussion" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v1 pve-esxi-import-tools 3/5] listvms: improve typing and add dataclasses to represent dicts
Date: Wed, 20 Mar 2024 11:08:10 +0100	[thread overview]
Message-ID: <CZYHTDDW8QJ3.3GV00W08AV639@proxmox.com> (raw)
In-Reply-To: <f38f57af-c4b2-4f5e-a88d-498024848f2a@proxmox.com>

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)
> >  





  reply	other threads:[~2024-03-20 10:08 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-19 15:32 [pve-devel] [PATCH v1 pve-esxi-import-tools 0/5] Improve listvms.py Max Carrara
2024-03-19 15:32 ` [pve-devel] [PATCH v1 pve-esxi-import-tools 1/5] listvms: remove unused import and variable Max Carrara
2024-03-19 15:32 ` [pve-devel] [PATCH v1 pve-esxi-import-tools 2/5] listvms: reorder imports Max Carrara
2024-03-19 15:32 ` [pve-devel] [PATCH v1 pve-esxi-import-tools 3/5] listvms: improve typing and add dataclasses to represent dicts Max Carrara
2024-03-20  9:38   ` Lukas Wagner
2024-03-20 10:08     ` Max Carrara [this message]
2024-03-19 15:32 ` [pve-devel] [PATCH v1 pve-esxi-import-tools 4/5] listvms: add arg parser, context manager for connections, fetch helper Max Carrara
2024-03-20  9:39   ` Lukas Wagner
2024-03-20 10:08     ` Max Carrara
2024-03-20 12:45   ` Wolfgang Bumiller
2024-03-20 17:00     ` Max Carrara
2024-03-19 15:32 ` [pve-devel] [PATCH v1 pve-esxi-import-tools 5/5] listvms: run formatter Max Carrara
2024-03-20  9:42 ` [pve-devel] [PATCH v1 pve-esxi-import-tools 0/5] Improve listvms.py Lukas Wagner
2024-03-20 10:08   ` Max Carrara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CZYHTDDW8QJ3.3GV00W08AV639@proxmox.com \
    --to=m.carrara@proxmox.com \
    --cc=l.wagner@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal