Nir Soffer has posted comments on this change. Change subject: Implement SDM.copy_data ......................................................................
Patch Set 1: (8 comments) https://gerrit.ovirt.org/#/c/60420/1/tests/storage_sdm_copy_data_test.py File tests/storage_sdm_copy_data_test.py: Line 45: Line 46: Line 47: def qemu_io_write(path): Line 48: cmd = ['qemu-io', '-f', 'raw', '-c', 'write -P %s 512 1k' % PATTERN, path] Line 49: subprocess.check_output(cmd, shell=False) shell=False is the default, unneeded. It would be better to use exeCmd, which logs the command the response. You can wrap it with a function that raises when the command fails. Line 50: Line 51: Line 52: def qemu_io_verify(path): Line 53: cmd = ['qemu-io', '-f', 'raw', Line 50: Line 51: Line 52: def qemu_io_verify(path): Line 53: cmd = ['qemu-io', '-f', 'raw', Line 54: '-c', 'read -P %s -s 0 -l 1k 512 1k' % PATTERN, path] This is not a good verification, you want to verify that the part before the pattern and after the pattern are same in the source and the destination. So maybe check 2 blocks - block before, modified block, block after. Line 55: out = subprocess.check_output(cmd, shell=False) Line 56: return "Pattern verification failed" not in out Line 57: Line 58: Line 51: Line 52: def qemu_io_verify(path): Line 53: cmd = ['qemu-io', '-f', 'raw', Line 54: '-c', 'read -P %s -s 0 -l 1k 512 1k' % PATTERN, path] Line 55: out = subprocess.check_output(cmd, shell=False) shell unneeded Line 56: return "Pattern verification failed" not in out Line 57: Line 58: Line 59: @expandPermutations Line 52: def qemu_io_verify(path): Line 53: cmd = ['qemu-io', '-f', 'raw', Line 54: '-c', 'read -P %s -s 0 -l 1k 512 1k' % PATTERN, path] Line 55: out = subprocess.check_output(cmd, shell=False) Line 56: return "Pattern verification failed" not in out This should raise AssetionError, will simplify the tests. Lets move these to storagetestlib, seems very useful. Line 57: Line 58: Line 59: @expandPermutations Line 60: class CopyDataTests(VdsmTestCase): Line 68: make_env = fake_file_env Line 69: elif env_type == 'block': Line 70: make_env = fake_block_env Line 71: else: Line 72: raise ValueError("Invalid env_type: %r" % env_type) This mean we need a fake_env getting a type instead of fake_file_env and fake_block_env. Line 73: Line 74: with make_env() as env: Line 75: rm = FakeResourceManager() Line 76: with MonkeyPatchScope([ Line 103: img_id=src_vol.imgUUID, vol_id=src_vol.volUUID)) Line 104: destination = storage.sdm.api.copy_data.CopyDataEndpoint( Line 105: dict(endpoint_type='div', sd_id=dst_vol.sdUUID, Line 106: img_id=dst_vol.imgUUID, vol_id=dst_vol.volUUID)) Line 107: args = dict(job_id=job_id, source=source, destination=destination) We don't need this, just call it with the parameters. Line 108: Line 109: self.assertFalse(qemu_io_verify(dst_vol.volumePath)) Line 110: job = storage.sdm.api.copy_data.Job(**args) Line 111: job.run() Line 106: img_id=dst_vol.imgUUID, vol_id=dst_vol.volUUID)) Line 107: args = dict(job_id=job_id, source=source, destination=destination) Line 108: Line 109: self.assertFalse(qemu_io_verify(dst_vol.volumePath)) Line 110: job = storage.sdm.api.copy_data.Job(**args) Would be simpler like this: job = storage.sdm.api.copy_data(job_id=job_id, source=source, destination=destination) Line 111: job.run() Line 112: wait_for_job(job) Line 113: Line 114: # TODO: Verify resources were acquired and released Line 111: job.run() Line 112: wait_for_job(job) Line 113: Line 114: # TODO: Verify resources were acquired and released Line 115: # TODO: Verify volumes were prepared and torn down This should be done in another test, this test is complex enough as is. Line 116: Line 117: self.assertEqual(jobs.STATUS.DONE, job.status) Line 118: self.assertEqual(100.0, job.progress) Line 119: self.assertNotIn('error', job.info()) -- To view, visit https://gerrit.ovirt.org/60420 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I30ff635c0c73f67b296033b4a506fc3b9ededfbe Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
