-
Notifications
You must be signed in to change notification settings - Fork 406
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 cache manager for yurthub #265
Conversation
7c18345
to
2b60ff7
Compare
for key, data := range contents { | ||
err := ds.create(key, data) | ||
if err != nil { | ||
klog.Errorf("failed to create %s in replace, %v", key, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If error happens, do we have retry mechanism? If not, do we have mechanism to do consistency check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not seriously thought about how to guarantee that object can be cached successfully. so i'd like to keep this subject as a TODO item. and we can solve it in the soon future.
if info.Verb == "list" && info.Name == "" { | ||
key, _ := util.KeyFunc(comp, info.Resource, info.Namespace, info.Name) | ||
if _, ok := cm.listReqCollector[key]; !ok { | ||
for k := range cm.listReqCollector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the purpose of having listReqCollector?
Also, can you give an example that "len(k) > len (key) " and an example for "len(k) < len(key)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- because func queryListObject() will get all pods for both requests instead of getting pods by request selector. so cache manager can not support same path list requests that has different selector. for example:
- request1: http://{ip:port}/api/v1/default/pods?labelSelector=foo=bar
- request2: http://{ip:port}/api/v1/default/pods?labelSelector=foo2=bar2
- because func queryListObject() will get all pods for both requests instead of getting pods by request selector. so cache manager can not support getting same resource list requests that has different path. for example:
- request1: http://{ip/port}/api/v1/pods?fieldSelector=spec.nodeName=foo
- request2: http://{ip/port}/api/v1/default/pods?fieldSelector=spec.nodeName=foo
in order to solve the above two problems, we need listReqCollector to keep list request info, and based on listReqCollector to verify list requests can cache or not.
Also, can you give an example that "len(k) > len (key) " and an example for "len(k) < len(key)"?
the second above example.
2b60ff7
to
8cb6608
Compare
1. replace local cache response for list request 2. fix cache conflict for labelselector/fieldselector list and full list.
8cb6608
to
5f4fad2
Compare
1. replace local cache response for list request 2. fix cache conflict for labelselector/fieldselector list and full list.
Ⅰ. Describe what this PR does
Backgound:
fieldselector=metadata.name=xxx
, the request path like:https://{ip:port}/api/v1/namespaces/default/pods?fieldselector=metadata.name=xxx
fieldselector=spec.nodeName=xxx
orlabelselector=foo=bar
, the request path like:https://{ip:port}/api/v1/pods?fieldselector=spec.nodeName=xxx
https://{ip:port}/api/v1/pods
problem:
solution:
Ⅱ. Does this pull request fix one issue?
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
make test
make all
and make sure yurthub cache manager can work
Ⅴ. Special notes for reviews