On 12/06/2015 06:11 PM, Jim Fehlig wrote:
On 12/06/2015 10:59 AM, Jim Fehlig wrote:
On 12/06/2015 10:04 AM, Jim Fehlig wrote:
On 12/04/2015 12:45 PM, Joao Martins wrote:
Commit d2e5538b1 changes virDomainDef to include ifnames that autogenerated by libxl, and that are also cleared on domain cleanup. One place that's missing is on migration, when domain xml is sent to dst libvirtd and would contain ifnames from the source libvirtd. This would lead to erronous behaviour (as seen in osstest CI) such as failing to migrate when a vif with the same name existed (belonging to another domain) on destination. Your patch is certainly one way to fix this issue, but I wonder if we should be adding the generated ifname to the XML that is sent to the destination. virDomainNetDefFormat() has this interesting piece of logic
if (def->ifname && !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) { /* Skip auto-generated target names for inactive config. */ virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname); }
In the Begin phase, we format the XML that will be sent to the destination based on current vm->def of the machine running on the source. Calling virDomainDefFormat() with VIR_DOMAIN_DEF_FORMAT_INACTIVE doesn't seem right. I tried to see how this is handled in the qemu driver, but couldn't quite figure it out. Jirka is one of the experts of the qemu migration code, adding him to cc to see if he has any insights. I think this is actually handled on the destination when parsing the incoming XML in the Prepare phase. Something like the below patch may be a better solution.
Sorry for spamming with all the self-replies...
From 8556bf5a116edf5989eac40f0a90feed6c003b38 Mon Sep 17 00:00:00 2001 From: Jim Fehlig <jfehlig@suse.com> Date: Sun, 6 Dec 2015 10:46:28 -0700 Subject: [PATCH] conf: add net device prefix for Xen
Commit d2e5538b1 changes virDomainDef to include ifnames autogenerated by libxl. When migrating a domain, the autogenerated name is included in the XML sent to the destination host. This would lead to erronous behaviour (as seen in osstest CI) such as failing to migrate when a vif with the same name existed (belonging to another domain) on destination.
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> --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 4 ++++ 2 files changed, 8 insertions(+), 2 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,
If this is acceptable, the below diff should be squashed in to use the #define instead of hardcoded "vif" in the libxl driver.
Wasn't aware that virDomainNetDefParseXML took care of removing ifnames already in Prepare phase. If so, your solution appears to be a better approach indeed. Regards, Joao
Regards, Jim
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)); } }
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel