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

Proposal: enhance the caching ability of Yurthub #231

Closed
wants to merge 0 commits into from

Conversation

qclc
Copy link
Member

@qclc qclc commented Mar 1, 2021

Ⅰ. Describe what this PR does

add proposal to enhance the caching ability of Yurthub, which is related to the issue: #162 , mainly include:

  1. delete the resourceToKindMap and resourceToListKindMap , which is related to the pr: Enhance YurtHub's caching ability #225
  2. add unstructuredNegotiatedSerializer to decode the Custom Resource

Ⅱ. 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

Ⅴ. Special notes for reviews


- Delete [resourceToKindMap](https://github.com/openyurtio/openyurt/blob/4d7463a40801c29d09c4f7d10ba46b73cb019915/pkg/yurthub/cachemanager/cache_manager.go#L46) and [resourceToListKindMap](https://github.com/openyurtio/openyurt/blob/4d7463a40801c29d09c4f7d10ba46b73cb019915/pkg/yurthub/cachemanager/cache_manager.go#L62) so that we can remove the restriction that yurthub can only cache certain resources;
- Decode Custom Resources that are not registered in the scheme into `Unstructured` structures;
- The Custom Resource is distinguished from the core resource based on `request.URL`. The core resource uses the existing serializer and the Custom Resource uses the `unstructuredNegotiatedSerializer`.
Copy link
Member

Choose a reason for hiding this comment

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

How about use unstructuredNegotiatedSerializer for all of resources?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unstructured objects still have functioning TypeMeta features-- kind, version, etc, but for private properties that distinguish resources, such as specs, registered structures are required to parse them out in detail. If use unstructuredNegotiatedSerializer to decode all resource, the Yurthub will lose the flexibility to operate core resources.
Or if you mean to use only one NegotiatedSerializer to handle all the resources, our current idea is to later encapsulate the two kind of serializers into one and decide internally which one to use.

Copy link
Member

Choose a reason for hiding this comment

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

Unstructured objects still have functioning TypeMeta features-- kind, version, etc, but for private properties that distinguish resources, such as specs, registered structures are required to parse them out in detail. If use unstructuredNegotiatedSerializer to decode all resource, the Yurthub will lose the flexibility to operate core resources.
Or if you mean to use only one NegotiatedSerializer to handle all the resources, our current idea is to later encapsulate the two kind of serializers into one and decide internally which one to use.

For the sake of flexibility to operate resources, we should use unstructuredNegotiatedSerializer uniformly, and then use NegotiatedSerializer only when resources need to be considered for detail.


Currently Yurthub can only encode and decode resources under these paths:

```go
Copy link
Member

Choose a reason for hiding this comment

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

Would you give the reference file path for following code?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reference is https://github.com/kubernetes/client-go/blob/master/kubernetes/scheme/register.go . I will add this reference in the proposal.

Copy link
Member

Choose a reason for hiding this comment

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

ok

Remove the restriction that only certain resources can be cached by removing [resourceToKindMap](https://github.com/openyurtio/openyurt/blob/4d7463a40801c29d09c4f7d10ba46b73cb019915/pkg/yurthub/cachemanager/cache_manager.go#L46) and [resourceToListKindMap](https://github.com/openyurtio/openyurt/blob/4d7463a40801c29d09c4f7d10ba46b73cb019915/pkg/yurthub/cachemanager/cache_manager.go#L62). Removing the above restriction requires solving the problem which is preserving the original GVK information while decoding the corresponding resources;

The reason why object misses the field of apiverison and kind is YurtHub using [serializer.WithoutConversionCodecFactory](https://github.com/alibaba/openyurt/blob/7095962687e2666a27d14e436b96ef893b228beb/pkg/yurthub/kubernetes/serializer/serializer.go#L48) as NegotiatedSerializer, which produces [WithoutVersionDecoder ](https://github.com/kubernetes/kubernetes/blob/994b5c6cc20bda188ac9bc33217f2dbc7ecf45bb/staging/src/k8s.io/apimachinery/pkg/runtime/helper.go#L256)and remove the apiversion/kind of object when serialized from http.response. The following code is the decoder that is invoked when the resource is decoded:

Copy link
Member

@rambohe-ch rambohe-ch Mar 2, 2021

Choose a reason for hiding this comment

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

When should we use WithoutVersionDecoder? would you give some examples to describe how to use these two Decoder?

Copy link
Member Author

@qclc qclc Mar 2, 2021

Choose a reason for hiding this comment

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

Currently, YurtHub use WithoutConversionCodecFactory as NegotiatedSerializer. Because the WithoutConversionCodecFactory is a CodecFactory that will explicitly ignore requests to perform conversion, the GVK information will be ignored when this WithoutConversionCodecFactory use the decoder (which is this Decoder).
So we're currently adding the new CodecFactory wrapper which is WithVersionCodecFactory to replace the old WithoutConversionCodecFactory as negotiatedSerializer, and rewriting the original Decode function to keep the GVK information.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, YurtHub use WithoutConversionCodecFactory as NegotiatedSerializer. Because the WithoutConversionCodecFactory is a CodecFactory that will explicitly ignore requests to perform conversion, the GVK information will be ignored when this WithoutConversionCodecFactory use the decoder (which is this Decoder).
So we're currently adding the new CodecFactory wrapper which is WithVersionCodecFactory to replace the old WithoutConversionCodecFactory as negotiatedSerializer, and rewriting the original Decode function to keep the GVK information.

And when we can use WithoutVersionDecoder ?

Copy link
Member

@rambohe-ch rambohe-ch left a comment

Choose a reason for hiding this comment

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

Please fix the format miss of markdown file that detected by github actions

@qclc qclc force-pushed the Addproposal branch 2 times, most recently from f51fb6b to cdf2cfd Compare March 2, 2021 03:40
@Fei-Guo
Copy link
Member

Fei-Guo commented Mar 3, 2021

FYI, CRD does not support protobuf (kubernetes/kubernetes#55541). Hence, we still can use unstruct type to handle CRDs which is already very useful in handling quite some use cases. For the built-in types, we probably have to enumerate all of them in case the protobuf is used.

@qclc
Copy link
Member Author

qclc commented Mar 3, 2021

FYI, CRD does not support protobuf (kubernetes/kubernetes#55541). Hence, we still can use unstruct type to handle CRDs which is already very useful in handling quite some use cases. For the built-in types, we probably have to enumerate all of them in case the protobuf is used.

Thanks for your information, it's very helpful. I will think carefully about the relevant suggestions and update my proposal.

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

3 participants