Re: [ovs-dev] [PATCH ovn v7 5/6] Support logical switches with multiple localnet ports
On 5/19/20 10:05 AM, Ihar Hrachyshka wrote: My understanding is that we assume native C99 _Bool in our compiler (not a typedef / macro mapped onto int) Oh I was too quick to assume that. As per coding-style.rst, Also, don't assume that a conversion to ``bool`` or ``_Bool`` follows C99 semantics, i.e. use ``(bool) (some_value != 0)`` rather than ``(bool) some_value``. The latter might produce unexpected results on non-C99 environments. For example, if ``bool`` is implemented as a typedef of char and ``some_value = 0x1000``. So we cannot rely on _Bool being a proper C99 boolean, and then your suggestion becomes extremely important just from safety perspective! Honestly, using booleans *without* C99 semantics is quite dangerous, so I'm surprised they are used at all. TIL. I will push v8 in an hour. Ihar ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v7 5/6] Support logical switches with multiple localnet ports
On 5/19/20 8:34 AM, Numan Siddique wrote:
On Wed, May 13, 2020 at 7:10 PM Ihar Hrachyshka wrote:
Assuming only a single localnet port is actually plugged mapped on
each chassis, this allows to maintain disjoint networks plugged to the
same switch. This is useful to simplify resource management for
OpenStack "routed provider networks" feature [1] where a single
"network" (which traditionally maps to logical switches in OVN) is
comprised of multiple L2 segments and assumes external L3 routing
implemented between the segments.
[1]:
https://docs.openstack.org/ocata/networking-guide/config-routed-networks.html
Signed-off-by: Ihar Hrachyshka
Hi Ihar,
Please see below. I've 2 small comments.
With the first comment addressed
Acked-by: Numan Siddique
Since Dumitru has already acked your patches, can you please put an
"Acked-by" tag for Dumitru
and also mine too when you spin up v8.
Thanks for review!
I'll push v8 once I understand the nature of the first request to change
(see below).
Thanks
Numan
---
controller/binding.c | 16 ++
controller/patch.c | 14 +-
northd/ovn-northd.c| 62 +++--
ovn-architecture.7.xml | 50 ++--
ovn-nb.xml | 23 +-
ovn-sb.xml | 21 +-
tests/ovn.at | 504 +
7 files changed, 637 insertions(+), 53 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 9d37a23cc..774c6f242 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -683,12 +683,28 @@ add_localnet_egress_interface_mappings(
}
}
+static bool
+is_network_plugged(const struct sbrec_port_binding *binding_rec,
+ struct shash *bridge_mappings)
+{
+const char *network = smap_get(&binding_rec->options, "network_name");
+if (!network) {
+return false;
+}
+return shash_find_data(bridge_mappings, network);
shash_find_data returns (void *) and not bool.
I'd suggest to have like
return network ? !!shash_find_data(bridge_mappings, network) : false;
It's not hard to change that, but I'm slightly puzzled and wonder if I
miss something in the picture that I should grasp before I adjust the
code without proper understanding. Let me elaborate.
My understanding is that we assume native C99 _Bool in our compiler (not
a typedef / macro mapped onto int). If that's the case, AFAIU C99
standard requires implicit conversion of pointer types to booleans, and
defines it safely (meaning, a NULL-pointer always maps to false
regardless of actual representation of the NULL-pointer in memory).
Considering this, your suggestion seems effectively the same as what is
already written, just more "traditional" (for example, it would be safer
in C89 environment). Which particular issue do you think we are to
tackle here?
+}
+
static void
consider_localnet_port(const struct sbrec_port_binding *binding_rec,
struct shash *bridge_mappings,
struct sset *egress_ifaces,
struct hmap *local_datapaths)
{
+/* Ignore localnet ports for unplugged networks. */
+if (!is_network_plugged(binding_rec, bridge_mappings)) {
+return;
+}
+
add_localnet_egress_interface_mappings(binding_rec,
bridge_mappings, egress_ifaces);
diff --git a/controller/patch.c b/controller/patch.c
index 349faae17..7ad30d9cc 100644
--- a/controller/patch.c
+++ b/controller/patch.c
@@ -198,8 +198,10 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
continue;
}
+bool is_localnet = false;
const char *patch_port_id;
if (!strcmp(binding->type, "localnet")) {
+is_localnet = true;
patch_port_id = "ovn-localnet-port";
} else if (!strcmp(binding->type, "l2gateway")) {
if (!binding->chassis
@@ -224,9 +226,15 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
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'",
-binding->type, binding->logical_port, network);
+if (!is_localnet) {
+VLOG_ERR_RL(&rl, "bridge not found for %s port '%s' "
+"with network name '%s'",
+binding->type, binding->logical_port, network);
+} else {
+VLOG_INFO_RL(&rl, "bridge not found for localnet port
'%s' "
+"with network name '%s'; skipping",
+binding->logical_port, network);
+}
continue;
}
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 5e7796c5b..2308b10e6 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -543,7 +54
Re: [ovs-dev] [PATCH ovn v7 5/6] Support logical switches with multiple localnet ports
On Wed, May 13, 2020 at 7:10 PM Ihar Hrachyshka wrote:
> Assuming only a single localnet port is actually plugged mapped on
> each chassis, this allows to maintain disjoint networks plugged to the
> same switch. This is useful to simplify resource management for
> OpenStack "routed provider networks" feature [1] where a single
> "network" (which traditionally maps to logical switches in OVN) is
> comprised of multiple L2 segments and assumes external L3 routing
> implemented between the segments.
>
> [1]:
> https://docs.openstack.org/ocata/networking-guide/config-routed-networks.html
>
> Signed-off-by: Ihar Hrachyshka
>
Hi Ihar,
Please see below. I've 2 small comments.
With the first comment addressed
Acked-by: Numan Siddique
Since Dumitru has already acked your patches, can you please put an
"Acked-by" tag for Dumitru
and also mine too when you spin up v8.
Thanks
Numan
---
> controller/binding.c | 16 ++
> controller/patch.c | 14 +-
> northd/ovn-northd.c| 62 +++--
> ovn-architecture.7.xml | 50 ++--
> ovn-nb.xml | 23 +-
> ovn-sb.xml | 21 +-
> tests/ovn.at | 504 +
> 7 files changed, 637 insertions(+), 53 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 9d37a23cc..774c6f242 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -683,12 +683,28 @@ add_localnet_egress_interface_mappings(
> }
> }
>
> +static bool
> +is_network_plugged(const struct sbrec_port_binding *binding_rec,
> + struct shash *bridge_mappings)
> +{
> +const char *network = smap_get(&binding_rec->options, "network_name");
> +if (!network) {
> +return false;
> +}
> +return shash_find_data(bridge_mappings, network);
>
shash_find_data returns (void *) and not bool.
I'd suggest to have like
return network ? !!shash_find_data(bridge_mappings, network) : false;
+}
> +
> static void
> consider_localnet_port(const struct sbrec_port_binding *binding_rec,
> struct shash *bridge_mappings,
> struct sset *egress_ifaces,
> struct hmap *local_datapaths)
> {
> +/* Ignore localnet ports for unplugged networks. */
> +if (!is_network_plugged(binding_rec, bridge_mappings)) {
> +return;
> +}
> +
> add_localnet_egress_interface_mappings(binding_rec,
> bridge_mappings, egress_ifaces);
>
> diff --git a/controller/patch.c b/controller/patch.c
> index 349faae17..7ad30d9cc 100644
> --- a/controller/patch.c
> +++ b/controller/patch.c
> @@ -198,8 +198,10 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
> continue;
> }
>
> +bool is_localnet = false;
> const char *patch_port_id;
> if (!strcmp(binding->type, "localnet")) {
> +is_localnet = true;
> patch_port_id = "ovn-localnet-port";
> } else if (!strcmp(binding->type, "l2gateway")) {
> if (!binding->chassis
> @@ -224,9 +226,15 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
> 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'",
> -binding->type, binding->logical_port, network);
> +if (!is_localnet) {
> +VLOG_ERR_RL(&rl, "bridge not found for %s port '%s' "
> +"with network name '%s'",
> +binding->type, binding->logical_port, network);
> +} else {
> +VLOG_INFO_RL(&rl, "bridge not found for localnet port
> '%s' "
> +"with network name '%s'; skipping",
> +binding->logical_port, network);
> +}
> continue;
> }
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 5e7796c5b..2308b10e6 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -543,7 +543,9 @@ struct ovn_datapath {
> /* The "derived" OVN port representing the instance of l3dgw_port on
> * the "redirect-chassis". */
> struct ovn_port *l3redirect_port;
> -struct ovn_port *localnet_port;
> +
> +struct ovn_port **localnet_ports;
> +size_t n_localnet_ports;
>
> struct ovs_list lr_list; /* In list of logical router datapaths. */
> /* The logical router group to which this datapath belongs.
> @@ -611,6 +613,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
> ovn_destroy_tnlids(&od->port_tnlids);
> bitmap_free(od->ipam_info.allocated_ipv4s);
> free(od->router_ports);
> +free(od->localnet_ports);
> ovn_ls_port_group_destroy(&od->nb_pgs);
> destroy_mcast
[ovs-dev] [PATCH ovn v7 5/6] Support logical switches with multiple localnet ports
Assuming only a single localnet port is actually plugged mapped on
each chassis, this allows to maintain disjoint networks plugged to the
same switch. This is useful to simplify resource management for
OpenStack "routed provider networks" feature [1] where a single
"network" (which traditionally maps to logical switches in OVN) is
comprised of multiple L2 segments and assumes external L3 routing
implemented between the segments.
[1]:
https://docs.openstack.org/ocata/networking-guide/config-routed-networks.html
Signed-off-by: Ihar Hrachyshka
---
controller/binding.c | 16 ++
controller/patch.c | 14 +-
northd/ovn-northd.c| 62 +++--
ovn-architecture.7.xml | 50 ++--
ovn-nb.xml | 23 +-
ovn-sb.xml | 21 +-
tests/ovn.at | 504 +
7 files changed, 637 insertions(+), 53 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 9d37a23cc..774c6f242 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -683,12 +683,28 @@ add_localnet_egress_interface_mappings(
}
}
+static bool
+is_network_plugged(const struct sbrec_port_binding *binding_rec,
+ struct shash *bridge_mappings)
+{
+const char *network = smap_get(&binding_rec->options, "network_name");
+if (!network) {
+return false;
+}
+return shash_find_data(bridge_mappings, network);
+}
+
static void
consider_localnet_port(const struct sbrec_port_binding *binding_rec,
struct shash *bridge_mappings,
struct sset *egress_ifaces,
struct hmap *local_datapaths)
{
+/* Ignore localnet ports for unplugged networks. */
+if (!is_network_plugged(binding_rec, bridge_mappings)) {
+return;
+}
+
add_localnet_egress_interface_mappings(binding_rec,
bridge_mappings, egress_ifaces);
diff --git a/controller/patch.c b/controller/patch.c
index 349faae17..7ad30d9cc 100644
--- a/controller/patch.c
+++ b/controller/patch.c
@@ -198,8 +198,10 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
continue;
}
+bool is_localnet = false;
const char *patch_port_id;
if (!strcmp(binding->type, "localnet")) {
+is_localnet = true;
patch_port_id = "ovn-localnet-port";
} else if (!strcmp(binding->type, "l2gateway")) {
if (!binding->chassis
@@ -224,9 +226,15 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
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'",
-binding->type, binding->logical_port, network);
+if (!is_localnet) {
+VLOG_ERR_RL(&rl, "bridge not found for %s port '%s' "
+"with network name '%s'",
+binding->type, binding->logical_port, network);
+} else {
+VLOG_INFO_RL(&rl, "bridge not found for localnet port '%s' "
+"with network name '%s'; skipping",
+binding->logical_port, network);
+}
continue;
}
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 5e7796c5b..2308b10e6 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -543,7 +543,9 @@ struct ovn_datapath {
/* The "derived" OVN port representing the instance of l3dgw_port on
* the "redirect-chassis". */
struct ovn_port *l3redirect_port;
-struct ovn_port *localnet_port;
+
+struct ovn_port **localnet_ports;
+size_t n_localnet_ports;
struct ovs_list lr_list; /* In list of logical router datapaths. */
/* The logical router group to which this datapath belongs.
@@ -611,6 +613,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
ovn_datapath *od)
ovn_destroy_tnlids(&od->port_tnlids);
bitmap_free(od->ipam_info.allocated_ipv4s);
free(od->router_ports);
+free(od->localnet_ports);
ovn_ls_port_group_destroy(&od->nb_pgs);
destroy_mcast_info_for_datapath(od);
@@ -2025,6 +2028,7 @@ join_logical_ports(struct northd_context *ctx,
struct ovn_datapath *od;
HMAP_FOR_EACH (od, key_node, datapaths) {
if (od->nbs) {
+size_t allocated_localnet_ports = 0;
for (size_t i = 0; i < od->nbs->n_ports; i++) {
const struct nbrec_logical_switch_port *nbsp
= od->nbs->ports[i];
@@ -2059,7 +2063,12 @@ join_logical_ports(struct northd_context *ctx,
}
if (!strcmp(nbsp->type, "localnet")) {
- od->localnet_port = op;
+ if (od->n_localnet_ports >= allocated_localnet_ports) {
+
