🌱 PartitionSet e2e #2642

fgiloux · 2023-01-18T10:32:13Z

Summary

This PR adds end-to-end tests for PartitionSet reconciliation.

Related issue(s)

Contributes to #2334

fgiloux · 2023-01-18T10:33:00Z

/hold
It will need to be rebased after #2513 has merged

fgiloux · 2023-02-09T06:12:51Z

/retest

p0lyn0mial · 2023-02-10T08:19:41Z

pkg/reconciler/topology/partitionset/resources.go

@@ -35,6 +36,7 @@ func generatePartition(name string, matchExpressions []metav1.LabelSelectorRequi
 	for _, label := range labels {
 		pname = pname + "-" + strings.ToLower(matchLabels[label])
 	}
+	pname = pname[:min(validation.DNS1123SubdomainMaxLength-1, len(pname))]


so, there is a limit :)

please add a unit test

I don't think it is useful but I have added it.

What about collisions? This is why we use the hash in many other places.

GenerateName is used to avoid collisions. I have been requested to have the name carrying some way of identifying a partition. The information was previously set to labels.

p0lyn0mial · 2023-02-10T08:22:36Z

pkg/reconciler/topology/partitionset/partitionset_reconcile.go

@@ -89,10 +89,11 @@ func (c *controller) reconcile(ctx context.Context, partitionSet *topologyv1alph
 	}

 	var matchLabelsMap map[string]map[string]string
+	dimensions := removeDuplicates(partitionSet.Spec.Dimensions)


should we put it into validation/cel ?
that way we wouldn't get any duplicates, right ?

@sttts wdyt?

There is no easy way of doing that with CEL. CEL is cautious with costly operations like list iteration, although it would be o(n) here.
The current implementation is tolerant to duplicates and still provide a predictable, not surprising for the user, outcome.

p0lyn0mial · 2023-02-10T08:34:32Z

test/e2e/reconciler/partition/partitionset_test.go

+limitations under the License.
+*/
+
+package partition


should we rename test/e2e/reconciler/partition/partitionset_test.go to test/e2e/reconciler/partitionset/partitionset_test.go ?

I can do that

p0lyn0mial · 2023-02-10T08:37:07Z

test/e2e/reconciler/partition/partitionset_test.go

+		}
+		return false, spew.Sdump(partitionSet.Status.Conditions)
+	}, wait.ForeverTestTimeout, 100*time.Millisecond, "expected valid partitionSet")
+


should we check that no Partition was created ?

IMHO we should focus a bit more on the happy cases in e2e, exhaustive negative checking can be fragile as we've learned and requires private fixtures...

p0lyn0mial · 2023-02-10T08:43:25Z

test/e2e/reconciler/partition/partitionset_test.go

+		partitions, err = partitionClient.Cluster(partitionClusterPath).List(ctx, metav1.ListOptions{})
+		require.NoError(t, err, "error retrieving partitions")
+		if len(partitions.Items) == 1 {
+			return true, ""


should we examine Partition.Spec.Selector ?

p0lyn0mial · 2023-02-10T08:47:16Z

test/e2e/reconciler/partition/partitionset_test.go

+		partitions, err = partitionClient.Cluster(partitionClusterPath).List(ctx, metav1.ListOptions{})
+		require.NoError(t, err, "error retrieving partitions")
+		if len(partitions.Items) == 2 {
+			return true, ""


should we check Partition.Spec.Selector ?

We need to validate that we get the right ones ehre

p0lyn0mial · 2023-02-10T08:52:09Z

test/e2e/reconciler/partition/partitionset_test.go

+		return false, fmt.Sprintf("expected 1 partition, but got %d", len(partitions.Items))
+	}, wait.ForeverTestTimeout, 100*time.Millisecond, "expected 1 partition")
+
+	// The following tests are focused on the admission


I'd move it to a new separate test that simply checks admission

I thought of that but did not do it due to the cost of running a private cluster.

p0lyn0mial · 2023-02-10T08:53:06Z

