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:reset wait timer when reading ip ospf dead-interval #16954

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

Conversation

Shbinging
Copy link
Contributor

The fix addressed the issue found at #16905.
In the fix, after reconfiguring the dead-interval, the waitTimer, like the helloTimer, will refresh immediately.

In the current OSPF implementation, after configuring the ip ospf dead-interval, the waitTimer does not refresh immediately but waits for the waitTimer to expire before being reconfigured.

In issue 16905, when we configure the waitTimer to a very large value and then clear the OSPF process using clear ip ospf process, the waitTimer is forcibly reset and will wait for this large value until it expires.At this point, configuring a very small value for the dead-interval will not take effect immediately, which results in having to wait a long time. It appears as though the state is stuck.Therefore, if the configured dead-interval is large, we have to use clear ip ospf process each time to refresh the waitTimer.

Copy link
Collaborator

@aceelindem aceelindem left a comment

Choose a reason for hiding this comment

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

Please provide a simpler solution that doesn't add so much unnecessary code and doesn't reset timers unless it actually changes.

ospfd/ospf_vty.c Show resolved Hide resolved
@ton31337 ton31337 added this to the 10.2 milestone Oct 2, 2024
@github-actions github-actions bot added size/S rebase PR needs rebase and removed size/M labels Oct 9, 2024
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Please fix Verify Source (check) issues.

@Shbinging
Copy link
Contributor Author

Please fix Verify Source (check) issues.

ok

@ton31337
Copy link
Member

@aceelindem is it good now?

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good

@riw777
Copy link
Member

riw777 commented Nov 5, 2024

I think we're waiting on blocking changes from @aceelindem ?

Copy link
Collaborator

@aceelindem aceelindem left a comment

Choose a reason for hiding this comment

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

Thanks for moving this code. See comments regarding the setting of reset_wait_time.

ospfd/ospf_vty.c Outdated Show resolved Hide resolved
ospfd/ospf_vty.c Outdated Show resolved Hide resolved
@@ -8103,6 +8105,32 @@ static int ospf_vty_dead_interval_set(struct vty *vty, const char *interval_str,
ospf_nbr_timer_update(oi);
}

/*Update wait timer.*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better comment here with proper formatting.

@Shbinging Shbinging force-pushed the fix_ip_ospf_deadinterval branch 2 times, most recently from 8fbcb8d to c06c55c Compare November 6, 2024 05:25
Copy link
Collaborator

@aceelindem aceelindem left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for addressing my comments.

@riw777
Copy link
Member

riw777 commented Nov 12, 2024

failures in pim ... rerunning to see if we can clear these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants