10:08 p.m.
The function virNetDevOpenvswitchGetMigrateData() uses the cmd ovs-vsctl to
get portdata. If no portdata is available, rather than failure in running
the cmd, we think we should just print a warning message here, rather than
taking it as wrong, because PortData is just optional for an ovs interface.
If we take it as wrong anyway, in function qemuMigrationBakeCookie() it will
terminate in the middle, and cookies such as NBD would not be baked, further
more errors would happen, such as nbd ports get overflowed, etc.
Thus, we add an if branch in virNetDevOpenvswitchGetMigrateData() to just warn
if portdata is unavailable.
Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com>
Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com>
---
src/util/virnetdevopenvswitch.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index e5c87bb..6116377 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -30,9 +30,12 @@
#include "virerror.h"
#include "virmacaddr.h"
#include "virstring.h"
+#include "virlog.h"
#define VIR_FROM_THIS VIR_FROM_NONE
+VIR_LOG_INIT("util.netdevopenvswitch");
+
/**
* virNetDevOpenvswitchAddPort:
* @brname: the bridge name
@@ -204,18 +207,29 @@ int virNetDevOpenvswitchRemovePort(const char *brname
ATTRIBUTE_UNUSED, const ch
int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
{
virCommandPtr cmd = NULL;
+ char *errbuf = NULL;
int ret = -1;
cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get",
"Interface",
ifname, "external_ids:PortData", NULL);
virCommandSetOutputBuffer(cmd, migrate);
+ virCommandSetErrorBuffer(cmd, &errbuf);
/* Run the command */
- if (virCommandRun(cmd, NULL) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to run command to get OVS port data for "
- "interface %s"), ifname);
+ ret = virCommandRun(cmd, NULL);
+ if (ret < 0) {
+ /*PortData is optional, thus do not take it as wrong if the PortData is not
found.*/
+ if (strstr(errbuf, "no key \"PortData\" in Interface
record")) {
+ VIR_WARN("Can't find OVS port data for interface %s", ifname);
+ if (*migrate)
+ VIR_FREE(*migrate);
+ ret = 0;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to run command to get OVS port data for
"
+ "interface %s"), ifname);
+ }
goto cleanup;
}
@@ -223,6 +237,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char
*ifname)
(*migrate)[strlen(*migrate) - 1] = '\0';
ret = 0;
cleanup:
+ VIR_FREE(errbuf);
virCommandFree(cmd);
return ret;
}
@@ -241,6 +256,11 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char
*ifname)
virCommandPtr cmd = NULL;
int ret = -1;
+ if (NULL == migrate) {
+ VIR_INFO("There is no need to set OVS port data for interface %s",
ifname);
+ return 0;
+ }
+
cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "set",
"Interface", ifname, NULL);
virCommandAddArgFormat(cmd, "external_ids:PortData=%s", migrate);
--
1.7.12.4
4:05 a.m.
On Thu, Feb 12, 2015 at 12:08:54PM +0800, Zhang Bo wrote:
The function virNetDevOpenvswitchGetMigrateData() uses the cmd
ovs-vsctl to
get portdata. If no portdata is available, rather than failure in running
the cmd, we think we should just print a warning message here, rather than
taking it as wrong, because PortData is just optional for an ovs interface.
If we take it as wrong anyway, in function qemuMigrationBakeCookie() it will
terminate in the middle, and cookies such as NBD would not be baked, further
more errors would happen, such as nbd ports get overflowed, etc.
Thus, we add an if branch in virNetDevOpenvswitchGetMigrateData() to just warn
if portdata is unavailable.
Won't that data be missing then?
I would format the subject line as "don't fail if..." to make it brief
and clean, but that's just a nit. Anyway, I see multiple problems
with this patch.
But first let me ask a question; how come there are no PortData on the
destination, when we set them unconditionally? I mean how did you get
to this problem in the first place?
Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com>
Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com>
---
src/util/virnetdevopenvswitch.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index e5c87bb..6116377 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -30,9 +30,12 @@
#include "virerror.h"
#include "virmacaddr.h"
#include "virstring.h"
+#include "virlog.h"
#define VIR_FROM_THIS VIR_FROM_NONE
+VIR_LOG_INIT("util.netdevopenvswitch");
+
/**
* virNetDevOpenvswitchAddPort:
* @brname: the bridge name
@@ -204,18 +207,29 @@ int virNetDevOpenvswitchRemovePort(const char *brname
ATTRIBUTE_UNUSED, const ch
int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
{
virCommandPtr cmd = NULL;
+ char *errbuf = NULL;
int ret = -1;
cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get",
"Interface",
ifname, "external_ids:PortData", NULL);
virCommandSetOutputBuffer(cmd, migrate);
+ virCommandSetErrorBuffer(cmd, &errbuf);
/* Run the command */
- if (virCommandRun(cmd, NULL) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to run command to get OVS port data for "
- "interface %s"), ifname);
+ ret = virCommandRun(cmd, NULL);
+ if (ret < 0) {
+ /*PortData is optional, thus do not take it as wrong if the PortData is not
found.*/
+ if (strstr(errbuf, "no key \"PortData\" in Interface
record")) {
+ VIR_WARN("Can't find OVS port data for interface %s", ifname);
+ if (*migrate)
+ VIR_FREE(*migrate);
+ ret = 0;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to run command to get OVS port data for
"
+ "interface %s"), ifname);
+ }
If there are no PortData set, then why don't we get that info from the
cookie and run the ovs-vsctl based on that?
goto cleanup;
}
@@ -223,6 +237,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char
*ifname)
(*migrate)[strlen(*migrate) - 1] = '\0';
ret = 0;
cleanup:
+ VIR_FREE(errbuf);
virCommandFree(cmd);
return ret;
}
@@ -241,6 +256,11 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char
*ifname)
virCommandPtr cmd = NULL;
int ret = -1;
+ if (NULL == migrate) {
We don't use Yoda conditions, sorry.
+ VIR_INFO("There is no need to set OVS port data for
interface %s", ifname);
this is not candidate for VIR_INFO(), rather for VIR_WARN(), but it
shouldn't be needed. How can we get here with migrate == NULL?
I'm afraid I have to NACK this patch for now, but it might be caused
by some missing ovs knowledge (highly possible). Feel free to correct
me, though.
+ return 0;
+ }
+
cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "set",
"Interface", ifname, NULL);
virCommandAddArgFormat(cmd, "external_ids:PortData=%s", migrate);
--
1.7.12.4
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:25 a.m.
On 2015/2/20 18:05, Martin Kletzander wrote:
On Thu, Feb 12, 2015 at 12:08:54PM +0800, Zhang Bo wrote:
> The function virNetDevOpenvswitchGetMigrateData() uses the cmd ovs-vsctl to
> get portdata. If no portdata is available, rather than failure in running
> the cmd, we think we should just print a warning message here, rather than
> taking it as wrong, because PortData is just optional for an ovs interface.
> If we take it as wrong anyway, in function qemuMigrationBakeCookie() it will
> terminate in the middle, and cookies such as NBD would not be baked, further
> more errors would happen, such as nbd ports get overflowed, etc.
> Thus, we add an if branch in virNetDevOpenvswitchGetMigrateData() to just warn
> if portdata is unavailable.
>
Sorry for replying so late, we were having Spring-Festival holiday then:)
Won't that data be missing then?
Yes, the portdata will be missing, but as that it's *OPTIONAL*, missing is not a big
deal.
We can tell that it's optional from the comments in
qemuMigrationCookieNetworkXMLParse():
"port data is optional, and may not exist".
It's optional as well as other external_ids for an ovs interface.
I would format the subject line as "don't fail if..." to make it brief
and clean, but that's just a nit. Anyway, I see multiple problems
with this patch.
I would modify that if this patch is decided to be acceptable later.
But first let me ask a question; how come there are no PortData on
the
destination, when we set them unconditionally? I mean how did you get
to this problem in the first place?
Sorry for missing the detailed description of the original problem.
The problem was actually:
After multiple times of migrating a domain, which has an ovs interface
with no portData set, with non-shared disk, nbd ports got overflowed.
The steps to reproduce the problem:
1 define and start a domain with its network configured as:
<interface type='bridge'>
<source bridge='br0'/>
<virtualport type='openvswitch'>
</virtualport>
<model type='virtio'/>
<driver name='vhost' queues='4'/>
</interface>
2 do not set the network's portData.
3 migrate(ToURI2) it with flag 91(1011011), which means:
VIR_MIGRATE_LIVE
VIR_MIGRATE_PEER2PEER
VIR_MIGRATE_PERSIST_DEST
VIR_MIGRATE_UNDEFINE_SOURCE
VIR_MIGRATE_NON_SHARED_DISK
4 migrate success, but we got an error log in libvirtd.log:
error : virCommandWait:2423 : internal error: Child process (ovs-vsctl --timeout=5
get Interface vnet1 external_ids:PortData) unexpected exit status 1: ovs-vsctl: no
key "PortData" in Interface record "vnet1" column external_ids
5 migrate it back, migrate it , migrate it back, .......
6 nbd port got overflowed.
The nbd port problem is directly caused by the problem in step 4. In fact the patch here
just fixed
problem 4. If we want to totally solve problem in step 6, lots more work has to be done.
We'll study the nbd problem later. If you have any suggestion, please let me know,
thanks.
The reasons for the problem is :
1 virNetDevOpenvswitchGetMigrateData() takes it as wrong if no portData is available for
the ovs interface of a domain. (We think it's not appropriate, as portData is just
OPTIONAL)
2 in func qemuMigrationBakeCookie(), it fails in qemuMigrationCookieAddNetwork(), and
returns
with -1. qemuMigrationCookieAddNBD() is not called thereafter, and mig->nbd is still
NULL.
3 However, qemuMigrationRun() just *WARN* if qemuMigrationBakeCookie() fails, migration
still successes.
cookie is NULL, it's not baked on the src side.
4 On the destination side, it would alloc a port first and then free the nbd port in
COOKIE.
But the cookie is NULL due to qemuMigrationCookieAddNetwork() failure at src side. thus
the nbd port
is not freed.
> Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com>
> Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com>
> ---
> src/util/virnetdevopenvswitch.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> index e5c87bb..6116377 100644
> --- a/src/util/virnetdevopenvswitch.c
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -30,9 +30,12 @@
> #include "virerror.h"
> #include "virmacaddr.h"
> #include "virstring.h"
> +#include "virlog.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> +VIR_LOG_INIT("util.netdevopenvswitch");
> +
> /**
> * virNetDevOpenvswitchAddPort:
> * @brname: the bridge name
> @@ -204,18 +207,29 @@ int virNetDevOpenvswitchRemovePort(const char *brname
ATTRIBUTE_UNUSED, const ch
> int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
> {
> virCommandPtr cmd = NULL;
> + char *errbuf = NULL;
> int ret = -1;
>
> cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get",
"Interface",
> ifname, "external_ids:PortData", NULL);
>
> virCommandSetOutputBuffer(cmd, migrate);
> + virCommandSetErrorBuffer(cmd, &errbuf);
>
> /* Run the command */
> - if (virCommandRun(cmd, NULL) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Unable to run command to get OVS port data for
"
> - "interface %s"), ifname);
> + ret = virCommandRun(cmd, NULL);
> + if (ret < 0) {
> + /*PortData is optional, thus do not take it as wrong if the PortData is not
found.*/
> + if (strstr(errbuf, "no key \"PortData\" in Interface
record")) {
> + VIR_WARN("Can't find OVS port data for interface %s",
ifname);
> + if (*migrate)
> + VIR_FREE(*migrate);
> + ret = 0;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to run command to get OVS port data
for "
> + "interface %s"), ifname);
> + }
If there are no PortData set, then why don't we get that info from the
cookie and run the ovs-vsctl based on that?
The cookie has not been generated yet, the purpose of this function here is to generate
the network part
of the cookie at src side.
> goto cleanup;
> }
>
> @@ -223,6 +237,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char
*ifname)
> (*migrate)[strlen(*migrate) - 1] = '\0';
> ret = 0;
> cleanup:
> + VIR_FREE(errbuf);
> virCommandFree(cmd);
> return ret;
> }
> @@ -241,6 +256,11 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char
*ifname)
> virCommandPtr cmd = NULL;
> int ret = -1;
>
> + if (NULL == migrate) {
We don't use Yoda conditions, sorry.
OK, thanks for your correction.
> + VIR_INFO("There is no need to set OVS port data for interface %s",
ifname);
this is not candidate for VIR_INFO(), rather for VIR_WARN(), but it
shouldn't be needed. How can we get here with migrate == NULL?
Because at the src side, cookie has not been baked(because it founds no portData available
as we mentiond above).
I'm afraid I have to NACK this patch for now, but it might be caused
by some missing ovs knowledge (highly possible). Feel free to correct
me, though.
I still think we should just warn if no portData is available, which is optional as well
as other external_ids.
Although another problem(nbd port got overflowed) is left to be perfectly solved.
Looking forward to your reply. thanks:)
> + return 0;
> + }
> +
> cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "set",
> "Interface", ifname, NULL);
> virCommandAddArgFormat(cmd, "external_ids:PortData=%s", migrate);
> --
> 1.7.12.4
>
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
3:25 a.m.
On Thu, Feb 12, 2015 at 12:08:54PM +0800, Zhang Bo wrote:
The function virNetDevOpenvswitchGetMigrateData() uses the cmd
ovs-vsctl to
get portdata. If no portdata is available, rather than failure in running
the cmd, we think we should just print a warning message here, rather than
taking it as wrong, because PortData is just optional for an ovs interface.
If we take it as wrong anyway, in function qemuMigrationBakeCookie() it will
terminate in the middle, and cookies such as NBD would not be baked, further
more errors would happen, such as nbd ports get overflowed, etc.
Thus, we add an if branch in virNetDevOpenvswitchGetMigrateData() to just warn
if portdata is unavailable.
Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com>
Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com>
---
src/util/virnetdevopenvswitch.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
In my previous review I mixed what Set/Get functions do, so that's why
that made way less sense for me :)
Anyway, review v2:
diff --git a/src/util/virnetdevopenvswitch.c
b/src/util/virnetdevopenvswitch.c
index e5c87bb..6116377 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -30,9 +30,12 @@
#include "virerror.h"
#include "virmacaddr.h"
#include "virstring.h"
+#include "virlog.h"
#define VIR_FROM_THIS VIR_FROM_NONE
+VIR_LOG_INIT("util.netdevopenvswitch");
+
/**
* virNetDevOpenvswitchAddPort:
* @brname: the bridge name
@@ -204,18 +207,29 @@ int virNetDevOpenvswitchRemovePort(const char *brname
ATTRIBUTE_UNUSED, const ch
int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
{
virCommandPtr cmd = NULL;
+ char *errbuf = NULL;
int ret = -1;
cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get",
"Interface",
ifname, "external_ids:PortData", NULL);
virCommandSetOutputBuffer(cmd, migrate);
+ virCommandSetErrorBuffer(cmd, &errbuf);
/* Run the command */
- if (virCommandRun(cmd, NULL) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to run command to get OVS port data for "
- "interface %s"), ifname);
+ ret = virCommandRun(cmd, NULL);
+ if (ret < 0) {
+ /*PortData is optional, thus do not take it as wrong if the PortData is not
found.*/
Missing space after asterisk and the line is long. Either wrap it or
change to:
/* PortData is optional, don't fail if there are none. */
+ if (strstr(errbuf, "no key \"PortData\" in
Interface record")) {
+ VIR_WARN("Can't find OVS port data for interface %s", ifname);
Thinking about these two messages, VIR_DEBUG() would be enough, so we
don't spam the logs with useless info.
+ if (*migrate)
+ VIR_FREE(*migrate);
"make syntax-check" would tell you that you don't have to do this and
it would fail making sure you'll fix it ;) Just remove the if,
VIR_FREE is safe for NULLs.
+ ret = 0;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to run command to get OVS port data for
"
+ "interface %s"), ifname);
Indentation is off here.
+ }
goto cleanup;
}
@@ -223,6 +237,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char
*ifname)
(*migrate)[strlen(*migrate) - 1] = '\0';
ret = 0;
cleanup:
+ VIR_FREE(errbuf);
virCommandFree(cmd);
return ret;
}
@@ -241,6 +256,11 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char
*ifname)
virCommandPtr cmd = NULL;
int ret = -1;
+ if (NULL == migrate) {
Change it to (!migrate).
+ VIR_INFO("There is no need to set OVS port data for
interface %s", ifname);
Same as above with VIR_WARN().
+ return 0;
+ }
+
cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "set",
"Interface", ifname, NULL);
virCommandAddArgFormat(cmd, "external_ids:PortData=%s", migrate);
--
1.7.12.4
Anyway, even with those changes reflected in v2 I don't feel confident
to ACK this even though the code looks perfectly OK for me, so I'd
like a second opinion from someone more OVS savvy.
Martin
3:52 a.m.
On Thu, Feb 26, 2015 at 10:25:45AM +0100, Martin Kletzander wrote:
On Thu, Feb 12, 2015 at 12:08:54PM +0800, Zhang Bo wrote:
>The function virNetDevOpenvswitchGetMigrateData() uses the cmd ovs-vsctl to
>get portdata. If no portdata is available, rather than failure in running
>the cmd, we think we should just print a warning message here, rather than
>taking it as wrong, because PortData is just optional for an ovs interface.
>If we take it as wrong anyway, in function qemuMigrationBakeCookie() it will
>terminate in the middle, and cookies such as NBD would not be baked, further
>more errors would happen, such as nbd ports get overflowed, etc.
>Thus, we add an if branch in virNetDevOpenvswitchGetMigrateData() to just warn
>if portdata is unavailable.
>
>Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com>
>Signed-off-by: Zhou Yimin <zhouyimin(a)huawei.com>
>---
>src/util/virnetdevopenvswitch.c | 28 ++++++++++++++++++++++++----
>1 file changed, 24 insertions(+), 4 deletions(-)
>
In my previous review I mixed what Set/Get functions do, so that's why
that made way less sense for me :)
Anyway, review v2:
>diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
>index e5c87bb..6116377 100644
>--- a/src/util/virnetdevopenvswitch.c
>+++ b/src/util/virnetdevopenvswitch.c
>@@ -30,9 +30,12 @@
>#include "virerror.h"
>#include "virmacaddr.h"
>#include "virstring.h"
>+#include "virlog.h"
>
>#define VIR_FROM_THIS VIR_FROM_NONE
>
>+VIR_LOG_INIT("util.netdevopenvswitch");
>+
>/**
> * virNetDevOpenvswitchAddPort:
> * @brname: the bridge name
>@@ -204,18 +207,29 @@ int virNetDevOpenvswitchRemovePort(const char *brname
ATTRIBUTE_UNUSED, const ch
>int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
>{
> virCommandPtr cmd = NULL;
>+ char *errbuf = NULL;
> int ret = -1;
>
> cmd = virCommandNewArgList(OVSVSCTL, "--timeout=5", "get",
"Interface",
> ifname, "external_ids:PortData", NULL);
Yet another idea, what if you used "--if-exists get Interface ..." and
then just checked for the empty line? The code would be cleaner as well.
3558
days inactive
3572
days old
4 comments
3 participants
participants (3)
-
Martin Kletzander -
zhang bo -
Zhang Bo