test/e2e/reconciler/partition/partitionset_test.go

+	}
+	_, err = partitionSetClient.Cluster(partitionClusterPath).Create(ctx, errorPartitionSet, metav1.CreateOptions{})
+	require.Error(t, err, "error creating partitionSet expected")
+


should we check that a PartitionSet wasn't created ?

no, this is standard CEL validation mechanism. If the expression is evaluated to false an error is returned and the resource is not created.

p0lyn0mial · 2023-02-10T08:55:49Z

test/e2e/reconciler/partition/partitionset_test.go

+	_, err = partitionSetClient.Cluster(partitionClusterPath).Create(ctx, errorPartitionSet, metav1.CreateOptions{})
+	require.Error(t, err, "error creating partitionSet expected")
+
+	t.Logf("Character not allowed at first place in matchExpressions values")


Not sure if we need to check all incorrect values, i'd rely on k8s's validation.
From our perspective the most important thing is to make sure an invalid request will be rejected and won't have any side effect (i.e. deleting a partition).

There is no kubernetes validation taking place for the admission. This is handled by the CEL expressions I crafted.

p0lyn0mial · 2023-02-10T09:01:40Z

test/e2e/reconciler/partition/partitionset_test.go

+	_, err = partitionSetClient.Cluster(partitionClusterPath).Create(ctx, errorPartitionSet, metav1.CreateOptions{})
+	require.Error(t, err, "error creating partitionSet expected")
+
+	t.Logf("Partition name cut when the label values sum up")


I'm okay with checking it on a unit test level.

You can't do that apart from copying the CEL expressions directly in the unit test and like, which does not bring value in my opinion.

stevekuznetsov

IMHO we should write these tests on the shared fixture. Generate your labels and shard names with random suffixes to allow these tests to co-exist even with other instances of the same test in parallel, and mark the shards you're creating as not ready for scehduling so as to not impact any other part of the system.

stevekuznetsov · 2023-02-10T13:36:00Z

pkg/reconciler/topology/partitionset/partitionset_reconcile.go

@@ -215,3 +216,16 @@ func partition(shards []*corev1alpha1.Shard, dimensions []string, shardSelectorL
 	}
 	return matchLabelsMap
 }
+
+// removeDuplicates makes sure that a value is not found more than once in the slice.
+func removeDuplicates(slice []string) []string {


sets.NewString(slice).List()

stevekuznetsov · 2023-02-10T13:37:06Z

pkg/reconciler/topology/partitionset/resources.go

@@ -35,6 +36,7 @@ func generatePartition(name string, matchExpressions []metav1.LabelSelectorRequi
 	for _, label := range labels {
 		pname = pname + "-" + strings.ToLower(matchLabels[label])
 	}
+	pname = pname[:min(validation.DNS1123SubdomainMaxLength-1, len(pname))]


What about collisions? This is why we use the hash in many other places.

stevekuznetsov · 2023-02-10T13:37:24Z

pkg/reconciler/topology/partitionset/resources.go

@@ -48,3 +50,10 @@ func generatePartition(name string, matchExpressions []metav1.LabelSelectorRequi
 		},
 	}
 }
