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

Add LeaderElectionSupport for unit testing #764

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

tpantelis
Copy link
Contributor

Refactored from the submariner project for reuse.

Related to submariner-io/submariner#2747

Refactored from the submariner project for reuse.

Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
@tpantelis tpantelis self-assigned this Oct 19, 2023
@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr764/tpantelis/leader_election_test_support

Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

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

Shouldn’t the various Await timeouts take the configured timeouts into account?

@tpantelis
Copy link
Contributor Author

Shouldn’t the various Await timeouts take the configured timeouts into account?

AwaitLeaseAcquired could - that should take the lease duration to occur. We could use that timeout but that's really kind of internal to the leader election code and we're not trying to verify exactness in that code. But I'm fine either way.

AwaitLeaseReleased should happen quickly and is not dependent on any configured timeout.

AwaitLeaseRenewed is called after losing the lease so, after the failure is cleared, renewal should happen after the retry period but, again, that timing seems internal to the leader election.

@skitt
Copy link
Member

skitt commented Oct 20, 2023

Right, I’m not interested in verifying the leader election code itself; my concern was to avoid issues with tests in scenarios where the tests themselves set up specific timeouts (whether longer or short than we expect). If the timeouts involved here are internal to the leader election that’s fine.

@tpantelis tpantelis enabled auto-merge (rebase) October 23, 2023 13:23
@tpantelis tpantelis requested a review from dfarrell07 October 24, 2023 00:18
@tpantelis tpantelis disabled auto-merge October 24, 2023 11:59
Signed-off-by: Thomas Pantelis <tompantelis@gmail.com>
@tpantelis tpantelis merged commit 44c404b into submariner-io:devel Oct 24, 2023
16 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr764/tpantelis/leader_election_test_support]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants