> On May 7, 2021, at 3:02 AM, Frode Nordahl <frode.nord...@canonical.com> wrote:
>
>
>
> fre. 7. mai 2021, 03:22 skrev Flavio Fernandes <fla...@flaviof.com
> <mailto:fla...@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 <fla...@flaviof.com
> <mailto:fla...@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
> d...@openvswitch.org <mailto:d...@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
