Change in vdsm[master]: vm: make new timekeeping revertable

Francesco Romani has uploaded a new change for review.

Change subject: vm: make new timekeeping revertable
......................................................................

vm: make new timekeeping revertable

The commit Icb0752e54a4cb9ff609b8ddfaf5c8fe2ed5b9e72
implemented the new timekeeping options recommended
by QEMU developers.

In order to maximize the backward compatibility and
to deal with possible regression with old guests,
this patch makes the new timekeeping settings revertable
by exposing a new configuration variable.

The default is enabled because those settings, being
recommended, are supposed to be safe.

Change-Id: I471be44454dcae6e73c46a473eb1eee19a5275ab
Signed-off-by: Francesco Romani 
---
M lib/vdsm/config.py.in
M vdsm/vm.py
2 files changed, 8 insertions(+), 3 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/43/24443/1

diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in
index 01590a1..075face 100644
--- a/lib/vdsm/config.py.in
+++ b/lib/vdsm/config.py.in
@@ -192,6 +192,9 @@

('transient_disks_repository', '@VDSMLIBDIR@/transient',
             'Local path to the transient disks repository.'),
+
+        ('new_timekeeping_enable', 'true',
+            'Enable the new recomended QEMU time keeping settings'),
     ]),

# Section: [ksm]
diff --git a/vdsm/vm.py b/vdsm/vm.py
index 9371049..d3baca8 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -970,10 +970,12 @@
         m = XMLElement('clock', offset='variable',
                        adjustment=str(self.conf.get('timeOffset', 0)))
         m.appendChildWithArgs('timer', name='rtc', tickpolicy='catchup')
-        m.appendChildWithArgs('timer', name='pit', tickpolicy='delay')

-        if self.arch == caps.Architecture.X86_64:
-            m.appendChildWithArgs('timer', name='hpet', present='no')
+        if config.getboolean('vars', 'new_timekeeping_enable'):
+            m.appendChildWithArgs('timer', name='pit', tickpolicy='delay')
+
+            if self.arch == caps.Architecture.X86_64:
+                m.appendChildWithArgs('timer', name='hpet', present='no')

self.dom.appendChild(m)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7234/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6344/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7128/ : SUCCESS

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 1:

Patch set 2:
looking for better, more meaningful name for the configurable.

The problem is: this setting does not just control the pit_delay, but also the HPET feature; I think it is better to have a single setting for both - given the reason why this setting exist at all, so the name
of the configurable must somehow reflect this.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7235/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6345/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7129/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/24443/2/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 193:         ('transient_disks_repository', '@VDSMLIBDIR@/transient',
Line 194:             'Local path to the transient disks repository.'),
Line 195: 
Line 196:         ('kvm_specific_timekeeping', 'true',
Line 197:             'Enable the best KVM time keeping settings;'
sorry for worrying about the name, but if we were 100% sure that what we use is the "best", we would not need this configurable. Also, "pit" is not kvm-specific. Plain qemu has it, I suppose.

maybe have

legacy_timekeeping_ovirt_3.0 = false

?
Line 198:             ' disable to revert to legacy QEMU settings'),
Line 199:     ]),
Line 200: 
Line 201:     # Section: [ksm]

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/24443/2/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 193:         ('transient_disks_repository', '@VDSMLIBDIR@/transient',
Line 194:             'Local path to the transient disks repository.'),
Line 195: 
Line 196:         ('kvm_specific_timekeeping', 'true',
Line 197:             'Enable the best KVM time keeping settings;'
> sorry for worrying about the name, but if we were 100% sure that what we us

I think you have a fair point here: the option name is a big part of this changeset after all. Is just I'm not the best at picking names :)
Line 198:             ' disable to revert to legacy QEMU settings'),
Line 199:     ]),
Line 200: 
Line 201:     # Section: [ksm]

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 2:

patch set 3: still chasing a good name. Followed Dan's advice for the new one.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 3: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7341/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6439/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7223/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 3: -Verified

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7345/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6443/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7227/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7346/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6444/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7228/ : SUCCESS

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 3: Code-Review-1

Marking with -1 only because I'd like to add this code only in case that a real problem materializes. Even in that case, per-vm custom property and a hook would be a better solution.

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 3: Verified+1

verification:

* added a custom vdsm.conf and runned vmTests.py.
* verified the XML produced is indeed the old one containing only the
<timer name="rtc" tickpolicy="catchup"/> element
* verified tests fail as expected

Itamar Heim has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 3:

ping

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 4: Code-Review-1 Verified+1

rebased to keep the patch current. Copied scores.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 4:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9028/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9169/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8240/ : SUCCESS

Michal Skrivanek has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 4:

how about joining this with hv_enlightenment's timer…maybe?

Itamar Heim has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 4:

ping

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 5: Verified-1 Code-Review-1

rebased
made hyperV-aware (and reverted the hyperV clock bit also)
copying score

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 5: Verified+1

fixed score :)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 5:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10310/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11095/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1328/ : There was an infra issue, please contact [e-mail]

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11252/ : FAILURE

Francesco Romani has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 6: Code-Review-1

(hopefully) last rebase before this precaution patch could be safely abandoned.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 6:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13899/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2009/ : There was an infra issue, please contact [e-mail]

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13110/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14063/ : FAILURE

Francesco Romani has abandoned this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Abandoned

we add a wide range of hyperv-related bugs and reports, but all calm from the side we were most concerned of. Abandoning, hopefully will never resurrect.

[e-mail] has posted comments on this change.

Change subject: vm: make new timekeeping revertable
......................................................................

Patch Set 6:

* Update tracker::IGNORE, no Bug-Url found