12:52 p.m.
Milan Zamazal has uploaded a new change for review.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
virt: Make boolean values from boolean migration options
`compressed' and `autoConverge' migration options have boolean values in
the string form. They must be converted to actual booleans before they
are checked.
Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Bug-Url: https://bugzilla.redhat.com/1380822
Backport-To: 4.0
Signed-off-by: Milan Zamazal <mzamazal(a)redhat.com>
---
M vdsm/virt/migration.py
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/07/65007/1
diff --git a/vdsm/virt/migration.py b/vdsm/virt/migration.py
index bed481e..3f51c89 100644
--- a/vdsm/virt/migration.py
+++ b/vdsm/virt/migration.py
@@ -111,8 +111,8 @@
kwargs.get('maxBandwidth') or
config.getint('vars', 'migration_max_bandwidth')
)
- self._autoConverge = autoConverge
- self._compressed = compressed
+ self._autoConverge = utils.tobool(autoConverge)
+ self._compressed = utils.tobool(compressed)
self._incomingLimit = kwargs.get('incomingLimit')
self._outgoingLimit = kwargs.get('outgoingLimit')
self.status = {
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
12:52 p.m.
gerrit-hooks has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1:
* #1380822::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1380822::OK, public bug
* Check Product::#1380822::OK, Correct product Red Hat Enterprise Virtualization Manager
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
1:37 p.m.
Michal Skrivanek has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7:30 a.m.
Francesco Romani has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1: Code-Review+1
I find surprising it didn't fail before :)
This is "just" about polishing the code or you hit a bug caused by this?
If so, could be worth backporting
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
9:10 a.m.
Milan Zamazal has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1:
This is "just" about polishing the code or you hit a bug
caused by this?
I wanted to file a bug on the crash due to missing compression stats and wondered why I
couldn't reproduce it using user accessible means. Disabling migration compression in
Engine doesn't actually disable it due to the problem this patch fixes, so it's a
real bug and I think it should be backported to 4.0.
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
10:01 a.m.
Francesco Romani has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
11:07 a.m.
Milan Zamazal has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1: Verified+1
Verified that with this change and "minimal downtime" migration policy migration
still works, 0MB of compressed bytes are always reported in vdsm.log and a noticeable
slowdown (apparently due to autoconverge) is observable in the VM.
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
1:18 p.m.
Nir Soffer has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/65007/1/vdsm/virt/migration.py
File vdsm/virt/migration.py:
Line 111: kwargs.get('maxBandwidth') or
Line 112: config.getint('vars', 'migration_max_bandwidth')
Line 113: )
Line 114: self._autoConverge = utils.tobool(autoConverge)
Line 115: self._compressed = utils.tobool(compressed)
When does this value come from?
If the source is engine, we must accept only boolean values. If old engine send string
value instead of boolean, we must convert the values when the value enter vdsm.
Application code must not do any type conversions.
Line 116: self._incomingLimit = kwargs.get('incomingLimit')
Line 117: self._outgoingLimit = kwargs.get('outgoingLimit')
Line 118: self.status = {
Line 119: 'status': {
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
2:33 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://gerrit.ovirt.org/#/c/65007/1/vdsm/virt/migration.py
File vdsm/virt/migration.py:
Line 111: kwargs.get('maxBandwidth') or
Line 112: config.getint('vars', 'migration_max_bandwidth')
Line 113: )
Line 114: self._autoConverge = utils.tobool(autoConverge)
Line 115: self._compressed = utils.tobool(compressed)
When does this value come from?
Nir, you are right; and there
has been a long-standing effort to normalize Vdsm API
(https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic...
hint hint)
However, we are not there (yet). There are dozens of calls to tobool in the code, most of
them outside of a canonicalization layer.
This place is no different, and it is urgently required to solve a 4.0 bug. Let us take it
in, with an apologetic TODO, promising to fix it in the API layer.
Line 116: self._incomingLimit = kwargs.get('incomingLimit')
Line 117: self._outgoingLimit = kwargs.get('outgoingLimit')
Line 118: self.status = {
Line 119: 'status': {
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
3:12 p.m.
Nir Soffer has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 1: -Code-Review
(1 comment)
https://gerrit.ovirt.org/#/c/65007/1/vdsm/virt/migration.py
File vdsm/virt/migration.py:
Line 111: kwargs.get('maxBandwidth') or
Line 112: config.getint('vars', 'migration_max_bandwidth')
Line 113: )
Line 114: self._autoConverge = utils.tobool(autoConverge)
Line 115: self._compressed = utils.tobool(compressed)
Nir, you are right; and there has been a long-standing effort to
normalize
OK, lets have a TODO here for now.
Line 116: self._incomingLimit = kwargs.get('incomingLimit')
Line 117: self._outgoingLimit = kwargs.get('outgoingLimit')
Line 118: self.status = {
Line 119: 'status': {
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
3:08 a.m.
gerrit-hooks has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 2:
* #1380822::Update tracker: OK
* Check Bug-Url::OK
* Check Public Bug::#1380822::OK, public bug
* Check Product::#1380822::OK, Correct classification oVirt
* Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0)
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
3:30 a.m.
Milan Zamazal has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 2: Verified+1
No code change since the last verification, so Jenkins is enough.
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
3:31 a.m.
Milan Zamazal has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 2:
TODO added.
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
1 p.m.
Nir Soffer has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
4:24 a.m.
Francesco Romani has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
4:28 a.m.
Nir Soffer has submitted this change and it was merged.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
virt: Make boolean values from boolean migration options
`compressed' and `autoConverge' migration options have boolean values in
the string form. They must be converted to actual booleans before they
are checked.
Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Bug-Url: https://bugzilla.redhat.com/1380822
Backport-To: 4.0
Signed-off-by: Milan Zamazal <mzamazal(a)redhat.com>
Reviewed-on: https://gerrit.ovirt.org/65007
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer <nsoffer(a)redhat.com>
Reviewed-by: Francesco Romani <fromani(a)redhat.com>
---
M vdsm/virt/migration.py
1 file changed, 4 insertions(+), 2 deletions(-)
Approvals:
Nir Soffer: Looks good to me, approved
Jenkins CI: Passed CI tests
Francesco Romani: Looks good to me, approved
Milan Zamazal: Verified
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
4:28 a.m.
gerrit-hooks has posted comments on this change.
Change subject: virt: Make boolean values from boolean migration options
......................................................................
Patch Set 3:
* #1380822::Update tracker: OK
* #65007::Update tracker: OK
* Set MODIFIED::bug 1380822::::#1380822::IGNORE, skipping for branch 'master'
--
To view, visit https://gerrit.ovirt.org/65007
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I642eb607785a1b6f877092e187c91b7a065b38e1
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
2796
days inactive
2801
days old
vdsm-patches@lists.fedorahosted.org
16 comments
6 participants
participants (6)
-
automation＠ovirt.org -
Dan Kenigsberg -
fromani＠redhat.com -
michal.skrivanek＠redhat.com -
mzamazal＠redhat.com -
Nir Soffer