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 leaderelection module #347

Merged
merged 6 commits into from
Jan 10, 2025
Merged

Conversation

JacobHenner
Copy link
Contributor

Add leaderelection module, based off of the leaderelection module in kubernetes-client/python. The module has been altered slightly to support asyncio.

Fixes #297

@JacobHenner
Copy link
Contributor Author

I started off by adapting the leaderelection module from kubernetes-client/python, but keeping the implementation as similar as possible. I'm not sure to what extent this project aims to provide a similar interface to kubernetes-client/python, and I'll call out some concerns I have in comments on the changed code.

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

Attention: Patch coverage is 61.79487% with 149 lines in your changes missing coverage. Please review.

Project coverage is 27.05%. Comparing base (3ab6408) to head (bed4ae1).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...yncio/leaderelection/resourcelock/configmaplock.py 0.00% 64 Missing ⚠️
...s_asyncio/leaderelection/resourcelock/leaselock.py 0.00% 63 Missing ⚠️
...ubernetes_asyncio/leaderelection/leaderelection.py 91.20% 8 Missing ⚠️
...ubernetes_asyncio/leaderelection/electionconfig.py 70.83% 7 Missing ⚠️
...etes_asyncio/leaderelection/leaderelection_test.py 95.07% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   26.91%   27.05%   +0.13%     
==========================================
  Files         799      805       +6     
  Lines       98316    98706     +390     
==========================================
+ Hits        26463    26704     +241     
- Misses      71853    72002     +149     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomplus
Copy link
Owner

tomplus commented Jan 5, 2025

I started off by adapting the leaderelection module from kubernetes-client/python, but keeping the implementation as similar as possible. I'm not sure to what extent this project aims to provide a similar interface to kubernetes-client/python, and I'll call out some concerns I have in comments on the changed code.

No worries, I usually adapt with modifications. Thanks!

Copy link
Owner

@tomplus tomplus left a comment

Choose a reason for hiding this comment

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

Excellent job, as always.
Thank you, Jacob.

kubernetes_asyncio/leaderelection/electionconfig.py Outdated Show resolved Hide resolved
kubernetes_asyncio/leaderelection/example.py Outdated Show resolved Hide resolved
kubernetes_asyncio/leaderelection/leaderelection.py Outdated Show resolved Hide resolved
kubernetes_asyncio/leaderelection/leaderelection.py Outdated Show resolved Hide resolved
Add leaderelection module, based off of the leaderelection module in
kubernetes-client/python. The module has been altered slightly to
support asyncio.

Fixes tomplus#297
@JacobHenner
Copy link
Contributor Author

@JacobHenner JacobHenner marked this pull request as ready for review January 7, 2025 20:16
@tomplus
Copy link
Owner

tomplus commented Jan 8, 2025

What do you think about the loglevels (mostly INFO)? I've preserved the log messages and levels used in kubernetes-client/python, but I wonder if some are a bit verbose.

I agree, some of them should be on debug level...

Instead of passing Callable[...,Coroutine] functions as callbacks, pass
Coroutines directly. This allows the caller to supply arbitrary context
associated with the Coroutine, such as the ApiClient used to establish
the leader election.
* Remove misplaced basicConfig
* Use %-style string interpolation
Copy link
Owner

@tomplus tomplus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@JacobHenner
Copy link
Contributor Author

What do you think about the loglevels (mostly INFO)? I've preserved the log messages and levels used in kubernetes-client/python, but I wonder if some are a bit verbose.

I agree, some of them should be on debug level...

I switched to a mix of debug for verbose debug conditions, and error/exception for messages that actually indicate problems. Let me know if you agree with the selections.

I also made an unrelated change in ef01ee6, switching the callback functions from Callable[..., Coroutine] to Coroutine, to facilitate context passing at the time of leader election establishment. I realized this would be useful as I wrote some code using the contents of this PR, and noticed that it'd be nice if there were an easier way to pass context to onstarted_leading. For example, the ApiClient used to establish the leader election:

    async with api_client.ApiClient() as apic:
        # Create config
        leader_election_config = electionconfig.Config(
            # A legacy ConfigMapLock is also available
            LeaseLock(lock_name, lock_namespace, candidate_id, apic),
            lease_duration=17,
            renew_deadline=15,
            retry_period=5,
            onstarted_leading=example_start_func(api_client=apic),
            onstopped_leading=example_end_func(),
        )

        # Enter leader election
        await leaderelection.LeaderElection(leader_election_config).run()

This seemed to be much more pleasant than other options for including arbitrary context, such as defining otherwise-unnecessary wrapper functions, using contextvars, etc.

Let me know what you think.

@tomplus
Copy link
Owner

tomplus commented Jan 8, 2025

I see two potential problems with passing coroutines:

  1. coroutines can be awaited only once so in an application you may need to recreate callbacks every time

  2. ugly messages from python if they aren't awaited

what about supporting two versions? we can check if it is a function or a coroutine and act accordingly.

@JacobHenner
Copy link
Contributor Author

what about supporting two versions?

For the function version, are you suggesting supporting non-async functions, or supporting async functions (that return coroutines)?

@tomplus
Copy link
Owner

tomplus commented Jan 9, 2025

what about supporting two versions?

For the function version, are you suggesting supporting non-async functions, or supporting async functions (that return coroutines)?

async functions and coroutines:

onstopped_leading=example_end_func,

# or:

onstopped_leading=example_end_func(),

# where:

async def example_end_func():
    pass

@tomplus tomplus merged commit f1d47ed into tomplus:master Jan 10, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a leader election module
2 participants