Re: [PATCH pve-manager v4 09/17] api: add endpoint querying available CPU flags cluster-wide

[Fiona Ebner <f.ebner@proxmox.com>]

Am 30.04.26 um 6:00 PM schrieb Arthur Bied-Charreton:
> Add endpoint at `/cluster/qemu/cpu-flags` allowing to query the CPU
> flags available on the whole cluster, filtered by acceleration type
> (kvm/tcg).
> 
> For each flag, a list of nodes supporting it is returned. This gives an
> accurate picture of which flags will actually be accepted by QEMU when
> starting a VM.
> 
> Currently only return available flags for x86_64, see documentation of
> query_available_cpu_flags.
> 
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  PVE/API2/Cluster.pm               |  7 ++++
>  PVE/API2/Cluster/Makefile         |  4 +-
>  PVE/API2/Cluster/Qemu.pm          | 41 +++++++++++++++++++
>  PVE/API2/Cluster/Qemu/CPUFlags.pm | 68 +++++++++++++++++++++++++++++++
>  PVE/API2/Cluster/Qemu/Makefile    | 17 ++++++++
>  5 files changed, 136 insertions(+), 1 deletion(-)
>  create mode 100644 PVE/API2/Cluster/Qemu.pm
>  create mode 100644 PVE/API2/Cluster/Qemu/CPUFlags.pm
>  create mode 100644 PVE/API2/Cluster/Qemu/Makefile
> 
> diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
> index 5a663b79..c25b8e32 100644
> --- a/PVE/API2/Cluster.pm
> +++ b/PVE/API2/Cluster.pm
> @@ -27,6 +27,7 @@ use PVE::API2::Backup;
>  use PVE::API2::Cluster::BackupInfo;
>  use PVE::API2::Cluster::BulkAction;
>  use PVE::API2::Cluster::Ceph;
> +use PVE::API2::Cluster::Qemu;

Nit: while the existing list is also not ordered, you could still put
the new one in the correct position

>  use PVE::API2::Cluster::Mapping;
>  use PVE::API2::Cluster::Jobs;
>  use PVE::API2::Cluster::MetricServer;
> @@ -59,6 +60,11 @@ __PACKAGE__->register_method({
>      path => 'notifications',
>  });
>  
> +__PACKAGE__->register_method({
> +    subclass => "PVE::API2::Cluster::Qemu",
> +    path => 'qemu',
> +});
> +
>  __PACKAGE__->register_method({
>      subclass => "PVE::API2::ClusterConfig",
>      path => 'config',
> @@ -157,6 +163,7 @@ __PACKAGE__->register_method({
>              { name => 'bulk-action' },
>              { name => 'ceph' },
>              { name => 'config' },
> +            { name => 'qemu' },

Nit: order

>              { name => 'firewall' },
>              { name => 'ha' },
>              { name => 'jobs' },
> diff --git a/PVE/API2/Cluster/Makefile b/PVE/API2/Cluster/Makefile
> index 6cffe4c9..fd33e31d 100644
> --- a/PVE/API2/Cluster/Makefile
> +++ b/PVE/API2/Cluster/Makefile
> @@ -1,13 +1,15 @@
>  include ../../../defines.mk
>  
>  SUBDIRS=Mapping		\
> -	BulkAction
> +	BulkAction 		\
> +	Qemu
>  
>  # for node independent, cluster-wide applicable, API endpoints
>  # ensure we do not conflict with files shipped by pve-cluster!!
>  PERLSOURCE= 			\
>  	BackupInfo.pm		\
>  	BulkAction.pm		\
> +	Qemu.pm		\

Nit: order
Nit: the backslash is not aligned with the others

>  	MetricServer.pm		\
>  	Mapping.pm		\
>  	Notifications.pm		\
> diff --git a/PVE/API2/Cluster/Qemu.pm b/PVE/API2/Cluster/Qemu.pm
> new file mode 100644
> index 00000000..ed37fd20
> --- /dev/null
> +++ b/PVE/API2/Cluster/Qemu.pm
> @@ -0,0 +1,41 @@
> +package PVE::API2::Cluster::Qemu;
> +
> +use v5.36;
> +
> +use PVE::API2::Cluster::Qemu::CPUFlags;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> +    subclass => "PVE::API2::Cluster::Qemu::CPUFlags",
> +    path => "cpu-flags",
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "Cluster index.",

Should have its own description, e.g. Cluster-wide QEMU(-related?) index

> +    permissions => { user => 'all' },
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => {},
> +    },
> +    returns => {
> +        type => 'array',
> +        items => {
> +            type => "object",
> +            properties => {},
> +        },
> +        links => [{ rel => 'child', href => "{name}" }],
> +    },
> +    code => sub {
> +        my ($param) = @_;
> +
> +        return [
> +            { name => 'cpu-flags' },
> +        ];
> +    },
> +});
> +
> +1;
> diff --git a/PVE/API2/Cluster/Qemu/CPUFlags.pm b/PVE/API2/Cluster/Qemu/CPUFlags.pm
> new file mode 100644
> index 00000000..a3328e85
> --- /dev/null
> +++ b/PVE/API2/Cluster/Qemu/CPUFlags.pm
> @@ -0,0 +1,68 @@
> +package PVE::API2::Cluster::Qemu::CPUFlags;
> +
> +use v5.36;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RESTHandler;
> +use PVE::Tools qw(extract_param);
> +
> +use PVE::QemuServer::Helpers;
> +use PVE::QemuServer::CPUFlags;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "List of available CPU flags. Returns an empty list for 'aarch64', "

Style nit: the space should be on the next line

> +        . "PVE does not currently ship a flags list for it.",

Nit: for user-facing strings, it should be "Proxmox VE". Users should
not be concerned with the implementation details, I'd just mention it's
not currently implemented for aarch64 and returns an empty list.

> +    permissions => { user => 'all' },

Should be restricted to auditors.