Change in vdsm[master]: sp: allow executing upgradePool even if there is a pending u...

nsoffer at redhat.com nsoffer at redhat.com
Tue Sep 8 12:41:36 UTC 2015


Nir Soffer 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:
> plese elaborate, i didn't understand what you meant to.
try finaly is fragile and error prone, do not use it when you can use with.
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)
> Instead of failing immediately when len(self._domainsToUpgrade) > 0 we have
_shutDownUpgrade is a list, which is thread safe in Python.

If we fixed the issue of cleaning up pending upgrades when deactivating a domain, a user can put domain to maintenance if the domain is not accessible (the reason the upgrade is left pending).

If we want to support this, maybe only in master, we need a simple change that does not try to lock so many things in the same time.

For example, keeping upgrade state in the pool, so we can easily fail here if a domain is upgrade is running, and a domain upgrade will be canceled if pull upgrade was started. Having to take 10's of locks to manage state is not maintainable.

Or maybe using current code - did you look in _shutdownUpgrade?

Not sure that we should get into this tricky stuff, the current code failing on non-empty _domainsToUpgrade is much safer.
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()
> same as the rest of the code - we are under the assumption that it shouldn'
Having bad code elsewhere is not  a good reason to add more.
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 <laravot at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list