Re: [Qemu-devel] [PATCH v3 1/3] backup: QEMU Backup Tool

[Ishani]

----- On Sep 2, 2017, at 4:17 AM, jsnow address@hidden wrote:

> I suggest as your commit message here, something like:
> 
> "backup: Add QEMU backup tool"
> 
> A good commit message should say what applying the patch will do:
> 
> "What will happen if I apply this patch?"
> "It will 'Add QEMU backup tool'"
> 
> On 08/30/2017 01:14 PM, Ishani Chugh wrote:
>> qemu-backup will be a command-line tool for performing full and
>> incremental disk backups on running VMs. It is intended as a
>> reference implementation for management stack and backup developers
>> to see QEMU's backup features in action. The tool writes details of
>> guest in a configuration file and the data is retrieved from the file
>> while creating a backup. The location of config file can be set as an
>> environment variable QEMU_BACKUP_CONFIG. The usage is as follows:
>> 
>> Add a guest
>> python qemu-backup.py guest add --guest <guest_name> --qmp <socket_path>
>> 
> 
> This should probably be organized by subcommand, so:
> 
> guest add
> guest remove
> guest list
> 
> drive add
> 
> backup
> 
> restore
> 
Will fix in next revision.

>> Add a drive for backup in a specified guest
>> python qemu-backup.py drive add --guest <guest_name> --id <drive_id> 
>> [--target
>> <target_file_path>]
>> 
>> Create backup of the added drives:
>> python qemu-backup.py backup --guest <guest_name>
>> 
>> List all guest configs in configuration file:
>> python qemu-backup.py guest list
>> 
>> Restore operation
>> python qemu-backup.py restore --guest <guest-name>
>> 
>> Remove a guest
>> python qemu-backup.py guest remove --guest <guest_name>
>> 
> 
> Looks good, but the man page should perhaps be patch 01 instead of 03 so
> that the usage can be reviewed outside of the commit message.

I will reorder the patches in version 4 with:
1) Manpage
2) Full Backup
3) Test for full backup

