Re: [ovs-dev] [PATCH ovn 1/2] ofctrl: Do not try to program long flows
On Tue, Aug 29, 2023 at 10:42 AM 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 > --- > controller/ofctrl.c | 74 - > 1 file changed, 53 insertions(+), 21 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 64a444ff6..bb42d474f 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); > +} > + > 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. */ > @@ -2227,15 +2249,18 @@ installed_flow_add(struct ovn_flow *d, > { > /* Send flow_mod to add flow. */ > struct ofputil_flow_mod fm = { > -.match = d->match, > -.priority = d->priority, > -.table_id = d->table_id, > -.ofpacts = d->ofpacts, > -.ofpacts_len = d->ofpacts_len, > -.new_cookie = htonll(d->cookie), > -.command = OFPFC_ADD, > +.match = d->match, > +.priority = d->priority, > +.table_id = d->table_id, > +.ofpacts = d->ofpacts, > +.ofpacts_len = d->ofpacts_len, > +.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 > @@ -2245,12 +2270,12 @@ installed_flow_mod(struct ovn_flow *i, struct > ovn_flow *d, > { > /* Update actions in installed flow. */ > struct ofputil_flow_mod fm = { > -.match = i->match, > -.priority = i->priority, > -.table_id = i->table_id, > -.ofpacts = d->ofpacts, > -.ofpacts_len = d->ofpacts_len, > -.command = OFPFC_MODIFY_STRICT, > +.match = i->match, > +.priority = i->priority, > +.table_id = i->table_id, > +.ofpacts = d->ofpacts, > +.ofpacts_len = d->ofpacts_len, > +.command = OFPFC_MODIFY_STRICT, > }; > /* Update cookie if it is changed. */ > if (i->cookie != d->cookie) { > @@ -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 > @@ -2275,12 +2304,15 @@ installed_flow_del(struct ovn_flow *i, > struct ovs_list *msgs) > { > struct ofputil_flow_mod fm = { > -.match = i->match, > -.priority = i->priority, > -.table_id = i->table_id, > -.command = OFPFC_DELETE_STRICT, > +.match = i->mat
[ovs-dev] [PATCH ovn 1/2] ofctrl: Do not try to program long flows
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 --- controller/ofctrl.c | 74 - 1 file changed, 53 insertions(+), 21 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 64a444ff6..bb42d474f 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); +} + 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. */ @@ -2227,15 +2249,18 @@ installed_flow_add(struct ovn_flow *d, { /* Send flow_mod to add flow. */ struct ofputil_flow_mod fm = { -.match = d->match, -.priority = d->priority, -.table_id = d->table_id, -.ofpacts = d->ofpacts, -.ofpacts_len = d->ofpacts_len, -.new_cookie = htonll(d->cookie), -.command = OFPFC_ADD, +.match = d->match, +.priority = d->priority, +.table_id = d->table_id, +.ofpacts = d->ofpacts, +.ofpacts_len = d->ofpacts_len, +.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 @@ -2245,12 +2270,12 @@ installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, { /* Update actions in installed flow. */ struct ofputil_flow_mod fm = { -.match = i->match, -.priority = i->priority, -.table_id = i->table_id, -.ofpacts = d->ofpacts, -.ofpacts_len = d->ofpacts_len, -.command = OFPFC_MODIFY_STRICT, +.match = i->match, +.priority = i->priority, +.table_id = i->table_id, +.ofpacts = d->ofpacts, +.ofpacts_len = d->ofpacts_len, +.command = OFPFC_MODIFY_STRICT, }; /* Update cookie if it is changed. */ if (i->cookie != d->cookie) { @@ -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 @@ -2275,12 +2304,15 @@ installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs) { struct ofputil_flow_mod fm = { -.match = i->match, -.priority = i->priority, -.table_id = i->table_id, -.command = OFPFC_DELETE_STRICT, +.match = i->match, +.priority = i->priority, +.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 -- 2.41.0 ___ d