Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ospfd: avoid the redundant timers #17803

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anlancs
Copy link
Contributor

@anlancs anlancs commented Jan 9, 2025

(config-router)# no summary-address 9.9.9.0/24
(config-router)# no summary-address 9.9.9.0/24
(config-router)# no summary-address 9.9.9.0/24
(config-router)# no summary-address 9.9.9.0/24
(config-router)# no summary-address 9.9.9.0/24

2025/01/09 09:30:10 OSPF: [S5BD4-3G5QX] ospf_external_aggr_timer: Start Aggregator delay timer 5(in seconds).
2025/01/09 09:30:11 OSPF: [S5BD4-3G5QX] ospf_external_aggr_timer: Start Aggregator delay timer 5(in seconds).
2025/01/09 09:30:11 OSPF: [S5BD4-3G5QX] ospf_external_aggr_timer: Start Aggregator delay timer 5(in seconds).
2025/01/09 09:30:11 OSPF: [S5BD4-3G5QX] ospf_external_aggr_timer: Start Aggregator delay timer 5(in seconds).
2025/01/09 09:30:12 OSPF: [S5BD4-3G5QX] ospf_external_aggr_timer: Start Aggregator delay timer 5(in seconds).

Since the timer thread for OSPF_ROUTE_AGGR_DEL has been created, the subsequent "no summary-address" commands shouldn't trigger redundant timers.

@@ -1159,7 +1159,8 @@ static void ospf_external_aggr_timer(struct ospf *ospf,
"%s, Restarting Aggregator delay timer.",
__func__);
EVENT_OFF(ospf->t_external_aggr);
}
} else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is too general. why not add a clause for repeated AGGR_DEL like the one that's already present for AGGR_ADD?

Copy link
Contributor Author

@anlancs anlancs Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. IMO "DEL" is not same to "ADD".
The continuous "DEL"s come, the first "DEL" should do the real thing: start timer and delete the item.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm - that's exactly what I wrote, but you said "no".
So: there's a clause for ADD that says "don't re-start the timer if it's already running for ADD". I suggest doing the same for DEL : if the timer is already running for DEL, emit a debug and don't reset the timer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood your meaning just now.
I got your point, and re-pushed one. Thanks for your review.

Since the timer thread for ```OSPF_ROUTE_AGGR_DEL``` has been created,
the subsequent "no summary-address" commands shouldn't trigger redundant timers.

Signed-off-by: anlan_cs <[email protected]>
@anlancs anlancs force-pushed the ospfd/fix-redundant-timers branch from 2489188 to a5ec72a Compare January 10, 2025 01:00
@anlancs anlancs requested a review from mjstapp January 10, 2025 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants