| Message ID | 20230424060103.33586-1-wanjunjie@bytedance.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ovs-ofctl: skip invalid flows for replace-flows | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Mon, Apr 24, 2023 at 02:01:03PM +0800, Wan Junjie via dev wrote: > ovs-save will do replace-flows for ovs restart. For some reason, ovs > could have an invalid action, like invalid meter. Then adding flows > will be stopped at the position of the first invalid flow. This could > happen when ovs restart and vm related resource allocatting or destructing > at the same time. > > This patch will skip the invalid flows and continue the process so > all valid flows will be added during the ovs restart. An destructed vm > will not affect those normal vm traffic. > > Signed-off-by: Wan Junjie <wanjunjie@bytedance.com> Hi Wan Junjie, thanks for your patch. > --- > include/openvswitch/vconn.h | 2 +- > lib/vconn.c | 23 ++++++++++++++--------- > utilities/ovs-ofctl.c | 19 +++++++++++++------ > 3 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h > index 5f69c732b..781c59075 100644 > --- a/include/openvswitch/vconn.h > +++ b/include/openvswitch/vconn.h > @@ -57,7 +57,7 @@ int vconn_recv_xid(struct vconn *, ovs_be32 xid, struct ofpbuf **); > int vconn_transact(struct vconn *, struct ofpbuf *, struct ofpbuf **); > int vconn_transact_noreply(struct vconn *, struct ofpbuf *, struct ofpbuf **); > int vconn_transact_multiple_noreply(struct vconn *, struct ovs_list *requests, > - struct ofpbuf **replyp); > + struct ovs_list *replies); > int vconn_transact_multipart(struct vconn *, struct ovs_list *request, > struct ovs_list *replies); > > diff --git a/lib/vconn.c b/lib/vconn.c > index b55676227..a5f6177ec 100644 > --- a/lib/vconn.c > +++ b/lib/vconn.c > @@ -926,23 +926,28 @@ vconn_transact_noreply(struct vconn *vconn, struct ofpbuf *request, > /* vconn_transact_noreply() for a list of "struct ofpbuf"s, sent one by one. > * All of the requests on 'requests' are always destroyed, regardless of the > * return value. */ > -int > -vconn_transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests, > - struct ofpbuf **replyp) > -{ > +int vconn_transact_multiple_noreply(struct vconn *vconn, > + struct ovs_list *requests, > + struct ovs_list *replies) { > struct ofpbuf *request; > + int error = 0; > > LIST_FOR_EACH_POP (request, list_node, requests) { > - int error; > + struct ofpbuf *reply; > + > + error = vconn_transact_noreply(vconn, request, &reply); > nit: no blank line here please > - error = vconn_transact_noreply(vconn, request, replyp); > - if (error || *replyp) { > - ofpbuf_list_delete(requests); > + if (error && !reply) { > return error; > } > + if (reply) { > + ovs_list_push_back(replies, &reply->list_node); > + } > } > > - *replyp = NULL; > + if (!ovs_list_is_empty(replies)) { > + return error; If there has been a reply then we get here. And return the most recent value of error which, by my reading, may not relate to any of the replies. Is this intentional? > + } > return 0; > } > > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c > index 24d0941cf..a84551672 100644 > --- a/utilities/ovs-ofctl.c > +++ b/utilities/ovs-ofctl.c > @@ -715,18 +715,25 @@ dump_trivial_transaction(const char *vconn_name, enum ofpraw ofpraw) > static void > transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests) > { > + struct ovs_list replies; > struct ofpbuf *reply; > > - run(vconn_transact_multiple_noreply(vconn, requests, &reply), > + ovs_list_init(&replies); > + > + run(vconn_transact_multiple_noreply(vconn, requests, &replies), > "talking to %s", vconn_get_name(vconn)); > - if (reply) { > + nit: no blank line here please > + if (ovs_list_is_empty(&replies)) { > + return; > + } > + > + LIST_FOR_EACH_POP (reply, list_node, &replies) { > ofp_print(stderr, reply->data, reply->size, > ports_to_show(vconn_get_name(vconn)), > - tables_to_show(vconn_get_name(vconn)), > - verbosity + 2); > - exit(1); > + tables_to_show(vconn_get_name(vconn)), verbosity + 2); nit: the change to the line immediately above seems to be whitepace only. I don't think it belongs in this patch. > + ofpbuf_delete(reply); > } > - ofpbuf_delete(reply); > + exit(1); > } > > /* Frees the error messages as they are printed. */ ...
On Wed, Apr 26, 2023 at 8:57 PM Simon Horman <simon.horman@corigine.com> wrote: > > On Mon, Apr 24, 2023 at 02:01:03PM +0800, Wan Junjie via dev wrote: > > ovs-save will do replace-flows for ovs restart. For some reason, ovs > > could have an invalid action, like invalid meter. Then adding flows > > will be stopped at the position of the first invalid flow. This could > > happen when ovs restart and vm related resource allocatting or destructing > > at the same time. > > > > This patch will skip the invalid flows and continue the process so > > all valid flows will be added during the ovs restart. An destructed vm > > will not affect those normal vm traffic. > > > > Signed-off-by: Wan Junjie <wanjunjie@bytedance.com> > > Hi Wan Junjie, > > thanks for your patch. > Hi Simon, Thank you for your review. > > --- > > include/openvswitch/vconn.h | 2 +- > > lib/vconn.c | 23 ++++++++++++++--------- > > utilities/ovs-ofctl.c | 19 +++++++++++++------ > > 3 files changed, 28 insertions(+), 16 deletions(-) > > > > diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h > > index 5f69c732b..781c59075 100644 > > --- a/include/openvswitch/vconn.h > > +++ b/include/openvswitch/vconn.h > > @@ -57,7 +57,7 @@ int vconn_recv_xid(struct vconn *, ovs_be32 xid, struct ofpbuf **); > > int vconn_transact(struct vconn *, struct ofpbuf *, struct ofpbuf **); > > int vconn_transact_noreply(struct vconn *, struct ofpbuf *, struct ofpbuf **); > > int vconn_transact_multiple_noreply(struct vconn *, struct ovs_list *requests, > > - struct ofpbuf **replyp); > > + struct ovs_list *replies); > > int vconn_transact_multipart(struct vconn *, struct ovs_list *request, > > struct ovs_list *replies); > > > > diff --git a/lib/vconn.c b/lib/vconn.c > > index b55676227..a5f6177ec 100644 > > --- a/lib/vconn.c > > +++ b/lib/vconn.c > > @@ -926,23 +926,28 @@ vconn_transact_noreply(struct vconn *vconn, struct ofpbuf *request, > > /* vconn_transact_noreply() for a list of "struct ofpbuf"s, sent one by one. > > * All of the requests on 'requests' are always destroyed, regardless of the > > * return value. */ > > -int > > -vconn_transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests, > > - struct ofpbuf **replyp) > > -{ > > +int vconn_transact_multiple_noreply(struct vconn *vconn, > > + struct ovs_list *requests, > > + struct ovs_list *replies) { > > struct ofpbuf *request; > > + int error = 0; > > > > LIST_FOR_EACH_POP (request, list_node, requests) { > > - int error; > > + struct ofpbuf *reply; > > + > > + error = vconn_transact_noreply(vconn, request, &reply); > > > > nit: no blank line here please > OK > > - error = vconn_transact_noreply(vconn, request, replyp); > > - if (error || *replyp) { > > - ofpbuf_list_delete(requests); > > + if (error && !reply) { > > return error; > > } > > + if (reply) { > > + ovs_list_push_back(replies, &reply->list_node); > > + } > > } > > > > - *replyp = NULL; > > + if (!ovs_list_is_empty(replies)) { > > + return error; > > If there has been a reply then we get here. > And return the most recent value of error which, > by my reading, may not relate to any of the replies. > Is this intentional? > As I can see in vconn_transact_noreply, there should be two return patterns. One is an error and no reply, as error means a vconn is broken. In this case we should stop further requests as they will fail anyway. The other is no error and a valid reply, though the reply means a protocol error. The original code take both patterns as errors and stop further requests. This change will take the second pattern as a normal response. So if it reaches the code of return error here, usually it means error = 0. > > + } > > return 0; > > } > > > > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c > > index 24d0941cf..a84551672 100644 > > --- a/utilities/ovs-ofctl.c > > +++ b/utilities/ovs-ofctl.c > > @@ -715,18 +715,25 @@ dump_trivial_transaction(const char *vconn_name, enum ofpraw ofpraw) > > static void > > transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests) > > { > > + struct ovs_list replies; > > struct ofpbuf *reply; > > > > - run(vconn_transact_multiple_noreply(vconn, requests, &reply), > > + ovs_list_init(&replies); > > + > > + run(vconn_transact_multiple_noreply(vconn, requests, &replies), > > "talking to %s", vconn_get_name(vconn)); > > - if (reply) { > > + > > nit: no blank line here please > OK > > + if (ovs_list_is_empty(&replies)) { > > + return; > > + } > > + > > + LIST_FOR_EACH_POP (reply, list_node, &replies) { > > ofp_print(stderr, reply->data, reply->size, > > ports_to_show(vconn_get_name(vconn)), > > - tables_to_show(vconn_get_name(vconn)), > > - verbosity + 2); > > - exit(1); > > + tables_to_show(vconn_get_name(vconn)), verbosity + 2); > > nit: the change to the line immediately above seems to be whitepace only. > I don't think it belongs in this patch. > code here move exit to a later place. Newline here is not intensional. > > + ofpbuf_delete(reply); > > } > > - ofpbuf_delete(reply); > > + exit(1); > > } > > > > /* Frees the error messages as they are printed. */ > > ...
diff --git a/include/openvswitch/vconn.h b/include/openvswitch/vconn.h index 5f69c732b..781c59075 100644 --- a/include/openvswitch/vconn.h +++ b/include/openvswitch/vconn.h @@ -57,7 +57,7 @@ int vconn_recv_xid(struct vconn *, ovs_be32 xid, struct ofpbuf **); int vconn_transact(struct vconn *, struct ofpbuf *, struct ofpbuf **); int vconn_transact_noreply(struct vconn *, struct ofpbuf *, struct ofpbuf **); int vconn_transact_multiple_noreply(struct vconn *, struct ovs_list *requests, - struct ofpbuf **replyp); + struct ovs_list *replies); int vconn_transact_multipart(struct vconn *, struct ovs_list *request, struct ovs_list *replies); diff --git a/lib/vconn.c b/lib/vconn.c index b55676227..a5f6177ec 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -926,23 +926,28 @@ vconn_transact_noreply(struct vconn *vconn, struct ofpbuf *request, /* vconn_transact_noreply() for a list of "struct ofpbuf"s, sent one by one. * All of the requests on 'requests' are always destroyed, regardless of the * return value. */ -int -vconn_transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests, - struct ofpbuf **replyp) -{ +int vconn_transact_multiple_noreply(struct vconn *vconn, + struct ovs_list *requests, + struct ovs_list *replies) { struct ofpbuf *request; + int error = 0; LIST_FOR_EACH_POP (request, list_node, requests) { - int error; + struct ofpbuf *reply; + + error = vconn_transact_noreply(vconn, request, &reply); - error = vconn_transact_noreply(vconn, request, replyp); - if (error || *replyp) { - ofpbuf_list_delete(requests); + if (error && !reply) { return error; } + if (reply) { + ovs_list_push_back(replies, &reply->list_node); + } } - *replyp = NULL; + if (!ovs_list_is_empty(replies)) { + return error; + } return 0; } diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index 24d0941cf..a84551672 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -715,18 +715,25 @@ dump_trivial_transaction(const char *vconn_name, enum ofpraw ofpraw) static void transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests) { + struct ovs_list replies; struct ofpbuf *reply; - run(vconn_transact_multiple_noreply(vconn, requests, &reply), + ovs_list_init(&replies); + + run(vconn_transact_multiple_noreply(vconn, requests, &replies), "talking to %s", vconn_get_name(vconn)); - if (reply) { + + if (ovs_list_is_empty(&replies)) { + return; + } + + LIST_FOR_EACH_POP (reply, list_node, &replies) { ofp_print(stderr, reply->data, reply->size, ports_to_show(vconn_get_name(vconn)), - tables_to_show(vconn_get_name(vconn)), - verbosity + 2); - exit(1); + tables_to_show(vconn_get_name(vconn)), verbosity + 2); + ofpbuf_delete(reply); } - ofpbuf_delete(reply); + exit(1); } /* Frees the error messages as they are printed. */
ovs-save will do replace-flows for ovs restart. For some reason, ovs could have an invalid action, like invalid meter. Then adding flows will be stopped at the position of the first invalid flow. This could happen when ovs restart and vm related resource allocatting or destructing at the same time. This patch will skip the invalid flows and continue the process so all valid flows will be added during the ovs restart. An destructed vm will not affect those normal vm traffic. Signed-off-by: Wan Junjie <wanjunjie@bytedance.com> --- include/openvswitch/vconn.h | 2 +- lib/vconn.c | 23 ++++++++++++++--------- utilities/ovs-ofctl.c | 19 +++++++++++++------ 3 files changed, 28 insertions(+), 16 deletions(-)