bug: ovs_options not handled as in ifupdown1, breaking Fake Bridge configurations

[abochem-bb]

TL;DR:

• ifupdown2 handles ovs_options differently from ifupdown1
• ifupdown2 thereby breaks using OVS Fake Bridges.
• ifupdown2's handling of ovs_options contradicts the help text included in ifupdown2 itself.
• In summary we consider this a bug in ifupdown2 and ask this to be fixed.

Detailed explanation:

openvswitch integration for ifupdown1 adds ovs_options as just additional parameters to the initial ovs-vsctl call, i.e. no double dashes added. See in https://github.com/openvswitch/ovs/blob/5046f2e35f62838c2ad410e20448e0b7c81d8234/debian/ifupdown.sh#L46-L91, quoting as example the setup of OVSBridge:

            ovs_vsctl -- --may-exist add-br "${IFACE}" ${IF_OVS_OPTIONS}\
                    ${OVS_EXTRA+-- $OVS_EXTRA}

(Note that there is still ovs_extra as a separate option if you do intend to create a new command separated by double dashes.)

The ifupdown2-addon for openvswitch instead creates a new ovs-vsctl command from ovs_options, see https://github.com/CumulusNetworks/ifupdown2/blob/1d6a726e5a492ce933369585e1c623c0d9e711c0/ifupdown2/addons/openvswitch.py#L151-L159

    cmd_list = []


    cmd = "--may-exist add-br %s"%(iface)
    cmd_list.append(cmd)


    if ovsoptions:
        cmd = "set bridge %s %s" %(iface, ovsoptions)
        cmd_list.append(cmd)

This change in behaviour creates errors when using the fake bridge feature of OVS. Quoting the man page of ovs-vsctl:

   [--may-exist] add-br bridge parent vlan
          Creates a ``fake bridge'' named bridge within the existing Open vSwitch bridge parent, which must already exist and must not itself be a
          fake  bridge.  The new fake bridge will be on 802.1Q VLAN vlan, which must be an integer between 0 and 4095.  The parent bridge must not
          already have a fake bridge for vlan.  Initially bridge will have no ports (other than bridge itself).

In /etc/network/interfaces, this used to be configured using ovs_options like this:

auto vmbr4
allow-br_mgmt vmbr4
iface vmbr4 inet manual
  ovs_type    OVSBridge
  ovs_options br_mgmt 4

Which in ifupdown1 correctly creates the single add-br call:
ovs-vsctl -- --may-exist add-br vmbr4 br_mgmt 4

ifupdown2, however, creates a plain add-br call followed by set bridge - which does not work and throws an error simply because this is not a case of setting options on an existing bridge, but changing the add-br call itself.

info: executing /usr/bin/ovs-vsctl -- --may-exist add-br vmbr4 -- set bridge vmbr4 br_mgmt 4
error: cmd '/usr/bin/ovs-vsctl -- --may-exist add-br vmbr4 -- set bridge vmbr4 br_mgmt 4' failed: returned 1 (ovs-vsctl: Bridge does not contain a column whose name matches "br_mgmt"

In addition, this behaviour contradicts the help text included in the ifupdown2 openvswitch integration itself, where ovs_options is explicitly mentioned to add arguments to a (existing) command as opposed to ovs_extra adding a whole new command separated by double dashes https://github.com/CumulusNetworks/ifupdown2/blob/1d6a726e5a492ce933369585e1c623c0d9e711c0/ifupdown2/addons/openvswitch.py#L58-L69:

        'ovs-options': {
            'help': 'This option lets you add extra arguments to a ovs-vsctl command',
            'required': False,
        },
        'ovs-extra': {
            'help': 'This option lets you run additional ovs-vsctl commands,'  +
                    'separated by "--" (double dash). Variables can be part of the "ovs_extra"' +
                    'option. You can provide all the standard environmental variables' + 
                    'described in the interfaces(5) man page. You can also pass shell' +
                    'commands.extra args',
            'required': False,
            'example': ['ovs_extra set bridge ${IFACE} other-config:hwaddr=00:59:cf:9c:84:3a -- br-set-external-id ${IFACE} bridge-id ${IFACE}']

[julienfortin]

@aderumier this one is for you 🙂

[aderumier]

@julienfortin
yep. (I'm already talking about this with the user on the proxmox forum)

[aderumier]

@abochem-bb

| openvswitch integration for ifupdown1 adds ovs_options as just additional parameters to the initial ovs-vsctl call, i.e. no double dashes added.
the ovs_options It has been implemented like this, because we also need to handle config change and reload, (as ifupdown1 can't do it, and only delete/recreate the ovs).
That was the best way to not break current setup.
(I think only special case of fake bridge don't work, I don't have seen any bug report since 2years ).

Also, I think we can't just keep the previous ifupdown1 syntax "ovs_options br_mgmt 4" , because we can't manage relationship with the main ovs parent.

Maybe something like:

auto vmbr4
iface vmbr4 inet manual
  ovs_bridge br_mgmt
  ovs_type  OVSBridge
  ovs_options tag=4

(so mostly like an internalport vlan)

could world. (with special handling of vlan=4, when it's a fake bridge )

I need to do tests, do some reload,etc...

[aderumier]

ok, I have found a way keep old syntax with

auto vmbr4
iface vmbr4 inet manual
  ovs_bridge br_mgmt
  ovs_type OVSBridge
  ovs_options br_mgmt 4

just need to add an extra ovs_bridge to handle relationship,
and if ovs_bridge is defined, then pass ovs_options directly after add-br. (as anyway, we can't add options to a fake bridge)

I'll send a small patch tomorrow for testing.

[abochem-bb]

To all the other readers FYI: This issue originated from a discussion in the Proxmox Forum about migrating an PVE Cluster from ifupdown to ifupdown2 with a legacy network configuration.

@aderumier

In the PVE scenario, there is a catch to your approach:
PVE does not allow this configuration to be set via it's WebUI. It only allows Bridge Ports (and Options) to be specified on an OVSBridge - not other Bridges - and checks the input accordingly. (In addition, it does not recognize an OVSBridge as such unless it is named "vmbr".)

Since ovs_options is handled differently from ifupdown1 anyway, would it be possible to specify all necessary information there instead?

Maybe like ovs_options fakebridge br_mgmt 4 - using the keyword fakebridge to detect special usage in the ifupdown2 code, and knowing that the following parameter specifies the parent bridge.

[aderumier]

Hi,
I have send a patch here

#246

It's really need "ovs_bridge br_mgmt" to handle relationship with ifupdown2.

I'll look to add this option in proxmox gui too