On Wed, Oct 5, 2022 at 8:06 AM Dumitru Ceara <[email protected]> wrote: > > On 10/5/22 07:42, Han Zhou wrote: > > On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara <[email protected]> wrote: > >> > >> Expand SB.Template_Var records in two stages: > >> 1. first expand them to local values in match/action strings > >> 2. then reparse the expanded strings > > > > I haven't completed the full review of this patch yet, but I have some > > concerns with this approach. Parsing the lflow match/action is the most > > expensive operation in ovn-controller, so I wonder if this "two-stage" > > parsing would increase the cost significantly when there are a big number > > In general, it's a bit hard to predict. The way I see it today, the CMS > would use templates with the ultimate goal of reducing the number of SB > resources northd will have to generate. So, in an ideal world, I'd > expect the number of logical flows that use template variables to be > relatively low.
Even just for the LB use cases, there are several lflows for each LB service. I heard from some folks that there can be tens of thousands of services for an ovn-k8s cluster, which would translate to equal or more number of lflows with template (assuming nodeport is enabled for these services). So I think it is better not to assume the number will be low. > > Back to the current code, I just realized I forgot that I always xstrdup > the flow's match and actions strings, even if they don't contain > template references. That needs to be fixed. > +1 (although it won't solve the issue completely) > One way to do it would be a linear scan of the string, e.g., strchr(), > to see if we need to expand the template values. > I saw that in the lex function you already do that check at the beginning, but I am not sure if strchr() is expensive enough for every lflow. May need a performance profiling to prove if it is or is not a problem. > A potentially better way would be to store in the lflow itself a tag > marking it as "template". I do that for LBs already. This would move > the linear scan to ovn-northd. I'm not sure if that's great but at > least it happens only on one node instead of on all the ones that > receive the lflow. > Assuming the strchr() cost is high enough and worth optimization, moving it to ovn-northd doesn't sound like a good idea, at least for the ovn-k8s deployment, where we tried to distribute as much work as we can to each node, because the number of lflows that need to be processed by each node is much smaller than northd. In addition, northd is already a bottleneck today, adding this extra burden to ovn-northd doesn't sound good. Probably we can do a little more fine-grained in ovn-northd to help. Instead of scanning for every lflow in ovn-northd at some final stage, we can make judgement for most of the lflows when generating them - ovn-northd knows that for most of them there is no template used at all, and then for the LB related, ovn-northd can just scan the part where it is possible to have template vars, and set the tags properly. For the custom match strings such as for ACL/QoS/PBR, it can either take the cost and scan, or, it can set a tag to the lflow as "use-template=possible" and leave the scan task to ovn-controller. Of course if strchr() doesn't contribute to the overall cost significantly the above doesn't matter, and what we really care about would only be the cost of parsing and expanding the template vars. > > of lflows. Would it be better just parsing it inlined, like how address-set > > and port-group are parsed? > > > > Initially I had tried to do it like this, parsing it inlined, but the > code quickly became complex and requiring the lexer module to support > expanding and reparsing lexer tokens on the fly. > > Another issue is that we'd still have to do some post processing after > parsing a flow that uses a template variable because that variable's > instance might expand to an expression that refers to an address set or > port group. Ok, I didn't think of this use case. It does sound more flexible and powerful if template var can be expanded to anything. However, I couldn't imagine a real world use case for this at least for the moment. (Even for the example of using template vars for LB backends I don't understand the real world scenario). So, I am not sure if we should limit the template var usage to only expanding to plain values to avoid the two-phase parsing, considering that the motivation of this feature was for performance and we might not want to introduce new performance problems because of this. > > For simplicity I decided to go for the less optimized approach. This, > with the use case I had in mind (load balancers) for which we don't > create so many logical flows when using templates. > This really depends on the amount of services we want to support. Probably some scale test for ovn-controller for such scenarios can help the discussion. Thanks, Han > > Another quick comment is, shall we add some unit tests for this patch, just > > to verify the correctness of parsing template vars in matches and actions, > > probably with some corner cases such as when var is undefined, or when the > > value is unexpected at the position, etc.? > > You're right. I hurried a bit too much with this. I'll add more tests > in v2. > > > > > Thanks, > > Han > > > > Thanks, > Dumitru > > >> > >> For the case when a lflow references a Template_Var also track > >> references (similar to the ones maintained for multicast groups, address > >> sets, port_groups, port bindings). > >> > >> Signed-off-by: Dumitru Ceara <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
