Enhance YurtHub's caching ability #225

qclc · 2021-02-23T02:51:20Z

Ⅰ. Describe what this PR does

Replace the original serializer.WithoutConversionCodecFactory with a custom WithVersionCodecFactory so that the GVK information can be preserved during deserialization.
Delete both the resourceToKindMap and the resourceToListKindMap
On the basis of the existing NegotiatedSerializer increase a new unstructuredNegotiatedSerializer, used to decode the structure of the unregistered Custom Resources.

Ⅱ. Does this pull request fix one issue?

fixes #162
This PR extends the ability to cache CR resources, removing the restriction that only resources in resourceToKindMap and resourceToListKindMap can be cached.

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Unit tests for CR caching and getting have been added to cache_manager_test.go;

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

rambohe-ch · 2021-02-23T03:06:05Z

@qclc Very much appreciate for your work that refactor cache manager in yurthub component.
Would you fix the error of github actions at first?
and please merge all commits in only one commit.

qclc · 2021-02-23T03:11:11Z

@qclc Very much appreciate for your work that refactor cache manager in yurthub component.
Would you fix the error of github actions at first?
and please merge all commits in only one commit.

Thanks for your suggestions, I will fix the error and merge all commits.

pkg/yurthub/cachemanager/cache_manager.go

rambohe-ch · 2021-03-15T08:57:07Z

pkg/yurthub/cachemanager/cache_manager.go

