9:35 a.m.
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 <fsimonce(a)redhat.com>
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 <fsimonce(a)redhat.com>
5:59 p.m.
Itamar Heim has abandoned this change.
Change subject: Add the hostId parameter to reconstructMaster
......................................................................
Abandoned
no reply - abandoning - please restore if still relevant
--
To view, visit http://gerrit.ovirt.org/5068
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: If665af4ed8be9b5b53dffc7e1fc258b818bf7f98
Gerrit-PatchSet: 14
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: Itamar Heim <iheim(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
3:30 p.m.
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 <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>
12:31 a.m.
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 <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>
4:18 a.m.
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?
--
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 <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: Itamar Heim <iheim(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No
9:14 a.m.
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 <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>
7:10 a.m.
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 <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>
3:21 a.m.
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>
3:09 a.m.
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 <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>
1:53 p.m.
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 <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>
2:55 a.m.
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 <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>
4:04 p.m.
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 <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>
12:50 p.m.
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 <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>
10:33 a.m.
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 <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>
9:02 a.m.
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 <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>
7:53 a.m.
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 <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>
2:51 a.m.
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 <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>
3637
days inactive
4342
days old
vdsm-patches@lists.fedorahosted.org
16 comments
6 participants
participants (6)
-
abaron＠redhat.com -
Allon Mureinik -
Dan Kenigsberg -
Federico Simoncelli -
iheim＠redhat.com -
smizrahi＠redhat.com