On Mon, Sep 23, 2019 at 5:09 PM Lorenzo Bianconi
<[email protected]> wrote:
>
> Introduce add_localnet_egress_interface_mappings routine in order to collect
> as
> egress interfaces all ovs bridge interfaces marked with ovn-egress-iface
> in the external_ids column of ovs interface table.
> ovn-egress-iface is used to indicate to which localnet ports QoS egress
> shaping has to be applied.
> Refactor add_bridge_mappings routine
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
Hi Lorenzo,
This change looks good to me. Just a few minor comments.
Thanks,
Dumitru
> ---
> controller/binding.c | 58 +++++++++++++++++++++++++++-
> controller/binding.h | 4 ++
> controller/ovn-controller.c | 3 +-
> controller/patch.c | 76 +++++++++++++++++++++----------------
> controller/patch.h | 4 ++
> 5 files changed, 110 insertions(+), 35 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 242163d59..89262b2d2 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -18,6 +18,7 @@
> #include "ha-chassis.h"
> #include "lflow.h"
> #include "lport.h"
> +#include "patch.h"
>
> #include "lib/bitmap.h"
> #include "openvswitch/poll-loop.h"
> @@ -532,6 +533,9 @@ consider_local_datapath(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> /* Add all localnet ports to local_lports so that we allocate ct
> zones
> * for them. */
> sset_add(local_lports, binding_rec->logical_port);
> + if (qos_map && ovs_idl_txn) {
> + get_qos_params(binding_rec, qos_map);
> + }
> } else if (!strcmp(binding_rec->type, "external")) {
> if (ha_chassis_group_contains(binding_rec->ha_chassis_group,
> chassis_rec)) {
> @@ -619,10 +623,55 @@ consider_local_datapath(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> }
> }
>
> +static void
> +add_localnet_egress_interface_mappings(
> + const struct sbrec_port_binding *port_binding,
> + struct shash *bridge_mappings, struct sset *egress_ifaces)
> +{
> + const char *network = smap_get(&port_binding->options, "network_name");
> + if (!network) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> + VLOG_ERR_RL(&rl, "%s port '%s' has no network name.",
> + port_binding->type, port_binding->logical_port);
We already log this in patch_run() -> add_bridge_mappings(). I think
we can skip the logging and just return.
> + return;
> + }
> +
> + struct ovsrec_bridge *br_ln = shash_find_data(bridge_mappings, network);
> + if (!br_ln) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> + VLOG_ERR_RL(&rl, "bridge not found for %s port '%s' "
> + "with network name '%s'", port_binding->type,
> + port_binding->logical_port, network);
Same here.
> + return;
> + }
> +
> + /* Add egress-ifaces from the connected bridge */
> + for (size_t i = 0; i < br_ln->n_ports; i++) {
> + const struct ovsrec_port *port_rec = br_ln->ports[i];
> +
> + for (size_t j = 0; j < port_rec->n_interfaces; j++) {
> + const struct ovsrec_interface *iface_rec;
> +
> + iface_rec = port_rec->interfaces[j];
> + bool is_egress_iface = smap_get_bool(&iface_rec->external_ids,
> + "ovn-egress-iface", false);
> + if (!is_egress_iface) {
> + continue;
> + }
> + sset_add(egress_ifaces, iface_rec->name);
> + }
> + }
> +}
> +
> static void
> consider_localnet_port(const struct sbrec_port_binding *binding_rec,
> + struct shash *bridge_mappings,
> + struct sset *egress_ifaces,
> struct hmap *local_datapaths)
> {
> + add_localnet_egress_interface_mappings(binding_rec,
> + bridge_mappings, egress_ifaces);
> +
> struct local_datapath *ld
> = get_local_datapath(local_datapaths,
> binding_rec->datapath->tunnel_key);
> @@ -655,6 +704,8 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> const struct ovsrec_bridge *br_int,
> const struct sbrec_chassis *chassis_rec,
> const struct sset *active_tunnels,
> + const struct ovsrec_bridge_table *bridge_table,
> + const struct ovsrec_open_vswitch_table *ovs_table,
> struct hmap *local_datapaths, struct sset *local_lports,
> struct sset *local_lport_ids)
> {
> @@ -663,6 +714,7 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> }
>
> const struct sbrec_port_binding *binding_rec;
> + struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
> struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface);
> struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces);
> struct hmap qos_map;
> @@ -688,14 +740,18 @@ binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>
> }
>
> + add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings);
> +
> /* Run through each binding record to see if it is a localnet port
> * on local datapaths discovered from above loop, and update the
> * corresponding local datapath accordingly. */
> SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) {
> if (!strcmp(binding_rec->type, "localnet")) {
> - consider_localnet_port(binding_rec, local_datapaths);
> + consider_localnet_port(binding_rec, &bridge_mappings,
> + &egress_ifaces, local_datapaths);
> }
> }
> + shash_destroy(&bridge_mappings);
>
> if (!sset_is_empty(&egress_ifaces)
> && set_noop_qos(ovs_idl_txn, port_table, qos_table, &egress_ifaces))
> {
> diff --git a/controller/binding.h b/controller/binding.h
> index bae162ede..924891c1b 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -26,6 +26,8 @@ struct ovsdb_idl_txn;
> struct ovsrec_bridge;
> struct ovsrec_port_table;
> struct ovsrec_qos_table;
> +struct ovsrec_bridge_table;
> +struct ovsrec_open_vswitch_table;
> struct sbrec_chassis;
> struct sbrec_port_binding_table;
> struct sset;
> @@ -42,6 +44,8 @@ void binding_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> const struct ovsrec_bridge *br_int,
> const struct sbrec_chassis *,
> const struct sset *active_tunnels,
> + const struct ovsrec_bridge_table *bridge_table,
> + const struct ovsrec_open_vswitch_table *ovs_table,
> struct hmap *local_datapaths,
> struct sset *local_lports, struct sset *local_lport_ids);
> bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 33ece59be..b46a1d151 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1082,7 +1082,8 @@ en_runtime_data_run(struct engine_node *node)
> sbrec_port_binding_by_name,
> port_table, qos_table, pb_table,
> br_int, chassis,
> - active_tunnels, local_datapaths,
> + active_tunnels, bridge_table,
> + ovs_table, local_datapaths,
> local_lports, local_lport_ids);
>
> update_ct_zones(local_lports, local_datapaths, ct_zones,
> diff --git a/controller/patch.c b/controller/patch.c
> index a6770c6d5..f2053de7b 100644
> --- a/controller/patch.c
> +++ b/controller/patch.c
> @@ -129,6 +129,48 @@ remove_port(const struct ovsrec_bridge_table
> *bridge_table,
> }
> }
>
> +void
> +add_ovs_bridge_mappings(const struct ovsrec_open_vswitch_table *ovs_table,
> + const struct ovsrec_bridge_table *bridge_table,
> + struct shash *bridge_mappings)
> +{
> + const struct ovsrec_open_vswitch *cfg;
> +
> + cfg = ovsrec_open_vswitch_table_first(ovs_table);
> + if (cfg) {
> + const char *mappings_cfg;
> + char *cur, *next, *start;
> +
> + mappings_cfg = smap_get(&cfg->external_ids, "ovn-bridge-mappings");
> + if (!mappings_cfg || !mappings_cfg[0]) {
> + return;
> + }
> +
> + next = start = xstrdup(mappings_cfg);
> + while ((cur = strsep(&next, ",")) && *cur) {
> + const struct ovsrec_bridge *ovs_bridge;
> + char *network, *bridge = cur;
> +
> + network = strsep(&bridge, ":");
> + if (!bridge || !*network || !*bridge) {
> + VLOG_ERR("Invalid ovn-bridge-mappings configuration: '%s'",
> + mappings_cfg);
> + break;
> + }
> +
> + ovs_bridge = get_bridge(bridge_table, bridge);
> + if (!ovs_bridge) {
> + VLOG_WARN("Bridge '%s' not found for network '%s'",
> + bridge, network);
> + continue;
> + }
> +
> + shash_add(bridge_mappings, network, ovs_bridge);
> + }
> + free(start);
> + }
> +}
> +
> /* Obtains external-ids:ovn-bridge-mappings from OVSDB and adds patch ports
> for
> * the local bridge mappings. Removes any patch ports for bridge mappings
> that
> * already existed from 'existing_ports'. */
> @@ -142,41 +184,9 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct sbrec_chassis *chassis)
> {
> /* Get ovn-bridge-mappings. */
> - const char *mappings_cfg = "";
> - const struct ovsrec_open_vswitch *cfg;
> - cfg = ovsrec_open_vswitch_table_first(ovs_table);
> - if (cfg) {
> - mappings_cfg = smap_get(&cfg->external_ids, "ovn-bridge-mappings");
> - if (!mappings_cfg || !mappings_cfg[0]) {
> - return;
> - }
> - }
> -
> - /* Parse bridge mappings. */
> struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
> - char *cur, *next, *start;
> - next = start = xstrdup(mappings_cfg);
> - while ((cur = strsep(&next, ",")) && *cur) {
> - char *network, *bridge = cur;
> - const struct ovsrec_bridge *ovs_bridge;
> -
> - network = strsep(&bridge, ":");
> - if (!bridge || !*network || !*bridge) {
> - VLOG_ERR("Invalid ovn-bridge-mappings configuration: '%s'",
> - mappings_cfg);
> - break;
> - }
>
> - ovs_bridge = get_bridge(bridge_table, bridge);
> - if (!ovs_bridge) {
> - VLOG_WARN("Bridge '%s' not found for network '%s'",
> - bridge, network);
> - continue;
> - }
> -
> - shash_add(&bridge_mappings, network, ovs_bridge);
> - }
> - free(start);
> + add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings);
>
> const struct sbrec_port_binding *binding;
> SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, port_binding_table) {
> diff --git a/controller/patch.h b/controller/patch.h
> index 9018e4967..49b0b2e90 100644
> --- a/controller/patch.h
> +++ b/controller/patch.h
> @@ -30,7 +30,11 @@ struct ovsrec_open_vswitch_table;
> struct ovsrec_port_table;
> struct sbrec_port_binding_table;
> struct sbrec_chassis;
> +struct shash;
>
> +void add_ovs_bridge_mappings(const struct ovsrec_open_vswitch_table
> *ovs_table,
> + const struct ovsrec_bridge_table *bridge_table,
> + struct shash *bridge_mappings);
> void patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct ovsrec_bridge_table *,
> const struct ovsrec_open_vswitch_table *,
> --
> 2.21.0
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
