Thanks for the review. ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng f...@redhat.com 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 <chugh.ish...@research.iiit.ac.in> >> --- >> 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
