2
0
mirror of https://github.com/openvswitch/ovs synced 2025-08-31 06:15:47 +00:00

vconn: Better bundle error management.

It is possible that a bundle add message fails, but the following
commit succeeds, since the message was not added to the bundle.  Make
ovs-ofctl fail also in these cases.

Also, the commit should not be sent if any of the bundled messages
failed.  To make sure all the errors are received before the commit is
sent, a barrier is required before sending the commit message.

Finally, make vconn collect bundle errors into a list instead of
calling a callback.  This makes bundle error management simpler.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
This commit is contained in:
Jarno Rajahalme
2016-07-29 16:52:03 -07:00
parent bcf899f712
commit 506c1ddb34
5 changed files with 170 additions and 63 deletions

View File

@@ -55,9 +55,22 @@ 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);
/* Bundle errors must be free()d by the caller. */
struct vconn_bundle_error {
struct ovs_list list_node;
/* OpenFlow header and some of the message contents for error reporting. */
union {
struct ofp_header ofp_msg;
uint8_t ofp_msg_data[64];
};
};
/* Bundle errors must be free()d by the caller. */
int vconn_bundle_transact(struct vconn *, struct ovs_list *requests,
uint16_t bundle_flags,
void (*error_reporter)(const struct ofp_header *));
struct ovs_list *errors);
void vconn_run(struct vconn *);
void vconn_run_wait(struct vconn *);

View File