>> Signed-off-by: Ishani Chugh <address@hidden>
>> ---
>>  contrib/backup/qemu-backup.py | 343 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 343 insertions(+)
>>  create mode 100644 contrib/backup/qemu-backup.py
>> 
>> diff --git a/contrib/backup/qemu-backup.py b/contrib/backup/qemu-backup.py
>> new file mode 100644
>> index 0000000..248ca9f
>> --- /dev/null
>> +++ b/contrib/backup/qemu-backup.py
>> @@ -0,0 +1,343 @@
>> +#!/usr/bin/python
>> +# -*- coding: utf-8 -*-
>> +#
>> +# Copyright (C) 2017 Ishani Chugh <address@hidden>
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +"""
>> +This file is an implementation of backup tool
>> +"""
>> +from __future__ import print_function
>> +from argparse import ArgumentParser
>> +import os
>> +import errno
>> +from socket import error as socket_error
>> +try:
>> +    import configparser
>> +except ImportError:
>> +    import ConfigParser as configparser
>> +import sys
>> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
>> +                             'scripts', 'qmp'))
>> +from qmp import QEMUMonitorProtocol
>> +
>> +
>> +class BackupTool(object):
>> +    """BackupTool Class"""
>> +    def __init__(self, config_file=os.path.expanduser('~') +
>> +                 '/.config/qemu/qemu-backup-config'):
>> +        if "QEMU_BACKUP_CONFIG" in os.environ:
>> +            self.config_file = os.environ["QEMU_BACKUP_CONFIG"]
>> +
>> +        else:
>> +            self.config_file = config_file
>> +            try:
>> +                if not os.path.isdir(os.path.dirname(self.config_file)):
>> +                    os.makedirs(os.path.dirname(self.config_file))
>> +            except:
>> +                print("Cannot create config directory", file=sys.stderr)
>> +                sys.exit(1)
>> +        self.config = configparser.ConfigParser()
>> +        self.config.read(self.config_file)
>> +        try:
>> +            if self.config.get('general', 'version') != '1.0':
>> +                    print("Version Conflict in config file", 
>> file=sys.stderr)
>> +                    sys.exit(1)
>> +        except:
>> +            self.config['general'] = {'version': '1.0'}
>> +            self.write_config()
>> +
>> +    def write_config(self):
>> +        """
>> +        Writes configuration to ini file.
>> +        """
>> +        config_file = open(self.config_file + ".tmp", 'w')
>> +        self.config.write(config_file)
>> +        config_file.flush()
>> +        os.fsync(config_file.fileno())
>> +        config_file.close()
>> +        os.rename(self.config_file + ".tmp", self.config_file)
>> +
>> +    def get_socket_address(self, socket_address):
>> +        """
>> +        Return Socket address in form of string or tuple
>> +        """
>> +        if socket_address.startswith('tcp'):
>> +            return (socket_address.split(':')[1],
>> +                    int(socket_address.split(':')[2]))
>> +        return socket_address.split(':', 2)[1]
>> +
>> +    def _full_backup(self, guest_name):
>> +        """
>> +        Performs full backup of guest
>> +        """
>> +        if guest_name not in self.config.sections():
>> +            print("Cannot find specified guest", file=sys.stderr)
>> +            sys.exit(1)
>> +
>> +        self.verify_guest_running(guest_name)
>> +        connection = QEMUMonitorProtocol(
>> +                                         self.get_socket_address(
>> +                                             
>> self.config[guest_name]['qmp']))
>> +        connection.connect()
>> +        cmd = {"execute": "transaction", "arguments": {"actions": []}}
>> +        drive_list = []
>> +        for key in self.config[guest_name]:
>> +            if key.startswith("drive_"):
>> +                drive = key[len('drive_'):]
>> +                drive_list.append(drive)
>> +                target = self.config[guest_name][key]
>> +                sub_cmd = {"type": "drive-backup", "data": {"device": drive,
>> +                                                            "target": 
>> target,
>> +                                                            "sync": "full"}}
>> +                cmd['arguments']['actions'].append(sub_cmd)
>> +        qmp_return = connection.cmd_obj(cmd)
>> +        if 'error' in qmp_return:
>> +            print(qmp_return['error']['desc'], file=sys.stderr)
>> +            sys.exit(1)
>> +        print("Backup Started")
>> +        while drive_list:
>> +            event = connection.pull_event(wait=True)
>> +            if event['event'] == 'SHUTDOWN':
>> +                print("The guest was SHUT DOWN", file=sys.stderr)
>> +                sys.exit(1)
>> +
>> +            if event['event'] == 'BLOCK_JOB_COMPLETED':
>> +                if event['data']['device'] in drive_list and \
>> +                        event['data']['type'] == 'backup':
>> +                        print("*" + event['data']['device'])
>> +                        drive_list.remove(event['data']['device'])
>> +
>> +            if event['event'] == 'BLOCK_JOB_ERROR':
>> +                if event['data']['device'] in drive_list and \
>> +                        event['data']['type'] == 'backup':
>> +                        print(event['data']['device'] + " Backup Failed",
>> +                              file=sys.stderr)
>> +                        drive_list.remove(event['data']['device'])
>> +        print("Backup Complete")
>> +
>> +    def _drive_add(self, drive_id, guest_name, target=None):
>> +        """
>> +        Adds drive for backup
>> +        """
>> +        if target is None:
>> +            target = os.path.abspath(drive_id)
> 
> Similar problem as with other commands:
> 
> address@hidden (review) ~/s/q/c/backup> ./qemu-backup.py drive add
> Traceback (most recent call last):
>  File "./qemu-backup.py", line 343, in <module>
>    main()
>  File "./qemu-backup.py", line 340, in main
>    args.func(args)
>  File "./qemu-backup.py", line 272, in drive_add_wrapper
>    self._drive_add(args.id, args.guest, args.target)
>  File "./qemu-backup.py", line 137, in _drive_add
>    target = os.path.abspath(drive_id)
>  File "/usr/lib64/python2.7/posixpath.py", line 360, in abspath
>    if not isabs(path):
>  File "/usr/lib64/python2.7/posixpath.py", line 54, in isabs
>    return s.startswith('/')
> AttributeError: 'NoneType' object has no attribute 'startswith'
> 
> This errors out if you don't tell it what drive you want to add.
> 
> 
> Meanwhile,
> 
> address@hidden (review) ~/s/q/c/backup> ./qemu-backup.py drive add --id
> foobar
> Cannot find specified guest
> 
> Should probably error out because you didn't set the guest name at all.
> 
>> +
>> +        if os.path.isdir(os.path.dirname(target)) is False:
>> +            print("Cannot find target directory", file=sys.stderr)
>> +            sys.exit(1)
>> +
>> +        if guest_name not in self.config.sections():
>> +            print("Cannot find specified guest", file=sys.stderr)
>> +            sys.exit(1)
>> +
>> +        if "drive_" + drive_id in self.config[guest_name]:
>> +            print("Drive already marked for backup", file=sys.stderr)
>> +            sys.exit(1)
>> +
>> +        self.verify_guest_running(guest_name)
>> +
>> +        connection = QEMUMonitorProtocol(
>> +                                         self.get_socket_address(
>> +                                             
>> self.config[guest_name]['qmp']))
>> +        connection.connect()
>> +        cmd = {'execute': 'query-block'}
>> +        returned_json = connection.cmd_obj(cmd)
>> +        device_present = False
>> +        for device in returned_json['return']:
>> +            if device['device'] == drive_id:
>> +                device_present = True
>> +                break
>> +
>> +        if not device_present:
>> +            print("No such drive in guest", file=sys.stderr)
>> +            sys.exit(1)
> 
> cool!
> 
>> +
>> +        drive_id = "drive_" + drive_id
>> +        for d_id in self.config[guest_name]:
>> +            if self.config[guest_name][d_id] == target:
>> +                print("Please choose different target", file=sys.stderr)
>> +                sys.exit(1)
>> +        self.config.set(guest_name, drive_id, target)
>> +        self.write_config()
>> +        print("Successfully Added Drive")
>> +
>> +    def verify_guest_running(self, guest_name):
>> +        """
>> +        Checks whether specified guest is running or not
>> +        """
>> +        socket_address = self.config.get(guest_name, 'qmp')> +        try:
>> +            connection = QEMUMonitorProtocol(self.get_socket_address(
>> +                                             socket_address))
>> +            connection.connect()
>> +        except socket_error:
>> +            if socket_error.errno != errno.ECONNREFUSED:
> 
> what happens if errno *IS* ECONNREFUSED?
> 
>> +                print("Connection to guest refused", file=sys.stderr)
>> +                sys.exit(1)
>> +
>> +    def _guest_add(self, guest_name, socket_address):
>> +        """
>> +        Adds a guest to the config file
>> +        """
>> +        if guest_name in self.config.sections():
>> +            print("ID already exists. Please choose a different guest name",
>> +                  file=sys.stderr)
>> +            sys.exit(1)
>> +        self.config[guest_name] = {'qmp': socket_address}
>> +        self.verify_guest_running(guest_name)
>> +        self.write_config()
>> +        print("Successfully Added Guest")
> 
> this function crashes if you do not provide a guest_name, it should
> report an error instead.
> 
> ./qemu-backup.py guest add
> ./qemu-backup.py guest add --guest foobar
> 
> both crash, please correct.
> 
> ./qemu-backup.py guest add --guest foobar --qmp foobar
> 
> also crashes, please error gracefully if you cannot parse the QMP
> argument as a valid socket.

