SyncTarget Uniqueness #1600

jmprusi · 2022-07-22T11:18:18Z

This PR improves the uniqueness of the labels/annotations/finalizers that rely currently on the SyncTarget Name by replacing it with a hash generated from the SyncTarget Name + SyncTarget workspace.

Introduces a new helper in workloadv1alpha1: ToSyncTargetKey(syncTargetName, syncTargetWorkspace)
Replaces all usages of SyncTarget Name in labels/annotations/finalizers

openshift-ci · 2022-07-22T11:18:22Z

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

jmprusi · 2022-07-25T17:13:05Z

/test e2e

jmprusi · 2022-07-25T18:08:29Z

/retest

jmprusi · 2022-07-26T08:59:19Z

/test all

jmprusi · 2022-07-27T13:47:23Z

/retest

jmprusi · 2022-07-27T13:48:47Z

/test all

jmprusi · 2022-07-27T13:52:22Z

/retest

sttts · 2022-07-29T20:04:12Z

pkg/virtual/syncer/controllers/apireconciler/syncer_apireconciler_controller.go

@@ -48,7 +50,7 @@ const (
 	byWorkspace    = ControllerName + "-byWorkspace" // will go away with scoping
 )

-type CreateAPIDefinitionFunc func(syncTargetName string, apiResourceSchema *apisv1alpha1.APIResourceSchema, version string, identityHash string) (apidefinition.APIDefinition, error)
+type CreateAPIDefinitionFunc func(syncTargetName string, syncTargetWorkspace logicalcluster.Name, apiResourceSchema *apisv1alpha1.APIResourceSchema, version string, identityHash string) (apidefinition.APIDefinition, error)


switch order, cluster first

sttts · 2022-07-29T20:06:58Z

test/e2e/framework/fixture.go

@@ -599,7 +599,7 @@ func (sf SyncerFixture) Start(t *testing.T) *StartedSyncerFixture {
 					// Not a kcp-synced namespace
 					continue
 				}
-				if locator.Workspace.String() != syncerConfig.SyncTargetWorkspace.String() {
+				if locator.SyncTarget.Workspace == syncerConfig.SyncTargetWorkspace.String() {


why does this only compare the workspace, not the name?

and why != => == ?

https://gist.github.com/a57ad8c7498773cd573d15696e739e1e

sttts · 2022-07-29T20:26:15Z

test/e2e/reconciler/cluster/controller_test.go

+					metav1.GetOptions{},
+				)
+				require.NoError(t, err)
+				syncTargetKey := workloadv1alpha1.ToSyncTargetKey(syncTarget.GetName(), logicalcluster.From(syncTarget))


why this change? Just take syncerFixture.SyncerConfig.SyncTargetName and syncerFixture.SyncerConfig.SyncTargetWorkspace ?

I get the syncTarget for the UID (line 91) and then reuse the name/workspace here, but I can use the ones from fixture.

sttts · 2022-07-29T20:28:36Z

test/e2e/reconciler/scheduling/placement_scheduler_test.go

@@ -157,6 +157,7 @@ func TestPlacementUpdate(t *testing.T) {
 		},
 	}, metav1.CreateOptions{})
 	require.NoError(t, err)
+	syncTargetKey := workloadv1alpha1.ToSyncTargetKey(firstSyncTargetName, syncerFixture.SyncerConfig.SyncTargetWorkspace)


if there is a second synctarget around, call this firstSyncTargetKey

sttts · 2022-07-29T20:30:13Z

test/e2e/syncer/syncer_test.go

 		_, err := downstreamKubeClient.AppsV1().Deployments(downstreamNamespaceName).Get(ctx, upstreamDeployment.Name, metav1.GetOptions{})
 		if apierrors.IsNotFound(err) {
 			return true
 		}
 		require.NoError(t, err)
 		return false
-	}, 5*time.Second, time.Second, "downstream Deployment got deleted or there was an error", downstreamNamespaceName, upstreamDeployment.Name)
+	}, 5*time.Second, time.Second, "downstream Deployment %s/%s got deleted or there was an error", downstreamNamespaceName, upstreamDeployment.Name)


Suggested change

}, 5*time.Second, time.Second, "downstream Deployment %s/%s got deleted or there was an error", downstreamNamespaceName, upstreamDeployment.Name)

}, 5*time.Second, time.Second, "downstream Deployment %s|%s got deleted or there was an error", downstreamNamespaceName, upstreamDeployment.Name)

jmprusi · 2022-08-01T19:05:47Z

/retest

sttts · 2022-08-01T21:01:03Z

Squashed.

/lgtm
/approve

openshift-ci · 2022-08-01T21:01:24Z

