[pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type

[pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type

[Dominik Csapak]

by converting the bool into an option, otherwise having the options not
set at all will fail the unload while deserializing with
'eject-before-unload is not optional'

Also if we can automatically decide this in the future, we can now
detect if the option was explicitely set or not.

Fixes: 66402cdc ("fix #4904: tape changer: add option to eject before unload")
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-api-types/src/tape/changer.rs | 17 ++++++++++-------
 src/tape/changer/mod.rs           |  2 +-
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/tape/changer.rs
index e3cf27c1..9e36b12e 100644
--- a/pbs-api-types/src/tape/changer.rs
+++ b/pbs-api-types/src/tape/changer.rs
@@ -39,18 +39,21 @@ Import/Export, i.e. any media in those slots are considered to be
 .format(&ApiStringFormat::PropertyString(&SLOT_ARRAY_SCHEMA))
 .schema();
 
-fn is_false(b: &bool) -> bool {
-    !b
-}
-
-#[api]
+#[api(
+    properties: {
+        "eject-before-unload": {
+            optional: true,
+            default: false,
+        },
+    },
+)]
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "kebab-case")]
 /// Options for Changers
 pub struct ChangerOptions {
-    #[serde(default, skip_serializing_if = "is_false")]
+    #[serde(skip_serializing_if = "Option::is_none")]
     /// if set to true, tapes are ejected manually before unloading
-    pub eject_before_unload: bool,
+    pub eject_before_unload: Option<bool>,
 }
 
 pub const CHANGER_OPTIONS_STRING_SCHEMA: Schema = StringSchema::new("Changer options")
diff --git a/src/tape/changer/mod.rs b/src/tape/changer/mod.rs
index df63f6f8..9d90e29d 100644
--- a/src/tape/changer/mod.rs
+++ b/src/tape/changer/mod.rs
@@ -433,7 +433,7 @@ impl MediaChange for MtxMediaChanger {
                 .parse_property_string(self.config.options.as_deref().unwrap_or_default())?,
         )?;
 
