3:01 a.m.
Royce Lv has uploaded a new change for review.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
fix masterVersion 0 err when connecting storage pool
when masterVersion is 0 getPoolParams will read saved connection data from
(self._poolFile),but self._poolFile is not exist at this moment, it will be created later
by pool.connect.So masterVersion 0 when connecting triggers error here
Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
---
M vdsm/storage/hsm.py
1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/79/2679/1
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
7:34 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 1: I would prefer that you didn't submit this
Is this another version of http://gerrit.ovirt.org/2645 ?
If so, could you abandon one of the two, and have a better commit message?
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
5:15 p.m.
Adam Litke has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2: Looks good to me, but someone else must approve
Looks good to me. Hopefully we can get an expert to verify that this doesn't have any
unintended consequences.
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
1:21 a.m.
Mark Wu has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2:
I think we need clarify that the parameter definition of API connectStoragePool. It seems
that we intend to use some special value to indicate that these parameters can be
retrieved from pool metadata and the client needn't pass them. If my understanding is
correct, those parameters should be given a default value in the API definition.
Do we want to use None as the special value? I found in vdsClient.py, the default value
for masterVersion is set to -1 if no value given from command line. Can anyone explain
what it expects by setting masterVersion to -1?
I am also confused about the following code snippet:
<snip>
pool = sp.StoragePool(spUUID, self.taskMng)
if not hostID or not scsiKey or not msdUUID or not masterVersion:
hostID, scsiKey, msdUUID, masterVersion = pool.getPoolParams()
res = pool.connect(hostID, scsiKey, msdUUID, masterVersion)
if res:
self.pools[spUUID] = pool
return res
</snip>
getPoolParams() is called before pool connection, the same problem described in commit
message will happen. So it should be a bug too. Please correct me if I am wrong.
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
10:01 a.m.
Eduardo has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2: I would prefer that you didn't submit this
Royce Lv why is this patch required?
Do you have a script driving this condition to occur?
masterVersion should be >= 1 on any master domain.
When connecting to a pool, the manager knows and sends the masterVersion.
-1 is only used in vdsClient and should be fixed.
There are parts of the code assuming that masterVersion = 0 is for regular, e.g. no
master, domains.
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
10:04 p.m.
Royce Lv has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2:
We check masterver 0 is because:
1.we have plan to support single host rest-api and have vdsm rest-api on the way,it means
bypass vdsClient to call API.py directly(actually this is found when testing in such
case)
2.as constants.py mentioned,
DOMAIN_VERSIONS = (0, 2, 3)
SUPPORTED_DOMAIN_VERSIONS = DOMAIN_VERSIONS
the default domain version -1 makes people confused because we don't support -1
explicitly,but support master version 0.
3.vdsclient is just a shell but api.py is in sdk layer, in order to isolate errors,
different layers should not depend on(trust) other layer's input as
default,instead,they should check the input and do the right thing.
Does it make sense?
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
2:39 a.m.
Mark Wu has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2:
Hi Eduardo,
Currently, "masterVersion >= 1" is guaranteed by ovirt-engine, not vdsm,
right? So I think we still need add input validation for it in vdsm to avoid misuse by
other management tools.
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
5:17 a.m.
Sheldon Feng has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/storage/hsm.py
Line 830: if msdUUID is None or masterVersion is None:
"not" include "None" case.
"if not msdUUID" means msdUUID is None or zero.
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Sheldon Feng <shaohef(a)linux.vnet.ibm.com>
1:36 p.m.
Xu He Jie has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2: (1 inline comment)
....................................................
File vdsm/storage/hsm.py
Line 853: if hostID is None or scsiKey is None or msdUUID is None or
masterVersion is None:
I think if masterVersion is None, getPoolparams still can not works. so the error message
will confuse people. Maybe add specific check and error message for None value is better.
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Sheldon Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
8:26 p.m.
Royce Lv has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 2:
Sheldon,He Jie, "not" include "None" an "0", vdsm can block
"None" but don't block "0",so I use "None",if
"masterver" is "None",it should raise an error, but "masterver =
0" is supported in constants.py
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Sheldon Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
5:12 a.m.
Xu He Jie has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Sheldon Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
8:35 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 3: Do not submit
(4 inline comments)
None of these can be None as they are recieved from XML RPC which doesn't support
passing None (at least not in the way we use it).
I don't understand the problem this is trying to solve
....................................................
File vdsm_cli/vdsClient.py
Line 815: master_ver = None
Are you sure you can send None over xmlrpc?
....................................................
File vdsm/storage/hsm.py
Line 825: if masterVersion is not None:#None just for connected pool,masterVersion
>= 0 for not connected one
Comments in the same line as the code are frowned upon.
Also try and keep width below 80 chars
Line 835: if msdUUID is None or masterVersion is None:
msdUUID can be an empty string
Line 859: if not scsiKey:
Scsi key is ignored, no need to make sure it was entered
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Sheldon Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
12:02 a.m.
Royce Lv has posted comments on this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 3:
Hi Saggi,
1.this change is to fix masterver = 0,not to fix masterver = None,when we use masterver =
0 to connect storage pool,it fails,you can try using xmlrpc directly(not via vdsclient) to
re-play this usecase.
2.again,the xmlrpc maybe not the only entrance to call vdsm in the future, we heard you
have plan to support vdsm rest-api.how can we guarantee it does not break this?
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Sheldon Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
1 a.m.
Royce Lv has abandoned this change.
Change subject: fix masterVersion 0 err when connecting storage pool
......................................................................
Patch Set 3: Abandoned
--
To view, visit http://gerrit.ovirt.org/2679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I7228bd340a4281400e16c26b311d9945dd5ce597
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Royce Lv <lvroyce(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Sheldon Feng <shaohef(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Xu He Jie <xuhj(a)linux.vnet.ibm.com>
4408
days inactive
4436
days old
vdsm-patches@lists.fedorahosted.org
13 comments
8 participants
participants (8)
-
agl＠us.ibm.com -
Dan Kenigsberg -
ewarszaw＠redhat.com -
lvroyce＠linux.vnet.ibm.com -
shaohef＠linux.vnet.ibm.com -
smizrahi＠redhat.com -
wudxw＠linux.vnet.ibm.com -
Xu He Jie