+	} else {
+		tmp := new(unstructured.UnstructuredList)
+		tmp.SetGroupVersionKind(listGvk)
+		listObj = tmp


how about delete tmp var and like that:

listObj = new(unstructured.UnstructuredList) listObj.GetObjectKind().SetGroupVersionKind(listGvk)

code is already updated, thanks for review

rambohe-ch · 2021-03-15T08:59:03Z

pkg/yurthub/cachemanager/cache_manager.go

+		tmp := new(unstructured.UnstructuredList)
+		tmp.SetGroupVersionKind(listGvk)
+		listObj = tmp
+	}
 	if err != nil {


how about move this if statement after 163 line(listObj, err = scheme.Scheme.New(listGvk))?

code is already updated, thanks for review

maybe comment is confusing, The if err !=nil statement is only need by listObj, err = scheme.Scheme.New(listGvk), so move into the following if statement.

if scheme.Scheme.Recognizes(listGvk) { listObj, err = scheme.Scheme.New(listGvk) if err != nil { .... }

I see. Thanks for reminding me

rambohe-ch · 2021-03-15T09:46:50Z

pkg/yurthub/kubernetes/serializer/serializer.go

+// Decode does not do conversion. It keeps the gvk during deserialization.
+func (d WithVersionDecoder) Decode(data []byte, defaults *schema.GroupVersionKind, into runtime.Object) (runtime.Object, *schema.GroupVersionKind, error) {
+	obj, gvk, err := d.Decoder.Decode(data, defaults, into)
+	return obj, gvk, err


how about return d.Decoder.Decode(data, defaults, into) directly ?

code is already updated, thanks for review

pkg/yurthub/cachemanager/cache_manager.go

rambohe-ch · 2021-03-15T13:06:02Z

pkg/yurthub/kubernetes/serializer/serializer.go

-	mediaTypes := sm.NegotiatedSerializer.SupportedMediaTypes()
+func (sm *SerializerManager) CreateSerializers(contentType, group, version, apiPrefix string) (*rest.Serializers, error) {
+	var mediaTypes []runtime.SerializerInfo
+	if apiPrefix == "api" {


I think there are some normal gvk with apiPrefix="api" and contentType="xxx.protobuf.xxx", and CreateSerializers can not handle them.

my suggestion: use gvr + restMapper to get gvk, so select serializer based on gvk.

code is already updated, thanks for review

rambohe-ch · 2021-03-15T13:10:33Z

pkg/yurthub/cachemanager/storage_wrapper.go

@@ -145,7 +156,18 @@ func (sw *storageWrapper) List(key string) ([]runtime.Object, error) {
 	}

 	for i := range bb {
-		obj, gvk, err := sw.backendSerializer.Decode(bb[i], nil, nil)
+		//get the gvk from json data
+		gvk, err := json.DefaultMetaFactory.Interpret(bb[i])


How about move Interpret out of for loop, so run Interpret only one time.

code is already updated, thanks for review

rambohe-ch · 2021-03-17T10:28:14Z

pkg/yurthub/cachemanager/cache_manager.go

@@ -248,20 +221,21 @@ func (cm *cacheManager) saveWatchObject(ctx context.Context, info *apirequest.Re

 	comp, _ := util.ClientComponentFrom(ctx)
 	reqContentType, _ := util.ReqContentTypeFrom(ctx)
-	serializers, err := cm.serializerManager.CreateSerializers(reqContentType, info.APIGroup, info.APIVersion)
+	serializers, err := cm.serializerManager.CreateSerializers(reqContentType, "", "v1", "events")


I think we need a serializer for WatchEvent, so maybe need to use cm.serializerManager.CreateSerializers(reqContentType, info.APIGroup, info.APIVersion, "watchevent") to create serializer.

and before making a pr we need to build a yurthub image and make sure the new image can work no problem.

Code is already updated, thanks for review

rambohe-ch · 2021-03-17T10:35:04Z

pkg/yurthub/kubernetes/serializer/serializer.go

+	// our resources.
+	for _, gv := range defaultGroupVersions {
+		for kind := range scheme.KnownTypes(gv) {
+			scope := meta.RESTScopeNamespace


I think that scopes for all gvk are meta.RESTScopeNamespace is not make sense.

Code is already updated, thanks for review

rambohe-ch · 2021-04-01T03:11:36Z

pkg/yurthub/cachemanager/cache_manager_test.go

+				ns:   "default",
+				kind: "CronTab",
+			},
+		},


how about add a namespaced=false test case?

Test cases is already added, thanks for review.

rambohe-ch · 2021-04-01T03:39:14Z

pkg/yurthub/cachemanager/handler_layer.go

@@ -0,0 +1,117 @@
+package cachemanager


please add OpenYurt license info

License info is already added, thanks for review.

rambohe-ch · 2021-04-01T07:00:39Z

pkg/yurthub/cachemanager/handler_layer.go

+	handlers := hl.GetHandlers(gvk)
+	if handlers != nil {
+		var newobj runtime.Object
+		newobj = obj


newobj is redundant, how about use obj directly?

Code is already updated, thanks for review

rambohe-ch · 2021-04-01T07:00:51Z

pkg/yurthub/cachemanager/handler_layer.go

+		var newobj runtime.Object
+		newobj = obj
+		for _, handler := range handlers {
+			tmp, err := handler.ServeObject(newobj)


tmp is the same as newobj

Code is already updated, thanks for review

rambohe-ch · 2021-04-01T07:02:03Z

pkg/yurthub/cachemanager/handler_layer.go

+}
+
+//SelectAndProcess determines whether an object meets the filter criteria and processes it if it does
+func (hl *HandlerLayer) SelectAndProcess(obj runtime.Object) (runtime.Object, bool, error) {


The caller of SelectAndProcess is not found?

Code is already updated, change SelectAndProcess to Filter function

rambohe-ch · 2021-04-01T07:07:16Z

pkg/yurthub/cachemanager/handler_layer.go

+type HandlerLayer struct {
+	sync.Mutex
+	handlers  map[schema.GroupVersionKind][]Handler
+	selectors map[schema.GroupVersionKind]*storage.SelectionPredicate


Use SelectionPredicate is a good idea, but I think that we need to filter object not only based on labels and fields. How about define a common filter function? like Filter(obj runtime.Object) bool, and SelectionPredicate is only a specific case.

The storage.AttrFunc of SelectionPredicate is used to extract the corresponding field data from the actual Object, which is defined by the user. It can basically meet the matching of all fields of runtime.Object. The newly added hanler_layer_test.go has concrete examples to refer to

rambohe-ch

LGTM

Enhance YurtHub's caching ability

qclc mentioned this pull request Feb 23, 2021

[feature request]all resources can be cached by yurt-hub #162

Closed

qclc force-pushed the deleteResourceToKind branch 2 times, most recently from 6c0d780 to 9aaa238 Compare February 23, 2021 07:53

qclc mentioned this pull request Mar 1, 2021

Proposal: enhance the caching ability of Yurthub #231

Closed

qclc changed the title ~~Delete resource to kind~~ Enhance Yuthub's caching capabilities Mar 15, 2021

qclc force-pushed the deleteResourceToKind branch from ab5eed7 to 8e46456 Compare March 15, 2021 03:51

qclc changed the title ~~Enhance Yuthub's caching capabilities~~ Enhance YurtHub's caching ability Mar 15, 2021

qclc force-pushed the deleteResourceToKind branch from 8e46456 to 6a95bed Compare March 15, 2021 08:36

rambohe-ch requested changes Mar 15, 2021

View reviewed changes

qclc force-pushed the deleteResourceToKind branch 2 times, most recently from 6171e97 to 1491e3f Compare March 16, 2021 15:38

rambohe-ch reviewed Mar 17, 2021

View reviewed changes

qclc mentioned this pull request Mar 23, 2021

Proposal: enhance the caching ability of YurtHub #244

Merged

qclc force-pushed the deleteResourceToKind branch 4 times, most recently from 352e7a4 to 1301a90 Compare March 29, 2021 16:27

rambohe-ch reviewed Apr 1, 2021

View reviewed changes

qclc force-pushed the deleteResourceToKind branch from 1301a90 to ec500a2 Compare April 5, 2021 08:11

Enhance Yurthub caching ability

4228462

qclc force-pushed the deleteResourceToKind branch from ec500a2 to 4228462 Compare April 21, 2021 02:51

rambohe-ch approved these changes Apr 21, 2021

View reviewed changes

rambohe-ch merged commit 6aef72b into openyurtio:master Apr 21, 2021

qclc mentioned this pull request May 31, 2021

[REQUEST Membership]: New membership for qclc openyurtio/community#7

Closed

6 tasks

qclc mentioned this pull request Sep 14, 2021

Add qclc as a reviewer of openyurt #469

Merged

6 tasks

qclc mentioned this pull request Nov 12, 2021

Add qclc as maintainer of OpenYurt #580

Merged

6 tasks

MrGirl pushed a commit to MrGirl/openyurt that referenced this pull request Mar 29, 2022

Merge pull request openyurtio#225 from qclc/deleteResourceToKind

644179e

Enhance YurtHub's caching ability

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance YurtHub's caching ability #225

Enhance YurtHub's caching ability #225

qclc commented Feb 23, 2021 •

edited

Loading

rambohe-ch commented Feb 23, 2021

qclc commented Feb 23, 2021

rambohe-ch Mar 15, 2021

qclc Mar 16, 2021

rambohe-ch Mar 15, 2021

qclc Mar 16, 2021

rambohe-ch Mar 16, 2021

qclc Mar 16, 2021

rambohe-ch Mar 15, 2021

qclc Mar 16, 2021

rambohe-ch Mar 15, 2021

rambohe-ch Mar 15, 2021

qclc Mar 16, 2021

rambohe-ch Mar 15, 2021

qclc Mar 16, 2021

rambohe-ch Mar 17, 2021 •

edited

Loading

qclc Apr 5, 2021

rambohe-ch Mar 17, 2021

qclc Apr 5, 2021

rambohe-ch Apr 1, 2021

qclc Apr 5, 2021

rambohe-ch Apr 1, 2021

qclc Apr 5, 2021

rambohe-ch Apr 1, 2021

qclc Apr 5, 2021

rambohe-ch Apr 1, 2021 •

edited

Loading

qclc Apr 5, 2021

rambohe-ch Apr 1, 2021

qclc Apr 5, 2021

rambohe-ch Apr 1, 2021 •

edited

Loading

qclc Apr 5, 2021 •

edited

Loading

rambohe-ch left a comment

Enhance YurtHub's caching ability #225

Enhance YurtHub's caching ability #225

Conversation

qclc commented Feb 23, 2021 • edited Loading

Ⅰ. Describe what this PR does

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

rambohe-ch commented Feb 23, 2021

qclc commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rambohe-ch Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rambohe-ch Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rambohe-ch Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

qclc Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

rambohe-ch left a comment

Choose a reason for hiding this comment

qclc commented Feb 23, 2021 •

edited

Loading

rambohe-ch Mar 17, 2021 •

edited

Loading

rambohe-ch Apr 1, 2021 •

edited

Loading

rambohe-ch Apr 1, 2021 •

edited

Loading

qclc Apr 5, 2021 •

edited

Loading