[pbs-devel] [PATCH v6 proxmox-backup 12/29] api/api-types: refactor api endpoint version, add api types

[Christian Ebner]

On 11/21/24 10:23, Thomas Lamprecht wrote:
> Am 20.11.24 um 18:34 schrieb Christian Ebner:
>>> Such a feature negotiation makes IMO mostly sense if I can use that to
>>> fallback to some other protocol/enpoint/parameter set transparently while
>>> still honoring what the user told us to do here.
>> In this case we use the feature negotiation to expose and additional
>> parameter to the snapshot/group delete endpoints, so that it behaves
>> differently (no hard failure when protected snapshots are present,
> 
> Hmm, I'm a bit torn, I can get where you come from, but this is a bit
> bigger change in terms of how we handled these in the past, and naturally
> a permanent commitment.
> 
> A "classic" alternative could be e.g. to expose it in the sync job and
> switch the default value for new jobs with the next major release.
> 
> I have some concerns about some feature explosion over the midterm if used
> at this level which can lead to rather odd effects for users, e.g. if one
> setup behaves very different than another but same job settings is used.
> Explicit settings and errors might not be _that_ convenient, but they are
> very telling and easy.
> 
> That said, do not take this as blocking this outright, maybe someone else
> can also share their opnion on this (or if you got further arguments of
> why my concerns are not warranted I'm obv. happy to hear these too)
> 
>> return delete stats). Without the feature exposed, the previous behavior
> 
> The stats are always returned now?

Yes, sorry that was incorrectly stated by me. Only the failure mode is 
handled differently, as already clarified by Fabian in his reply. But 
the stats are returned unconditionally on success.

> Am 20.11.24 um 18:34 schrieb Christian Ebner:
>>> But if we ignore the need then yes, feature lists might be a bit nicer, they
>>> decouple versioning and provide more semantic meaning on their own, that IME
>>> reduces error-potential to hold them wrong.
>> I would argue in favor of the feature list here, as this makes it:
>> - easier to see from the context what is needed
>> - independent of version bumps
> 
> Albeit, for the PDM we will go for simple version matching to know what
> APIs can be used, as we normally try to batch bigger changes at major
> releases, and for bigger new features minor releases work fine too.
> We can naturally do this for PBS and do not have to then use that paradigm
> everywhere, so it's not coupled, as IMO maintaining feature lists over many
> releases and that over multiple product is not something I want to do for PDM,
> albeit it's a bit more gut feeling, backed by some experience but still, I
> certainly to not assume I'm right, this is far from black and white.

Well, that is something I did not consider at all! So with that 
viewpoint, adding this to PBS specifically is surely not the best way. 
As discussed with Fabain off list, version based matching will be the 
best way forward here, and dropping the incompatibility check once EOL 
is reached.

> 
> Something tangentially related:
> 
> In general, it might be also worth thinking about how the protection flag can
> be better synced – FWICT it's now set if the source has it set and then never
> will get unset manually anymore? Remembering the source of the flag (i.e.,
> sync from remote vs local api) could be an option to differentiate here when
> it's OK to clear on sync transiently again (probably guarded as option in the
> job). But here I'm a bit more distanced from the matter than you are, I'll need
> to think a bit more about this all.
> 
> For now maybe order the whole API feature thing towards the end of the series
> and we can still commit all earlier patches already and decide on this a
> (short) time later.

Not sure if I correctly interpreted you rational here.
As Fabian mentioned, the additional parameter only included in the api 
calls is not to handle how we sync the flag, but rather how to act in 
case the sync jobs should prune vanished snapshots/groups from the remote.