[libvirt] [FOR 1.3.0 PATCH] conf: add net device prefix for Xen
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure. This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- This is an alternative approach to Joao's fix for this regression https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes. src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 4 ++++ src/libxl/libxl_domain.c | 5 +++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f5c0ed..debcf4e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8428,7 +8428,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, ifname = virXMLPropString(cur, "dev"); if (ifname && (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && - STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX)) { + (STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) || + STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX_XEN))) { /* An auto-generated target name, blank it out */ VIR_FREE(ifname); } @@ -19790,7 +19791,8 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); if (def->ifname && !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) { + (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX) || + STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX_XEN)))) { /* Skip auto-generated target names for inactive config. */ virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 90d8e13..d2cc26f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1085,6 +1085,10 @@ struct _virDomainNetDef { * by libvirt, and cannot be used for a persistent network name. */ # define VIR_NET_GENERATED_PREFIX "vnet" +/* Used for prefix of ifname of any network name generated dynamically + * by libvirt for Xen, and cannot be used for a persistent network name. */ +# define VIR_NET_GENERATED_PREFIX_XEN "vif" + typedef enum { VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT = 0, VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index ef92974..c5d84a4 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -734,7 +734,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - if (STRPREFIX(net->ifname, "vif")) + if (STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX_XEN)) VIR_FREE(net->ifname); } } @@ -918,7 +918,8 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) if (net->ifname) continue; - ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s", + ignore_value(virAsprintf(&net->ifname, + VIR_NET_GENERATED_PREFIX_XEN "%d.%d%s", def->id, x_nic->devid, suffix)); } } -- 2.6.1
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure.
This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is an alternative approach to Joao's fix for this regression
https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes.
Hmm, yes, tricky one. If we only care about XML parsing, then you could register a post parse callback instead to do this. I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback. The other option would be to make the reserved prefix be a capability that the parser/formatter could read.
src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 4 ++++ src/libxl/libxl_domain.c | 5 +++-- 3 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f5c0ed..debcf4e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8428,7 +8428,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, ifname = virXMLPropString(cur, "dev"); if (ifname && (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && - STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX)) { + (STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) || + STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX_XEN))) { /* An auto-generated target name, blank it out */ VIR_FREE(ifname); } @@ -19790,7 +19791,8 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); if (def->ifname && !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) { + (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX) || + STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX_XEN)))) { /* Skip auto-generated target names for inactive config. */ virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 90d8e13..d2cc26f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1085,6 +1085,10 @@ struct _virDomainNetDef { * by libvirt, and cannot be used for a persistent network name. */ # define VIR_NET_GENERATED_PREFIX "vnet"
+/* Used for prefix of ifname of any network name generated dynamically + * by libvirt for Xen, and cannot be used for a persistent network name. */ +# define VIR_NET_GENERATED_PREFIX_XEN "vif" + typedef enum { VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT = 0, VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index ef92974..c5d84a4 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -734,7 +734,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i];
- if (STRPREFIX(net->ifname, "vif")) + if (STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX_XEN)) VIR_FREE(net->ifname); } } @@ -918,7 +918,8 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) if (net->ifname) continue;
- ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s", + ignore_value(virAsprintf(&net->ifname, + VIR_NET_GENERATED_PREFIX_XEN "%d.%d%s", def->id, x_nic->devid, suffix)); } } -- 2.6.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure.
This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is an alternative approach to Joao's fix for this regression
https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes. Hmm, yes, tricky one.
If we only care about XML parsing, then you could register a post parse callback instead to do this.
AFAIK, XML parsing is all that's in play here.
I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback.
The other option would be to make the reserved prefix be a capability that the parser/formatter could read.
This seems like the best option, since a post-parse callback doesn't solve the problem in virDomainNetDefFormat. It also has the upshot of making the prefix visible and known to users. But I doubt such a change is suitable during 1.3.0 freeze. With the freeze in mind, seems the best solution to the libxl migration regression is revert d2e5538b1. It can be added again post-1.3.0 release, after adding the prefix to capabilities. DV, since you may be making the release soon, feel free to revert d2e5538b1 if you agree. Regards, Jim
On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure.
This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is an alternative approach to Joao's fix for this regression
https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes. Hmm, yes, tricky one.
If we only care about XML parsing, then you could register a post parse callback instead to do this.
AFAIK, XML parsing is all that's in play here.
I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback.
The other option would be to make the reserved prefix be a capability that the parser/formatter could read.
This seems like the best option, since a post-parse callback doesn't solve the problem in virDomainNetDefFormat. It also has the upshot of making the prefix visible and known to users. But I doubt such a change is suitable during 1.3.0 freeze. With the freeze in mind, seems the best solution to the libxl migration regression is revert d2e5538b1. It can be added again post-1.3.0 release, after adding the prefix to capabilities.
DV, since you may be making the release soon, feel free to revert d2e5538b1 if you agree.
Yeah, just go ahead & revert it Jim, DV isn't doing the releae until tomorrow morning 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 :|
Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure.
This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is an alternative approach to Joao's fix for this regression
https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes. Hmm, yes, tricky one.
If we only care about XML parsing, then you could register a post parse callback instead to do this. AFAIK, XML parsing is all that's in play here.
I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback.
The other option would be to make the reserved prefix be a capability that the parser/formatter could read. This seems like the best option, since a post-parse callback doesn't solve the problem in virDomainNetDefFormat. It also has the upshot of making the prefix visible and known to users. But I doubt such a change is suitable during 1.3.0 freeze. With the freeze in mind, seems the best solution to the libxl migration regression is revert d2e5538b1. It can be added again post-1.3.0 release, after adding the prefix to capabilities.
DV, since you may be making the release soon, feel free to revert d2e5538b1 if you agree.
Yeah, just go ahead & revert it Jim, DV isn't doing the releae until tomorrow morning
I've pushed the revert. Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after exposing the prefix in capabilities. Regards, Jim
On 12/08/2015 04:06 PM, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure.
This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is an alternative approach to Joao's fix for this regression
https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes. Hmm, yes, tricky one.
If we only care about XML parsing, then you could register a post parse callback instead to do this. AFAIK, XML parsing is all that's in play here.
I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback.
The other option would be to make the reserved prefix be a capability that the parser/formatter could read. This seems like the best option, since a post-parse callback doesn't solve the problem in virDomainNetDefFormat. It also has the upshot of making the prefix visible and known to users. But I doubt such a change is suitable during 1.3.0 freeze. With the freeze in mind, seems the best solution to the libxl migration regression is revert d2e5538b1. It can be added again post-1.3.0 release, after adding the prefix to capabilities.
DV, since you may be making the release soon, feel free to revert d2e5538b1 if you agree.
Yeah, just go ahead & revert it Jim, DV isn't doing the releae until tomorrow morning
I've pushed the revert.
Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after exposing the prefix in capabilities.
Hey Jim, I had the impression we had pushed this back in (i.e. cherry-picking d2e5538) but I was double checking and that's doesn't seem to be case. In adding support for the prefix in capabilities I found out one issue on the migration failure path leading to dereferencing a NULL pointer on the destination. If you agree could we squash the following chunk in addition to the cherry-pick of d2e5538: diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 63c5b24..f73bfb3 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -768,7 +768,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - if (STRPREFIX(net->ifname, "vif")) + if (net->ifname && STRPREFIX(net->ifname, "vif")) VIR_FREE(net->ifname); } } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3f4457f..5479441 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5590,7 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = { .domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */ - .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */ + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */ .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */ .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */ Or do you think it should be deferred to 1.3.3 ? Thanks, Joao
Regards, Jim
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/22/2016 11:15 PM, Joao Martins wrote:
On 12/08/2015 04:06 PM, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure.
This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is an alternative approach to Joao's fix for this regression
https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes. Hmm, yes, tricky one.
If we only care about XML parsing, then you could register a post parse callback instead to do this. AFAIK, XML parsing is all that's in play here.
I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback.
The other option would be to make the reserved prefix be a capability that the parser/formatter could read. This seems like the best option, since a post-parse callback doesn't solve the problem in virDomainNetDefFormat. It also has the upshot of making the prefix visible and known to users. But I doubt such a change is suitable during 1.3.0 freeze. With the freeze in mind, seems the best solution to the libxl migration regression is revert d2e5538b1. It can be added again post-1.3.0 release, after adding the prefix to capabilities.
DV, since you may be making the release soon, feel free to revert d2e5538b1 if you agree.
Yeah, just go ahead & revert it Jim, DV isn't doing the releae until tomorrow morning
I've pushed the revert.
Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after exposing the prefix in capabilities.
Hey Jim,
I had the impression we had pushed this back in (i.e. cherry-picking d2e5538) but I was double checking and that's doesn't seem to be case. In adding support for the prefix in capabilities I found out one issue on the migration failure path leading to dereferencing a NULL pointer on the destination. If you agree could we squash the following chunk in addition to the cherry-pick of d2e5538:
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 63c5b24..f73bfb3 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -768,7 +768,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i];
- if (STRPREFIX(net->ifname, "vif")) + if (net->ifname && STRPREFIX(net->ifname, "vif")) VIR_FREE(net->ifname); } } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3f4457f..5479441 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5590,7 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = { .domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */ - .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */ + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */ .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */ .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
Or do you think it should be deferred to 1.3.3 ?
In case this is considered, please ignore the above chunk in my previous message. The one below should be better since I introduced LIBXL_GENERATED_PREFIX_XEN on a040ba9 which means no need to hardcode "vif". Regards, Joao diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 63c5b24..8313c6e 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -768,7 +768,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - if (STRPREFIX(net->ifname, "vif")) + if (net->ifname && + STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) VIR_FREE(net->ifname); } } @@ -960,7 +961,8 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) if (net->ifname) continue; - ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s", + ignore_value(virAsprintf(&net->ifname, + LIBXL_GENERATED_PREFIX_XEN "%d.%d%s", def->id, x_nic->devid, suffix)); } } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3f4457f..5479441 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5590,7 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = { .domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */ - .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */ + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */ .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */ .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
Regards, Jim
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/22/2016 04:15 PM, Joao Martins wrote:
On 12/08/2015 04:06 PM, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
In commit d2e5538b1, the libxl driver was changed to copy interface names autogenerated by libxl to the corresponding network def in the domain's virDomainDef object. The copied name is freed when the domain transitions to the shutoff state. But when migrating a domain, the autogenerated name is included in the XML sent to the destination host. It is possible an interface with the same name already exists on the destination host, causing migration to fail. Indeed the Xen project's OSSTEST CI already encountered such a failure.
This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing the autogenerated names to be excluded when parsing and formatting inactive config.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is an alternative approach to Joao's fix for this regression
https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
I think it is the same approach used by the qemu driver. My only reservation is that it expands the potential for clashes with user-defined names. I.e. with this change both 'vnet' and 'vif' are reserved prefixes. Hmm, yes, tricky one.
If we only care about XML parsing, then you could register a post parse callback instead to do this. AFAIK, XML parsing is all that's in play here.
I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback.
The other option would be to make the reserved prefix be a capability that the parser/formatter could read. This seems like the best option, since a post-parse callback doesn't solve the problem in virDomainNetDefFormat. It also has the upshot of making the prefix visible and known to users. But I doubt such a change is suitable during 1.3.0 freeze. With the freeze in mind, seems the best solution to the libxl migration regression is revert d2e5538b1. It can be added again post-1.3.0 release, after adding the prefix to capabilities.
DV, since you may be making the release soon, feel free to revert d2e5538b1 if you agree. Yeah, just go ahead & revert it Jim, DV isn't doing the releae until tomorrow morning I've pushed the revert.
Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after exposing the prefix in capabilities.
Hey Jim,
I had the impression we had pushed this back in (i.e. cherry-picking d2e5538) but I was double checking and that's doesn't seem to be case. In adding support for the prefix in capabilities I found out one issue on the migration failure path leading to dereferencing a NULL pointer on the destination. If you agree could we squash the following chunk in addition to the cherry-pick of d2e5538:
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 63c5b24..f73bfb3 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -768,7 +768,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i];
- if (STRPREFIX(net->ifname, "vif")) + if (net->ifname && STRPREFIX(net->ifname, "vif")) VIR_FREE(net->ifname); } } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3f4457f..5479441 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5590,7 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = { .domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */ - .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */ + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */ .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */ .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
Or do you think it should be deferred to 1.3.3 ?
I wanted to ask you about this on the recent Xen+libvirt+OpenStack sync. Thanks for checking. It would be a shame if this patch didn't make 1.3.2 since all of your prerequisite work is in, but perhaps a new patch is best given the changes? Regards, Jim
On 02/24/2016 04:31 AM, Jim Fehlig wrote:
On 02/22/2016 04:15 PM, Joao Martins wrote:
On 12/08/2015 04:06 PM, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote: > In commit d2e5538b1, the libxl driver was changed to copy interface > names autogenerated by libxl to the corresponding network def in the > domain's virDomainDef object. The copied name is freed when the domain > transitions to the shutoff state. But when migrating a domain, the > autogenerated name is included in the XML sent to the destination host. > It is possible an interface with the same name already exists on the > destination host, causing migration to fail. Indeed the Xen project's > OSSTEST CI already encountered such a failure. > > This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing > the autogenerated names to be excluded when parsing and formatting > inactive config. > > Signed-off-by: Jim Fehlig <jfehlig@suse.com> > --- > > This is an alternative approach to Joao's fix for this regression > > https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html > > I think it is the same approach used by the qemu driver. My only > reservation is that it expands the potential for clashes with > user-defined names. I.e. with this change both 'vnet' and 'vif' are > reserved prefixes. Hmm, yes, tricky one.
If we only care about XML parsing, then you could register a post parse callback instead to do this. AFAIK, XML parsing is all that's in play here.
I'm not clear why we also have it in the virDomainNetDefFormat method - and we can't solve that with a post-parse callback.
The other option would be to make the reserved prefix be a capability that the parser/formatter could read. This seems like the best option, since a post-parse callback doesn't solve the problem in virDomainNetDefFormat. It also has the upshot of making the prefix visible and known to users. But I doubt such a change is suitable during 1.3.0 freeze. With the freeze in mind, seems the best solution to the libxl migration regression is revert d2e5538b1. It can be added again post-1.3.0 release, after adding the prefix to capabilities.
DV, since you may be making the release soon, feel free to revert d2e5538b1 if you agree. Yeah, just go ahead & revert it Jim, DV isn't doing the releae until tomorrow morning I've pushed the revert.
Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after exposing the prefix in capabilities.
Hey Jim,
I had the impression we had pushed this back in (i.e. cherry-picking d2e5538) but I was double checking and that's doesn't seem to be case. In adding support for the prefix in capabilities I found out one issue on the migration failure path leading to dereferencing a NULL pointer on the destination. If you agree could we squash the following chunk in addition to the cherry-pick of d2e5538:
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 63c5b24..f73bfb3 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -768,7 +768,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i];
- if (STRPREFIX(net->ifname, "vif")) + if (net->ifname && STRPREFIX(net->ifname, "vif")) VIR_FREE(net->ifname); } } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3f4457f..5479441 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5590,7 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = { .domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */ .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */ .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */ - .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */ + .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */ .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */ .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 0.9.0 */ .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
Or do you think it should be deferred to 1.3.3 ?
I wanted to ask you about this on the recent Xen+libvirt+OpenStack sync. Thanks for checking. It would be a shame if this patch didn't make 1.3.2 since all of your prerequisite work is in, but perhaps a new patch is best given the changes?
Yeap, I just sent a new patch with these changes. Thanks! Joao
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Daniel P. Berrange -
Jim Fehlig -
Joao Martins