[libvirt] [PATCH 0/2] qemu: Refresh guest's RTC on daemon restart

Thing is, there are is an event that we use to keep our internal state in sync with qemu. If libvirtd is not running, our internal state gets out of sync. Therefore we must refresh it as soon as we are started. Michal Privoznik (2): qemu: Introduce qemuMonitorGetRTCTime qemu: Refresh RTC adjustment on qemuProcessReconnect src/qemu/qemu_monitor.c | 11 +++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 +++ src/qemu/qemu_process.c | 44 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+) -- 2.8.1

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 11 +++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 +++ 4 files changed, 62 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fe8e89f..d918662 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3856,3 +3856,14 @@ qemuMonitorMigrateStartPostCopy(qemuMonitorPtr mon) return qemuMonitorJSONMigrateStartPostCopy(mon); } + +int +qemuMonitorGetRTCTime(qemuMonitorPtr mon, + struct tm *tm) +{ + VIR_DEBUG("mon=%p", mon); + + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONGetRTCTime(mon, tm); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 470c729..5e1232b 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -970,4 +970,7 @@ int qemuMonitorMigrateIncoming(qemuMonitorPtr mon, int qemuMonitorMigrateStartPostCopy(qemuMonitorPtr mon); +int qemuMonitorGetRTCTime(qemuMonitorPtr mon, + struct tm *tm); + #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7bb9976..6f12ee1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6984,3 +6984,48 @@ qemuMonitorJSONMigrateStartPostCopy(qemuMonitorPtr mon) virJSONValueFree(reply); return ret; } + +int +qemuMonitorJSONGetRTCTime(qemuMonitorPtr mon, + struct tm *tm) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", + "s:path", "/machine", + "s:property", "rtc-time", + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-get reply was missing return data")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberInt(data, "tm_year", &tm->tm_year) < 0 || + virJSONValueObjectGetNumberInt(data, "tm_mon", &tm->tm_mon) < 0 || + virJSONValueObjectGetNumberInt(data, "tm_mday", &tm->tm_mday) < 0 || + virJSONValueObjectGetNumberInt(data, "tm_hour", &tm->tm_hour) < 0 || + virJSONValueObjectGetNumberInt(data, "tm_min", &tm->tm_min) < 0 || + virJSONValueObjectGetNumberInt(data, "tm_sec", &tm->tm_sec) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu returned malformed time")); + goto cleanup; + } + + ret = 0; + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 8b5d422..40f290e 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -494,4 +494,7 @@ int qemuMonitorJSONMigrateIncoming(qemuMonitorPtr mon, int qemuMonitorJSONMigrateStartPostCopy(qemuMonitorPtr mon) ATTRIBUTE_NONNULL(1); +int qemuMonitorJSONGetRTCTime(qemuMonitorPtr mon, + struct tm *tm) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif /* QEMU_MONITOR_JSON_H */ -- 2.8.1

On Fri, Apr 29, 2016 at 18:43:09 +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor.c | 11 +++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 +++ 4 files changed, 62 insertions(+)
[...]
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7bb9976..6f12ee1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6984,3 +6984,48 @@ qemuMonitorJSONMigrateStartPostCopy(qemuMonitorPtr mon) virJSONValueFree(reply); return ret; } + +int +qemuMonitorJSONGetRTCTime(qemuMonitorPtr mon, + struct tm *tm) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", + "s:path", "/machine", + "s:property", "rtc-time", + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-get reply was missing return data"));
Presence of the "return" object is guaranteed by qemuMonitorJSONCheckError at this point so this is dead code.
+ goto cleanup; + } +
ACK without the check.

https://bugzilla.redhat.com/show_bug.cgi?id=1139766 Thing is, for some reasons you can have your domain's RTC to be in something different than UTC. More weirdly, it's not only time zone what you can shift it of, but an arbitrary value. So, if domain is configured that way, libvirt will correctly put it onto qemu cmd line and moreover track it as this offset changes during domain's life time (e.g. because guest OS decides the best thing to do is set new time to RTC). Anyway, they way in which this tracking is implemented is events. But we've got a problem if change in guest's RTC occurs and the daemon is not running. The event is lost and we end up reporting invalid value in domain XML. Therefore, when the daemon is starting up again and it is reconnecting to all running domains, re-fetch their RTC so the correct offset value can be computed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0ccc3ac..9dd063c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1992,6 +1992,47 @@ qemuRefreshVirtioChannelState(virQEMUDriverPtr driver, return ret; } +static int +qemuRefreshRTC(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + time_t now, then; + struct tm thenbits; + long localOffset; + int ret, rv; + + if (vm->def->clock.offset != VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) + return 0; + + memset(&thenbits, 0, sizeof(thenbits)); + qemuDomainObjEnterMonitor(driver, vm); + now = time(NULL); + rv = qemuMonitorGetRTCTime(priv->mon, &thenbits); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + rv = -1; + + if (rv < 0) + goto cleanup; + + thenbits.tm_isdst = -1; + if ((then = mktime(&thenbits)) == (time_t) -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to convert time")); + goto cleanup; + } + + /* Thing is, @now is in local TZ but @then in UTC. */ + if (virTimeLocalOffsetFromUTC(&localOffset) < 0) + goto cleanup; + + vm->def->clock.data.variable.adjustment = then - now + localOffset; + + ret = 0; + + cleanup: + return ret; +} int qemuProcessRefreshBalloonState(virQEMUDriverPtr driver, @@ -3679,6 +3720,9 @@ qemuProcessReconnect(void *opaque) if (qemuRefreshVirtioChannelState(driver, obj) < 0) goto error; + if (qemuRefreshRTC(driver, obj) < 0) + goto error; + if (qemuProcessRefreshBalloonState(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) goto error; -- 2.8.1