+
+func min(a, b int) int {


IMHO we don't need to accumulate this sort of logic. Just inline it.

inlining is done by the compiler. I find this way more readable and it seems that it is not an uncommon practice.

stevekuznetsov · 2023-02-10T13:37:46Z

test/e2e/reconciler/partition/partitionset_test.go

+	t.Parallel()
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	server := framework.PrivateKcpServer(t)


stevekuznetsov · 2023-02-10T13:38:01Z

test/e2e/reconciler/partition/partitionset_test.go

+	defer cancel()
+	server := framework.PrivateKcpServer(t)
+
+	// Create organization and workspace


Do we need an org anymore? Why?

we don't. It does not hurt either

stevekuznetsov · 2023-02-10T13:40:18Z

test/e2e/reconciler/partition/partitionset_test.go

+	shardClient := kcpClusterClient.CoreV1alpha1().Shards()
+	_, err = shardClient.Cluster(core.RootCluster.Path()).Create(ctx, shard1a, metav1.CreateOptions{})
+	require.NoError(t, err, "error creating shard")
+	framework.Eventually(t, func() (bool, string) {


framework.EventuallyCondition

did not exist when this test was written, does not allow to check values of multiple conditions.

stevekuznetsov · 2023-02-10T13:40:45Z

test/e2e/reconciler/partition/partitionset_test.go

+		return false, fmt.Sprintf("expected 1 partition, but got %d", partitionSet.Status.Count)
+	}, wait.ForeverTestTimeout, 100*time.Millisecond, "expected the partition count to be 1")
+	partitionClient := kcpClusterClient.TopologyV1alpha1().Partitions()
+	var partitions *topologyv1alpha1.PartitionList


Please continue to t.Logf() the steps

test/e2e/reconciler/partition/partitionset_test.go

stevekuznetsov · 2023-02-10T13:41:53Z

test/e2e/reconciler/partition/partitionset_test.go

+	}
+	shard2, err = shardClient.Cluster(core.RootCluster.Path()).Create(ctx, shard2, metav1.CreateOptions{})
+	require.NoError(t, err, "error creating shard")
+	framework.Eventually(t, func() (bool, string) {


eventuallycondition

stevekuznetsov · 2023-02-10T13:42:11Z

test/e2e/reconciler/partition/partitionset_test.go

+		partitions, err = partitionClient.Cluster(partitionClusterPath).List(ctx, metav1.ListOptions{})
+		require.NoError(t, err, "error retrieving partitions")
+		if len(partitions.Items) == 2 {
+			return true, ""


We need to validate that we get the right ones ehre

fgiloux · 2023-02-10T16:08:24Z

IMHO we should write these tests on the shared fixture. Generate your labels and shard names with random suffixes to allow these tests to co-exist even with other instances of the same test in parallel, and mark the shards you're creating as not ready for scehduling so as to not impact any other part of the system.

I had proposed something similar in a past PR. I see you have relaunched a discussion in slack on that. The change in the approach is out of scope of this PR.

stevekuznetsov · 2023-02-10T17:27:07Z

Sorry, I don't think we need to check in more tests using private fixtures. We're already at critical mass with e2e flakiness and adding much more load there is not reasonable. How hard is it to implement some "not-ready-for-scheduling" field on the shard? We're in alpha versions, can iterate.

stevekuznetsov · 2023-02-10T17:27:12Z

/hold

fgiloux · 2023-02-10T17:36:25Z

Sorry, I don't think we need to check in more tests using private fixtures. We're already at critical mass with e2e flakiness and adding much more load there is not reasonable. How hard is it to implement some "not-ready-for-scheduling" field on the shard? We're in alpha versions, can iterate.

Great! Give me a shout when you agree on something.

stevekuznetsov · 2023-02-10T18:38:26Z

Sounds like a field shard.spec.schedulable={true,false} would suffice, and we'd just need to update the workspace reconciler to choose only the schedulable subset when workspace.spec.location is unset.

p0lyn0mial · 2023-02-20T11:01:34Z

FYI #2782 should unblock this PR.

Signed-off-by: Frederic Giloux <[email protected]>

fgiloux · 2023-02-23T09:57:36Z

FYI #2782 should unblock this PR.

This does not seem to be sufficient:

    apibinding_test.go:348: === Testing identity wildcards
    apibinding_test.go:379: Verify "root:e2e-workspace-wqp4k:consumer-1-bound-against-1" bound to service provider 1 ("root:e2e-workspace-wqp4k:service-provider-1") wildcard list works
    apibinding_test.go:351: Get APIBinding for workspace root:e2e-workspace-wqp4k:consumer-1-bound-against-1
    apibinding_test.go:360: Doing a wildcard identity list for wildwest.dev/v1alpha1, Resource=cowboys:6440e16b668082bc273a03875cca56af91f1855ab12089d0a1d79dfdd1e8019b against root:e2e-workspace-wqp4k:consumer-1-bound-against-1 workspace on shard partition-shard-1a
    apibinding_test.go:361: kubeconfig for shard "partition-shard-1a" not found

pushing a commit to fix it. I am not sure that there are no other issues.

… by other e2e test Signed-off-by: Frederic Giloux <[email protected]>

Signed-off-by: Frederic Giloux <[email protected]>

fgiloux · 2023-02-23T16:23:34Z

/unhold

fgiloux · 2023-02-23T16:27:18Z

@p0lyn0mial @stevekuznetsov I have refactored the tests to use a shared server. I have also added a commit to patch some sensitivity to non-schedulable shards within the APIBinding tests.
Can you please have a look? I am hoping we can get this PR merged now.

sttts · 2023-03-01T12:23:34Z

/lgtm
/approve

openshift-ci · 2023-03-01T12:23:45Z

[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

openshift-ci bot requested review from qiujian16 and shawn-hurley January 18, 2023 10:32

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

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

fgiloux force-pushed the partitionset-e2e branch from 55d536b to ef9b368 Compare January 18, 2023 18:06

fgiloux force-pushed the partitionset-e2e branch 2 times, most recently from c587d0c to 4d12faa Compare February 8, 2023 21:44

fgiloux requested a review from p0lyn0mial February 9, 2023 06:32

p0lyn0mial reviewed Feb 10, 2023

View reviewed changes

stevekuznetsov reviewed Feb 10, 2023

View reviewed changes

fgiloux force-pushed the partitionset-e2e branch from 4d12faa to b2c6fee Compare February 10, 2023 16:08

fgiloux force-pushed the partitionset-e2e branch from b2c6fee to 194c1b7 Compare February 10, 2023 17:32

fgiloux added 2 commits February 23, 2023 07:05

Fix issue when GenerateName size limit is reached

2a68f30

Signed-off-by: Frederic Giloux <[email protected]>

Fix issue when duplicate dimensions are provided

ea2a0ac

Signed-off-by: Frederic Giloux <[email protected]>

fgiloux force-pushed the partitionset-e2e branch 3 times, most recently from 57f2bbc to 41c2d83 Compare February 23, 2023 09:49

fgiloux force-pushed the partitionset-e2e branch from 41c2d83 to 0e338c8 Compare February 23, 2023 10:42

Prevent APIBinding test to fail when non schedulable shards are added…

46c00df

… by other e2e test Signed-off-by: Frederic Giloux <[email protected]>

fgiloux force-pushed the partitionset-e2e branch 6 times, most recently from c5ad38b to da8db7a Compare February 23, 2023 14:14

Add end-to-end tests for PartitionSets

d6bdad4

Signed-off-by: Frederic Giloux <[email protected]>

fgiloux force-pushed the partitionset-e2e branch from da8db7a to d6bdad4 Compare February 23, 2023 15:35

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2023

openshift-ci bot assigned sttts Mar 1, 2023

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

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

openshift-merge-robot merged commit 4cb7736 into kcp-dev:main Mar 1, 2023

🌱 PartitionSet e2e #2642

🌱 PartitionSet e2e #2642

Conversation

fgiloux commented Jan 18, 2023

Summary

Related issue(s)

fgiloux commented Jan 18, 2023

fgiloux commented Feb 9, 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

fgiloux Feb 10, 2023 • 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

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

stevekuznetsov left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgiloux commented Feb 10, 2023

stevekuznetsov commented Feb 10, 2023

stevekuznetsov commented Feb 10, 2023

fgiloux commented Feb 10, 2023

stevekuznetsov commented Feb 10, 2023

p0lyn0mial commented Feb 20, 2023

fgiloux commented Feb 23, 2023 • edited Loading

fgiloux commented Feb 23, 2023

fgiloux commented Feb 23, 2023

sttts commented Mar 1, 2023

openshift-ci bot commented Mar 1, 2023

fgiloux Feb 10, 2023 •

edited

Loading

fgiloux commented Feb 23, 2023 •

edited

Loading