Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
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 mpriv...@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. It's not like if on input someone had down for the state that we'd attempt to set the link down... or if the speed on input was 500, but the file had 1000 - there's no changing of the file. John Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
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 mpriv...@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. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
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 mpriv...@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 :-/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
On Wed, Jun 11, 2014 at 01:04:02PM +0300, Laine Stump wrote: 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 mpriv...@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 :-/ Yes, you're right actually. I should have clarified I meant inactive XML. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
On 06/11/2014 05:19 AM, 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 mpriv...@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. So - I was just too lazy/tired to look last night, but formatdomain.html.in has an Network interfaces sub-section... Perhaps woefully under described though... John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
On 06/11/2014 01:11 PM, Daniel P. Berrange wrote: On Wed, Jun 11, 2014 at 01:04:02PM +0300, Laine Stump wrote: 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 :-/ Yes, you're right actually. I should have clarified I meant inactive XML. Whew! I feel much better now :-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
On Wed, Jun 11, 2014 at 06:46:25AM -0400, John Ferlan wrote: On 06/11/2014 05:19 AM, 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 mpriv...@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. So - I was just too lazy/tired to look last night, but formatdomain.html.in has an Network interfaces sub-section... Perhaps woefully under described though... NB the network interfaces schema for virDomain (as documented in the formatdomain.html page) is different from the network interfaces schema for virInterface (as not documented :-) Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
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 mpriv...@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 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. It's not like if on input someone had down for the state that we'd attempt to set the link down... or if the speed on input was 500, but the file had 1000 - there's no changing of the file. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/4] virInterface: Expose link state speed
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 mpriv...@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(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 34ef613..5fe3a97 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -397,4 +397,29 @@ /optional /define + define name=link-speed-state +optional + element name=link +optional + attribute name=speed +ref name=unsignedInt/ + /attribute +/optional +optional + attribute name=state +choice + valueunknown/value + valuenotpresent/value + valuedown/value + valuelowerlayerdown/value + valuetesting/value + valuedormant/value + valueup/value +/choice + /attribute +/optional + /element +/optional + /define + /grammar diff --git a/docs/schemas/interface.rng b/docs/schemas/interface.rng index 3984b63..8e2218d 100644 --- a/docs/schemas/interface.rng +++ b/docs/schemas/interface.rng @@ -41,6 +41,7 @@ attribute name=addressref name=macAddr//attribute /element /optional +ref name=link-speed-state/ !-- FIXME: Allow (some) ethtool options -- /define diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index 317fdf2..6412d24 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -38,6 +38,13 @@ VIR_ENUM_IMPL(virDeviceAddressPCIMulti, on, off) +VIR_ENUM_IMPL(virInterfaceState, + VIR_INTERFACE_STATE_LAST, + /* value of zero means no state */, + unknown, notpresent, + down, lowerlayerdown, + testing, dormant, up) + int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) { /* PCI bus has 32 slots and 8 functions per slot */ @@ -142,3 +149,58 @@ virDevicePCIAddressEqual(virDevicePCIAddress *addr1, } return false; } + +int +virInterfaceLinkParseXML(xmlNodePtr node, + virInterfaceLinkPtr lnk) +{ +int ret = -1; +char *stateStr, *speedStr; +int state; + +stateStr = virXMLPropString(node, state); +speedStr = virXMLPropString(node, speed); + +if (stateStr) { +if ((state = virInterfaceStateTypeFromString(stateStr)) 0) { +virReportError(VIR_ERR_XML_ERROR, + _(unknown link state: %s), + stateStr); +goto cleanup; +} +lnk-state = state; +} + +if (speedStr +virStrToLong_ui(speedStr, NULL, 10, lnk-speed) 0) { +virReportError(VIR_ERR_XML_ERROR, + _(Unable to parse link speed: %s), + speedStr); +goto cleanup; +} + +ret = 0; + cleanup: +VIR_FREE(stateStr); +VIR_FREE(speedStr); +return ret; +} + +int +virInterfaceLinkFormat(virBufferPtr buf, + const virInterfaceLink *lnk) +{ +if (!lnk-speed !lnk-state) { +/* If there's nothing to format, return early. */ +return 0; +} + +virBufferAddLit(buf, link); +if (lnk-speed) +virBufferAsprintf(buf, speed='%u', lnk-speed); +if (lnk-state) +virBufferAsprintf(buf, state='%s', + virInterfaceStateTypeToString(lnk-state)); +virBufferAddLit(buf, /\n); +return 0; +} diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index d66afd2..be63ba7 100644 --- a/src/conf/device_conf.h +++