Change in vdsm[master]: Add the hostId parameter to reconstructMaster

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Add the hostId parameter to reconstructMaster

Signed-off-by: Federico Simoncelli 
Change-Id: If665af4ed8be9b5b53dffc7e1fc258b818bf7f98
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
4 files changed, 37 insertions(+), 39 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/68/5068/1
--
To view, visit http://gerrit.ovirt.org/5068
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If665af4ed8be9b5b53dffc7e1fc258b818bf7f98
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 11: Verified; Looks good to me, approved

--
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 12: Verified; Looks good to me, approved

--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 12: I would prefer that you didn't submit this

(2 inline comments)

Federico, I know these patches sit for so long, but I think they deserve a real review from *someone*.

....................................................
Commit Message
Line 7: Add the hostId parameter to reconstructMaster
what is the motivation for this patch?

....................................................
File vdsm/storage/hsm.py
Line 1448:                           ioOpTimeoutSec=None, leaseRetries=None, hostId=None,
wouldn't this change draw the universe to its end?
i.e., is the "options" arg has never ever been used by anyone?

--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 12: (1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 1448:                           ioOpTimeoutSec=None, leaseRetries=None, hostId=None,
Yes the "options" arg was never used. Now when we have a 3.0 engine managing newer vdsm versions both hostId and options will be None (old behavior). On the other hand if the engine is 3.1 and the vdsm version is old, options will contain the hostId but it will be discarded.

--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 12: Looks good to me, but someone else must approve

--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 13: Verified; Looks good to me, approved

--
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 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 13: Looks good to me, but someone else must approve

--
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 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 13: I would prefer that you didn't submit this

--
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 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 13:

Not sure why my comments weren't published before.
Trying again

--
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 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

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 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 13: (6 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 1489:             pool = sp.StoragePool(spUUID, self.taskMng)
Done

....................................................
File vdsm/storage/sp.py
Line 732:         temporaryLock = None
see below

Line 738:             if not self.id and not hostId:
The use of a real hostId is disjoint from the new flow where you don't disconnect. So I definitely agree on checking the version to make sure we reconstruct while connected only with v3, but I don't see any particular advantage on forbidding a real hostid for version < 3.

Line 745:                 temporaryLock = False
see below

Line 750:                               safeLease)
The missing indentation in the last part of your comment it made hard to understand.

self.refresh(...) # here below, line 751

Takes care of:

sdCache.refresh(...)
 self.__rebuild(...)

The only missing thing is (hopefully):

self.__createMailboxMonitor()

Line 764:             elif temporaryLock == False:
Currently it can: it's a final clause where we could end up if _acquireTemporaryClusterLock or acquireHostId+acquireClusterLock are failing. In such case we shouldn't be releasing anything here. I'll improve this moving the _acquireTemporaryClusterLock and acquireHostId+acquireClusterLock out of the try section.

--
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 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 13: (3 inline comments)

....................................................
File vdsm/storage/sp.py
Line 746:             else:
You did not comment on this, do you agree? nack?

Line 750:                               safeLease)
it should be solved in a way that if connect is modified this is automatically modified as well (i.e. call _connect or something)

Line 764:             elif temporaryLock == False:
it would be easiest if it were "with clusteredLock" where you'd set clusteredLock to the appropriate object before.
That way you don't need 'temporaryLock' at all.

--
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 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 14: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 819:         if not msdUUID or not masterVersion:
these params should be retrieved under lock and only in case we're not already connected.

--
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

[Allon Mureinik]

Allon Mureinik has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 14: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 434:             self.hsmMailer = None
wouldn't you prefer doing this refactor in a sepatare patch?
this patchset isn't too small as it is

--
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Saggi Mizrahi

Itamar Heim has posted comments on this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Patch Set 14:

still relevant with reconstruct master hopefully going away?

Itamar Heim has abandoned this change.

Change subject: Add the hostId parameter to reconstructMaster
......................................................................

Abandoned

no reply - abandoning - please restore if still relevant