| Message ID | 20210507012226.1504699-1-flavio@flaviof.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v1,1/1] ovn-controller: Ensure br-int is using secure fail-mode | expand |
fre. 7. mai 2021, 03:22 skrev Flavio Fernandes <flavio@flaviof.com>: > By default, OVS bridges use standalone fail-mode, which means it is > configured with a single row with the NORMAL action as its OpenFlow table. > Upon system reboot, an integration bridge with many ports and such a table > could create broadcast storms and duplicate packets. That is why > ovn-controller creates the integration bridge with secure fail-mode. > Under that mode, the OpenFlow table remains empty until the controller > populates it, which could happen many seconds after the bridge is > operational. Unfortunately, the fail-mode setting was not being > done if the bridge was already created by the time ovn-controller > starts. This change fixes that and logs a warning should the fail-mode > ever needed to be corrected. > It would indeed be good to liberate the deployment tooling from the responsibility of maintaining the fail-mode, so I welcome this change. Reported-at: https://bugzilla.redhat.com/1957025 > Signed-off-by: Flavio Fernandes <flavio@flaviof.com> > --- > controller/ovn-controller.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 6106a9661..e4cbf3583 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -401,6 +401,12 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn, > ovs_table); > if (!br_int) { > br_int = create_br_int(ovs_idl_txn, ovs_table); > + } else if (ovs_idl_txn) { > Would it make sense to put this code into the branch below instead? It appears to me it already executes on the condition you want. + const char *fail_mode = br_int->fail_mode; > + if (!fail_mode || strcmp(fail_mode, "secure")) { > + ovsrec_bridge_set_fail_mode(br_int, "secure"); > + VLOG_WARN("Integration bridge fail-mode set to secure."); > While I agree this action should be logged, I wonder if the text could be rephrased. As it stands it could read as having br-int in secure fail-mode is a undesirable situation, whereas the opposite is true. + } > } > if (br_int && ovs_idl_txn) { > const struct ovsrec_open_vswitch *cfg; > -- > 2.25.1 > -- Frode Nordahl > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On May 7, 2021, at 3:02 AM, Frode Nordahl <frode.nordahl@canonical.com> wrote: > > > > fre. 7. mai 2021, 03:22 skrev Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>>: > By default, OVS bridges use standalone fail-mode, which means it is > configured with a single row with the NORMAL action as its OpenFlow table. > Upon system reboot, an integration bridge with many ports and such a table > could create broadcast storms and duplicate packets. That is why > ovn-controller creates the integration bridge with secure fail-mode. > Under that mode, the OpenFlow table remains empty until the controller > populates it, which could happen many seconds after the bridge is > operational. Unfortunately, the fail-mode setting was not being > done if the bridge was already created by the time ovn-controller > starts. This change fixes that and logs a warning should the fail-mode > ever needed to be corrected. > > It would indeed be good to liberate the deployment tooling from the responsibility of maintaining the fail-mode, so I welcome this change. Cool beans! > > Reported-at: https://bugzilla.redhat.com/1957025 <https://bugzilla.redhat.com/1957025> > Signed-off-by: Flavio Fernandes <flavio@flaviof.com <mailto:flavio@flaviof.com>> > --- > controller/ovn-controller.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 6106a9661..e4cbf3583 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -401,6 +401,12 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn, > ovs_table); > if (!br_int) { > br_int = create_br_int(ovs_idl_txn, ovs_table); > + } else if (ovs_idl_txn) { > > Would it make sense to put this code into the branch below instead? It appears to me it already executes on the condition you want. Yeah, we could do that. Right now, the creation of the bridge in "create_br_int" is already setting the fail-mode and by moving this below would make that potentially redundant. But the cost of an additional strcmp() for sake of simplicity may be a valid trade off? So the new code would look more like: if (br_int && ovs_idl_txn) { const struct ovsrec_open_vswitch *cfg; ... if (datapath_type && strcmp(br_int->datapath_type, datapath_type)) { ovsrec_bridge_set_datapath_type(br_int, datapath_type); } + const char *fail_mode = br_int->fail_mode; + if (!fail_mode || strcmp(fail_mode, "secure")) { + ovsrec_bridge_set_fail_mode(br_int, "secure"); + VLOG_WARN("Integration bridge fail-mode needed to be set as secure.");} + } } return br_int; Sounds reasonable? > > + const char *fail_mode = br_int->fail_mode; > + if (!fail_mode || strcmp(fail_mode, "secure")) { > + ovsrec_bridge_set_fail_mode(br_int, "secure"); > + VLOG_WARN("Integration bridge fail-mode set to secure."); > > While I agree this action should be logged, I wonder if the text could be rephrased. As it stands it could read as having br-int in secure fail-mode is a undesirable situation, whereas the opposite is true. heh.... English troubles! ;) I can see what you mean now. Do you think this is more appropriate: Integration bridge fail-mode needed to be set as secure. ? Chime in with any tweaks, please. -- flaviof > > + } > } > if (br_int && ovs_idl_txn) { > const struct ovsrec_open_vswitch *cfg; > -- > 2.25.1 > > -- > Frode Nordahl > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
On Fri, May 7, 2021 at 3:23 PM Flavio Fernandes <flavio@flaviof.com> wrote: > On May 7, 2021, at 3:02 AM, Frode Nordahl <frode.nordahl@canonical.com> wrote: > fre. 7. mai 2021, 03:22 skrev Flavio Fernandes <flavio@flaviof.com>: >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 6106a9661..e4cbf3583 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -401,6 +401,12 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn, >> ovs_table); >> if (!br_int) { >> br_int = create_br_int(ovs_idl_txn, ovs_table); >> + } else if (ovs_idl_txn) { > > Would it make sense to put this code into the branch below instead? It appears to me it already executes on the condition you want. > > Yeah, we could do that. Right now, the creation of the bridge in "create_br_int" is already setting the fail-mode and > by moving this below would make that potentially redundant. But the cost of an additional strcmp() for sake of simplicity > may be a valid trade off? So the new code would look more like: Ah I see, I like conservatism, but in reality it would only optimize away the call for the first iteration of the main loop if the bridge did not exist? > if (br_int && ovs_idl_txn) { > const struct ovsrec_open_vswitch *cfg; > ... > if (datapath_type && strcmp(br_int->datapath_type, datapath_type)) { > ovsrec_bridge_set_datapath_type(br_int, datapath_type); > } > + const char *fail_mode = br_int->fail_mode; > + if (!fail_mode || strcmp(fail_mode, "secure")) { > + ovsrec_bridge_set_fail_mode(br_int, "secure"); > + VLOG_WARN("Integration bridge fail-mode needed to be set as secure.");} > + } > } > return br_int; > > Sounds reasonable? I think this looks much better, one minor nit: the fail_mode variable is not really necessary, or perhaps we could use it to store the desired fail_mode "secure" instead, to avoid repetition of that constant in the code? > >> + const char *fail_mode = br_int->fail_mode; >> + if (!fail_mode || strcmp(fail_mode, "secure")) { >> + ovsrec_bridge_set_fail_mode(br_int, "secure"); >> + VLOG_WARN("Integration bridge fail-mode set to secure."); > > > While I agree this action should be logged, I wonder if the text could be rephrased. As it stands it could read as having br-int in secure fail-mode is a undesirable situation, whereas the opposite is true. > > > heh.... English troubles! ;) I can see what you mean now. > > Do you think this is more appropriate: > > Integration bridge fail-mode needed to be set as secure. > > ? Chime in with any tweaks, please. Better, this could also work: "Integration bridge fail-mode changed to 'secure'." Cheers! -- Frode Nordahl > > -- flaviof > > >> + } >> } >> if (br_int && ovs_idl_txn) { >> const struct ovsrec_open_vswitch *cfg; >> -- >> 2.25.1 > > > -- > Frode Nordahl > >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 6106a9661..e4cbf3583 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -401,6 +401,12 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn, ovs_table); if (!br_int) { br_int = create_br_int(ovs_idl_txn, ovs_table); + } else if (ovs_idl_txn) { + const char *fail_mode = br_int->fail_mode; + if (!fail_mode || strcmp(fail_mode, "secure")) { + ovsrec_bridge_set_fail_mode(br_int, "secure"); + VLOG_WARN("Integration bridge fail-mode set to secure."); + } } if (br_int && ovs_idl_txn) { const struct ovsrec_open_vswitch *cfg;
By default, OVS bridges use standalone fail-mode, which means it is configured with a single row with the NORMAL action as its OpenFlow table. Upon system reboot, an integration bridge with many ports and such a table could create broadcast storms and duplicate packets. That is why ovn-controller creates the integration bridge with secure fail-mode. Under that mode, the OpenFlow table remains empty until the controller populates it, which could happen many seconds after the bridge is operational. Unfortunately, the fail-mode setting was not being done if the bridge was already created by the time ovn-controller starts. This change fixes that and logs a warning should the fail-mode ever needed to be corrected. Reported-at: https://bugzilla.redhat.com/1957025 Signed-off-by: Flavio Fernandes <flavio@flaviof.com> --- controller/ovn-controller.c | 6 ++++++ 1 file changed, 6 insertions(+)