Skip to content
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.

Sign up for GitHub

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(flaky): ensure that the space is always unblocked for its deletion #1090

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(flaky): ensure that the space is always unblocked for its deletion #1090

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dba11a5
fix(flaky): ensure that the space is always unblocked for its deletion
MatousJobanek Jan 8, 2025
de8dd00
comment
MatousJobanek Jan 8, 2025
b4aa021
Merge branch 'master' into fix-flaky-blocked-space-deletion
MatousJobanek Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions test/e2e/parallel/space_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package parallel
import (
"context"
"sort"
"sync"
"testing"

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
Expand All @@ -13,7 +14,6 @@ import (
"github.com/codeready-toolchain/toolchain-e2e/testsupport/tiers"
"github.com/codeready-toolchain/toolchain-e2e/testsupport/util"
"github.com/codeready-toolchain/toolchain-e2e/testsupport/wait"

"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
)
Expand Down Expand Up @@ -77,6 +77,20 @@ func TestCreateSpace(t *testing.T) {
// then
VerifyResourcesProvisionedForSpace(t, awaitilities, space.Name)

// ensure that the space is reset back to the original (valid) target cluster
// so the cleanup logic can delete the Space
var resetOnce sync.Once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... what's the point of using Once here? resetOnce.Do() will be called a single time only (for this specific resetOnce instance) anyway. What am I missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to ensure that this reset is called only once - either as part of the sub-test or at the end of the test. If it happens that the sub-test (that resets the target cluster) is called, then the space would be deleted - see:

toolchain-e2e/test/e2e/parallel/space_test.go

Lines 112 to 120 in de8dd00

t.Run("update target cluster to unblock deletion", func(t *testing.T) {
// when
reset()
// then
// space should be finally deleted
err = hostAwait.WaitUntilSpaceAndSpaceBindingsDeleted(t, s.Name)
require.NoError(t, err)
})

so the space wouldn't be present anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or were you were asking why do I define the variable resetOnce outside of the function? Well, the variable has to stay outside, so when the function is called, it always uses the same instance of the Once. If it was moved inside of the function, then it would create a new instance of Once for every call of the function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be a problem - but should we move this definition in the parent test TestCreateSpace ( where we define awaitilities , hostAwait, memberAwait ) ? Just to be on the safe side.

Copy link
Contributor

@alexeykazakov alexeykazakov Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was missing is that the reset() was called twice (not once!). Once in the defer and the second time in the sub-test bellow. I saw it only in called by defer and was wondering why do you need once if it's already called only once :)
Sorry for the confusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we move this definition in the parent test TestCreateSpace ( where we define awaitilities , hostAwait, memberAwait ) ? Just to be on the safe side.

not really, it has to stay in the same (sub)test where the space is provisioned, so it's tied to the actual space (and also the the function that executes this sub-test)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, it has to stay in the same (sub)test where the space is provisioned, so it's tied to the actual space

makes sense , thanks for explaining that 👍

reset := func() {
resetOnce.Do(func() {
_, err := wait.For(t, hostAwait.Awaitility, &toolchainv1alpha1.Space{}).
Update(space.Name, hostAwait.Namespace, func(s *toolchainv1alpha1.Space) {
s.Spec.TargetCluster = memberAwait.ClusterName
})
require.NoError(t, err)
})
}
defer reset()

t.Run("unable to delete space that was already provisioned", func(t *testing.T) {
// given
s, err := wait.For(t, hostAwait.Awaitility, &toolchainv1alpha1.Space{}).
Expand All @@ -97,11 +111,7 @@ func TestCreateSpace(t *testing.T) {

t.Run("update target cluster to unblock deletion", func(t *testing.T) {
// when
s, err = wait.For(t, hostAwait.Awaitility, &toolchainv1alpha1.Space{}).
Update(s.Name, hostAwait.Namespace, func(s *toolchainv1alpha1.Space) {
s.Spec.TargetCluster = memberAwait.ClusterName
})
require.NoError(t, err)
reset()

// then
// space should be finally deleted
Expand Down
Loading