| Message ID | 20200227131556.73C522033E@silver.osuosl.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,ovn,v2] ovn-controller: No bridge for localnet port is not an error | expand |
On Thu, Feb 27, 2020 at 6:46 PM Frode Nordahl <frode.nordahl@canonical.com> wrote: > > There is a pattern among CMSes to create a `localnet` port > binding without any chassis affiliation. > > It is then up to the user to configure chassis with external > connectivity by adding a mapping under the > `external_ids:ovn-bridge-mappings` key in the Open_vSwitch > table of the local OVS instance. This may be some chassis > or all chassis depending on end user requirements. > Hi Frode, Thanks for the patch. I've few comments. ovn-controller creates a patch port between br-int and provider bridge only if it is required. Let's say you create a distributed router port and set the gateway chassis (or ha chassis group) for this port, then if no ovn-bridge-mappings is configured on that gateway chassis, ovn-controller logs it as error. I think that should be fine. ovn-controller also creates a bridge mapping if you create a logical port on the provider logical switch (i.e with localnet port) and this port is claimed by a chassis. I think even in this scenario it should log as error. Why do you think it is not an error ? Probably ovn-controller should not even try to create a patch port if it is not really required. Probably we should fix the error there. What do you think ? Thanks Numan > At present `ovn-controller` will repeatedly log an error on > chassis without mapping for a localnet port while in reality > it is not an error. > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > --- > controller/patch.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/controller/patch.c b/controller/patch.c > index 349faae17..2eb62b193 100644 > --- a/controller/patch.c > +++ b/controller/patch.c > @@ -198,8 +198,13 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, > continue; > } > > + enum vlog_level level = VLL_ERR; > const char *patch_port_id; > if (!strcmp(binding->type, "localnet")) { > + /* Not having external connectivity present on all chassis is > + * a feature our user may choose to use, let's not log it as an > + * error. */ > + level = VLL_DBG; > patch_port_id = "ovn-localnet-port"; > } else if (!strcmp(binding->type, "l2gateway")) { > if (!binding->chassis > @@ -224,7 +229,7 @@ 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' " > + VLOG_RL(&rl, level, "bridge not found for %s port '%s' " > "with network name '%s'", > binding->type, binding->logical_port, network); > continue; > -- > 2.25.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Feb 28, 2020 at 5:53 PM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Feb 27, 2020 at 6:46 PM Frode Nordahl > <frode.nordahl@canonical.com> wrote: > > > > There is a pattern among CMSes to create a `localnet` port > > binding without any chassis affiliation. > > > > It is then up to the user to configure chassis with external > > connectivity by adding a mapping under the > > `external_ids:ovn-bridge-mappings` key in the Open_vSwitch > > table of the local OVS instance. This may be some chassis > > or all chassis depending on end user requirements. > > > > Hi Frode, > > Thanks for the patch. I've few comments. Thank you for swift review/response, Numan > ovn-controller creates a patch port between br-int and provider bridge only > if it is required. Let's say you create a distributed router port and > set the gateway chassis (or ha chassis group) > for this port, then if no ovn-bridge-mappings is configured on that > gateway chassis, ovn-controller logs > it as error. > > I think that should be fine. > > ovn-controller also creates a bridge mapping if you create a logical > port on the provider logical switch (i.e > with localnet port) and this port is claimed by a chassis. I think > even in this scenario it should log as error. > > Why do you think it is not an error ? Probably ovn-controller should > not even try to create a patch port if > it is not really required. Probably we should fix the error there. > What do you think ? I'm definitely for solving the problem rather than the symptoms, so it may well be I have jumped the gun on this one and made an incorrect assumption or two. I'll go back and track down when/where something changed. Cheers! -- Frode Nordahl > Thanks > Numan > > > > At present `ovn-controller` will repeatedly log an error on > > chassis without mapping for a localnet port while in reality > > it is not an error. > > > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > > --- > > controller/patch.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/controller/patch.c b/controller/patch.c > > index 349faae17..2eb62b193 100644 > > --- a/controller/patch.c > > +++ b/controller/patch.c > > @@ -198,8 +198,13 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, > > continue; > > } > > > > + enum vlog_level level = VLL_ERR; > > const char *patch_port_id; > > if (!strcmp(binding->type, "localnet")) { > > + /* Not having external connectivity present on all chassis is > > + * a feature our user may choose to use, let's not log it as an > > + * error. */ > > + level = VLL_DBG; > > patch_port_id = "ovn-localnet-port"; > > } else if (!strcmp(binding->type, "l2gateway")) { > > if (!binding->chassis > > @@ -224,7 +229,7 @@ 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' " > > + VLOG_RL(&rl, level, "bridge not found for %s port '%s' " > > "with network name '%s'", > > binding->type, binding->logical_port, network); > > continue; > > -- > > 2.25.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/patch.c b/controller/patch.c index 349faae17..2eb62b193 100644 --- a/controller/patch.c +++ b/controller/patch.c @@ -198,8 +198,13 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, continue; } + enum vlog_level level = VLL_ERR; const char *patch_port_id; if (!strcmp(binding->type, "localnet")) { + /* Not having external connectivity present on all chassis is + * a feature our user may choose to use, let's not log it as an + * error. */ + level = VLL_DBG; patch_port_id = "ovn-localnet-port"; } else if (!strcmp(binding->type, "l2gateway")) { if (!binding->chassis @@ -224,7 +229,7 @@ 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' " + VLOG_RL(&rl, level, "bridge not found for %s port '%s' " "with network name '%s'", binding->type, binding->logical_port, network); continue;
There is a pattern among CMSes to create a `localnet` port binding without any chassis affiliation. It is then up to the user to configure chassis with external connectivity by adding a mapping under the `external_ids:ovn-bridge-mappings` key in the Open_vSwitch table of the local OVS instance. This may be some chassis or all chassis depending on end user requirements. At present `ovn-controller` will repeatedly log an error on chassis without mapping for a localnet port while in reality it is not an error. Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> --- controller/patch.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)