On 5/18/23 12:56, Ihar Hrachyshka wrote:
> This will be used in a later patch to calculate the effective interface
> MTU after considering tunneling overhead.
>
> NOTE: ideally, OVN would support Logical_Port MTU, in which case we
> wouldn't have to track OVSDB for interfaces.
>
> Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
> ---
Hi Ihar,
> controller/binding.c | 4 +-
> controller/if-status.c | 36 ++++++++++++++++--
> controller/if-status.h | 6 +++
> controller/ovn-controller.c | 76 +++++++++++++++++++++++++++++++++++++
> controller/ovsport.c | 9 +++++
> controller/ovsport.h | 2 +
> controller/physical.h | 2 +
> 7 files changed, 129 insertions(+), 6 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index bd810f669..a627618b7 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1273,7 +1273,7 @@ claim_lport(const struct sbrec_port_binding *pb,
> }
> set_pb_chassis_in_sbrec(pb, chassis_rec, true);
> } else {
> - if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> + if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
> sb_readonly);
> }
> register_claim_timestamp(pb->logical_port, now);
> @@ -1288,7 +1288,7 @@ claim_lport(const struct sbrec_port_binding *pb,
> !smap_get_bool(&iface_rec->external_ids,
> OVN_INSTALLED_EXT_ID, false)) {
> if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
> - sb_readonly);
> + iface_rec, sb_readonly);
> }
> }
> }
> diff --git a/controller/if-status.c b/controller/if-status.c
> index 8503e5daa..dc4062559 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -18,12 +18,14 @@
> #include "binding.h"
> #include "if-status.h"
> #include "ofctrl-seqno.h"
> +#include "ovsport.h"
> #include "simap.h"
>
> #include "lib/hmapx.h"
> #include "lib/util.h"
> #include "timeval.h"
> #include "openvswitch/vlog.h"
> +#include "lib/vswitch-idl.h"
> #include "lib/ovn-sb-idl.h"
>
> VLOG_DEFINE_THIS_MODULE(if_status);
> @@ -181,6 +183,7 @@ struct ovs_iface {
> * be fully programmed in OVS. Only used in
> state
> * OIF_INSTALL_FLOWS.
> */
> + uint16_t mtu; /* Extracted from OVS interface.mtu field. */
> };
>
> static uint64_t ifaces_usage;
> @@ -205,9 +208,10 @@ struct if_status_mgr {
> uint32_t iface_seqno;
> };
>
> -static struct ovs_iface *ovs_iface_create(struct if_status_mgr *,
> - const char *iface_id,
> - enum if_state );
> +static struct ovs_iface *
> +ovs_iface_create(struct if_status_mgr *, const char *iface_id,
> + const struct ovsrec_interface *iface_rec,
> + enum if_state);
> static void add_to_ovn_uninstall_hash(struct if_status_mgr *, const char *,
> const struct uuid *);
> static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface *);
> @@ -272,13 +276,14 @@ void
> if_status_mgr_claim_iface(struct if_status_mgr *mgr,
> const struct sbrec_port_binding *pb,
> const struct sbrec_chassis *chassis_rec,
> + const struct ovsrec_interface *iface_rec,
> bool sb_readonly)
> {
> const char *iface_id = pb->logical_port;
> struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
>
> if (!iface) {
> - iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED);
> + iface = ovs_iface_create(mgr, iface_id, iface_rec, OIF_CLAIMED);
> }
>
> memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid));
> @@ -639,14 +644,37 @@ ovn_uninstall_hash_account_mem(const char *name, bool
> erase)
> }
> }
>
> +uint16_t
> +if_status_mgr_iface_get_mtu(const struct if_status_mgr *mgr,
> + const char *iface_id)
> +{
> + const struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> + return iface ? iface->mtu : 0;
> +}
> +
> +bool
> +if_status_mgr_iface_set_mtu(const struct if_status_mgr *mgr,
> + const char *iface_id,
> + uint16_t mtu)
> +{
> + struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> + if (iface && iface->mtu != mtu) {
> + iface->mtu = mtu;
> + return true;
> + }
> + return false;
> +}
> +
> static struct ovs_iface *
> ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id,
> + const struct ovsrec_interface *iface_rec,
> enum if_state state)
> {
> struct ovs_iface *iface = xzalloc(sizeof *iface);
>
> VLOG_DBG("Interface %s create.", iface_id);
> iface->id = xstrdup(iface_id);
> + iface->mtu = get_iface_mtu(iface_rec);
> shash_add_nocopy(&mgr->ifaces, iface->id, iface);
> ovs_iface_set_state(mgr, iface, state);
> ovs_iface_account_mem(iface_id, false);
> diff --git a/controller/if-status.h b/controller/if-status.h
> index 8ba80acd9..0ed43431f 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -30,6 +30,7 @@ void if_status_mgr_destroy(struct if_status_mgr *);
> void if_status_mgr_claim_iface(struct if_status_mgr *,
> const struct sbrec_port_binding *pb,
> const struct sbrec_chassis *chassis_rec,
> + const struct ovsrec_interface *iface_rec,
> bool sb_readonly);
> void if_status_mgr_release_iface(struct if_status_mgr *, const char
> *iface_id);
> void if_status_mgr_delete_iface(struct if_status_mgr *, const char
> *iface_id);
> @@ -56,5 +57,10 @@ bool if_status_handle_claims(struct if_status_mgr *mgr,
> void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
> const char *name,
> const struct uuid *uuid);
> +uint16_t if_status_mgr_iface_get_mtu(const struct if_status_mgr *mgr,
> + const char *iface_id);
> +bool if_status_mgr_iface_set_mtu(const struct if_status_mgr *mgr,
> + const char *iface_id,
> + uint16_t mtu);
>
> # endif /* controller/if-status.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index de90025f0..132831dde 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -60,6 +60,7 @@
> #include "lib/ovn-dirs.h"
> #include "lib/ovn-sb-idl.h"
> #include "lib/ovn-util.h"
> +#include "ovsport.h"
> #include "patch.h"
> #include "vif-plug.h"
> #include "vif-plug-provider.h"
> @@ -1056,6 +1057,7 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd_status);
> + ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_mtu);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
> @@ -4046,6 +4048,9 @@ static void init_physical_ctx(struct engine_node *node,
> const struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve =
> engine_get_input_data("mff_ovn_geneve", node);
>
> + const struct ovsrec_interface_table *ovs_interface_table =
> + EN_OVSDB_GET(engine_get_input("OVS_interface", node));
> +
> const struct ovsrec_open_vswitch_table *ovs_table =
> EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
> const struct ovsrec_bridge_table *bridge_table =
> @@ -4070,6 +4075,7 @@ static void init_physical_ctx(struct engine_node *node,
> p_ctx->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> p_ctx->sbrec_port_binding_by_datapath = sbrec_port_binding_by_datapath;
> p_ctx->port_binding_table = port_binding_table;
> + p_ctx->ovs_interface_table = ovs_interface_table;
> p_ctx->mc_group_table = multicast_group_table;
> p_ctx->br_int = br_int;
> p_ctx->chassis_table = chassis_table;
> @@ -4083,6 +4089,9 @@ static void init_physical_ctx(struct engine_node *node,
> p_ctx->patch_ofports = &non_vif_data->patch_ofports;
> p_ctx->chassis_tunnels = &non_vif_data->chassis_tunnels;
>
> + struct controller_engine_ctx *ctrl_ctx =
> engine_get_context()->client_ctx;
> + p_ctx->if_mgr = ctrl_ctx->if_mgr;
> +
> pflow_output_get_debug(node, &p_ctx->debug);
> }
>
> @@ -4126,6 +4135,71 @@ en_pflow_output_run(struct engine_node *node, void
> *data)
> engine_set_node_state(node, EN_UPDATED);
> }
>
> +static bool
> +pflow_output_ovs_interface_handler(struct engine_node *node,
> + void *data)
> +{
> + enum engine_node_state state = EN_UNCHANGED;
> +
> + struct ed_type_pflow_output *pfo = data;
> + struct ed_type_runtime_data *rt_data =
> + engine_get_input_data("runtime_data", node);
> + struct ed_type_non_vif_data *non_vif_data =
> + engine_get_input_data("non_vif_data", node);
> +
> + struct physical_ctx p_ctx;
> + init_physical_ctx(node, rt_data, non_vif_data, &p_ctx);
> +
> + const struct ovsrec_interface *iface;
> + const struct ovsrec_interface_table *ovs_interface_table =
> + p_ctx.ovs_interface_table;
> + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface, ovs_interface_table) {
> + const char *iface_id = smap_get(&iface->external_ids, "iface-id");
> + if (!iface_id) {
> + continue;
> + }
> +
> + uint16_t mtu = get_iface_mtu(iface);
> + if (!if_status_mgr_iface_set_mtu(p_ctx.if_mgr, iface_id, mtu)) {
> + continue;
> + }
> + const struct sbrec_port_binding *pb = lport_lookup_by_name(
> + p_ctx.sbrec_port_binding_by_name, iface_id);
> + if (!pb) {
> + continue;
> + }
> + if (pb->n_additional_chassis) {
> + /* Update flows for all ports in datapath. */
> + struct sbrec_port_binding *target =
> + sbrec_port_binding_index_init_row(
> + p_ctx.sbrec_port_binding_by_datapath);
> + sbrec_port_binding_index_set_datapath(target, pb->datapath);
> +
> + const struct sbrec_port_binding *binding;
> + SBREC_PORT_BINDING_FOR_EACH_EQUAL (
> + binding, target, p_ctx.sbrec_port_binding_by_datapath) {
> + bool removed = sbrec_port_binding_is_deleted(binding);
> + if (!physical_handle_flows_for_lport(binding, removed,
> &p_ctx,
> + &pfo->flow_table)) {
> + return false;
> + }
> + state = EN_UPDATED;
> + }
> + sbrec_port_binding_index_destroy_row(target);
> + } else {
> + /* If any multichassis ports, update flows for the port. */
> + bool removed = sbrec_port_binding_is_deleted(pb);
> + if (!physical_handle_flows_for_lport(pb, removed, &p_ctx,
> + &pfo->flow_table)) {
> + return false;
> + }
> + state = EN_UPDATED;
> + }
> + }
> + engine_set_node_state(node, state);
It's not OK to set the I-P node's state to EN_UNCHANGED from within an
I-P handler. We might be overwriting EN_UPDATED set by a different I-P
handler.
Note: it _is_ however OK to do this from the _run() handler.
Which brings me to the question: don't we need to call
if_status_mgr_iface_set_mtu() from the path that processes a recompute
of the pflow_output I-P node? Maybe somewhere in the
en_pflow_output_run() -> physical_run() -> consider_port_binding() call
chain?
The rest looks OK to me.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
