On 05/22/2014 05:07 AM, Laine Stump wrote:

Since there isn't a single libc API to get this value, this patch supplies one which gets the value by grabbing current UTC, then converting that into a struct tm with localtime_r(), then back to a time_t using mktime; it again does the same operation, but using gmtime_r() instead (for UTC). It then subtracts utc time from the localtime, and finally adjusts if dst is set in the localtime timeinfo (because for some reason mktime doesn't take that into account).

This function should be POSIX-compliant, and is threadsafe, but not async signal safe. If it was ever necessary to know this value in a child process, we could cache it with a one-time init function when libvirtd starts, then just supply the cached value, but that complexity isn't needed for current usage. ---

Change from V1: add test cases with TZ set to different values (if someone knows how to force DST on/off, I would gladly add some test cases for this as well).

Re-reading POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html I think you can do something like: VIR00:30VID where "VIR" is the normal time, and "VID" is the daylight-savings time 1 hour ahead. It was the absence of a second name that made POSIX treat the TZ value as not having daylight savings. I'll review the actual patch in another mail. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 05/22/2014 09:49 PM, Marcelo Tosatti wrote:

On Thu, May 22, 2014 at 02:07:27PM +0300, Laine Stump wrote:

...

Since there isn't a single libc API to get this value, this patch supplies one which gets the value by grabbing current UTC, then converting that into a struct tm with localtime_r(), then back to a time_t using mktime; it again does the same operation, but using gmtime_r() instead (for UTC). It then subtracts utc time from the localtime, and finally adjusts if dst is set in the localtime timeinfo (because for some reason mktime doesn't take that into account).

This function should be POSIX-compliant, and is threadsafe, but not async signal safe. If it was ever necessary to know this value in a child process, we could cache it with a one-time init function when libvirtd starts, then just supply the cached value, but that complexity isn't needed for current usage. ---

Change from V1: add test cases with TZ set to different values (if someone knows how to force DST on/off, I would gladly add some test cases for this as well).

man tzset:

The second format is used when there is daylight saving time:

std offset dst [offset],start[/time],end[/time]

Aha! And combining that with the "VIR" timezone idea that we're already using, I've found that the following string *does* set DST: TZ="VIR02:30VID,0,365" while the following *doesn't* (at least not today :-): TZ="VIR02:30VID,300,365" So I can add the former as a test case. Thanks!

On 05/23/2014 03:47 PM, Eric Blake wrote:

On 05/23/2014 05:43 AM, Laine Stump wrote:

...

...

man tzset:

The second format is used when there is daylight saving time:

std offset dst [offset],start[/time],end[/time] Aha! And combining that with the "VIR" timezone idea that we're already using, I've found that the following string *does* set DST:

TZ="VIR02:30VID,0,365"

That probably still has a window where running 'make check' on Dec 31 may fail; maybe using the time argument as in /23:59 will minimize the window to one minute?

Ah, right - because that is the *end* day. I just tried this in a virtual machine: declare -x TZ="VIR00:30VID,0,366 date --set "Dec 31 2012 23:59:57" (2012 is a leap year) The output of date showed "VID" (i.e. DST is in effect), and it continued to show VID right through midnight.

...

while the following *doesn't* (at least not today :-):

TZ="VIR02:30VID,300,365"

So I can add the former as a test case. It's awkward testing for daylight savings cases if the test is dependent on today's date; hopefully whatever you come up with is sufficiently isolated that it doesn't introduce spurious failures on certain days of the year.

I think the above does that.

Also, if I understand correctly, once you have a TZ with daylight savings rules, the tm_isdst of struct tm can be set to negative to use the TZ rules for determining if it is in effect, to 0 to force the standard time (even if the current time is during dst) and to 1 to force the daylight time (even if the current time is during std). (And that this is true for tm_isdst, but NOT for the global 'daylight', which is merely specified as positive year-round if TZ includes daylight rules).

Since the existence of tm_isdst is transparent to the caller of virTimeLocallOffsetFromUTC, we can't use that for testing purposes. And from the point of view of the function itself, I'd rather not manipulate that value if I don't have to - I'd rather leave as much to the system as possible, and just detect it with the test on systems that might behave differently - then we can fix it if necessary.

On 05/23/2014 05:45 PM, Laine Stump wrote:

On 05/22/2014 10:03 PM, Eric Blake wrote:

...

It would be a LOT simpler to just do:

#include <time.h>

tzset(); *offset = timezone;

except that some older builds of mingw lack the extern variable timezone. Or maybe even do a configure check, for AC_CHECK_DECLS([timezone]) (untested, just throwing out the idea), and having #if HAVE_DECL_TIMEZONE with the short code and the #else clause using this dance as the fallback for mingw? Or even just ditch older mingw? I see this in Fedora 20's cross-packages for mingw:

I'm so sick of this topic right now that I'd prefer leaving as is, although that is tempting.

Let me think about it for the next couple hours and get back to you.

