Change in vdsm[master]: sp: improve domainStateChange event handling

ishaby at redhat.com ishaby at redhat.com
Wed Jan 6 13:25:26 UTC 2016


Idan Shaby has posted comments on this change.

Change subject: sp: improve domainStateChange event handling
......................................................................


Patch Set 3:

(9 comments)

https://gerrit.ovirt.org/#/c/51393/3//COMMIT_MSG
Commit Message:

Line 8: 
Line 9: onDomainStateChange is registered in the constructor of StoragePool.
Line 10: This is a waste of resources since these events can be called only after
Line 11: the connect method is called (specifically, after __rebuild is called
Line 12: from within connect).
> Correction - the main issue is an issue - we do a new registeration for eac
Done
Line 13: 
Line 14: This patch moves the registration to onDomainStateChange to be right
Line 15: before it can actually be called (in connect), and calls unregister if
Line 16: an error has occurred in __rebuild.


Line 18: won't be listening to events that cannot be called anymore.
Line 19: 
Line 20: In addition, this patch locks the pool and the domain id's in
Line 21: _domainStateChange to prevent concurrent calls to _refreshDomainLinks
Line 22: from other threads.
> We take a shared lock on the pool since we use query pool state, and an exc
Done
Line 23: 
Line 24: Change-Id: If116100d3ae967f6a5490a2d91bf923e953cb4ee


https://gerrit.ovirt.org/#/c/51393/3/vdsm/storage/sp.py
File vdsm/storage/sp.py:

Line 640
Line 641
Line 642
Line 643
Line 644
> Add comment about starting monitoring before __rebuild, since it starts (in
Done


Line 641
Line 642
Line 643
Line 644
Line 645
> If this raises, the pool is not considered connected, and we should stop wa
Done


Line 139: 
Line 140:     def _domainStateChange(self, sdUUID, isValid):
Line 141:         if not isValid:
Line 142:             return
Line 143: 
> Produce the domain here, add comment about producing it before taking locks
Done
Line 144:         with rmanager.acquireResource(STORAGE, self.spUUID,
Line 145:                                       rm.LockType.shared):
Line 146:             if sdUUID not in self.getDomains():
Line 147:                 self.log.debug("Domain %s is not a member of pool %s, "


Line 150:                 return
Line 151:             with rmanager.acquireResource(STORAGE, sdUUID,
Line 152:                                           rm.LockType.exclusive):
Line 153:                 self.log.debug("Refreshing domain links for %s", sdUUID)
Line 154:                 self._refreshDomainLinks(sdCache.produce(sdUUID))
> We should produce the domain before locking, since it may block for long ti
Done
Line 155: 
Line 156:     def _upgradePoolDomain(self, sdUUID, isValid):
Line 157:         # This method is called everytime the onDomainStateChange
Line 158:         # event is emitted, this event is emitted even when a domain goes


Line 663:         return True
Line 664: 
Line 665:     @unsecured
Line 666:     def _startWatchingDomainsState(self):
Line 667:         self.log.debug("Starting to monitor domains state")
> Starting to monitor -> Start monitoring
Done
Line 668:         self.domainMonitor.onDomainStateChange.register(
Line 669:             self._domainStateCallback)
Line 670: 
Line 671:     @unsecured


Line 669:             self._domainStateCallback)
Line 670: 
Line 671:     @unsecured
Line 672:     def _stopWatchingDomainsState(self):
Line 673:         self.log.debug("Stopping to monitor domains state")
> Stopping to monitor -> Stop monitoring
Done
Line 674:         self.domainMonitor.onDomainStateChange.unregister(
Line 675:             self._domainStateCallback)
Line 676: 
Line 677:     @unsecured


Line 671:     @unsecured
Line 672:     def _stopWatchingDomainsState(self):
Line 673:         self.log.debug("Stopping to monitor domains state")
Line 674:         self.domainMonitor.onDomainStateChange.unregister(
Line 675:             self._domainStateCallback)
> You should handle KeyError here, see _finalizePoolUpgradeIfNeeded, but log 
Done
Line 676: 
Line 677:     @unsecured
Line 678:     def stopMonitoringDomains(self):
Line 679:         self.domainMonitor.stopMonitoring(self.domainMonitor.poolDomains)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If116100d3ae967f6a5490a2d91bf923e953cb4ee
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby 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: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list