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

[proxy] Implement compute node info cache #3331

Merged
merged 25 commits into from
Feb 1, 2023

Conversation

funbringer
Copy link
Contributor

@funbringer funbringer commented Jan 13, 2023

  • Find a decent LRU implementation.
  • Implement timed LRU on top of that.
  • Cache results of proxy_wake_compute API call.
  • Don't invalidate newer cache entries for the same key.
  • Add cmdline configuration knobs (requires some refactoring).
  • Add failed connection estab metric.
  • Refactor auth backends to make things simpler (retries, cache placement, etc).
  • Address review comments (add code comments + cleanup).
  • Retry /proxy_wake_compute if we couldn't connect to a compute (e.g. stalled cache entry).
  • Add high-level description for TimedLru.
  • Add cache metrics (hit, spurious hit, miss).
  • Synchronize http requests across concurrent per-client tasks ([proxy] Implement compute node info cache #3331 (comment)).
  • Cache results of proxy_get_role_secret API call.
  • Benchmark and deploy to staging.

@funbringer funbringer requested a review from a team as a code owner January 13, 2023 15:12
@funbringer funbringer requested review from nikitakalyanov and removed request for a team January 13, 2023 15:12
@funbringer funbringer marked this pull request as draft January 13, 2023 15:12
@vadim2404
Copy link
Contributor

Closes #3332

@funbringer funbringer force-pushed the funbringer/proxy-console-resp-cache branch from 958bc45 to 9913f4e Compare January 13, 2023 19:24
@funbringer funbringer force-pushed the funbringer/proxy-console-resp-cache branch 2 times, most recently from cd116de to e339048 Compare January 18, 2023 19:31
@funbringer funbringer marked this pull request as ready for review January 18, 2023 19:31
@funbringer funbringer changed the title [WIP] [proxy] Implement cloud API caches [proxy] Implement cloud API caches Jan 18, 2023
@funbringer funbringer force-pushed the funbringer/proxy-console-resp-cache branch 3 times, most recently from c665108 to 1c0922a Compare January 19, 2023 14:12
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Patch is looking good, also did some digging into surrounding proxy parts, which are looking great. Some comment specific ideas , and question about logging.

I doubt benchmarking would provide too interesting results, because the time to wakeup currently fluctuates and will dominate the time before this patch and with happen every once per ttl with this patch.

proxy/src/auth/backend/console.rs Outdated Show resolved Hide resolved
proxy/src/proxy.rs Outdated Show resolved Hide resolved
proxy/src/proxy.rs Outdated Show resolved Hide resolved
@funbringer funbringer force-pushed the funbringer/proxy-console-resp-cache branch from 1c0922a to 810ab86 Compare January 20, 2023 14:52
@funbringer funbringer enabled auto-merge (squash) January 20, 2023 15:43
@funbringer funbringer enabled auto-merge (squash) January 20, 2023 15:51
Copy link
Contributor

@LizardWizzard LizardWizzard left a comment

Choose a reason for hiding this comment

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

Looks very nice!

AM I right that invalidation happens based on either time or error during compute connection establishment?

proxy/src/auth/backend/console.rs Outdated Show resolved Hide resolved
proxy/src/cache.rs Show resolved Hide resolved
proxy/src/auth/backend/console.rs Outdated Show resolved Hide resolved
proxy/src/auth/backend.rs Outdated Show resolved Hide resolved
proxy/src/cache.rs Show resolved Hide resolved
proxy/src/cache.rs Outdated Show resolved Hide resolved
proxy/src/cache.rs Show resolved Hide resolved
proxy/src/cache.rs Outdated Show resolved Hide resolved
proxy/src/proxy.rs Outdated Show resolved Hide resolved
proxy/src/auth/backend.rs Outdated Show resolved Hide resolved
proxy/src/main.rs Outdated Show resolved Hide resolved
proxy/src/config.rs Outdated Show resolved Hide resolved
@hlinnaka
Copy link
Contributor

If you have a burst of 100 connections to the same database simultaneously, does all the connections race and perform 100 lookups in the console, and then all insert the same value in the cache? It would be better to keep track of which lookups are in progress, so that only one connection performs the lookup, and the others wait for it to finish. That is more efficient.

I don't think that's must-have, just something to consider and maybe add on later.

@lubennikovaav lubennikovaav removed their request for review January 24, 2023 16:46
@funbringer funbringer force-pushed the funbringer/proxy-console-resp-cache branch from dbfe888 to bf455d0 Compare January 30, 2023 16:03
@funbringer funbringer force-pushed the funbringer/proxy-console-resp-cache branch from ac5d021 to 8627113 Compare February 1, 2023 13:41
@funbringer
Copy link
Contributor Author

I don't think this PR is too big. Most of the changes are straightforward and actually facilitate the review process. For instance, there used to be an ugly auth::backend::console::handle_client which grew completely out of control once I added a few more parameters:

изображение

Now everything related to the console API lives in src/console and the rest of the auth backends has been freed from the lower-level boilerplate (http requests and such). We now have a decent console::Api abstraction which has helped me tremendously because it battles the overall complexity.

Copy link
Contributor

@LizardWizzard LizardWizzard 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 to me. I've had a call with @funbringer discussing the implementation which resulted in a few extra comments in code. The control flow indeed became simpler

I think we can move forward with testing this on staging

@funbringer funbringer merged commit ea0278c into main Feb 1, 2023
@funbringer funbringer deleted the funbringer/proxy-console-resp-cache branch February 1, 2023 14:11
@funbringer funbringer changed the title [proxy] Implement cloud API caches [proxy] Implement compute node info cache (the result of /proxy_wake_compute) Feb 1, 2023
@funbringer funbringer changed the title [proxy] Implement compute node info cache (the result of /proxy_wake_compute) [proxy] Implement compute node info cache (the result of proxy_wake_compute) Feb 1, 2023
@funbringer funbringer changed the title [proxy] Implement compute node info cache (the result of proxy_wake_compute) [proxy] Implement compute node info cache Feb 1, 2023
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.

None yet

6 participants