On 12/5/22 18:54, Vladislav Odintsov wrote: > Do you mean that northd should have some kind of common code, which is used > to generate buckets?
I'm not sure if that's easy to implement indeed. > Initially I wanted to prevent the possibility even to send such a big OF > group to OVS. For all the cases: > - a new possible functionality which doesn’t limit buckets count; > - some kind of northd bug. > True, that's something we might want to take into account. > This place looks to me like a "last resort" for OF group generation. All > buckets are parsed/converted to OF syntax here. > Please correct me if I’m wrong. I think we could use the same argument you had for northd, e.g., "a new possible functionality which doesn't limit buckets count". The main reason why I'd prefer limiting the number of buckets at the source is that then a user would only have to check logs from a single place (i.e., northd) to notice that some of the buckets were not generated. OTOH, as Ilya pointed out on the other thread, we have the same issue for any OVN generated OpenFlow because there's no bound on the action size. In particular, AFAICT, the problem you're trying to fix was due to the fact that ECMP routes can have UINT_MAX paths in OVN. Such a deployment seems unreasonable and your patch 6/7 takes care of it and (implicitly) of the large OF group bucket count we obtain in that case. Maybe that's good enough for now and we can try to find a generic way of preventing ovn from installing invalid OpenFlow as a separate effort. Regards, Dumitru > > Regards, > Vladislav Odintsov > >> On 5 Dec 2022, at 19:37, Dumitru Ceara <[email protected]> wrote: >> >> On 12/2/22 18:31, Vladislav Odintsov wrote: >>> It is possible to send OpenFlow group_mod message to OVS to create a >>> group with any number of buckets: >>> >>> ovs-ofctl dump-groups br-int >>> NXST_GROUP_DESC reply (xid=0x2): >>> group_id=4,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=load:0x1->OXM_OF_PKT_REG4[48..63],resubmit(,20),...bucket=bucket_id:10000,... >>> >>> This patch introduces a limit of buckets that may be requested to 1024. >>> In case the limit is reached, ovn-controller will write WARN log about >>> this fact. >> >> Isn't it simpler to just limit the number of buckets in northd instead? >> What is the downside of doing that instead? >> >> Thanks, >> Dumitru >> >>> >>> Signed-off-by: Vladislav Odintsov <[email protected]> >>> --- >>> lib/actions.c | 40 ++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 38 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/actions.c b/lib/actions.c >>> index adbb42db4..4322556bf 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -44,6 +44,9 @@ >>> #include "controller/lflow.h" >>> >>> VLOG_DEFINE_THIS_MODULE(actions); >>> + >>> +#define MAX_BUCKETS_PER_GROUP 1024 >>> + >>> >>> /* Prototypes for functions to be defined by each action. */ >>> #define OVNACT(ENUM, STRUCT) \ >>> @@ -1371,7 +1374,18 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, >>> BUILD_ASSERT(MFF_LOG_CT_ZONE < MFF_REG0 + FLOW_N_REGS); >>> BUILD_ASSERT(MFF_LOG_DNAT_ZONE >= MFF_REG0); >>> BUILD_ASSERT(MFF_LOG_DNAT_ZONE < MFF_REG0 + FLOW_N_REGS); >>> - for (size_t bucket_id = 0; bucket_id < cl->n_dsts; bucket_id++) { >>> + >>> + int n_buckets; >>> + bool group_overflow = false; >>> + if (cl->n_dsts > MAX_BUCKETS_PER_GROUP) { >>> + n_buckets = MAX_BUCKETS_PER_GROUP; >>> + group_overflow = true; >>> + } >>> + else { >>> + n_buckets = cl->n_dsts; >>> + } >>> + >>> + for (size_t bucket_id = 0; bucket_id < n_buckets; bucket_id++) { >>> const struct ovnact_ct_lb_dst *dst = &cl->dsts[bucket_id]; >>> char ip_addr[INET6_ADDRSTRLEN]; >>> if (dst->family == AF_INET) { >>> @@ -1405,6 +1419,12 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, >>> /* Create an action to set the group. */ >>> og = ofpact_put_GROUP(ofpacts); >>> og->group_id = table_id; >>> + >>> + if (group_overflow) { >>> + VLOG_WARN("OF group id '%d' is desired to have more than " >>> + "MAX_BUCKETS_PER_GROUP (%d) buckets. Limited to >>> maximum.", >>> + table_id, MAX_BUCKETS_PER_GROUP); >>> + } >>> } >>> >>> static void >>> @@ -1542,7 +1562,17 @@ encode_SELECT(const struct ovnact_select *select, >>> >>> struct mf_subfield sf = expr_resolve_field(&select->res_field); >>> >>> - for (size_t bucket_id = 0; bucket_id < select->n_dsts; bucket_id++) { >>> + int n_buckets; >>> + bool group_overflow = false; >>> + if (select->n_dsts > MAX_BUCKETS_PER_GROUP) { >>> + n_buckets = MAX_BUCKETS_PER_GROUP; >>> + group_overflow = true; >>> + } >>> + else { >>> + n_buckets = select->n_dsts; >>> + } >>> + >>> + for (size_t bucket_id = 0; bucket_id < n_buckets; bucket_id++) { >>> const struct ovnact_select_dst *dst = &select->dsts[bucket_id]; >>> ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:%"PRIu16 >>> ",actions=", bucket_id, dst->weight); >>> @@ -1561,6 +1591,12 @@ encode_SELECT(const struct ovnact_select *select, >>> /* Create an action to set the group. */ >>> og = ofpact_put_GROUP(ofpacts); >>> og->group_id = table_id; >>> + >>> + if (group_overflow) { >>> + VLOG_WARN("OF group id '%d' is desired to have more than " >>> + "MAX_BUCKETS_PER_GROUP (%d) buckets. Limited to >>> maximum.", >>> + table_id, MAX_BUCKETS_PER_GROUP); >>> + } >>> } >>> >>> static void >> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