-        if options.eject_before_unload {
+        if options.eject_before_unload.unwrap_or(false) {
             let file = open_lto_tape_device(&self.drive.path)?;
             let mut handle = LtoTapeHandle::new(file)?;
 
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type

[Thomas Lamprecht]

Am 13/12/2023 um 11:11 schrieb Dominik Csapak:
> by converting the bool into an option, otherwise having the options not
> set at all will fail the unload while deserializing with
> 'eject-before-unload is not optional'
> 
> Also if we can automatically decide this in the future, we can now
> detect if the option was explicitely set or not.
> 
> Fixes: 66402cdc ("fix #4904: tape changer: add option to eject before unload")
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/tape/changer.rs | 17 ++++++++++-------
>  src/tape/changer/mod.rs           |  2 +-
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/tape/changer.rs
> index e3cf27c1..9e36b12e 100644
> --- a/pbs-api-types/src/tape/changer.rs
> +++ b/pbs-api-types/src/tape/changer.rs
> @@ -39,18 +39,21 @@ Import/Export, i.e. any media in those slots are considered to be
>  .format(&ApiStringFormat::PropertyString(&SLOT_ARRAY_SCHEMA))
>  .schema();
>  
> -fn is_false(b: &bool) -> bool {
> -    !b
> -}
> -
> -#[api]
> +#[api(
> +    properties: {
> +        "eject-before-unload": {
> +            optional: true,
> +            default: false,
> +        },
> +    },
> +)]
>  #[derive(Serialize, Deserialize)]
>  #[serde(rename_all = "kebab-case")]
>  /// Options for Changers
>  pub struct ChangerOptions {

semi-related to your change, but as I talked with Dietmar off-list about this and
thus checked it out a bit more closely:

As this is already on the changer section type it might make more sense to just have
the "eject-before-unload" option there directly though.

As having a property-string here seems a bit odd, we mostly use them if we want to
configure more things at once on a single (sub)-subject, like network for VMs, but
if you'd have this as "plain" option for the change section-type, it would already
affect the correct subject.

If we get many more such options in the future, we can always move them into a
property-string grouping for a next major release, but tbh. I would be slightly
surprised if it's more than a handful such new options in the next decade.
And even if, as long as they affect the changer, not a sub-subject, it can be
still fine to have any such new option also standalone on the top-level

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type

[Dominik Csapak]

On 12/14/23 08:46, Thomas Lamprecht wrote:
> Am 13/12/2023 um 11:11 schrieb Dominik Csapak:
>> by converting the bool into an option, otherwise having the options not
>> set at all will fail the unload while deserializing with
>> 'eject-before-unload is not optional'
>>
>> Also if we can automatically decide this in the future, we can now
>> detect if the option was explicitely set or not.
>>
>> Fixes: 66402cdc ("fix #4904: tape changer: add option to eject before unload")
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   pbs-api-types/src/tape/changer.rs | 17 ++++++++++-------
>>   src/tape/changer/mod.rs           |  2 +-
>>   2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/pbs-api-types/src/tape/changer.rs b/pbs-api-types/src/tape/changer.rs
>> index e3cf27c1..9e36b12e 100644
>> --- a/pbs-api-types/src/tape/changer.rs
>> +++ b/pbs-api-types/src/tape/changer.rs
>> @@ -39,18 +39,21 @@ Import/Export, i.e. any media in those slots are considered to be
>>   .format(&ApiStringFormat::PropertyString(&SLOT_ARRAY_SCHEMA))
>>   .schema();
>>   
>> -fn is_false(b: &bool) -> bool {
>> -    !b
>> -}
>> -
>> -#[api]
>> +#[api(
>> +    properties: {
>> +        "eject-before-unload": {
>> +            optional: true,
>> +            default: false,
>> +        },
>> +    },
>> +)]
>>   #[derive(Serialize, Deserialize)]
>>   #[serde(rename_all = "kebab-case")]
>>   /// Options for Changers
>>   pub struct ChangerOptions {
> 
> semi-related to your change, but as I talked with Dietmar off-list about this and
> thus checked it out a bit more closely:
> 
> As this is already on the changer section type it might make more sense to just have
> the "eject-before-unload" option there directly though.
> 
> As having a property-string here seems a bit odd, we mostly use them if we want to
> configure more things at once on a single (sub)-subject, like network for VMs, but
> if you'd have this as "plain" option for the change section-type, it would already
> affect the correct subject.
> 
> If we get many more such options in the future, we can always move them into a
> property-string grouping for a next major release, but tbh. I would be slightly
> surprised if it's more than a handful such new options in the next decade.
> And even if, as long as they affect the changer, not a sub-subject, it can be
> still fine to have any such new option also standalone on the top-level
> 


mhmm i modeled it after the 'tuning' options of the datastore.
maybe it was named badly, and the 'options' should be replaced by 
'quirks' (as in, changer quirks that only some people need, like
the datastore tuning options)

but if you both want a more straight forward option directly in
the changer config, then it's also fine with me (i just
did not want to pollute the changer config with rather
specific quirk workarounds, and I doubt this is
the last of them, even though we don't see such things often)

just tell me if i should rename it to something else (like quirks)
or if i should put it directly in the changer config, and i'll send
the patches for it + docs update

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type

[Dietmar Maurer]

> mhmm i modeled it after the 'tuning' options of the datastore.

Which is also clumsy and not really required... I would have used normal options instead...

> maybe it was named badly, and the 'options' should be replaced by 
> 'quirks' (as in, changer quirks that only some people need, like
> the datastore tuning options)
> 
> but if you both want a more straight forward option directly in
> the changer config, then it's also fine with me (i just

> did not want to pollute the changer config with rather
> specific quirk workarounds, and I doubt this is
> the last of them, even though we don't see such things often)

The changer config only has a few option, so we are far away from
a polluted config.

> just tell me if i should rename it to something else (like quirks)
> or if i should put it directly in the changer config, and i'll send
> the patches for it + docs update

Yes, please use normal options unless there is a real requirement to
use a property string.

Re: [pbs-devel] [PATCH proxmox-backup] tape: fix 'eject-before-unload' api type

[Thomas Lamprecht]

Am 14/12/2023 um 08:52 schrieb Dominik Csapak:
> mhmm i modeled it after the 'tuning' options of the datastore.
> maybe it was named badly, and the 'options' should be replaced by 
> 'quirks' (as in, changer quirks that only some people need, like
> the datastore tuning options)

'quirks' would be already a lot better than the overly generic 'options',
but IMO the changer scope of possible functionality and how, and in which
context, they can be used is quite a bit smaller than the scope of a
datastore, which can be involved in many different actions/jobs.  For
tuning's we also already knew a few specific things that could be done, so
there it was IMO indeed slightly better to go for a property string.

And sure, this here isn't a total clear-cut where having it in a property
string is just plain wrong, it definitively isn't just wrong, but IMO, as
long as we do not have more options of this category for changers it's too
hard to future-proof this now already, and in that case the simpler way (for
user and API maintenance) is having this an explicit separate option, not a
nested one.

> but if you both want a more straight forward option directly in
> the changer config, then it's also fine with me (i just
> did not want to pollute the changer config with rather
> specific quirk workarounds, and I doubt this is
> the last of them, even though we don't see such things often)

Yeah, I also think that there will be some, but I'd find it odd if there are
many. And if many have to be added in a short period it's probably due to
some standard change (e.g., LTO 10 doing/requiring very new weird stuff),
and in that case it might be better to have that set of new options in a
option group that is specific to that change, for the previous example that
could be grouped to something like `lto: [<version>=]10[,<other-options>]` 

And whatever it will be, once it isn't a pipe dream prediction anymore, but
became actual reality, we can way better choose; and if it then means moving
those to a "quirks" property to avoid bloat/whatever, then you were right
and we can change it to that (with some, but not all to big, backward compat
work). 
But for this option I don't see the hard evidence that just would suggest
now that a relative generic 'quirks' will be the best fit.

> just tell me if i should rename it to something else (like quirks)
> or if i should put it directly in the changer config, and i'll send
> the patches for it + docs update

I'd favor having this moved out directly in the change config for now.