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 <fsimonce(a)redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>