🌱 tmc e2e : Split SyncerFixture #2730

davidfestal · 2023-02-01T18:34:29Z

Summary

Split the SyncerFixture start method to provide a way to only maintain Synctarget heartbeat and import apis, without
effectively syncing upstream resources.

Related issue(s)

No related issue, but this is required in order to avoid conflicting with the e2e tests of the Upsyncer virtual workspace with the implementation of the Upsyncer controller (on the Syncer side) is merged.
This will also allow lighter e2e tests for all the tests that don't need effective syncing of resources to the downstream cluster.

test/e2e/reconciler/locationworkspace/local_apiexport_test.go

pkg/syncer/resourcesync/controller.go

test/e2e/framework/syncer.go

sttts · 2023-02-06T13:55:23Z

test/e2e/framework/syncer.go

+	kcpSyncTargetInformerFactory.Start(ctx.Done())
+	kcpSyncTargetInformerFactory.WaitForCacheSync(ctx.Done())
+
+	go apiImporter.Start(klog.NewContext(ctx, klog.FromContext(ctx).WithValues("resources", resources)), 5*time.Second)


why is the api importer needed in this fixture?

Could we make it optional in another StartAPIImporter method?

I.e. something like syncerFxture.StartHeartBeat(...).StartAPIImport(...)?

Rule of thumb: smaller fixtures

And a fixture without the need of a downstream cluster would be probably a good idea.

For the SyncTarget to be ready to use, we would expected, in most tests, that the API compatibility check has been run and that the Accepted (or Imcompatible in some cases) state would be set to every resource of the synctarget.Status.SyncedResources array.

There are very few e2e tests that don't even require this information in the SyncTarget, even maybe none.

I have separated the 2 functions as you proposed:

syncerFixture.CreateAndStartHeartBeat(t).StartAPIImporter(t)

And a fixture without the need of a downstream cluster would be probably a good idea.

I'd rather keep that for later, since I don't see the need of this right now, and this would involve more coding / refactoring. Do you agree ?

I went a step further and allowed stopping the heatbeat and starting it again. This would be useful for some tests @jmprusi is currently writing.

So the way to create a syncer with APIImports and heatbeats is now:

syncerFixture..Create(t).StartAPIImporter(t).StartHeartBeat(t)

Thanks! worked perfectly for my PR! being able to start/stop the heartbeat allows us to test some "complex" scheduling scenarios!

davidfestal · 2023-02-06T18:49:08Z

@jmprusi Is the new ability to call StopHeartBeat() and call the StartHeartBeat() again later working for your new e2e test case ?

pkg/syncer/syncer.go

test/e2e/framework/syncer.go

sttts · 2023-02-07T09:11:52Z

test/e2e/framework/syncer.go

