Bug 1160815 - DELAY=0 in bridge configuration results in default (30 second) listening/learning delay
DELAY=0 in bridge configuration results in default (30 second) listening/lear...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: NetworkManager (Show other bugs)
21
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-05 12:17 EST by Adam Williamson
Modified: 2015-01-31 09:33 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-01-19 06:10:17 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
[PATCH] bridge: Properly check range of STP properties (2.30 KB, text/plain)
2014-11-10 12:58 EST, Lubomir Rintel
no flags Details

  None (edit)
Description Adam Williamson 2014-11-05 12:17:12 EST
I had a bridge configured with this line in its ifcfg-br0:

DELAY=0

I believe I found this parameter from https://docs.fedoraproject.org/en-US/Fedora/17/html/System_Administrators_Guide/s2-networkscripts-interfaces_network-bridge.html (or if not, some similar reference) after configuring bridging and being annoyed by the 30 second delay on activation before the connection actually became usable.

I'm about 99% sure that the DELAY=0 setting 'worked' - as in, caused the connection to be activated rapidly - for some time, but then in the last few months (with F21), it seemed to 'stop working'. Logs confirm the delay went back to exactly 30 seconds - 15 seconds for 'listening', 15 seconds for 'learning'.

dcbw traced it out and found the kernel actually has a minimum delay of 2 seconds, and treats a setting of 0 as 'not set', so it seems NM was passing a setting of 0 through to the kernel and the kernel was treating it as 'not set, use default'. Indeed, changing the setting to:

DELAY=2

seems to 'work', i.e. it makes connection activation pretty quick, with the specified two second delay.

Probably NM should treat a setting of <2 as 2, or find a value to pass through to the kernel which will cause the kernel to pick the minimum delay (to guard against the possibility that it changes). The only thing we didn't figure out is why this stopped 'working' for me relatively recently.
Comment 1 Dan Williams 2014-11-05 12:27:04 EST
NM notes...

DELAY (eg, forward_delay) is only valid when STP is enabled.  When STP is disabled, NMSettingBridge::forward_delay should be 0.  When STP is enabled, forward_delay must be between 2 and 30 seconds.

NMSettingBridge::verify() does not enforce the correct value ranges for STP-related options when STP is enabled, but it should do so.  So if forward_delay=0 and STP is enabled, the kernel rejects the attempt to set the forward_delay with -ERANGE and the value is left as the default (15 seconds).

Nor does ifcfg-rh warn when STP is enabled but DELAY=0, which it probably should do.
Comment 2 Lubomir Rintel 2014-11-10 12:58:38 EST
Created attachment 955919 [details]
[PATCH] bridge: Properly check range of STP properties

(In reply to Dan Williams from comment #1)
> NM notes...
> 
> DELAY (eg, forward_delay) is only valid when STP is enabled.  When STP is
> disabled, NMSettingBridge::forward_delay should be 0.

The property defaults to 15. The connection with stp=0 and forward_delay unset would not pass validation.

> When STP is enabled,
> forward_delay must be between 2 and 30 seconds.
> 
> NMSettingBridge::verify() does not enforce the correct value ranges for
> STP-related options when STP is enabled, but it should do so.  So if
> forward_delay=0 and STP is enabled, the kernel rejects the attempt to set
> the forward_delay with -ERANGE and the value is left as the default (15
> seconds).
> 
> Nor does ifcfg-rh warn when STP is enabled but DELAY=0, which it probably
> should do.

Why? It doesn't look like a right place to do that (would it make sense to check this in the keyfile plugin as well then?) and the NMSettingBridge::verify() should cover that anyway.
Comment 3 Dan Williams 2014-12-09 12:27:07 EST
BR_MIN_AGEING_TIME = 0, so should that check pass !priv->stp, or always TRUE?

Also since min is unsigned, I think Coverity will yell at us for the "if (val < min || val > max) {" check for BR_MIN_AGEING_TIME (which is 0).  Not sure we care about that much though, unless you want to change min/max to gint64.

Also I'd change "zero" to "zero_valid" to make it clearer.

Overall looks good to me.
Comment 4 Lubomir Rintel 2014-12-11 10:20:29 EST
(In reply to Dan Williams from comment #3)
> BR_MIN_AGEING_TIME = 0, so should that check pass !priv->stp, or always TRUE?

Well, either will work. I'd prefer keeping the STP condition for consistency, but I don't really care.

> Also since min is unsigned, I think Coverity will yell at us for the "if
> (val < min || val > max) {" check for BR_MIN_AGEING_TIME (which is 0).  Not
> sure we care about that much though, unless you want to change min/max to
> gint64.

Hmm, I'd assume Coverity is smarter and don't really know what would look good to it instead. Can I keep it as it is here and address the Coverity issues separately (bug #1173171)?

> Also I'd change "zero" to "zero_valid" to make it clearer.

Done.

> Overall looks good to me.
Comment 5 Ferry Huberts 2014-12-17 03:53:00 EST
A forward delay of zero used to work.
I think F20 broke it.
Also, I noticed that I couldn't set it to zero using brctl, so maybe something changed there too
Comment 6 Dan Williams 2015-01-08 15:21:01 EST
(In reply to Ferry Huberts from comment #5)
> A forward delay of zero used to work.
> I think F20 broke it.
> Also, I noticed that I couldn't set it to zero using brctl, so maybe
> something changed there too

It actually never "worked", it was just ignored by NetworkManager and then defaulted to 15 anyway.
Comment 7 Ferry Huberts 2015-01-08 16:41:29 EST
I distinctly remember setting it to 0 and seeing it at 0.
Maybe that was RHEL6
Comment 8 Jirka Klimes 2015-01-09 04:15:37 EST
The patch looks good to me (after the changes).
Comment 9 Lubomir Rintel 2015-01-19 06:10:17 EST
Seems like the fix went out with last f21 update to a later NM Git snapshot.
Comment 10 Ferry Huberts 2015-01-31 09:33:10 EST
(In reply to Ferry Huberts from comment #7)
> I distinctly remember setting it to 0 and seeing it at 0.
> Maybe that was RHEL6

Now confirmed it: DELAY=0 works fine on RHEL6

If fact, I believe it to be a mistake that it currently can't be set any lower than 2.

Note You need to log in before you can comment on or make changes to this bug.