feat: implements the never evict repository policy in the LRU cache given a YAML configuration file #28

k1nho · 2023-07-21T00:45:50Z

Description

This PR adds a flag config that will parse a string path during server startup. It defaults to an empty string, if no config flag is provided

What type of PR is this? (check all applicable)

Related Tickets & Documents

#26 Flag to consume yaml file at path to prevent eviction from cache of configured repos

Mobile & Desktop Screenshots/Recordings

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

jpmcb

👏🏼 Nice contribution! This is a really good start but I think we'd want something abit more feature complete before merging in the config file stuff.

See a few of my comments below.

pkg/server/server.go

main.go

jpmcb

Looking good so far!

This will be feature complete once the bits are in place in the tryEvict function within the cache. You'll want to pass in the data structure of repos to not evict down into the LRU cache.

pkg/server/server.go

main.go

pkg/server/server.go

pkg/providers/cache.go

jpmcb

Just a small error flow change I noticed. Happy to tackle that in a subsequent change.

main.go

jpmcb · 2023-08-01T15:31:38Z

@k1nho - are you interested in tackling the integration of the "never evict" map into the LRU cache itself? We shouldn't merge code that is half-baked (pun intended) and I'd like to avoid the main branch having commits that are not feature complete. Although, this isn't introducing breaking changes, it would just be confusing for anyone looking at the main branch commits.

k1nho · 2023-08-01T17:07:21Z

@jpmcb sure, I can give it a try! I was waiting based on the scope of the issue, but if it is ok to do it in this one I can try. I would like to know more about certain cases, right now I think of two in particular, would like to get more info on this.

