Liron Aravot has posted comments on this change. Change subject: sp: allow executing upgradePool even if there is a pending update ......................................................................
Patch Set 1: (3 comments) https://gerrit.ovirt.org/#/c/45774/1/vdsm/storage/sp.py File vdsm/storage/sp.py: Line 427: rm.LockType.exclusive): Line 428: acquiredDomainResources = [] Line 429: Line 430: sd.validateDomainVersion(targetDomVersion) Line 431: try: > Do not replace with with try finally. plese elaborate, i didn't understand what you meant to. Line 432: self.log.info("Trying to lock domains that are currently pending for an update") Line 433: for sdUUID in self._domainsToUpgrade: Line 434: try: Line 435: res = rmanager.acquireResource(STORAGE, "upgrade_" + sdUUID, Line 432: self.log.info("Trying to lock domains that are currently pending for an update") Line 433: for sdUUID in self._domainsToUpgrade: Line 434: try: Line 435: res = rmanager.acquireResource(STORAGE, "upgrade_" + sdUUID, Line 436: rm.LockType.exclusive) > Please do not change locking in this delicate parts. Instead of failing immediately when len(self._domainsToUpgrade) > 0 we have to take the lock for the domains pending for upgrade to avoid having the upgrade modify self._domainsToUpgrade concurrently. that seems like a reasonable change to me, can you please explain what is the issue you see with it or what is the other alternative? Line 437: except (rm.RequestTimedOutError, se.ResourceAcqusitionFailed): Line 438: self.log.info("Cannot acquire domain upgrade resource '%s'", str(sdUUID)) Line 439: raise se.PoolUpgradeInProgress(self.spUUID, self.sdUUID) Line 440: else: Line 453: except ValueError: Line 454: pass Line 455: finally: Line 456: for res in acquiredDomainResources: Line 457: res.release() > This is not safe, if res.release() raises, you are left with locks that nob same as the rest of the code - we are under the assumption that it shouldn't raise and if it does something is terribly broken (even if we do catch the exception, we might left with a locked resource, the one that we failed to release and what do we do then?) i can add here a try except, but that is the way we are doing that all over the code - see for example __getResourceCandidatesList() in resourceFactories.py. Line 458: Line 459: self.log.debug("Registering with state change event") Line 460: self.domainMonitor.onDomainStateChange.register( Line 461: self._upgradeCallback) -- To view, visit https://gerrit.ovirt.org/45774 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3c30439b5b84f64fd2ad69d82f33dfdfc4c9ef68 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Liron Aravot <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
