Re: [Qemu-devel] [RFC] RFC on Backup tool

[Ishani]

Thanks for the review.

----- On Jul 17, 2017, at 12:48 PM, Fam Zheng address@hidden wrote:

> On Sun, 07/16 02:13, Ishani Chugh wrote:
>> This is a Request For Comments patch for qemu backup tool. As an
>> Outreachy intern, I am assigned to the project for creating a backup
>> tool. qemu-backup will be a command-line tool for performing full and
>> incremental disk backups on running VMs.
> 
> Only full backup is implemented in this patch, is the plan to add incremental
> backup on top?  I'm curious because you have target file path set during drive
> add, but in the incremental case, it should be possible that each backup 
> creates
> a new target file that chains up with earlier ones, so I think the target file
> should be an option for "backup" command as well.

Yes. Incremental backup is to be added. I am still in learning phase with
respect to incremental backups. I will modify the arguments and workflow 
accordingly.

>> 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 usage is as follows:
>> Add a guest
>> python qemu-backup.py guest add --guest <guest_name> --qmp <socket_path> 
>> [--tcp]
>> 
>> 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
> 
> Please put these examples into the help output of the command, this way users
> can read it as well.

I have created a manpage documentation for the tool.
http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg05241.html
It is to be updated continuously as the development progresses.

>> 
>> I will be obliged by any feedback.
> 
> Thanks for doing this!
> 
>> 
>> Signed-off-by: Ishani Chugh <address@hidden>
>> ---
>>  contrib/backup/qemu-backup.py | 244 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 244 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..9c3dc53
>> --- /dev/null
>> +++ b/contrib/backup/qemu-backup.py
>> @@ -0,0 +1,244 @@
>> +#!/usr/bin/python
>> +# -*- coding: utf-8 -*-
> 
> You need a copyright header here (the default choice for QEMU is GPLv2 but 
> there
> is no strict restrictions for scripts). See examples in other *.py files.

Thanks. Will update in next revision.
 
>> +"""
>> +This file is an implementation of backup tool
>> +"""
>> +from argparse import ArgumentParser
>> +import os
>> +import errno
>> +from socket import error as socket_error
>> +import configparser
> 
> Python2 has ConfigParser while python3 has configparser. Please be specific
> about the python compatibility level of this script - my system (Fedora) has
> python2 as /usr/bin/python, so the shebang and your example command in the
> commit message don't really work. "six" module can handle python 2/3
> differentiations, or you can use '#!/usr/bin/env python2' to specify a python
> version explicitly.

The script is supposed to be both python2/3 compatible. I will take reference
from Stefan's review and fix it in next revision.

>> +import sys
>> +sys.path.append('../../scripts/qmp')
> 
> This expects the script to be invoked from its local directory? It's better to
> use the __file__ trick to allow arbitrary workdir:
> 
> $ git grep __file__
> scripts/device-crash-test:sys.path.append(os.path.join(os.path.dirname(__file__),
> '..', 'scripts'))
> scripts/qemu-gdb.py:sys.path.append(os.path.dirname(__file__))
> tests/migration/guestperf/engine.py:sys.path.append(os.path.join(os.path.dirname(__file__),
> '..', '..', '..', 'scripts'))
> tests/qemu-iotests/iotests.py:sys.path.append(os.path.join(os.path.dirname(__file__),
> '..', '..', 'scripts'))

Thanks. Will fix it in next revision.

>> +from qmp import QEMUMonitorProtocol
>> +
>> +
>> +class BackupTool(object):
>> +    """BackupTool Class"""
>> +    def __init__(self, config_file='backup.ini'):
> 
> Is it better to put this in a more predictable place such as
> "$HOME/.qemu-backup.ini" and/or make it a command line option?

It is planned to be an optional command-line option, if not
provided, the default location suggested should be taken.  

>> +        self.config_file = config_file
>> +        self.config = configparser.ConfigParser()
>> +        self.config.read(self.config_file)
>> +
>> +    def write_config(self):
>> +        """
>> +        Writes configuration to ini file.
>> +        """
>> +        with open(self.config_file, 'w') as config_file:
>> +            self.config.write(config_file)
>> +
>> +    def get_socket_path(self, socket_path, tcp):
>> +        """
>> +        Return Socket address in form of string or tuple
>> +        """
>> +        if tcp is False:
>> +            return os.path.abspath(socket_path)
>> +        return (socket_path.split(':')[0], int(socket_path.split(':')[1]))
>> +
>> +    def __full_backup(self, guest_name):
> 
> I think double underscore attributes are special:
> 
> https://www.python.org/dev/peps/pep-0008/#id47
> 
> If it's not intended, perhaps it's enough to just stick to one "_". The same
> applies to a few more below.

I used double underscores to stick with internal usage. However, single
underscores can be used as well for weak internal usage. Will fix in
next revision.
 
