Dan Kenigsberg has posted comments on this change.
Change subject: BZ#744704 - Add rollback support for complex tasks ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(5 inline comments)
.................................................... File vdsm/storage/blockVolume.py Line 249: cls.log.warn("Cannot deactivate new created volume %s/%s", sdUUID, volUUID, exc_info=True) Line 250: Line 251: # Remove all previous rollbacks for 'halfbaked' volume and add rollback for 'real' volume creation Line 252: vars.task.replaceRecoveries(task.Recovery("create block volume rollback", "blockVolume", "BlockVolume", "createVolumeRollback", Line 253: [repoPath, sdUUID, imgUUID, volUUID, imageDir]), True) seems like a good place to use python's named args.. I almost forgot what True means here. Line 254: return volUUID Line 255: Line 256: Line 257: def delete(self, postZero, force):
.................................................... File vdsm/storage/task.py Line 73: FIELD_SEP = "," Line 74: RESOURCE_SEP = "!" Line 75: TASK_METADATA_VERSION = 1 Line 76: Line 77: STUB_ROLLBACK = "stub rollback" that's actually commony called "sentinel" http://en.wikipedia.org/wiki/Sentinel_value Line 78: Line 79: class State: Line 80: unknown = "unknown" Line 81: init = "init"
Line 1047: self.recoveries.append(recovery) Line 1048: self.persist() Line 1049: Line 1050: Line 1051: def replaceRecoveries(self, recovery, singleTask=False): Is there ANY need to keep the former semantic? Removing unrelated tasks feels wrong and misleading. Who needs it? Have him call another replaceAllRecoveries function. Line 1052: if not isinstance(recovery, Recovery): Line 1053: raise TypeError("recovery param %s(%s) must be Recovery object" % (repr(recovery), type(recovery))) Line 1054: if not recovery.name: Line 1055: raise ValueError("replaceRecoveries: name is required")
.................................................... File vdsm/storage/volume.py Line 327: @classmethod Line 328: def startCreateVolumeRollback(cls, taskObj, sdUUID, imgUUID, volUUID): Line 329: cls.log.info("startCreateVolumeRollback: sdUUID=%s imgUUID=%s "\ Line 330: "volUUID=%s " % ( sdUUID, imgUUID, volUUID)) Line 331: # This rollback it's actually do nothing. it's --> doesn't Line 332: # In general the createVolume rollbacks are a list of small rollbacks Line 333: # that replaced by the one major rollback at the end of the task. Line 334: # This rollback is a simple marker that must to be the first rollback Line 335: # in the list of createVolume rollbacks.
Line 329: cls.log.info("startCreateVolumeRollback: sdUUID=%s imgUUID=%s "\ Line 330: "volUUID=%s " % ( sdUUID, imgUUID, volUUID)) Line 331: # This rollback it's actually do nothing. Line 332: # In general the createVolume rollbacks are a list of small rollbacks Line 333: # that replaced by the one major rollback at the end of the task. that replaced -> that are replaced Line 334: # This rollback is a simple marker that must to be the first rollback Line 335: # in the list of createVolume rollbacks. Line 336: # We need it in cases when createVolume is a part of more complex task and Line 337: # not a task by itself. In such cases when we will replace the list of
-- To view, visit http://gerrit.usersys.redhat.com/1041 To unsubscribe, visit http://gerrit.usersys.redhat.com/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ie36ac02475f3f6438a63fbcbd26730862216cebd Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Ayal Baron Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
vdsm-patches@lists.fedorahosted.org