Add test for a new backup option: discard-source.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
Message-Id: <20240313152822.626493-6-vsementsov@yandex-team.ru>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
.../qemu-iotests/tests/backup-discard-source | 152 ++++++++++++++++++
.../tests/backup-discard-source.out | 5 +
2 files changed, 157 insertions(+)
create mode 100755 tests/qemu-iotests/tests/backup-discard-source
create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
diff --git a/tests/qemu-iotests/tests/backup-discard-source b/tests/qemu-iotests/tests/backup-discard-source
new file mode 100755
index 0000000000..2391b12acd
--- /dev/null
+++ b/tests/qemu-iotests/tests/backup-discard-source
@@ -0,0 +1,152 @@
+#!/usr/bin/env python3
+#
+# Test backup discard-source parameter
+#
+# Copyright (c) Virtuozzo International GmbH.
+# Copyright (c) Yandex
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+
+import iotests
+from iotests import qemu_img_create, qemu_img_map, qemu_io
+
+
+temp_img = os.path.join(iotests.test_dir, 'temp')
+source_img = os.path.join(iotests.test_dir, 'source')
+target_img = os.path.join(iotests.test_dir, 'target')
+size = '1M'
+
+
+def get_actual_size(vm, node_name):
+ nodes = vm.cmd('query-named-block-nodes', flat=True)
+ node = next(n for n in nodes if n['node-name'] == node_name)
+ return node['image']['actual-size']
+
+
+class TestBackup(iotests.QMPTestCase):
+ def setUp(self):
+ qemu_img_create('-f', iotests.imgfmt, source_img, size)
+ qemu_img_create('-f', iotests.imgfmt, temp_img, size)
+ qemu_img_create('-f', iotests.imgfmt, target_img, size)
+ qemu_io('-c', 'write 0 1M', source_img)
+
+ self.vm = iotests.VM()
+ self.vm.launch()
+
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'cbw',
+ 'driver': 'copy-before-write',
+ 'file': {
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': source_img,
+ }
+ },
+ 'target': {
+ 'driver': iotests.imgfmt,
+ 'discard': 'unmap',
+ 'node-name': 'temp',
+ 'file': {
+ 'driver': 'file',
+ 'filename': temp_img
+ }
+ }
+ })
+
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'access',
+ 'discard': 'unmap',
+ 'driver': 'snapshot-access',
+ 'file': 'cbw'
+ })
+
+ self.vm.cmd('blockdev-add', {
+ 'driver': iotests.imgfmt,
+ 'node-name': 'target',
+ 'file': {
+ 'driver': 'file',
+ 'filename': target_img
+ }
+ })
+
+ self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+
+ def tearDown(self):
+ # That should fail, because region is discarded
+ self.vm.hmp_qemu_io('access', 'read 0 1M')
+
+ self.vm.shutdown()
+
+ self.assertTrue('read failed: Permission denied' in self.vm.get_log())
+
+ # Final check that temp image is empty
+ mapping = qemu_img_map(temp_img)
+ self.assertEqual(len(mapping), 1)
+ self.assertEqual(mapping[0]['start'], 0)
+ self.assertEqual(mapping[0]['length'], 1024 * 1024)
+ self.assertEqual(mapping[0]['data'], False)
+
+ os.remove(temp_img)
+ os.remove(source_img)
+ os.remove(target_img)
+
+ def do_backup(self):
+ self.vm.cmd('blockdev-backup', device='access',
+ sync='full', target='target',
+ job_id='backup0',
+ discard_source=True)
+
+ self.vm.event_wait(name='BLOCK_JOB_COMPLETED')
+
+ def test_discard_written(self):
+ """
+ 1. Guest writes
+ 2. copy-before-write operation, data is stored to temp
+ 3. start backup(discard_source=True), check that data is
+ removed from temp
+ """
+ # Trigger copy-before-write operation
+ result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
+ self.assert_qmp(result, 'return', '')
+
+ # Check that data is written to temporary image
+ self.assertGreater(get_actual_size(self.vm, 'temp'), 1024 * 1024)
+
+ self.do_backup()
+
+ def test_discard_cbw(self):
+ """
+ 1. do backup(discard_source=True), which should inform
+ copy-before-write that data is not needed anymore
+ 2. Guest writes
+ 3. Check that copy-before-write operation is not done
+ """
+ self.do_backup()
+
+ # Try trigger copy-before-write operation
+ result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
+ self.assert_qmp(result, 'return', '')
+
+ # Check that data is not written to temporary image, as region
+ # is discarded from copy-before-write process
+ self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'],
+ supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/backup-discard-source.out b/tests/qemu-iotests/tests/backup-discard-source.out
new file mode 100644
index 0000000000..fbc63e62f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/backup-discard-source.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
--
2.34.1On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote: > Add test for a new backup option: discard-source. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Reviewed-by: Fiona Ebner <f.ebner@proxmox.com> > Tested-by: Fiona Ebner <f.ebner@proxmox.com> > Message-Id: <20240313152822.626493-6-vsementsov@yandex-team.ru> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > .../qemu-iotests/tests/backup-discard-source | 152 ++++++++++++++++++ > .../tests/backup-discard-source.out | 5 + > 2 files changed, 157 insertions(+) > create mode 100755 tests/qemu-iotests/tests/backup-discard-source > create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out This fails check-python-minreqs https://gitlab.com/qemu-project/qemu/-/jobs/6739551782 It appears to be a pylint issue. r~
[Add John]
On 29.04.24 17:18, Richard Henderson wrote:
> On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:
>> Add test for a new backup option: discard-source.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
>> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
>> Message-Id: <20240313152822.626493-6-vsementsov@yandex-team.ru>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> .../qemu-iotests/tests/backup-discard-source | 152 ++++++++++++++++++
>> .../tests/backup-discard-source.out | 5 +
>> 2 files changed, 157 insertions(+)
>> create mode 100755 tests/qemu-iotests/tests/backup-discard-source
>> create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
>
> This fails check-python-minreqs
>
> https://gitlab.com/qemu-project/qemu/-/jobs/6739551782
>
> It appears to be a pylint issue.
>
>
tests/export-incoming-iothread:1:0: R0801: Similar lines in 2 files
==tests.backup-discard-source:[52:61]
==tests.copy-before-write:[54:63]
'file': {
'driver': iotests.imgfmt,
'file': {
'driver': 'file',
'filename': source_img,
}
},
'target': {
'driver': iotests.imgfmt, (duplicate-code)
Hmm. I don't think, that something should be changed in code. splitting out part of this json object to a function? That's a test for QMP command, and it's good that we see the command as is in one place. I can swap some lines or rename variables to hack the linter, but I'd prefer not doing so:)
For me that looks like a false-positive: yes it's a duplication, but it should better be duplication, than complicating raw json objects by reusing common parts.
What to do? As described in 22305c2a081b8b6 "python: Reduce strictness of pylint's duplicate-code check", this check could be either be disabled completely, or we can increase min-similarity-lines config value.
I'd just disable it completely. Any thoughts?
--
Best regards,
Vladimir
Am 29.04.2024 um 20:39 hat Vladimir Sementsov-Ogievskiy geschrieben:
> [Add John]
>
> On 29.04.24 17:18, Richard Henderson wrote:
> > On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:
> > > Add test for a new backup option: discard-source.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> > > Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> > > Message-Id: <20240313152822.626493-6-vsementsov@yandex-team.ru>
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > > .../qemu-iotests/tests/backup-discard-source | 152 ++++++++++++++++++
> > > .../tests/backup-discard-source.out | 5 +
> > > 2 files changed, 157 insertions(+)
> > > create mode 100755 tests/qemu-iotests/tests/backup-discard-source
> > > create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
> >
> > This fails check-python-minreqs
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/6739551782
> >
> > It appears to be a pylint issue.
> >
> >
>
> tests/export-incoming-iothread:1:0: R0801: Similar lines in 2 files
> ==tests.backup-discard-source:[52:61]
> ==tests.copy-before-write:[54:63]
> 'file': {
> 'driver': iotests.imgfmt,
> 'file': {
> 'driver': 'file',
> 'filename': source_img,
> }
> },
> 'target': {
> 'driver': iotests.imgfmt, (duplicate-code)
>
>
>
> Hmm. I don't think, that something should be changed in code.
> splitting out part of this json object to a function? That's a test
> for QMP command, and it's good that we see the command as is in one
> place. I can swap some lines or rename variables to hack the linter,
> but I'd prefer not doing so:)
>
>
> For me that looks like a false-positive: yes it's a duplication, but
> it should better be duplication, than complicating raw json objects by
> reusing common parts.
>
>
> What to do? As described in 22305c2a081b8b6 "python: Reduce strictness
> of pylint's duplicate-code check", this check could be either be
> disabled completely, or we can increase min-similarity-lines config
> value.
>
> I'd just disable it completely. Any thoughts?
I think it would make sense to treat tests differently from real
production code. In tests/, some duplication is bound to happen and
trying to unify things across test cases (which would mean moving to
iotests.py) often doesn't make sense. On the other hand, in python/ we
would probably want to unify duplicated code.
Kevin
On 30.04.24 12:13, Kevin Wolf wrote:
> Am 29.04.2024 um 20:39 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> [Add John]
>>
>> On 29.04.24 17:18, Richard Henderson wrote:
>>> On 4/29/24 04:51, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add test for a new backup option: discard-source.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
>>>> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
>>>> Message-Id: <20240313152822.626493-6-vsementsov@yandex-team.ru>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> ---
>>>> .../qemu-iotests/tests/backup-discard-source | 152 ++++++++++++++++++
>>>> .../tests/backup-discard-source.out | 5 +
>>>> 2 files changed, 157 insertions(+)
>>>> create mode 100755 tests/qemu-iotests/tests/backup-discard-source
>>>> create mode 100644 tests/qemu-iotests/tests/backup-discard-source.out
>>>
>>> This fails check-python-minreqs
>>>
>>> https://gitlab.com/qemu-project/qemu/-/jobs/6739551782
>>>
>>> It appears to be a pylint issue.
>>>
>>>
>>
>> tests/export-incoming-iothread:1:0: R0801: Similar lines in 2 files
>> ==tests.backup-discard-source:[52:61]
>> ==tests.copy-before-write:[54:63]
>> 'file': {
>> 'driver': iotests.imgfmt,
>> 'file': {
>> 'driver': 'file',
>> 'filename': source_img,
>> }
>> },
>> 'target': {
>> 'driver': iotests.imgfmt, (duplicate-code)
>>
>>
>>
>> Hmm. I don't think, that something should be changed in code.
>> splitting out part of this json object to a function? That's a test
>> for QMP command, and it's good that we see the command as is in one
>> place. I can swap some lines or rename variables to hack the linter,
>> but I'd prefer not doing so:)
>>
>>
>> For me that looks like a false-positive: yes it's a duplication, but
>> it should better be duplication, than complicating raw json objects by
>> reusing common parts.
>>
>>
>> What to do? As described in 22305c2a081b8b6 "python: Reduce strictness
>> of pylint's duplicate-code check", this check could be either be
>> disabled completely, or we can increase min-similarity-lines config
>> value.
>>
>> I'd just disable it completely. Any thoughts?
>
> I think it would make sense to treat tests differently from real
> production code. In tests/, some duplication is bound to happen and
> trying to unify things across test cases (which would mean moving to
> iotests.py) often doesn't make sense. On the other hand, in python/ we
> would probably want to unify duplicated code.
>
Agree. Happily, it turns out that tests already have separate tests/qemu-iotests/pylintrc file, so that's not a problem. Still I decided to anot disable the check completely, but just bump the limit, see "[PATCH] iotests/pylintrc: allow up to 10 similar lines"
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.