Bug 1371327

Summary: Issues using miimon in active-passive with openvswitch
Product: Red Hat OpenStack Reporter: David Hill <dhill>
Component: openvswitchAssignee: Aaron Conole <aconole>
Status: CLOSED NEXTRELEASE QA Contact: Ofer Blaut <oblaut>
Severity: high Docs Contact:
Priority: high    
Version: 7.0 (Kilo)CC: aconole, apevec, beagles, chrisw, dhill, dsneddon, fleitner, jjoyce, jschluet, mburns, rhos-maint, srevivo
Target Milestone: ---Keywords: ZStream
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-05-30 14:52:09 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Attachments:
Description Flags
poc patch none

Description David Hill 2016-08-29 23:45:18 UTC
Description of problem:
Issues using miimon with openvswitch .   When we set bond-detect-mode to "carrier" it actually seems to be working fine.  I suspect the following chunk of code to be the culprit in bridge.c:

    detect_s = smap_get(&port->cfg->other_config, "bond-detect-mode");
    if (!detect_s || !strcmp(detect_s, "carrier")) {
        miimon_interval = 0;
    } else if (strcmp(detect_s, "miimon")) {
        VLOG_WARN("port %s: unsupported bond-detect-mode %s, "
                  "defaulting to carrier", port->name, detect_s);
        miimon_interval = 0;
    }

The way I understand it is:
  if bond-detect-mode is not set or not carrier:
     miimon_interval =  0
  else if bond-detect-mode is miimon:
     miimon_interval = 0

This would always set miimon_interval = 0 if we're trying to use miimon and set it to our wanted value if bond-detect-mode is actually set to carrier .


Version-Release number of selected component (if applicable):


How reproducible:
Always

Steps to Reproduce:
1. Configure ovs bonds with active-passive and a mii_interval of 200
2.
3.

Actual results:
Unpluging one of the interfaces will never trigger the failover

Expected results:
Unpluging one of the interfaces should always trigger a failover

Additional info:

Comment 1 David Hill 2016-08-30 14:53:07 UTC
The used driver is ixgbe

Comment 3 David Hill 2016-08-30 18:22:26 UTC
Shouldn't it use netif_carrier_ok() if possible?  Also, in this function, it seems like if the device doesn't return an error at SIOCGMIIPHY but returns one at SIOCGMIIPHY, it won't try ethtool which is our issue right now.


netdev_linux_get_miimon(const char *name, bool *miimon)
{
    struct mii_ioctl_data data;
    int error;

    *miimon = false;

    memset(&data, 0, sizeof data);
    error = netdev_linux_do_miimon(name, SIOCGMIIPHY, "SIOCGMIIPHY", &data);
    if (!error) {
        /* data.phy_id is filled out by previous SIOCGMIIPHY miimon call. */
        data.reg_num = MII_BMSR;
        error = netdev_linux_do_miimon(name, SIOCGMIIREG, "SIOCGMIIREG",
                                       &data);

        if (!error) {
            *miimon = !!(data.val_out & BMSR_LSTATUS);
        } else {
            VLOG_WARN_RL(&rl, "%s: failed to query MII", name);
        }
    } else {
        struct ethtool_cmd ecmd;

        VLOG_DBG_RL(&rl, "%s: failed to query MII, falling back to ethtool",
                    name);

        COVERAGE_INC(netdev_get_ethtool);
        memset(&ecmd, 0, sizeof ecmd);
        error = netdev_linux_do_ethtool(name, &ecmd, ETHTOOL_GLINK,
                                        "ETHTOOL_GLINK");
        if (!error) {
            struct ethtool_value eval;

            memcpy(&eval, &ecmd, sizeof eval);
            *miimon = !!eval.data;
        } else {
            VLOG_WARN_RL(&rl, "%s: ethtool link status failed", name);
        }
    }

    return error;
}

Comment 4 David Hill 2016-08-30 18:35:38 UTC
Created attachment 1196051 [details]
poc patch

Comment 5 David Hill 2016-08-31 20:19:59 UTC
We also got this patch[1] for the ixgbe driver.  This would be kernel then.

[1] http://patchwork.ozlabs.org/patch/664633/

Comment 9 Aaron Conole 2017-02-16 16:51:29 UTC
This is available with OVS 2.6.1.