Nir Soffer has posted comments on this change.

Change subject: storage: Fix abort race in SDM.copy_data
......................................................................


Patch Set 5: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/65150/5/tests/storage_sdm_copy_data_test.py
File tests/storage_sdm_copy_data_test.py:

Line 309:                 job_id = make_uuid()
Line 310:                 job = storage.sdm.api.copy_data.Job(job_id, 0, 
source, dest)
Line 311:                 # Simulate status changing to aborting right after 
_run called
Line 312:                 job._run = partial(_fake_run, job, job._run)
Line 313:                 job.run()
I think we can simplify - since we are already using job._run, we can simulate 
the framework:

    job = Job()
    job._status = RUNNING
    job.abort()
    
The job should be in ABORTING state, so:

    with self.assertRaises(ActionStopped):
        job._run()
Line 314:                 self.assertEqual(jobs.STATUS.ABORTED, job.status)
Line 315:                 self.assertEqual(sc.LEGAL_VOL, dst_vol.getLegality())
Line 316:                 self.assertEqual(gen_id, 
dst_vol.getMetaParam(sc.GENERATION))
Line 317: 


https://gerrit.ovirt.org/#/c/65150/5/vdsm/storage/sdm/api/copy_data.py
File vdsm/storage/sdm/api/copy_data.py:

Line 73: 
Line 74:                 with self._status_lock:
Line 75:                     # Do not start copying if we have already been 
aborted
Line 76:                     if self._status == jobs.STATUS.ABORTING:
Line 77:                         raise exception.ActionStopped()
This will be needed by every job supporting abort. We probably need to add a 
helper in the framework, like _check_aborting.

We also need to rename the exception to Aborted.
Line 78:                     self._operation = qemuimg.convert(
Line 79:                         self._source.path,
Line 80:                         self._dest.path,
Line 81:                         srcFormat=src_format,


-- 
To view, visit https://gerrit.ovirt.org/65150
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c7d9aadb981e8b851a70c7ad0fdc0d75b46dc51
Gerrit-PatchSet: 5
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
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to