@@ -744,9 +744,21 @@ vconn_recv_block(struct vconn *vconn, struct ofpbuf **msgp)
return retval;
}
static void
vconn_add_bundle_error(const struct ofp_header *oh, struct ovs_list *errors)
{
if (errors) {
struct vconn_bundle_error *err = xmalloc(sizeof *err);
size_t len = ntohs(oh->length);
memcpy(err->ofp_msg_data, oh, MIN(len, sizeof err->ofp_msg_data));
ovs_list_push_back(errors, &err->list_node);
}
}
static int
vconn_recv_xid__(struct vconn *vconn, ovs_be32 xid, struct ofpbuf **replyp,
void (*error_reporter)(const struct ofp_header *))
struct ovs_list *errors)
{
for (;;) {
ovs_be32 recv_xid;
@@ -768,8 +780,8 @@ vconn_recv_xid__(struct vconn *vconn, ovs_be32 xid, struct ofpbuf **replyp,
}
error = ofptype_decode(&type, oh);
if (!error && type == OFPTYPE_ERROR && error_reporter) {
error_reporter(oh);
if (!error && type == OFPTYPE_ERROR) {
vconn_add_bundle_error(oh, errors);
} else {
VLOG_DBG_RL(&bad_ofmsg_rl, "%s: received reply with xid %08"PRIx32
" != expected %08"PRIx32,
@@ -793,8 +805,7 @@ vconn_recv_xid(struct vconn *vconn, ovs_be32 xid, struct ofpbuf **replyp)
static int
vconn_transact__(struct vconn *vconn, struct ofpbuf *request,
struct ofpbuf **replyp,
void (*error_reporter)(const struct ofp_header *))
struct ofpbuf **replyp, struct ovs_list *errors)
{
ovs_be32 send_xid = ((struct ofp_header *) request->data)->xid;
int error;
@@ -804,8 +815,7 @@ vconn_transact__(struct vconn *vconn, struct ofpbuf *request,
if (error) {
ofpbuf_delete(request);
}
return error ? error : vconn_recv_xid__(vconn, send_xid, replyp,
error_reporter);
return error ? error : vconn_recv_xid__(vconn, send_xid, replyp, errors);
}
/* Sends 'request' to 'vconn' and blocks until it receives a reply with a
@@ -825,6 +835,22 @@ vconn_transact(struct vconn *vconn, struct ofpbuf *request,
return vconn_transact__(vconn, request, replyp, NULL);
}
static int
vconn_send_barrier(struct vconn *vconn, ovs_be32 *barrier_xid)
{
struct ofpbuf *barrier;
int error;
/* Send barrier. */
barrier = ofputil_encode_barrier_request(vconn_get_version(vconn));
*barrier_xid = ((struct ofp_header *) barrier->data)->xid;
error = vconn_send_block(vconn, barrier);
if (error) {
ofpbuf_delete(barrier);
}
return error;
}
/* Sends 'request' followed by a barrier request to 'vconn', then blocks until
* it receives a reply to the barrier. If successful, stores the reply to
* 'request' in '*replyp', if one was received, and otherwise NULL, then
@@ -842,7 +868,6 @@ vconn_transact_noreply(struct vconn *vconn, struct ofpbuf *request,
{
ovs_be32 request_xid;
ovs_be32 barrier_xid;
struct ofpbuf *barrier;
int error;
*replyp = NULL;
@@ -856,11 +881,8 @@ vconn_transact_noreply(struct vconn *vconn, struct ofpbuf *request,
}
/* Send barrier. */
barrier = ofputil_encode_barrier_request(vconn_get_version(vconn));
barrier_xid = ((struct ofp_header *) barrier->data)->xid;
error = vconn_send_block(vconn, barrier);
error = vconn_send_barrier(vconn, &barrier_xid);
if (error) {
ofpbuf_delete(barrier);
return error;
}
@@ -924,7 +946,7 @@ vconn_transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests,
static enum ofperr
vconn_bundle_reply_validate(struct ofpbuf *reply,
struct ofputil_bundle_ctrl_msg *request,
void (*error_reporter)(const struct ofp_header *))
struct ovs_list *errors)
{
const struct ofp_header *oh;
enum ofptype type;
@@ -938,7 +960,7 @@ vconn_bundle_reply_validate(struct ofpbuf *reply,
}
if (type == OFPTYPE_ERROR) {
error_reporter(oh);
vconn_add_bundle_error(oh, errors);
return ofperr_decode_msg(oh, NULL);
}
if (type != OFPTYPE_BUNDLE_CONTROL) {
@@ -964,15 +986,15 @@ vconn_bundle_reply_validate(struct ofpbuf *reply,
/* Send bundle control message 'bc' of 'type' via 'vconn', and wait for either
* an error or the corresponding bundle control message response.
*
* 'error_reporter' is called for any error responses received, which may be
* also regarding earlier OpenFlow messages than this bundle control message.
* 'errors' is a list to which any OpenFlow errors relating to bundle
* processing are appended. Caller is responsible for releasing the memory of
* each node in the list on return.
*
* Returns errno value, or 0 when successful. */
static int
vconn_bundle_control_transact(struct vconn *vconn,
struct ofputil_bundle_ctrl_msg *bc,
uint16_t type,
void (*error_reporter)(const struct ofp_header *))
uint16_t type, struct ovs_list *errors)
{
struct ofpbuf *request, *reply;
int error;
@@ -981,21 +1003,12 @@ vconn_bundle_control_transact(struct vconn *vconn,
bc->type = type;
request = ofputil_encode_bundle_ctrl_request(vconn->version, bc);
ofpmsg_update_length(request);
error = vconn_transact__(vconn, request, &reply, error_reporter);
error = vconn_transact__(vconn, request, &reply, errors);
if (error) {
return error;
}
ofperr = vconn_bundle_reply_validate(reply, bc, error_reporter);
if (ofperr) {
VLOG_WARN_RL(&bad_ofmsg_rl, "Bundle %s failed (%s).",
type == OFPBCT_OPEN_REQUEST ? "open"
: type == OFPBCT_CLOSE_REQUEST ? "close"
: type == OFPBCT_COMMIT_REQUEST ? "commit"
: type == OFPBCT_DISCARD_REQUEST ? "discard"
: "control message",
ofperr_to_string(ofperr));
}
ofperr = vconn_bundle_reply_validate(reply, bc, errors);
ofpbuf_delete(reply);
return ofperr ? EPROTO : 0;
@@ -1003,8 +1016,7 @@ vconn_bundle_control_transact(struct vconn *vconn,
/* Checks if error responses can be received on 'vconn'. */
static void
vconn_recv_error(struct vconn *vconn,
void (*error_reporter)(const struct ofp_header *))
vconn_recv_error(struct vconn *vconn, struct ovs_list *errors)
{
int error;
@@ -1020,7 +1032,7 @@ vconn_recv_error(struct vconn *vconn,
oh = reply->data;
ofperr = ofptype_decode(&type, oh);
if (!ofperr && type == OFPTYPE_ERROR) {
error_reporter(oh);
vconn_add_bundle_error(oh, errors);
} else {
VLOG_DBG_RL(&bad_ofmsg_rl,
"%s: received unexpected reply with xid %08"PRIx32,
@@ -1031,10 +1043,32 @@ vconn_recv_error(struct vconn *vconn,
} while (!error);
}
/* Sends a barrier and waits for the barrier response and stores any errors
* that are received before the barrier response. */
static int
vconn_bundle_barrier_transact(struct vconn *vconn, struct ovs_list *errors)
{
struct ofpbuf *reply;
ovs_be32 barrier_xid;
int error;
error = vconn_send_barrier(vconn, &barrier_xid);
if (error) {
return error;
}
error = vconn_recv_xid__(vconn, barrier_xid, &reply, errors);
if (error) {
return error;
}
ofpbuf_delete(reply);
return 0;
}
static int
vconn_bundle_add_msg(struct vconn *vconn, struct ofputil_bundle_ctrl_msg *bc,
struct ofpbuf *msg,
void (*error_reporter)(const struct ofp_header *))
struct ovs_list *errors)
{
struct ofputil_bundle_add_msg bam;
struct ofpbuf *request;
@@ -1052,45 +1086,55 @@ vconn_bundle_add_msg(struct vconn *vconn, struct ofputil_bundle_ctrl_msg *bc,
if (!error) {
/* Check for an error return, so that the socket buffer does not become
* full of errors. */
vconn_recv_error(vconn, error_reporter);
vconn_recv_error(vconn, errors);
}
return error;
}
int
vconn_bundle_transact(struct vconn *vconn, struct ovs_list *requests,
uint16_t flags,
void (*error_reporter)(const struct ofp_header *))
uint16_t flags, struct ovs_list *errors)
{
struct ofputil_bundle_ctrl_msg bc;
struct ofpbuf *request;
int error;
ovs_list_init(errors);
memset(&bc, 0, sizeof bc);
bc.flags = flags;
error = vconn_bundle_control_transact(vconn, &bc, OFPBCT_OPEN_REQUEST,
error_reporter);
errors);
if (error) {
return error;
}
LIST_FOR_EACH (request, list_node, requests) {
error = vconn_bundle_add_msg(vconn, &bc, request, error_reporter);
error = vconn_bundle_add_msg(vconn, &bc, request, errors);
if (error) {
break;
}
}
if (!error) {
/* A failing message does not invalidate the bundle, but the message is
* simply not added to the bundle. Since we do not want to commit if
* any of the messages failed, we need to explicitly sync with barrier
* before we issue the commit message. */
error = vconn_bundle_barrier_transact(vconn, errors);
}
if (!error && !ovs_list_is_empty(errors)) {
error = EPROTO;
}
/* Commit only if no errors are received. */
if (!error) {
error = vconn_bundle_control_transact(vconn, &bc,
OFPBCT_COMMIT_REQUEST,
error_reporter);
errors);
} else {
/* Do not overwrite the error code from vconn_bundle_add_msg().
* Any error in discard should be either reported or logged, so it
* should not get lost. */
vconn_bundle_control_transact(vconn, &bc, OFPBCT_DISCARD_REQUEST,
error_reporter);
errors);
}
return error;
}

View File

@@ -4409,6 +4409,8 @@ OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:88:99:aa:bb:cc idle:70 actions=ou
vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.4): DEL table:255 in_port=2,dl_src=00:88:99:aa:bb:cc actions=drop
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.4):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.4):
vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
@@ -4440,6 +4442,8 @@ OFPT_FLOW_MOD (OF1.4): MOD actions=drop
vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.4): MOD_STRICT in_port=2,dl_src=00:77:88:99:aa:bb actions=output:7
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.4):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.4):
vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
@@ -4474,6 +4478,8 @@ OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:255 in_port=2,dl_src=00:66:77:88:99:aa a
vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.4): ADD in_port=2,dl_src=00:66:77:88:99:aa actions=drop
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.4):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.4):
vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
@@ -4528,15 +4534,11 @@ delete in_port=2 dl_src=00:88:99:aa:bb:cc
add table=254 actions=drop
])
AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt 2>&1 | sed '/|WARN|/d
s/unix:.*br0\.mgmt/unix:br0.mgmt/' | sed 's/(.* error)/(error)/'],
AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt 2>&1 | sed '/talking to/,$d'],
[0], [dnl
OFPT_ERROR (OF1.4) (xid=0xb): OFPBRC_EPERM
OFPT_FLOW_MOD (OF1.4) (xid=0xb): ADD table:254 actions=drop
OFPT_ERROR (OF1.4) (xid=0xd): OFPBFC_MSG_FAILED
OFPT_BUNDLE_CONTROL (OF1.4) (xid=0xd):
Error OFPBRC_EPERM for: OFPT_FLOW_MOD (OF1.4) (xid=0xb): ADD table:254 actions=drop
Error OFPBFC_MSG_FAILED for: OFPT_BUNDLE_CONTROL (OF1.4) (xid=0xe):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
ovs-ofctl: talking to unix:br0.mgmt (error)
])
AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
@@ -4894,6 +4896,8 @@ OFPT_FLOW_MOD (OF1.3): ADD in_port=2,dl_src=00:88:99:aa:bb:cc idle:70 actions=ou
vconn|DBG|unix: received: ONFT_BUNDLE_ADD_MESSAGE (OF1.3):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.3): DEL table:255 in_port=2,dl_src=00:88:99:aa:bb:cc actions=drop
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.3):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.3):
vconn|DBG|unix: received: ONFT_BUNDLE_CONTROL (OF1.3):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): ONFT_BUNDLE_CONTROL (OF1.3):
@@ -4925,6 +4929,8 @@ OFPT_FLOW_MOD (OF1.3): MOD actions=drop
vconn|DBG|unix: received: ONFT_BUNDLE_ADD_MESSAGE (OF1.3):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.3): MOD_STRICT in_port=2,dl_src=00:77:88:99:aa:bb actions=output:7
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.3):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.3):
vconn|DBG|unix: received: ONFT_BUNDLE_CONTROL (OF1.3):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): ONFT_BUNDLE_CONTROL (OF1.3):
@@ -4959,6 +4965,8 @@ OFPT_FLOW_MOD (OF1.3): DEL_STRICT table:255 in_port=2,dl_src=00:66:77:88:99:aa a
vconn|DBG|unix: received: ONFT_BUNDLE_ADD_MESSAGE (OF1.3):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.3): ADD in_port=2,dl_src=00:66:77:88:99:aa actions=drop
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.3):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.3):
vconn|DBG|unix: received: ONFT_BUNDLE_CONTROL (OF1.3):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): ONFT_BUNDLE_CONTROL (OF1.3):
@@ -4981,7 +4989,7 @@ OVS_VSWITCHD_STOP
AT_CLEANUP
AT_SETUP([ofproto - failing bundle commit (OpenFlow 1.3)])
AT_SETUP([ofproto - failing bundle add message (OpenFlow 1.3)])
AT_KEYWORDS([monitor])
OVS_VSWITCHD_START
@@ -5013,15 +5021,11 @@ delete in_port=2 dl_src=00:88:99:aa:bb:cc
add table=254 actions=drop
])
AT_CHECK([ovs-ofctl -O OpenFlow13 --bundle add-flows br0 flows.txt 2>&1 | sed '/|WARN|/d
s/unix:.*br0\.mgmt/unix:br0.mgmt/' | sed 's/(.* error)/(error)/'],
AT_CHECK([ovs-ofctl -O OpenFlow13 --bundle add-flows br0 flows.txt 2>&1 | sed '/talking to/,$d'],
[0], [dnl
OFPT_ERROR (OF1.3) (xid=0xb): OFPBRC_EPERM
OFPT_FLOW_MOD (OF1.3) (xid=0xb): ADD table:254 actions=drop
OFPT_ERROR (OF1.3) (xid=0xd): OFPBFC_MSG_FAILED
ONFT_BUNDLE_CONTROL (OF1.3) (xid=0xd):
Error OFPBRC_EPERM for: OFPT_FLOW_MOD (OF1.3) (xid=0xb): ADD table:254 actions=drop
Error OFPBFC_MSG_FAILED for: ONFT_BUNDLE_CONTROL (OF1.3) (xid=0xe):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
ovs-ofctl: talking to unix:br0.mgmt (error)
])
AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl

View File

@@ -2972,6 +2972,8 @@ OFPT_FLOW_MOD (OF1.4): ADD table:7 dl_vlan=7 importance:7 actions=drop
vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.4): ADD table:8 dl_vlan=8 importance:8 actions=drop
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.4):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.4):
vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):
@@ -3019,6 +3021,8 @@ OFPT_FLOW_MOD (OF1.4): ADD table:7 dl_vlan=7 importance:17 actions=drop
vconn|DBG|unix: received: OFPT_BUNDLE_ADD_MESSAGE (OF1.4):
bundle_id=0 flags=atomic ordered
OFPT_FLOW_MOD (OF1.4): DEL_STRICT table:8 dl_vlan=8 actions=drop
vconn|DBG|unix: received: OFPT_BARRIER_REQUEST (OF1.4):
vconn|DBG|unix: sent (Success): OFPT_BARRIER_REPLY (OF1.4):
vconn|DBG|unix: received: OFPT_BUNDLE_CONTROL (OF1.4):
bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
vconn|DBG|unix: sent (Success): OFPT_BUNDLE_CONTROL (OF1.4):

