On Wed, Oct 2, 2024 at 4:11 AM Ales Musil <[email protected]> wrote:
>
> On Tue, Aug 27, 2024 at 3:49 PM Xavier Simonart <[email protected]> wrote:
>
> > Do not allocate ct_zones for virtual ports.
> >
> > Signed-off-by: Xavier Simonart <[email protected]>
> > ---
> >
>
> Hi Xavier,
>
> I have one comment down below.
>
> controller/ct-zone.c | 11 +++++++++--
> > controller/ct-zone.h | 3 ++-
> > controller/ovn-controller.c | 5 ++++-
> > tests/ovn.at | 26 +++++++++++++++++++++++++-
> > 4 files changed, 40 insertions(+), 5 deletions(-)
> >
> > diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> > index 77eb16ac9..2385b700d 100644
> > --- a/controller/ct-zone.c
> > +++ b/controller/ct-zone.c
> > @@ -19,6 +19,7 @@
> > #include "binding.h"
> > #include "chassis.h"
> > #include "ct-zone.h"
> > +#include "lport.h"
> > #include "local_data.h"
> > #include "openvswitch/vlog.h"
> >
> > @@ -169,7 +170,8 @@ out:
> > void
> > ct_zones_update(const struct sset *local_lports,
> > const struct ovsrec_open_vswitch_table *ovs_table,
> > - const struct hmap *local_datapaths, struct ct_zone_ctx
> > *ctx)
> > + const struct hmap *local_datapaths, struct ct_zone_ctx
> > *ctx,
> > + struct ovsdb_idl_index *sbrec_port_binding_by_name)
> > {
> > int min_ct_zone, max_ct_zone;
> > const char *user;
> > @@ -179,8 +181,13 @@ ct_zones_update(const struct sset *local_lports,
> > struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones);
> >
> > const char *local_lport;
> > + const struct sbrec_port_binding *pb;
> > SSET_FOR_EACH (local_lport, local_lports) {
> > - sset_add(&all_users, local_lport);
> > + pb = lport_lookup_by_name(sbrec_port_binding_by_name,
> > + local_lport);
> > + if (!pb || strcmp(pb->type, "virtual")) {
> > + sset_add(&all_users, local_lport);
> > + }
> >
>
> I don't like the fact that we would have to do a lookup for each local port.
> Could we utilize "struct local_binding_data" as we do for
> "ct_zones_limits_sync()"?
I agree here. Doing a lookup for each local_lport in the sset
"local_lports" seems to be expensive to me.
I looked into the suggestion by Ales. "struct local_binding_data"
doesn't have an entry for l2gatewa, l3gateway
and localnet ports.
ports. I'm not sure of the side effects of using "struct
local_binding_data" instead of sset "local_lports".
I'd say to drop this patch. I think it's o.k to allocate a zone id
for a virtual port.
Thanks
Numan
>
>
> }
> >
> > /* Local patched datapath (gateway routers) need zones assigned. */
> > diff --git a/controller/ct-zone.h b/controller/ct-zone.h
> > index 6df03975c..fcb100619 100644
> > --- a/controller/ct-zone.h
> > +++ b/controller/ct-zone.h
> > @@ -73,7 +73,8 @@ void ct_zones_restore(struct ct_zone_ctx *ctx,
> > void ct_zones_update(const struct sset *local_lports,
> > const struct ovsrec_open_vswitch_table *ovs_table,
> > const struct hmap *local_datapaths,
> > - struct ct_zone_ctx *ctx);
> > + struct ct_zone_ctx *ctx,
> > + struct ovsdb_idl_index *sbrec_port_binding_by_name);
> > void ct_zones_commit(const struct ovsrec_bridge *br_int,
> > const struct ovsrec_datapath *ovs_dp,
> > struct ovsdb_idl_txn *ovs_idl_txn,
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index a10f1af8a..911c7f162 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1174,6 +1174,7 @@ struct ed_type_runtime_data {
> > struct shash local_active_ports_ras;
> >
> > struct sset *postponed_ports;
> > + struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > };
> >
> > /* struct ed_type_runtime_data has the below members for tracking the
> > @@ -1438,6 +1439,7 @@ en_runtime_data_run(struct engine_node *node, void
> > *data)
> >
> > binding_run(&b_ctx_in, &b_ctx_out);
> > rt_data->localnet_learn_fdb = b_ctx_out.localnet_learn_fdb;
> > + rt_data->sbrec_port_binding_by_name =
> > b_ctx_in.sbrec_port_binding_by_name;
> >
> > engine_set_node_state(node, EN_UPDATED);
> > }
> > @@ -2230,7 +2232,8 @@ en_ct_zones_run(struct engine_node *node, void *data)
> >
> > ct_zones_restore(&ct_zones_data->ctx, ovs_table, dp_table, br_int);
> > ct_zones_update(&rt_data->local_lports, ovs_table,
> > - &rt_data->local_datapaths, &ct_zones_data->ctx);
> > + &rt_data->local_datapaths, &ct_zones_data->ctx,
> > + rt_data->sbrec_port_binding_by_name);
> > ct_zones_limits_sync(&ct_zones_data->ctx, &rt_data->local_datapaths,
> > &rt_data->lbinding_data.lports);
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 4d9beea1b..ff7281e59 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -38951,8 +38951,29 @@ check ovn-nbctl lrp-add r1 r1-ls2
> > f0:00:00:00:00:02 192.168.3.1/24 \
> > -- lsp-add ls2 ls2-r1 \
> > -- set Logical_Switch_Port ls2-r1 type=router
> > options:router-port=r1-ls2 addresses=router
> >
> > +check ovn-nbctl lsp-add ls1 ls1-vir \
> > + -- lsp-set-addresses ls1-vir "50:54:00:00:00:10
> > 192.168.2.2" \
> > + -- lsp-set-type ls1-vir virtual \
> > + -- set logical_switch_port ls1-vir
> > options:virtual-ip=192.168.2.2 \
> > + -- set logical_switch_port ls1-vir
> > options:virtual-parents=lsp1
> > +
> > check ovn-nbctl --wait=hv sync
> >
> > +send_garp() {
> > + hv=$1
> > + dev=$2
> > + eth_src=$3
> > + ip=$4
> > +
> > + packet=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${eth_src}')/ \
> > + ARP(op=2, hwsrc='${eth_src}', hwdst='${eth_src}', \
> > + psrc='${ip}', pdst='${ip}')")
> > + as $hv ovs-appctl netdev-dummy/receive $dev $packet
> > +}
> > +
> > +# Sending garp to claim virtual port.
> > +send_garp hv1 vif1 "50:54:00:00:00:03" 192.168.2.2
> > +
> > AS_BOX([With patch ports])
> > zone_list_hv1=$(as hv1 ovn-appctl -t ovn-controller ct-zone-list | sort)
> > AT_CHECK([echo "$zone_list_hv1" | sed "s/ [[0-9]]*/ ??/" | sort], [0],
> > [dnl
> > @@ -39062,7 +39083,10 @@ zone_list_hv2_after_recompute=$(as hv2 ovn-appctl
> > -t ovn-controller ct-zone-list
> > AT_CHECK([test "X$zone_list_hv1" = "X$zone_list_hv1_after_recompute"])
> > AT_CHECK([test "X$zone_list_hv2" = "X$zone_list_hv2_after_recompute"])
> >
> > -OVN_CLEANUP([hv1],[hv2])
> > +
> > +OVN_CLEANUP([hv1
> > +/parse MAC binding/d
> > +],[hv2])
> >
> > AT_CLEANUP
> > ])
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> [email protected]
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
