Hi Ales
Thanks for the patch!
On Tue, Jan 28, 2025 at 10:45 AM Ales Musil <amu...@redhat.com> wrote:
> The registers used in actions and lflow created by controller are
> hardcoded, which causes upgrade issues. When the register are changed
> the "old" version might be still propagated by northd until upgraded.
>
> To prevent that add flag that will be populated only by northd that
> is using the "new" version of registers. The controller checks
> this flag and uses proper registers in lflows/actions.
>
> Reported-by: Xavier Simonart <xsimo...@redhat.com>
> Suggested-by: Dumitru Ceara <dce...@redhat.com>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> ---
> controller/lflow.c | 52 +++++++++++++++++++++++-------------
> controller/lflow.h | 1 +
> controller/ovn-controller.c | 19 +++++++++++++
> include/ovn/actions.h | 4 +++
> include/ovn/logical-fields.h | 7 +++++
> lib/actions.c | 10 +++++--
> northd/en-global-config.c | 4 +++
> tests/test-ovn.c | 1 +
> 8 files changed, 78 insertions(+), 20 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 856261f40..a77d07d22 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -100,7 +100,8 @@ consider_logical_flow(const struct sbrec_logical_flow
> *lflow,
> static void
> consider_lb_hairpin_flows(const struct ovn_controller_lb *lb,
> const struct hmap *local_datapaths,
> - struct ovn_desired_flow_table *flow_table);
> + struct ovn_desired_flow_table *flow_table,
> + bool register_consolidation);
>
> static void add_port_sec_flows(const struct shash *binding_lports,
> const struct sbrec_chassis *,
> @@ -878,6 +879,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> .lflow_uuid = lflow->header_.uuid,
> .dp_key = ldp->datapath->tunnel_key,
> .explicit_arp_ns_output = l_ctx_in->explicit_arp_ns_output,
> + .register_consolidation = l_ctx_in->register_consolidation,
>
> .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
> .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
> @@ -1645,7 +1647,8 @@ static void
> add_lb_vip_hairpin_flows(const struct ovn_controller_lb *lb,
> struct ovn_lb_vip *lb_vip,
> struct ovn_lb_backend *lb_backend,
> - struct ovn_desired_flow_table *flow_table)
> + struct ovn_desired_flow_table *flow_table,
> + bool register_consolidation)
> {
> uint64_t stub[1024 / 8];
> struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
> @@ -1677,9 +1680,10 @@ add_lb_vip_hairpin_flows(const struct
> ovn_controller_lb *lb,
> if (!lb->hairpin_orig_tuple) {
> match_set_ct_nw_dst(&hairpin_match, vip4);
> } else {
> - match_set_reg(&hairpin_match,
> - MFF_LOG_LB_ORIG_DIP_IPV4 - MFF_LOG_REG0,
> - ntohl(vip4));
> + enum mf_field_id id = register_consolidation
> + ? MFF_LOG_LB_ORIG_DIP_IPV4
> + : MFF_LOG_LB_ORIG_DIP_IPV4_OLD;
> + match_set_reg(&hairpin_match, id - MFF_LOG_REG0, ntohl(vip4));
> }
>
> add_lb_vip_hairpin_reply_action(NULL, snat_vip4, lb->proto,
> @@ -1788,7 +1792,8 @@ static void
> add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb,
> const struct ovn_lb_vip *lb_vip,
> const struct hmap *local_datapaths,
> - struct ovn_desired_flow_table *flow_table)
> + struct ovn_desired_flow_table *flow_table,
> + bool register_consolidation)
> {
> uint64_t stub[1024 / 8];
> struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
> @@ -1845,8 +1850,10 @@ add_lb_ct_snat_hairpin_vip_flow(const struct
> ovn_controller_lb *lb,
> if (!lb->hairpin_orig_tuple) {
> match_set_ct_nw_dst(&match, vip4);
> } else {
> - match_set_reg(&match, MFF_LOG_LB_ORIG_DIP_IPV4 - MFF_LOG_REG0,
> - ntohl(vip4));
> + enum mf_field_id id = register_consolidation
> + ? MFF_LOG_LB_ORIG_DIP_IPV4
> + : MFF_LOG_LB_ORIG_DIP_IPV4_OLD;
> + match_set_reg(&match, id - MFF_LOG_REG0, ntohl(vip4));
> }
> } else {
> match_set_dl_type(&match, htons(ETH_TYPE_IPV6));
> @@ -1922,7 +1929,8 @@ add_lb_ct_snat_hairpin_vip_flow(const struct
> ovn_controller_lb *lb,
> static void
> add_lb_ct_snat_hairpin_flows(const struct ovn_controller_lb *lb,
> const struct hmap *local_datapaths,
> - struct ovn_desired_flow_table *flow_table)
> + struct ovn_desired_flow_table *flow_table,
> + bool register_consolidation)
> {
> /* We must add a flow for each LB VIP. In the general case, this flow
> is added to the OFTABLE_CT_SNAT_HAIRPIN table. If it matches, we
> @@ -1956,14 +1964,15 @@ add_lb_ct_snat_hairpin_flows(const struct
> ovn_controller_lb *lb,
>
> for (int i = 0; i < lb->n_vips; i++) {
> add_lb_ct_snat_hairpin_vip_flow(lb, &lb->vips[i], local_datapaths,
> - flow_table);
> + flow_table,
> register_consolidation);
> }
> }
>
> static void
> consider_lb_hairpin_flows(const struct ovn_controller_lb *lb,
> const struct hmap *local_datapaths,
> - struct ovn_desired_flow_table *flow_table)
> + struct ovn_desired_flow_table *flow_table,
> + bool register_consolidation)
> {
> for (size_t i = 0; i < lb->n_vips; i++) {
> struct ovn_lb_vip *lb_vip = &lb->vips[i];
> @@ -1971,11 +1980,13 @@ consider_lb_hairpin_flows(const struct
> ovn_controller_lb *lb,
> for (size_t j = 0; j < lb_vip->n_backends; j++) {
> struct ovn_lb_backend *lb_backend = &lb_vip->backends[j];
>
> - add_lb_vip_hairpin_flows(lb, lb_vip, lb_backend, flow_table);
> + add_lb_vip_hairpin_flows(lb, lb_vip, lb_backend, flow_table,
> + register_consolidation);
> }
> }
>
> - add_lb_ct_snat_hairpin_flows(lb, local_datapaths, flow_table);
> + add_lb_ct_snat_hairpin_flows(lb, local_datapaths, flow_table,
> + register_consolidation);
> }
>
> /* Adds OpenFlow flows to flow tables for each Load balancer VIPs and
> @@ -1983,11 +1994,13 @@ consider_lb_hairpin_flows(const struct
> ovn_controller_lb *lb,
> static void
> add_lb_hairpin_flows(const struct hmap *local_lbs,
> const struct hmap *local_datapaths,
> - struct ovn_desired_flow_table *flow_table)
> + struct ovn_desired_flow_table *flow_table,
> + bool register_consolidation)
> {
> const struct ovn_controller_lb *lb;
> HMAP_FOR_EACH (lb, hmap_node, local_lbs) {
> - consider_lb_hairpin_flows(lb, local_datapaths, flow_table);
> + consider_lb_hairpin_flows(lb, local_datapaths, flow_table,
> + register_consolidation);
> }
> }
>
> @@ -2147,7 +2160,8 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
> lflow_ctx_out *l_ctx_out)
> l_ctx_out->flow_table);
> add_lb_hairpin_flows(l_ctx_in->local_lbs,
> l_ctx_in->local_datapaths,
> - l_ctx_out->flow_table);
> + l_ctx_out->flow_table,
> + l_ctx_in->register_consolidation);
> add_fdb_flows(l_ctx_in->fdb_table, l_ctx_in->local_datapaths,
> l_ctx_out->flow_table,
> l_ctx_in->sbrec_port_binding_by_key,
> @@ -2401,7 +2415,8 @@ lflow_handle_changed_lbs(struct lflow_ctx_in
> *l_ctx_in,
> UUID_FMT, UUID_ARGS(&uuid_node->uuid));
> ofctrl_remove_flows(l_ctx_out->flow_table, &uuid_node->uuid);
> consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
> - l_ctx_out->flow_table);
> + l_ctx_out->flow_table,
> + l_ctx_in->register_consolidation);
> }
>
> UUIDSET_FOR_EACH (uuid_node, new_lbs) {
> @@ -2410,7 +2425,8 @@ lflow_handle_changed_lbs(struct lflow_ctx_in
> *l_ctx_in,
> VLOG_DBG("Add load balancer hairpin flows for "UUID_FMT,
> UUID_ARGS(&uuid_node->uuid));
> consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
> - l_ctx_out->flow_table);
> + l_ctx_out->flow_table,
> + l_ctx_in->register_consolidation);
> }
>
> return true;
> diff --git a/controller/lflow.h b/controller/lflow.h
> index 206328f9e..93a9f3b7e 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -135,6 +135,7 @@ struct lflow_ctx_in {
> bool localnet_learn_fdb;
> bool localnet_learn_fdb_changed;
> bool explicit_arp_ns_output;
> + bool register_consolidation;
> };
>
> struct lflow_ctx_out {
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 910963e25..63c787bde 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3520,6 +3520,7 @@ struct ed_type_northd_options {
> * switch (i.e with localnet port) should
> * be tunnelled or sent via the localnet
> * port. Default value is 'false'. */
> + bool register_consolidation;
> };
>
>
> @@ -3557,6 +3558,12 @@ en_northd_options_run(struct engine_node *node,
> void *data)
> false)
> : false;
>
> + n_opts->register_consolidation =
> + sb_global
> + ? smap_get_bool(&sb_global->options, "register_consolidation",
> + false)
> + : false;
> +
> engine_set_node_state(node, EN_UPDATED);
> }
>
> @@ -3591,6 +3598,17 @@ en_northd_options_sb_sb_global_handler(struct
> engine_node *node, void *data)
> engine_set_node_state(node, EN_UPDATED);
> }
>
> + bool register_consolidation =
> + sb_global
> + ? smap_get_bool(&sb_global->options, "register_consolidation",
> + false)
> + : false;
> +
> + if (register_consolidation != n_opts->register_consolidation) {
> + n_opts->register_consolidation = register_consolidation;
> + engine_set_node_state(node, EN_UPDATED);
> + }
> +
> return true;
> }
>
> @@ -3820,6 +3838,7 @@ init_lflow_ctx(struct engine_node *node,
> l_ctx_in->localnet_learn_fdb_changed =
> rt_data->localnet_learn_fdb_changed;
> l_ctx_in->chassis_tunnels = &non_vif_data->chassis_tunnels;
> l_ctx_in->explicit_arp_ns_output = n_opts->explicit_arp_ns_output;
> + l_ctx_in->register_consolidation = n_opts->register_consolidation;
> l_ctx_in->nd_ra_opts = &fo->nd_ra_opts;
> l_ctx_in->dhcp_opts = &dhcp_opts->v4_opts;
> l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts;
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index db7342f1d..3c3d06570 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -893,6 +893,10 @@ struct ovnact_encode_params {
> /* Indication if we should add explicit output after arp/nd_ns
> action. */
> bool explicit_arp_ns_output;
>
> + /* Indication if we should use the "old" version of registers. */
> + bool register_consolidation;
> +
> +
> /* OVN maps each logical flow table (ltable), one-to-one, onto a
> physical
> * OpenFlow flow table (ptable). A number of parameters describe this
> * mapping and data related to flow tables:
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index 6a87fc386..f853b1f61 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -64,6 +64,13 @@ enum ovn_controller_event {
> #define MFF_LOG_CT_ORIG_TP_DST_PORT MFF_REG2 /*
> REG_ORIG_TP_DPORT
> * (bits 0..15). */
>
> +/* Logical registers that are needed for backwards
> + * compatibility with older northd versions.
> + * XXX: All of them can be removed in 26.09. */
> +#define MFF_LOG_LB_ORIG_DIP_IPV4_OLD MFF_REG1
> +#define MFF_LOG_LB_AFF_MATCH_PORT_OLD MFF_REG8
> +#define MFF_LOG_LB_AFF_MATCH_LS_IP6_ADDR_OLD MFF_XXREG0
> +
> void ovn_init_symtab(struct shash *symtab);
>
> /* MFF_LOG_FLAGS_REG bit assignments */
> diff --git a/lib/actions.c b/lib/actions.c
> index 62e73b2c5..5dbcfe324 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -5371,7 +5371,10 @@ encode_COMMIT_LB_AFF(const struct
> ovnact_commit_lb_aff *lb_aff,
> imm_backend_ip = (union mf_value) {
> .ipv6 = lb_aff->backend,
> };
> - ol_spec->dst.field = mf_from_id(MFF_LOG_LB_AFF_MATCH_IP6_ADDR);
> + enum mf_field_id id = !ep->register_consolidation && ep->is_switch
> + ? MFF_LOG_LB_AFF_MATCH_LS_IP6_ADDR_OLD
> + : MFF_LOG_LB_AFF_MATCH_IP6_ADDR;
> + ol_spec->dst.field = mf_from_id(id);
> } else {
> ovs_be32 ip4 = in6_addr_get_mapped_ipv4(&lb_aff->backend);
> imm_backend_ip = (union mf_value) {
> @@ -5399,7 +5402,10 @@ encode_COMMIT_LB_AFF(const struct
> ovnact_commit_lb_aff *lb_aff,
> .be16 = htons(lb_aff->backend_port),
> };
>
> - ol_spec->dst.field = mf_from_id(MFF_LOG_LB_AFF_MATCH_PORT);
> + enum mf_field_id id = !ep->register_consolidation
> + ? MFF_LOG_LB_AFF_MATCH_PORT_OLD
> + : MFF_LOG_LB_AFF_MATCH_PORT;
> + ol_spec->dst.field = mf_from_id(id);
> ol_spec->dst_type = NX_LEARN_DST_LOAD;
> ol_spec->src_type = NX_LEARN_SRC_IMMEDIATE;
> ol_spec->dst.ofs = 0;
> diff --git a/northd/en-global-config.c b/northd/en-global-config.c
> index ce16c26f2..9aef9209f 100644
> --- a/northd/en-global-config.c
> +++ b/northd/en-global-config.c
> @@ -590,6 +590,10 @@ update_sb_config_options_to_sbrec(struct
> ed_type_global_config *config_data,
> * arp/nd_ns action. */
> smap_add(options, "arp_ns_explicit_output", "true");
>
> + /* Adds indication that northd has code with consolidated
> + * register usage. */
> + smap_add(options, "register_consolidation", "true");
> +
> if (!smap_equal(&sb->options, options)) {
> sbrec_sb_global_set_options(sb, options);
> }
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index b097ec084..c310e197e 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1366,6 +1366,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> .meter_table = &meter_table,
> .collector_ids = &collector_ids,
> .explicit_arp_ns_output = true,
> + .register_consolidation = true,
>
> .pipeline = OVNACT_P_INGRESS,
> .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
> --
> 2.48.1
>
> It looks good to me.
I also run some tests in an "upgrade-like" scenario (old ovn-northd with
new ovn-controller), and they look ok.
Acked-by: Xavier Simonart <xsimo...@redhat.com>
Thanks
Xavier
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
