[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 0/3] Xen: Fix <clock> handling



On 02/09/2012 04:36 AM, Philipp Hahn wrote:
> Before version 3.1 xen only implemented clock/@offset='utc' and 'localtime'.
> With the introduction of managed domains in 3.1 xend keeps track of the
> rtc_timeoffset, even over reboots. This translates to  libvirts
> clock/@offset='variable' variant. Be advised that only HV domains have a RTC.
> 
> In addition xen also supports a variant where the offset is tracked to
> 'localtime', which is currently not supported by libvirt. To make matters
> worse, this was somehow broken in some versions of xen and was finally fixed
> with version xen-3.4.
> 
> The following patch set ...
> * adds support for handling variable offsets relative to localtime,
> * fixes libvirt to use clock/@offset='variable' for newer xen versions,
> * adapts the test suit accordingly
> 
> I've tested this on CenOS5 (xend-3.0.3 + 3.1.2 hypervisor?), UCS-2.3
> (xen-3.2.1), UCS-2.4 (xen-3.4.3) and UCS-3.0 (xen-4.1.2).
> 
> Since v1:
> + fix handling of direct-PV-domains
> + added handling of localtime=1 + rtc_timeoffset
> + fixed test suite
> 
> Questions:
> 1. Handling of legacy XML domain configurations
>    I'm reluctant to define LIBVIRT_XEND_CLOCK_STRICT, since this would probably
>    break many existing XML configuration files, since they would be rejected
>    libvirt now for still using clock/@offset='utc'/'localtime'. Luckily for us
>    libvirt doesn't support snapshots with xen, because then this XML
>    configuration would be stored in the snapshot meta data, rendering them all
>    invalid.

We absolutely cannot reject existing older xml files; but must load them
with the semantics that make sense.

If I understand correctly, the situation is that old clients had:

<clock offset='localtime'/>

whereas the semantics are more correctly listed as your proposed:

<clock offset='variable' adjustment='nnn' basis='localtime'/>

and you are worried that if you rewrite the former into the latter, you
are preserving the semantics that make the most sense as a default, but
you are making it impossible to implement alternate semantics that are
also possible with xen and which are documented as the current behavior.
 Your idea was to compile two versions of libvirt, either with or
without -DLIBVIRT_XEND_CLOCK_STRICT.

But a better idea is to represent the XML in a way where the default
conversion makes sense, but where a user can explicitly change the XML
to get what they want.  I'm thinking:

If offset is 'utc' or 'localtime', we add a new attribute 'adjustment',
whose value can be either 'reset' to signify that the offset is reset on
each boot, or a timeDelta, as shorthand for specifying offset='variable'
adjustment='nnn' basis='utc|localtime'.  If the attribute is missing,
the hypervisor determines whether the default is 'reset' or '0'.  Qemu
would pick 'reset', xend would pick '0'.

That way, we would convert domains by default, but a user that _wants_
the documented reset abilities can add adjustment='reset' to their XML
and force us to leave things alone, without having a compilation switch.

When writing XML back out via dumpxml, libvirt would generate either the
offset='utc' adjustment='reset', or the offset='variable'
adjustment='nnn' basis='utc'; and would not generate offset='utc'
adjustment='nnn', even though that is a valid input form.

That will touch a bit more in the testsuite (all qemu tests would also
end up being converted by the new output), but would make things more
specific, and still give the user runtime control over XML behavior
without having to resort to recompilation of libvirt with a new compiler
macro.

RNG-wise, this would be:

      <element name='clock'>
        <choice>
          <group>
            <attribute name='offset'>
              <choice>
                <value>localtime</value>
                <value>utc</value>
              </choice>
            </attribute>
            <optional>
              <attribute name='adjustment'>
                <choice>
                  <ref name='timeDelta'/>
                  <value>reset</value>
                </choice>
              </attribute>
            </optional>
          </group>
          <group>
            <attribute name='offset'>
              <value>timezone</value>
            </attribute>
            <optional>
              <attribute name='timezone'>
                <ref name='timeZone'/>
              </attribute>
            </optional>
          </group>
          <group>
            <attribute name='offset'>
              <value>variable</value>
            </attribute>
            <optional>
              <attribute name='adjustment'>
                <ref name='timeDelta'/>
              </attribute>
            </optional>
          </group>
        </choice>
        <zeroOrMore>
          <ref name='timer'/>
        </zeroOrMore>
      </element>



> 2. Error path handling
>    There are now a lot of different cases (PV, HV<3.1, HV>3.1), which all have
>    the same error path. This is somehow ugly to code in C, I hope it's okay to
>    use vmlocaltime=-1 to indicate an error instead of using "goto". But if you
>    have a better idea, please feel free to comments.

I didn't look closely at that part; I'd rather get consensus on the XML
first.

> 3. libvirt-0.9.10 or .11
>    This patch-series  should probably receive some more testing, so for 0.9.10
>    it's too late. If it's not included, I'd need to update the reference to the
>    version when this patch will get in.

Agreed on this point - we are now at post-0.9.10 material.

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]