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

refactor yurthub cache to adapt different storages #882

Merged
merged 15 commits into from
Nov 21, 2022

Conversation

Congrool
Copy link
Member

Signed-off-by: Congrool [email protected]

What type of PR is this?

/kind enhancement
/sig storage

What this PR does / why we need it:

Currently, the cacheManager deeply couple with the diskStorage. In #778 , we'll add a new storage backend for cacheManager, called pool-coordiantor which actually an etcd storage at the view of cacheManager. There're some problems need to be solved before implement this feature:

  1. the format of key for disk storage is different from that of pool-coordinator, the former is /component/resource/namespace/name and the later is /registry/resource/namespace/name. Also considering that there're some special case for pool-coordinator, such as resource nodes should be minions instead.
  2. For update operation, there's also difference. For disk storage, we have to decode the stored json data and retrieve the rv field, but for pool-coordinator we can directly compare the rv in etcd. Also considering that, the comparision and update should be atomic, so we should not compare the rv in cache manager and update in storage.
  3. The storage.Store interface is not explict enough. We should define the operation of each function before implementing other storages.
  4. We should make the in-memory cache at the level of cachemanager, which is used to optimize the cache query effiency, because there're two many kinds that can read/write storage, it's not easy to manage the in-memory cache. But at the cachemanager layer, only CacheResponse will write to the storage and QueryCache will read from the storage, the management of in-memory cache will be much more easier.

@Congrool
Copy link
Member Author

Currently, it's for preview and has never been tested.

@Congrool
Copy link
Member Author

Congrool commented Jun 14, 2022

TODO List:

  • revise unit tests
  • check the cache ability in actual environment

@Congrool Congrool changed the title [WIP] refactor yurthub cache components in order to use gernal backend storage [WIP]refactor yurthub cache components in order to use gernal backend storage Jun 14, 2022
@openyurt-bot openyurt-bot added do-not-merge/work-in-progress do-not-merge/work-in-progress kind/enhancement kind/enhancement sig/storage sig/storage size/XXL labels Jun 14, 2022
@Congrool
Copy link
Member Author

Proposal: #897

@codecov
Copy link

codecov bot commented Jul 17, 2022

Codecov Report

Merging #882 (d7b5398) into master (775d69d) will increase coverage by 1.14%.
The diff coverage is 63.90%.

@@            Coverage Diff             @@
##           master     #882      +/-   ##
==========================================
+ Coverage   50.78%   51.92%   +1.14%     
==========================================
  Files          96       98       +2     
  Lines       12795    13371     +576     
==========================================
+ Hits         6498     6943     +445     
- Misses       5754     5846      +92     
- Partials      543      582      +39     
Flag Coverage Δ
unittests 51.92% <63.90%> (+1.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/yurthub/certificate/hubself/fake_cert_mgr.go 0.00% <0.00%> (ø)
pkg/yurthub/filter/servicetopology/filter.go 0.00% <0.00%> (ø)
pkg/yurthub/healthchecker/node_lease.go 91.00% <ø> (ø)
pkg/yurthub/proxy/remote/remote.go 1.35% <0.00%> (ø)
pkg/yurthub/util/util.go 80.15% <ø> (+2.22%) ⬆️
pkg/yurthub/certificate/hubself/cert_mgr.go 20.43% <6.66%> (+0.38%) ⬆️
pkg/yurthub/proxy/local/local.go 70.13% <33.33%> (ø)
pkg/yurthub/proxy/util/util.go 80.07% <45.83%> (-7.55%) ⬇️
pkg/yurthub/healthchecker/health_checker.go 79.86% <52.00%> (-6.09%) ⬇️
pkg/yurthub/kubernetes/meta/restmapper.go 74.07% <55.17%> (-2.40%) ⬇️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Congrool Congrool changed the title [WIP]refactor yurthub cache components in order to use gernal backend storage [WIP]refactor yurthub cache to adapt different storages Jul 17, 2022
@Congrool
Copy link
Member Author

Congrool commented Jul 25, 2022

I reconsider the timing when merge this pr. It seems more work need to be done than original thought. Is it better to merge this pr after v1.0?

Meanwhile, I'll continue my work about autonomy tests, #778 and #857 based on this pr.

@Congrool Congrool force-pushed the poolcache branch 5 times, most recently from 82c5fa8 to 7fbc8f9 Compare November 2, 2022 08:37
@Congrool Congrool changed the title [WIP]refactor yurthub cache to adapt different storages refactor yurthub cache to adapt different storages Nov 2, 2022
@openyurt-bot openyurt-bot removed the do-not-merge/work-in-progress do-not-merge/work-in-progress label Nov 2, 2022
@Congrool
Copy link
Member Author

Congrool commented Nov 4, 2022

I've rebased the pr of autonomy e2e tests, and seems that all tests can pass. I think it's ready for review, PTAL @rambohe-ch
And the proposal #897 has been updated, motivations of most modifications are described in it.

@openyurt-bot openyurt-bot added the approved approved label Nov 21, 2022
@rambohe-ch
Copy link
Member

/lgtm
/approve

@openyurt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Congrool, rambohe-ch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved approved kind/enhancement kind/enhancement lgtm lgtm sig/storage sig/storage size/XXL
Projects
None yet
3 participants