[ovs-dev] [PATCH ovn v1 1/1] ovn-controller: Ensure br-int is using secure fail-mode

[Flavio Fernandes]

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