I will set the required options to True for mandatory arguments.

>> +
>> +    def _guest_remove(self, guest_name):
>> +        """
>> +        Removes a guest from config file
>> +        """
>> +        if guest_name not in self.config.sections():
>> +            print("Guest Not present", file=sys.stderr)
>> +            sys.exit(1)
>> +        self.config.remove_section(guest_name)
>> +        print("Guest successfully deleted")
>> +
> 
> If you don't supply a guest name, you get the "Guest Not present" error
> message, but really we should error out as if it was a parsing error.
> 
> address@hidden (review) ~/s/q/c/backup> ./qemu-backup.py guest remove
> --guest general
> Guest successfully deleted
> 
> Whoops!
> 
>> +    def _restore(self, guest_name):
>> +        """
>> +        Prints Steps to perform restore operation
>> +        """
>> +        if guest_name not in self.config.sections():
>> +            print("Cannot find specified guest", file=sys.stderr)
>> +            sys.exit(1)
>> +
>> +        self.verify_guest_running(guest_name)
>> +        connection = QEMUMonitorProtocol(
>> +                                         self.get_socket_address(
>> +                                             
>> self.config[guest_name]['qmp']))
>> +        connection.connect()
>> +        print("To perform restore:")
>> +        print("Shut down guest")
>> +        for key in self.config[guest_name]:
>> +            if key.startswith("drive_"):
>> +                drive = key[len('drive_'):]
>> +                target = self.config[guest_name][key]
>> +                cmd = {'execute': 'query-block'}
>> +                returned_json = connection.cmd_obj(cmd)
>> +                device_present = False
>> +                for device in returned_json['return']:
>> +                    if device['device'] == drive:
>> +                        device_present = True
>> +                        location = device['inserted']['image']['filename']
>> +                        print("qemu-img convert " + target + " " + location)
>> +
>> +                if not device_present:
>> +                    print("No such drive in guest", file=sys.stderr)
>> +                    sys.exit(1)
>> +
>> +    def guest_remove_wrapper(self, args):
>> +        """
>> +        Wrapper for _guest_remove method.
>> +        """
>> +        guest_name = args.guest
>> +        self._guest_remove(guest_name)
>> +        self.write_config()
>> +
>> +    def list(self, args):
>> +        """
>> +        Prints guests present in Config file
>> +        """
>> +        for guest_name in self.config.sections():
>> +            print(guest_name)
> 
> This will print out "general" too, which I think should be exempted from
> this list, as it is not actually a guest!
> 
> I propose something like this:
> 
> [general]
> guests=ishanivm1 ishanivm2 ishanivm3
> 
> [guest:ishanivm]
> foo=bar
> 
> [guest:ishanivm2]
> foo=bar2
> 
> and so on.
> 
> ("What happens if someone tries to name a guest 'guest:foobar'? Well,
> then the section just gets named 'guest:guest:foobar' and we go about
> our happy way.")
> 
> Whenever a guest is added or removed, you update the list and delete or
> add the corresponding section appropriately.

The section general will contain the version number. Will it not be better
to add an additional condition to skip 'general' rather than modifying the
design of config file?

>> +
>> +    def guest_add_wrapper(self, args):
>> +        """
>> +        Wrapper for _quest_add method
> 
> s/quest/guest/

Will fix in next revision.
 
>> +        """
>> +        self._guest_add(args.guest, args.qmp)
>> +> +    def drive_add_wrapper(self, args):
>> +        """
>> +        Wrapper for _drive_add method
>> +        """
>> +        self._drive_add(args.id, args.guest, args.target)
>> +
>> +    def fullbackup_wrapper(self, args):
>> +        """
>> +        Wrapper for _full_backup method
>> +        """
>> +        self._full_backup(args.guest)
>> +
>> +    def restore_wrapper(self, args):
>> +        """
>> +        Wrapper for restore
>> +        """
>> +        self._restore(args.guest)
>> +
>> +
>> +def main():
>> +    backup_tool = BackupTool()
>> +    parser = ArgumentParser()
> 
> The following is a pretty big block without much visual structure to
> guide the eye. I'd recommend:
> 
> (1) Splitting this out into a helper function so the overall flow of
> main() is easier to spot at a glance, and
> (2) Adding a few comments to just visually break up the monotony.
> 
>> +    subparsers = parser.add_subparsers(title='Subcommands',
>> +                                       description='Valid Subcommands',
>> +                                       help='Subcommand help')       # guest
>> +    guest_parser = subparsers.add_parser('guest', help='Adds or \
>> +                                                   removes and lists 
>> guest(s)')
> 
> Try "Manage guest(s)" as a shorthand, or just write "add, remove, or
> list guests."
> 
> "Adds (or) removes (and) lists" is an awkward construct.
> 
>> +    guest_subparsers = guest_parser.add_subparsers(title='Guest Subparser')
>       # guest list
>> +    guest_list_parser = guest_subparsers.add_parser('list',
>> +                                                    help='Lists all guests')
>> +    guest_list_parser.set_defaults(func=backup_tool.list)
>> +
>       # guest add
>> +    guest_add_parser = guest_subparsers.add_parser('add', help='Adds a 
>> guest')
>> +    guest_add_parser.add_argument('--guest', action='store', type=str,
>> +                                  help='Name of the guest')
>> +    guest_add_parser.add_argument('--qmp', action='store', type=str,
>> +                                  help='Path of socket')
>> +    guest_add_parser.set_defaults(func=backup_tool.guest_add_wrapper)
>> +
>       # guest remove
>> +    guest_remove_parser = guest_subparsers.add_parser('remove',
>> +                                                      help='removes a 
>> guest')
>> +    guest_remove_parser.add_argument('--guest', action='store', type=str,
>> +                                     help='Name of the guest')
>> +    guest_remove_parser.set_defaults(func=backup_tool.guest_remove_wrapper)
>> +
>       # drive
>> +    drive_parser = subparsers.add_parser('drive',
>> +                                         help='Adds drive(s) for backup')
>> +    drive_subparsers = drive_parser.add_subparsers(title='Add subparser',
>> +                                                   description='Drive \
>> +                                                                subparser')
>       # drive add
>> +    drive_add_parser = drive_subparsers.add_parser('add',
>> +                                                   help='Adds new \
>> +                                                         drive for backup')
>> +    drive_add_parser.add_argument('--guest', action='store',
>> +                                  type=str, help='Name of the guest')
>> +    drive_add_parser.add_argument('--id', action='store',
>> +                                  type=str, help='Drive ID')
>> +    drive_add_parser.add_argument('--target', nargs='?',
>> +                                  default=None, help='Destination path')
>> +    drive_add_parser.set_defaults(func=backup_tool.drive_add_wrapper)
>> +
>       # backup
>> +    backup_parser = subparsers.add_parser('backup', help='Creates backup')
>> +    backup_parser.add_argument('--guest', action='store',
>> +                               type=str, help='Name of the guest')
>> +    backup_parser.set_defaults(func=backup_tool.fullbackup_wrapper)
>> +
>       # restore
>> +    backup_parser = subparsers.add_parser('restore', help='Restores drives')
>> +    backup_parser.add_argument('--guest', action='store',
>> +                               type=str, help='Name of the guest')
>> +    backup_parser.set_defaults(func=backup_tool.restore_wrapper)
>> +
> 
> maybe something like:
>       parser = buildMyParser()
> 
> ?

Will fix in next revision. I have also identified that 
./qemu-backup.py guest add -h 
shows every argument as optional argument(even when they are set to required).
I will also fix it in v4.

>> +    args = parser.parse_args()
>> +    args.func(args)
>> +
>> +if __name__ == '__main__':
>> +    main()
>> 
> 
> Looking good overall; my advice is to try testing your tool by
> intentionally omitting items and giving it bad information. Make sure
> that any arguments marked as mandatory in the manpage/parser are
> actually mandatory and try to eliminate stack trace dumps from your
> python tool.
> 
> I stopped a little short of reviewing the entire tool this time, so
> there may be more cases that are similar to the other issues I've
> already pointed out.
> 
> Thanks!
Thanks for review.

Regards,
Ishani