Okay, I implemented this and the two tests I just added that checked DST started to fail. It appears that timezone doesn't account for DST. So unless there is also a standard way to get that information from a global, I think it would be cleaner just to forget about the global timezone and have the single implementation

Laine Stump wrote:

Since there isn't a single libc API to get this value, this patch supplies one which gets the value by grabbing current UTC, then converting that into a struct tm with localtime_r(), then back to a time_t using mktime; it again does the same operation, but using gmtime_r() instead (for UTC). It then subtracts utc time from the localtime, and finally adjusts if dst is set in the localtime timeinfo (because for some reason mktime doesn't take that into account).

This function should be POSIX-compliant, and is threadsafe, but not async signal safe. If it was ever necessary to know this value in a child process, we could cache it with a one-time init function when libvirtd starts, then just supply the cached value, but that complexity isn't needed for current usage.

Appears it breaks virtimetest on FreeBSD: TEST: virtimetest 20) Test localtime offset for "VIR-00:30" ... OK 21) Test localtime offset for "VIR-01:30" ... OK 22) Test localtime offset for "VIR-00:30VID,0/00:00:00,366/23:59:59" ... FAILED 23) Test localtime offset for "VIR-02:30VID,0/00:00:00,366/23:59:59" ... FAILED 24) Test localtime offset for "VIR-02:30VID-04:30,0/00:00:00,366/23:59:59" ... FAILED 25) Test localtime offset for "VIR-12:00VID-13:00,0/00:00:00,366/23:59:59" ... FAILED 26) Test localtime offset for "VIR02:45VID00:45,0/00:00:00,366/23:59:59" ... FAILED 27) Test localtime offset for "VIR05:00VID04:00,0/00:00:00,366/23:59:59" ... FAILED 28) Test localtime offset for "VIR11:00VID10:00,0/00:00:00,366/23:59:59" ... FAILED My initial impression is that it's something wrong on the FreeBSD side, because date(1) doesn't respect TZs from failing tests, for example: $ date Sat May 31 23:15:45 MSK 2014 $ TZ=VIR-00:30 date Sat May 31 19:45:53 VIR 2014 $ TZ=VIR-00:30VID,0/00:00:00,366/23:59:59 date Sat May 31 19:15:58 UTC 2014 So it looks like it's failing back to the UTC time. Roman Bogorodskiy

On 05/31/2014 11:11 PM, Roman Bogorodskiy wrote:

Roman Bogorodskiy wrote:

...

Laine Stump wrote:

...

Since there isn't a single libc API to get this value, this patch supplies one which gets the value by grabbing current UTC, then converting that into a struct tm with localtime_r(), then back to a time_t using mktime; it again does the same operation, but using gmtime_r() instead (for UTC). It then subtracts utc time from the localtime, and finally adjusts if dst is set in the localtime timeinfo (because for some reason mktime doesn't take that into account).

This function should be POSIX-compliant, and is threadsafe, but not async signal safe. If it was ever necessary to know this value in a child process, we could cache it with a one-time init function when libvirtd starts, then just supply the cached value, but that complexity isn't needed for current usage. Appears it breaks virtimetest on FreeBSD:

TEST: virtimetest 20) Test localtime offset for "VIR-00:30" ... OK 21) Test localtime offset for "VIR-01:30" ... OK 22) Test localtime offset for "VIR-00:30VID,0/00:00:00,366/23:59:59" ... FAILED 23) Test localtime offset for "VIR-02:30VID,0/00:00:00,366/23:59:59" ... FAILED 24) Test localtime offset for "VIR-02:30VID-04:30,0/00:00:00,366/23:59:59" ... FAILED 25) Test localtime offset for "VIR-12:00VID-13:00,0/00:00:00,366/23:59:59" ... FAILED 26) Test localtime offset for "VIR02:45VID00:45,0/00:00:00,366/23:59:59" ... FAILED 27) Test localtime offset for "VIR05:00VID04:00,0/00:00:00,366/23:59:59" ... FAILED 28) Test localtime offset for "VIR11:00VID10:00,0/00:00:00,366/23:59:59" ... FAILED

My initial impression is that it's something wrong on the FreeBSD side, because date(1) doesn't respect TZs from failing tests, for example:

$ date Sat May 31 23:15:45 MSK 2014 $ TZ=VIR-00:30 date Sat May 31 19:45:53 VIR 2014 $ TZ=VIR-00:30VID,0/00:00:00,366/23:59:59 date Sat May 31 19:15:58 UTC 2014

So it looks like it's failing back to the UTC time. After some more experiments I noticed that changing 366 to 365 makes it work:

$ TZ=VIR-00:30VID,0/00:00:00,365/23:59:59 date Sat May 31 21:34:25 VID 2014 $

And also changing that in the test makes it pass.

Sigh. Yeah, I made it 366 in an attempt to force DST to be always active, and it didn't seem to have any adverse side effects. In the meantime I ended up making most of the DST tests not run on Dec 31 / Jan 1 anyway, but never changed the end day because it wasn't causing harm. I just made that change locally and did a short test that ran successfully. If anything, it *could* cause a failure of the few DST tests that are left enabled all year, but only at the end of the year. Since the release of 1.2.5 is imminent, I've pushed the patch as a buil breaker.