[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]
~~pkg/apis/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 · 2022-08-01T21:01:40Z

/lgtm

jmprusi · 2022-08-01T22:31:47Z

/retest

jmprusi · 2022-08-01T22:35:51Z

/test e2e-multiple-runs

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 Jul 22, 2022

openshift-ci bot requested review from davidfestal and stevekuznetsov July 22, 2022 11:18

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch 3 times, most recently from 75c54f7 to 70acfbc Compare July 25, 2022 16:58

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch from 70acfbc to c9aeb5c Compare July 26, 2022 08:50

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch 2 times, most recently from a49af13 to ab779a3 Compare July 27, 2022 10:21

kcp-dev deleted a comment from openshift-ci bot Jul 27, 2022

jmprusi changed the title ~~SyncTarget Uniqueness~~ [WIP] SyncTarget Uniqueness Jul 27, 2022

jmprusi marked this pull request as ready for review July 27, 2022 13:51

openshift-ci bot requested review from ncdc and sttts July 27, 2022 13:52

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch 2 times, most recently from a9173b5 to f58643d Compare July 28, 2022 19:59

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

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch from f58643d to 54be0f4 Compare July 29, 2022 10:15

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

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 29, 2022

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch 2 times, most recently from 3d3d72a to a6150fe Compare July 29, 2022 11:01

sttts reviewed Jul 29, 2022

View reviewed changes

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch 2 times, most recently from 283e706 to 3c7c6ef Compare August 1, 2022 15:31

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

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch from 3c7c6ef to 39dafd5 Compare August 1, 2022 15:41

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

jmprusi force-pushed the jmprusi/uniqueness-synctarget branch 2 times, most recently from 2435b2f to 1f302cd Compare August 1, 2022 17:43

sttts force-pushed the jmprusi/uniqueness-synctarget branch from 1f302cd to 02f26dc Compare August 1, 2022 21:00

openshift-ci bot assigned sttts Aug 1, 2022

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2022

jmprusi added 3 commits August 1, 2022 23:01

workload/v1alpha1: ToSyncTarget helper func

402ea75

Wire up SyncTargetKey

bde54f8

Update go.mod

fc38492

sttts force-pushed the jmprusi/uniqueness-synctarget branch from 02f26dc to fc38492 Compare August 1, 2022 21:01

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2022

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

openshift-merge-robot merged commit 9802ecf into kcp-dev:main Aug 2, 2022

ncdc mentioned this pull request Aug 2, 2022

💣 – workload labels+finalizers are not fully qualified #1281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SyncTarget Uniqueness #1600

SyncTarget Uniqueness #1600

jmprusi commented Jul 22, 2022 •

edited

Loading

openshift-ci bot commented Jul 22, 2022

jmprusi commented Jul 25, 2022

jmprusi commented Jul 25, 2022

jmprusi commented Jul 26, 2022

jmprusi commented Jul 27, 2022

jmprusi commented Jul 27, 2022

jmprusi commented Jul 27, 2022

sttts Jul 29, 2022

sttts Jul 29, 2022

sttts Jul 29, 2022

sttts Jul 29, 2022

sttts Jul 29, 2022

jmprusi Aug 1, 2022

sttts Jul 29, 2022

jmprusi Aug 1, 2022

sttts Jul 29, 2022

jmprusi commented Aug 1, 2022

sttts commented Aug 1, 2022

openshift-ci bot commented Aug 1, 2022

sttts commented Aug 1, 2022

jmprusi commented Aug 1, 2022

jmprusi commented Aug 1, 2022

	}, 5*time.Second, time.Second, "downstream Deployment %s/%s got deleted or there was an error", downstreamNamespaceName, upstreamDeployment.Name)
	}, 5*time.Second, time.Second, "downstream Deployment %s\|%s got deleted or there was an error", downstreamNamespaceName, upstreamDeployment.Name)

SyncTarget Uniqueness #1600

SyncTarget Uniqueness #1600

Conversation

jmprusi commented Jul 22, 2022 • edited Loading

openshift-ci bot commented Jul 22, 2022

jmprusi commented Jul 25, 2022

jmprusi commented Jul 25, 2022

jmprusi commented Jul 26, 2022

jmprusi commented Jul 27, 2022

jmprusi commented Jul 27, 2022

jmprusi commented Jul 27, 2022

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

jmprusi commented Aug 1, 2022

sttts commented Aug 1, 2022

openshift-ci bot commented Aug 1, 2022

sttts commented Aug 1, 2022

jmprusi commented Aug 1, 2022

jmprusi commented Aug 1, 2022

jmprusi commented Jul 22, 2022 •

edited

Loading