On 10/7/22 10:26, Dumitru Ceara wrote:
> On 10/7/22 04:03, 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
>>>
>>> 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]>
>>
>> Hi Dumitru,
>>
>
> Hi Han,
>
>> In addition to the two-stage parsing concerns we are discussing in the
>
> I'm doing some more targetted performance testing. I'll update the
> other thread when I have more data to share.
>
>> other thread, I have some minor comments below. The major one is whether we
>> should allow matching hostname or not.
>>
>>> ---
>>> controller/lflow.c | 59 +++++++-
>>> controller/lflow.h | 1
>>> controller/lport.c | 3
>>> controller/ofctrl.c | 9 +
>>> controller/ofctrl.h | 3
>>> controller/ovn-controller.c | 317
>> +++++++++++++++++++++++++++++++++++++++++++
>>> include/ovn/expr.h | 4 -
>>> include/ovn/lex.h | 14 +-
>>> lib/actions.c | 9 +
>>> lib/expr.c | 18 ++
>>> lib/lex.c | 55 +++++++
>>> lib/objdep.c | 1
>>> lib/objdep.h | 1
>>> lib/ovn-util.c | 7 +
>>> lib/ovn-util.h | 3
>>> tests/ovn.at | 2
>>> tests/test-ovn.c | 16 ++
>>> utilities/ovn-trace.c | 26 +++-
>>> 18 files changed, 512 insertions(+), 36 deletions(-)
>>>
...
>>> +struct ed_type_template_vars {
>>> + struct local_templates var_table;
>>> +
>>> + bool change_tracked;
>>> + struct sset new;
>>> + struct sset deleted;
>>> + struct sset updated;
>>> +};
>>> +
>>> +static void
>>> +template_vars_init(const struct sbrec_template_var_table *tv_table,
>>> + const struct sbrec_chassis *chassis,
>>> + struct local_templates *var_table)
>>> +{
>>> + const struct sbrec_template_var *tv;
>>> + SBREC_TEMPLATE_VAR_TABLE_FOR_EACH (tv, tv_table) {
>>> + if (chassis_name_equals(tv->chassis_name, chassis)) {
>>
>> I am not sure if it is a good idea to allow using hostname to match the
>> template var name. It provides flexibility to CMS, but we will need more
>> complexity to protect against corner cases.
>> For example, if there are two records:
>> r1: name="abc", value="v1"
>> r2: hostname="abc", value="v2"
>>
>> Now with the current logic, whichever is handled later will take precedence
>> (in the local_templates.vars) and the value will be used (assume r2 "v2" is
>> used). This may be fine, because the user should be responsible for the
>> inconsistent configurations.
>>
>> Later, when the user detects the problem and wants to correct the
>> configuration. He/she deletes the r2 and expects the var "abc" to be
>> expanded as "v1". But the logic in template_vars_update() would call
>> local_templates_remove() which simply deletes the var ("abc" -> "v2")
>> instead of replacing it with ("abc" -> "v1"). The uuid of "abc" -> "v1"
>> will still be left in the uuidset, which is useless. This is an unexpected
>> behavior.
>>
>> Similar behavior would happen if there are duplicate hostnames, e.g.:
>> r1: hostname="abc", value="v1"
>> r2: hostname="abc", value="v2"
>>
>
> Very good point, nice catch!
In general it might make sense to choose a bit different database schema,
e.g.:
"Chassis_Template_Vars": {
"columns": {
"chassis": {"type": "string"},
"variables": {
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}},
"indexes": [["chassis"]],
"isRoot": true}
Here 'variables' or 'templates' or whatever you want to call it is a
Var->Value map.
Index on the 'chassis' column will provide uniqueness of chassis names,
map has unique keys, so all variable names are unique within a chassis
as well. This should cover all the possible misconfigurations on the
database level.
As a bonus we will also save a lot of database space by not storing
millions of copies of chassis and variable names. May speed up the
Nb->Sb synchronization as well.
One downside is that we can't have true I-P for that table due to
inability to track which values in a map actually changed. Though
it should be possible to figure out the diff in a semi-linear time
from the number of variables. OpenFlow rules can still be processed
incrementally after that. So, I'm not sure if that is a big performance
concern. Testing is needed, I guess.
Conditional monitoring will be very simple. Chassis will receive only
one row in most cases. update2 will cover variable updates.
What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
