OSPF timers throttle spf: holdTimeMultiplier not reset, causing incorrect min_holdtime

[Shbinging]

Description

When the SPF throttle timers are updated with a new value, particularly when the new value is larger than the previous one, the holdTimeMultiplier is not reset to 1. This results in the min_holdtime being larger than expected under the new settings, as the multiplier remains greater than 1 initially.

This potential issue can be found in the ospf_spf_calculate_schedule function within ospf_spf.c and can be reproduced under these conditions

If you confirm this issue, I will submit a PR to fix it.

Version

10.1

How to reproduce

Turn the port on and off once to make the holdTimeMultiplier value greater than 1, then use the timers throttle spf command to set a new value.

Expected behavior

holdTimeMultiplier = 1

Actual behavior

holdTimeMultiplier > 1

Additional context

No response

Checklist

• I have searched the open issues for this bug.
• I have not included sensitive information in this report.

[riw777]

Do you mean when the multiplier is reduced, or when it is increased? Does it ever reset, or does it always stay at the initial value?

[aceelindem]

This is somewhat subjective. Since the current SPF delay state is being changed, I can see the argument for resetting the spf_hold_multiplier in ospf_timers_spf_set(). However, perhaps this should only be done if the current state is changed?

if (ospf->spf_delay != delay || ospf->spf_holdtime != hold || ospf->spf_max_holdtime != max) {
o
o
o

[Shbinging]

Do you mean when the multiplier is reduced, or when it is increased? Does it ever reset, or does it always stay at the initial value?

I mean when the multiplier is increased. The issue occurs when the new value set using the timers throttle spf command is larger than the previous one. In this case, the holdTimeMultiplier does not reset to 1 and remains greater than 1, leading to an incorrect min_holdtime. It does not reset automatically; that's the core of the issue.

[Shbinging]

This is somewhat subjective. Since the current SPF delay state is being changed, I can see the argument for resetting the spf_hold_multiplier in ospf_timers_spf_set(). However, perhaps this should only be done if the current state is changed?

if (ospf->spf_delay != delay || ospf->spf_holdtime != hold || ospf->spf_max_holdtime != max) { o o o

I agree that it's subjective and resetting the spf_hold_multiplier when the SPF delay state is changed makes sense. Implementing the reset only when the state changes, as you mentioned, could be a good approach. Checking if ospf->spf_delay, ospf->spf_holdtime, or ospf->spf_max_holdtime has changed before resetting the multiplier seems like a reasonable condition to prevent unnecessary resets when the values remain the same.

I'll include this condition in the commit to ensure it only resets when there's a meaningful state change.

[Shbinging]

I have submitted a PR #16851