On 9/6/23 16:39, Mark Michelson wrote: > Thanks Ales, > > Acked-by: Mark Michelson <[email protected]> >
Hi Ales, Mark, > I think this change should be applied as-is. I also wanted to contribute > some thoughts. > I shared some thoughts too, below. > As you mentioned in the commit message, this is not a self-healing > process. I was looking at the OpenFlow specification, and I am having > trouble figuring out how to properly handle this. It doesn't appear that > OpenFlow provides a way to append new actions to an existing flow or to > add new match conditions to an existing flow. The best you can do is to > replace the actions or replace the match. This doesn't help when the > match or actions are too long to send. > > The best I could think of was to break the flow up and evaluate across > multiple tables. This would work, but it would also completely break the > model for how OVN correlates OpenFlow table numbers with specific > logical stages. Are there any alternatives? > Not a real alternative solution but should we also try a (single) recompute and also clear the installed OpenFlows and install the new set? That's just in case it "fixes" the problem due to desired flows being built in a different (equivalent) way. > On 8/29/23 04:47, Ales Musil wrote: >> If the flow size is bigger than UINT16_MAX it doesn't >> fit into openflow message. Programming of such flow will >> fail which results in disconnect of ofctrl. After reconnect >> we program everything from scratch, in case the long flow still >> remains the cycle continues. This causes the node to be almost >> unusable as there will be massive traffic disruptions. >> >> To prevent that check if the flow is within the allowed size. >> If not log the flow that would trigger this problem and do not program >> it. This isn't a self-healing process, but the chance of this happening >> are very slim. Also, any flow that is bigger than allowed size is OVN >> bug, and it should be fixed. >> >> Reported-at: https://bugzilla.redhat.com/1955167 >> Signed-off-by: Ales Musil <[email protected]> >> --- >> v2: Fix the formatting error. >> --- >> controller/ofctrl.c | 40 ++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 36 insertions(+), 4 deletions(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index 64a444ff6..9de8a145f 100644 >> --- a/controller/ofctrl.c >> +++ b/controller/ofctrl.c >> @@ -1769,6 +1769,16 @@ ovn_flow_log(const struct ovn_flow *f, const >> char *action) >> } >> } >> +static void >> +ovn_flow_log_size_err(const struct ovn_flow *f) >> +{ >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> + >> + char *s = ovn_flow_to_string(f); >> + VLOG_ERR_RL(&rl, "The FLOW_MOD message is too big: %s", s); >> + free(s); It might be useful to add a coverage counter too. Regards, Dumitru >> +} >> + >> static void >> ovn_flow_uninit(struct ovn_flow *f) >> { >> @@ -1888,15 +1898,27 @@ encode_bundle_add(struct ofpbuf *msg, struct >> ofputil_bundle_ctrl_msg *bc) >> return ofputil_encode_bundle_add(OFP15_VERSION, &bam); >> } >> -static void >> +static bool >> add_flow_mod(struct ofputil_flow_mod *fm, >> struct ofputil_bundle_ctrl_msg *bc, >> struct ovs_list *msgs) >> { >> struct ofpbuf *msg = encode_flow_mod(fm); >> struct ofpbuf *bundle_msg = encode_bundle_add(msg, bc); >> + >> + uint32_t flow_mod_len = msg->size; >> + uint32_t bundle_len = bundle_msg->size; >> + >> ofpbuf_delete(msg); >> + >> + if (flow_mod_len > UINT16_MAX || bundle_len > UINT16_MAX) { >> + ofpbuf_delete(bundle_msg); >> + >> + return false; >> + } >> + >> ovs_list_push_back(msgs, &bundle_msg->list_node); >> + return true; >> } >> >> /* group_table. */ >> @@ -2235,7 +2257,10 @@ installed_flow_add(struct ovn_flow *d, >> .new_cookie = htonll(d->cookie), >> .command = OFPFC_ADD, >> }; >> - add_flow_mod(&fm, bc, msgs); >> + >> + if (!add_flow_mod(&fm, bc, msgs)) { >> + ovn_flow_log_size_err(d); >> + } >> } >> static void >> @@ -2259,7 +2284,7 @@ installed_flow_mod(struct ovn_flow *i, struct >> ovn_flow *d, >> /* Use OFPFC_ADD so that cookie can be updated. */ >> fm.command = OFPFC_ADD; >> } >> - add_flow_mod(&fm, bc, msgs); >> + bool result = add_flow_mod(&fm, bc, msgs); >> /* Replace 'i''s actions and cookie by 'd''s. */ >> mem_stats.installed_flow_usage -= i->ofpacts_len - d->ofpacts_len; >> @@ -2267,6 +2292,10 @@ installed_flow_mod(struct ovn_flow *i, struct >> ovn_flow *d, >> i->ofpacts = xmemdup(d->ofpacts, d->ofpacts_len); >> i->ofpacts_len = d->ofpacts_len; >> i->cookie = d->cookie; >> + >> + if (!result) { >> + ovn_flow_log_size_err(i); >> + } >> } >> static void >> @@ -2280,7 +2309,10 @@ installed_flow_del(struct ovn_flow *i, >> .table_id = i->table_id, >> .command = OFPFC_DELETE_STRICT, >> }; >> - add_flow_mod(&fm, bc, msgs); >> + >> + if (!add_flow_mod(&fm, bc, msgs)) { >> + ovn_flow_log_size_err(i); >> + } >> } >> static void > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
