qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 09/11] iotests: test 124 - drive object refactor


From: John Snow
Subject: Re: [Qemu-devel] [PATCH 09/11] iotests: test 124 - drive object refactoring
Date: Tue, 17 Mar 2015 19:40:32 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0



On 03/17/2015 04:44 PM, Max Reitz wrote:
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 <address@hidden>
---
  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?


I don't want to startle the cat.


I'd rather get the 'transactionless' checked in, then worry about features that are important for transactions in the series that introduces those features.

If we keep thinking "Oh, but this function hasn't gone upstream in the first place, so let's fix it there" in this series, then the first series getting committed is going to wind up dependent on this series getting reviewed as we keep changing things.

Since this group of patches is more experimental, I'd really prefer to keep it separate that way.

Even if this patch is ugly. Sorry about that.

Reviewed-by: Max Reitz <address@hidden>

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__))


Can't please everyone. :)

+
          # 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


You're right! I missed one.

-            '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')





reply via email to

[Prev in Thread] Current Thread [Next in Thread]