On 7/31/24 11:05, Dumitru Ceara wrote:
> From: Ales Musil <[email protected]>
>
> Currently, OVN would generate up to 2 flows per sample, depending
> on the configuration. Add optimization that can reduce the number
> of flows added into the ACL pipeline down to 3 per collector. This
> optimization can be achieved only when the sample action with
> registers is supported in OvS and the sample has only single
> collector. The single collector per sample should be the case
> in most configurations, usually even the same collector
> for all samples which greatly reduces the number of flows per
> ACL with sampling.
>
> If there are more collectors per sample or the OvS feature is not
> supported, the implementation will fall back to flows per sample.
>
> Reported-at: https://issues.redhat.com/browse/FDP-709
> Signed-off-by: Ales Musil <[email protected]>
> ---
> include/ovn/logical-fields.h | 2 +
> lib/logical-fields.c | 8 +
> northd/northd.c | 256 +++++++++++++++++++-------
> tests/ovn-northd.at | 341 ++++++++++++++++++++++++++++++-----
> tests/ovn.at | 2 +
> tests/system-ovn.at | 10 +-
> 6 files changed, 509 insertions(+), 110 deletions(-)
Not a full review, but a couple comments inline.
>
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index ce79b501cf..d6c4a9b6b3 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -197,6 +197,8 @@ const struct ovn_field *ovn_field_from_name(const char
> *name);
> #define OVN_CT_NATTED_BIT 1
> #define OVN_CT_LB_SKIP_SNAT_BIT 2
> #define OVN_CT_LB_FORCE_SNAT_BIT 3
> +#define OVN_CT_OBS_STAGE_1ST_BIT 4
> +#define OVN_CT_OBS_STAGE_END_BIT 5
>
> #define OVN_CT_BLOCKED 1
> #define OVN_CT_NATTED 2
> diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> index 0c187e1c84..134d2674fd 100644
> --- a/lib/logical-fields.c
> +++ b/lib/logical-fields.c
> @@ -165,6 +165,14 @@ ovn_init_symtab(struct shash *symtab)
> OVN_CT_STR(OVN_CT_LB_FORCE_SNAT_BIT)
> "]",
> WR_CT_COMMIT);
> + expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_stage", NULL,
> + "ct_mark["
> + OVN_CT_STR(OVN_CT_OBS_STAGE_1ST_BIT) ".."
> + OVN_CT_STR(OVN_CT_OBS_STAGE_END_BIT)
> + "]",
> + WR_CT_COMMIT);
> + expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_collector_id", NULL,
> + "ct_mark[16..23]", WR_CT_COMMIT);
>
> expr_symtab_add_field_scoped(symtab, "ct_label", MFF_CT_LABEL, NULL,
> false, WR_CT_COMMIT);
> diff --git a/northd/northd.c b/northd/northd.c
> index 6bf3ca4d8c..fd9cfe82db 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -144,8 +144,20 @@ static bool vxlan_mode;
> #define REGBIT_ACL_VERDICT_ALLOW "reg8[16]"
> #define REGBIT_ACL_VERDICT_DROP "reg8[17]"
> #define REGBIT_ACL_VERDICT_REJECT "reg8[18]"
> +#define REGBIT_ACL_OBS_STAGE "reg8[19..20]"
> #define REG_ACL_TIER "reg8[30..31]"
>
> +enum acl_observation_stage {
> + ACL_OBS_FROM_LPORT,
> + ACL_OBS_FROM_LPORT_AFTER_LB,
> + ACL_OBS_TO_LPORT,
> + ACL_OBS_STAGE_MAX
> +};
Values of this enum end up in the database as part of actions.
It's better to define exact values for them.
<snip>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 52b2b84d7b..a9cb54b452 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -7724,7 +7724,7 @@ NS_CHECK_EXEC([sw0-p3], [ping -q -c 10 -i 0.3 -w 15
> 10.0.0.2 | FORMAT_PING], \
> AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \
> sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \
> sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'],
> [0], [dnl
What's the point of replacing with 24 zeroes?
Maybe just 0x4d3 ?
Would be nice to check that we matched 24 characters, but
I'm not sure if there is a nice portable way to do that.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
