1:45 p.m.
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.
This patch adds a helper libxlDomainFreeIfaceNames for
clearing ifnames on both migration prepare phase and domain
cleanup. It is done on prepare phase so that we don't affect
source domain (specially in cases when the migration fails),
while still allowing interface stats to be consulted during
the migration.
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
Changes since v1:
- Commit message is changed
- Clear ifnames on prepare phase as opposed to begin phase.
---
src/libxl/libxl_domain.c | 32 ++++++++++++++++++++++----------
src/libxl/libxl_domain.h | 3 +++
src/libxl/libxl_migration.c | 2 ++
3 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ef92974..487589d 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -728,16 +728,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
}
}
- if ((vm->def->nnets)) {
- size_t i;
-
- for (i = 0; i < vm->def->nnets; i++) {
- virDomainNetDefPtr net = vm->def->nets[i];
-
- if (STRPREFIX(net->ifname, "vif"))
- VIR_FREE(net->ifname);
- }
- }
+ libxlDomainFreeIfaceNames(vm->def);
if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir,
vm->def->name) > 0) {
if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
@@ -923,6 +914,27 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config
*d_config)
}
}
+/*
+ * Removes autogenerated interface names for the network devices in
+ * parameter def. User-provided interface names are skipped.
+ */
+void
+libxlDomainFreeIfaceNames(virDomainDefPtr def)
+{
+ size_t i;
+
+ for (i = 0; i < def->nnets; i++) {
+ virDomainNetDefPtr net = def->nets[i];
+
+ if (!net->ifname)
+ continue;
+
+ if (STRPREFIX(net->ifname, "vif"))
+ VIR_FREE(net->ifname);
+ }
+}
+
+
/*
* Start a domain through libxenlight.
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 44b3e0b..70c139c 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -135,6 +135,9 @@ int
libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver,
virDomainObjPtr vm);
+void
+libxlDomainFreeIfaceNames(virDomainDefPtr def);
+
int
libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 0d23e5f..1ca31ab 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -288,6 +288,8 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
VIR_DOMAIN_DEF_PARSE_INACTIVE)))
goto cleanup;
+ libxlDomainFreeIfaceNames(def);
+
if (dname) {
name = def->name;
if (VIR_STRDUP(def->name, dname) < 0) {
--
2.1.4
11:04 a.m.
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'd like to get this resolved and a fix pushed before 1.3.0 is released since it
causes a migration regression.
Regards,
Jim
This patch adds a helper libxlDomainFreeIfaceNames for
clearing ifnames on both migration prepare phase and domain
cleanup. It is done on prepare phase so that we don't affect
source domain (specially in cases when the migration fails),
while still allowing interface stats to be consulted during
the migration.
Signed-off-by: Joao Martins <joao.m.martins(a)oracle.com>
---
Changes since v1:
- Commit message is changed
- Clear ifnames on prepare phase as opposed to begin phase.
---
src/libxl/libxl_domain.c | 32 ++++++++++++++++++++++----------
src/libxl/libxl_domain.h | 3 +++
src/libxl/libxl_migration.c | 2 ++
3 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ef92974..487589d 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -728,16 +728,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
}
}
- if ((vm->def->nnets)) {
- size_t i;
-
- for (i = 0; i < vm->def->nnets; i++) {
- virDomainNetDefPtr net = vm->def->nets[i];
-
- if (STRPREFIX(net->ifname, "vif"))
- VIR_FREE(net->ifname);
- }
- }
+ libxlDomainFreeIfaceNames(vm->def);
if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir,
vm->def->name) > 0) {
if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
@@ -923,6 +914,27 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config
*d_config)
}
}
+/*
+ * Removes autogenerated interface names for the network devices in
+ * parameter def. User-provided interface names are skipped.
+ */
+void
+libxlDomainFreeIfaceNames(virDomainDefPtr def)
+{
+ size_t i;
+
+ for (i = 0; i < def->nnets; i++) {
+ virDomainNetDefPtr net = def->nets[i];
+
+ if (!net->ifname)
+ continue;
+
+ if (STRPREFIX(net->ifname, "vif"))
+ VIR_FREE(net->ifname);
+ }
+}
+
+
/*
* Start a domain through libxenlight.
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 44b3e0b..70c139c 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -135,6 +135,9 @@ int
libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver,
virDomainObjPtr vm);
+void
+libxlDomainFreeIfaceNames(virDomainDefPtr def);
+
int
libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 0d23e5f..1ca31ab 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -288,6 +288,8 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
VIR_DOMAIN_DEF_PARSE_INACTIVE)))
goto cleanup;
+ libxlDomainFreeIfaceNames(def);
+
if (dname) {
name = def->name;
if (VIR_STRDUP(def->name, dname) < 0) {
11:59 a.m.
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.
Regards,
Jim
12:11 p.m.
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(a)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(a)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.
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));
}
}
5:27 p.m.
New subject: [libvirt] [Xen-devel] [PATCH v2] libxl: free ifname on libxlDomainMigrationPrepareDef
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(a)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(a)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(a)lists.xen.org
http://lists.xen.org/xen-devel
3270
days inactive
3272
days old
4 comments
2 participants
participants (2)
-
Jim Fehlig -
Joao Martins