Looking at this document:

http://pubs.opengroup.org/onlinepubs/9699919799/

It describes TZ environment variable format and says about the days that it should be:

0 <= n <= 365

I haven't yet found what should happen when the format given in TZ environment variable doesn't follow the format.

Roman Bogorodskiy

On 05/22/2014 05:07 AM, Laine Stump wrote:

This reverts commit e31b5cf393857a6ca78d148c19393e28dfb39de1.

This commit attempted to work around a bug in the offset value reported by qemu's RTC_CHANGE event in the case that a variable base date was given on the qemu commandline. The patch mixed up the math involved in arriving at the corrected offset to report, and in the process added an unnecessary private attribute to the clock element. Since that element is private/internal and not used by anyone else, it makes sense to simplify things by removing it. --- Unchanged from V2. This is the direct result of:

git revert e31b5cf393857a6ca78d148c19393e28dfb39de1

src/conf/domain_conf.c | 27 ++++----------------------- src/conf/domain_conf.h | 5 ----- src/qemu/qemu_command.c | 3 --- src/qemu/qemu_process.c | 13 ------------- 4 files changed, 4 insertions(+), 44 deletions(-)

ACK; but don't push until we have the rest of the series in good shape. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

[Adding qemu] On 05/22/2014 05:07 AM, Laine Stump wrote:

commit e31b5cf393857 attempted to fix libvirt's VIR_DOMAIN_EVENT_ID_RTC_CHANGE, which is documentated to always

s/documentated/documented/

provide the new offset of the domain's real time clock from UTC. The problem was that, in the case that qemu is provided with an "-rtc base=x" where x is an absolute time (rather than "utc" or "localtime"), the offset sent by qemu's RTC_CHANGE event is *not* the new offset from UTC, but rather is the sum of all changes to the domain's RTC since it was started with base=x.

So, despite what was said in commit e31b5cf393857, if we assume that the original value stored in "adjustment" was the offset from UTC at the time the domain was started, we can always determine the current offset from UTC by simply adding the most recent (i.e. current) offset from qemu to that original adjustment.

Is this true even if we miss an RTC update event from qemu? I'm worried about the following situation: user prepares to do a libvirtd upgrade, so libvirtd is shut down. Then the guest triggers an RTC update, so qemu sends an event, but the event is lost. Then libvirtd starts again, and doesn't realize the event is lost. Do we need more help from qemu, such as a new field to an existing QMP command (or a new QMP command) that lists the cumulative offset that qemu is using, where we call that query command any time after an RTC update event or after a libvirtd restart? I'm wondering if this is more a bug in qemu for not providing the right information rather than libvirt's responsibility to work around it. If the only way to keep accurate information is to sum the values we get from events, we are at risk of a lost event getting us messed up.

This patch accomplishes that by storing the initial adjustment in the domain's status as "adjustment0". Each time a new RTC_CHANGE event is received from qemu, we simply add adjustment0 to the value sent by qemu, store that as the new adjustment, and forward that value on to any event handler.

This patch (*not* e31b5cf393857, which should be reverted prior to applying this patch) fixes:

https://bugzilla.redhat.com/show_bug.cgi?id=964177

(for the case where basis='utc'. It does not fix basis='localtime') ---

Changes from V1: remove all attempts to fix basis='localtime' in favor of fixing it in a simpler and better manner in a separate patch.

I'd also appreciate it if the qemu developers can chime in on what is supposed to happen for localtime guests. There are at least four combinations: host running on UTC bios time, guest running on UTC time (in my opinion, the only sane setting, but we're talking about reality not sanity) host running on UTC, guest running on localtime (perhaps the guest is windows, and we know that windows prefers to run on localtime) host running on localtime bios (perhaps because it is dual-boot with windows, and windows prefers bios in localtime), guest running on UTC time host running on localtime, guest running on localtime But it gets even more complicated. The host localtime need not be consistent with the guest localtime. That is, I could be a cloud provider with servers on the east coast, and renting out processor time to a client on the west coast that wants their guest tied to west coast localtime. And that's assuming that both host and guest switch in and out of daylight savings at the same time, which falls apart when you cross political boundaries. Then there's the fun of migration (what if my server farm is spread across multiple timezones - does migration take into account the difference in localtime between source and destination servers). I can _totally_ understand the desire to run a GUEST in such a way that the guest thinks it has a bios stored in localtime (and when the guest updates the RTC twice a year to account for daylight savings, it changes what offset we track about the guest). But I think it is INSANITY to ever try and run a host on a localtime system (daylight savings changes in the host are just asking for problems to the guests) - so even if the host is tied to localtime bios, it is still probably wiser for qemu to base its offsets to UTC no matter what. If the commandline allows a specification of a localtime offset, I think it should be used ONLY for a one-time up-front conversion into a corresponding UTC offset, and then execute qemu in relation to utc thereafter (therefore, migration is always done in terms of utc, without regards for whether source and destination have a different localtime).

src/conf/domain_conf.c | 21 +++++++++++++++++---- src/conf/domain_conf.h | 7 +++++++ src/qemu/qemu_command.c | 9 +++++++++ src/qemu/qemu_process.c | 27 ++++++++++++++++++++++----- 4 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c

- if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) + if (vm->def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) { + /* when a basedate is manually given on the qemu commandline + * rather than simply "-rtc base=utc", the offset sent by qemu + * in this event is *not* the new offset from UTC, but is + * instead the new offset from the *original basedate* + + * uptime. For example, if the original offset was 3600 and + * the guest clock has been advanced by 10 seconds, qemu will + * send "10" in the event - this means that the new offset + * from UTC is 3610, *not* 10. If the guest clock is advanced + * by another 10 seconds, qemu will now send "20" - i.e. each + * event is the sum of the most recent change and all previous + * changes since the domain was started. Fortunately, we have + * saved the initial offset in "adjustment0", so to arrive at + * the proper new "adjustment", we just add the most recent + * offset to adjustment0. + */ + offset += vm->def->clock.data.variable.adjustment0; vm->def->clock.data.variable.adjustment = offset;

- if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - VIR_WARN("unable to save domain status with RTC change"); + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + VIR_WARN("unable to save domain status with RTC change"); + } + + event = virDomainEventRTCChangeNewFromObj(vm, offset);

virObjectUnlock(vm);

-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 05/23/2014 06:50 AM, Marcelo Tosatti wrote:
> On Thu, May 22, 2014 at 01:33:14PM -0600, Eric Blake wrote:
>> [Adding qemu]
>>
>> On 05/22/2014 05:07 AM, Laine Stump wrote:
>>> commit e31b5cf393857 attempted to fix libvirt's
>>> VIR_DOMAIN_EVENT_ID_RTC_CHANGE, which is documentated to always
>> s/documentated/documented/
>>
>>> provide the new offset of the domain's real time clock from UTC. The
>>> problem was that, in the case that qemu is provided with an "-rtc
>>> base=x" where x is an absolute time (rather than "utc" or
>>> "localtime"), the offset sent by qemu's RTC_CHANGE event is *not* the
>>> new offset from UTC, but rather is the sum of all changes to the
>>> domain's RTC since it was started with base=x.
>>>
>>> So, despite what was said in commit e31b5cf393857, if we assume that
>>> the original value stored in "adjustment" was the offset from UTC at
>>> the time the domain was started, we can always determine the current
>>> offset from UTC by simply adding the most recent (i.e. current) offset
>>> from qemu to that original adjustment.
>> Is this true even if we miss an RTC update event from qemu?  I'm worried
>> about the following situation:
>>
>> user prepares to do a libvirtd upgrade, so libvirtd is shut down. 
> If adjustment0 field is updated from adjustment, via a libvirtd shutdown, the current
> patch will also break, i believe. Not sure if thats possible, though.

No, adjustment0 is only set at the time a new qemu process is started.

>
>> Then the guest triggers an RTC update, so qemu sends an event, but the
>> event is lost. Then libvirtd starts again, and doesn't realize the
>> event is lost.

That case would only be a problem until the *next* time an RTC update is
sent; at that time the adjustment would be readjusted to adjustment0 +
new offset (and that new offset is the cumulative sum of all adjustments
since the domain was started).

> Yes, but that case is also true for any other QMP asynchronous event,
> and therefore should be handled generically i suppose (QMP channel data
> should be maintained across libvirtd shutdown). Luiz?
>
>> Do we need more help from qemu, such as a new field to an existing QMP
>> command (or a new QMP command) that lists the cumulative offset that
>> qemu is using, where we call that query command any time after an RTC
>> update event or after a libvirtd restart? I'm wondering if this is more
>> a bug in qemu for not providing the right information rather than
>> libvirt's responsibility to work around it.  If the only way to keep
>> accurate information is to sum the values we get from events, we are at
>> risk of a lost event getting us messed up.
> Good point, unsure whether its specific to this command, though. Could
> add a new QMP command to query the RTC offset, yes.
>
>>> This patch accomplishes that by storing the initial adjustment in the
>>> domain's status as "adjustment0". Each time a new RTC_CHANGE event is
>>> received from qemu, we simply add adjustment0 to the value sent by
>>> qemu, store that as the new adjustment, and forward that value on to
>>> any event handler.
>>>
>>> This patch (*not* e31b5cf393857, which should be reverted prior to
>>> applying this patch) fixes:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=964177
>>>
>>> (for the case where basis='utc'. It does not fix basis='localtime')
>>> ---
>>>
>>> Changes from V1: remove all attempts to fix basis='localtime' in favor
>>> of fixing it in a simpler and better manner in a separate patch.
>> I'd also appreciate it if the qemu developers can chime in on what is
>> supposed to happen for localtime guests.
>>
>> There are at least four combinations:
>>
>> host running on UTC bios time, guest running on UTC time (in my opinion,
>> the only sane setting, but we're talking about reality not sanity)
> 1) Why does Windows keep your BIOS clock on local time?
> http://blogs.msdn.com/b/oldnewthing/archive/2004/09/02/224672.aspx
>
> 2) Windows with RTC UTC
> https://wiki.archlinux.org/index.php/Time#UTC_in_Windows
>
>> host running on UTC, guest running on localtime (perhaps the guest is
>> windows, and we know that windows prefers to run on localtime)
> Its the default, thats all. See 1) above.
>
>> host running on localtime bios (perhaps because it is dual-boot with
>> windows, and windows prefers bios in localtime), guest running on UTC time
> Can't see why hosts using localtime or UTC is relevant. Assume host is
> synchronized to UTC via NTP (so you can use UTC or convert to localtime
> if desired).