On Fri, Apr 29, 2016 at 18:43:10 +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1139766
Thing is, for some reasons you can have your domain's RTC to be in something different than UTC. More weirdly, it's not only time zone what you can shift it of, but an arbitrary value. So, if domain is configured that way, libvirt will correctly put it onto qemu cmd line and moreover track it as this offset changes during domain's life time (e.g. because guest OS decides the best thing to do is set new time to RTC). Anyway, they way in which this tracking is implemented is events. But we've got a problem if change in guest's RTC occurs and the daemon is not running. The event is lost and we end up reporting invalid value in domain XML. Therefore, when the daemon is starting up again and it is reconnecting to all running domains, re-fetch their RTC so the correct offset value can be computed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
[...]
int qemuProcessRefreshBalloonState(virQEMUDriverPtr driver, @@ -3679,6 +3720,9 @@ qemuProcessReconnect(void *opaque) if (qemuRefreshVirtioChannelState(driver, obj) < 0) goto error;
+ if (qemuRefreshRTC(driver, obj) < 0) + goto error;
Any failure in qemuProcessReconnect results into libvirtd killing the process. I don't think this failure is criticall enough to allow such brutality. Peter

On 02.05.2016 16:36, Peter Krempa wrote:
On Fri, Apr 29, 2016 at 18:43:10 +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1139766
Thing is, for some reasons you can have your domain's RTC to be in something different than UTC. More weirdly, it's not only time zone what you can shift it of, but an arbitrary value. So, if domain is configured that way, libvirt will correctly put it onto qemu cmd line and moreover track it as this offset changes during domain's life time (e.g. because guest OS decides the best thing to do is set new time to RTC). Anyway, they way in which this tracking is implemented is events. But we've got a problem if change in guest's RTC occurs and the daemon is not running. The event is lost and we end up reporting invalid value in domain XML. Therefore, when the daemon is starting up again and it is reconnecting to all running domains, re-fetch their RTC so the correct offset value can be computed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
[...]
int qemuProcessRefreshBalloonState(virQEMUDriverPtr driver, @@ -3679,6 +3720,9 @@ qemuProcessReconnect(void *opaque) if (qemuRefreshVirtioChannelState(driver, obj) < 0) goto error;
+ if (qemuRefreshRTC(driver, obj) < 0) + goto error;
Any failure in qemuProcessReconnect results into libvirtd killing the process. I don't think this failure is criticall enough to allow such brutality.
Peter
Well, if guest has updated its RTC while we were not running, we would report spurious time in our XML. Since we are doing something similar for ejectable disks, i.e. CDROMs too, I presumed it is okay. But I don't mind changing this to something softer. Michal

On Mon, May 02, 2016 at 18:35:35 +0200, Michal Privoznik wrote:
On 02.05.2016 16:36, Peter Krempa wrote:
On Fri, Apr 29, 2016 at 18:43:10 +0200, Michal Privoznik wrote:
[...]
Any failure in qemuProcessReconnect results into libvirtd killing the process. I don't think this failure is criticall enough to allow such brutality.
Peter
Well, if guest has updated its RTC while we were not running, we would report spurious time in our XML. Since we are doing something similar for ejectable disks, i.e. CDROMs too, I presumed it is okay. But I don't mind changing this to something softer.
As long as it's guaranteed that any target qemu supported by libvirt does support the interrogation you are going to do it's probably fine. The problem with your patch is that qom-get was added in qemu 1.1 whereas libvirt supports versions since 0.12 This would effectively reduce compatibility to 1.1 which we should not do as a side effect of a unrelated change. Peter

On 03.05.2016 08:14, Peter Krempa wrote:
On Mon, May 02, 2016 at 18:35:35 +0200, Michal Privoznik wrote:
On 02.05.2016 16:36, Peter Krempa wrote:
On Fri, Apr 29, 2016 at 18:43:10 +0200, Michal Privoznik wrote:
[...]
Any failure in qemuProcessReconnect results into libvirtd killing the process. I don't think this failure is criticall enough to allow such brutality.
Peter
Well, if guest has updated its RTC while we were not running, we would report spurious time in our XML. Since we are doing something similar for ejectable disks, i.e. CDROMs too, I presumed it is okay. But I don't mind changing this to something softer.
As long as it's guaranteed that any target qemu supported by libvirt does support the interrogation you are going to do it's probably fine.
The problem with your patch is that qom-get was added in qemu 1.1 whereas libvirt supports versions since 0.12
This would effectively reduce compatibility to 1.1 which we should not do as a side effect of a unrelated change.
Okay, so if I make that check less drastic, do I have your ACK or do you want me to send v2? Michal

On Tue, May 03, 2016 at 09:31:13 +0200, Michal Privoznik wrote:
On 03.05.2016 08:14, Peter Krempa wrote:
[...]
As long as it's guaranteed that any target qemu supported by libvirt does support the interrogation you are going to do it's probably fine.
The problem with your patch is that qom-get was added in qemu 1.1 whereas libvirt supports versions since 0.12
This would effectively reduce compatibility to 1.1 which we should not do as a side effect of a unrelated change.
Okay, so if I make that check less drastic, do I have your ACK or do you want me to send v2?
I would have explicitly stated it if I did not want to see it. Peter
participants (2)
-
Michal Privoznik
-
Peter Krempa