[libvirt] [PATCH v2 1/4] virInterface: Expose link state & speed

[Laine Stump]

On 06/11/2014 12:28 PM, Daniel P. Berrange wrote:
> On Wed, Jun 11, 2014 at 11:19:14AM +0200, Michal Privoznik wrote:
>> On 11.06.2014 03:13, John Ferlan wrote:
>>>
>>> On 06/05/2014 11:39 AM, Michal Privoznik wrote:
>>>> Currently it is not possible to determine the speed of an interface
>>>> and whether a link is actually detected from the API. Orchestrating
>>>> platforms want to be able to determine when the link has failed and
>>>> where multiple speeds may be available which one the interface is
>>>> actually connected at. This commit introduces an extension to our
>>>> interface XML (without implementation to interface driver backends):
>>>>
>>>>   <interface type='ethernet' name='eth0'>
>>>>     <start mode='none'/>
>>>>     <mac address='aa:bb:cc:dd:ee:ff'/>
>>>>     <link speed='1000' state='up'/>
>>>>     <mtu size='1492'/>
>>>>     ...
>>>>   </interface>
>>>>
>>>> Where @speed is negotiated link speed in Mbits per second, and state
>>>> is the current NIC state (can be one of the following:  "unknown",
>>>> "notpresent", "down", "lowerlayerdown","testing", "dormant", "up").
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>>>  docs/schemas/basictypes.rng                     | 25 ++++++++++
>>>>  docs/schemas/interface.rng                      |  1 +
>>>>  src/conf/device_conf.c                          | 62 +++++++++++++++++++++++++
>>>>  src/conf/device_conf.h                          | 27 ++++++++++-
>>>>  src/conf/interface_conf.c                       | 11 ++++-
>>>>  src/conf/interface_conf.h                       |  2 +
>>>>  src/libvirt_private.syms                        |  4 ++
>>>>  tests/interfaceschemadata/bridge-no-address.xml |  1 +
>>>>  tests/interfaceschemadata/bridge.xml            |  1 +
>>>>  tests/interfaceschemadata/ethernet-dhcp.xml     |  1 +
>>>>  10 files changed, 133 insertions(+), 2 deletions(-)
>>>>
>>> Already been ACK'd, but I will point out the interface.html.in hasn't
>>> been updated - later you update the nodedev.html.in file - so probably
>>> reasonable to do so again. While at it the consistency of using "Mbits"
>>> vs. "Mb" in the comment in device_conf.h
>> The first issue has simple explanation - there's no interface.html.in yet.
>> Yes we lack the virInterface XML documentation. The second one I'm fixing
>> prior to push.
>>
>>> Just so I understand - the reality is regardless of what the XML is on
>>> input - the code will still check/get/set the link state/speed - so this
>>> is mostly an output thing.
>> Yes. So far this is only for getting the link speed & state. Which brings up
>> an interesting question. With recent issue with QoS on bridgeless networks
>> on mind - should we make virInterface XML parsing actually reject any
>> <link/> since it's output element only? One could argue that we usually
>> ignore unknown elements, but then <link/> is not unknown element anymore.
>> One way or another, it can be done in a separate patch.
> Apps must always be able to round-trip XML. ie they should be able to
> do DumpXML and then feed that back into DefineXML and have no functional
> change. Thus we must not reject <link/> with an error when parsing, we
> must ignore it or honour it as appropriate for the desired semantics.

I agree that <link> should just be ignored when defining a domain. But
as a general response to that last paragraph, there are cases where
doing a virDomainGetXMLDesc() on an active domain (without the INACTIVE
flag), followed by a virDomainDefine using that output, will *not*
produce the same domain. The example at the front of my mind is what
happens with an <interface type='network'>, as related in my RFC email
yesterday:

  https://www.redhat.com/archives/libvir-list/2014-June/msg00416.html

but a more general problem is that if you run virDomainGetXMLDesc() with
no INACTIVE flag, you will miss any previous edits to the domain
definition since the last time it was started. This had actually been
the cause of a long-running bug with "virsh net-edit" as a matter of fact.

I had thought that what was required was the ability to roundtrip the
*inactive* xml back into a define, not the status XML. If it's really a
requirement that we be able to feed back the output of the status XML to
get the exact same domain, then I think I've made a big mistake :-/