Change in vdsm[master]: vdsm: log proper tag for messages comming from vdsm-logrotate
Tomas Golembiovsky has posted comments on this change. Change subject: vdsm: log proper tag for messages comming from vdsm-logrotate .. Patch Set 1: CI failure unrelated: stderr: fatal: Unable to look up gerrit.ovirt.org (port 9418) (Name or service not known) -- To view, visit https://gerrit.ovirt.org/65604 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I75e1eae3e75928034edaa24bbcd6fae44920c22d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: vdsm: report correct exit value in vdsm-logrotate
Tomas Golembiovsky has posted comments on this change. Change subject: vdsm: report correct exit value in vdsm-logrotate .. Patch Set 1: CI failure unrelated: stderr: fatal: Unable to look up gerrit.ovirt.org (port 9418) (Name or service not known) -- To view, visit https://gerrit.ovirt.org/65603 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I29df6cac36c64be2bf20337ab096ec471d864ca5 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: vdsm: document why we prefer running logrotate manually
Tomas Golembiovsky has posted comments on this change. Change subject: vdsm: document why we prefer running logrotate manually .. Patch Set 4: CI failure unrelated: stderr: fatal: Unable to look up gerrit.ovirt.org (port 9418) (Name or service not known) -- To view, visit https://gerrit.ovirt.org/63682 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Fabian Deutsch <fabi...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: vdsm: log proper tag for messages comming from vdsm-logrotate
Tomas Golembiovsky has uploaded a new change for review.
Change subject: vdsm: log proper tag for messages comming from vdsm-logrotate
..
vdsm: log proper tag for messages comming from vdsm-logrotate
The log messages comming from vdsm-logrotate were improperly tagged as
'logrotate'. To avoid confusion with real logrotate we should properly
mark our messages as vdsm-logrotate.
Change-Id: I75e1eae3e75928034edaa24bbcd6fae44920c22d
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M vdsm/vdsm-logrotate
1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/04/65604/1
diff --git a/vdsm/vdsm-logrotate b/vdsm/vdsm-logrotate
index e44e079..0c84ca3 100755
--- a/vdsm/vdsm-logrotate
+++ b/vdsm/vdsm-logrotate
@@ -12,14 +12,14 @@
EXITVALUE=$?
if [ $EXITVALUE != 0 ]; then
-/usr/bin/logger -t logrotate "ALERT exited abnormally with [$EXITVALUE]"
+/usr/bin/logger -t vdsm-logrotate "ALERT logrotate exited abnormally with
[$EXITVALUE]"
fi
if [ -d /var/log/core ] ; then
/usr/bin/find /var/log/core -type f -name '*xz' -mtime +7 -exec /bin/rm -f
'{}' \;
RET=$?
if [ $RET != 0 ]; then
-/usr/bin/logger -t logrotate "ALERT clean old core files exited
abnormally with [$RET]"
+/usr/bin/logger -t vdsm-logrotate "ALERT clean old core files exited
abnormally with [$RET]"
EXITVALUE=$RET
fi
fi
@@ -28,7 +28,7 @@
/usr/bin/find /var/log/vdsm/import -type f -mtime +30 -exec /bin/rm -f
'{}' \;
RET=$?
if [ $RET != 0 ]; then
-/usr/bin/logger -t logrotate "ALERT clean of old import log files
exited abnormally with [$RET]"
+/usr/bin/logger -t vdsm-logrotate "ALERT clean of old import log files
exited abnormally with [$RET]"
EXITVALUE=$RET
fi
fi
--
To view, visit https://gerrit.ovirt.org/65604
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I75e1eae3e75928034edaa24bbcd6fae44920c22d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: vdsm: report correct exit value in vdsm-logrotate
Tomas Golembiovsky has uploaded a new change for review.
Change subject: vdsm: report correct exit value in vdsm-logrotate
..
vdsm: report correct exit value in vdsm-logrotate
The exit code was reported improperly. If all but the last command had
failed we would still return 0. Now we are reporting exit code of last
failed command or 0 when all exited successfully.
Change-Id: I29df6cac36c64be2bf20337ab096ec471d864ca5
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M vdsm/vdsm-logrotate
1 file changed, 8 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/03/65603/1
diff --git a/vdsm/vdsm-logrotate b/vdsm/vdsm-logrotate
index a29cbe5..e44e079 100755
--- a/vdsm/vdsm-logrotate
+++ b/vdsm/vdsm-logrotate
@@ -17,17 +17,19 @@
if [ -d /var/log/core ] ; then
/usr/bin/find /var/log/core -type f -name '*xz' -mtime +7 -exec /bin/rm -f
'{}' \;
-EXITVALUE=$?
-if [ $EXITVALUE != 0 ]; then
-/usr/bin/logger -t logrotate "ALERT clean old core files exited
abnormally with [$EXITVALUE]"
+RET=$?
+if [ $RET != 0 ]; then
+/usr/bin/logger -t logrotate "ALERT clean old core files exited
abnormally with [$RET]"
+EXITVALUE=$RET
fi
fi
if [ -d /var/log/vdsm/import ] ; then
/usr/bin/find /var/log/vdsm/import -type f -mtime +30 -exec /bin/rm -f
'{}' \;
-EXITVALUE=$?
-if [ $EXITVALUE != 0 ]; then
-/usr/bin/logger -t logrotate "ALERT clean of old import log files
exited abnormally with [$EXITVALUE]"
+RET=$?
+if [ $RET != 0 ]; then
+/usr/bin/logger -t logrotate "ALERT clean of old import log files
exited abnormally with [$RET]"
+EXITVALUE=$RET
fi
fi
--
To view, visit https://gerrit.ovirt.org/65603
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I29df6cac36c64be2bf20337ab096ec471d864ca5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: v2v: support for block devices
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: support for block devices .. Patch Set 4: (1 comment) I investigated this a little and we can get the device size with libvirt call virDomainGetBlockInfo(). Another thing is that the kvm2ovirt tool needs to be updated too. It also assumes that the disk is volume in storage pool, which a block device is not. Another downloading logic has to be implemented around virDomainBlockPeek(). https://gerrit.ovirt.org/#/c/64272/4//COMMIT_MSG Commit Message: PS4, Line 9: VMWare Is that really possible in VMware? -- To view, visit https://gerrit.ovirt.org/64272 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f7a85715764efded7b296e858b130a05fe10f2a Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: v2v: support for block devices
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: support for block devices
..
Patch Set 4: Code-Review-1
(1 comment)
As pointed out by a user on ML the change breaks engine. VDSM fails to fetch
disk size and logs an exception. Although the the disk size field is marked as
optional in the scheme engine does not consider the size opotional aparently.
Maybe we should provide an alternative logic of fetching disk size for block
devices. Adding -1 for the moment.
https://gerrit.ovirt.org/#/c/64272/2/tests/v2vTests.py
File tests/v2vTests.py:
PS2, Line 130: {dis
> yes,
Are you sure? Based on what I see in docs it's more like file attribute is for
file and dev attribute is for block.
--
To view, visit https://gerrit.ovirt.org/64272
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f7a85715764efded7b296e858b130a05fe10f2a
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: v2v: support for block devices
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: support for block devices .. Patch Set 1: Code-Review+1 Adding +1, but it leaves me thinking whether we should do something for the other types as well. Based on documentation there are three other disk types: dir, netowork and volume. -- To view, visit https://gerrit.ovirt.org/64272 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f7a85715764efded7b296e858b130a05fe10f2a Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: alignmentScan: Use proper environment
Tomas Golembiovsky has posted comments on this change. Change subject: alignmentScan: Use proper environment .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/64123 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I522ea5454a77b06e833723e0995ccbdeab9e5e5e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: alignmentScan: Use proper environment
Tomas Golembiovsky has posted comments on this change. Change subject: alignmentScan: Use proper environment .. Patch Set 1: (1 comment) Just a typo in commit message. Other than that LGTM. https://gerrit.ovirt.org/#/c/64123/1//COMMIT_MSG Commit Message: PS1, Line 10: leaned Typo? -- To view, visit https://gerrit.ovirt.org/64123 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I522ea5454a77b06e833723e0995ccbdeab9e5e5e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: vdsm: Rely on system for logrotation
Tomas Golembiovsky has posted comments on this change. Change subject: vdsm: Rely on system for logrotation .. Patch Set 3: Code-Review-1 Thanks Dan. Somehow I have missed this change when browsing through related history. If we really can generate more than 15 MB of logs per day and we want to run logrotate hourly (as opposed to daily by the system) then it makes sense to stick with what we do now. In this case we can abandon this change. -- To view, visit https://gerrit.ovirt.org/63682 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Fabian Deutsch <fabi...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: v2v: add test for Xen block device
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: add test for Xen block device .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/63022 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1afc7d21857e59ebf69856e99e15cc3796149e4e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: v2v: Support for ova exported from AWS
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: Support for ova exported from AWS
..
Patch Set 3: Code-Review+1
(1 comment)
https://gerrit.ovirt.org/#/c/63517/3/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
Line 1187: vmName = node.find('./ovf:VirtualSystem/ovf:Name', ns)
Line 1188: if vmName is not None:
Line 1189: vm['vmName'] = vmName.text
Line 1190: else:
Line 1191: vm['vmName'] =
os.path.splitext(os.path.basename(ova_path))[0]
> nice idea. This could not raise, right?
IMO it's safe.
Line 1192:
Line 1193: memSize = node.find('.//ovf:Item[rasd:ResourceType="%d"]/'
Line 1194: 'rasd:VirtualQuantity' %
_OVF_RESOURCE_MEMORY, ns)
Line 1195: if memSize is not None:
--
To view, visit https://gerrit.ovirt.org/63517
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6215491cb724c2131d440b00e767a5690070a7b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: vdsm: Rely on system for logrotation
Tomas Golembiovsky has posted comments on this change.
Change subject: vdsm: Rely on system for logrotation
..
Patch Set 1:
(4 comments)
https://gerrit.ovirt.org/#/c/63682/1//COMMIT_MSG
Commit Message:
Line 10: is no real reason to invoke logrotate manualy.
Line 11:
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into
the
Line 14: the directory with logrotate configuration.
> This approach is problematic.
Ok, let's take one step at a time then. I will restore the denpendency on
logrotate.
Line 15:
Line 16: The patch also renames script vdsm-logrotate to vdsm-remove-logs
because
Line 17: it doesn't run logrotate anymore. It only removes old log files.
Line 18:
https://gerrit.ovirt.org/#/c/63682/1/static/Makefile.am
File static/Makefile.am:
Line 36:./etc/vdsm/mom.d/04-cputune.policy \
Line 37:./etc/vdsm/mom.d/05-iotune.policy \
Line 38:$(NULL)
Line 39:
Line 40: vdsmconfrotatedir = $(sysconfdir)/logrotate.d
> Yes
Ah, now I understand what you're aiming at. Will do.
Line 41:
Line 42: vdsmconfrotate_DATA = \
Line 43:./etc/logrotate.d/vdsm \
Line 44:$(NULL)
https://gerrit.ovirt.org/#/c/63682/1/vdsm/vdsm-remove-logs
File vdsm/vdsm-remove-logs:
Line 10: if [ -d /var/log/core ] ; then
Line 11: /usr/bin/find /var/log/core -type f -name '*xz' -mtime +7 -exec
/bin/rm -f '{}' \;
Line 12: EXITVALUE=$?
Line 13: if [ $EXITVALUE != 0 ]; then
Line 14: /usr/bin/logger -t logrotate "ALERT clean old core files
exited abnormally with [$EXITVALUE]"
> If we depend on logrotate running from cron, why do we need to run it here?
We don't. This only logs a message to syslog. Maybe we should change the tag
(-t) from 'logrotate' to 'vdsm'. To avoid cofusion and make it clear where the
log message comes from.
Line 15: fi
Line 16: fi
Line 17:
Line 18: if [ -d /var/log/vdsm/import ] ; then
Line 18: if [ -d /var/log/vdsm/import ] ; then
Line 19: /usr/bin/find /var/log/vdsm/import -type f -mtime +30 -exec
/bin/rm -f '{}' \;
Line 20: EXITVALUE=$?
Line 21: if [ $EXITVALUE != 0 ]; then
Line 22: /usr/bin/logger -t logrotate "ALERT clean of old import log
files exited abnormally with [$EXITVALUE]"
> Same
Same as above.
Line 23: fi
Line 24: fi
Line 25:
--
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabi...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: vdsm: Rely on system for logrotation
Tomas Golembiovsky has posted comments on this change.
Change subject: vdsm: Rely on system for logrotation
..
Patch Set 1:
(4 comments)
https://gerrit.ovirt.org/#/c/63682/1//COMMIT_MSG
Commit Message:
Line 5: CommitDate: 2016-09-11 13:33:20 +0200
Line 6:
Line 7: vdsm: Rely on system for logrotation
Line 8:
Line 9: When logrotate is installed it is normaly run periodicaly by cron. There
> I am missing value of this change here. Why do we want to do it?
The value is simplification at least on the two fronts:
1) We are duplicating code that is already present in the system. If logrotate
is already installed it knows how to make sure it is run properly.
2) We are enforcing dependency on logrotate. The dependency is not necessary.
See next comment.
Line 10: is no real reason to invoke logrotate manualy.
Line 11:
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into
the
Line 10: is no real reason to invoke logrotate manualy.
Line 11:
Line 12: Also if we rely on system to run logrotate we can remove the dependency
Line 13: on it from RPMs. It is enough to install the configuration file into
the
Line 14: the directory with logrotate configuration.
> Based on spec change in line 134 it was installed with vdsm.
It's not our problem. Whether logrotate is installed or not should be
administrator's decision and not ours. We are present with a .d directory where
to put our configuration. If and how the configuraiton is used shouldn't be our
concern (arguably in d/s, but definitely not in u/s).
NB: IIRC logrotate is installed as part of base system on RHEL and CentOS. So
if the package is not installed there is probably a reason for that.
Line 15:
Line 16: The patch also renames script vdsm-logrotate to vdsm-remove-logs
because
Line 17: it doesn't run logrotate anymore. It only removes old log files.
Line 18:
https://gerrit.ovirt.org/#/c/63682/1/static/Makefile.am
File static/Makefile.am:
Line 36:./etc/vdsm/mom.d/04-cputune.policy \
Line 37:./etc/vdsm/mom.d/05-iotune.policy \
Line 38:$(NULL)
Line 39:
Line 40: vdsmconfrotatedir = $(sysconfdir)/logrotate.d
> This looks more like syslogrotatedir - nothing in this path is related to v
You mean to define new variable like:
syslogrotatedir=${sysconfdir}/logrotate.d
and use the variable in Makefile? (sysconfdir comes from configure)
Line 41:
Line 42: vdsmconfrotate_DATA = \
Line 43:./etc/logrotate.d/vdsm \
Line 44:$(NULL)
https://gerrit.ovirt.org/#/c/63682/1/vdsm/vdsm-remove-logs
File vdsm/vdsm-remove-logs:
Line 20: EXITVALUE=$?
Line 21: if [ $EXITVALUE != 0 ]; then
Line 22: /usr/bin/logger -t logrotate "ALERT clean of old import log
files exited abnormally with [$EXITVALUE]"
Line 23: fi
Line 24: fi
> Can we use logrotate for this?
That would be nice. But unfortunately I have no idea how to do that.
Line 25:
--
To view, visit https://gerrit.ovirt.org/63682
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: build: Don't use fixed path to systemd directory
Tomas Golembiovsky has posted comments on this change.
Change subject: build: Don't use fixed path to systemd directory
..
Patch Set 1:
Verified. The files are properly installed into ${prefix} when make install is
used. The files are in /usr/lib/systemd/system/ in the RPM.
--
To view, visit https://gerrit.ovirt.org/63687
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6306e2e449d3762a7b9dcfac58d09970fac83574
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Artyom Lukianov <aluki...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: build: Don't use fixed path to systemd directory
Tomas Golembiovsky has posted comments on this change. Change subject: build: Don't use fixed path to systemd directory .. Patch Set 1: Verified+1 -- To view, visit https://gerrit.ovirt.org/63687 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6306e2e449d3762a7b9dcfac58d09970fac83574 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Artyom Lukianov <aluki...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: build: Don't use fixed path to systemd directory
Tomas Golembiovsky has uploaded a new change for review.
Change subject: build: Don't use fixed path to systemd directory
..
build: Don't use fixed path to systemd directory
Since commit 9d6f46d9 the project can no longer be built and installed
by non-root user. There is probably nothing wrong with the patch itself.
It's rather the fact that we use fixed path to the systemd directory
which prevents us from installing into some other prefix.
This patch changes the value of SYSTEMD_UNIT_DIR to use ${prefix}
which is normaly set to "/usr" during packaging.
Change-Id: I6306e2e449d3762a7b9dcfac58d09970fac83574
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M configure.ac
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/87/63687/1
diff --git a/configure.ac b/configure.ac
index 5756499..2b546d2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -236,7 +236,7 @@
AC_SUBST([CDROMGROUP], [cdrom])
# Systemd units default path
-AC_SUBST([SYSTEMD_UNIT_DIR], ['/usr/lib/systemd/system'])
+AC_SUBST([SYSTEMD_UNIT_DIR], ['${prefix}/lib/systemd/system'])
# VDSM default paths
AC_SUBST([vdsmdir], ['${datarootdir}/vdsm'])
--
To view, visit https://gerrit.ovirt.org/63687
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6306e2e449d3762a7b9dcfac58d09970fac83574
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: vdsm: Rely on system for logrotation
Tomas Golembiovsky has uploaded a new change for review.
Change subject: vdsm: Rely on system for logrotation
..
vdsm: Rely on system for logrotation
When logrotate is installed it is normaly run periodicaly by cron. There
is no real reason to invoke logrotate manualy.
Also if we rely on system to run logrotate we can remove the dependency
on it from RPMs. It is enough to install the configuration file into the
the directory with logrotate configuration.
The patch also renames script vdsm-logrotate to vdsm-remove-logs because
it doesn't run logrotate anymore. It only removes old log files.
Change-Id: Ica888244bd7c65121f55983e5716a6eae5662879
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M static/Makefile.am
R static/etc/logrotate.d/vdsm
M vdsm.spec.in
M vdsm/Makefile.am
R vdsm/vdsm-remove-logs
5 files changed, 12 insertions(+), 21 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/82/63682/1
diff --git a/static/Makefile.am b/static/Makefile.am
index 040d6ec..22c8736 100644
--- a/static/Makefile.am
+++ b/static/Makefile.am
@@ -37,10 +37,10 @@
./etc/vdsm/mom.d/05-iotune.policy \
$(NULL)
-vdsmconfrotatedir = $(vdsmconfdir)/logrotate
+vdsmconfrotatedir = $(sysconfdir)/logrotate.d
vdsmconfrotate_DATA = \
- ./etc/vdsm/logrotate/vdsm \
+ ./etc/logrotate.d/vdsm \
$(NULL)
sudoersdir = $(sysconfdir)/sudoers.d
diff --git a/static/etc/vdsm/logrotate/vdsm b/static/etc/logrotate.d/vdsm
similarity index 100%
rename from static/etc/vdsm/logrotate/vdsm
rename to static/etc/logrotate.d/vdsm
diff --git a/vdsm.spec.in b/vdsm.spec.in
index 26c36d3..dafeb9c 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -131,7 +131,6 @@
Requires: ethtool
Requires: which
Requires: sudo >= 1.7.3
-Requires: logrotate
Requires: xz
Requires: ntp
Requires: iproute >= 3.10.0
@@ -948,7 +947,7 @@
%config(noreplace) %{_sysconfdir}/%{vdsm_name}/svdsm.logger.conf
%config(noreplace) %{_sysconfdir}/%{vdsm_name}/mom.conf
%config(noreplace) %{_sysconfdir}/%{vdsm_name}/mom.d/*.policy
-%config(noreplace) %{_sysconfdir}/%{vdsm_name}/logrotate/vdsm
+%config(noreplace) %{_sysconfdir}/logrotate.d/vdsm
%config(noreplace) %{_sysconfdir}/rwtab.d/vdsm
%config(noreplace) %{_sysconfdir}/sysctl.d/vdsm.conf
%config(noreplace) %{_sysconfdir}/modules-load.d/vdsm.conf
@@ -956,7 +955,7 @@
%{_sysconfdir}/dhcp/dhclient.d/sourceRoute.sh
%{_sysconfdir}/modprobe.d/vdsm-bonding-modprobe.conf
%{_sysconfdir}/sudoers.d/50_vdsm
-%{_sysconfdir}/cron.hourly/vdsm-logrotate
+%{_sysconfdir}/cron.hourly/vdsm-remove-logs
%{_sysconfdir}/libvirt/hooks/qemu
%{_libexecdir}/%{vdsm_name}/curl-img-wrap
%{_libexecdir}/%{vdsm_name}/fc-scan
diff --git a/vdsm/Makefile.am b/vdsm/Makefile.am
index 8b535b6..0b5e807 100644
--- a/vdsm/Makefile.am
+++ b/vdsm/Makefile.am
@@ -76,7 +76,7 @@
vdsm-gencerts.sh.in \
vdsm-libvirt-access.pkla \
vdsm-libvirt-access.rules \
- vdsm-logrotate \
+ vdsm-remove-logs \
vdsm-store-net-config.in \
vdsmd.8.in \
$(NULL)
@@ -94,7 +94,7 @@
install-data-bonding-defaults \
install-data-dhclient-hooks \
install-data-libvirtpass \
- install-data-logrotate \
+ install-data-logs \
install-data-vdsmconf
$(MKDIR_P) $(DESTDIR)$(vdsmrepo)
$(MKDIR_P) $(DESTDIR)$(vdsmrepo)/hsm-tasks
@@ -118,7 +118,7 @@
uninstall-data-bonding-defaults \
uninstall-data-dhclient-hooks \
uninstall-data-libvirtpass \
- uninstall-data-logrotate \
+ uninstall-data-logs \
uninstall-data-vdsmconf
install-data-libvirtpass:
@@ -139,14 +139,13 @@
$(RM) $(DESTDIR)$(vdsmdir)/bonding-defaults.json
$(RM) $(DESTDIR)$(vdsmdir)/bonding-name2numeric.json
-install-data-logrotate:
+install-data-logs:
$(MKDIR_P) $(DESTDIR)$(sysconfdir)/cron.hourly
- $(INSTALL_SCRIPT) $(srcdir)/vdsm-logrotate \
- $(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-logrotate
- $(MKDIR_P) $(DESTDIR)$(sysconfdir)/cron.d
+ $(INSTALL_SCRIPT) $(srcdir)/vdsm-remove-logs \
+ $(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-remove-logs
-uninstall-data-logrotate:
- $(RM) $(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-logrotate
+uninstall-data-logs:
+ $(RM) $(DESTDIR)$(sysconfdir)/cron.hourly/vdsm-remove-logs
install-data-vdsmconf:
$(MKDIR_P) $(DESTDIR)$(vdsmconfdir)/vdsm.conf.d
diff --git a/vdsm/vdsm-logrotate b/vdsm/vdsm-remove-logs
similarity index 77%
rename from vdsm/vdsm-logrotate
rename to vdsm/vdsm-remove-logs
index 11050b9..7760b6b 100755
--- a/vdsm/vdsm-logrotate
+++ b/vdsm/vdsm-remove-logs
@@ -1,12 +1,5 @@
#!/bin/sh
-/usr/sbin/logrotate /etc/vdsm/logrotate/vdsm
-
-EXITVALUE=$?
-if [ $EXITVALUE != 0 ]; then
-/usr/bin/logger -t logr
Change in vdsm[master]: v2v: Support for ova exported from AWS
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Support for ova exported from AWS .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/63517 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6215491cb724c2131d440b00e767a5690070a7b Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Support for ova exported from AWS
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Support for ova exported from AWS .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/63517/1//COMMIT_MSG Commit Message: Line 8: Line 9: When exporting VM from AWS as VMWare OVA file it doesn't export Line 10: tag for the VM. Line 11: The Name is not Mandatory for the ovf format - using default as a name Line 12: AWS the name tag is not mandatory - using default as a name. This line seems superfluous or the wording of the sentence is confusing. Line 13: Line 14: The Name is not relevant from the engine point of view since the user Line 15: can change it while importing the OVA. Line 16: -- To view, visit https://gerrit.ovirt.org/63517 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6215491cb724c2131d440b00e767a5690070a7b Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tests: split vmfakecon out of vmfakelib
Tomas Golembiovsky has posted comments on this change. Change subject: tests: split vmfakecon out of vmfakelib .. Patch Set 1: Actually, what about removing vmSecretTests.py from blacklisted tests? Or will that go into some other patch? -- To view, visit https://gerrit.ovirt.org/63197 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id393cf07c0de8ea06dceec3d79e5d618f79fa6cc Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tests: split vmfakecon out of vmfakelib
Tomas Golembiovsky has posted comments on this change. Change subject: tests: split vmfakecon out of vmfakelib .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/63197 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id393cf07c0de8ea06dceec3d79e5d618f79fa6cc Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tests: install fake-virt-v2v.err to fix tests
Tomas Golembiovsky has posted comments on this change. Change subject: tests: install fake-virt-v2v.err to fix tests .. Patch Set 3: Verified+1 The missing file is present in tarball and rpm, the V2V tests run OK. -- To view, visit https://gerrit.ovirt.org/63273 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc749a5580de712b2eea954afa78e69722d13619 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: tests: install fake-virt-v2v.err to fix tests
Tomas Golembiovsky has posted comments on this change. Change subject: tests: install fake-virt-v2v.err to fix tests .. Patch Set 3: (1 comment) CI failure seems unrelated. Fails on tests: ERROR: test_local_auto_with_dynamic_address_from_ra (network.netinfo_test.TestIPv6Addresses) FAIL: test_ip_info (network.netinfo_test.TestNetinfo) https://gerrit.ovirt.org/#/c/63273/2//COMMIT_MSG Commit Message: PS2, Line 11: unusabl > ... unusable. Done -- To view, visit https://gerrit.ovirt.org/63273 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc749a5580de712b2eea954afa78e69722d13619 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: py3: taskset.py/tasksetTests.py compliance.
Tomas Golembiovsky has posted comments on this change. Change subject: py3: taskset.py/tasksetTests.py compliance. .. Patch Set 11: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/63223 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If56e64350a7937e667a520b7dc53c01184e13810 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Leon Goldberg <leon.ot...@gmail.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Leon Goldberg <leon.ot...@gmail.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: py3: taskset.py/tasketTests compliance.
Tomas Golembiovsky has posted comments on this change. Change subject: py3: taskset.py/tasketTests compliance. .. Patch Set 10: Code-Review-1 (3 comments) I'm sorry to nitpick. Generaly it's OK, just adding -1 because of small typos in commit message. https://gerrit.ovirt.org/#/c/63223/10//COMMIT_MSG Commit Message: PS10, Line 7: tasketTests It should read 'tasksetTests.py'. I.e. add missing 's' and the '.py' extension (since you have it for taskset.py). PS10, Line 9: et Typo, missing 's' PS10, Line 17: et Typo, missing 's' -- To view, visit https://gerrit.ovirt.org/63223 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If56e64350a7937e667a520b7dc53c01184e13810 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Leon Goldberg <leon.ot...@gmail.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Leon Goldberg <leon.ot...@gmail.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: add test for Xen block device
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: add test for Xen block device .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/63022 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1afc7d21857e59ebf69856e99e15cc3796149e4e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[ovirt-4.0]: v2v: filter out Xen VMs with block storage
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: filter out Xen VMs with block storage .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/63133 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84512f54f9949640a568a444dd7eeb78db852134 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-4.0 Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Running virt-v2v with some sane environment
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Running virt-v2v with some sane environment .. Patch Set 5: Ignore the rebases, please. It's the result of unsuccessful attempt to cherry-pick the change onto another branch. I have reverted it to state at PS3. -- To view, visit https://gerrit.ovirt.org/62765 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae6455c3189b7fbbf0d9d3ad5369a06b3cd6017e Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: filter out Xen VMs with block storage
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: filter out Xen VMs with block storage .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/62368 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84512f54f9949640a568a444dd7eeb78db852134 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Running virt-v2v with some sane environment
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Running virt-v2v with some sane environment .. Patch Set 3: (1 comment) https://gerrit.ovirt.org/#/c/62765/3/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS3, Line 467: env['LIBGUESTFS_BACKEND'] = 'direct' > wasn't it always mandatory? Yes. Nothing has changed about it. -- To view, visit https://gerrit.ovirt.org/62765 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae6455c3189b7fbbf0d9d3ad5369a06b3cd6017e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Running virt-v2v with some sane environment
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Running virt-v2v with some sane environment .. Patch Set 3: Verified+1 (1 comment) Verified. Now virt-v2v completes the import successfully. https://gerrit.ovirt.org/#/c/62765/2/lib/vdsm/v2v.py File lib/vdsm/v2v.py: Line 466: # virt-v2v specific variables Line 467: env['LIBGUESTFS_BACKEND'] = 'direct' Line 468: if 'virtio_iso_path' in self._vminfo: Line 469: env['VIRTIO_WIN'] = self._vminfo['virtio_iso_path'] Line 470: return env > spurious extra line. Nice, but unneeded. Removed. Line 471: Line 472: @contextmanager Line 473: def _password_file(self): Line 474: fd = os.open(self._passwd_file, os.O_WRONLY | os.O_CREAT, 0o600) -- To view, visit https://gerrit.ovirt.org/62765 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae6455c3189b7fbbf0d9d3ad5369a06b3cd6017e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Running virt-v2v with some sane environment
Tomas Golembiovsky has uploaded a new change for review.
Change subject: v2v: Running virt-v2v with some sane environment
..
v2v: Running virt-v2v with some sane environment
Starting virt-v2v with (almost) empty environment no longer works and
virt-v2v expect at least PATH to be set properly. Otherwise it will fail
to find external tools.
Still, even if virt-v2v was OK with it, having empty environment is not
a good idea either. Since it depends on couple external tools they may
also require some basic environment to work properly.
Change-Id: Iae6455c3189b7fbbf0d9d3ad5369a06b3cd6017e
Bug-Url: https://bugzilla.redhat.com/1367839
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M lib/vdsm/v2v.py
1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/65/62765/1
diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py
index c6a9943..8b11cf3 100644
--- a/lib/vdsm/v2v.py
+++ b/lib/vdsm/v2v.py
@@ -460,9 +460,14 @@
return path.rsplit(os.sep, 3)[0]
def _environment(self):
-env = {'LIBGUESTFS_BACKEND': 'direct'}
+# Provide some sane environment
+env = os.environ.copy()
+
+# virt-v2v specific variables
+env['LIBGUESTFS_BACKEND'] = 'direct'
if 'virtio_iso_path' in self._vminfo:
env['VIRTIO_WIN'] = self._vminfo['virtio_iso_path']
+
return env
@contextmanager
--
To view, visit https://gerrit.ovirt.org/62765
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iae6455c3189b7fbbf0d9d3ad5369a06b3cd6017e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: filter out Xen VMs with block storage
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: filter out Xen VMs with block storage .. Patch Set 1: Code-Review+1 (1 comment) https://gerrit.ovirt.org/#/c/62368/1//COMMIT_MSG Commit Message: Line 6: Line 7: v2v: filter out Xen VMs with block storage Line 8: Line 9: virt-v2v is not support importing Xen VMs with block storage domain. Line 10: No need to return these VMs via get_external_vms verb > There is no filter in the engine for snapshots... (there is an RFE but solu Ok. Line 11: Line 12: Change-Id: I84512f54f9949640a568a444dd7eeb78db852134 Line 13: Bug-Url: https://bugzilla.redhat.com/1365411 -- To view, visit https://gerrit.ovirt.org/62368 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84512f54f9949640a568a444dd7eeb78db852134 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add PipelineProc, pipeline wrapper object
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Add PipelineProc, pipeline wrapper object .. Patch Set 4: Verified+1 (2 comments) No significant changes, restoring V+1. https://gerrit.ovirt.org/#/c/62094/3/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS3, Line 657: > please make try block as small as possible; place this logging just above i Done https://gerrit.ovirt.org/#/c/62094/4/lib/vdsm/v2v.py File lib/vdsm/v2v.py: Line 700: while monotonic_time() < deadline: Line 701: p.poll() Line 702: if p.returncode is not None: Line 703: break Line 704: time.sleep(1) Small fix pointed out by Vinzenz. I haved moved the sleep after the poll(). It's not a big deal, but it can save some unnecessary sleeps in certain situations. Line 705: else: Line 706: p.wait() Line 707: Line 708: if deadline is not None: -- To view, visit https://gerrit.ovirt.org/62094 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0c3741ae7ef9731a2cd9d587e86766b9e6e64f62 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: filter out Xen VMs with block storage
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: filter out Xen VMs with block storage .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/62368/1//COMMIT_MSG Commit Message: Line 6: Line 7: v2v: filter out Xen VMs with block storage Line 8: Line 9: virt-v2v is not support importing Xen VMs with block storage domain. Line 10: No need to return these VMs via get_external_vms verb Shouldn't we rather behave like with snapsnots and running VMs and leave the filtering to the engine? Line 11: Line 12: Change-Id: I84512f54f9949640a568a444dd7eeb78db852134 Line 13: Bug-Url: https://bugzilla.redhat.com/1365411 -- To view, visit https://gerrit.ovirt.org/62368 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84512f54f9949640a568a444dd7eeb78db852134 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add PipelineProc, pipeline wrapper object
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Add PipelineProc, pipeline wrapper object .. Patch Set 3: Verified+1 Verified the topic as whole. Tried successful and unsuccessful import. In both cases return value is properly propagated to engine and virt-v2v log is created as expected. -- To view, visit https://gerrit.ovirt.org/62094 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0c3741ae7ef9731a2cd9d587e86766b9e6e64f62 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 16: Verified+1 Verified the topic as whole. Tried successful and unsuccessful import. In both cases return value is properly propagated to engine and virt-v2v log is created as expected. -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add helper that redirects stdin/out/err
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Add helper that redirects stdin/out/err .. Patch Set 3: Verified the topic as whole. Tried successful and unsuccessful import. In both cases return value is properly propagated to engine and virt-v2v log is created as expected. -- To view, visit https://gerrit.ovirt.org/62092 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c748155e233ebfb2c0c223308693ccaab0ebc18 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add helper that redirects stdin/out/err
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Add helper that redirects stdin/out/err .. Patch Set 3: Verified+1 -- To view, visit https://gerrit.ovirt.org/62092 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c748155e233ebfb2c0c223308693ccaab0ebc18 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 15: (1 comment) https://gerrit.ovirt.org/#/c/59834/14/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS14, Line 816: FileIO > Thanks, this fully makes sense, but now I wonder: 1) That's because AsyncProc object does something similar to get rid of the simple File object. But instead of io.FileIO it has it's own implementation for accessing the FD (AsyncProc._streamedWrapper). It then wraps the AsyncProc._streamedWrapper object in io.BuffereReader just like we do here (both _streamedWrapper and io.FileIO are low-level and don't provide iterators). 2) We can give it a try later. -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add PipelineProc, pipeline wrapper object
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Add PipelineProc, pipeline wrapper object .. Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/62094/2/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS2, Line 643: _proc > _procs (please note the trailing 's') is a bit clearer Done PS2, Line 648: nlike regular kill() we do not raise : OSError if the processess have already terminated. > please add another sentence to explain why we do so. Reprhased. I am not sure if this is the best way to mimic the regular kill(). We could raise OSError either when there is at least one process that has terminated, or (which might be better) raise OSError when all processes have terminated. However, we don't need any of this at the moment. PS2, Line 660: @property : def pid(self): : return [p.pid for p in self._proc] > this is supposed to mimic `pid` as returned by process wrappers returned by Good point. Changed to pids. -- To view, visit https://gerrit.ovirt.org/62094 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0c3741ae7ef9731a2cd9d587e86766b9e6e64f62 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add helper that redirects stdin/out/err
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Add helper that redirects stdin/out/err .. Patch Set 2: (1 comment) https://gerrit.ovirt.org/#/c/62092/2//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2016-08-08 14:23:25 +0200 Line 4: Commit: Tomáš Golembiovský <tgole...@redhat.com> Line 5: CommitDate: 2016-08-09 16:21:43 +0200 Line 6: Line 7: v2v: Add execCmd() that accepts stdin/out/err > so let's bend the rules ;) Done Line 8: Line 9: There is no way to pass stdin, stdout and stderr streams to Popen via Line 10: execCmd. This will be useful in subsequent changes introducing logging Line 11: of virt-v2v output. -- To view, visit https://gerrit.ovirt.org/62092 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c748155e233ebfb2c0c223308693ccaab0ebc18 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 14: (1 comment) https://gerrit.ovirt.org/#/c/59834/14/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS14, Line 816: FileIO > OK, which kind of unexpected behaviour? I'm just worried this could hide ot Data loss to be precise. I also forgot to mention that it is prevented by Python. When trying to mix the read methods I get the exception: ValueError: Mixing iteration and read methods would lose data -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add execCmd() that accepts stdin/out/err
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Add execCmd() that accepts stdin/out/err .. Patch Set 2: (2 comments) https://gerrit.ovirt.org/#/c/62092/2//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2016-08-08 14:23:25 +0200 Line 4: Commit: Tomáš Golembiovský <tgole...@redhat.com> Line 5: CommitDate: 2016-08-09 16:21:43 +0200 Line 6: Line 7: v2v: Add execCmd() that accepts stdin/out/err > what about: v2v: add simple_exec_cmd that... Tried that. It's too long. :) If we're not too strict about the 50 character limit on the short description I can change that. Line 8: Line 9: There is no way to pass stdin, stdout and stderr streams to Popen via Line 10: execCmd. This will be useful in subsequent changes introducing logging Line 11: of virt-v2v output. Line 7: v2v: Add execCmd() that accepts stdin/out/err Line 8: Line 9: There is no way to pass stdin, stdout and stderr streams to Popen via Line 10: execCmd. This will be useful in subsequent changes introducing logging Line 11: of virt-v2v output. > In the end we are duplicating execCmd. I don't have a better alternative, s Not really. IMO the only "feature" of execCmd we duplicate is the call to cmdutils.wrap_command(). Line 12: Line 13: Change-Id: I6c748155e233ebfb2c0c223308693ccaab0ebc18 Line 14: Bug-Url: https://bugzilla.redhat.com/1350465 -- To view, visit https://gerrit.ovirt.org/62092 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c748155e233ebfb2c0c223308693ccaab0ebc18 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Bump version requirement on python-cpopen.
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Bump version requirement on python-cpopen. .. Patch Set 2: Verified+1 -- To view, visit https://gerrit.ovirt.org/62093 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I540180130a80f5c3c19007e10851b7713db596f9 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: compat: py3: capture output of CPopen.communicate
Tomas Golembiovsky has posted comments on this change.
Change subject: compat: py3: capture output of CPopen.communicate
..
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/61393/3/lib/vdsm/commands.py
File lib/vdsm/commands.py:
Line 67:
Line 68: execCmdLogger.debug(cmdutils.command_log_line(printable, cwd=cwd))
Line 69:
Line 70: extra = {}
Line 71: extra['stderr'] = subprocess.PIPE
You're missing import subprocess.
Line 72: extra['stdout'] = subprocess.PIPE
Line 73: if deathSignal is not None:
Line 74: extra['deathSignal'] = deathSignal
Line 75: p = CPopen(command, close_fds=True, cwd=cwd, env=env, **extra)
--
To view, visit https://gerrit.ovirt.org/61393
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac9c1c1bd3ca1902103fac1f8675b7d42ca78a33
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 13: (4 comments) Mostly splitting parts into separate commits. Added code for removing old log files https://gerrit.ovirt.org/#/c/59834/13/lib/vdsm/v2v.py File lib/vdsm/v2v.py: Line 648: ret.append(vol['path']) Line 649: return ret Line 650: Line 651: Line 652: class PipelineProc(object): > good project standards would require this to be in a separate change, with Separated into https://gerrit.ovirt.org/#/c/62094 Line 653: Line 654: def __init__(self, proc1, proc2): Line 655: self._proc = (proc1, proc2) Line 656: self._stdout = proc2.stdout Line 802: self._id, self._proc.pid) Line 803: Line 804: def _watch_process_output(self): Line 805: out = io.BufferedReader(io.FileIO(self._proc.stdout.fileno(), Line 806: mode='r', closefd=False), BUFFSIZE) > I didn't see this before. Seems OK, but could you please elaborate why we n This is needed to avoid rewriting OutputParser. Aparently using both iteration and read() on a simple File object can lead to undefined behaviour. Line 807: parser = OutputParser() Line 808: for event in parser.parse(out): Line 809: if isinstance(event, ImportProgress): Line 810: self._status = STATUS.COPYING_DISK Line 1217: net['type'] = 'interface' Line 1218: vm['networks'].append(net) Line 1219: Line 1220: Line 1221: def _simple_exec_cmd(command, env=None, nice=None, ioclass=None, > good project standards would require this to be in a separate change, with Separated into https://gerrit.ovirt.org/#/c/62092 Line 1222: stdin=subprocess.PIPE, stdout=subprocess.PIPE, Line 1223: stderr=subprocess.PIPE): Line 1224: Line 1225: command = cmdutils.wrap_command(command, with_ioclass=ioclass, https://gerrit.ovirt.org/#/c/59834/13/vdsm.spec.in File vdsm.spec.in: Line 81: BuildRequires: openssl Line 82: BuildRequires: policycoreutils-python Line 83: BuildRequires: psmisc Line 84: BuildRequires: PyYAML Line 85: BuildRequires: python-cpopen >= 1.4-1 > good project standards would require this to be in a separate trivial chang Separated into https://gerrit.ovirt.org/#/c/62093 Line 86: BuildRequires: python-inotify Line 87: BuildRequires: python-ioprocess >= 0.16.1-1 Line 88: BuildRequires: python-pthreading Line 89: BuildRequires: qemu-img -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Bump version requirement on python-cpopen.
Tomas Golembiovsky has uploaded a new change for review. Change subject: v2v: Bump version requirement on python-cpopen. .. v2v: Bump version requirement on python-cpopen. In order to support redirection of stderr to stdout we need to bump version requirement on python-cpopen. This feature is broken in previous versions. Change-Id: I540180130a80f5c3c19007e10851b7713db596f9 Bug-Url: https://bugzilla.redhat.com/1350465 Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com> --- M tests/v2vTests.py M vdsm.spec.in 2 files changed, 8 insertions(+), 1 deletion(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/62093/1 diff --git a/tests/v2vTests.py b/tests/v2vTests.py index 3978035..6d3a8f4 100644 --- a/tests/v2vTests.py +++ b/tests/v2vTests.py @@ -571,6 +571,13 @@ out = p.stdout.read() self.assertEqual(out, msg) +p = v2v._simple_exec_cmd(['/bin/sh', '-c', 'echo -en "%s" >&2' % msg], + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) +p.wait() +out = p.stdout.read() +self.assertEqual(out, msg) + class MockVirConnectTests(TestCaseBase): def setUp(self): diff --git a/vdsm.spec.in b/vdsm.spec.in index cc79f14..bf53001 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -82,7 +82,7 @@ BuildRequires: policycoreutils-python BuildRequires: psmisc BuildRequires: PyYAML -BuildRequires: python-cpopen >= 1.4 +BuildRequires: python-cpopen >= 1.4-1 BuildRequires: python-inotify BuildRequires: python-ioprocess >= 0.16.1-1 BuildRequires: python-pthreading -- To view, visit https://gerrit.ovirt.org/62093 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I540180130a80f5c3c19007e10851b7713db596f9 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add execCmd() that accepts stdin/out/err
Tomas Golembiovsky has uploaded a new change for review. Change subject: v2v: Add execCmd() that accepts stdin/out/err .. v2v: Add execCmd() that accepts stdin/out/err There is no way to pass stdin, stdout and stderr streams to Popen via execCmd. This will be useful in subsequent changes introducing logging of virt-v2v output. Change-Id: I6c748155e233ebfb2c0c223308693ccaab0ebc18 Bug-Url: https://bugzilla.redhat.com/1350465 Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com> --- M lib/vdsm/v2v.py M tests/v2vTests.py 2 files changed, 29 insertions(+), 1 deletion(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/92/62092/1 diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py index c6a9943..860768e 100644 --- a/lib/vdsm/v2v.py +++ b/lib/vdsm/v2v.py @@ -42,9 +42,10 @@ from vdsm.commands import execCmd from vdsm.common import zombiereaper +from vdsm.compat import CPopen from vdsm.constants import P_VDSM_RUN, EXT_KVM_2_OVIRT from vdsm.define import errCode, doneCode -from vdsm import libvirtconnection, response, concurrent +from vdsm import cmdutils, concurrent, libvirtconnection, response from vdsm.utils import traceback, CommandPath, NICENESS, IOCLASS try: @@ -1139,3 +1140,18 @@ else: net['type'] = 'interface' vm['networks'].append(net) + + +def _simple_exec_cmd(command, env=None, nice=None, ioclass=None, + stdin=None, stdout=None, stderr=None): + +command = cmdutils.wrap_command(command, with_ioclass=ioclass, +ioclassdata=None, with_nice=nice, +with_setsid=False, with_sudo=False, +reset_cpu_affinity=True) + +logging.debug(cmdutils.command_log_line(command, cwd=None)) + +p = CPopen(command, close_fds=True, cwd=None, env=env, + stdin=stdin, stdout=stdout, stderr=stderr) +return p diff --git a/tests/v2vTests.py b/tests/v2vTests.py index b7bfc54..3978035 100644 --- a/tests/v2vTests.py +++ b/tests/v2vTests.py @@ -20,6 +20,7 @@ from collections import namedtuple from contextlib import contextmanager from StringIO import StringIO +import subprocess import tarfile import zipfile import uuid @@ -559,6 +560,17 @@ self.assertEqual(job.status, v2v.STATUS.DONE) +def testSimpleExecCmd(self): +p = v2v._simple_exec_cmd(['cat'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE) +msg = "test\ntest" +p.stdin.write(msg) +p.stdin.close() +p.wait() +out = p.stdout.read() +self.assertEqual(out, msg) + class MockVirConnectTests(TestCaseBase): def setUp(self): -- To view, visit https://gerrit.ovirt.org/62092 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6c748155e233ebfb2c0c223308693ccaab0ebc18 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Add PipelineProc, pipeline wrapper object
Tomas Golembiovsky has uploaded a new change for review.
Change subject: v2v: Add PipelineProc, pipeline wrapper object
..
v2v: Add PipelineProc, pipeline wrapper object
We plan to pipe the output of virt-v2v to tee to store the log file. In
order to manage the pipelined processes new class is introduced.
Change-Id: I0c3741ae7ef9731a2cd9d587e86766b9e6e64f62
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M lib/vdsm/v2v.py
M tests/v2vTests.py
2 files changed, 132 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/94/62094/1
diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py
index 860768e..e149ddc 100644
--- a/lib/vdsm/v2v.py
+++ b/lib/vdsm/v2v.py
@@ -34,6 +34,7 @@
import re
import signal
import tarfile
+import time
import threading
import xml.etree.ElementTree as ET
import zipfile
@@ -46,7 +47,8 @@
from vdsm.constants import P_VDSM_RUN, EXT_KVM_2_OVIRT
from vdsm.define import errCode, doneCode
from vdsm import cmdutils, concurrent, libvirtconnection, response
-from vdsm.utils import traceback, CommandPath, NICENESS, IOCLASS
+from vdsm.utils import monotonic_time, traceback, CommandPath, \
+NICENESS, IOCLASS
try:
import ovirt_imageio_common
@@ -635,6 +637,78 @@
return ret
+class PipelineProc(object):
+
+def __init__(self, proc1, proc2):
+self._proc = (proc1, proc2)
+self._stdout = proc2.stdout
+
+def kill(self):
+"""
+Kill all processes in a pipeline. Unlike regular kill() we do not raise
+OSError if the processess have already terminated.
+"""
+for p in self._proc:
+try:
+logging.debug("Killing pid=%d", p.pid)
+p.kill()
+except OSError as e:
+# Probably the process has already terminated
+if e.errno != errno.ESRCH:
+raise e
+
+@property
+def pid(self):
+return [p.pid for p in self._proc]
+
+@property
+def returncode(self):
+"""
+Returns None if any of the processes is still running. Returns 0 if all
+processes have finished with a zero exit code, otherwise return first
+nonzero exit code.
+"""
+ret = 0
+for p in self._proc:
+p.poll()
+if p.returncode is None:
+return None
+if p.returncode != 0 and ret == 0:
+# One of the processes has failed
+ret = p.returncode
+
+# All processes have finished
+return ret
+
+@property
+def stdout(self):
+return self._stdout
+
+def wait(self, timeout=None):
+if timeout is not None:
+deadline = monotonic_time() + timeout
+else:
+deadline = None
+
+for p in self._proc:
+if deadline is not None:
+# NOTE: CPopen doesn't support timeout argument.
+while monotonic_time() < deadline:
+time.sleep(1)
+p.poll()
+if p.returncode is not None:
+break
+else:
+p.wait()
+
+if deadline is not None:
+if deadline < monotonic_time() or self.returncode is None:
+# Timed out
+return False
+
+return True
+
+
class ImportVm(object):
TERM_DELAY = 30
PROC_WAIT_TIMEOUT = 30
diff --git a/tests/v2vTests.py b/tests/v2vTests.py
index 6d3a8f4..a0ae3b7 100644
--- a/tests/v2vTests.py
+++ b/tests/v2vTests.py
@@ -579,6 +579,63 @@
self.assertEqual(out, msg)
+@expandPermutations
+class PipelineProcTests(TestCaseBase):
+
+PROC_WAIT_TIMEOUT = 30
+
+def testRun(self):
+msg = 'foo\nbar'
+p1 = v2v._simple_exec_cmd(['echo', '-n', msg],
+ stdout=subprocess.PIPE)
+p2 = v2v._simple_exec_cmd(['cat'],
+ stdin=p1.stdout,
+ stdout=subprocess.PIPE)
+
+p = v2v.PipelineProc(p1, p2)
+self.assertEqual(p.pid, [p1.pid, p2.pid])
+
+ret = p.wait(self.PROC_WAIT_TIMEOUT)
+self.assertEqual(ret, True)
+
+out = p.stdout.read()
+self.assertEqual(out, msg)
+
+@permutations([
+# (cmd1, cmd2, returncode)
+['false', 'true', 1],
+['true', 'false', 1],
+['true', 'true', 0],
+])
+def testReturncode(self, cmd1, cmd2, returncode):
+p1 = v2v._simple_exec_cmd([cmd1],
+ stdout=subprocess.PIPE)
+p2 = v2v._simple_exec_cmd([cmd2],
+ stdin=p1.stdout,
+ stdout=subprocess.PIPE)
+p = v2v.PipelineProc(p1, p2)
+p.wait(self.PROC_WAIT
Change in vdsm[master]: compat: py3: capture output of CPopen.communicate
Tomas Golembiovsky has posted comments on this change. Change subject: compat: py3: capture output of CPopen.communicate .. Patch Set 2: (2 comments) https://gerrit.ovirt.org/#/c/61393/2//COMMIT_MSG Commit Message: PS2, Line 12: stdin you mean stdout/stderr https://gerrit.ovirt.org/#/c/61393/2/lib/vdsm/compat.py File lib/vdsm/compat.py: Line 51: if 'stderr' not in kwargs: Line 52: kwargs['stderr'] = subprocess.PIPE Line 53: if 'stdout' not in kwargs: Line 54: kwargs['stdout'] = subprocess.PIPE Line 55: subprocess.Popen.__init__(self, args, **kwargs) > please set explicitly stderr and stdout to PIPE in execCmd - that way only I aggree. This should be fixed in execCmd. And the few cases that use CPopen directly should be reviewed and fixed if necessary. Otherwise we'll just end up reimplementing old CPopen behaviour over subprocess.Popen. Line 56: Line 57: try: Line 58: from contextlib import suppress Line 59: suppress # make pyflakes happy -- To view, visit https://gerrit.ovirt.org/61393 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iac9c1c1bd3ca1902103fac1f8675b7d42ca78a33 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 10: (2 comments) https://gerrit.ovirt.org/#/c/59834/10/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS10, Line 658: def __len__(self): : return len(self._proc) > why do we need this? We don't. At least not any more. I forgot to remove it. Done. https://gerrit.ovirt.org/#/c/59834/10/tests/fake-virt-v2v File tests/fake-virt-v2v: PS10, Line 90: # NOTE: Most verbose messages go to stderr, but some go to stdout. This can : # potentialy mess with out parsing routine. > let's make sure we imitate this on our fake added some traces to stderr too -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 10: CI-1 is unrelated: 11:30:57 ./automation/check-patch.sh: line 7: easy_install: command not found -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: command: Store stdout and stderr to a log file
Tomas Golembiovsky has abandoned this change. Change subject: command: Store stdout and stderr to a log file .. Abandoned Touching execCmd/AsyncProc is frowned upon. -- To view, visit https://gerrit.ovirt.org/59833 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I5a8b9afaa196729c8ab98f380a39833a9a0840cd Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 8: CI failure is unrelated: 13:10:58 == Running the shellscript automation/check-patch.sh 13:10:58 + easy_install pip 13:10:58 ./automation/check-patch.sh: line 7: easy_install: command not found -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: Log detailed output of virt-v2v
..
Patch Set 8:
(1 comment)
https://gerrit.ovirt.org/#/c/59834/6/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
Line 722: self._wait_for_process()
Line 723:
Line 724: if self._proc.returncode != 0:
Line 725: raise V2VProcessError('Job %r process failed
exit-code: %r' %
Line 726: (self._id,
This doesn't realy work now that traces are included in the output and served
only for logging on engine side. Now that we have full logs on vdsm side it's
not necessary anyway.
Line 727:self._proc.returncode))
Line 728:
Line 729: if self._status != STATUS.ABORTED:
Line 730: self._status = STATUS.DONE
--
To view, visit https://gerrit.ovirt.org/59834
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 6: (3 comments) https://gerrit.ovirt.org/#/c/59834/4//COMMIT_MSG Commit Message: Line 13: It would be cleaner to modify execCmd() to store the log output, but Line 14: this can dangerous without the function being refactored first. Line 15: Therefore we resort to a dirty hack with shell to redirect stderr to Line 16: stdout and store everything in a log file. This hack should be cleaned Line 17: once execCmd() is updated. > 1. agreed. Do we have a BZ about this? just for tracking purposes (we could I don't know about any BZ. I'll try to find out or create a new one, if there is none yet. Yes, you get it right. I don't know how we could possibly merge the two together after the process finishes. And yes, when the command being executed is written to stdout and the error it produced goes to stderr we lose the connection. We can only guess the the error in stderr belongs to the last command we see in stdout. Here is the stdout: https://paste.fedoraproject.org/389824/82285371/ And here is part of the error stderr: https://paste.fedoraproject.org/389835/68229240/ Line 18: Line 19: Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Line 20: Bug-Url: https://bugzilla.redhat.com/1350465 https://gerrit.ovirt.org/#/c/59834/6/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS6, Line 399: # XXX: This is a rather hacky way to redirect stderr to stdout. It : # should be rewritten once execCmd has some clean way to do that. > ybronhei suggested to use watchCmd. Perhaps it could help. Did you tried it I'm still not sure how is it supposed to help me. I can run async proces directly with execCmd and I don't need a stop condition. Is there anythin I'm missing? PS6, Line 855: ln = self._stream.__iter__().next() > why not Oh, right! I will fix that in next PS. -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 6: Verified+1 Rebase, no code changes. Restoring V+1 -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/59834/4//COMMIT_MSG Commit Message: Line 13: It would be cleaner to modify execCmd() to store the log output, but Line 14: this can dangerous without the function being refactored first. Line 15: Therefore we resort to a dirty hack with shell to redirect stderr to Line 16: stdout and store everything in a log file. This hack should be cleaned Line 17: once execCmd() is updated. > I prefer this to be solved in the v2v tool or a helpr script that merges st As I see it, there are two or three problems that sholdn't be mixed together! 1) virt-v2v is messy and should be fixed -- this is something we can't enforce. even if we get the patch in, it will take some time before the fixed version is available to everyone (u/s and d/s). 2) VDSM has no way to run a command (assynchronously) and merge stdout to stderr. This is the main issue here. It is a general feature that is not related to virt-v2v. It is something I belive should be fixed elsewhere. (And it would make sense to use such feature here even when virt-v2v was clean about traces.) 3) The actual logging. This is something that could in theory go to v2v.py, provided we have 2) working. But I would still prefer it to be somewhere else, because it's again a general feature (maybe as a wrapper around execCmd). Line 18: Line 19: Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Line 20: Bug-Url: https://bugzilla.redhat.com/1350465 -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 4: Verified-1 (1 comment) The basic command works as it should, but the error code is not propagated correctly. Adding V-1 for now. https://gerrit.ovirt.org/#/c/59834/4/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS4, Line 401: cmd = ['sh', '-c', :'exec 2>&1 "$@" | tee "%s"' % logFile, # Command :'exec'] # Passed as argv[0] : cmd.extend(self._command()) > I'm a bit confused, could you please show how the final command line will l The final command looks like: 'sh' '-c' 'exec 2>&1 "$@" | tee "/var/log/vdsm/import/virt-v2v-.log"' 'exec' '/usr/bin/virt-v2v' '-v' '-x' '-ic' '' ... After execCmd kicks in and adds the wrappers, it will be: '/usr/bin/taskset' '--cpu-list' '0-1' '/usr/bin/nice' '-n' '19' '/usr/bin/ionice' '-c' '3' 'sh' '-c' 'exec 2>&1 "$@" | tee "/var/log/vdsm/import/virt-v2v-.log"' 'exec' '/usr/bin/virt-v2v' '-v' '-x' '-ic' '' ... -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: Log detailed output of virt-v2v
..
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/59834/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
Line 811: chunk = ''
Line 812: while True:
Line 813: c = stream.read(1)
Line 814: if not c:
Line 815: raise OutputParserError('copy-disk stream closed
unexpectedly')
> ok, let's add a test about this.
I didn't add any explicit test about it, but I did change fake-virt-v2v. The
existing tests will fail if the input is not handled properly.
Line 816: chunk += c
Line 817: if c == '\r':
Line 818: yield chunk
Line 819: chunk = ''
--
To view, visit https://gerrit.ovirt.org/59834
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Log detailed output of virt-v2v .. Patch Set 3: Main changes: * storing logs in /var/log/vdsm/import instead of /var/run/vdsm/v2v because /var/run is cleaned automaticaly * don't rely on execCmd to store the logs -- To view, visit https://gerrit.ovirt.org/59834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: command: Store stdout and stderr to a log file
Tomas Golembiovsky has posted comments on this change. Change subject: command: Store stdout and stderr to a log file .. Patch Set 1: Code-Review-1 (1 comment) https://gerrit.ovirt.org/#/c/59833/1//COMMIT_MSG Commit Message: PS1, Line 13: This change adds the possibility to store the stdout and stderr to a log : file > It *is* a good idea IMHO, is just that in this very specific case the price Yes, unfortunately useful info ends up in both streams. In that case let's drop this subject and I will go with some dirty solution for virt-v2v and we can do it properly when the appropriate things are refactored. -- To view, visit https://gerrit.ovirt.org/59833 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5a8b9afaa196729c8ab98f380a39833a9a0840cd Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: command: Store stdout and stderr to a log file
Tomas Golembiovsky has posted comments on this change. Change subject: command: Store stdout and stderr to a log file .. Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/59833/1//COMMIT_MSG Commit Message: PS1, Line 13: This change adds the possibility to store the stdout and stderr to a log : file > I'm not strongly against this change (so not -2), but I'd *really*, *really Making it general seemed as a good idea. Especially since all the stream related magic is already present in AsyncProc stream wrappers. If you feel it's dangerous area to touch I can try to reimplement it just for v2v. The little problem with virt-v2v is that it cannot decide whether to trace messages on stderr or stdout. That means I need to watch both streams and merge the output as it comes. I'm not sure how reliably will the polling work with the buffered stream wrapping the AsyncProc streams, but I can try that. Alternatively I can wrap the virt-v2v call by shell and use 'tee' to do the writing. https://gerrit.ovirt.org/#/c/59833/1/lib/vdsm/commands.py File lib/vdsm/commands.py: PS1, Line 56: if errToOutLog and sync: : # Using errToOutLog together with sync makes not much sense. Since we : # do not get messages as they come, all we can do is write all the : # stderr after all the stdout. > why not just derive errToOutLog? Do we need it explicitely? Yeah, that could work too. -- To view, visit https://gerrit.ovirt.org/59833 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5a8b9afaa196729c8ab98f380a39833a9a0840cd Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: Log detailed output of virt-v2v
..
Patch Set 1:
(3 comments)
https://gerrit.ovirt.org/#/c/59834/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
Line 383: self._vmid = vmid
Line 384: self._irs = irs
Line 385: self._prepared_volumes = []
Line 386: self._passwd_file = os.path.join(_V2V_DIR, "%s.tmp" % vmid)
Line 387: self._base_command = [_VIRT_V2V.cmd, '-v', '-x']
> +1, how hard is to optionally add those?
There is no reason not to. Any performance penalty caused by traces is
negligible since most of the time of the import is spent on converting the
disks.
Do we have a way to run vdsm in debug mode? Or you mean to add a flag to UI?
Either way it should be possible, but is it necessary? Bear in mind that
converting the VM is a lengthy process that can potentialy take hours. In case
of import failure this requires the user to enable debugging and rerun the
import instead of just looking up the log.
Line 388:
Line 389: def execute(self):
Line 390: raise NotImplementedError("Subclass must implement this")
Line 391:
Line 785: yield ImportProgress(int(current_disk),
int(disk_count),
Line 786: description)
Line 787: for chunk in self._iter_progress(stream):
Line 788: progress = self._parse_progress(chunk)
Line 789: if progress is not None:
> Maybe its verbose flag related?
It's not unrelated. Se below.
Line 790: yield DiskProgress(progress)
Line 791: if progress == 100:
Line 792: break
Line 793:
Line 811:
Line 812: def _parse_progress(self, chunk):
Line 813: m = self.DISK_PROGRESS_RE.match(chunk)
Line 814: if m is None:
Line 815: return None
> unrelated?
When verbose output is enabled there are other messages before the proggress.
We don't want to throw an exception on those.
Line 816: try:
Line 817: return int(m.group(1))
Line 818: except ValueError:
Line 819: raise OutputParserError('error parsing progress regex: %r'
--
To view, visit https://gerrit.ovirt.org/59834
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: command: Store stdout and stderr to a log file
Tomas Golembiovsky has posted comments on this change.
Change subject: command: Store stdout and stderr to a log file
..
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/59833/1/tests/commands_test.py
File tests/commands_test.py:
Line 98: errLog=errLog)
Line 99: self.assertEquals(rc, 0)
Line 100: checklog()
Line 101:
Line 102: # Async: wait
Is this the rigt place fot the async tests? There are also test classes
AsyncProcTests (in miscTests.py) and AsyncProcessOperationTests (in
utilsTests.py). But I'm not sure if they are more appropriate for this.
Line 103: p = commands.execCmd(cmd(('bash', '-c', command)),
Line 104: sync=False,
Line 105: outLog=outLog,
Line 106: errLog=errLog)
--
To view, visit https://gerrit.ovirt.org/59833
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5a8b9afaa196729c8ab98f380a39833a9a0840cd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: v2v: Log detailed output of virt-v2v
Tomas Golembiovsky has uploaded a new change for review.
Change subject: v2v: Log detailed output of virt-v2v
..
v2v: Log detailed output of virt-v2v
The detailed log virt-v2v output is often necessary to debug conversion
failures. We provide '-v -x' arguments to virt-v2v to get the detailed
output and store the logs in the VDSM run directory.
Change-Id: I6a8d9284316a551edeaffdd66dfcd299fa02478e
Bug-Url: https://bugzilla.redhat.com/1350465
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M lib/vdsm/v2v.py
M tests/fake-virt-v2v
2 files changed, 63 insertions(+), 27 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/34/59834/1
diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py
index defbe14..0948741 100644
--- a/lib/vdsm/v2v.py
+++ b/lib/vdsm/v2v.py
@@ -384,6 +384,7 @@
self._irs = irs
self._prepared_volumes = []
self._passwd_file = os.path.join(_V2V_DIR, "%s.tmp" % vmid)
+self._base_command = [_VIRT_V2V.cmd, '-v', '-x']
def execute(self):
raise NotImplementedError("Subclass must implement this")
@@ -394,7 +395,10 @@
deathSignal=signal.SIGTERM,
nice=NICENESS.HIGH,
ioclass=IOCLASS.IDLE,
- env=self._environment())
+ env=self._environment(),
+ outLog=os.path.join(_V2V_DIR,
+ "%s.log" % self._vmid),
+ errToOutLog=True)
def _get_disk_format(self):
fmt = self._vminfo.get('format', 'raw').lower()
@@ -490,11 +494,11 @@
self._password = password
def _command(self):
-cmd = [_VIRT_V2V.cmd,
- '-ic', self._uri,
- '-o', 'vdsm',
- '-of', self._get_disk_format(),
- '-oa', self._vminfo.get('allocation', 'sparse').lower()]
+cmd = self._base_command
+cmd.extend(['-ic', self._uri,
+'-o', 'vdsm',
+'-of', self._get_disk_format(),
+'-oa', self._vminfo.get('allocation', 'sparse').lower()])
cmd.extend(self._disk_parameters())
cmd.extend(['--password-file',
self._passwd_file,
@@ -521,19 +525,19 @@
self._ova_path = ova_path
def _command(self):
-cmd = [_VIRT_V2V.cmd,
- '-i', 'ova', self._ova_path,
- '-o', 'vdsm',
- '-of', self._get_disk_format(),
- '-oa', self._vminfo.get('allocation', 'sparse').lower(),
- '--vdsm-vm-uuid',
- self._vmid,
- '--vdsm-ovf-output',
- _V2V_DIR,
- '--machine-readable',
- '-os',
- self._get_storage_domain_path(
- self._prepared_volumes[0]['path'])]
+cmd = self._base_command
+cmd.extend(['-i', 'ova', self._ova_path,
+'-o', 'vdsm',
+'-of', self._get_disk_format(),
+'-oa', self._vminfo.get('allocation', 'sparse').lower(),
+'--vdsm-vm-uuid',
+self._vmid,
+'--vdsm-ovf-output',
+_V2V_DIR,
+'--machine-readable',
+'-os',
+self._get_storage_domain_path(
+self._prepared_volumes[0]['path'])])
cmd.extend(self._disk_parameters())
return cmd
@@ -559,11 +563,11 @@
self._ssh_agent = SSHAgent()
def _command(self):
-cmd = [_VIRT_V2V.cmd,
- '-ic', self._uri,
- '-o', 'vdsm',
- '-of', self._get_disk_format(),
- '-oa', self._vminfo.get('allocation', 'sparse').lower()]
+cmd = self._base_command
+cmd.extend(['-ic', self._uri,
+'-o', 'vdsm',
+'-of', self._get_disk_format(),
+'-oa', self._vminfo.get('allocation', 'sparse').lower()])
cmd.extend(self._disk_parameters())
cmd.extend(['--vdsm-vm-uuid',
self._vmid,
@@ -782,7 +786,8 @@
description)
for chunk in self._iter_progress(stream):
progress = self._parse_progress(chunk)
-yield DiskProgress(progress)
+if progress is not None:
+yield DiskProgress(progress)
if progress == 100:
break
@@ -807,8 +812,7 @@
def _parse_progress(self, chunk):
m = self.DISK_PROGRESS_RE.match(chunk)
if m is None:
-raise OutputParserError('error parsing progress, chunk: %r'
-% chunk)
+return None
tr
Change in vdsm[master]: command: Store stdout and stderr to a log file
Tomas Golembiovsky has uploaded a new change for review.
Change subject: command: Store stdout and stderr to a log file
..
command: Store stdout and stderr to a log file
Method execCmd() can be used to execute arbitrary command and return
stdout and stderr either as a string, or provide wrapper streams to the
file descriptors in case of asynchronous execution.
This change adds the possibility to store the stdout and stderr to a log
file independently of the execution method used (synchronous or
asynchronous). Redirecting both stdout and stderr into a single file is
also possible for asynchronous processesing.
Change-Id: I5a8b9afaa196729c8ab98f380a39833a9a0840cd
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M lib/vdsm/commands.py
M tests/commands_test.py
2 files changed, 128 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/33/59833/1
diff --git a/lib/vdsm/commands.py b/lib/vdsm/commands.py
index bffec83..de96097 100644
--- a/lib/vdsm/commands.py
+++ b/lib/vdsm/commands.py
@@ -43,7 +43,8 @@
def execCmd(command, sudo=False, cwd=None, data=None, raw=False,
printable=None, env=None, sync=True, nice=None, ioclass=None,
ioclassdata=None, setsid=False, execCmdLogger=logging.root,
-deathSignal=0, resetCpuAffinity=True):
+deathSignal=0, resetCpuAffinity=True, outLog=None, errLog=None,
+errToOutLog=False):
"""
Executes an external command, optionally via sudo.
@@ -52,6 +53,14 @@
a temporary thread, spawn a sync=False sub-process, and have the thread
finish, the new subprocess would die immediately.
"""
+if errToOutLog and sync:
+# Using errToOutLog together with sync makes not much sense. Since we
+# do not get messages as they come, all we can do is write all the
+# stderr after all the stdout.
+if outLog is None:
+outLog = errLog
+else:
+errLog = outLog
command = cmdutils.wrap_command(command, with_ioclass=ioclass,
ioclassdata=ioclassdata, with_nice=nice,
@@ -70,7 +79,8 @@
p = CPopen(command, close_fds=True, cwd=cwd, env=env,
deathSignal=deathSignal)
if not sync:
-p = AsyncProc(p)
+p = AsyncProc(p, outLog=outLog, errLog=errLog,
+ errToOutLog=errToOutLog)
if data is not None:
p.stdin.write(data)
p.stdin.flush()
@@ -85,6 +95,24 @@
out = ""
execCmdLogger.debug(cmdutils.retcode_log_line(p.returncode, err=err))
+
+# Dump stdout
+try:
+if outLog is not None:
+with open(outLog, 'w') as f:
+f.write(out)
+except IOError:
+execCmdLogger.exception(
+"Failed to dump stdout to log file '%s'" % outLog)
+
+# Dump stderr
+try:
+if errLog is not None:
+with open(errLog, 'w') as f:
+f.write(err)
+except IOError:
+execCmdLogger.exception(
+"Failed to dump stderr to log file '%s'" % errLog)
if not raw:
out = out.splitlines(False)
@@ -192,10 +220,15 @@
return len(data)
-def __init__(self, popenToWrap):
+def __init__(self, popenToWrap, outLog=None, errLog=None,
+ errToOutLog=False):
# this is an ugly hack to let this module load on Python 3, and fail
# later when AsyncProc is used.
from StringIO import StringIO
+
+if errToOutLog and outLog is None:
+outLog = errLog
+errLog = None
self._streamLock = threading.Lock()
self._proc = popenToWrap
@@ -227,6 +260,32 @@
self.stdin = io.BufferedWriter(self._streamWrapper(self,
self._stdin, self._fdin), BUFFSIZE)
+if outLog is None:
+self._outLog = None
+else:
+try:
+self._outLog = open(outLog, 'w')
+except IOError:
+logging.exception(
+"Failed to open stdout log file '%s'" % outLog)
+self._outLog = None
+
+if errToOutLog:
+self._errLog = self._outLog
+elif errLog is not None:
+try:
+self._errLog = open(errLog, 'w')
+except IOError:
+logging.exception(
+"Failed to open stderr log file '%s'" % errLog)
+self._errLog = None
+else:
+self._errLog = None
+
+self._fdMapLog = {fdout: self._outLog,
+ fderr: self._errLog,
+ self._fdin: None}
+
self._returncode = None
self.blocking = False
@@ -249,6 +308,8 @@
for fd, e
Change in vdsm[master]: build: Make sure run_tests*.sh scripts are executable
Tomas Golembiovsky has posted comments on this change. Change subject: build: Make sure run_tests*.sh scripts are executable .. Patch Set 7: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/55949 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1e3dc8ace7f69801b765262352903020cc8aef Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: pep8: Excluded files were not excluded from check
Tomas Golembiovsky has posted comments on this change. Change subject: pep8: Excluded files were not excluded from check .. Patch Set 1: Verified by running the pep8 manualy with all the generated arguments and with '-v' that prints names of all processed files. -- To view, visit https://gerrit.ovirt.org/59623 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f8485f0ced9c117d3416967a11dc6aef3a1dfe Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: pep8: Excluded files were not excluded from check
Tomas Golembiovsky has posted comments on this change. Change subject: pep8: Excluded files were not excluded from check .. Patch Set 1: Verified+1 -- To view, visit https://gerrit.ovirt.org/59623 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f8485f0ced9c117d3416967a11dc6aef3a1dfe Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: pep8: Excluded files were not excluded from check
Tomas Golembiovsky has posted comments on this change. Change subject: pep8: Excluded files were not excluded from check .. Patch Set 1: (1 comment) https://gerrit.ovirt.org/#/c/59623/1/tox.sh File tox.sh: Line 34 Line 35 Line 36 Line 37 Line 38 > well.. I have no problem with this patch. But locally and in jenkins, it ne Well I found it rather strange that nobody noticed it sooner. For me it runs the check on all the blacklisted files and reports errors. Could you please recheck it realy works for you the way it is? Try adding the '-v' argument, it will print the names of all files it checks. -- To view, visit https://gerrit.ovirt.org/59623 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f8485f0ced9c117d3416967a11dc6aef3a1dfe Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: pep8: Excluded files were not excluded from check
Tomas Golembiovsky has uploaded a new change for review.
Change subject: pep8: Excluded files were not excluded from check
..
pep8: Excluded files were not excluded from check
The files listed for exclusion were not excluded from PEP8 check. This
had two reasons.
First the exclusion pattern was improperly composed, because $$ was used
to expand variables instead of a single $. This is because the script
snippet originated from Makefile.
Second problem was that `pep8` does accept only one `--except` argument.
This meant that the first pattern list (which contained garbage anyway)
was ignored.
Change-Id: I40f8485f0ced9c117d3416967a11dc6aef3a1dfe
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M tox.sh
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/59623/1
diff --git a/tox.sh b/tox.sh
index 5d01520..717f547 100755
--- a/tox.sh
+++ b/tox.sh
@@ -33,9 +33,9 @@
if [ 'pep8' = "$1" ]; then
for x in ${PEP8_BLACKLIST[@]}; do \
-exclude="$${exclude},$${x}" ; \
+exclude="${exclude},${x}" ; \
done ; \
-pep8 --exclude="$${exclude}" --exclude='.tox/*' \
+pep8 --exclude="${exclude},.tox" \
--filename '*.py' . \
"${WHITELIST[@]}"
fi
--
To view, visit https://gerrit.ovirt.org/59623
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I40f8485f0ced9c117d3416967a11dc6aef3a1dfe
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: build: Make sure run_tests*.sh scripts are executable
Tomas Golembiovsky has posted comments on this change. Change subject: build: Make sure run_tests*.sh scripts are executable .. Patch Set 5: (1 comment) https://gerrit.ovirt.org/#/c/55949/5/build-aux/Makefile.subs File build-aux/Makefile.subs: Line 22: Line 23: # Reference: Line 24: # http://www.gnu.org/software/automake/manual/html_node/Scripts.html Line 25: PATHSUBST = sed \ Line 26:-e "s,[@]top_srcdir[@],$(top_srcdir),g" \ > this part still seems not related Yes and no. If we don't replace top_srcdir with the path provided in the Makefile, config.status will replace it later. But config.status will always replace it with '.' instead of the real path to the root directory. That will lead to broken paths in all generated .sh files that are not in the root directory (e.g. those in the tests/). Line 27:-e "s,[@]BACKUPDIR[@],$(vdsmbackupdir),g" \ Line 28:-e "s,[@]BINDIR[@],$(bindir),g" \ Line 29:-e "s,[@]CONFDIR[@],$(vdsmconfdir),g" \ Line 30:-e "s,[@]HOOKSDIR[@],$(vdsmhooksdir),g" \ -- To view, visit https://gerrit.ovirt.org/55949 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1e3dc8ace7f69801b765262352903020cc8aef Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: build: Make sure run_tests*.sh scripts are executable
Tomas Golembiovsky has posted comments on this change. Change subject: build: Make sure run_tests*.sh scripts are executable .. Patch Set 5: Code-Review+1 Verified+1 -- To view, visit https://gerrit.ovirt.org/55949 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1e3dc8ace7f69801b765262352903020cc8aef Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: build: Make sure run_tests*.sh scripts are executable
Tomas Golembiovsky has posted comments on this change. Change subject: build: Make sure run_tests*.sh scripts are executable .. Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/55949/4/build-aux/Makefile.subs File build-aux/Makefile.subs: Line 39: Line 40: CONFIGSUBST = $(top_builddir)/config.status --file=- Line 41: Line 42: %:: %.in Line 43:@echo " SED $@"; $(PATHSUBST) $< |$(CONFIGSUBST) >$@ > You've got the docs almost right. It's linked from the URL you posted and i I have found out why it worked for Milan. Automake generated Makefiles have a rule '.SUFFIXES:' which removes the dummy rule that prevents matching the *.sh targets. It deletes all known suffixes for suffix rules. It is documented here: https://www.gnu.org/software/make/manual/html_node/Suffix-Rules.html#index-SUFFIXES The list of know suffixes also affect the match anything rules: Dummy pattern rules such as the one for ‘%.p’ are made for every suffix listed as valid for use in suffix rules (see Old-Fashioned Suffix Rules). Apparently cleaning the list also removes all the dummy patterns used to limit the matching. Anyway, by using :: to mark the rule as terminal puts us on the safe side. Line 44:@if expr "$@" : ".*\\.sh" >/dev/null ; then \ Line 45:chmod a+x "$@" ; \ -- To view, visit https://gerrit.ovirt.org/55949 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1e3dc8ace7f69801b765262352903020cc8aef Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: build: Make sure run_tests*.sh scripts are executable
Tomas Golembiovsky has posted comments on this change. Change subject: build: Make sure run_tests*.sh scripts are executable .. Patch Set 4: Code-Review+1 (1 comment) https://gerrit.ovirt.org/#/c/55949/4/build-aux/Makefile.subs File build-aux/Makefile.subs: Line 39: Line 40: CONFIGSUBST = $(top_builddir)/config.status --file=- Line 41: Line 42: %:: %.in Line 43:@echo " SED $@"; $(PATHSUBST) $< |$(CONFIGSUBST) >$@ > my proposal should chmod the file after they are created during make.. why You've got the docs almost right. It's linked from the URL you posted and it's here: https://www.gnu.org/software/make/manual/html_node/Match_002dAnything-Rules.html The important paragraphs are: If you do not mark the match-anything rule as terminal, then it is non-terminal. A non-terminal match-anything rule cannot apply to a file name that indicates a specific type of data. A file name indicates a specific type of data if some non-match-anything implicit rule target matches it. ... Special built-in dummy pattern rules are provided solely to recognize certain file names so that non-terminal match-anything rules will not be considered. These dummy rules have no prerequisites and no recipes, and they are ignored for all other purposes. Unfortunately .sh files are amog them and that's why '%: %.in' rule will not match them. Yaniv: The problem is that everytime you add new .sh file you have to modify the Makefile rule. Milan: Are you sure it works without it? If yes, it could be related to specific version of make. You can try the following Makefile. Running `make prepare && make` should fail to build `foo.sh`. all: bar baz.conf foo.sh % : %.in touch $@ prepare: touch foo.sh.in bar.in baz.conf.in Line 44:@if expr "$@" : ".*\\.sh" >/dev/null ; then \ Line 45:chmod a+x "$@" ; \ -- To view, visit https://gerrit.ovirt.org/55949 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1e3dc8ace7f69801b765262352903020cc8aef Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
Change in vdsm[master]: schema: Marked optional fields in ExternalVmInfo.
Tomas Golembiovsky has posted comments on this change. Change subject: schema: Marked optional fields in ExternalVmInfo. .. Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/57417/1//COMMIT_MSG Commit Message: PS1, Line 9: empty in certain cases > for my understanding, please provide some examples I might be just too paranoid. But the thing is we are assuming all the data is in the returned XML. The relevant code is in lib/vdsm/v2v.py in _add_general_info(). If something were missing we would just silently ignore that. On the second thought, adding at least log.info in such case wouldn't hurt. I didn't see any info which elements/fields are mandatory in the XML docs. In general it seems to be driver/version dependent. In this specific case we may probably take the bet and safely assume the necessary info will always be there. If you think I'm just too paranoid we can make this change only about preparation for the upcoming API change. https://gerrit.ovirt.org/#/c/57417/1/lib/api/vdsmapi-schema.json File lib/api/vdsmapi-schema.json: Line 4003: # map of VM properties Line 4004: # Line 4005: # @vmName: The name of the VM Line 4006: # Line 4007: # @status: The current VM status What if we mark this field as optional too? Just for the completeness sake -- in relation to the following API change. That way we can later drop the state from returned values and return really just the names. Line 4008: # Line 4009: # @vmId:#optional The VM UUID Line 4010: # Line 4011: # @smp: #optional Number of CPUs -- To view, visit https://gerrit.ovirt.org/57417 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e2e7f81ba39be2616f732de1deff477a1ca6c65 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: tests: Prevent multiple invocations of makecerts.sh
Tomas Golembiovsky has posted comments on this change. Change subject: tests: Prevent multiple invocations of makecerts.sh .. Patch Set 1: Verified+1 -- To view, visit https://gerrit.ovirt.org/57344 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5553057e0c0a66ccb96aa099a6608002f42d513f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: Making try block smaller in tobool()
Tomas Golembiovsky has posted comments on this change. Change subject: utils: Making try block smaller in tobool() .. Patch Set 1: Based on the documentation I would say that 'type()' and 'isinstance()' is safe in our case. I am not sure if there is any danger with the 'lower()' method (any dangerous strings?), but I would guess not. But we can catch 'UnicodeError' to be safe. So the only real danger is the 'int()' call. Can you think of any other exceptions besides 'ValueError' and 'TypeError' it can possibly raise? -- To view, visit https://gerrit.ovirt.org/57549 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c7326798fc5a2c8c1491fb7433657fa460495be Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: Properly handle int argument in tobool().
Tomas Golembiovsky has posted comments on this change. Change subject: utils: Properly handle int argument in tobool(). .. Patch Set 4: (2 comments) https://gerrit.ovirt.org/#/c/57511/4//COMMIT_MSG Commit Message: Line 8: Line 9: The code attempts to call 'lower()' method withouth making sure the type Line 10: of the argument is a string. Line 11: Line 12: Although the code suggests the method should handle an int argument it > Why should we handle int argument in a helper for converting configuration I might have misunderstood the idea. It could be that the trick with bool(int()) is only to handle strings '0' and '1' as truth values. Line 13: actually always returns 'False' on int argument because the exception: Line 14: Line 15: AttributeError: 'int' object has no attribute 'lower' Line 16: https://gerrit.ovirt.org/#/c/57511/4/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 407 Line 408 Line 409 Line 410 Line 411 > s == string - int in not a valid input here. So if I understand it right, you would strip it to: def tobool(s): if s.lower() == 'true': return True return False What about this: def tobool(s): if not isinstance(s, basestring): raise TypeError if s.lower() == 'true': return True try: return bool(int(s)) except ValueError: return False That way we can keep the posibility to use strings '0' and '1'. -- To view, visit https://gerrit.ovirt.org/57511 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Lazy loading of external VMs info
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Lazy loading of external VMs info .. Patch Set 3: (3 comments) https://gerrit.ovirt.org/#/c/57418/3//COMMIT_MSG Commit Message: PS3, Line 7: Lazy > (similar to patch reviewed not-long ago) I'm not fan of term "lazy" as it's What about "on demand"? https://gerrit.ovirt.org/#/c/57418/3/lib/api/vdsm-api.yml File lib/api/vdsm-api.yml: PS3, Line 7108: - string > Extra indent? So it seems. I thought it should be indented. I will fix it. https://gerrit.ovirt.org/#/c/57418/3/lib/vdsm/v2v.py File lib/vdsm/v2v.py: PS3, Line 146: if vm_names is not None and len(vm_names) == 0: > Why does vdsClient allow 4 arguments then? names_only should imply >= 1 vm_ These two arguments are unrelated. names_only is for limiting what kind of information we return whereas vm_names says for which VMs we return the information. -- To view, visit https://gerrit.ovirt.org/57418 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3dc6fc4573b2c0b1c03ed87025452e14af2fc566 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: Making try block smaller in tobool()
Tomas Golembiovsky has uploaded a new change for review. Change subject: utils: Making try block smaller in tobool() .. utils: Making try block smaller in tobool() The try-except block was too broad and spanned unnecessarily large block of code. That is a good way to hide bugs. Change-Id: I9c7326798fc5a2c8c1491fb7433657fa460495be Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com> --- M lib/vdsm/utils.py 1 file changed, 7 insertions(+), 7 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/49/57549/1 diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py index 40bb4d5..23e3171 100644 --- a/lib/vdsm/utils.py +++ b/lib/vdsm/utils.py @@ -409,15 +409,15 @@ def tobool(s): +if s is None: +return False +if type(s) == bool: +return s +if isinstance(s, basestring) and s.lower() == 'true': +return True try: -if s is None: -return False -if type(s) == bool: -return s -if isinstance(s, basestring) and s.lower() == 'true': -return True return bool(int(s)) -except: +except ValueError: return False -- To view, visit https://gerrit.ovirt.org/57549 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I9c7326798fc5a2c8c1491fb7433657fa460495be Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: Properly handle int argument in tobool().
Tomas Golembiovsky has posted comments on this change. Change subject: utils: Properly handle int argument in tobool(). .. Patch Set 3: (1 comment) https://gerrit.ovirt.org/#/c/57511/3/lib/vdsm/utils.py File lib/vdsm/utils.py: PS3, Line 420:except ValueError: > +1. The former one-liner solves the bug in the function. I aggree that having the try-except only on the last return should be enought. Fair enough, I will make a follow-up patch for that. -- To view, visit https://gerrit.ovirt.org/57511 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: Fix bug in tobool()
Tomas Golembiovsky has posted comments on this change. Change subject: utils: Fix bug in tobool() .. Patch Set 1: (4 comments) https://gerrit.ovirt.org/#/c/57511/1//COMMIT_MSG Commit Message: PS1, Line 7: utils: Fix bug in tobool() > I'd write: Done https://gerrit.ovirt.org/#/c/57511/1/lib/vdsm/utils.py File lib/vdsm/utils.py: PS1, Line 411: tobool > let's add tests to verify that While we're at it, I would rather test all values that this method tries to support. PS1, Line 417: if str(s).lower() == 'true': > Milan raised a good point. Please do it his way. Done PS1, Line 420: except: > This needs to be specialized. Done -- To view, visit https://gerrit.ovirt.org/57511 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: Fix bug in tobool()
Tomas Golembiovsky has uploaded a new change for review. Change subject: utils: Fix bug in tobool() .. utils: Fix bug in tobool() The code attempts to call 'lower()' method withouth making sure the type of the argument is a string. Although the code suggests the method should handle an int argument it actually always returns 'False' on int argument because the exception: AttributeError: 'int' object has no attribute 'lower' is silently handled. Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com> --- M lib/vdsm/utils.py 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/11/57511/1 diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py index e6b8dd5..0bee912 100644 --- a/lib/vdsm/utils.py +++ b/lib/vdsm/utils.py @@ -414,7 +414,7 @@ return False if type(s) == bool: return s -if s.lower() == 'true': +if str(s).lower() == 'true': return True return bool(int(s)) except: -- To view, visit https://gerrit.ovirt.org/57511 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1740f82f5fa5eb50c319b46ce428bbb9f8c0c15e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Lazy loading of external VMs info
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: Lazy loading of external VMs info
..
Patch Set 1:
(3 comments)
Francesco: You mean why not add another API call? The name 'getExternalVMs'
seems generic enough to encompass this change. The new argument governs what
kind of information we include int the result. In fact the name
'getExternalVMs' does not say anything about what to expect in the result (is
it names? IDs? UUIDs? some other info?...).
This is even more complicated by the fact that the "new" call does not return
just the names but also the sate (!) as pointed out in other comments. If the
return value were only the list of names then adding a new API call like
'listExternalVMsNames' would be worth considering. But at current situation...
I am against it.
https://gerrit.ovirt.org/#/c/57418/1/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 413: if s is None:
Line 414: return False
Line 415: if type(s) == bool:
Line 416: return s
Line 417: if str(s).lower() == 'true':
> +1
I don't really need this. Although I'm using tobool() my 's' is already a
string.
It's just a bug I discovered during the process. I'll make a separate patch for
it then.
Line 418: return True
Line 419: return bool(int(s))
Line 420: except:
Line 421: return False
https://gerrit.ovirt.org/#/c/57418/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
Line 166: _add_vm(conn, vms, vm)
Line 167: else:
Line 168: vms.append({'vmName': vm.name(),
Line 169: 'status': 'Up' if vm.ID() > -1 else
'Down'})
Line 170:
> ... and we are already inconsistent with vm.ID() vs. vm.isActive() without
Yes, it already is inconsistent. The reason for that is that isActive()
requires another API call to the hypervisor which I try to avoid. Whereas the
ID() is already available after the discovery we have done in _list_domains().
I will add a comment about it.
I don't particularly like it, and I would rather return just the names. It is
an ugly hack. I don't know if it's even officialy documented somewhere. It
looks just like a convention of libvirt that active VMs have defined ID and
stopped VMs have ID set to -1. Sadly it seems necessary to return the state in
order to solve the 'chicken and egg' problem. Today the engine does some
filtering based on the state already while displaying the list of VM names. So
until somebody reimplements the engine side to use our new API we probably need
to return the state.
Line 171: return {'status': doneCode, 'vmList': vms}
Line 172:
Line 173:
Line 174: def convert_external_vm(uri, username, password, vminfo, job_id, irs):
https://gerrit.ovirt.org/#/c/57418/1/tests/v2vTests.py
File tests/v2vTests.py:
Line 328: def _connect(uri, username, passwd):
Line 329: return MockVirConnect(vms=self._vms)
Line 330:
Line 331: vmIDs = [1, 3]
Line 332: names = [vm.name for vm in VM_SPECS if vm.id in vmIDs]
> I'd suggest adding a non-existent name here to check that nothing bad happe
Done
Line 333:
Line 334: with MonkeyPatchScope([(libvirtconnection, 'open_connection',
Line 335: _connect)]):
Line 336: vms = v2v.get_external_vms('esx://mydomain', 'user',
--
To view, visit https://gerrit.ovirt.org/57418
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3dc6fc4573b2c0b1c03ed87025452e14af2fc566
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Treat Xen specially after all.
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: Treat Xen specially after all.
..
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/57431/1/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
Line 993: try:
Line 994: ret = vm.hasCurrentSnapshot()
Line 995: except libvirt.libvirtError as e:
Line 996: if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT or \
Line 997: conn.getType() != 'Xen':
> Well, if we expect that this on any driver but xen, and we never expect it
This question is sort of point of this discussion and reason why the change is
submitted as two separate patches. I guess I should have started the comment to
make it clear.
So, do we want to be generic here and make the decision regardless of the
driver, or does the driver matter to us? Francesco prefers the generic way
IIRC. Me on the other hand, I would rather not make any assumptions about other
drivers as it might hide some errors that we would like to know about later.
Note that the discussion is solely about whether to have an error message in a
log or not. The fact that the API call fails does not mean we return failure to
the callee.
Line 998: logging.exception('Error checking for existing
snapshots.')
Line 999: # else: The snapshot related API is not implemented in all
libvirt
Line 1000: # drivers. It is missing e.g. in Xen. But we know that
already,
Line 1001: # so no need to log the exception.
--
To view, visit https://gerrit.ovirt.org/57431
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I29befae33113f80a2011670c1edbff331becc72d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Treat Xen specially after all.
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Treat Xen specially after all. .. Patch Set 1: The cause of CI -1 seems unrelated to the change. Yum failed to install stuff from EPEL. -- To view, visit https://gerrit.ovirt.org/57431 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I29befae33113f80a2011670c1edbff331becc72d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Treat Xen specially after all.
Tomas Golembiovsky has uploaded a new change for review.
Change subject: v2v: Treat Xen specially after all.
..
v2v: Treat Xen specially after all.
The condition in the exception handler may be too general and may hide
some unforeseen error cases in other drivers that we would like to know
about.
Change-Id: I29befae33113f80a2011670c1edbff331becc72d
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M lib/vdsm/v2v.py
1 file changed, 4 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/31/57431/1
diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py
index 946f61b..25d0a1b 100644
--- a/lib/vdsm/v2v.py
+++ b/lib/vdsm/v2v.py
@@ -993,10 +993,12 @@
try:
ret = vm.hasCurrentSnapshot()
except libvirt.libvirtError as e:
-if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT:
+if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT or \
+conn.getType() != 'Xen':
logging.exception('Error checking for existing snapshots.')
# else: The snapshot related API is not implemented in all libvirt
-# drivers. It is missing e.g. in Xen.
+# drivers. It is missing e.g. in Xen. But we know that already,
+# so no need to log the exception.
else:
params['has_snapshots'] = ret > 0
--
To view, visit https://gerrit.ovirt.org/57431
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I29befae33113f80a2011670c1edbff331becc72d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Do not give Xen a special treatment.
Tomas Golembiovsky has uploaded a new change for review.
Change subject: v2v: Do not give Xen a special treatment.
..
v2v: Do not give Xen a special treatment.
Allways perform the check for snapshots and do not give Xen a special
treatment. Instead watch for VIR_ERR_NOT_SUPPORTED, which is more
generic.
Based on the discussion here:
https://gerrit.ovirt.org/#/c/56574/10/lib/vdsm/v2v.py@903
Change-Id: I597118940d42ee953d9cdc83bb44afd13b5e882a
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M lib/vdsm/v2v.py
1 file changed, 5 insertions(+), 6 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/30/57430/1
diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py
index a4a4f17..946f61b 100644
--- a/lib/vdsm/v2v.py
+++ b/lib/vdsm/v2v.py
@@ -990,14 +990,13 @@
def _add_snapshot_info(conn, vm, params):
-# Snapshot related API is not yet implemented in the libvirt's Xen driver
-if conn.getType() == 'Xen':
-return
-
try:
ret = vm.hasCurrentSnapshot()
-except libvirt.libvirtError:
-logging.exception('Error checking for existing snapshots.')
+except libvirt.libvirtError as e:
+if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT:
+logging.exception('Error checking for existing snapshots.')
+# else: The snapshot related API is not implemented in all libvirt
+# drivers. It is missing e.g. in Xen.
else:
params['has_snapshots'] = ret > 0
--
To view, visit https://gerrit.ovirt.org/57430
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I597118940d42ee953d9cdc83bb44afd13b5e882a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: schema: Marked optional fields in ExternalVmInfo.
Tomas Golembiovsky has posted comments on this change. Change subject: schema: Marked optional fields in ExternalVmInfo. .. Patch Set 1: Verified+1 There is nothing to verify, is there? -- To view, visit https://gerrit.ovirt.org/57417 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e2e7f81ba39be2616f732de1deff477a1ca6c65 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Lazy loading of external VMs info
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Lazy loading of external VMs info .. Patch Set 1: Verified+1 (1 comment) https://gerrit.ovirt.org/#/c/57418/1/lib/api/vdsm-api.yml File lib/api/vdsm-api.yml: Line 7095: type: string Line 7096: Line 7097: - defaultvalue: False Line 7098: description: Request only names, not complete info about hosts Line 7099: name: names_only This is a misnomer actually, because we are returning also VM state and not just the name. Should I rename it somehow, or is this OK? Any suggestions? Line 7100: added: '4.0' Line 7101: type: boolean Line 7102: Line 7103: - defaultvalue: null -- To view, visit https://gerrit.ovirt.org/57418 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3dc6fc4573b2c0b1c03ed87025452e14af2fc566 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Lazy loading of external VMs info
Tomas Golembiovsky has uploaded a new change for review.
Change subject: v2v: Lazy loading of external VMs info
..
v2v: Lazy loading of external VMs info
API changes allowing implementation of lazy loading of information about
external VMs. Fetching of all the information is rather expensive
operation especially from VMware hosts. Now we can fetch the names first
and then load all the other information only for specified subset of
VMs.
Change-Id: I3dc6fc4573b2c0b1c03ed87025452e14af2fc566
Bug-Url: https://bugzilla.redhat.com/1294629
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M client/vdsClient.py
M lib/api/vdsm-api.yml
M lib/api/vdsmapi-schema.json
M lib/vdsm/rpc/bindingxmlrpc.py
M lib/vdsm/utils.py
M lib/vdsm/v2v.py
M tests/v2vTests.py
M vdsm/API.py
8 files changed, 107 insertions(+), 20 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/18/57418/1
diff --git a/client/vdsClient.py b/client/vdsClient.py
index c7b359d..514cd92 100755
--- a/client/vdsClient.py
+++ b/client/vdsClient.py
@@ -1900,10 +1900,19 @@
return 0, ''
def getExternalVMs(self, args):
-if len(args) != 3:
+if len(args) < 3:
raise ValueError('Wrong number of arguments')
-uri, username, password = args
-status = self.s.getExternalVMs(uri, username, password)
+
+uri, username, password = args[0:3]
+if len(args) > 3:
+names_only = utils.tobool(args[3])
+vm_names = args[4:]
+else:
+names_only = False
+vm_names = []
+
+status = self.s.getExternalVMs(uri, username, password, names_only,
+ vm_names)
if status['status']['code'] == 0:
vmList = status['vmList']
for vm in vmList:
@@ -2865,7 +2874,7 @@
)),
'externalVMList': (
serv.getExternalVMs, (
-' ',
+' [ [ ...] ]'
'get VMs from external hypervisor'
)),
'getHostJobs': (
diff --git a/lib/api/vdsm-api.yml b/lib/api/vdsm-api.yml
index 40a0cfe..6c7c17b 100644
--- a/lib/api/vdsm-api.yml
+++ b/lib/api/vdsm-api.yml
@@ -7093,6 +7093,20 @@
- description: libvirt connection password
name: password
type: string
+
+- defaultvalue: False
+description: Request only names, not complete info about hosts
+name: names_only
+added: '4.0'
+type: boolean
+
+- defaultvalue: null
+description: Limit query only to hosts with specified names
+name: vm_names
+added: '4.0'
+type:
+- string
+
return:
description: A list of VMs (List of ExternalVmInfo)
type:
diff --git a/lib/api/vdsmapi-schema.json b/lib/api/vdsmapi-schema.json
index b2dd77a..391666b 100644
--- a/lib/api/vdsmapi-schema.json
+++ b/lib/api/vdsmapi-schema.json
@@ -4049,13 +4049,20 @@
#
# @password: libvirt connection password
#
+# @names_only: #optional Request only names, not complete info about hosts
+# (new in version 4.18.0)
+#
+# @vm_names: #optional Limit query only to hosts with specified names.
+# (new in version 4.18.0)
+#
# Returns:
# A list of VMs (List of ExternalVmInfo)
#
# Since: 4.17.0
##
{'command': {'class': 'Host', 'name': 'getExternalVMs'},
- 'data': {'uri': 'str', 'username': 'str', 'password': 'str'},
+ 'data': {'uri': 'str', 'username': 'str', 'password': 'str',
+ '*names_only': 'bool', '*vm_names': '[str]'},
'returns': ['ExternalVmInfo']}
diff --git a/lib/vdsm/rpc/bindingxmlrpc.py b/lib/vdsm/rpc/bindingxmlrpc.py
index 8b8dcc7..e38b5bf 100644
--- a/lib/vdsm/rpc/bindingxmlrpc.py
+++ b/lib/vdsm/rpc/bindingxmlrpc.py
@@ -363,10 +363,12 @@
job_type = None if job_type == '' else job_type
return api.getJobs(job_type=job_type, job_ids=job_ids)
-def getExternalVMs(self, uri, username, password):
+def getExternalVMs(self, uri, username, password, names_only=False,
+ vm_names=None):
password = ProtectedPassword(password)
api = API.Global()
-return api.getExternalVMs(uri, username, password)
+return api.getExternalVMs(uri, username, password, names_only,
+ vm_names)
def getExternalVmFromOva(self, ova_path):
api = API.Global()
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py
index e6b8dd5..0bee912 100644
--- a/lib/vdsm/utils.py
+++ b/lib/vdsm/utils.py
@@ -414,7 +414,7 @@
return False
if type(s) == bool:
return s
-if s.lower() == 'true':
+if str(s).lower() == 'true':
return True
return bool(int(s))
except:
diff --git a/lib/vdsm/v2v.py b/lib/vdsm/v2v.py
index 4bedf18..3adeda2 100644
--- a/lib/vdsm/v2v.py
+++ b/lib/vdsm/v2v.
Change in vdsm[master]: schema: Marked optional fields in ExternalVmInfo.
Tomas Golembiovsky has uploaded a new change for review.
Change subject: schema: Marked optional fields in ExternalVmInfo.
..
schema: Marked optional fields in ExternalVmInfo.
Some fields may remain empty in certain cases, but they were not marked
as optional in the scheme. Small changes to capitalization included.
We prefer to fix the schema instead of adding defaults to the code.
Returning (nearly) empty structure will come in handy for next API
change that will provida us with the ability to do a lazy loading of
information.
Change-Id: I8e2e7f81ba39be2616f732de1deff477a1ca6c65
Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com>
---
M lib/api/vdsm-api.yml
M lib/api/vdsmapi-schema.json
2 files changed, 11 insertions(+), 8 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/17/57417/1
diff --git a/lib/api/vdsm-api.yml b/lib/api/vdsm-api.yml
index 81d86b3..40a0cfe 100644
--- a/lib/api/vdsm-api.yml
+++ b/lib/api/vdsm-api.yml
@@ -924,11 +924,13 @@
name: allocation
type: *VolumeAllocation
-- description: size of actual memory in MB
+- defaultvalue: null
+description: Size of actual memory in MB
name: memSize
type: uint
-- description: number of cpu
+- defaultvalue: null
+description: Number of CPUs
name: smp
type: uint
@@ -937,7 +939,8 @@
name: virtio_iso_path
type: string
-- description: The VM uuid
+- defaultvalue: null
+description: The VM UUID
name: vmId
type: *UUID
diff --git a/lib/api/vdsmapi-schema.json b/lib/api/vdsmapi-schema.json
index c2da818..b2dd77a 100644
--- a/lib/api/vdsmapi-schema.json
+++ b/lib/api/vdsmapi-schema.json
@@ -4006,11 +4006,11 @@
#
# @status: The current VM status
#
-# @vmId:The VM uuid
+# @vmId:#optional The VM UUID
#
-# @smp: number of cpu
+# @smp: #optional Number of CPUs
#
-# @memSize: size of actual memory in MB
+# @memSize: #optional Size of actual memory in MB
#
# @disks: #optional list of disk devices
#
@@ -4032,8 +4032,8 @@
# Since: 4.17.0
##
{'type': 'ExternalVmInfo',
- 'data': {'vmName': 'str', 'status': 'VmStatus', 'vmId': 'UUID', 'smp': 'uint',
- 'memSize': 'uint', '*disks': ['ExternalDiskInfo'],
+ 'data': {'vmName': 'str', 'status': 'VmStatus', '*vmId': 'UUID',
+ '*smp': 'uint', '*memSize': 'uint', '*disks': ['ExternalDiskInfo'],
'*networks': ['ExternalNetworkInfo'], '*format': 'VolumeFormat',
'*allocation': 'VolumeAllocation', '*virtio_iso_path': 'str',
'*graphics': 'str', '*video': 'str'}}
--
To view, visit https://gerrit.ovirt.org/57417
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e2e7f81ba39be2616f732de1deff477a1ca6c65
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Detect VM with snapshots
Tomas Golembiovsky has posted comments on this change.
Change subject: v2v: Detect VM with snapshots
..
Patch Set 11:
(1 comment)
https://gerrit.ovirt.org/#/c/56574/10/lib/vdsm/v2v.py
File lib/vdsm/v2v.py:
Line 899:
Line 900: e = root.find('./vcpu')
Line 901: if e is not None:
Line 902: try:
Line 903: params['smp'] = int(e.text)
> How about trying to acess libvirt, and catching the libvirt.VIR_ERR_NO_SUPP
Yes, I know. For listAllDomains() it makes sense to catch the exception,
because we have an alternative way to get the info. In this case we don't. It
would also make sense if we expected the snapshot related API to be filled in
at some point in the future. But since NONE of the snapshot related API calls
has been implemented in Xen driver so far, I think it's rather questionable it
will ever (or in near future) be.
Line 904: except ValueError:
Line 905: raise InvalidVMConfiguration("Invalid 'vcpu' value: %r" %
e.text)
Line 906:
Line 907: e = root.find('./os/type/[@arch]')
--
To view, visit https://gerrit.ovirt.org/56574
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9aa4de2faff92625cd0de8e3ae2a10a2d58aa823
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: tests: Prevent multiple invocations of makecerts.sh
Tomas Golembiovsky has uploaded a new change for review. Change subject: tests: Prevent multiple invocations of makecerts.sh .. tests: Prevent multiple invocations of makecerts.sh Wrapped the invocation of `makecerts.sh` in special rule to prevent multiple concurrent invocations of the script when make is running with multiple jobs (-j). Also there was no rule to build `server.p12` because it was missing from `makecerts.sh` targets in the build rule. To fix this and improve the readability all files are specified only on one place in a variable. Change-Id: I5553057e0c0a66ccb96aa099a6608002f42d513f Signed-off-by: Tomáš Golembiovský <tgole...@redhat.com> --- M tests/Makefile.am 1 file changed, 13 insertions(+), 4 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/44/57344/1 diff --git a/tests/Makefile.am b/tests/Makefile.am index 45f16bf..2e35f86 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -185,14 +185,20 @@ makecert.sh \ $(NULL) -dist_noinst_DATA = \ +server_certificates = \ server.crt \ server.csr \ server.key \ - server.p12 \ + server.p12 + +other_certificates = \ other.crt \ other.csr \ - other.key \ + other.key + +dist_noinst_DATA = \ + $(server_certificates) \ + $(other_certificates) \ run_tests_local.sh dist_vdsmtests_DATA = \ @@ -261,7 +267,10 @@ all-local: \ $(nodist_vdsmtests_PYTHON) -server.crt server.csr server.key other.crt other.csr other.key: makecert.sh +$(server_certificates) $(other_certificates): make_certificates + +.PHONY: make_certificates +make_certificates: makecert.sh ./makecert.sh run_modules = $(test_modules) -- To view, visit https://gerrit.ovirt.org/57344 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5553057e0c0a66ccb96aa099a6608002f42d513f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: v2v: Detect VM with snapshots
Tomas Golembiovsky has posted comments on this change. Change subject: v2v: Detect VM with snapshots .. Patch Set 11: Verified+1 -- To view, visit https://gerrit.ovirt.org/56574 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9aa4de2faff92625cd0de8e3ae2a10a2d58aa823 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Golembiovsky <tgole...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