Correct - the host's use of its own RTC is irrelevant. The only
potential difference is basis='utc' vs. basis='localtime', and anyone
using basis='localtime' more or less deserves the confusion it causes
:-P (That really is a "tongue in cheek" emoticon - I'm only half serious).

*However*, this discussion forced me to investigate some of the basic
assumptions that I'd been making when coming in to fix this bug. In
particular, my assumption was that the value of "adjustment" that was
set in the status would be preserved across a domain save/restore
operation, or a migration, but after talking to jdenemar and looking at
the code, I believe that this is *not* the case. As I understand it, one
of the driving factors behind having adjustment='variable' was that
changes to the RTC would be properly maintained across save/resoter and
migrations, and thus I ASSumed that the setting *was* being maintained,
but that the math was wrong. As far as I can tell, though, only the
*inactive* XML is saved in a save image or sent in a migration, while
the modified adjustment is only in the transient/status XML; the result
is that the modified adjustment (and modified basis per patch 4/4) are
lost any time there is a save/restore or migration.

So while these patches are correcting the math, I guess any claims that
I make in the commit logs about them fixing problems with migration or
save/restore are ill-informed, and should be removed (and we should
file/track a separate bug about that issue).

>> host running on localtime, guest running on localtime
>>
>> But it gets even more complicated.  The host localtime need not be
>> consistent with the guest localtime.  That is, I could be a cloud
>> provider with servers on the east coast, and renting out processor time
>> to a client on the west coast that wants their guest tied to west coast
>> localtime.  And that's assuming that both host and guest switch in and
>> out of daylight savings at the same time, which falls apart when you
>> cross political boundaries.  Then there's the fun of migration (what if
>> my server farm is spread across multiple timezones - does migration take
>> into account the difference in localtime between source and destination
>> servers).

I'm pretty sure that Daniel's suggestion (which I've implemented in
patch 4) removes all of the problems *that don't involve migration or
save/restore* to the extent that they can be removed (and they *do* set
the adjustment/basis in the status such that if they *were* preserved
across migrate or save/restore, behavior would then be correct) although
there is still a problem if you don't know which timezone the initial
host will be in, but want your guest to have RTC set to the localtime of
a particular timezone - I suppose for that we would need to expand
"basis='utc|localtime'" to allow specifying a particular timezone, then
learn the time of that timezone when qemu is started. That's beyond the
scope of this "bugfix" patch series though.


>>
>> I can _totally_ understand the desire to run a GUEST in such a way that
>> the guest thinks it has a bios stored in localtime (and when the guest
>> updates the RTC twice a year to account for daylight savings, it changes
>> what offset we track about the guest).  But I think it is INSANITY to
>> ever try and run a host on a localtime system (daylight savings changes
>> in the host are just asking for problems to the guests) - so even if the
>> host is tied to localtime bios, it is still probably wiser for qemu to
>> base its offsets to UTC no matter what.  If the commandline allows a
>> specification of a localtime offset, I think it should be used ONLY for
>> a one-time up-front conversion into a corresponding UTC offset, and then
>> execute qemu in relation to utc thereafter (therefore, migration is
>> always done in terms of utc, without regards for whether source and
>> destination have a different localtime).

Yep :-)

> Guest side:
> * Emulated RTC CMOS clock is initialized to either UTC or localtime when the 
> guest initializes.
> * Guest reads RTC CMOS clock on boot, or if explicitly requested to
> during runtime, and transfers that value to its system time.
>
> * Migration maintains the emulated RTC CMOS clock value, but a
> subsequent VM restart will use the destination hosts localtime,
> in case -rtc base=localtime.
>
> So using 
>
> -rtc base= "localtime_r(time())" (this is a date) on VM creation
>
> and then maintaining that base, along with guest RTC writes, across VM
> restarts, seems much saner than ever using -rtc base=localtime on a cluster
> with different timezones.
>
>

Again, yep :-)

On 05/23/2014 04:19 AM, Laine Stump wrote:

On 05/23/2014 12:17 PM, Laine Stump wrote:

...

*However*, this discussion forced me to investigate some of the basic assumptions that I'd been making when coming in to fix this bug. In particular, my assumption was that the value of "adjustment" that was set in the status would be preserved across a domain save/restore operation, or a migration, but after talking to jdenemar and looking at the code, I believe that this is *not* the case.

