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

[Fabian Grünbichler]

> Thomas Lamprecht <t.lamprecht at proxmox.com> hat am 21.11.2024 10:23 CET geschrieben:
> 
>  
> 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?

no, the stats are returned if an opt-in flag is set, otherwise encountering protected snapshots/groups makes removal fail. that way, old clients still get the used-to behaviour, but can opt into not treating such cases as fatal error and instead get structured data about the situation as a result.

sync wants to set the flag to get the better behaviour/information, if the remote does support it already (there is no hard requirement for push support other than namespaces). it's purely an ergonomics/UX improvement to only set the flag if supported and handle the returned stats properly depending on that state. I prepared a follow-up commit that guards this based on the version and drops the feature list altogether for now.

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

the protection flag (and notes, and changes to the unprotected part of the manifest after the initial sync) are not (re-)synced at all (neither with pull, nor with push), this is just about what to do if a snapshot/group/namespace has vanished but protection prevents complete removal.

(re)syncing protection status, better handling of verification state in sync context, and other similar things might be nice as an opt-in feature though.
 
> 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.

see above.