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. 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
I will be obliged by any feedback.
Signed-off-by: Ishani Chugh <chugh.ishani@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 -*-
+"""
+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
+import sys
+sys.path.append('../../scripts/qmp')
+from qmp import QEMUMonitorProtocol
+
+
+class BackupTool(object):
+ """BackupTool Class"""
+ def __init__(self, config_file='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):
+ """
+ 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")
+ return
+
+ if "drive_"+drive_id in self.config[guest_name]:
+ 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]:
+ 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")
+ 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)
+
+if __name__ == '__main__':
+ main()
--
2.7.4
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.
> 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 will be obliged by any feedback.
Thanks for doing this!
>
> Signed-off-by: Ishani Chugh <chugh.ishani@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.
> +"""
> +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.
> +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'))
> +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?
> + 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.
> + """
> + 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.
> + return
> +
> + if "drive_"+drive_id in self.config[guest_name]:
Whitespace around "+"?
> + 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.
> + 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".
> + 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.
> +
> +if __name__ == '__main__':
> + main()
> --
Nice patch, thanks!
Fam
On Mon, Jul 17, 2017 at 03:18:35PM +0800, Fam Zheng wrote:
> On Sun, 07/16 02:13, Ishani Chugh wrote:
> > +"""
> > +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.
I haven't compared Python 2 ConfigParser and Python 3 configparser in
detail, but I they have a common API subset. It should be possible to
do something like:
try:
from configparser import ConfigParser # Python 3
except ImportError:
from ConfigParser import ConfigParser # Python 2
> > +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?
Yes, it's common to take a configuration file path on the command-line
with a default value of $HOME/.program-name if no command-line option
was given.
Thanks for the review.
----- On Jul 17, 2017, at 12:48 PM, Fam Zheng famz@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.ishani@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
On 07/17/2017 03:37 PM, Ishani wrote:
> ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng famz@redhat.com 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.
>>> 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.
>>>
>>> I will be obliged by any feedback.
>>
>> Thanks for doing this!
>>
Yes, thank you :)
>>>
>>> Signed-off-by: Ishani Chugh <chugh.ishani@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.
>
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
>>> +"""
>>> +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.
>>> + 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.
--John
On 07/18/2017 04:29 PM, John Snow wrote: >>> >>> 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. More precisely, the qemu binary is GPLv2 (only), as that is the only license compatible as the sum of its parts. But new files default to GPLv2+ (upgradable to GPLv3, except when mixed with GPLv2-only code). > > (For instance, documentation and other non-code documents can sometimes > be better served by different licenses.) Indeed, some people (not me) express concern whether a specification documentation that is GPL would require all implementations of that specification to also be GPL, and thus intentionally choose a more permissive license for the documentation (BSD being a common example). (Personally, I think that a GPL specification merely means that you can't copy the docs without your copy being GPL, but that you can still implement the specification using from-scratch non-GPL code - but I'm not a lawyer, so listening to me may not be the best thing) > > We tend to avoid the implicit copyright when possible, so including an > explicit GPLv2 license is preferable to declare intent. Remember, an explicit GPLv2-only license needs strong justification, since the implicit license is GPLv2+. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
----- On Jul 19, 2017, at 2:59 AM, jsnow jsnow@redhat.com wrote:
> On 07/17/2017 03:37 PM, Ishani wrote:
>> ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng famz@redhat.com 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 <chugh.ishani@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.
>>
>
> 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
On Tue, Jul 18, 2017 at 05:29:11PM -0400, John Snow wrote: > > > On 07/17/2017 03:37 PM, Ishani wrote: > > ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng famz@redhat.com 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. Patterns would be nice. You may find string.Template() useful: https://docs.python.org/2.7/library/string.html#template-strings https://docs.python.org/3/library/string.html#template-strings
Ishani <chugh.ishani@research.iiit.ac.in> writes:
> Thanks for the review.
>
> ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng famz@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.ishani@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.
Can it be $HOME/.config/qemu/backup.ini please as per:
https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
Cheers,
--
Alex Bennée
On Sun, Jul 16, 2017 at 02:13:21AM +0530, Ishani Chugh wrote:
> + def write_config(self):
> + """
> + Writes configuration to ini file.
> + """
> + with open(self.config_file, 'w') as config_file:
> + self.config.write(config_file)
Please update the config file atomically. That means the file must be
consistent at all points in time. Things to consider:
1. open(self.config_file, 'w') truncates the destination file. If the
program crashes or is paused before self.config.write() then the
config file will appear empty (its contents are lost)!
2. Data is written to the file at the end of the 'with' statement
(because that flushes the buffer and closes the file), but there is
no guarantee that data is safely stored on disk. Please use
https://docs.python.org/3/library/os.html#os.fsync to prevent data
loss in case of power failure.
The usual pattern is:
1. Create a temporary file next to the file you wish to modify. "Next
to" is important because if you create it on another file system
(like tmpfs) it may not be possible to atomically replace the old
file.
2. Write data
3. file.flush() + os.fsync() to safely store data in the new file
4. os.rename() to atomically replace the old file
For an example, see:
https://stackoverflow.com/questions/2333872/atomic-writing-to-file-with-python
> +
> + def get_socket_path(self, socket_path, tcp):
> + """
> + Return Socket address in form of string or tuple
Please rename this function get_socket_address(). The return value is
an 'address', not a 'path'.
> + """
> + if tcp is False:
PEP8 says:
Don't compare boolean values to True or False using == .
Yes: if greeting:
No: if greeting == True:
Worse: if greeting is True:
So this should be:
if not tcp:
> + return os.path.abspath(socket_path)
> + return (socket_path.split(':')[0], int(socket_path.split(':')[1]))
The str.split(delim, maxsplit) argument can be used but feel free to
keep your version if you prefer:
return tuple(socket_path.split(':', 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")
> + return
> + if self.is_guest_running(guest_name, self.config[guest_name]['qmp'],
> + self.config[guest_name]['tcp']) is False:
This is clearer if self.is_guest_running() looks up the necessary
information in self.config[] itself:
if not self.is_guest_running(guest_name):
...
Since is_guest_running() is a method it has access to self.config[] and
there's no need for every caller to fetch 'qmp'/'tcp'.
> + 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:]
len('drive_') is a little clearer than 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))
The non-RFC version of this patch should not print raw qmp.py objects.
That's useful for debugging but not user-friendly. Either there could
be no output and a 0 exit code (indicating success), or there could be a
message like:
Starting full backup of drives:
* drive0
* drive1
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) + ".img"
Not sure if this is really useful. Many image files have a non-.img
file extension like .qcow2. Guessing the target filename is probably
just confusing because the user experience will be inconsistent
(sometimes it works, sometimes it doesn't, depending on the file
extension and the current working directory).
> +
> + if guest_name not in self.config.sections():
> + print ("Cannot find specified guest")
> + return
> +
> + if "drive_"+drive_id in self.config[guest_name]:
> + 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
Silently exiting is confusing behavior. There should be an error
message and the process should terminate with a failure exit code (1).
By the way, it may be best to avoid repetition by writing methods for
these checks:
def verify_guest_exists(self, guest_name):
if guest_name not in self.config.sections():
print('Cannot find specified guest', file=sys.stderr)
sys.exit(1)
def verify_guest_running(self, guest_name):
if not self.is_guest_running(guest_name):
print('Unable to connect to guest (is the guest running?)', file=sys.stderr)
sys.exit(1)
def a_command(self, guest_name):
self.verify_guest_exists(guest_name)
self.verify_guest_running(guest_name)
> +
> + 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]:
> + if self.config[guest_name][id] == target:
> + print ("Please choose different target")
> + return
Please make it clear from the error message that this is a duplicate
target. "Please choose different target" doesn't explain why there is a
problem and users would be confused.
> + 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):
It is not necessary to distinguish between UNIX domain socket paths and
TCP <host, port> pairs here or in the config file:
>>> c.add_section('a')
>>> c.set('a', 'b', ('hello', 'world'))
>>> c.get('a', 'b')
('hello', 'world')
>>> c.set('a', 'b', '/hello/world')
>>> c.get('a', 'b')
'/hello/world'
If you treat the value as an opaque address type it doesn't matter
whether it's a TCP <host, port> pair or a UNIX domain socket path. You
can pass the return value of c.get() directly to the QEMUMonitorProtocol
constructor:
QEMUMonitorProtocol(self.config.get(guest_name, 'qmp'))
You still need to parse the address in 'qemu-backup guest add' though.
> + """
> + 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")
What is the purpose of this if statement?
----- On Jul 17, 2017, at 8:02 PM, stefanha stefanha@redhat.com wrote:
> On Sun, Jul 16, 2017 at 02:13:21AM +0530, Ishani Chugh wrote:
>> + def write_config(self):
>> + """
>> + Writes configuration to ini file.
>> + """
>> + with open(self.config_file, 'w') as config_file:
>> + self.config.write(config_file)
>
> Please update the config file atomically. That means the file must be
> consistent at all points in time. Things to consider:
>
> 1. open(self.config_file, 'w') truncates the destination file. If the
> program crashes or is paused before self.config.write() then the
> config file will appear empty (its contents are lost)!
>
> 2. Data is written to the file at the end of the 'with' statement
> (because that flushes the buffer and closes the file), but there is
> no guarantee that data is safely stored on disk. Please use
> https://docs.python.org/3/library/os.html#os.fsync to prevent data
> loss in case of power failure.
>
> The usual pattern is:
> 1. Create a temporary file next to the file you wish to modify. "Next
> to" is important because if you create it on another file system
> (like tmpfs) it may not be possible to atomically replace the old
> file.
> 2. Write data
> 3. file.flush() + os.fsync() to safely store data in the new file
> 4. os.rename() to atomically replace the old file
>
> For an example, see:
> https://stackoverflow.com/questions/2333872/atomic-writing-to-file-with-python
Thanks. I will update in next revision.
>> +
>> + def get_socket_path(self, socket_path, tcp):
>> + """
>> + Return Socket address in form of string or tuple
>
> Please rename this function get_socket_address(). The return value is
> an 'address', not a 'path'.
Will update in next revision.
>> + """
>> + if tcp is False:
>
> PEP8 says:
>
> Don't compare boolean values to True or False using == .
>
> Yes: if greeting:
> No: if greeting == True:
> Worse: if greeting is True:
>
> So this should be:
>
> if not tcp:
>
Will update in next revision.
>> + return os.path.abspath(socket_path)
>> + return (socket_path.split(':')[0], int(socket_path.split(':')[1]))
>
> The str.split(delim, maxsplit) argument can be used but feel free to
> keep your version if you prefer:
>
> return tuple(socket_path.split(':', 1))
>
Thanks for suggestion. It is a much cleaner method.
>> +
>> + def __full_backup(self, guest_name):
>> + """
>> + 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:
>
> This is clearer if self.is_guest_running() looks up the necessary
> information in self.config[] itself:
>
> if not self.is_guest_running(guest_name):
> ...
>
> Since is_guest_running() is a method it has access to self.config[] and
> there's no need for every caller to fetch 'qmp'/'tcp'.
I agree. I am using the same method to check if a guest is running before adding
an entry to config file when user gives command to add guest. It is possible to
first add entries in config file and then check if the guest is running and delete
the entry from config file if guest is found not running.
>> + 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:]
>
> len('drive_') is a little clearer than key.index('_')+1.
Thanks. Will update in next revision.
>> + 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))
>
> The non-RFC version of this patch should not print raw qmp.py objects.
> That's useful for debugging but not user-friendly. Either there could
> be no output and a 0 exit code (indicating success), or there could be a
> message like:
>
> Starting full backup of drives:
> * drive0
> * drive1
> Backup complete!
Thanks. Will update in next revision.
>> +
>> + 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"
>
> Not sure if this is really useful. Many image files have a non-.img
> file extension like .qcow2. Guessing the target filename is probably
> just confusing because the user experience will be inconsistent
> (sometimes it works, sometimes it doesn't, depending on the file
> extension and the current working directory).
Is it okay if I give the target image a name same as drive id without any
extension?
>> +
>> + if guest_name not in self.config.sections():
>> + print ("Cannot find specified guest")
>> + return
>> +
>> + if "drive_"+drive_id in self.config[guest_name]:
>> + 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
>
> Silently exiting is confusing behavior. There should be an error
> message and the process should terminate with a failure exit code (1).
>
> By the way, it may be best to avoid repetition by writing methods for
> these checks:
>
> def verify_guest_exists(self, guest_name):
> if guest_name not in self.config.sections():
> print('Cannot find specified guest', file=sys.stderr)
> sys.exit(1)
>
> def verify_guest_running(self, guest_name):
> if not self.is_guest_running(guest_name):
> print('Unable to connect to guest (is the guest running?)', file=sys.stderr)
> sys.exit(1)
>
> def a_command(self, guest_name):
> self.verify_guest_exists(guest_name)
> self.verify_guest_running(guest_name)
The intention was similar behind this. I will rewrite and send the revision.
>> +
>> + 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]:
>> + if self.config[guest_name][id] == target:
>> + print ("Please choose different target")
>> + return
>
> Please make it clear from the error message that this is a duplicate
> target. "Please choose different target" doesn't explain why there is a
> problem and users would be confused.
>
Thanks. Will update in next revision.
>> + 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):
>
> It is not necessary to distinguish between UNIX domain socket paths and
> TCP <host, port> pairs here or in the config file:
>
> >>> c.add_section('a')
> >>> c.set('a', 'b', ('hello', 'world'))
> >>> c.get('a', 'b')
> ('hello', 'world')
> >>> c.set('a', 'b', '/hello/world')
> >>> c.get('a', 'b')
> '/hello/world'
>
> If you treat the value as an opaque address type it doesn't matter
> whether it's a TCP <host, port> pair or a UNIX domain socket path. You
> can pass the return value of c.get() directly to the QEMUMonitorProtocol
> constructor:
>
> QEMUMonitorProtocol(self.config.get(guest_name, 'qmp'))
>
> You still need to parse the address in 'qemu-backup guest add' though.
>
Will update in next revision.
>> + """
>> + 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")
>
> What is the purpose of this if statement?
It should be removed. Will fix in next revision.
Thanks,
Ishani
© 2016 - 2024 Red Hat, Inc.