[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] RFC on Backup tool
|
From: |
Ishani |
|
Subject: |
Re: [Qemu-devel] [RFC] RFC on Backup tool |
|
Date: |
Fri, 21 Jul 2017 02:48:45 +0530 (IST) |
----- On Jul 19, 2017, at 2:59 AM, jsnow address@hidden wrote:
> On 07/17/2017 03:37 PM, Ishani wrote:
>> ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng address@hidden wrote:
>>> On Sun, 07/16 02:13, Ishani Chugh wrote:
>
> [...]
>
>>> 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.
>>
>
> You may consider solidifying the backup target *pattern* during drive
> add as an alternative, such as:
>
> .../path/to/backup/%VM%/%DRIVE%/%yyyy%-%mm%-%dd%.qcow2
>
> Or some such scheme. Simple numerals work well, too:
>
> myvm/sda/incr.0.qcow2
> myvm/sda/incr.1.qcow2
>
> Simple numerals offer the benefit that it is easier to reconstruct the
> chain if you lose your metadata in the python script.
>
> Also consider that even for non-incremental backups, we want full
> backups made subsequently to not, in general, overwrite the previous
> full backup, so the TARGET is more of a "living entity" than a fixed
> thing, even in the simple case.
Okay. Thats a great suggestion. But, it might not work when the entire chain
of backups is not present at the same location. Can a case like this arise?
>>>> 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.
>>
>
> You can include this (or a similar link) in future cover letters for the
> benefit of reviewers.
Okay. Will update in next revision.
>>>>
>>>> I will be obliged by any feedback.
>>>
>>> Thanks for doing this!
>>>
>
> Yes, thank you :)
>
Thanks for review. :)
>>>>
>>>> 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.
>>
>
> Yes, up to you. Files without a copyright default to GPLv2 in the QEMU
> project, but if you feel strongly one way or another you can argue for
> that license in upstream review.
>
> (For instance, documentation and other non-code documents can sometimes
> be better served by different licenses.)
>
> We tend to avoid the implicit copyright when possible, so including an
> explicit GPLv2 license is preferable to declare intent.
>
> QEMU as a whole is GPLv2, so this is a good license to use if you don't
> have any strong feelings on the matter, but please take a moment to read
> the implications of the license as a new contributor to our project:
>
> https://tldrlegal.com/license/gnu-general-public-license-v2
>
> And the full, legal text:
>
> https://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html
>
I will go through the licenses and update in next revision. Thanks.
>>>> +"""
>>>> +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.
>>
>
> Thanks for the hint, Fam!
>
>>>> +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.
>>
>
> $HOME/.config/QEMU/backup.ini is my preference here.
>
> Just a preference; but we may wind up needing more than one or two
> secret files, and this would be a good place to keep them.
If we require multiple files, then this is a good location. Is it okay that,
at present, since we require only config file, I put the default location in
$HOME/.qemu-backup.ini ?
>>>> + 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.
>>
>
> Fam's the best! :)
>
>> Regards,
>> Ishani
>>
>
> Thank you both.
Thanks for review.
> --John