🌱 TMC E2E tests sharding support - step 1 #2908
Conversation
test/e2e/framework/syncer.go
Outdated
| var syncTargetClusterName logicalcluster.Name | ||
| Eventually(t, func() (success bool, reason string) { | ||
| syncTarget, err := kcpClusterClient.Cluster(syncerConfig.SyncTargetPath).WorkloadV1alpha1().SyncTargets().Get(ctx, syncerConfig.SyncTargetName, metav1.GetOptions{}) | ||
| require.NoError(t, err) | ||
| if len(syncTarget.Status.VirtualWorkspaces) != 1 { | ||
| return false, "" | ||
| if len(syncTarget.Status.VirtualWorkspaces) != len(sf.upstreamServer.ShardNames()) { |
There was a problem hiding this comment.
IMHO it is not sufficient for downstream users to compare the lengths of these two slices to determine if the sync target is "ready" - the consumer should stream the set of virtual workspace URLs and handle them as they pop into and out of existence.
There was a problem hiding this comment.
Note that:
- set equality is really what we want, not length
- making partial progress when one URL is missing / slow to be posted is desired
There was a problem hiding this comment.
downstream users
what do you mean here ?
compare the lengths of these two slices to determine if the sync target is "ready"
Having the VW URLs set is not what makes the SyncTarget in general. In the Syncer, we watch these URLs and react to any change (when a new shard is added).
But in the context of e2e tests, when the number of shards is a characteristic of the test case (how the shared server is being deployed by the related fixture), we wait until the SyncTarget has the expected number of virtual workspaces).
set equality is really what we want, not length
equality with what ? chacking the context of the SyncTarget VW URLs, to ensure they match (though not being equal with) the VW URLS of the shards, would be the purpose of a dedicated test. I don't think it makes sense.
making partial progress when one URL is missing / slow to be posted is desired
I don't really understand the point here, in the e2e fixture.
There was a problem hiding this comment.
We should never write code that's end-to-end test specific. All that does is remove useful learning from real use-cases.
equality with what ?
Shards A, B, C -> virtual workspace URLs for those shards. Using the current logic we race: shards A B D, URLs A, B, C still is called "ready"
I don't really understand the point here, in the e2e fixture.
A static list of URLs is the wrong abstraction with which we should expose the URLs associated with this virtual view. If/when we start having dynamic shard lifecycle in the tests, this will lead to flakes.
There was a problem hiding this comment.
According to what we discussed, I changed the static URL lists in the SyncTarget fixture to be functions.
This we can easily follow the same pattern as what is done for APIExport and not depend anymore on the number of shards.
There was a problem hiding this comment.
Fixed in the rebased commit: 9403375
davidfestal
force-pushed
the
tmc-e2e-sharding-step-1
branch
from
639d927
to
a04e095
Compare
Signed-off-by: David Festal <dfestal@redhat.com>
openshift-merge-robot
added
the
needs-rebase
davidfestal
force-pushed
the
tmc-e2e-sharding-step-1
branch
from
a04e095
to
74d9b89
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/approve @stevekuznetsov had comments. Leaving lgtm to him. |
openshift-ci
bot
added
the
approved
Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
Signed-off-by: David Festal <dfestal@redhat.com>
davidfestal
force-pushed
the
tmc-e2e-sharding-step-1
branch
from
74d9b89
to
d14686a
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stevekuznetsov, 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:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
size/L
Summary
First step towars sharding support for TMC e2e tests
This PR is a prefactored PR coming from PR #2675
Related issue(s)
kcp-dev/contrib-tmc#45