Re: [PATCH pve-storage 7/7] api: add /nodes/<node>/storage/<storage>/instance-id route

[Christian Ebner <c.ebner@proxmox.com>]

On 4/15/26 10:40 AM, Lukas Wagner wrote:
> On Tue Apr 14, 2026 at 4:38 PM CEST, Christian Ebner wrote:
>> On 4/13/26 2:11 PM, Lukas Wagner wrote:
>>> This API allows us to retrieve the instance ID of PBS storages. This is
>>> useful to establish a mapping between PBS storage and PBS remotes in
>>> PDM.
>>>
>>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>>> ---
>>>
>>> Notes:
>>>       Maybe we want to make this more general and just call it /id or
>>>       something, in case we want to use a similar mechanism for other storage
>>>       types as well?
>>>       
>>>       Also could make sense to cache the response.
>>>
>>>    src/PVE/API2/Storage/Status.pm | 50 +++++++++++++++++++++++++++++++++-
>>>    1 file changed, 49 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
>>> index 8225c3a..52aa673 100644
>>> --- a/src/PVE/API2/Storage/Status.pm
>>> +++ b/src/PVE/API2/Storage/Status.pm
>>> @@ -14,7 +14,7 @@ use PVE::JSONSchema qw(get_standard_option);
>>>    use PVE::RESTHandler;
>>>    use PVE::RPCEnvironment;
>>>    use PVE::RRD;
>>> -use PVE::Tools qw(run_command);
>>> +use PVE::Tools qw(run_command extract_param);
>>>    
>>>    use PVE::API2::Storage::Content;
>>>    use PVE::API2::Storage::FileRestore;
>>> @@ -308,6 +308,7 @@ __PACKAGE__->register_method({
>>>                { subdir => 'download-url' },
>>>                { subdir => 'file-restore' },
>>>                { subdir => 'import-metadata' },
>>> +            { subdir => 'instance-id' },
>>>                { subdir => 'oci-registry-pull' },
>>>                { subdir => 'prunebackups' },
>>>                { subdir => 'rrd' },
>>> @@ -1118,4 +1119,51 @@ __PACKAGE__->register_method({
>>>        },
>>>    });
>>>    
>>> +__PACKAGE__->register_method({
>>> +    name => 'instance_id',
>>> +    path => '{storage}/instance-id',
>>> +    method => 'GET',
>>> +    description => "Return unique storage instance ID (PBS only)",
>>> +    permissions => {
>>> +        check => ['perm', '/storage/{storage}', ['Datastore.Audit']],
>>> +    },
>>> +    protected => 1,
>>> +    proxyto => 'node',
>>> +    parameters => {
>>> +        additionalProperties => 0,
>>> +        properties => {
>>> +            node => get_standard_option('pve-node'),
>>> +            storage => get_standard_option('pve-storage-id'),
>>> +        },
>>> +    },
>>> +    returns => {
>>> +        type => "object",
>>> +        properties => {
>>> +            'instance-id' => {
>>> +                type => 'string',
>>> +                description => 'Unique instance ID',
>>> +            },
> 
> With the changes described below, I'd
>    - rename the property to 'pbs-instance-id'
>    - make it optional (as a preparation for other storages that could
>      have some 'identity info' that would be stored in a different key)
> 
>>> +        },
>>> +    },
>>> +    code => sub {
>>> +        my ($param) = @_;
>>> +
>>> +        my $rpcenv = PVE::RPCEnvironment::get();
>>> +        my $user = $rpcenv->get_user();
>>> +
>>> +        my $storeid = extract_param($param, 'storage');
>>> +
>>> +        my $cfg = PVE::Storage::config();
>>> +        my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>>> +
>>> +        raise_param_exc({ 'storage' => "Only PBS storages supported." })
>>> +            if $scfg->{type} ne 'pbs';
>>> +
>>> +        my $client = PVE::PBSClient->new($scfg, $storeid);
>>> +        my $ret = $client->get_instance_id();
>>
>> nit: same as on previous patches, the client part should be called
>> get_pbs_instance_id() to reflect that this stems from the server.
>>
>> I do agree however that this API endpoint name for PVE should be chosen
>> as generic as possible for better future extensibility.
>>
> 
> I think I'm going to rename the API path to `identity`, as this seems to
> be generic enough to be useful for other storages as well. Maybe the
> method in PBSClient could then be called `get_server_identity`?
> 
> What do you think?

Yes, this definitely is a more fitting name with less potential for 
confusion on the client side.