>> +        """
>> +        Performs full backup of guest
>> +        """
>> +        if guest_name not in self.config.sections():
>> +            print ("Cannot find specified guest")
>> +            return
>> +        if self.is_guest_running(guest_name, self.config[guest_name]['qmp'],
>> +                                 self.config[guest_name]['tcp']) is False:
>> +            return
>> +        connection = QEMUMonitorProtocol(
>> +                                         self.get_socket_path(
>> +                                             self.config[guest_name]['qmp'],
>> +                                             
>> self.config[guest_name]['tcp']))
>> +        connection.connect()
>> +        cmd = {"execute": "transaction", "arguments": {"actions": []}}
>> +        for key in self.config[guest_name]:
>> +            if key.startswith("drive_"):
>> +                drive = key[key.index('_')+1:]
>> +                target = self.config[guest_name][key]
>> +                sub_cmd = {"type": "drive-backup", "data": {"device": drive,
>> +                                                            "target": 
>> target,
>> +                                                            "sync": "full"}}
>> +                cmd['arguments']['actions'].append(sub_cmd)
>> +        print (connection.cmd_obj(cmd))
>> +
>> +    def __drive_add(self, drive_id, guest_name, target=None):
>> +        """
>> +        Adds drive for backup
>> +        """
>> +        if target is None:
>> +            target = os.path.abspath(drive_id) + ".img"
>> +
>> +        if guest_name not in self.config.sections():
>> +            print ("Cannot find specified guest")
> 
> Error messages should go to stderr.

Will update in next revision.

> 
>> +            return
>> +
>> +        if "drive_"+drive_id in self.config[guest_name]:
> 
> Whitespace around "+"?
> 

I am not sure how I missed it. I ran the code through online
PEP8 checker. Will fix it.

>> +            print ("Drive already marked for backup")
>> +            return
>> +
>> +        if self.is_guest_running(guest_name, self.config[guest_name]['qmp'],
>> +                                 self.config[guest_name]['tcp']) is False:
>> +            return
>> +
>> +        connection = QEMUMonitorProtocol(
>> +                                         self.get_socket_path(
>> +                                             self.config[guest_name]['qmp'],
>> +                                             
>> self.config[guest_name]['tcp']))
>> +        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 device_present is False:
>> +            print ("No such drive in guest")
>> +            return
>> +
>> +        drive_id = "drive_" + drive_id
>> +        for id in self.config[guest_name]:
> 
> I think id is a python built-in function, please avoid using it as variable
> name.
>

Will fix in next revision.

>> +            if self.config[guest_name][id] == target:
>> +                print ("Please choose different target")
>> +                return
>> +        self.config.set(guest_name, drive_id, target)
>> +        self.write_config()
>> +        print("Successfully Added Drive")
>> +
>> +    def is_guest_running(self, guest_name, socket_path, tcp):
>> +        """
>> +        Checks whether specified guest is running or not
>> +        """
>> +        try:
>> +            connection = QEMUMonitorProtocol(
>> +                                             self.get_socket_path(
>> +                                                  socket_path, tcp))
>> +            connection.connect()
>> +        except socket_error:
>> +            if socket_error.errno != errno.ECONNREFUSED:
>> +                print ("Connection to guest refused")
>> +            return False
>> +        except:
>> +            print ("Unable to connect to guest")
>> +            return False
>> +        return True
>> +
>> +    def __guest_add(self, guest_name, socket_path, tcp):
>> +        """
>> +        Adds a guest to the config file
>> +        """
>> +        if self.is_guest_running(guest_name, socket_path, tcp) is False:
>> +            return
>> +
>> +        if guest_name in self.config.sections():
>> +            print ("ID already exists. Please choose a different guestname")
> 
> "guest name".

Will fix in next revision. Thanks.

>> +            return
>> +
>> +        self.config[guest_name] = {'qmp': socket_path}
>> +        self.config.set(guest_name, 'tcp', str(tcp))
>> +        self.write_config()
>> +        print("Successfully Added Guest")
>> +
>> +    def __guest_remove(self, guest_name):
>> +        """
>> +        Removes a guest from config file
>> +        """
>> +        if guest_name not in self.config.sections():
>> +            print("Guest Not present")
>> +            return
>> +        self.config.remove_section(guest_name)
>> +        print("Guest successfully deleted")
>> +
>> +    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)
>> +
>> +    def guest_add_wrapper(self, args):
>> +        """
>> +        Wrapper for __quest_add method
>> +        """
>> +        if args.tcp is False:
>> +            self.__guest_add(args.guest, args.qmp, False)
>> +        else:
>> +            self.__guest_add(args.guest, args.qmp, True)
>> +
>> +    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 main():
>> +    backup_tool = BackupTool()
>> +    parser = ArgumentParser()
>> +    subparsers = parser.add_subparsers(title='Subcommands',
>> +                                       description='Valid Subcommands',
>> +                                       help='Subcommand help')
>> +    guest_parser = subparsers.add_parser('guest', help='Adds or \
>> +                                                   removes and lists 
>> guest(s)')
>> +    guest_subparsers = guest_parser.add_subparsers(title='Guest Subparser')
>> +    guest_list_parser = guest_subparsers.add_parser('list',
>> +                                                    help='Lists all guests')
>> +    guest_list_parser.set_defaults(func=backup_tool.list)
>> +
>> +    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.add_argument('--tcp', nargs='?', type=bool,
>> +                                  default=False,
>> +                                  help='Specify if socket is tcp')
>> +    guest_add_parser.set_defaults(func=backup_tool.guest_add_wrapper)
>> +
>> +    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_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_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_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)
>> +
>> +    args = parser.parse_args()
>> +    args.func(args)
> 
> All exit points should report a status code (zero for success and non-zero for
> failures). This way the user or upper layer software know if backup has
> succeeded.

I agree. Will update in next revision. Thanks.

>> +
>> +if __name__ == '__main__':
>> +    main()
>> --
> 
> Nice patch, thanks!

Thanks for a deep and detailed review.

Regards,
Ishani