backup: Fix docs and add schema for LUKS encrypted backups

[PATCH 1/2] docs: backup: Remove references to push backup to network disk

Posted by Peter Krempa 5 years, 10 months ago

It was never implemented and for now I don't think there's demand to do
it. Remove the reference.

https://bugzilla.redhat.com/show_bug.cgi?id=1812100

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 docs/formatbackup.html.in | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
index 55acd13ddc..87744bac98 100644
--- a/docs/formatbackup.html.in
+++ b/docs/formatbackup.html.in
@@ -97,12 +97,11 @@
               <dt><code>type</code></dt>
               <dd>A mandatory attribute to describe the type of the
                 disk, except when <code>backup='no'</code> is
-                used. Valid values include <code>file</code>,
-                <code>block</code>, or <code>network</code>.
+                used. Valid values include <code>file</code>, or
+                <code>block</code>.
                 Similar to a disk declaration for a domain, the choice of type
                 controls what additional sub-elements are needed to describe
-                the destination (such as <code>protocol</code> for a
-                network destination).</dd>
+                the destination.
               <dt><code>target</code></dt>
               <dd>Valid only for push mode backups, this is the
                 primary sub-element that describes the file name of
-- 
2.26.0

Re: [PATCH 1/2] docs: backup: Remove references to push backup to network disk

Posted by Eric Blake 5 years, 9 months ago

On 4/14/20 4:22 AM, Peter Krempa wrote:
> It was never implemented and for now I don't think there's demand to do
> it. Remove the reference.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1812100
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   docs/formatbackup.html.in | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
> index 55acd13ddc..87744bac98 100644
> --- a/docs/formatbackup.html.in
> +++ b/docs/formatbackup.html.in
> @@ -97,12 +97,11 @@
>                 <dt><code>type</code></dt>
>                 <dd>A mandatory attribute to describe the type of the
>                   disk, except when <code>backup='no'</code> is
> -                used. Valid values include <code>file</code>,
> -                <code>block</code>, or <code>network</code>.
> +                used. Valid values include <code>file</code>, or
> +                <code>block</code>.

I think we should implement block, rather than delete it.  It matters 
for the same reason that it matters in the destination of block copy: if 
you want to set a highest-byte watermark threshold (to be warned by qemu 
when it is time to resize the disk larger), you NEED a block device, not 
a file.  But libvirt treats all <disk type='file'> as files, even when 
opening /path/to/device; you HAVE to use <disk type='block'> when 
specifying a block device to get the behaviors needed for handling it as 
a block device rather than a file.

>                   Similar to a disk declaration for a domain, the choice of type
>                   controls what additional sub-elements are needed to describe
> -                the destination (such as <code>protocol</code> for a
> -                network destination).</dd>
> +                the destination.
>                 <dt><code>target</code></dt>
>                 <dd>Valid only for push mode backups, this is the
>                   primary sub-element that describes the file name of
> 

I'm inclined to NACK this patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/2] docs: backup: Remove references to push backup to network disk

Posted by Peter Krempa 5 years, 9 months ago

On Tue, Apr 14, 2020 at 12:36:56 -0500, Eric Blake wrote:
> On 4/14/20 4:22 AM, Peter Krempa wrote:
> > It was never implemented and for now I don't think there's demand to do
> > it. Remove the reference.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1812100
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >   docs/formatbackup.html.in | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
> > index 55acd13ddc..87744bac98 100644
> > --- a/docs/formatbackup.html.in
> > +++ b/docs/formatbackup.html.in
> > @@ -97,12 +97,11 @@
> >                 <dt><code>type</code></dt>
> >                 <dd>A mandatory attribute to describe the type of the
> >                   disk, except when <code>backup='no'</code> is
> > -                used. Valid values include <code>file</code>,
> > -                <code>block</code>, or <code>network</code>.
> > +                used. Valid values include <code>file</code>, or
> > +                <code>block</code>.
> 
> I think we should implement block, rather than delete it.  It matters for
> the same reason that it matters in the destination of block copy: if you

I'm deleting 'network'. Block is implemented, working and tested.

> want to set a highest-byte watermark threshold (to be warned by qemu when it
> is time to resize the disk larger), you NEED a block device, not a file.

You can do this on a file too.

> But libvirt treats all <disk type='file'> as files, even when opening
> /path/to/device; you HAVE to use <disk type='block'> when specifying a block
> device to get the behaviors needed for handling it as a block device rather
> than a file.
> 
> >                   Similar to a disk declaration for a domain, the choice of type
> >                   controls what additional sub-elements are needed to describe
> > -                the destination (such as <code>protocol</code> for a
> > -                network destination).</dd>
> > +                the destination.
> >                 <dt><code>target</code></dt>
> >                 <dd>Valid only for push mode backups, this is the
> >                   primary sub-element that describes the file name of
> > 
> 
> I'm inclined to NACK this patch.

Wouldn't mean that much since it's necessary to add schema if you want
it.

> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/2] docs: backup: Remove references to push backup to network disk

Posted by Eric Blake 5 years, 9 months ago

On 4/14/20 2:06 PM, Peter Krempa wrote:
> On Tue, Apr 14, 2020 at 12:36:56 -0500, Eric Blake wrote:
>> On 4/14/20 4:22 AM, Peter Krempa wrote:
>>> It was never implemented and for now I don't think there's demand to do
>>> it. Remove the reference.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1812100
>>>
>>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>>> ---
>>>    docs/formatbackup.html.in | 7 +++----
>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
>>> index 55acd13ddc..87744bac98 100644
>>> --- a/docs/formatbackup.html.in
>>> +++ b/docs/formatbackup.html.in
>>> @@ -97,12 +97,11 @@
>>>                  <dt><code>type</code></dt>
>>>                  <dd>A mandatory attribute to describe the type of the
>>>                    disk, except when <code>backup='no'</code> is
>>> -                used. Valid values include <code>file</code>,
>>> -                <code>block</code>, or <code>network</code>.
>>> +                used. Valid values include <code>file</code>, or
>>> +                <code>block</code>.
>>
>> I think we should implement block, rather than delete it.  It matters for
>> the same reason that it matters in the destination of block copy: if you
> 
> I'm deleting 'network'. Block is implemented, working and tested.

Then shame on me for misreading the patch.

Okay, we may someday add network, but for now making the documentation 
match what we have as existing implementation makes sense.

> 
>> want to set a highest-byte watermark threshold (to be warned by qemu when it
>> is time to resize the disk larger), you NEED a block device, not a file.
> 
> You can do this on a file too.

I seem to recall differences in how the two behave at the qemu level; 
but it doesn't affect this patch if we have the means for telling 
libvirt which of the two (file or block) we meant.


>> I'm inclined to NACK this patch.
> 
> Wouldn't mean that much since it's necessary to add schema if you want
> it.

NACK withdrawn; you've convinced me that the patch (dropping 'network', 
not 'block') is reasonable for matching what we have.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org