✨ feature: sync resources from syncer virtual server #1995

qiujian16 · 2022-09-15T08:22:35Z

Signed-off-by: Jian Qiu jqiu@redhat.com

Summary

In addition to sync resource based on resources flag on syncer, also read the syncedResource on synctarget. It requires to start spec/status syncer per gvr dynamically. With this change, user can specify the apiexport on synctarget and do not need to set resource flag on syncer any more.

Related issue(s)

Fixes #1888

pkg/syncer/apiimporter.go

stevekuznetsov

Are we certain that a reconciler process per synctarget like this is the most efficient way to do it?

pkg/syncer/apiimporter.go

pkg/syncer/resourcesync/controller.go

stevekuznetsov · 2022-09-15T15:36:17Z

pkg/syncer/resourcesync/controller.go

+		return nil
+	}
+
+	syncerVirtualWorkspaceURL := syncTarget.Status.VirtualWorkspaces[0].URL


Is it too early to handle the N>1 case?

pkg/syncer/resourcesync/controller.go

stevekuznetsov · 2022-09-15T15:42:11Z

pkg/syncer/resourcesync/controller.go

+
+	syncTargetKey := workloadv1alpha1.ToSyncTargetKey(c.syncTargetWorkspace, c.syncTargetName)
+
+	upstreamInformers := dynamicinformer.NewFilteredDynamicSharedInformerFactoryWithOptions(c.upstreamDynamicClusterClient.Cluster(logicalcluster.Wildcard), metav1.NamespaceAll, func(o *metav1.ListOptions) {


Is it at all possible to have one set of informers and to defer filtering? Do we care about the load here?

The filter is to handle the certain synctarget only.

stevekuznetsov · 2022-09-15T15:43:08Z

pkg/syncer/resourcesync/controller.go

+}
+
+func getAllGVRs(synctarget *workloadv1alpha1.SyncTarget) (map[schema.GroupVersionResource]bool, error) {
+	// add configmap and secrets by default


Why? We have a list of built-in types that's more than just this, and regardless don't we have e.g. the kubernetes export with these bits?

This is from the old code :) https://github.com/kcp-dev/kcp/blob/main/pkg/syncer/syncer.go#L292. I think the reason is we do not sync cluster scoped and we do not sync sa. The kubernetes export does not include configmaps and secrets.

qiujian16 · 2022-09-16T08:59:00Z

Are we certain that a reconciler process per synctarget like this is the most efficient way to do it?

would you explain more on the question? I think the syncer process is to handle a single synctarget.

stevekuznetsov · 2022-09-16T13:13:14Z

@qiujian16 do we think at a fundamental design level this could be done by dynamically managing the lifecycle of informer's handlers in the syncer controllers as-is? E.G. when the spec or status syncer sees that the set of GVRs to manage changes, start & stop handling events on the correct informers. That would allow us to have fewer controllers happening here and likely make the approach a little simpler.

ncdc · 2022-09-16T13:13:18Z

@qiujian16 could you please explain what you're doing here? 😄 I'm curious why you are starting 1 set of controllers per GVR?

I think you could accomplish the same thing by just filtering out "invalid" GVRs when processing resources, instead of starting lots and lots of controllers?

qiujian16 · 2022-09-17T10:09:15Z

@qiujian16 could you please explain what you're doing here? 😄 I'm curious why you are starting 1 set of controllers per GVR?

I think you could accomplish the same thing by just filtering out "invalid" GVRs when processing resources, instead of starting lots and lots of controllers?

we do not know what is a "invalid" GVR, the objective is the syncer could know from synctarget API (or maybe discovery API from the syncer virtual workspace) what are the GVRs to be synced from the upstream. The GVRs list can be pretty dynamic. so we need to dynamically add/remove upstream/downstream informer per gvr for spec/status sync.

qiujian16 · 2022-09-17T10:14:52Z

@qiujian16 do we think at a fundamental design level this could be done by dynamically managing the lifecycle of informer's handlers in the syncer controllers as-is? E.G. when the spec or status syncer sees that the set of GVRs to manage changes, start & stop handling events on the correct informers. That would allow us to have fewer controllers happening here and likely make the approach a little simpler.

I think we could. We still need to start/stop informer per gvr based on synctarget API. But we could just set eventHandler for the spec/status sync, instead of start spec/status sync per gvr. Does it sound sensible?

qiujian16 · 2022-09-19T08:38:03Z

@stevekuznetsov @ncdc PTAL again, change to avoid creating spec/status syncer per gvr

pkg/syncer/apiimporter.go

pkg/syncer/resourcesync/controller.go

stevekuznetsov · 2022-09-20T13:58:33Z

pkg/syncer/resourcesync/controller.go

+	defer c.mutex.Unlock()
+
+	if _, ok := c.syncerInformerMap[gvr]; ok {
+		return


how do you handle the case where the set of resources for a synctarget changes.?

in sync target, we only track per gvr. If a gvr is included in the syncTarget, we start the informer when it is not started.

qiujian16 · 2022-09-21T07:54:54Z

/retest

qiujian16 · 2022-09-28T08:37:50Z

/retest

pkg/cliplugins/workload/plugin/sync.go

pkg/syncer/resourcesync/controller.go

davidfestal · 2022-09-28T15:07:33Z

Some minor comments, but apart from that LGTM

davidfestal

As we discussed during the meeting, do you think we could:

issue a SSAR before adding a GVR to the map of syncer informers (here ?)
Trigger a new reconcile (every 5 or 10 minutes) for resources that have not the expected permissions
Add the related tests

Signed-off-by: Jian Qiu <jqiu@redhat.com>

qiujian16 · 2022-09-30T03:16:54Z

As we discussed during the meeting, do you think we could:

Trigger a new reconcile (every 5 or 10 minutes) for resources that have not the expected permissions

I think we already have heartbeat that will trigger the reconcile here every 20s.

Signed-off-by: Jian Qiu <jqiu@redhat.com>

qiujian16 · 2022-09-30T08:35:25Z

ssar check and related test added.

openshift-ci · 2022-10-04T13:22:17Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: davidfestal

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:

~~hack/OWNERS~~ [davidfestal]
pkg/apis/OWNERS
~~pkg/cliplugins/OWNERS~~ [davidfestal]
~~pkg/syncer/OWNERS~~ [davidfestal]
~~test/OWNERS~~ [davidfestal]

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

openshift-ci bot requested review from csams and davidfestal September 15, 2022 08:22

qiujian16 force-pushed the dynamic-syncer branch from 962d0af to 8f0061b Compare September 15, 2022 08:44

stevekuznetsov reviewed Sep 15, 2022

View reviewed changes

pkg/syncer/apiimporter.go Outdated Show resolved Hide resolved

stevekuznetsov reviewed Sep 15, 2022

View reviewed changes

qiujian16 force-pushed the dynamic-syncer branch from 8f0061b to fb90e1d Compare September 16, 2022 08:55

qiujian16 force-pushed the dynamic-syncer branch 3 times, most recently from 9004f53 to e974da8 Compare September 16, 2022 10:16

qiujian16 force-pushed the dynamic-syncer branch 4 times, most recently from cefc822 to 75ca22e Compare September 19, 2022 08:31

stevekuznetsov reviewed Sep 20, 2022

View reviewed changes

qiujian16 force-pushed the dynamic-syncer branch from 75ca22e to b2bbc31 Compare September 21, 2022 03:55

qiujian16 force-pushed the dynamic-syncer branch 8 times, most recently from 76d5a87 to 79c24d8 Compare September 23, 2022 05:29

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2022

qiujian16 force-pushed the dynamic-syncer branch 6 times, most recently from 98f44f5 to c3b6f9c Compare September 28, 2022 08:10

davidfestal reviewed Sep 28, 2022

View reviewed changes

pkg/cliplugins/workload/plugin/sync.go Outdated Show resolved Hide resolved

pkg/syncer/resourcesync/controller.go Outdated Show resolved Hide resolved

pkg/syncer/resourcesync/controller.go Outdated Show resolved Hide resolved

pkg/syncer/resourcesync/controller.go Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022

qiujian16 force-pushed the dynamic-syncer branch from c3b6f9c to dda8b95 Compare September 29, 2022 04:26

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 29, 2022

davidfestal suggested changes Sep 29, 2022

View reviewed changes

openshift-ci bot assigned davidfestal Sep 29, 2022

qiujian16 added 2 commits September 30, 2022 09:16

sync resources from syncer virtual server

d9ba6fd

Signed-off-by: Jian Qiu <jqiu@redhat.com>

Fix typos and remove unnecessary filter

3fdebef

Signed-off-by: Jian Qiu <jqiu@redhat.com>

qiujian16 force-pushed the dynamic-syncer branch from dda8b95 to a249760 Compare September 30, 2022 01:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2022

Add ssar check before starting informer

d1aafb3

Signed-off-by: Jian Qiu <jqiu@redhat.com>

qiujian16 force-pushed the dynamic-syncer branch from a249760 to d1aafb3 Compare September 30, 2022 04:26

davidfestal approved these changes Oct 4, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2022

davidfestal added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2022

openshift-merge-robot merged commit 7cda1cb into kcp-dev:main Oct 4, 2022

qiujian16 deleted the dynamic-syncer branch October 8, 2022 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ feature: sync resources from syncer virtual server #1995

✨ feature: sync resources from syncer virtual server #1995

qiujian16 commented Sep 15, 2022

stevekuznetsov left a comment

stevekuznetsov Sep 15, 2022

stevekuznetsov Sep 15, 2022

qiujian16 Sep 16, 2022

stevekuznetsov Sep 15, 2022

qiujian16 Sep 16, 2022

qiujian16 commented Sep 16, 2022

stevekuznetsov commented Sep 16, 2022

ncdc commented Sep 16, 2022

qiujian16 commented Sep 17, 2022

qiujian16 commented Sep 17, 2022

qiujian16 commented Sep 19, 2022

stevekuznetsov Sep 20, 2022

qiujian16 Sep 20, 2022

qiujian16 commented Sep 21, 2022

qiujian16 commented Sep 28, 2022

davidfestal commented Sep 28, 2022

davidfestal left a comment

qiujian16 commented Sep 30, 2022

qiujian16 commented Sep 30, 2022

openshift-ci bot commented Oct 4, 2022


		syncTargetKey := workloadv1alpha1.ToSyncTargetKey(c.syncTargetWorkspace, c.syncTargetName)

		upstreamInformers := dynamicinformer.NewFilteredDynamicSharedInformerFactoryWithOptions(c.upstreamDynamicClusterClient.Cluster(logicalcluster.Wildcard), metav1.NamespaceAll, func(o *metav1.ListOptions) {

✨ feature: sync resources from syncer virtual server #1995

✨ feature: sync resources from syncer virtual server #1995

Conversation

qiujian16 commented Sep 15, 2022

Summary

Related issue(s)

stevekuznetsov left a comment

Choose a reason for hiding this comment

stevekuznetsov Sep 15, 2022

Choose a reason for hiding this comment

stevekuznetsov Sep 15, 2022

Choose a reason for hiding this comment

qiujian16 Sep 16, 2022

Choose a reason for hiding this comment

stevekuznetsov Sep 15, 2022

Choose a reason for hiding this comment

qiujian16 Sep 16, 2022

Choose a reason for hiding this comment

qiujian16 commented Sep 16, 2022

stevekuznetsov commented Sep 16, 2022

ncdc commented Sep 16, 2022

qiujian16 commented Sep 17, 2022

qiujian16 commented Sep 17, 2022

qiujian16 commented Sep 19, 2022

stevekuznetsov Sep 20, 2022

Choose a reason for hiding this comment

qiujian16 Sep 20, 2022

Choose a reason for hiding this comment

qiujian16 commented Sep 21, 2022

qiujian16 commented Sep 28, 2022

davidfestal commented Sep 28, 2022

davidfestal left a comment

Choose a reason for hiding this comment

qiujian16 commented Sep 30, 2022

qiujian16 commented Sep 30, 2022

openshift-ci bot commented Oct 4, 2022