+// and then starts a new syncer against the given upstream kcp workspace.
+// Whether the syncer runs in-process or deployed on a pcluster will depend
+// whether --pcluster-kubeconfig and --syncer-image are supplied to the test invocation.
+func (sf *syncerFixture) CreateAndStart(t *testing.T) *StartedSyncerFixture {


am confused. How is a SyncTarget applied to the physical cluster?

This looks a whole lot like Create(t).StartSyncer(t), i.e. it overlaps with Create, no?

Yeah, bad wording. The SyncTarget is not what is applied. But the Syncer-related resources are applied in the physical cluster.

Fixed in commit 13a4a06

This looks a whole lot like Create(t).StartSyncer(t), i.e. it overlaps with Create, no?

I changed it to have either:

syncerFixture.Create(t).StartSyncer(t)

or

syncerFixture.Create(t).StartAPIImporter(t).StartHeartBeat(t)

sttts · 2023-02-07T09:15:58Z

test/e2e/framework/syncer.go

+	kcpSyncTargetInformerFactory.Start(ctx.Done())
+	kcpSyncTargetInformerFactory.WaitForCacheSync(ctx.Done())
+
+	go apiImporter.Start(klog.NewContext(ctx, klog.FromContext(ctx).WithValues("resources", resources)), 5*time.Second)


I think I asked before, maybe the comment/answer got lost: why do we have to run a full api importer, why not just set some conditions to fake the outcome?

Afair you asked about replacing the the whole logic of checking authorization for synced resources in the downstream cluster, which drives the "Authorized" condition. This logic I effectively removed, since the Authorized condition is not checked by any KCP controller.

It seems to me that the API importer is something else: It doesn't drive a direct condition, but allows the compatibility check controller, which runs on the KCP side, to set the Accepted status on every resource of the syncTarget.syncedResources array.
If we try to set this status manually on the SyncTarget in the test fixture, the KCP controller will very probably reset the values to incompatible until it has the downstream schemas imported.

See my previous comment: #2730 (comment)

test/e2e/reconciler/namespace/controller_test.go

test/e2e/reconciler/scheduling/api_compatibility_test.go

jmprusi · 2023-02-07T11:16:00Z

/lgtm

test/e2e/framework/syncer.go

... to provide a way to only maintain heartbeat and import apis without effectively syncing. And change all the candidate tests to use the new function. Signed-off-by: David Festal <[email protected]>

Signed-off-by: David Festal <[email protected]>

jmprusi · 2023-02-07T14:04:16Z

@jmprusi Is the new ability to call StopHeartBeat() and call the StartHeartBeat() again later working for your new e2e test case ?

Yes, I'm using it for testing the resource scheduler interaction with upsynced resources (#2533), and being able to stop the syncer and restart is required.

... into 2 functions: `Create()` (already exists) and `StartSyncer()`. Signed-off-by: David Festal <[email protected]>

Signed-off-by: David Festal <[email protected]>

test/e2e/framework/syncer.go

Signed-off-by: David Festal <[email protected]>

sttts · 2023-02-07T16:25:34Z

/approve

openshift-ci · 2023-02-07T16:25:42Z

[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:

~~pkg/syncer/OWNERS~~ [sttts]
~~test/OWNERS~~ [sttts]

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

jmprusi · 2023-02-07T16:26:39Z

/lgtm

test/e2e/framework/syncer.go

Signed-off-by: David Festal <[email protected]>

jmprusi · 2023-02-07T16:48:21Z

/lgtm

davidfestal requested a review from jmprusi February 1, 2023 18:34

openshift-ci bot requested a review from kylape February 1, 2023 18:34

sttts reviewed Feb 1, 2023

View reviewed changes

test/e2e/reconciler/locationworkspace/local_apiexport_test.go Outdated Show resolved Hide resolved

davidfestal force-pushed the split-SyncerFixture branch 2 times, most recently from a8933e9 to d597bdf Compare February 2, 2023 20:17

davidfestal changed the title ~~🌱 [WIP] tmc e2e : Split SyncerFixture~~ 🌱 tmc e2e : Split SyncerFixture Feb 2, 2023

davidfestal force-pushed the split-SyncerFixture branch from d597bdf to f7a8a44 Compare February 2, 2023 20:29

sttts reviewed Feb 3, 2023

View reviewed changes

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

sttts reviewed Feb 3, 2023

View reviewed changes

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

sttts reviewed Feb 3, 2023

View reviewed changes

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

sttts reviewed Feb 3, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

sttts reviewed Feb 3, 2023

View reviewed changes

test/e2e/framework/syncer.go Show resolved Hide resolved

sttts reviewed Feb 3, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

davidfestal force-pushed the split-SyncerFixture branch from f7a8a44 to 8a9580d Compare February 6, 2023 13:22

sttts reviewed Feb 6, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

sttts reviewed Feb 6, 2023

View reviewed changes

davidfestal force-pushed the split-SyncerFixture branch 4 times, most recently from e2d15f4 to ac998d9 Compare February 6, 2023 18:44

sttts reviewed Feb 7, 2023

View reviewed changes

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

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/reconciler/namespace/controller_test.go Outdated Show resolved Hide resolved

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/reconciler/scheduling/api_compatibility_test.go Outdated Show resolved Hide resolved

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/reconciler/scheduling/api_compatibility_test.go Outdated Show resolved Hide resolved

openshift-ci bot assigned jmprusi Feb 7, 2023

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

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

jmprusi mentioned this pull request Feb 7, 2023

🐛 workload/resource: handle upsynced resources #2533

Merged

davidfestal added 2 commits February 7, 2023 15:04

tmc e2e : Split SyncerFixture...

a1476bf

... to provide a way to only maintain heartbeat and import apis without effectively syncing. And change all the candidate tests to use the new function. Signed-off-by: David Festal <[email protected]>

Fixes after PR review comments

6dae242

Signed-off-by: David Festal <[email protected]>

e2e: Separate SyncerFixture.CreateAndStart()...

8f125fe

... into 2 functions: `Create()` (already exists) and `StartSyncer()`. Signed-off-by: David Festal <[email protected]>

davidfestal force-pushed the split-SyncerFixture branch from 13a4a06 to 8f125fe Compare February 7, 2023 15:02

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

Remove unnecessary private function

0da8dad

Signed-off-by: David Festal <[email protected]>

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

Fix review last comments

17a6ba8

Signed-off-by: David Festal <[email protected]>

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

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

sttts reviewed Feb 7, 2023

View reviewed changes

test/e2e/framework/syncer.go Outdated Show resolved Hide resolved

Rename as requested in review

77af933

Signed-off-by: David Festal <[email protected]>

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

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

openshift-merge-robot merged commit 74e06bb into kcp-dev:main Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 tmc e2e : Split SyncerFixture #2730

🌱 tmc e2e : Split SyncerFixture #2730

davidfestal commented Feb 1, 2023

sttts Feb 6, 2023

sttts Feb 6, 2023

sttts Feb 6, 2023

davidfestal Feb 6, 2023

davidfestal Feb 6, 2023

davidfestal Feb 6, 2023 •

edited

Loading

jmprusi Feb 7, 2023

davidfestal commented Feb 6, 2023

sttts Feb 7, 2023

sttts Feb 7, 2023

davidfestal Feb 7, 2023

davidfestal Feb 7, 2023

sttts Feb 7, 2023

davidfestal Feb 7, 2023

sttts Feb 7, 2023

davidfestal Feb 7, 2023

jmprusi commented Feb 7, 2023

jmprusi commented Feb 7, 2023

sttts commented Feb 7, 2023

openshift-ci bot commented Feb 7, 2023

jmprusi commented Feb 7, 2023

jmprusi commented Feb 7, 2023

🌱 tmc e2e : Split SyncerFixture #2730

🌱 tmc e2e : Split SyncerFixture #2730

Conversation

davidfestal commented Feb 1, 2023

Summary

Related issue(s)

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

davidfestal Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfestal commented Feb 6, 2023

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

jmprusi commented Feb 7, 2023

jmprusi commented Feb 7, 2023

sttts commented Feb 7, 2023

openshift-ci bot commented Feb 7, 2023

jmprusi commented Feb 7, 2023

jmprusi commented Feb 7, 2023

davidfestal Feb 6, 2023 •

edited

Loading