Okay, disregard this "sky is falling" outburst. I was misreading the code and misinterpreting what jdenemar told me. The updated value of adjustment and basis *are* properly preserved across save/restore and migrate.

Okay, now to rephrase things to see if I understand correctly, or maybe add more confusion to the discussion. If I understand it, the qemu event is always outputting the 'current adjustment to the command-line offset', and not 'the delta applied to the most recent RTC change'; while libvirt is trying to report 'the current delta that the guest is using in relation to UTC'. If the command line was specified with UTC as the basis (that is, 0 command-line offset), then the qemu offset happens to also be the libvirt desired offset from UTC. If the command line was specified with any other offset, then the offset from UTC is always the sum of the command line initial offset + the current offset reported by the event, and libvirt is not altering the initial offset while the guest runs. And I don't think libvirt has a way for management to query the current offset (libvirt was only tracking the current offset of running domains in its private xml). For a guest that uses localtime bios, you would start the guest initially with a non-zero command-line offset (representing the offset of the timezone the guest thinks it is running in). Twice a year, the guest will change its RTC by an hour, and the GOAL is that if we stop the guest and then restart it later, the restarted guest will resume with bios at the same offset from UTC as what it last had (that is, the freshly-booted guest will behave as if bios has silently advanced by the amount of time that the guest was not running, similar to how bare-metal hardware has a battery powering the RTC even when the box is completely off and unplugged). If, during the downtime, the guest has crossed a daylight savings boundary, we don't care - the guest OS upon boot will recognize that it is using localtime bios, and that it missed a dst boundary while the machine was powered off, and so it will presumably do an RTC update by an hour fairly early in its boot process - which will be caught as an RTC update event so we once again know the new offset to be applied the next time we boot the guest. For this to work, a persistent guest definition needs to record the offset that was in use at the time the guest powered off, and the next time qemu is started, pass _that_ offset as the command-line offset against UTC. In libvirt's case, we intentionally run qemu to stay alive even after the guest quits, and send an event that the guest is no longer running; this event could be our trigger to read the qemu offset and adjust the persistent state to track the new offset to use on the next boot of the guest. More interesting is migration (whether by saving to a file or by migration between hosts). Libvirt is able to query the current qemu offset prior to starting the migration, but what happens if the guest changes the RTC on the source after the time that libvirt did the query but before the guest is paused in preparation for the destination to start running the guest? Is the RTC offset transmitted as part of the migration stream, even if it is a different offset at the time the migration finally converges than what it was when the migration started? Furthermore, what command line offset should libvirt tell the destination machine to use, since qemu is only tracking the RTC offset relative to the command line, and not the offset relative to UTC? Is there a race here? More concretely, suppose I start a guest with a command-line offset of 3 hours. Then the guest does an RTC change to 4 hours (an offset of one hour from the command line). Then I want to do a migration - do I tell the destination qemu to start with a 3 hour offset (identical to what the source had) and the migration stream corrects it so that the destination picks up with RTC already at 4 hours (and the guest sees no discontinuity)? Or does libvirt have to query the current offset at the time it gets ready to start the migration, and start the destination with a command-line offset of 4 hours, because the offset is not migrated? If the latter, what happens if the guest changes RTC in between when libvirt queries the source for the value to give to the destination, vs. actually starting the migration? It sounds like the only sane way for migration to work is for the RTC offset to be part of the stream, and that the destination must be started with the SAME command line as the source; and therefore, the only time the command line should be altered is when the guest is shut down (not for live migration) - where libvirt must update the domain XML to track the offset in use at the time the guest shuts down. Meanwhile, I know we also recently added the qemu-guest-agent command to set time, with two modes: 1. tell the guest what UTC time it should be using, and update RTC to match (which will trigger an RTC change event; and perhaps can be used to snoop whether the guest is setting RTC to a localtime rather than UTC time); 2. tell the guest to re-read the RTC and adjust its local time to match (which pre-supposes that RTC is accurate). The idea is that after management does something where the guest has a long downtime (either migration to file, or a long suspend), the guest's internal notion of time did not advance (because the CPU was not running) while the RTC did advance (because qemu is still exposing RTC relative to the host's UTC), so the management will tell the guest to resync time as part of resuming. Now, what good does the RTC change event do, if libvirt really only needs to query the current RTC offset at the time shuts down? Given that the guest-agent command deals in UTC (when setting time), does the management app need to know when the guest changes RTC due to daylight savings? Or is the case where management tells guest to reset its own time by re-reading RTC matter for management to know whether that RTC is not tracking UTC? Also, it seems like most people want their guests to run with a proper notion of current time, even over gaps of time where the guest is not running (true if the guest is connected to a network and expects to do anything where being in sync with other computers is important). But is there ever someone that wants a guest to run as though it were seeing all possible time values, and thus where every time the guest is paused and then resumed, its offset from UTC is increased by the amount of downtime that elapsed (most likely, for a standalone guest with no emulated network connections)? Do we need to do anything different for a management app trying to run a guest in such a mode? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 05/23/2014 08:54 AM, Eric Blake wrote:

For this to work, a persistent guest definition needs to record the offset that was in use at the time the guest powered off, and the next time qemu is started, pass _that_ offset as the command-line offset against UTC. In libvirt's case, we intentionally run qemu to stay alive even after the guest quits, and send an event that the guest is no longer running; this event could be our trigger to read the qemu offset and adjust the persistent state to track the new offset to use on the next boot of the guest.

Hmm, I wonder if we have a libvirt design dilemma on our hands. Remember, ever since qemu exposed the -no-shutdown flag, we have been using it in libvirt. It says that even though the guest has quit executing, the qemu process must stick around until we explicitly let go of it. This is because the guest can shut down during a libvirtd restart, so the new libvirtd can still connect to the qemu process, learn that the guest has shut down, and take appropriate actions (query qemu for other details like the final state of the RTC offset, tidy up any transient XML, generate the libvirt event to management app, etc). As long as the libvirt guest is persistent, the management app can still query about the guest even after it shuts down. But VDSM uses only transient guests. Similar to how libvirt uses -no-shutdown to keep qemu hanging around until libvirt acknowledges that the guest is done, what recourse does VDSM have to keep a transient guest's virDomainPtr around (even across libvirtd restarts) until VDSM has had time to query the RTC offset from libvirt? That is, since the only libvirt use of RTC offset is to adjust the persistent definition for the next boot, but VDSM is using only transient guests, VDSM needs a way to track the RTC offset to tell libvirt to use the next time VDSM creates a new transient guest for the same disk image. Or put another way, the moment that libvirt offloads persistence to a higher level application, that higher level application needs a way to grab all information that libvirt would have grabbed for a persistent guest, which means a transient guest needs to exist in the shutoff state for at least as long as VDSM hasn't closed it. Do we have a design flaw in that currently libvirt gets rid of transient guests as soon as they shutdown, rather than waiting for management to close their last reference to the shutdown domain? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, 23 May 2014 00:50:38 -0300 Marcelo Tosatti <mtosatti@redhat.com> wrote:

...

Then the guest triggers an RTC update, so qemu sends an event, but the event is lost. Then libvirtd starts again, and doesn't realize the event is lost.

Yes, but that case is also true for any other QMP asynchronous event, and therefore should be handled generically i suppose (QMP channel data should be maintained across libvirtd shutdown). Luiz?

Maintaining QMP channel data doesn't solve this problem, because all sorts of race conditions are still possible. For example, libvirt could crash after having received the event but before handling it. The most reliable way we found to solve this problem, and that's what we do for other events, is to allow libvirt to query the information the event is reporting. An event is nothing more than a state change in QEMU, and QEMU state is persistent during the life time of the VM, so we allow libvirt to query the state of anything that may send an event.

On Fri, May 23, 2014 at 03:35:19PM +0200, Markus Armbruster wrote:

Luiz Capitulino <lcapitulino@redhat.com> writes:

...

On Fri, 23 May 2014 00:50:38 -0300 Marcelo Tosatti <mtosatti@redhat.com> wrote:

...

...

Then the guest triggers an RTC update, so qemu sends an event, but the event is lost. Then libvirtd starts again, and doesn't realize the event is lost.

Yes, but that case is also true for any other QMP asynchronous event, and therefore should be handled generically i suppose (QMP channel data should be maintained across libvirtd shutdown). Luiz?

Maintaining QMP channel data doesn't solve this problem, because all sorts of race conditions are still possible. For example, libvirt could crash after having received the event but before handling it.

The most reliable way we found to solve this problem, and that's what we do for other events, is to allow libvirt to query the information the event is reporting. An event is nothing more than a state change in QEMU, and QEMU state is persistent during the life time of the VM, so we allow libvirt to query the state of anything that may send an event.

In fact, this is a general rule: when libvirt tracks an event, it also needs a way to poll for the information in the event.

I see. This also seems pretty harmful wrt losing events: /* Global, one-time initializer to configure the rate limiting * and initialize state */ static void monitor_protocol_event_init(void) { /* Limit RTC & BALLOON events to 1 per second */ monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000); Better remove it.

On Fri, 23 May 2014 10:48:18 -0300 Marcelo Tosatti <mtosatti@redhat.com> wrote:

On Fri, May 23, 2014 at 03:35:19PM +0200, Markus Armbruster wrote:

...

Luiz Capitulino <lcapitulino@redhat.com> writes:

...

On Fri, 23 May 2014 00:50:38 -0300 Marcelo Tosatti <mtosatti@redhat.com> wrote:

...

...

Then the guest triggers an RTC update, so qemu sends an event, but the event is lost. Then libvirtd starts again, and doesn't realize the event is lost.

Yes, but that case is also true for any other QMP asynchronous event, and therefore should be handled generically i suppose (QMP channel data should be maintained across libvirtd shutdown). Luiz?

Maintaining QMP channel data doesn't solve this problem, because all sorts of race conditions are still possible. For example, libvirt could crash after having received the event but before handling it.

The most reliable way we found to solve this problem, and that's what we do for other events, is to allow libvirt to query the information the event is reporting. An event is nothing more than a state change in QEMU, and QEMU state is persistent during the life time of the VM, so we allow libvirt to query the state of anything that may send an event.

In fact, this is a general rule: when libvirt tracks an event, it also needs a way to poll for the information in the event.

I see.

This also seems pretty harmful wrt losing events:

/* Global, one-time initializer to configure the rate limiting * and initialize state */ static void monitor_protocol_event_init(void) { /* Limit RTC & BALLOON events to 1 per second */ monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);

Better remove it.

You mean, this is causing problems to the RTC_CHANGE event or you found a general problem? Can you give more details?

On 05/23/2014 07:48 AM, Marcelo Tosatti wrote:

This also seems pretty harmful wrt losing events:

/* Global, one-time initializer to configure the rate limiting * and initialize state */ static void monitor_protocol_event_init(void) { /* Limit RTC & BALLOON events to 1 per second */ monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);

Better remove it.

No, this throttling MUST be present to avoid a guest-triggered denial-of-service attack (otherwise the guest could trigger RTC change events fast enough to starve the host in dealing with them). Again, lost events are expected. What is important is that when throttling, and event is guaranteed to be sent at the end of the throttling period so that libvirt will always eventually get an event with the latest state (even if intermediate events were lost) with no more latency than the throttling period; and either the event caries the current state (and not something that needs accumulation), or the event is a witness that libvirt needs to do an additional query- command to learn the current state. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, May 23, 2014 at 11:36:56PM +0200, Paolo Bonzini wrote:

You can get the current time via the qom-get command, and then follow the same algorithm as QEMU:

time_t seconds;

if (rtc_date_offset == -1) { if (rtc_utc) { seconds = mktimegm(tm); } else { struct tm tmp = *tm; tmp.tm_isdst = -1; /* use timezone to figure it out */ seconds = mktime(&tmp); } } else { seconds = mktimegm(tm) + rtc_date_offset; } return seconds - time(NULL);

Unfortunately the QOM path to the RTC device is not stable. We can add a /machine/rtc link, and if the PPC guys implement the link and current-time property as well, the same mechanism can work for any board.

Paolo

I like that idea. Questions: - What guarantees are there that the interface is stable? (you can argue that "now it is"). - Perhaps add /machine/stable/ path to contain stable links ? - How do i go about adding a link to a device again?

On 05/23/2014 01:36 PM, Eric Blake wrote:

...

This patch accomplishes that by storing the initial adjustment in the domain's status as "adjustment0". Each time a new RTC_CHANGE event is received from qemu, we simply add adjustment0 to the value sent by qemu, store that as the new adjustment, and forward that value on to any event handler.

Okay, I'm convinced that this guarantees that the right value is exposed in the event handler, so this patch is a strict improvement.

ACK.

ACK still holds on the grounds of strict improvement, even if we end up changing things further; although given my naming suggestions below you may still want to do a v3. More thoughts on the matter: We NEED to store adjustment0 in XML: both persistent xml (to control what the next boot uses) and in the runtime xml (so that when converting RTC change events from qemu into libvirt events, we have the right conversion). But if you look, we ALREADY store adjustment0 in the xml: <clock offset='variable' adjustment='10962' basis='utc'/> The adjustment attribute IS adjustment0, when basis is utc (and patch 4/4 makes it so that any other basis is immediately converted to utc at the first time we start qemu). Meanwhile, I argued that 'adjustment' is a live-only attribute. You could argue that if qemu gave us a way to learn the current adjustment via a query command, rather than having to listen for the most recent RTC change event, we would not even need to store 'adjustment' in the live XML (the only two times we would use 'adjustment' are when converting an RTC event [if we got the event, we don't need anything from storage; if we missed the event, no one will ever know] and when writing the final state of adjustment into the persistent adjustment0 when the live domain shuts down [but we'd use the new qemu query command for that]). On the other hand, if qemu doesn't have the new query command, then storing 'adjustment' in the live xml proves to be a nice fallback so that guest shutdown still lets us write the correct value to the persistent xml in all cases where we didn't miss an event. Furthermore, I think it should be user-visible rather than hidden, so that VDSM can learn the current adjustment in use by the guest (whether or not we also solve the problem of letting VDSM do post-mortem queries after a transient domain shuts down). So I'm wondering if: <clock offset='variable' adjustment='109062' delta='3600' basis='utc'/> makes sense for live xml - that is, the XML 'adjustment' attribute matches what you called 'adjustment0' and appears in both live and persistent, and the XML 'delta' attribute matches what you called 'adjustment' and which exists only for a running domain (the above domain was started with a command-line offset of 109062, and the current RTC offset in the guest is 3600 whether it was done via one or a series of RTC change events; the next time VDSM creates a new transient guest for the same resources, it should do so with adjustment='112662'). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org