Re: [Qemu-devel] [PATCH v4 3/6] add backup related monitor commands

[Stefan Hajnoczi]

On Thu, Feb 21, 2013 at 06:21:32AM +0000, Dietmar Maurer wrote:
> > > +    /* add configuration file to archive */
> > > +    if (has_config_file) {
> > > +        char *cdata = NULL;
> > > +        gsize clen = 0;
> > > +        GError *err = NULL;
> > > +        if (!g_file_get_contents(config_file, &cdata, &clen, &err)) {
> > > +            error_setg(errp, "unable to read file '%s'", config_file);
> > > +            goto err;
> > > +        }
> > > +
> > > +        const char *basename = g_path_get_basename(config_file);
> > > +        if (driver->register_config_cb(writer, basename, cdata, clen) < 
> > > 0) {
> > > +            error_setg(errp, "register_config failed");
> > > +            g_free(cdata);
> > > +            goto err;
> > > +        }
> > > +        g_free(cdata);
> > > +    }
> > 
> > This is doing too much inside QEMU.
> > 
> > First off, when QEMU is sandboxed or run with SELinux, we cannot expect to 
> > be
> > able to access arbitrary files.  This is why new QMP commands of this sort
> > usually support file descriptor passing - then a more privileged component 
> > can
> > give QEMU access.
> 
> We can allow to pass fd for that later (if someone really uses it that way).
> 
> > g_file_get_contents() hangs the VM if the file is over NFS while the server 
> > is
> > down.  This is bad for reliability.
> 
> The solution is to not pass a path which is on NFS. But that is up to the 
> management tool.
>  
> > Management tools (proxmox, libvirt, or something else) handle the VM
> > configuration.  It may not be a single file.  Saving external VM 
> > configuration is
> > out of scope for QEMU.
> 
> Backup includes configuration, so you need a way to save that.
> But I do not really get your suggestion - creating a backup without 
> configuration
> does not make much sense?
> 
> In future, we can allow to pass multiple config files - the vma archive 
> format can
> already handle that.

My point is that QEMU has no business dealing with the management tool's
VM configuration file.  And I think the management tool-specific archive
format also shouldn't be in QEMU.

How will a proxmox user restore a VMA file generated with oVirt since
the configuration file comes from the management layer?  They can't
because the VMA is specific to the management layer and should be
handled there, not inside QEMU.

QEMU must provide the mechanism for point-in-time backups of block
devices - your backup block job implements this.

Where I disagree with this patch series is that you put the management
tool-specific archive format writer into QEMU.  That is outside the
scope of QEMU.

The management tool should tell QEMU to take the backups of block
devices and do a live migration to file.

The management tool can use a NBD server if it wants to capture all the
block device backups into a single archive.  And the management tool can
bundle the VM configuration into that archive too.  But those steps are
beyond the scope of QEMU.

The approach I'm suggesting is more modular.  For example, the backup
block job can also be used to copy out the state of a disk into a new
qcow2 file.

> > > +##
> > > +# @backup:
> > > +#
> > > +# Starts a VM backup.
> > > +#
> > > +# @backup-file: the backup file name
> > > +#
> > > +# @format: format of the backup file
> > > +#
> > > +# @config-filename: #optional name of a configuration file to include
> > > +into # the backup archive.
> > > +#
> > > +# @speed: #optional the maximum speed, in bytes per second # #
> > > address@hidden: #optional list of block device names (separated by ',', 
> > > ';'
> > > +# or ':'). By default the backup includes all writable block devices.
> > > +#
> > > +# Returns: the uuid of the backup job # # Since: 1.5.0 ## {
> > > +'command': 'backup', 'data': { 'backup-file': 'str',
> > > +                                 '*format': 'BackupFormat',
> > > +                                 '*config-file': 'str',
> > > +                                 '*devlist': 'str', '*speed': 'int'
> > > +},
> > 
> > devlist should be ['String'].
> 
> I want to be able to use that command on the human monitor.
> That is no longer possible if I use ['String']?

Good question.  I don't know the answer but perhaps Luiz or Anthony do
(CCed)?

> > > diff --git a/qmp-commands.hx b/qmp-commands.hx index 799adea..17e381b
> > > 100644
> > > --- a/qmp-commands.hx
> > > +++ b/qmp-commands.hx
> > > @@ -889,6 +889,18 @@ EQMP
> > >      },
> > >
> > >      {
> > > +        .name       = "backup",
> > > +        .args_type  = 
> > > "backup-file:s,format:s?,config-file:F?,speed:o?,devlist:s?",
> > > +        .mhandler.cmd_new = qmp_marshal_input_backup,
> > > +    },
> > > +
> > > +    {
> > > +        .name       = "backup-cancel",
> > > +        .args_type  = "",
> > > +        .mhandler.cmd_new = qmp_marshal_input_backup_cancel,
> > > +    },
> > 
> > We might want to more than one backup if the guest has multiple disks.
> > For example, the database disk is backed up independently of the main OS 
> > disk.
> > 
> > This API only allows one backup to run at a time.
> 
> I do not want multiple backup jobs. You can easily run 2 jobs in sequence to 
> solve above use case.

Why do you not want multiple backup jobs?  It makes perfect sense to
separate database disks from main OS disks.  They have different backup
characteristics (how often to back up, how to restore) so it's likely
that users will ask for multiple backup jobs at the same time.  Let's
get the QMP interfaces right so that it can be supported in the future,
if not right away.

Stefan