7:33 a.m.
Yair Zaslavsky has uploaded a new change for review.
Change subject: Change storageServer to parse some params as strings
......................................................................
Change storageServer to parse some params as strings
engine is sending the connection information as a map of
string keys and string values. VDSM tries to parse version,retrans and
timeout as numbers, but due to the nature of the passed map
it receives a string containing numerical values for these arguments.
Since these arguments are later on added to a comma delimited list
I suggest to parse them as strings.
Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Signed-off-by: Yair Zaslavsky <yzaslavs(a)redhat.com>
---
M vdsm/storage/storageServer.py
1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/62/5062/1
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
4:21 a.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Igor Lvovsky has submitted this change and it was merged.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Change storageServer to handle numeric connection values
engine is sending the connection information as a map of
string keys and string values. VDSM tries to parse version,retrans and
timeout as numbers, but due to the nature of the passed map
it receives a string containing numerical values for these arguments, and
fails.
This patch performs cast to int of the numercial values, and in case of
failure, throws an exception indicating which value raised the exception
Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Signed-off-by: Yair Zaslavsky <yzaslavs(a)redhat.com>
---
M vdsm/storage/storageServer.py
1 file changed, 10 insertions(+), 3 deletions(-)
Approvals:
Ayal Baron: Looks good to me, approved
Dan Kenigsberg: Looks good to me, but someone else must approve
Igor Lvovsky:
Yair Zaslavsky: Verified
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
3:33 a.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Ayal Baron has posted comments on this change.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Patch Set 5: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
2:09 p.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Dan Kenigsberg has posted comments on this change.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Patch Set 5: Looks good to me, but someone else must approve
(1 inline comment)
....................................................
File vdsm/storage/storageServer.py
Line 100: def _addIntegerOption(options, key, value):
I would've called it _appendIntegerOption, but that's fine.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
1:46 p.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Yair Zaslavsky has posted comments on this change.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Patch Set 5: Verified
Verified , similar scenario as before.
Patchset 5 changes:
(Following Dan's comments) -
1. addIntrgerOption is now a local function _addIntegerOption (does not accept
"self" as parameter).
2. Less usage of the prefix option inside the signature and body of _addIntegerOption
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
12:28 p.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Yair Zaslavsky has posted comments on this change.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Patch Set 4: (2 inline comments)
....................................................
File vdsm/storage/storageServer.py
Line 242: self.addIntegerOption(options, "timeo", timeout)
1. It was important for Ayal to do this validation, and I can understand why.
2. Actually, I tried making it what you call a "local function" (didn't see
any reason to use "self" - I totally agree here) but in vein, probably my
mistake as a Python newbie.
3. I agree about the "_" - my bad.
Line 249: def addIntegerOption(self, options, optionKey, optionValue):
Agreed.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
11:39 a.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Dan Kenigsberg has posted comments on this change.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(2 inline comments)
....................................................
File vdsm/storage/storageServer.py
Line 242: self.addIntegerOption(options, "timeo", timeout)
frankly, I've preferred patchset1...
If Ayal insists on validating that values are int, then fine.
HOWEVER, addIntegerOption must not be in the public API of storageServer. you can call it
_appendIntegerOption.
Consider making it a local function, as it has nothing to do with self.
Line 249: def addIntegerOption(self, options, optionKey, optionValue):
"option" is repeated too often here. I'd call the args (key, value) for
shortness.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
10 a.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Yair Zaslavsky has posted comments on this change.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Patch Set 4:
Patchset 4 :
Verified.
Changes:
(Following Dan's comments) -
a. Added some spaces
b. Replaced usage of join.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
8:49 a.m.
New subject: Change in vdsm[master]: Change storageServer to handle numeric connection values
Yair Zaslavsky has posted comments on this change.
Change subject: Change storageServer to handle numeric connection values
......................................................................
Patch Set 2: Verified
Verified both positive and negative flows -
Positive flow - added nfs with version 3, retrans=7 timeo=700, used mount on host to see
it worked well.
Negative flow - changed on purpose engine code to pass wrong value at timeo (a String) and
saw that a proper exception is thrown from VDSM and handled at engine.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
3:20 a.m.
Ayal Baron has posted comments on this change.
Change subject: Change storageServer to parse some params as strings
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
....................................................
File vdsm/storage/storageServer.py
Line 241: options.append("timeo=%s" % timeout)
the alternative is to do:
int(timeout)
which would also verify that the parameter passed is ok.
Even better, can just change the xmlrpc bindings to convert it to int
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
2:31 p.m.
Yair Zaslavsky has posted comments on this change.
Change subject: Change storageServer to parse some params as strings
......................................................................
Patch Set 1:
Dan - I totally agree with you here.
Engine should support using a map (using Java terms here) of Map<String,Object> and
not Map<String,String> - however, this would currently require more changes at
engine side than this patch.
As we would like to fix the nfs options feature ASAP, I suggested this fix.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
1:47 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Change storageServer to parse some params as strings
......................................................................
Patch Set 1: Looks good to me, but someone else must approve
yeah, I'm aware that Engine has long-standing issues with passing non-strings. Vdsm
may be more lenient.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
7:41 a.m.
Yair Zaslavsky has posted comments on this change.
Change subject: Change storageServer to parse some params as strings
......................................................................
Patch Set 1: Verified
Verified.
Used engine in order to add NFS storage domain, passed it version 3, retrans 7 and timeo
700.
Used mount in order to see the new mount on the host.
The mount showed the correct information.
--
To view, visit http://gerrit.ovirt.org/5062
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d1716a6f66a108d60f39fad14d92625a692d0e3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <yzaslavs(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzaslavs(a)redhat.com>
4362
days inactive
4364
days old
vdsm-patches@lists.fedorahosted.org
12 comments
4 participants
participants (4)
-
abaron＠redhat.com -
Dan Kenigsberg -
Igor Lvovsky -
yzaslavs＠redhat.com