From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50871) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXyLl-0005Cu-E9 for qemu-devel@nongnu.org; Tue, 17 Mar 2015 16:44:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXyLh-0007pu-7a for qemu-devel@nongnu.org; Tue, 17 Mar 2015 16:44:29 -0400 Message-ID: <55089225.9080104@redhat.com> Date: Tue, 17 Mar 2015 16:44:21 -0400 From: Max Reitz MIME-Version: 1.0 References: <1425528911-10300-1-git-send-email-jsnow@redhat.com> <1425528911-10300-10-git-send-email-jsnow@redhat.com> In-Reply-To: <1425528911-10300-10-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 09/11] iotests: test 124 - drive object refactoring List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 2015-03-04 at 23:15, John Snow wrote: > The original test was not particularly good about keeping the > relationships between bitmaps, drives, and images very explicit, > so this patch adds an explicit 'drive' dict that is used to > keep these relationships explicit. > > This is necessary in order to test two full backup chains > simultaneously for two drives, which will happen in a forthcoming > test that examines failure scenarios for incremental backups created > for multiple drives in a single transaction. > > Highlights: > > - Each test case carries around a list of drives > - Each bitmap now acknowledges the full backup belonging to the drive > as its "last target" if it hasn't made an incremental backup yet. > - Most functions generally try to accept a drive argument instead of > target or format arguments. > - Filenames are now based on their formatting and id name. > > Signed-off-by: John Snow > --- > tests/qemu-iotests/124 | 212 +++++++++++++++++++++++++++++-------------------- > 1 file changed, 126 insertions(+), 86 deletions(-) How about putting the previous patch in your transactionless series and squashing this one into the respective patches there? Reviewed-by: Max Reitz Some remarks below, whether you follow them or not is up to you. > diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124 > index da0bf6d..2eccc3e 100644 > --- a/tests/qemu-iotests/124 > +++ b/tests/qemu-iotests/124 > @@ -29,14 +29,18 @@ def io_write_patterns(img, patterns): > iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img) > > class Bitmap: > - def __init__(self, name, node): > + def __init__(self, name, drive): > self.name = name > - self.node = node > + self.drive = drive > self.pattern = os.path.join(iotests.test_dir.replace('%', '%%'), > - '%s.backup.%%i.img' % name) > + '%s.%s.backup.%%i.img' % (drive['id'], > + name)) > self.num = 0 > self.backups = list() > > + def base_target(self): > + return self.drive['backup'] > + > def new_target(self, num=None): > if num is None: > num = self.num > @@ -46,7 +50,9 @@ class Bitmap: > return target > > def last_target(self): > - return self.backups[-1] > + if self.backups: > + return self.backups[-1] > + return self.base_target() > > def del_target(self): > os.remove(self.backups.pop()) > @@ -63,19 +69,35 @@ class TestIncrementalBackup(iotests.QMPTestCase): > def setUp(self): > self.bitmaps = list() > self.files = list() > + self.drives = list() > self.vm = iotests.VM() > - self.test_img = os.path.join(iotests.test_dir, 'base.img') > - self.full_bak = os.path.join(iotests.test_dir, 'backup.img') > - self.foo_img = os.path.join(iotests.test_dir, 'foo.bar') > - self.img_create(self.test_img, iotests.imgfmt) > - self.vm.add_drive(self.test_img) > + self.err_img = os.path.join(iotests.test_dir, 'err.%s' % iotests.imgfmt) Nice. *g* (using the format name as the file extension) ((even though you're not doing it in __init__)) > + > # Create a base image with a distinctive patterning > - io_write_patterns(self.test_img, (('0x41', 0, 512), > - ('0xd5', '1M', '32k'), > - ('0xdc', '32M', '124k'))) > + drive0 = self.add_node('drive0') > + self.img_create(drive0['file'], drive0['fmt']) > + self.vm.add_drive(drive0['file']) > + io_write_patterns(drive0['file'], (('0x41', 0, 512), > + ('0xd5', '1M', '32k'), > + ('0xdc', '32M', '124k'))) > self.vm.launch() > > > + def add_node(self, node_id, fmt=iotests.imgfmt, path=None, backup=None): > + if path is None: > + path = os.path.join(iotests.test_dir, '%s.%s' % (node_id, fmt)) > + if backup is None: > + backup = os.path.join(iotests.test_dir, > + '%s.full.backup.%s' % (node_id, fmt)) > + > + self.drives.append({ > + 'id': node_id, > + 'file': path, > + 'backup': backup, > + 'fmt': fmt }) > + return self.drives[-1] > + > + > def img_create(self, img, fmt=iotests.imgfmt, size='64M', > parent=None, parentFormat=None): > plist = list() > @@ -89,25 +111,31 @@ class TestIncrementalBackup(iotests.QMPTestCase): > self.files.append(img) > > > - def create_full_backup(self, drive='drive0'): > - res = self.vm.qmp('drive-backup', device=drive, > - sync='full', format=iotests.imgfmt, > - target=self.full_bak) > + def create_full_backup(self, drive=None): > + if drive is None: > + drive = self.drives[-1] > + > + res = self.vm.qmp('drive-backup', device=drive['id'], > + sync='full', format=drive['fmt'], > + target=drive['backup']) > self.assert_qmp(res, 'return', {}) > - self.wait_until_completed(drive) > - self.check_full_backup() > - self.files.append(self.full_bak) > + self.wait_until_completed(drive['id']) > + self.check_full_backup(drive) > + self.files.append(drive['backup']) > + return drive['backup'] > > > - def check_full_backup(self): > - self.assertTrue(iotests.compare_images(self.test_img, self.full_bak)) > + def check_full_backup(self, drive=None): > + if drive is None: > + drive = self.drives[-1] > + self.assertTrue(iotests.compare_images(drive['file'], drive['backup'])) > > > - def add_bitmap(self, name, node='drive0'): > - bitmap = Bitmap(name, node) > + def add_bitmap(self, name, drive): > + bitmap = Bitmap(name, drive) > self.bitmaps.append(bitmap) > - result = self.vm.qmp('block-dirty-bitmap-add', node=bitmap.node, > - name=bitmap.name) > + result = self.vm.qmp('block-dirty-bitmap-add', node=drive['id'], > + name=name) No real need to replace bitmap.name by name here, but no harm in doing it. > self.assert_qmp(result, 'return', {}) > return bitmap > > @@ -117,37 +145,43 @@ class TestIncrementalBackup(iotests.QMPTestCase): > if bitmap is None: > bitmap = self.bitmaps[-1] > > - # If this is the first incremental backup for a bitmap, > - # use the full backup as a backing image. Otherwise, use > - # the last incremental backup. > if parent is None: > - if bitmap.num == 0: > - parent = self.full_bak > - else: > - parent = bitmap.last_target() > + parent = bitmap.last_target() > > target = bitmap.new_target(num) > - self.img_create(target, iotests.imgfmt, parent=parent) > + self.img_create(target, bitmap.drive['fmt'], parent=parent) > > - result = self.vm.qmp('drive-backup', device=bitmap.node, > + result = self.vm.qmp('drive-backup', device=bitmap.drive['id'], > sync='dirty-bitmap', bitmap=bitmap.name, > - format=iotests.imgfmt, target=target, > + format=bitmap.drive['fmt'], target=target, > mode='existing') > self.assert_qmp(result, 'return', {}) > > - event = self.wait_until_completed(bitmap.node, check_offset=validate, > - allow_failures=(not validate)) > - if 'error' in event['data']: > + return self.wait_incremental(bitmap, validate) > + > + > + def wait_incremental(self, bitmap=None, > + validate=True, error='Input/output error'): > + if bitmap is None: > + bitmap = self.bitmaps[-1] > + > + event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED", > + match={'data': {'device': > + bitmap.drive['id']}}) > + if validate: > + self.assert_qmp_absent(event, 'data/error') > + return self.check_incremental(bitmap) > + else: > + self.assert_qmp(event, 'data/error', error) > bitmap.del_target() > return False > - if validate: > - return self.check_incremental(target) > > > - def check_incremental(self, target=None): > - if target is None: > - target = self.bitmaps[-1].last_target() > - self.assertTrue(iotests.compare_images(self.test_img, target)) > + def check_incremental(self, bitmap=None): > + if bitmap is None: > + bitmap = self.bitmaps[-1] > + self.assertTrue(iotests.compare_images(bitmap.drive['file'], > + bitmap.last_target())) > return True > > > @@ -166,20 +200,20 @@ class TestIncrementalBackup(iotests.QMPTestCase): > i.e.; after IO requests begin modifying the drive. > ''' > self.create_full_backup() > - self.add_bitmap('bitmap0', 'drive0') > + self.add_bitmap('bitmap0', self.drives[0]) > > # Sanity: Create a "hollow" incremental backup > self.create_incremental() > # Three writes: One complete overwrite, one new segment, > # and one partial overlap. > - self.hmp_io_writes('drive0', (('0xab', 0, 512), > - ('0xfe', '16M', '256k'), > - ('0x64', '32736k', '64k'))) > + self.hmp_io_writes(self.drives[0]['id'], (('0xab', 0, 512), > + ('0xfe', '16M', '256k'), > + ('0x64', '32736k', '64k'))) > self.create_incremental() > # Three more writes, one of each kind, like above > - self.hmp_io_writes('drive0', (('0x9a', 0, 512), > - ('0x55', '8M', '352k'), > - ('0x78', '15872k', '1M'))) > + self.hmp_io_writes(self.drives[0]['id'], (('0x9a', 0, 512), > + ('0x55', '8M', '352k'), > + ('0x78', '15872k', '1M'))) > self.create_incremental() > return True > > @@ -191,41 +225,43 @@ class TestIncrementalBackup(iotests.QMPTestCase): > bitmap AFTER writes have already occurred. Use transactions to create > a full backup and synchronize both bitmaps to this backup. > Create an incremental backup through both bitmaps and verify that > - both backups match the full backup. > + both backups match the current drive0 image. > ''' > - bitmap0 = self.add_bitmap('bitmap0', 'drive0') > - self.hmp_io_writes('drive0', (('0xab', 0, 512), > - ('0xfe', '16M', '256k'), > - ('0x64', '32736k', '64k'))) > - bitmap1 = self.add_bitmap('bitmap1', 'drive0') > + > + drive0 = self.drives[0] > + bitmap0 = self.add_bitmap('bitmap0', self.drives[0]) > + self.hmp_io_writes(drive0['id'], (('0xab', 0, 512), > + ('0xfe', '16M', '256k'), > + ('0x64', '32736k', '64k'))) > + bitmap1 = self.add_bitmap('bitmap1', drive0) > > result = self.vm.qmp('transaction', actions=[ > { > 'type': 'block-dirty-bitmap-clear', > - 'data': { 'node': 'drive0', > - 'name': 'bitmap0' }, > + 'data': { 'node': bitmap0.drive['id'], > + 'name': bitmap0.name }, > }, > { > 'type': 'block-dirty-bitmap-clear', > - 'data': { 'node': 'drive0', > - 'name': 'bitmap1' }, > + 'data': { 'node': bitmap1.drive['id'], > + 'name': bitmap1.name }, > }, > { > 'type': 'drive-backup', > - 'data': { 'device': 'drive0', > + 'data': { 'device': drive0['id'], > 'sync': 'full', > - 'format': iotests.imgfmt, > - 'target': self.full_bak }, > + 'format': drive0['fmt'], > + 'target': drive0['backup'] }, > } > ]) > self.assert_qmp(result, 'return', {}) > self.wait_until_completed() > - self.files.append(self.full_bak) > + self.files.append(drive0['backup']) > self.check_full_backup() > > - self.hmp_io_writes('drive0', (('0x9a', 0, 512), > - ('0x55', '8M', '352k'), > - ('0x78', '15872k', '1M'))) > + self.hmp_io_writes(drive0['id'], (('0x9a', 0, 512), > + ('0x55', '8M', '352k'), > + ('0x78', '15872k', '1M'))) > # Both bitmaps should be in sync and create fully valid > # incremental backups > res1 = self.create_incremental(bitmap0) > @@ -241,15 +277,19 @@ class TestIncrementalBackup(iotests.QMPTestCase): > afterwards and verify that the backup created is correct. > ''' > > - # Create a blkdebug interface to this img as 'drive1' > + # Create a blkdebug interface to this img as 'drive1', > + # but don't actually create a new image. > + drive1 = self.add_node('drive1', self.drives[0]['fmt'], > + path=self.drives[0]['file'], > + backup=self.drives[0]['backup']) > result = self.vm.qmp('blockdev-add', options={ > 'id': 'drive1', "'id': drive1['id']" would work, too. Max > - 'driver': iotests.imgfmt, > + 'driver': drive1['fmt'], > 'file': { > 'driver': 'blkdebug', > 'image': { > 'driver': 'file', > - 'filename': self.test_img > + 'filename': drive1['file'] > }, > 'set-state': [{ > 'event': 'flush_to_disk', > @@ -267,38 +307,38 @@ class TestIncrementalBackup(iotests.QMPTestCase): > }) > self.assert_qmp(result, 'return', {}) > > - self.create_full_backup() > - self.add_bitmap('bitmap0', 'drive1') > + self.create_full_backup(self.drives[0]) > + self.add_bitmap('bitmap0', drive1) > # Note: at this point, during a normal execution, > # Assume that the VM resumes and begins issuing IO requests here. > > - self.hmp_io_writes('drive1', (('0xab', 0, 512), > - ('0xfe', '16M', '256k'), > - ('0x64', '32736k', '64k'))) > + self.hmp_io_writes(drive1['id'], (('0xab', 0, 512), > + ('0xfe', '16M', '256k'), > + ('0x64', '32736k', '64k'))) > > result = self.create_incremental(validate=False) > self.assertFalse(result) > - self.hmp_io_writes('drive1', (('0x9a', 0, 512), > - ('0x55', '8M', '352k'), > - ('0x78', '15872k', '1M'))) > + self.hmp_io_writes(drive1['id'], (('0x9a', 0, 512), > + ('0x55', '8M', '352k'), > + ('0x78', '15872k', '1M'))) > self.create_incremental() > > > def test_sync_dirty_bitmap_missing(self): > self.assert_no_active_block_jobs() > - self.files.append(self.foo_img) > - result = self.vm.qmp('drive-backup', device='drive0', > - sync='dirty-bitmap', format=iotests.imgfmt, > - target=self.foo_img) > + self.files.append(self.err_img) > + result = self.vm.qmp('drive-backup', device=self.drives[0]['id'], > + sync='dirty-bitmap', format=self.drives[0]['fmt'], > + target=self.err_img) > self.assert_qmp(result, 'error/class', 'GenericError') > > > def test_sync_dirty_bitmap_not_found(self): > self.assert_no_active_block_jobs() > - self.files.append(self.foo_img) > - result = self.vm.qmp('drive-backup', device='drive0', > + self.files.append(self.err_img) > + result = self.vm.qmp('drive-backup', device=self.drives[0]['id'], > sync='dirty-bitmap', bitmap='unknown', > - format=iotests.imgfmt, target=self.foo_img) > + format=self.drives[0]['fmt'], target=self.err_img) > self.assert_qmp(result, 'error/class', 'GenericError') > >