Change in vdsm[master]: storage: Fix abort race in SDM.copy_data

[nsoffer]

Nir Soffer has posted comments on this change.

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


Patch Set 2: Code-Review-1

(2 comments)

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

Line 82
Line 83
Line 84
Line 85
Line 86
I wonder if we should merge start and wait_for_completion() to run():

    with self._dest.volume_operation():
        self._operation.run()

We can keep separate start() and wait() methods, or add them later if needed.

I like to have generic start(), stop(), wait(), and run() for everything, if 
possible.

Did you consider to create the operation inside the volume operation context?

    with guarded.context(locks):
        with prepare(src), prepare(dst):
            with dst.volume_operation:
                with self._status_lock:
                    if self._status == ABORTING:
                        raise ActionStopped
                    self._operation = qemuimg.convert(...)
                self._operation.run()

This means that we check the state right before we start the process. If the 
job was aborted during locking, taking the lease, setting status to illegal, we 
abort without starting qemu.


Line 60:         if self._operation:
Line 61:             self._operation.abort()
Line 62: 
Line 63:     def _run(self):
Line 64:         print "called: status=%r" % self.status
leftover?
Line 65:         with guarded.context(self._source.locks + self._dest.locks):
Line 66:             with self._source.prepare(), self._dest.prepare():
Line 67:                 # Workaround for volumes containing VM configuration 
info that
Line 68:                 # were created with invalid vdsm metadata.


-- 
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: 2
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