Say we are about to exceed our minimum disk space and we call tryEvict, the LRU element is part of never evict, what is the expected behavior? go down the DLL to find the nearest LRU?
Kinda tied to first case, but say we have [https://github.com/open-sauced/insights, https://github.com/kubernetes/kubernetes, https://github.com/open-sauced/pizza] as our never evict repos (imagine more repos this is just small example), and we are about to exceed our minimum disk space with the next entry, we try to evict by looking for nearest LRU element not part ofneverEvictRepos but all the repos in the cache are part of neverEvictRepos, should we fail?

jpmcb · 2023-08-01T17:14:52Z

I think both those cases are misconfigurations: ideally someone shouldn't configure the cache to be smaller than their list of "never evict" repos nor should they have

We should provide feedback that something's not right. Let's start with a warning message and go from there.

k1nho · 2023-08-01T20:06:45Z

Ok, trying to understand better the implementation. This case, for example, where we have repo3 on the LRU spot, but since repo3 is on neverEvictRepos map, we cannot evict it instead we should evict nearest LRU which happens to be 'a'. After, the swap we have a full cache with all repos being neverEvictRepos, so any subsequent Put() call can never evict a repo? or is the setup of neverEvictRepos like a constant, meaning we do not put them on the DLL but they are just placed in hm?

jpmcb · 2023-08-03T17:49:28Z

or is the setup of neverEvictRepos like a constant, meaning we do not put them on the DLL but they are just placed in hm?

Right. neverEvictRepos is more of a receipt to "check" when eviction is going to take place. neverEvictRepos shouldn't be a "pre-load" of repos since that would likely trigger massive rate limiting from git service providers.

ince repo3 is on neverEvictRepos map, we cannot evict it instead we should evict nearest LRU which happens to be 'a'

Correct.

After, the swap we have a full cache with all repos being neverEvictRepos, so any subsequent Put() call can never evict a repo?

That's correct. Typically that would be a cache "drop". But again, I would say that's a misconfiguration on the part of the user deploying this service. There should always be some amount of disk space free and the size of the repos in neverEvictRepos shouldn't be equal to (or greater than) the size of the attached disk.

In that scenario, we should log out a warning message that can be surfaced by administrators. And in the future, we may add some metrics that denote cache "drops" so they can see a problem has started with the service.

But for now, start with something basic. Something like a check in the eviction algorithm that sees if the node up for eviction is in the hashmap. If it is, skip and go to the next node in the list.

k1nho · 2023-08-04T19:20:18Z

Awesome, I understand better now! I did the implementation, but need some help with the test setup. Right now, we have a test TestTryEvict and this line will evict all the repos.

https://github.com/open-sauced/pizza/blob/e99d988bbc30d3fc9fa4e4a6cbb0d789a5298d76/pkg/cache/lrucache_test.go#L194C8-L194C8

The way I have it now is that when nil is reached on the DLL, it means that all the repositories in the cache are neverEvictRepos and thus we error because of the misconfiguration. I had a test for tryEvict, but it will fail because of trying to evict everything (which it should since it represents a misconfiguration).

jpmcb

Preliminary review: I think you're on the right track! Thanks for sticking with this one. It's a huge feature. Hoping to give this a more in depth review this week.

pkg/cache/lrucache.go

main.go

jpmcb

This is getting very close! Almost there!!

pkg/cache/lrucache.go

Add flag to acquire filepath, and consume yaml for server configuration Drill config to LRU cache Remove else branch, early return on bad config Change providers to take the NeverEvictRepo type instead of Config change error flow structure make errors descriptive and use Faltalf implementing never evict repos into the LRU remove test for now change int for bool type and clarify lru to lruNode add neverEvictRepos as a data member to GitRepoLRUCache to maintain Put API free of the neverEvictRepos argument go mod tidy separate group imports

jpmcb

Great work!! Very well done!!!! 👏🏼👏🏼

brandonroberts

🍕

k1nho changed the title ~~Add flag to consume yaml file at path for future configurations~~ Add flag to consume yaml file at a given path for future configurations Jul 21, 2023

jpmcb reviewed Jul 21, 2023

View reviewed changes

pkg/server/server.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

jpmcb reviewed Jul 21, 2023

View reviewed changes

pkg/server/server.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

pkg/server/server.go Outdated Show resolved Hide resolved

jpmcb reviewed Jul 24, 2023

View reviewed changes

pkg/providers/cache.go Outdated Show resolved Hide resolved

jpmcb approved these changes Jul 25, 2023

View reviewed changes

jpmcb reviewed Jul 31, 2023

View reviewed changes

main.go Outdated Show resolved Hide resolved

jpmcb reviewed Aug 1, 2023

View reviewed changes

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

k1nho force-pushed the feature/config-flag-path branch from f7ea79f to f076008 Compare August 6, 2023 20:48

jpmcb reviewed Aug 7, 2023

View reviewed changes

pkg/cache/lrucache.go Outdated Show resolved Hide resolved

pkg/cache/lrucache.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

jpmcb reviewed Aug 8, 2023

View reviewed changes

pkg/cache/lrucache.go Outdated Show resolved Hide resolved

jpmcb linked an issue Aug 8, 2023 that may be closed by this pull request

Feature: Flag to consume yaml file at path to prevent eviction from cache of configured repos #26

Closed

2 tasks

k1nho changed the title ~~Add flag to consume yaml file at a given path for future configurations~~ feat: implements the never evict repository policy in the LRU cache given a YAML configuration file Aug 9, 2023

k1nho force-pushed the feature/config-flag-path branch from 93adada to 8c22e1f Compare August 9, 2023 17:26

jpmcb approved these changes Aug 12, 2023

View reviewed changes

jpmcb requested review from brandonroberts and bdougie August 16, 2023 16:05

brandonroberts approved these changes Aug 16, 2023

View reviewed changes

jpmcb merged commit c9ff004 into open-sauced:main Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implements the never evict repository policy in the LRU cache given a YAML configuration file #28

feat: implements the never evict repository policy in the LRU cache given a YAML configuration file #28

k1nho commented Jul 21, 2023 •

edited

Loading

jpmcb left a comment

jpmcb left a comment

jpmcb left a comment

jpmcb commented Aug 1, 2023

k1nho commented Aug 1, 2023 •

edited

Loading

jpmcb commented Aug 1, 2023

k1nho commented Aug 1, 2023

jpmcb commented Aug 3, 2023

k1nho commented Aug 4, 2023

jpmcb left a comment

jpmcb left a comment

jpmcb left a comment

brandonroberts left a comment

feat: implements the never evict repository policy in the LRU cache given a YAML configuration file #28

feat: implements the never evict repository policy in the LRU cache given a YAML configuration file #28

Conversation

k1nho commented Jul 21, 2023 • edited Loading

Description

What type of PR is this? (check all applicable)

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

jpmcb left a comment

Choose a reason for hiding this comment

jpmcb left a comment

Choose a reason for hiding this comment

jpmcb left a comment

Choose a reason for hiding this comment

jpmcb commented Aug 1, 2023

k1nho commented Aug 1, 2023 • edited Loading

jpmcb commented Aug 1, 2023

k1nho commented Aug 1, 2023

jpmcb commented Aug 3, 2023

k1nho commented Aug 4, 2023

jpmcb left a comment

Choose a reason for hiding this comment

jpmcb left a comment

Choose a reason for hiding this comment

jpmcb left a comment

Choose a reason for hiding this comment

brandonroberts left a comment

Choose a reason for hiding this comment

k1nho commented Jul 21, 2023 •

edited

Loading

k1nho commented Aug 1, 2023 •

edited

Loading