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

Count repos across federation members in quota #1461

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

minrk
Copy link
Member

@minrk minrk commented Mar 8, 2022

This is an alternative to #1460, also for jupyterhub/mybinder.org-deploy#2143

Unlike #1460, it keeps all the definitions of quotas and such, except:

  1. repo counts are added to health check reports (doesn't require any additional k8s API calls, since we already collect them for the count)
  2. each federation member collects repo counts from the health endpoint of each other federation member
  3. repo_quota is measured against the sum of all federation members, instead of only the current instance

This is quite a bit simpler thank #1460, but potentially more costly (I'm not sure)

Additionally, I placed the binder repo_url in pod annotations, for easier lookup.

includes #1459

Make binderspawner_mixin.py canonical, instead of python-in-yaml

easier to edit with editors, linters, etc.

removes use of redundant pyyaml
for easier access than env
@choldgraf
Copy link
Member

When you say "costly" do you mean in terms of infrastructure costs? My intuition is that in the long term, our people costs are still much higher than our cloud costs, and so I am generally +1 on taking "simpler approaches that are a bit costlier in cloud".

This seems like a good iterative step forward on what we currently have to me.

@@ -612,6 +613,41 @@ def _default_build_token_secret(self):
"""
)

federation_id = Unicode(
Copy link
Member

Choose a reason for hiding this comment

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

it feels a bit weird that we are now encoding the notion of a "federation" in BinderHub itself - do we imagine any other usecase than mybinder.org where a user would want this? Or are we OK with designing BinderHub features that are just for mybinder.org?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main difference between the two - this PR requires BinderHubs to 'pull' quotas and know about its peers, whereas #1460 uses a 'push' model, which works if we tell all the instances to push their quota consumption to the same place.

I can also adjust #1460 to change less - keep the concurrent-session quota counting instead of also switching to rate limiting. But then frankly, we are starting to reimplement a KV store, and should probably deploy etcd/memcache/redis to handle that.

@minrk
Copy link
Member Author

minrk commented Mar 9, 2022

When you say "costly" do you mean in terms of infrastructure costs?

I mean performance-wise (making things slower, not costing more money). This approach moves larger amounts of information around and possibly making more k8s API calls (I don't think so, since it's piggy-backing on the same calls used by the health check already called every 10s by federation-redirect).

On the other hand, #1460 results in additional HTTP requests for every build, so that might slow things down itself.

@choldgraf
Copy link
Member

choldgraf commented Mar 10, 2022

Without understanding the technical details that much, my vote is for whatever is conceptually simplest, and has the least amount of maintenance burden over time. It seems like that is this PR?

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.

2 participants