Re: [pve-devel] [PATCH manager 9/9] report: add microcode info to better assess possible system impacts
On Fri, 2024-03-22 at 17:44 +0100, Stoiko Ivanov wrote:
> On Fri, 22 Mar 2024 14:59:33 +0100
> Alexander Zeidler wrote:
>
> > * list availability and installation status of `*microcode` packages
> > * grep for applied "Early OS Microcode Updates"
> > * grep for (un)patched CPU vulnerability messages
> >
> > Signed-off-by: Alexander Zeidler
> > ---
> > PVE/Report.pm | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/PVE/Report.pm b/PVE/Report.pm
> > index fe497b43..18c554ec 100644
> > --- a/PVE/Report.pm
> > +++ b/PVE/Report.pm
> > @@ -108,6 +108,8 @@ my $init_report_cmds = sub {
> > 'dmidecode -t bios -q',
> > 'dmidecode -t memory | grep -E
> > "Capacity|Devices|Size|Manu|Part" | sed -Ez "s/\n\t(M|P)[^:]*:
> > (\S*)/\t\2/g" | sort',
> > 'lscpu',
> > + 'apt list *microcode 2>/dev/null | column -tL',
> While `apt` works really well and its output hasn't changed since I
> started using it (wheezy or jessie) - I still want to mention it's output
> when piping:
> ```
> WARNING: apt does not have a stable CLI interface. Use with caution in
> scripts. ```
> potentially consider either using our code directly or switching to
> `dpkg -l`?
> (but as said `apt` has been pretty stable, and we simply dump the output -
> so probably the warning is not too relevant here)
Thank you! I have noticed the missing -a to list possible further package
versions for downgrading if needed. So `dpkg` and its verbose output would
not be an equal solution. However, since previous package versions can be
looked up in the Debian repo, the whole command may not be needed in the
first place.
Instead it may be better to include the current installed microcode version
in `pveversion` and use the
> > + 'dmesg | grep -i "microcode\|vuln"',
to see if microcode was loaded during this boot.
> > 'lspci -nnk',
> > ],
> > },
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-esxi-import-tools 6/7] listvms: run formatter
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
---
Changes v1 --> v2:
* none
listvms.py | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/listvms.py b/listvms.py
index 58b09df..90b27bf 100755
--- a/listvms.py
+++ b/listvms.py
@@ -170,22 +170,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:
@@ -196,13 +203,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: Any = content.viewManager.CreateContainerView(
+content.rootFolder, [vim.Datacenter], True
+)
datacenters = dc_view.view
dc_view.Destroy()
return datacenters
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-esxi-import-tools 5/7] listvms: dump json directly to stdout
Signed-off-by: Max Carrara --- Changes v1 --> v2: * new (thanks for the suggestion, Lukas!) listvms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/listvms.py b/listvms.py index 354844b..58b09df 100755 --- a/listvms.py +++ b/listvms.py @@ -254,7 +254,7 @@ def main(): file=sys.stderr, ) -print(json.dumps(data, indent=2, default=json_dump_helper)) +json.dump(data, sys.stdout, indent=2, default=json_dump_helper) if __name__ == "__main__": -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-esxi-import-tools 7/7] use mypy for automatic type checks in Python
This commit adds mypy [0] as build dependency and ensures it is invoked during the package build process. mypy can also be manually invoked via `make lint`. A mypy.ini file [1] is also added to disable errors regarding missing type stubs for pyVmomi and pyVim. [0]: https://www.mypy-lang.org/ [1]: https://mypy.readthedocs.io/en/stable/config_file.html Signed-off-by: Max Carrara --- Changes v1 --> v2: * new (thanks for suggesting to add mypy.ini, Lukas!) Makefile | 13 - debian/control | 1 + mypy.ini | 8 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 mypy.ini diff --git a/Makefile b/Makefile index 73e7c5e..cc70305 100644 --- a/Makefile +++ b/Makefile @@ -28,6 +28,7 @@ BINARY = $(COMPILEDIR)/esxi-folder-fuse SCRIPT = listvms.py CARGO ?= cargo +MYPY ?= mypy .PHONY: all all: $(BINARY) @@ -40,8 +41,17 @@ check: test test: $(CARGO) test $(CARGO_BUILD_ARGS) +.lint-incremental: $(SCRIPT) + $(MYPY) $? + touch "$@" + +.PHONY: lint +lint: $(SCRIPT) + $(MYPY) $(SCRIPT) + touch ".lint-incremental" + .PHONY: install -install: $(BINARY) $(SCRIPT) +install: $(BINARY) $(SCRIPT) .lint-incremental install -m755 -d $(DESTDIR)$(LIBEXECDIR)/pve-esxi-import-tools install -m755 -t $(DESTDIR)$(LIBEXECDIR)/pve-esxi-import-tools $(BINARY) install -m755 -t $(DESTDIR)$(LIBEXECDIR)/pve-esxi-import-tools $(SCRIPT) @@ -55,6 +65,7 @@ $(BUILD_DIR): cp -t $@.tmp -a \ debian \ Makefile \ + mypy.ini \ listvms.py \ Cargo.toml \ src diff --git a/debian/control b/debian/control index 3c12f29..8687d6d 100644 --- a/debian/control +++ b/debian/control @@ -30,6 +30,7 @@ Build-Depends: cargo:native (>= 0.65.0~), librust-tokio-1+rt-multi-thread-dev, librust-tokio-1+time-dev, libstd-rust-dev, + mypy, rustc:native, Maintainer: Proxmox Support Team Standards-Version: 4.6.2 diff --git a/mypy.ini b/mypy.ini new file mode 100644 index 000..e6724c8 --- /dev/null +++ b/mypy.ini @@ -0,0 +1,8 @@ +[mypy] + +[mypy-pyVmomi] +ignore_missing_imports = True + +[mypy-pyVim.*] +ignore_missing_imports = True + -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-esxi-import-tools 3/7] listvms: improve typing and add dataclasses to represent dicts
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
---
Changes v1 --> v2:
* rebase so patch applies onto previous patch
(the `Tuple` import removal)
listvms.py | 99 --
1 file changed, 73 insertions(+), 26 deletions(-)
diff --git a/listvms.py b/listvms.py
index 0b64b0b..fe257a4 100755
--- a/listvms.py
+++ b/listvms.py
@@ -1,16 +1,59 @@
#!/usr/bin/python3
+import dataclasses
import json
import ssl
import sys
-from typing import List, Dict, Optional
+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:
@@ -20,10 +63,10 @@ def get_datacenter_of_vm(vm: vim.VirtualMachine) ->
Optional[vim.Datacenter]:
return None
-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(
+vm_view: Any = content.viewManager.CreateContainerView(
content.rootFolder,
[vim.VirtualMachine],
True,
@@ -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,
-})
+di
[pve-devel] [PATCH v2 pve-esxi-import-tools 2/7] listvms: reorder imports
Signed-off-by: Max Carrara --- Changes v1 --> v2: * remove unused import of `Tuple` type (was accidentally added during spurious rebasing) listvms.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/listvms.py b/listvms.py index d52d184..0b64b0b 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 + from pyVim.connect import SmartConnect, Disconnect from pyVmomi import vim -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-esxi-import-tools 4/7] listvms: add arg parser, context manager for connections, fetch helper
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
---
Changes v1 --> v2:
* rebase onto master
* use `Generator` as return type for the ESXi connection context
manager
* do not strip all whitespace from the read password file and retain
original behaviour of only removing a trailing newline
listvms.py | 195 ++---
1 file changed, 142 insertions(+), 53 deletions(-)
diff --git a/listvms.py b/listvms.py
index fe257a4..354844b 100755
--- a/listvms.py
+++ b/listvms.py
@@ -1,18 +1,113 @@
#!/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
+from typing import Any, Generator
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 = " ".join(text.split())
+
+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,
+) -> Generator[vim.ServiceInstance, None, None]:
+"""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()
+if password.endswith("\n"):
+password = password[:-1]
+
+connection = None
+
+try:
+connection = SmartConnect(
+host=args.hostname,
+user=args.username,
+pwd=password,
+sslContext=ssl_context,
+)
+
+yield connection
+
+except ssl.SSLCertVerificationError:
+raise ConnectionError(
+"Failed to verify certificate - add the CA of your ESXi to the "
+"system trust store or skip verification",
+)
+
+finally:
+if connection is not None:
+Disconnect(connection)
+
+
@dataclass
class VmVmxInfo:
datastore: str
@@ -112,65 +207,59 @@ def get_all_datacenters(service_instance:
vim.ServiceInstance) -> list[vim.Datac
dc_view.Destroy()
return datacenters
+
[pve-devel] [PATCH v2 pve-esxi-import-tools 1/7] listvms: remove unused import and variable
Signed-off-by: Max Carrara
---
Changes v1 --> v2:
* none
listvms.py | 2 --
1 file changed, 2 deletions(-)
diff --git a/listvms.py b/listvms.py
index 0a1b830..d52d184 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
@@ -103,7 +102,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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 pve-esxi-import-tools 0/7] Improve listvms.py
Improve listvms.py - Version 2 == Notable Changes Since v1 * mypy [0] is now a build dependency and runs automatically on `make install` (and thus also on `make deb` etc.) * JSON output is now directly streamed to stdout via `json.dump()` instead of creating and printing a string For a detailed list of changes, please see the comments in the invididual patches. Older Versions -- v1: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062258.html References -- [0]: https://www.mypy-lang.org/ Summary of Changes -- Max Carrara (7): 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: dump json directly to stdout listvms: run formatter use mypy for automatic type checks in Python Makefile | 13 ++- debian/control | 1 + listvms.py | 300 + mypy.ini | 8 ++ 4 files changed, 247 insertions(+), 75 deletions(-) create mode 100644 mypy.ini -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 8/9] report: add info about (un)used memory slots
On Fri, 2024-03-22 at 14:59 +0100, Alexander Zeidler wrote: > * to see if a RAM upgrade is slot/capacity-wise possible > * to spot added/replaced RAM that may now be causing issues > > # dmidecode -t memory ... > Maximum Capacity: 2 TB > Number Of Devices: 8 > Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1 > Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1 > Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1 > Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1 > Size: No Module Installed > Size: No Module Installed > Size: No Module Installed > Size: No Module Installed > > + 'dmidecode -t memory | grep -E > "Capacity|Devices|Size|Manu|Part" | sed -Ez "s/\n\t(M|P)[^:]*: (\S*)/\t\2/g" > | sort', Thank you Mira for pointed out, that the regex is not strict enough on your system. Originally, I limited the strictness as a trade-off to the command length. However, by slightly modifying the output, the command line can be shortened and a higher level of strictness can be achieved: dmidecode -t16,17 | grep -P "^\t(Max[^:]*city|Size|Part)" | sed -Ez "s/\n(\tP[^\n]*)/\1/g" | sort Maximum Capacity: 2 TB Size: 16 GB Part Number: 18ASF2G72PZ-2G6D1 Size: 16 GB Part Number: 18ASF2G72PZ-2G6D1 Size: 16 GB Part Number: 18ASF2G72PZ-2G6D1 Size: 16 GB Part Number: 18ASF2G72PZ-2G6D1 Size: No Module Installed Size: No Module Installed Size: No Module Installed Size: No Module Installed ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 9/9] report: add microcode info to better assess possible system impacts
On Fri, 22 Mar 2024 14:59:33 +0100
Alexander Zeidler wrote:
> * list availability and installation status of `*microcode` packages
> * grep for applied "Early OS Microcode Updates"
> * grep for (un)patched CPU vulnerability messages
>
> Signed-off-by: Alexander Zeidler
> ---
> PVE/Report.pm | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/PVE/Report.pm b/PVE/Report.pm
> index fe497b43..18c554ec 100644
> --- a/PVE/Report.pm
> +++ b/PVE/Report.pm
> @@ -108,6 +108,8 @@ my $init_report_cmds = sub {
> 'dmidecode -t bios -q',
> 'dmidecode -t memory | grep -E
> "Capacity|Devices|Size|Manu|Part" | sed -Ez "s/\n\t(M|P)[^:]*: (\S*)/\t\2/g"
> | sort',
> 'lscpu',
> + 'apt list *microcode 2>/dev/null | column -tL',
While `apt` works really well and its output hasn't changed since I
started using it (wheezy or jessie) - I still want to mention it's output
when piping:
```
WARNING: apt does not have a stable CLI interface. Use with caution in
scripts. ```
potentially consider either using our code directly or switching to
`dpkg -l`?
(but as said `apt` has been pretty stable, and we simply dump the output -
so probably the warning is not too relevant here)
> + 'dmesg | grep -i "microcode\|vuln"',
> 'lspci -nnk',
> ],
> },
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH cluster] ssh: default to 4096 bit keys when generating
Am 21.12.23 um 10:46 schrieb Fabian Grünbichler:
> Signed-off-by: Fabian Grünbichler
Reviewed-by: Fiona Ebner
Tested-by: Fiona Ebner
by removing my existing ones, attempt to SSH to other node and fail
(just to be sure), running pvecm updatecerts and SSH-ing to other node
with new key.
> ---
> src/PVE/Cluster/Setup.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/Cluster/Setup.pm b/src/PVE/Cluster/Setup.pm
> index 07020d7..4b12bb8 100644
> --- a/src/PVE/Cluster/Setup.pm
> +++ b/src/PVE/Cluster/Setup.pm
> @@ -157,7 +157,7 @@ sub setup_rootsshconfig {
> # create ssh key if it does not exist
> if (! -f $ssh_root_rsa_key_public) {
> mkdir '/root/.ssh/';
> - system ("echo|ssh-keygen -t rsa -N '' -b 2048 -f
> ${ssh_root_rsa_key_private}");
> + system ("echo|ssh-keygen -t rsa -N '' -b 4096 -f
> ${ssh_root_rsa_key_private}");
> }
>
> # create ssh config if it does not exist
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server 3/3] api: include not mapped resources for running vms in migrate preconditions
Am 20.03.24 um 13:51 schrieb Dominik Csapak:
> so that we can show a proper warning in the migrate dialog and check it
> in the bulk migrate precondition check
>
> the unavailable_storages and allowed_nodes should be the same as before
>
> Signed-off-by: Dominik Csapak
> ---
> not super happy with this partial approach, we probably should just
> always return the 'allowed_nodes' and 'not_allowed_nodes' and change
> the gui to handle the running vs not running state?
So not_allowed_nodes can already be returned in both states after this
patch. But allowed nodes still only if not running. I mean, there could
be API users that break if we'd always return allowed_nodes, but it
doesn't sound unreasonable for me to do so. Might even be an opportunity
to structure the code in a bit more straightforward manner.
>
> PVE/API2/Qemu.pm | 27 +++
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8581a529..b0f155f7 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4439,7 +4439,7 @@ __PACKAGE__->register_method({
> not_allowed_nodes => {
> type => 'object',
> optional => 1,
> - description => "List not allowed nodes with additional
> informations, only passed if VM is offline"
> + description => "List not allowed nodes with additional
> informations",
> },
> local_disks => {
> type => 'array',
> @@ -4496,25 +4496,28 @@ __PACKAGE__->register_method({
>
> # if vm is not running, return target nodes where local storage/mapped
> devices are available
> # for offline migration
> + my $checked_nodes = {};
> + my $allowed_nodes = [];
> if (!$res->{running}) {
> - $res->{allowed_nodes} = [];
> - my $checked_nodes =
> PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> + $checked_nodes =
> PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> delete $checked_nodes->{$localnode};
> + }
>
> - foreach my $node (keys %$checked_nodes) {
> - my $missing_mappings = $missing_mappings_by_node->{$node};
> - if (scalar($missing_mappings->@*)) {
> - $checked_nodes->{$node}->{'unavailable-resources'} =
> $missing_mappings;
> - next;
> - }
> + foreach my $node ((keys $checked_nodes->%*, keys
> $missing_mappings_by_node->%*)) {
Style nit: please use 'for' instead of 'foreach'
Like this you might iterate over certain nodes twice and then push them
onto the allowed_nodes array twice.
> + my $missing_mappings = $missing_mappings_by_node->{$node};
> + if (scalar($missing_mappings->@*)) {
> + $checked_nodes->{$node}->{'unavailable-resources'} =
> $missing_mappings;
> + next;
> + }
>
> + if (!$res->{running}) {
> if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
> - push @{$res->{allowed_nodes}}, $node;
> + push $allowed_nodes->@*, $node;
> }
> -
> }
> - $res->{not_allowed_nodes} = $checked_nodes;
> }
> + $res->{not_allowed_nodes} = $checked_nodes if
> scalar(keys($checked_nodes->%*)) || !$res->{running};
Why not return the empty hash if running? The whole post-if is just
covering that single special case.
> + $res->{allowed_nodes} = $allowed_nodes if scalar($allowed_nodes->@*) ||
> !$res->{running};
Nit: Right now, $allowed_nodes can only be non-empty if
!$res->{running}, so the first part of the check is redundant.
>
> my $local_disks = &$check_vm_disks_local($storecfg, $vmconf, $vmid);
> $res->{local_disks} = [ values %$local_disks ];;
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH storage] zfs: fix duplicate word typo in error message
On 30/01/2024 10:11, Fiona Ebner wrote: > Signed-off-by: Fiona Ebner > --- > src/PVE/Storage/ZFSPoolPlugin.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > applied (since a while already), thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server 2/3] migrate: call vm_stop_cleanup after stopping in phase3_cleanup
Am 20.03.24 um 13:51 schrieb Dominik Csapak:
> @@ -1591,12 +1593,10 @@ sub phase3_cleanup {
> $self->{errors} = 1;
> }
>
> -# always deactivate volumes - avoid lvm LVs to be active on several nodes
> -eval {
> - PVE::Storage::deactivate_volumes($self->{storecfg}, $sourcevollist);
> -};
> +# stop with nocheck does not do a cleanup, so do it here with the
> original config
> +eval { PVE::QemuServer::vm_stop_cleanup($self->{storecfg}, $vmid,
> $oldconf, 0) };
The function has more parameters, so this does not set noerr to 0.
> if (my $err = $@) {
> - $self->log('err', $err);
> + $self->log('err', "cleanup for vm failed - $err");
> $self->{errors} = 1;
> }
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index d53e9693..54f73093 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6160,7 +6160,9 @@ sub cleanup_pci_devices {
> }
>
> sub vm_stop_cleanup {
> -my ($storecfg, $vmid, $conf, $keepActive, $apply_pending_changes) = @_;
> +my ($storecfg, $vmid, $conf, $keepActive, $apply_pending_changes,
> $noerr) = @_;
> +
> +$noerr //= 1; # defaults to warning
Not too happy about this, because usually passing undef and 0 for a
"boolean" is the same, but not here. And Perl won't help you. There's
not many callers, maybe just adapt them instead? Should be its own patch
then.
>
> eval {
>
> @@ -6186,7 +6188,13 @@ sub vm_stop_cleanup {
>
> vmconfig_apply_pending($vmid, $conf, $storecfg) if
> $apply_pending_changes;
> };
> -warn $@ if $@; # avoid errors - just warn
> +if (my $err = $@) {
> + if ($noerr) {
> + warn $err;
> + } else {
> + die $err;
> + }
Style nit: we usually have something like
die $err if !$noerr;
warn $err;
which avoids the line bloat.
> +}
> }
>
> # call only in locked context
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server 3/3] api: include not mapped resources for running vms in migrate preconditions
On Wed Mar 20, 2024 at 1:51 PM CET, Dominik Csapak wrote:
> so that we can show a proper warning in the migrate dialog and check it
> in the bulk migrate precondition check
>
> the unavailable_storages and allowed_nodes should be the same as before
>
> Signed-off-by: Dominik Csapak
> ---
> not super happy with this partial approach, we probably should just
> always return the 'allowed_nodes' and 'not_allowed_nodes' and change
> the gui to handle the running vs not running state?
>
> PVE/API2/Qemu.pm | 27 +++
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8581a529..b0f155f7 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4439,7 +4439,7 @@ __PACKAGE__->register_method({
> not_allowed_nodes => {
> type => 'object',
> optional => 1,
> - description => "List not allowed nodes with additional
> informations, only passed if VM is offline"
> + description => "List not allowed nodes with additional
> informations",
"information" has no plural, this should just be "additional
information".
> },
> local_disks => {
> type => 'array',
> @@ -4496,25 +4496,28 @@ __PACKAGE__->register_method({
>
> # if vm is not running, return target nodes where local storage/mapped
> devices are available
> # for offline migration
> + my $checked_nodes = {};
> + my $allowed_nodes = [];
> if (!$res->{running}) {
> - $res->{allowed_nodes} = [];
> - my $checked_nodes =
> PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> + $checked_nodes =
> PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> delete $checked_nodes->{$localnode};
> + }
>
> - foreach my $node (keys %$checked_nodes) {
> - my $missing_mappings = $missing_mappings_by_node->{$node};
> - if (scalar($missing_mappings->@*)) {
> - $checked_nodes->{$node}->{'unavailable-resources'} =
> $missing_mappings;
> - next;
> - }
> + foreach my $node ((keys $checked_nodes->%*, keys
> $missing_mappings_by_node->%*)) {
> + my $missing_mappings = $missing_mappings_by_node->{$node};
> + if (scalar($missing_mappings->@*)) {
> + $checked_nodes->{$node}->{'unavailable-resources'} =
> $missing_mappings;
> + next;
> + }
>
> + if (!$res->{running}) {
> if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
> - push @{$res->{allowed_nodes}}, $node;
> + push $allowed_nodes->@*, $node;
> }
> -
> }
> - $res->{not_allowed_nodes} = $checked_nodes;
> }
> + $res->{not_allowed_nodes} = $checked_nodes if
> scalar(keys($checked_nodes->%*)) || !$res->{running};
> + $res->{allowed_nodes} = $allowed_nodes if scalar($allowed_nodes->@*) ||
> !$res->{running};
>
> my $local_disks = &$check_vm_disks_local($storecfg, $vmconf, $vmid);
> $res->{local_disks} = [ values %$local_disks ];;
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server 1/3] stop cleanup: remove unnecessary tpmstate cleanup
Am 20.03.24 um 13:51 schrieb Dominik Csapak:
> tpmstate0 is already included in `get_vm_volumes`, and our only storage
> plugin that has unmap_volume implemented is the RBDPlugin, where we call
> unmap in `deactivate_volume`. So it's already ummapped by the
> `deactivate_volumes` calls above.
>
> Signed-off-by: Dominik Csapak
> ---
There are third-party storage plugins too. But it's natural to expect
that deactivate_volume() would also remove a mapping for the volume.
OTOH, there is an explicit map_volume() call in start_swtpm() (can't use
librbd), so it's natural to expect an explicit unmap_volume() call.
However, the order of calls right now is
1. activate_volume()
2. map_volume()
3. deactivate_volume()
4. unmap_volume()
I'd rather expect 3 and 4 to be swapped if doing it "properly".
All that said, I think we can try dropping this and risk the, arguably
unlikely, scenario that a third-party plugin breaks. If it really
happens, then we can adapt according to the actual needs.
Acked-by: Fiona Ebner
> PVE/QemuServer.pm | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ef3aee20..d53e9693 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6167,14 +6167,6 @@ sub vm_stop_cleanup {
> if (!$keepActive) {
> my $vollist = get_vm_volumes($conf);
> PVE::Storage::deactivate_volumes($storecfg, $vollist);
> -
> - if (my $tpmdrive = $conf->{tpmstate0}) {
> - my $tpm = parse_drive("tpmstate0", $tpmdrive);
> - my ($storeid, $volname) =
> PVE::Storage::parse_volume_id($tpm->{file}, 1);
> - if ($storeid) {
> - PVE::Storage::unmap_volume($storecfg, $tpm->{file});
> - }
> - }
> }
>
> foreach my $ext (qw(mon qmp pid vnc qga)) {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] ui: pool members: avoid setting request parameter for all edit windows
On 14/03/2024 15:43, Stefan Sterz wrote:
> On Wed Mar 13, 2024 at 9:44 AM CET, Friedrich Weber wrote:
>> Currently, after adding a storage to a pool, opening any edit window
>> will send a GET request with a superfluous `poolid` parameter and
>> cause a parameter verification error in the GUI. This breaks all edit
>> windows of the current session. A workaround is to reload the current
>> browser session.
>>
>> This happens because the `PVE.pool.AddStorage` component inadvertently
>> adds `poolid` to an `extraRequestParams` object that is shared by all
>> instances of `Proxmox.window.Edit`, affecting all edit windows in the
>> current session. Fix this by instead creating a new object that is
>> local to the component.
>>
>> Fixes: cd731902b7a724b1ab747276f9c6343734f1d8cb
>> Signed-off-by: Friedrich Weber
>> ---
>>
>> Notes:
>> To check if we have this problem at other places, I did a quick search
>> for `extraRequestParams` in PVE+PBS: Seems like for all other usages,
>> the object is created fresh already.
>>
>> www/manager6/grid/PoolMembers.js | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/www/manager6/grid/PoolMembers.js
>> b/www/manager6/grid/PoolMembers.js
>> index 75f20cab..61e27dff 100644
>> --- a/www/manager6/grid/PoolMembers.js
>> +++ b/www/manager6/grid/PoolMembers.js
>> @@ -123,7 +123,7 @@ Ext.define('PVE.pool.AddStorage', {
>> me.isAdd = true;
>> me.url = "/pools/";
>> me.method = 'PUT';
>> -me.extraRequestParams.poolid = me.pool;
>> +me.extraRequestParams = { 'poolid': me.pool };
>
> note that you don't need to quote `poolid` here as it doesn't contain
> hyphens as such. quickly grepping over our codebase it seems that not
> quoting keys is preferred if it isn't needed
You're right, thanks for spotting this (my Python upbringing got the
better of me here).
I'll send a v2 that fixes this, and will probably include a patch that
rewrites the other occurrences of `extraRequestParams`.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 1/3] fix #5255: node: wol: add optional bind interface
> On 21.03.2024 18:27 CET Thomas Lamprecht wrote:
>
>
> On 05/03/2024 13:54, Christian Ebner wrote:
> > +'wakeonlan-bind-interface' => {
> > + type => 'string',
> > + description => 'Bind to this interface when sending wake on LAN packet',
> > + format => 'pve-iface',
> > + optional => 1,
> > +},
>
> we could transform the existing "wakeonlan" property into a format string,
> keep the new mac property as default_key there for backwards compat, kinda
> like "acme" is a format string in the same config.
>
> For such a config option that would be IMO fitting and avoid bloating the
> "top-level" format.
Agreed, will send a new version with the suggested changes to the format
string.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 7/9] report: add info of the mainboard in use
If we add more info, then dmidecode -t {1,2,3} might be interesting as
well as those deliver motherboard, system and chassis infos.
If you want to see what you get there, run them on some decent servers
as most consumer boards and systems will not show useful information here
On 2024-03-22 14:59, Alexander Zeidler wrote:
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index 3a81bdb2..f28d7b38 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -104,6 +104,7 @@ my $init_report_cmds = sub {
hardware => {
order => 70,
cmds => [
+ 'cd /sys/devices/virtual/dmi/id; cat board_vendor board_name',
'dmidecode -t bios -q',
'lscpu',
'lspci -nnk',
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 7/9] report: add info of the mainboard in use
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index 3a81bdb2..f28d7b38 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -104,6 +104,7 @@ my $init_report_cmds = sub {
hardware => {
order => 70,
cmds => [
+ 'cd /sys/devices/virtual/dmi/id; cat board_vendor board_name',
'dmidecode -t bios -q',
'lscpu',
'lspci -nnk',
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 9/9] report: add microcode info to better assess possible system impacts
* list availability and installation status of `*microcode` packages
* grep for applied "Early OS Microcode Updates"
* grep for (un)patched CPU vulnerability messages
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index fe497b43..18c554ec 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -108,6 +108,8 @@ my $init_report_cmds = sub {
'dmidecode -t bios -q',
'dmidecode -t memory | grep -E
"Capacity|Devices|Size|Manu|Part" | sed -Ez "s/\n\t(M|P)[^:]*: (\S*)/\t\2/g" |
sort',
'lscpu',
+ 'apt list *microcode 2>/dev/null | column -tL',
+ 'dmesg | grep -i "microcode\|vuln"',
'lspci -nnk',
],
},
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 8/9] report: add info about (un)used memory slots
* to see if a RAM upgrade is slot/capacity-wise possible
* to spot added/replaced RAM that may now be causing issues
# dmidecode -t memory ...
Maximum Capacity: 2 TB
Number Of Devices: 8
Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1
Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1
Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1
Size: 16 GB Micron Technology 18ASF2G72PZ-2G6D1
Size: No Module Installed
Size: No Module Installed
Size: No Module Installed
Size: No Module Installed
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index f28d7b38..fe497b43 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -106,6 +106,7 @@ my $init_report_cmds = sub {
cmds => [
'cd /sys/devices/virtual/dmi/id; cat board_vendor board_name',
'dmidecode -t bios -q',
+ 'dmidecode -t memory | grep -E
"Capacity|Devices|Size|Manu|Part" | sed -Ez "s/\n\t(M|P)[^:]*: (\S*)/\t\2/g" |
sort',
'lscpu',
'lspci -nnk',
],
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 6/9] report: switch `dmidecode` to quiet to omit almost never needed info
like on this system:
# dmidecode -t bios
# dmidecode 3.4
Getting SMBIOS data from sysfs.
SMBIOS 3.0.0 present.
Handle 0x, DMI type 0, 24 bytes
Handle 0x005C, DMI type 13, 22 bytes
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index 505629c7..3a81bdb2 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -104,7 +104,7 @@ my $init_report_cmds = sub {
hardware => {
order => 70,
cmds => [
- 'dmidecode -t bios',
+ 'dmidecode -t bios -q',
'lscpu',
'lspci -nnk',
],
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 2/9] report: add `jobs.cfg` to debug related network/load/backup/etc issues
Suggested-by: Friedrich Weber
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index d4191769..6014f13e 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -35,6 +35,7 @@ my $init_report_cmds = sub {
'pveversion --verbose',
'cat /etc/hosts',
'pvesubscription get',
+ 'cat /etc/pve/jobs.cfg',
'cat /etc/apt/sources.list',
sub { dir2text('/etc/apt/sources.list.d/', '.+\.list') },
sub { dir2text('/etc/apt/sources.list.d/', '.+\.sources') },
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 3/9] report: add list of upgradable packages
* to easily see if APT already knows about old packages in use and
their exact version
* to reconsider asking for applying updates as a first recommendation
if the list is empty and no updates have been released very recently
# apt list --upgradable ...
Listing...
pve-manager testing 8.1.6 amd64 [upgradable from: 8.1.4]
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index 6014f13e..ad5c2aa0 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -36,6 +36,7 @@ my $init_report_cmds = sub {
'cat /etc/hosts',
'pvesubscription get',
'cat /etc/pve/jobs.cfg',
+ 'apt list --upgradable 2>/dev/null | sed "s/\//\t/g" | column
-tL',
'cat /etc/apt/sources.list',
sub { dir2text('/etc/apt/sources.list.d/', '.+\.list') },
sub { dir2text('/etc/apt/sources.list.d/', '.+\.sources') },
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 5/9] report: move `lscpu` & cluster info to more appropriate sections
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index 2c2a5e12..505629c7 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -41,8 +41,6 @@ my $init_report_cmds = sub {
'cat /etc/apt/sources.list',
sub { dir2text('/etc/apt/sources.list.d/', '.+\.list') },
sub { dir2text('/etc/apt/sources.list.d/', '.+\.sources') },
- 'lscpu',
- 'pvesh get /cluster/resources --type node --output-format=yaml',
],
},
'system-load' => {
@@ -96,6 +94,7 @@ my $init_report_cmds = sub {
order => 60,
cmds => [
'pvecm nodes',
+ 'pvesh get /cluster/resources --type node --output-format=yaml',
'pvecm status',
'cat /etc/pve/corosync.conf 2>/dev/null',
'ha-manager status',
@@ -106,6 +105,7 @@ my $init_report_cmds = sub {
order => 70,
cmds => [
'dmidecode -t bios',
+ 'lscpu',
'lspci -nnk',
],
},
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 1/9] report: add kernel command line including boot time
While using `/proc/cmdline` would already provide an initial info for
debugging passthrough and similar, the use of `dmesg` is an easy way
to get the boot date as an absolute value for free (additional to the
relative value in `uptime` from `top`).
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index 53ffdcbb..d4191769 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -31,6 +31,7 @@ my $init_report_cmds = sub {
cmds => [
'hostname',
'date -R',
+ 'dmesg -T | head | grep Command',
'pveversion --verbose',
'cat /etc/hosts',
'pvesubscription get',
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 4/9] report: add `apt-cache policy` to list recognized APT sources
with their details as well as pinned packages. Omit the "origin"
lines, as their value is already visible in the URLs.
# apt-cache policy ...
Package files:
100 /var/lib/dpkg/status
release a=now
500 https://enterprise.proxmox.com/debian/pve bookworm/pve-enterprise amd64
Packages
release o=Proxmox,a=stable,n=bookworm,l=Proxmox VE Enterprise Debian
Repository,c=pve-enterprise,b=amd64
...
Pinned packages:
intel-microcode -> 3.20231114.1~deb12u1 with priority 1234
Signed-off-by: Alexander Zeidler
---
PVE/Report.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/Report.pm b/PVE/Report.pm
index ad5c2aa0..2c2a5e12 100644
--- a/PVE/Report.pm
+++ b/PVE/Report.pm
@@ -37,6 +37,7 @@ my $init_report_cmds = sub {
'pvesubscription get',
'cat /etc/pve/jobs.cfg',
'apt list --upgradable 2>/dev/null | sed "s/\//\t/g" | column
-tL',
+ 'apt-cache policy | grep -vP "^ +origin "',
'cat /etc/apt/sources.list',
sub { dir2text('/etc/apt/sources.list.d/', '.+\.list') },
sub { dir2text('/etc/apt/sources.list.d/', '.+\.sources') },
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server 4/4] api: enable live migration for marked mapped pci devices
Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> They have to be marked as 'live-migration-capable' in the mapping
> config, and the driver and qemu must support it.
>
> For the gui checks, we now return a list of 'mapped-with-live-migration'
> entries in the migration preflight api call too.
>
> Signed-off-by: Dominik Csapak
> ---
> PVE/API2/Qemu.pm | 5 +
> PVE/QemuMigrate.pm | 12
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 4ecaeb91..8581a529 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4453,6 +4453,10 @@ __PACKAGE__->register_method({
> type => 'array',
> description => "List of mapped resources e.g. pci, usb"
> },
> + 'mapped-with-live-migration' => {
> + type => 'array',
> + description => "List of mapped resources that are marked as
> capable of live-migration",
> + },
Should we merge this with 'mapped-resources' for the next major release
(and add a reminder comment here)? I.e. make that return the objects
with the additional information.
> },
> },
> code => sub {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common 2/2] mapping: pci: optionally return the config in 'find_on_current_node'
Am 18.03.24 um 12:18 schrieb Dominik Csapak: > this is useful to get to the config without having to parse it again > You could also adapt the call sites that need it to use PVE::Mapping::PCI::config() and PVE::Mapping::PCI::get_node_mapping() instead of PVE::Mapping::PCI::find_on_current_node(). That would avoid overloading the return value. IMHO find_on_current_node() doesn't sound like it should return the full map config, even if just optionally. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server 3/4] check_local_resources: add more info per mapped device
Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> such as the mapping name and if it's marked for live-migration (pci only)
>
> Signed-off-by: Dominik Csapak
> ---
> PVE/API2/Qemu.pm | 2 +-
> PVE/QemuMigrate.pm | 5 +++--
> PVE/QemuServer.pm | 10 ++
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 497987ff..4ecaeb91 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4516,7 +4516,7 @@ __PACKAGE__->register_method({
> $res->{local_disks} = [ values %$local_disks ];;
>
> $res->{local_resources} = $local_resources;
> - $res->{'mapped-resources'} = $mapped_resources;
> + $res->{'mapped-resources'} = [ map { "$_->{key}" }
> $mapped_resources->@* ];
Or should it become a hash? Then you could use 'keys' instead of map and
a 'key' property.
>
> return $res;
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 8d9b35ae..6fe8157d 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -232,7 +232,7 @@ sub prepare {
> my ($loc_res, $mapped_res, $missing_mappings_by_node) =
> PVE::QemuServer::check_local_resources($conf, 1);
> my $blocking_resources = [];
> for my $res ($loc_res->@*) {
> - if (!grep($res, $mapped_res->@*)) {
Seems like this is currently broken. I.e. it should be $res eq $_
> + if (!grep { $_->{key} eq $res } $mapped_res->@*) {
> push $blocking_resources->@*, $res;
> }
> }
> @@ -246,8 +246,9 @@ sub prepare {
>
> if (scalar($mapped_res->@*)) {
> my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
> + my $mapped_text = join(", ", map { $_->{key} } $mapped_res->@*);
> if ($running) {
> - die "can't migrate running VM which uses mapped devices: " .
> join(", ", $mapped_res->@*) . "\n";
> + die "can't migrate running VM which uses mapped devices: " .
> $mapped_text . "\n";
Style nit: no need for the concatenation anymore, can just use the
variable inside the string.
> } elsif (scalar($missing_mappings->@*)) {
> die "can't migrate to '$self->{node}': missing mapped devices " .
> join(", ", $missing_mappings->@*) . "\n";
> } else {
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 6e2c8052..ef3aee20 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2600,14 +2600,16 @@ sub check_local_resources {
> next if $entry->{host} && $entry->{host} =~ m/^spice$/i;
> if ($entry->{mapping}) {
> $add_missing_mapping->('usb', $k, $entry->{mapping});
> - push @$mapped_res, $k;
> + push @$mapped_res, { key => $k, device => $entry->{mapping} };
Should we name the second key 'name'/'mapping'/'id' instead of 'device'?
> }
> }
> if ($k =~ m/^hostpci/) {
> my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
> - if ($entry->{mapping}) {
> - $add_missing_mapping->('pci', $k, $entry->{mapping});
> - push @$mapped_res, $k;
> + if (my $name = $entry->{mapping}) {
> + $add_missing_mapping->('pci', $k, $name);
> + my $mapped_device = { key => $k, device => $name };
> + $mapped_device->{'live-migration'} = 1 if
> $pci_map->{ids}->{$name}->{'live-migration-capable'};
Style nit: line too long
> + push @$mapped_res, $mapped_device;
> }
> }
> # sockets are safe: they will recreated be on the target side
> post-migrate
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common 1/2] mapping: pci: add 'live-migration-capable' flag to mappings
Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> so that we can decide in qemu-server to allow live-migration.
> the driver and qemu must be capable of that, and it's the
> admins responsibility to know and configure that
>
Nit: "The" and "QEMU" should be capitalized like this. "admins" ->
"admin's". Missing period at the end.
> Mark the option as experimental in the description.
>
> Signed-off-by: Dominik Csapak
> ---
> src/PVE/Mapping/PCI.pm | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
> index 19ace98..0866175 100644
> --- a/src/PVE/Mapping/PCI.pm
> +++ b/src/PVE/Mapping/PCI.pm
> @@ -100,8 +100,16 @@ my $defaultData = {
> maxLength => 4096,
> },
> mdev => {
> + description => "Marks the device(s) as being capable of providing
> mediated devices.",
> type => 'boolean',
> optional => 1,
> + default => 0,
> + },
Above should be its own patch. Most likely, I'm missing it, but where
exactly does the 'mdev' property from the mapping have an effect? Just
in the UI? At least telling from 'qm showcmd', the 'mdev' property for a
'hostpciN' VM config entry will not be ignored even if the mapping has
'mdev=0'. And it's also possible to run 'qm set 112 --hostpci0
mapping=bar,mdev=foo' without any warning if the mapping has 'mdev=0'.
> + 'live-migration-capable' => {
> + description => "Marks the device(s) as being able to be
> live-migrated (Experimental).",
The bit about QEMU and the driver needing to support it should be
mentioned here.
> + type => 'boolean',
> + optional => 1,
> + default => 0,
> },
> map => {
> type => 'array',
> @@ -123,6 +131,7 @@ sub options {
> return {
> description => { optional => 1 },
> mdev => { optional => 1 },
> + 'live-migration-capable' => { optional => 1 },
> map => {},
> };
> }
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server 1/4] usb: fix undef error on string match
Am 18.03.24 um 12:18 schrieb Dominik Csapak:
> '$entry->{host}' can be empty, so we have to check for that before
> doing a regex check, otherwise we get ugly errors in the log
>
> Signed-off-by: Dominik Csapak
applied this one, thanks!
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docs] installation: update link to installing on top of Debian to bookworm version
Seems this just was forgotten, Buster is quite old at this point. Signed-off-by: Christoph Heiss --- pve-installation.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pve-installation.adoc b/pve-installation.adoc index d123cd4..2c034d6 100644 --- a/pve-installation.adoc +++ b/pve-installation.adoc @@ -401,7 +401,7 @@ See Also * link:/wiki/Prepare_Installation_Media[Prepare Installation Media] -* link:/wiki/Install_Proxmox_VE_on_Debian_Buster[Install Proxmox VE on Debian Buster] +* link:/wiki/Install_Proxmox_VE_on_Debian_12_Bookworm[Install Proxmox VE on Debian 12 Bookworm] * link:/wiki/System_Requirements[System Requirements] -- 2.43.1 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH installer] build: run shellcheck as part of `test` step
On 15/03/2024 11:23, Christoph Heiss wrote: > Especially unconfigured.sh is worth checking consistently. > > Running shellcheck also does not really have any notable impact on build > time, so no downside there either. > > Signed-off-by: Christoph Heiss > --- > Makefile| 14 +- > debian/control | 1 + > unconfigured.sh | 3 +++ > xinitrc | 2 ++ > 4 files changed, 15 insertions(+), 5 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH installer] unconfigured: move terminal size setting before starting debug shell
On 12/03/2024 12:59, Christoph Heiss wrote: > Otherwise, when using the serial debug shell, the console size will be > 0x0. This in turn breaks the TUI installer, as it cannot detect the size > properly. > > It also adjust the size to the proper 80x24 instead of 80x25, as > advertised in the log message. > > Signed-off-by: Christoph Heiss > --- > unconfigured.sh | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > applied, with the info and references you provided in your reply massaged into the commit message, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH docs v3 0/5] improve & unify installation(-related) documentation
On 21/03/2024 16:29, Christoph Heiss wrote: > This series in short tries to bring the documentation for the > ISO installation flow and anything related to it in line the with > respective documentation for PMG. As both products use the same > installer (minus small differences such as LVM options and BTRFS > support) and overall same basic system setup, it is worth unifying them. > > There is (of course) still room for improvement, esp. regarding the apt > repo/update section as pointed out by Alex. Still like to get this in > rather sooner than later, to not diverge even more. > > I'll send the appropriate pmg-docs series (see v1 of that [0]) when this > has been finalized & applied, to keep it transparent and easier to > reason about. > > [0] https://lists.proxmox.com/pipermail/pmg-devel/2024-March/002870.html > > Revision history > > v1: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062056.html > v2: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062159.html > > Notable changes v1 -> v2: > * dropped already-applied patches > * slight rewording in some places based on suggestions > > Notable changes v2 -> v3: > * fixed small typo > > Christoph Heiss (5): > package-repos: improve wording partly based on pmg-docs > installation: iso: improve & align wording with pmg-docs > installation: lvm-options: improve & align wording with pmg-docs > installation: zfs-options: improve & align wording with pmg-docs > installation: iso: reflow location and password dialog screenshots > > pve-installation.adoc | 144 ++--- > pve-package-repos.adoc | 26 > 2 files changed, 107 insertions(+), 63 deletions(-) > applied series, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH storage v2] esxi: detect correct os type in 'other' family
On 21/03/2024 10:07, Gabriel Goller wrote: > This patch introduces the conversion table for all possible OS Types > that are in the VMWare 'other' family and sets the pve counterpart. > Our default OS Type is 'linux', so including mappings to 'other' makes > sense. > > Signed-off-by: Gabriel Goller > --- > > v2, thanks @sterzy: > - removed perltidy output > > src/PVE/Storage/ESXiPlugin.pm | 43 +++ > 1 file changed, 39 insertions(+), 4 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH stable-7 qemu] fix #2258: select correct device when removing drive snapshot via QEMU
On 21/03/2024 13:29, Maximiliano Sandoval wrote: > The QMP command needs to be issued for the device where the disk is > currently attached, not for the device where the disk was attached at > the time the snapshot was taken. > > Fixes the following scenario with a disk image for which > do_snapshots_with_qemu() is true (i.e. qcow2 or RBD+krbd=0): > 1. Take snapshot while disk image is attached to a given bus+ID. > 2. Detach disk image. > 3. Attach disk image to a different bus+ID. > 4. Remove snapshot. > > Previously, this would result in an error like: >> blockdev-snapshot-delete-internal-sync' failed - Cannot find >> device=drive-scsi1 nor node_name=drive-scsi1 > > While the $running parameter for volume_snapshot_delete() is planned > to be removed on the next storage plugin APIAGE reset, it currently > causes an immediate return in Storage/Plugin.pm. So passing a truthy > value would prevent removing a snapshot from an unused qcow2 disk that > was still used at the time the snapshot was taken. Thus, and because > some exotic third party plugin might be using it for whatever reason, > it's necessary to keep passing the same value as before. > > Signed-off-by: Fiona Ebner > Signed-off-by: Maximiliano Sandoval > --- > > This is a backport for Proxmox VE 7 of > https://lists.proxmox.com/pipermail/pve-devel/2022-September/054124.html which > was applied at > https://git.proxmox.com/?p=qemu-server.git;a=commit;h=c60586838a4bfbd4f879c509195cbfb1291db443. > If you use `git cherry-pick -xs COMMIT` the -x will add a reference that this was cherry-picked from a commit and the -s adds your S-o-b, which makes it a bit nicer as one can immediately see where it comes from when checking the git log. I amended that info in this time. > I tested this by creating a new Proxmox VE 7.4 VM, creating a new VM inside > with > storage as IDE, taking a snapshot while the VM is running, detach the storage > and add it back as SCSI, then deleting the snapshot. > > PVE/QemuServer.pm | 19 --- > 1 file changed, 12 insertions(+), 7 deletions(-) applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] release a new pve-network package ? (evpn fix)
Hi! On 21/03/2024 09:13, DERUMIER, Alexandre wrote: > a critical bug in evpn with multiple nodes is fixed in git, > https://git.proxmox.com/?p=pve-network.git;a=commit;h=e614da43f13e3c61f9b78ee9984364495eff91b6 > but package is still not released > > I see a lot of user bug report since 4 months about this, like this > recent one: > https://forum.proxmox.com/threads/evpn-sdn-issues-after-upgrade-proxmox-ve-from-7-to-8.131236/post-645618 > > > could it be possible to release à 0.9.6 version? Yes, just uploaded a new version to pvetest – sorry for the wait. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] ui: storage: esxi: check 'skip certificate verification' by default
On 3/22/24 09:46, Thomas Lamprecht wrote: On 22/03/2024 08:29, Dominik Csapak wrote: On 3/21/24 18:07, Thomas Lamprecht wrote: On 20/03/2024 16:39, Dominik Csapak wrote: needing one less step when adding the storage, assuming most esxi certificates are self-signed. Well this makes it insecure by default though? Which is not something I'd just not mention in such a commit message... imho it is very obvious what it does from the commit subject? 'skipping the certificate verification' ? but ok, i can add a sentence more in the description.. as always, the reasoning and why's count a bit more in such cases – making the default insecure is something where a giving a reason for why this is OK is rather a must... ok sorry i think i misunderstood you, yes, the reason should be clear but i thought i did that by saying 'most esxi certs are self-signed' As that was the original reason I ticked it in the first place when pondering between security and convenience... the thought here was that users that make the effort of giving their esxi instances valid certificates, can simply uncheck the checkbox? So can the others?! And that would be pretty obvious if the error message gets passed through like I requested already off list over a week ago.. As again... this is making the connection completely insecure, and users with valid certs won't be notified of that fact "so simply check" it is really not something a user can be aware of if it "works" without enabling basic security.. > and i guess many of the users won't bother doing that for the esxi instances? (e.g. vcenter does not make that distinction, all it does is ask for hostname/ip + password, and cert management seems to be non-trivial) again, not an argument for why we should make it less secure. If we do this I'd rather rename it to "Check Certificate" and have that unticked. ok makes sense, i'd name it 'verify certificate' though to be in line with our realm/metric server wording also should this be only in the frontend, or do we want to reverse the api/config option as well? No, I'd keep it off there, so having the user pass the --skip- option makes it more clear. But tbh. I probably won't apply this in any way, as mentioned there are other ways to actually improve on this, the error message would be a relatively easy first one. ok fine with me ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] ui: storage: esxi: check 'skip certificate verification' by default
On 22/03/2024 08:29, Dominik Csapak wrote: > On 3/21/24 18:07, Thomas Lamprecht wrote: >> On 20/03/2024 16:39, Dominik Csapak wrote: >>> needing one less step when adding the storage, assuming most esxi >>> certificates are self-signed. >> >> Well this makes it insecure by default though? Which is not something >> I'd just not mention in such a commit message... > > imho it is very obvious what it does from the commit subject? > > 'skipping the certificate verification' > > ? > but ok, i can add a sentence more in the description.. as always, the reasoning and why's count a bit more in such cases – making the default insecure is something where a giving a reason for why this is OK is rather a must... >> As that was the original reason I ticked it in the first place >> when pondering between security and convenience... >> > > the thought here was that users that make the effort of giving > their esxi instances valid certificates, can simply uncheck the checkbox? So can the others?! And that would be pretty obvious if the error message gets passed through like I requested already off list over a week ago.. As again... this is making the connection completely insecure, and users with valid certs won't be notified of that fact "so simply check" it is really not something a user can be aware of if it "works" without enabling basic security.. > and i guess many of the users won't bother doing that for the > esxi instances? (e.g. vcenter does not make that distinction, all > it does is ask for hostname/ip + password, and cert management seems > to be non-trivial) again, not an argument for why we should make it less secure. >> If we do this I'd rather rename it to "Check Certificate" and have >> that unticked. > > ok makes sense, i'd name it 'verify certificate' though to be in line > with our realm/metric server wording > > also should this be only in the frontend, or do we want to reverse > the api/config option as well? No, I'd keep it off there, so having the user pass the --skip- option makes it more clear. But tbh. I probably won't apply this in any way, as mentioned there are other ways to actually improve on this, the error message would be a relatively easy first one. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] ui: storage: esxi: check 'skip certificate verification' by default
On 3/21/24 18:07, Thomas Lamprecht wrote: On 20/03/2024 16:39, Dominik Csapak wrote: needing one less step when adding the storage, assuming most esxi certificates are self-signed. Well this makes it insecure by default though? Which is not something I'd just not mention in such a commit message... imho it is very obvious what it does from the commit subject? 'skipping the certificate verification' ? but ok, i can add a sentence more in the description.. As that was the original reason I ticked it in the first place when pondering between security and convenience... the thought here was that users that make the effort of giving their esxi instances valid certificates, can simply uncheck the checkbox? and i guess many of the users won't bother doing that for the esxi instances? (e.g. vcenter does not make that distinction, all it does is ask for hostname/ip + password, and cert management seems to be non-trivial) If we do this I'd rather rename it to "Check Certificate" and have that unticked. ok makes sense, i'd name it 'verify certificate' though to be in line with our realm/metric server wording also should this be only in the frontend, or do we want to reverse the api/config option as well? Even better would be to be able to pass a finger-print, which was our first idea, but Wolfgang found that the esxi python wrapper is to enterprisy to hook into basic TLS validation, and he also rejected proxying.. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
