Message ID | 20200511170021.448353-7-ihrachys@redhat.com |
---|---|
State | Superseded, archived |
Headers | show |
Series | Support logical switches with multiple localnet ports | expand |
On 5/11/20 7:00 PM, Ihar Hrachyshka wrote: > Having some localnet ports missing a bridge device on a particular > chassis is a supported configuration (e.g. used to implement "routed > provider networks" for OpenStack) and should not flood logs with > duplicate messages. > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > --- > controller/patch.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/controller/patch.c b/controller/patch.c > index 52255cc3a..2a757bb00 100644 > --- a/controller/patch.c > +++ b/controller/patch.c > @@ -24,6 +24,7 @@ > #include "openvswitch/hmap.h" > #include "openvswitch/vlog.h" > #include "ovn-controller.h" > +#include "sset.h" > > VLOG_DEFINE_THIS_MODULE(patch); > > @@ -184,6 +185,8 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, > const struct sbrec_chassis *chassis, > const struct hmap *local_datapaths) > { > + static struct sset missed_bridges = SSET_INITIALIZER(&missed_bridges); > + > /* Get ovn-bridge-mappings. */ > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > > @@ -220,20 +223,25 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, > binding->type, binding->logical_port); > continue; > } > + char *msg_key = xasprintf("%s;%s", binding->logical_port, network); > 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); > if (!is_localnet) { > + 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); > } else { > - VLOG_INFO_RL(&rl, "bridge not found for localnet port '%s' " > - "with network name '%s'; skipping", > - binding->logical_port, network); > + if (!sset_contains(&missed_bridges, msg_key)) { > + VLOG_INFO("bridge not found for localnet port '%s' with " > + "network name '%s'; skipping", > + binding->logical_port, network); Shouldn't we rate limit this log message too? Also, I'm a bit confused about how this should work: the "<logical-port>;<network>" msg_key will generate unique values for each port binding. So, won't the condition above always be true? > + sset_add(&missed_bridges, msg_key); > + } > }> continue; > } > + sset_find_and_delete(&missed_bridges, msg_key); > We need to free msg_key in some cases, e.g., if is_localnet == false. Regards, Dumitru > const char *patch_port_id; > if (is_localnet) { >
Hi Dumitru, thanks a lot for attentive review. I'm pushing another version of the series with your comments addressed. Some answers inline. On 5/12/20 10:30 AM, Dumitru Ceara wrote: > On 5/11/20 7:00 PM, Ihar Hrachyshka wrote: >> Having some localnet ports missing a bridge device on a particular >> chassis is a supported configuration (e.g. used to implement "routed >> provider networks" for OpenStack) and should not flood logs with >> duplicate messages. >> >> Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> >> --- >> controller/patch.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/controller/patch.c b/controller/patch.c >> index 52255cc3a..2a757bb00 100644 >> --- a/controller/patch.c >> +++ b/controller/patch.c >> @@ -24,6 +24,7 @@ >> #include "openvswitch/hmap.h" >> #include "openvswitch/vlog.h" >> #include "ovn-controller.h" >> +#include "sset.h" >> >> VLOG_DEFINE_THIS_MODULE(patch); >> >> @@ -184,6 +185,8 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, >> const struct sbrec_chassis *chassis, >> const struct hmap *local_datapaths) >> { >> + static struct sset missed_bridges = SSET_INITIALIZER(&missed_bridges); >> + >> /* Get ovn-bridge-mappings. */ >> struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); >> >> @@ -220,20 +223,25 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, >> binding->type, binding->logical_port); >> continue; >> } >> + char *msg_key = xasprintf("%s;%s", binding->logical_port, network); >> 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); >> if (!is_localnet) { >> + 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); >> } else { >> - VLOG_INFO_RL(&rl, "bridge not found for localnet port '%s' " >> - "with network name '%s'; skipping", >> - binding->logical_port, network); >> + if (!sset_contains(&missed_bridges, msg_key)) { >> + VLOG_INFO("bridge not found for localnet port '%s' with " >> + "network name '%s'; skipping", >> + binding->logical_port, network); > Shouldn't we rate limit this log message too? The idea behind the set holding all keys for all unwired localnet ports is to have a single info message reported per controller run. This function is called from under the main process loop trying to reconcile database with network stack configuration. It makes sense to warn a user over and over (and hence also rate limit) when we detect an invalid configuration. But having multiple localnet ports per switch, some that belong to unmapped / unwired networks is a legal use case and not misconfiguration (since this series), so the set is used to guarantee that a user will see the info message once in controller lifetime. Hence no need for explicit rate limit. Side note that there *is* a scenario where several messages for the same localnet port will be reported. This can happen when a port was unwired (hence the first message logged), then it got wired (so the main loop reconciled the database and wired it, also clearing the msg_key from the set), then it got unwired again (the message is logged again since the set doesn't contain msg_key). While the code as written handles the scenario, I am not sure if it's a valid scenario to care about. Still, I make some effort here to handle it. > Also, I'm a bit confused about how this should work: the > "<logical-port>;<network>" msg_key will generate unique values for each > port binding. So, won't the condition above always be true? As explained in more words above, it will not be true on all executions except the very first one (note the set is static hence survives returning from the function). I've added an inline comment to explain this. >> + sset_add(&missed_bridges, msg_key); >> + } >> }> continue; >> } >> + sset_find_and_delete(&missed_bridges, msg_key); >> > We need to free msg_key in some cases, e.g., if is_localnet == false. Actually, it should be always freed since adding a key to a set clones it. I am sorry for all the embarrassing memory leaks I left, I have to be a lot more cautious about it. In my pitty excuse, this is what using gc languages for years makes to you. Oh well. Thanks again, appreciate all the comments! Ihar
On 5/12/20 6:52 PM, Ihar Hrachyshka wrote: > Hi Dumitru, > > thanks a lot for attentive review. I'm pushing another version of the > series with your comments addressed. Some answers inline. > > > On 5/12/20 10:30 AM, Dumitru Ceara wrote: >> On 5/11/20 7:00 PM, Ihar Hrachyshka wrote: >>> Having some localnet ports missing a bridge device on a particular >>> chassis is a supported configuration (e.g. used to implement "routed >>> provider networks" for OpenStack) and should not flood logs with >>> duplicate messages. >>> >>> Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> >>> --- >>> controller/patch.c | 16 ++++++++++++---- >>> 1 file changed, 12 insertions(+), 4 deletions(-) >>> >>> diff --git a/controller/patch.c b/controller/patch.c >>> index 52255cc3a..2a757bb00 100644 >>> --- a/controller/patch.c >>> +++ b/controller/patch.c >>> @@ -24,6 +24,7 @@ >>> #include "openvswitch/hmap.h" >>> #include "openvswitch/vlog.h" >>> #include "ovn-controller.h" >>> +#include "sset.h" >>> VLOG_DEFINE_THIS_MODULE(patch); >>> @@ -184,6 +185,8 @@ add_bridge_mappings(struct ovsdb_idl_txn >>> *ovs_idl_txn, >>> const struct sbrec_chassis *chassis, >>> const struct hmap *local_datapaths) >>> { >>> + static struct sset missed_bridges = >>> SSET_INITIALIZER(&missed_bridges); >>> + >>> /* Get ovn-bridge-mappings. */ >>> struct shash bridge_mappings = >>> SHASH_INITIALIZER(&bridge_mappings); >>> @@ -220,20 +223,25 @@ add_bridge_mappings(struct ovsdb_idl_txn >>> *ovs_idl_txn, >>> binding->type, binding->logical_port); >>> continue; >>> } >>> + char *msg_key = xasprintf("%s;%s", binding->logical_port, >>> network); >>> 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); >>> if (!is_localnet) { >>> + 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); >>> } else { >>> - VLOG_INFO_RL(&rl, "bridge not found for localnet >>> port '%s' " >>> - "with network name '%s'; skipping", >>> - binding->logical_port, network); >>> + if (!sset_contains(&missed_bridges, msg_key)) { >>> + VLOG_INFO("bridge not found for localnet port >>> '%s' with " >>> + "network name '%s'; skipping", >>> + binding->logical_port, network); >> Shouldn't we rate limit this log message too? > > > The idea behind the set holding all keys for all unwired localnet ports > is to have a single info message reported per controller run. This > function is called from under the main process loop trying to reconcile > database with network stack configuration. It makes sense to warn a user > over and over (and hence also rate limit) when we detect an invalid > configuration. But having multiple localnet ports per switch, some that > belong to unmapped / unwired networks is a legal use case and not > misconfiguration (since this series), so the set is used to guarantee > that a user will see the info message once in controller lifetime. Hence > no need for explicit rate limit. > > > Side note that there *is* a scenario where several messages for the same > localnet port will be reported. This can happen when a port was unwired > (hence the first message logged), then it got wired (so the main loop > reconciled the database and wired it, also clearing the msg_key from the > set), then it got unwired again (the message is logged again since the > set doesn't contain msg_key). While the code as written handles the > scenario, I am not sure if it's a valid scenario to care about. Still, I > make some effort here to handle it. > > >> Also, I'm a bit confused about how this should work: the >> "<logical-port>;<network>" msg_key will generate unique values for each >> port binding. So, won't the condition above always be true? > > > As explained in more words above, it will not be true on all executions > except the very first one (note the set is static hence survives > returning from the function). > My bad, I had missed the "static" completely. > > I've added an inline comment to explain this. > > >>> + sset_add(&missed_bridges, msg_key); >>> + } >>> }> continue; >>> } >>> + sset_find_and_delete(&missed_bridges, msg_key); >>> >> We need to free msg_key in some cases, e.g., if is_localnet == false. > > > Actually, it should be always freed since adding a key to a set clones it. > Right. > > I am sorry for all the embarrassing memory leaks I left, I have to be a > lot more cautious about it. In my pitty excuse, this is what using gc > languages for years makes to you. Oh well. > No problem :) > > Thanks again, appreciate all the comments! > > Ihar >
diff --git a/controller/patch.c b/controller/patch.c index 52255cc3a..2a757bb00 100644 --- a/controller/patch.c +++ b/controller/patch.c @@ -24,6 +24,7 @@ #include "openvswitch/hmap.h" #include "openvswitch/vlog.h" #include "ovn-controller.h" +#include "sset.h" VLOG_DEFINE_THIS_MODULE(patch); @@ -184,6 +185,8 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, const struct sbrec_chassis *chassis, const struct hmap *local_datapaths) { + static struct sset missed_bridges = SSET_INITIALIZER(&missed_bridges); + /* Get ovn-bridge-mappings. */ struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); @@ -220,20 +223,25 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, binding->type, binding->logical_port); continue; } + char *msg_key = xasprintf("%s;%s", binding->logical_port, network); 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); if (!is_localnet) { + 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); } else { - VLOG_INFO_RL(&rl, "bridge not found for localnet port '%s' " - "with network name '%s'; skipping", - binding->logical_port, network); + if (!sset_contains(&missed_bridges, msg_key)) { + VLOG_INFO("bridge not found for localnet port '%s' with " + "network name '%s'; skipping", + binding->logical_port, network); + sset_add(&missed_bridges, msg_key); + } } continue; } + sset_find_and_delete(&missed_bridges, msg_key); const char *patch_port_id; if (is_localnet) {
Having some localnet ports missing a bridge device on a particular chassis is a supported configuration (e.g. used to implement "routed provider networks" for OpenStack) and should not flood logs with duplicate messages. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- controller/patch.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)