✨ Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller #2452

davidfestal · 2022-12-05T15:45:27Z

Summary

PROOF PR of a refactoring of the Syncer to use common ddsif informer factories for all the Syncer + other controllers.
It also includes a controller manager, which allows starting / stopping GVR-specific controllers when new GVRs are supported in the list of Synced resources.

It is based on the work done on the ddsif in PR #2440

Related issue(s)

No related issue. But it is a pre-requisite to the merge of PRs like #2338

openshift-ci · 2022-12-05T15:45:32Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ncdc · 2022-12-16T22:18:50Z

/hold

pending workspace refactor

sttts · 2023-01-10T09:23:40Z

pkg/informer/informer.go

 // known by this informer factory, and that are synced.
 //
 // If any informers aren't synced, their GVRs are returned so that they can be
 // checked and processed later.
-func (d *GenericDiscoveringDynamicSharedInformerFactory[Informer, Lister, GenericInformer]) Listers() (listers map[schema.GroupVersionResource]Lister, notSynced []schema.GroupVersionResource) {
-	listers = map[schema.GroupVersionResource]Lister{}
+func (d *GenericDiscoveringDynamicSharedInformerFactory[Informer, Lister, GenericInformer]) Informers() (informers map[schema.GroupVersionResource]GenericInformer, notSynced []schema.GroupVersionResource) {


👍, much better

sttts · 2023-01-10T09:29:31Z

pkg/syncer/controllermanager/controllermanager.go

+		}
+
+		// Create the controller
+


nit: no empty line

sttts · 2023-01-10T09:37:36Z

pkg/syncer/syncer.go

+	go upstreamUpsyncerControllerManager.Start(ctx)
+
+	downstreamSyncerControllerManager := controllermanager.NewControllerManager(ctx,
+		"downstream",


nit: downstream-syncer to be consistent

sttts · 2023-01-10T09:42:20Z

pkg/syncer/syncer.go

@@ -134,9 +141,18 @@ func StartSyncer(ctx context.Context, cfg *SyncerConfig, numSyncerThreads int, i

 	upstreamConfig := rest.CopyConfig(cfg.UpstreamConfig)
 	upstreamConfig.Host = syncerVirtualWorkspaceURL
-	rest.AddUserAgent(upstreamConfig, "kcp#spec-syncer/"+kcpVersion)
+	rest.AddUserAgent(upstreamConfig, "kcp#syncing/"+kcpVersion)


don't think that's a valid user agent.

It seems that the way user agents are set in the Syncer should be reworked globally to bring back both inner consistency, and also consistency with other KCP components.

Dedicated issue created: https://github.com/kcp-dev/kcp/issues/2581

Signed-off-by: David Festal

Signed-off-by: David Festal <dfestal@redhat.com>

... of a GVR-specific controller driven by the controller manager. Signed-off-by: David Festal <dfestal@redhat.com>

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2023-01-10T10:47:59Z

/test test

jmprusi · 2023-01-10T11:32:37Z

/lgtm

sttts · 2023-01-10T12:06:00Z

/retest
/approve

openshift-ci · 2023-01-10T12:06:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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:

~~OWNERS~~ [sttts]

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

sttts · 2023-01-10T12:06:27Z

Failing DNS tests twice. Coincidence?

davidfestal · 2023-01-10T12:17:35Z

Failing DNS tests twice. Coincidence?

Unfortunately I approved PR #2542 yesterday (GH tests were successful), but in fact it might be that it needed some rebase first. And now the related tests are failing. So it doesn't seem to be related to this PR, but I still have to check.

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2023-01-10T12:57:49Z

Failing DNS tests twice. Coincidence?

Unfortunately I approved PR #2542 yesterday (GH tests were successful), but in fact it might be that it needed some rebase first. And now the related tests are failing. So it doesn't seem to be related to this PR, but I still have to check.

Well, in fact the test is perfect and worked exactly as expected. it was this PR having a bug. See commit 3de7ac8

Great and very useful e2e test @lionelvillard !

jmprusi · 2023-01-10T13:43:20Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2022

davidfestal force-pushed the refactor-syncer-resource-controller branch 8 times, most recently from 3aac818 to a548f62 Compare December 14, 2022 21:00

davidfestal changed the title ~~WIP Refactor syncer resource controller based on the ddsif~~ PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller Dec 14, 2022

davidfestal changed the title ~~PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller~~ ✨ PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller Dec 14, 2022

davidfestal requested review from jmprusi and stevekuznetsov and removed request for stevekuznetsov December 14, 2022 21:06

davidfestal marked this pull request as ready for review December 14, 2022 21:07

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2022

openshift-ci bot requested a review from s-urbaniak December 14, 2022 21:07

davidfestal changed the title ~~✨ PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller~~ ✨ WIP PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller Dec 14, 2022

davidfestal added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/transparent-multi-cluster Related to scheduling of workloads into pclusters. labels Dec 14, 2022

davidfestal force-pushed the refactor-syncer-resource-controller branch from a548f62 to b649de6 Compare December 15, 2022 09:14

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022

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

davidfestal force-pushed the refactor-syncer-resource-controller branch 2 times, most recently from 3048f4e to c2d8575 Compare January 3, 2023 14:31

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2023

davidfestal force-pushed the refactor-syncer-resource-controller branch 2 times, most recently from 74b99ad to d7913f9 Compare January 3, 2023 16:02

davidfestal changed the title ~~✨ WIP PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller~~ ✨ PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller Jan 9, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2023

davidfestal mentioned this pull request Jan 9, 2023

✨ Synctarget: Have URLs for both syncer and upsyncer #2570

Merged

sttts reviewed Jan 10, 2023

View reviewed changes

pkg/syncer/controllermanager/controllermanager.go Outdated

}

// Create the controller

Copy link

Member

sttts Jan 10, 2023

Choose a reason for hiding this comment

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

nit: no empty line

sttts reviewed Jan 10, 2023

View reviewed changes

davidfestal added 5 commits January 10, 2023 11:25

Syncer: separate resource controller and informers

c5a4b0f

Signed-off-by: David Festal

Syncer: add controller manager

8e1fe9c

Signed-off-by: David Festal <dfestal@redhat.com>

Syncer: Wire the Upsyncer ddsif

104dc47

Signed-off-by: David Festal <dfestal@redhat.com>

Syncer: add endpoints controller as an example...

7f76f5e

... of a GVR-specific controller driven by the controller manager. Signed-off-by: David Festal <dfestal@redhat.com>

Fix PR comments

a22427d

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal force-pushed the refactor-syncer-resource-controller branch from 5aed415 to a22427d Compare January 10, 2023 10:38

davidfestal changed the title ~~✨ PROOF: Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller~~ ✨ Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller Jan 10, 2023

openshift-ci bot assigned jmprusi Jan 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2023

davidfestal mentioned this pull request Jan 10, 2023

🌱 Debug dns e2e test failures #2585

Closed

Fix wrong informer use

3de7ac8

Signed-off-by: David Festal <dfestal@redhat.com>

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2023

openshift-merge-robot merged commit 4b36a83 into kcp-dev:main Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller #2452

✨ Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller #2452

davidfestal commented Dec 5, 2022 •

edited

Loading

openshift-ci bot commented Dec 5, 2022

ncdc commented Dec 16, 2022

sttts Jan 10, 2023

sttts Jan 10, 2023

sttts Jan 10, 2023

sttts Jan 10, 2023

davidfestal Jan 10, 2023

davidfestal commented Jan 10, 2023

jmprusi commented Jan 10, 2023

sttts commented Jan 10, 2023

openshift-ci bot commented Jan 10, 2023

sttts commented Jan 10, 2023

davidfestal commented Jan 10, 2023

davidfestal commented Jan 10, 2023

jmprusi commented Jan 10, 2023

✨ Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller #2452

✨ Refactor Syncer based on the enhanced ddsif, with controller manager and endpoints controller #2452

Conversation

davidfestal commented Dec 5, 2022 • edited Loading

Summary

Related issue(s)

openshift-ci bot commented Dec 5, 2022

ncdc commented Dec 16, 2022

sttts Jan 10, 2023

Choose a reason for hiding this comment

sttts Jan 10, 2023

Choose a reason for hiding this comment

sttts Jan 10, 2023

Choose a reason for hiding this comment

sttts Jan 10, 2023

Choose a reason for hiding this comment

davidfestal Jan 10, 2023

Choose a reason for hiding this comment

davidfestal commented Jan 10, 2023

jmprusi commented Jan 10, 2023

sttts commented Jan 10, 2023

openshift-ci bot commented Jan 10, 2023

sttts commented Jan 10, 2023

davidfestal commented Jan 10, 2023

davidfestal commented Jan 10, 2023

jmprusi commented Jan 10, 2023

davidfestal commented Dec 5, 2022 •

edited

Loading