View File

@@ -660,18 +660,60 @@ transact_multiple_noreply(struct vconn *vconn, struct ovs_list *requests)
ofpbuf_delete(reply);
}
/* Frees the error messages as they are printed. */
static void
bundle_error_reporter(const struct ofp_header *oh)
bundle_print_errors(struct ovs_list *errors, struct ovs_list *requests)
{
ofp_print(stderr, oh, ntohs(oh->length), verbosity + 1);
struct vconn_bundle_error *error, *next;
struct ofpbuf *bmsg;
INIT_CONTAINER(bmsg, requests, list_node);
LIST_FOR_EACH_SAFE (error, next, list_node, errors) {
enum ofperr ofperr;
struct ofpbuf payload;
ofperr = ofperr_decode_msg(&error->ofp_msg, &payload);
if (!ofperr) {
fprintf(stderr, "***decode error***");
} else {
/* Default to the likely truncated message. */
const struct ofp_header *ofp_msg = payload.data;
size_t msg_len = payload.size;
/* Find the failing message from the requests list to be able to
* dump the whole message. We assume the errors are returned in
* the same order as in which the messages are sent to get O(n)
* rather than O(n^2) processing here. If this heuristics fails we
* may print the truncated hexdumps instead. */
LIST_FOR_EACH_CONTINUE (bmsg, list_node, requests) {
const struct ofp_header *oh = bmsg->data;
if (oh->xid == error->ofp_msg.xid) {
ofp_msg = oh;
msg_len = bmsg->size;
break;
}
}
fprintf(stderr, "Error %s for: ", ofperr_get_name(ofperr));
ofp_print(stderr, ofp_msg, msg_len, verbosity + 1);
}
free(error);
}
fflush(stderr);
}
static void
bundle_transact(struct vconn *vconn, struct ovs_list *requests, uint16_t flags)
{
run(vconn_bundle_transact(vconn, requests, flags, bundle_error_reporter),
"talking to %s", vconn_get_name(vconn));
struct ovs_list errors;
int retval = vconn_bundle_transact(vconn, requests, flags, &errors);
bundle_print_errors(&errors, requests);
if (retval) {
ovs_fatal(retval, "talking to %s", vconn_get_name(vconn));
}
}
/* Sends 'request', which should be a request that only has a reply if an error