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 <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
