-
Notifications
You must be signed in to change notification settings - Fork 427
fix TestValidatingAdmissionPolicyCrossWorkspaceAPIBinding flaky test #3754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix TestValidatingAdmissionPolicyCrossWorkspaceAPIBinding flaky test #3754
Conversation
Signed-off-by: olalekan odukoya <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
| t.Logf("Creating default namespace in target workspace") | ||
| defaultNS := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "default", | ||
| }, | ||
| } | ||
| _, err = kubeClusterClient.Cluster(targetPath).CoreV1().Namespaces().Create(ctx, defaultNS, metav1.CreateOptions{}) | ||
| if err != nil && !errors.IsAlreadyExists(err) { | ||
| require.NoError(t, err, "failed to create default namespace") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the change below is more a bandaid instead of a fix. It'd be better to address the root cause of the missing namespace.
Looking at this code this has been a problem in the past:
kcp/test/e2e/reconciler/workspacedeletion/controller_test.go
Lines 103 to 122 in c885efc
| t.Logf("Wait until the %q workspace is ready", workspace.Name) | |
| kcptestinghelpers.Eventually(t, func() (bool, string) { | |
| workspace, err := server.kcpClusterClient.Cluster(orgPath).TenancyV1alpha1().Workspaces().Get(ctx, workspace.Name, metav1.GetOptions{}) | |
| require.NoError(t, err, "failed to get workspace") | |
| if actual, expected := workspace.Status.Phase, corev1alpha1.LogicalClusterPhaseReady; actual != expected { | |
| return false, fmt.Sprintf("workspace phase is %s, not %s", actual, expected) | |
| } | |
| return workspace.Status.Phase == corev1alpha1.LogicalClusterPhaseReady, fmt.Sprintf("workspace phase is %s", workspace.Status.Phase) | |
| }, wait.ForeverTestTimeout, time.Millisecond*100, "failed to wait for workspace %s to become ready", orgPath.Join(workspace.Name)) | |
| workspaceCluster := orgPath.Join(workspace.Name) | |
| t.Logf("Wait for default namespace to be created") | |
| kcptestinghelpers.Eventually(t, func() (bool, string) { | |
| _, err := server.kubeClusterClient.Cluster(workspaceCluster).CoreV1().Namespaces().Get(ctx, "default", metav1.GetOptions{}) | |
| if err != nil { | |
| return false, err.Error() | |
| } | |
| return true, "" | |
| }, wait.ForeverTestTimeout, 100*time.Millisecond, "default namespace was never created") |
Instead of waiting on the namespace it would be preferable to inspect why the workspace phase is set to ready before the default namespace is present.
| _, err = kcpClusterClient.Cluster(sourcePath).ApisV1alpha2().APIExports().Create(ctx, cowboysAPIExport, metav1.CreateOptions{}) | ||
| require.NoError(t, err) | ||
|
|
||
| t.Logf("Creating default namespace in target workspace") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yap, this is not the fix. + it will try to create namespace where workspace bootstrap process does the same. THis might cause other isues
|
/test all |
|
/retest |
|
my attempt to fix this #3777 |
|
Fixed by: #3777 |
Summary
What Type of PR Is This?
/kind bug
Related Issue(s)
Fixes 3753
Release Notes