Ayal Baron has posted comments on this change. Change subject: Add the hostId parameter to reconstructMaster ......................................................................
Patch Set 13: (7 inline comments) Apparently it won't publish my comments on an old revision, copied and moved to newest. .................................................... File vdsm/storage/hsm.py Line 1489: pool = sp.StoragePool(spUUID, self.taskMng) shouldn't we keep the 'else' clause and check if dom version is <3 (for bc) .................................................... File vdsm/storage/sp.py Line 732: temporaryLock = None why not: temporaryLock = False ? Line 738: if not self.id and not hostId: why not limit this to <v3 storage domains only? i.e. instead of (or in addition to) checking the parameters, checking the version of the domain. This way if someone erroneously uses the deprecated method with a new domain it will fail. Line 745: temporaryLock = False this looks redundant to me. Line 746: else: I would move this check to the beginning of the function i.e. before/after 'if msdUUID not in domDict' I'd add: if domVer < 3: if self.id or hostId: raise #n.b if using an old domain and passing hostId then it's not clear to me that behaviour is currently correct which is why I wrote 'or hostId' elif not hostId: raise Line 750: safeLease) removal of stopMonitoringDomains means that at the end of reconstruct we're connected to the pool? (I actually think this is the proper behaviour, but if this is the case, I'm not sure whether everything that needs to be done to connect is actually done here). Specifically I'm missing: # Make sure SDCache doesn't have stale data (it can be in case of FC) sdCache.refresh() # Rebuild whole Pool self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion) self.__createMailboxMonitor() Line 764: elif temporaryLock == False: iiuc it is impossible to reach here with temporaryLock == None so this should just be 'else' -- To view, visit http://gerrit.ovirt.org/5068 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If665af4ed8be9b5b53dffc7e1fc258b818bf7f98 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
