[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] RFC on Backup tool
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [RFC] RFC on Backup tool |
Date: |
Fri, 21 Jul 2017 17:16:19 +0100 |
User-agent: |
mu4e 0.9.19; emacs 25.2.50.3 |
Ishani <address@hidden> writes:
> Thanks for the review.
>
> ----- On Jul 17, 2017, at 12:48 PM, Fam Zheng address@hidden wrote:
>
>> On Sun, 07/16 02:13, Ishani Chugh wrote:
>>> This is a Request For Comments patch for qemu backup tool. As an
>>> Outreachy intern, I am assigned to the project for creating a backup
>>> tool. qemu-backup will be a command-line tool for performing full and
>>> incremental disk backups on running VMs.
>>
>> Only full backup is implemented in this patch, is the plan to add incremental
>> backup on top? I'm curious because you have target file path set during
>> drive
>> add, but in the incremental case, it should be possible that each backup
>> creates
>> a new target file that chains up with earlier ones, so I think the target
>> file
>> should be an option for "backup" command as well.
>
> Yes. Incremental backup is to be added. I am still in learning phase with
> respect to incremental backups. I will modify the arguments and workflow
> accordingly.
>
>>> It is intended as a
>>> reference implementation for management stack and backup developers
>>> to see QEMU's backup features in action. The tool writes details of
>>> guest in a configuration file and the data is retrieved from the file
>>> while creating a backup. The usage is as follows:
>>> Add a guest
>>> python qemu-backup.py guest add --guest <guest_name> --qmp <socket_path>
>>> [--tcp]
>>>
>>> Add a drive for backup in a specified guest
>>> python qemu-backup.py drive add --guest <guest_name> --id <drive_id>
>>> [--target
>>> <target_file_path>]
>>>
>>> Create backup of the added drives:
>>> python qemu-backup.py backup --guest <guest_name>
>>>
>>> List all guest configs in configuration file:
>>> python qemu-backup.py guest list
>>
>> Please put these examples into the help output of the command, this way users
>> can read it as well.
>
> I have created a manpage documentation for the tool.
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg05241.html
> It is to be updated continuously as the development progresses.
>
>>>
>>> I will be obliged by any feedback.
>>
>> Thanks for doing this!
>>
>>>
>>> Signed-off-by: Ishani Chugh <address@hidden>
>>> ---
>>> contrib/backup/qemu-backup.py | 244
>>> ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 244 insertions(+)
>>> create mode 100644 contrib/backup/qemu-backup.py
>>>
>>> diff --git a/contrib/backup/qemu-backup.py b/contrib/backup/qemu-backup.py
>>> new file mode 100644
>>> index 0000000..9c3dc53
>>> --- /dev/null
>>> +++ b/contrib/backup/qemu-backup.py
>>> @@ -0,0 +1,244 @@
>>> +#!/usr/bin/python
>>> +# -*- coding: utf-8 -*-
>>
>> You need a copyright header here (the default choice for QEMU is GPLv2 but
>> there
>> is no strict restrictions for scripts). See examples in other *.py files.
>
> Thanks. Will update in next revision.
>
>>> +"""
>>> +This file is an implementation of backup tool
>>> +"""
>>> +from argparse import ArgumentParser
>>> +import os
>>> +import errno
>>> +from socket import error as socket_error
>>> +import configparser
>>
>> Python2 has ConfigParser while python3 has configparser. Please be specific
>> about the python compatibility level of this script - my system (Fedora) has
>> python2 as /usr/bin/python, so the shebang and your example command in the
>> commit message don't really work. "six" module can handle python 2/3
>> differentiations, or you can use '#!/usr/bin/env python2' to specify a python
>> version explicitly.
>
> The script is supposed to be both python2/3 compatible. I will take reference
> from Stefan's review and fix it in next revision.
>
>>> +import sys
>>> +sys.path.append('../../scripts/qmp')
>>
>> This expects the script to be invoked from its local directory? It's better
>> to
>> use the __file__ trick to allow arbitrary workdir:
>>
>> $ git grep __file__
>> scripts/device-crash-test:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', 'scripts'))
>> scripts/qemu-gdb.py:sys.path.append(os.path.dirname(__file__))
>> tests/migration/guestperf/engine.py:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', '..', '..', 'scripts'))
>> tests/qemu-iotests/iotests.py:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', '..', 'scripts'))
>
> Thanks. Will fix it in next revision.
>
>>> +from qmp import QEMUMonitorProtocol
>>> +
>>> +
>>> +class BackupTool(object):
>>> + """BackupTool Class"""
>>> + def __init__(self, config_file='backup.ini'):
>>
>> Is it better to put this in a more predictable place such as
>> "$HOME/.qemu-backup.ini" and/or make it a command line option?
>
> It is planned to be an optional command-line option, if not
> provided, the default location suggested should be taken.
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