SANDBOX-838: loop update calls #1074

rsoaresd · 2024-11-28T13:47:29Z

Description

#1071 addressed a potential fix, but to ensure we do not face it in the future, we should do the update call in a loop.

Since other update calls are not being done in a loop, this PR addressed it as well.

Issue ticket number and link

SANDBOX-838

rsoaresd · 2024-11-28T15:46:28Z

/retest

hitting known flaky test SANDBOX-837

rsoaresd · 2024-11-28T18:27:34Z

/retest

hitting known flaky test SANDBOX-836

MatousJobanek

Thanks a lot for taking care of this.
I believe that we could lower the amount of copy-pasting by expanding the Waiter generic type

toolchain-e2e/testsupport/wait/awaitility.go

Lines 669 to 675 in f22abed

    
           // Waiter is a helper struct for `wait.For()` that provides functions to query the cluster waiting 
        
           // for the results. 
        
           type Waiter[T client.Object] struct { 
        
           	await *Awaitility 
        
           	t     *testing.T 
        
           	gvk   schema.GroupVersionKind 
        
           }

cc @metlos
Something like this could work:

func (w *Waiter[T]) Update(status bool, objectName string, modify func(T)) (T, error) {
	var objectToReturn T
	err := wait.PollUntilContextTimeout(context.TODO(), w.await.RetryInterval, w.await.Timeout, true, func(ctx context.Context) (done bool, err error) {
		obj := &unstructured.Unstructured{}
		obj.SetGroupVersionKind(w.gvk)
		if err := w.await.Client.Get(context.TODO(), types.NamespacedName{Namespace: w.await.Namespace, Name: objectName}, obj); err != nil {
			return true, err
		}
		object, err := w.cast(obj)
		if err != nil {
			return false, fmt.Errorf("failed to cast the object to GVK %v: %w", w.gvk, err)
		}
		modify(object)
		if status {
			// Update the Status
			if err := w.await.Client.Status().Update(context.TODO(), object); err != nil {
				w.t.Logf("error updating '%v' Status '%s': %s. Will retry again...", w.gvk, objectName, err.Error())
				return false, err
			}
		} else {
			// Update the Spec
			if err := w.await.Client.Update(context.TODO(), object); err != nil {
				w.t.Logf("Error updating '%v' Spec '%s': %s. Will retry again...", w.gvk, objectName, err.Error())
				return false, err
			}
		}
		objectToReturn = object
		return true, nil
	})
	return objectToReturn, err
}

so you could then call it:

_, err = For(t, hostAwait.Awaitility, &toolchainv1alpha1.NSTemplateTier{}).
	Update(false, tier.NSTemplateTier.Name, func(nstt *toolchainv1alpha1.NSTemplateTier) {
		nstt.Spec = tier.NSTemplateTier.Spec
	})

or:

event, err = For(t, hostAwait.Awaitility, &toolchainv1alpha1.SocialEvent{}).
	Update(false, event.Name, func(ev *toolchainv1alpha1.SocialEvent) {
		ev.Spec.SpaceTier = "base"
	})

You could also add other methods with some syntactic sugar:

func (w *Waiter[T]) Update(objectName string, modify func(T)) (T, error) {
	return w.doUpdate(false, objectName, modify)
}

func (w *Waiter[T]) UpdateStatus(objectName string, modify func(T)) (T, error) {
	return w.doUpdate(true, objectName, modify)
}

func (w *Waiter[T]) doUpdate(status bool, objectName string, modify func(T)) (T, error) {
	var objectToReturn T
...

MatousJobanek · 2024-11-29T09:23:16Z

test/e2e/parallel/proxy_test.go

 				t.Run("use proxy to update a HAS Application CR in the user appstudio namespace via proxy API", func(t *testing.T) {
-					// Update application
+					// Get application name
 					applicationName := user.getApplicationName(0)
-					// Get application
-					proxyApp := user.getApplication(t, proxyCl, applicationName)
 					// Update DisplayName
 					changedDisplayName := fmt.Sprintf("Proxy test for user %s - updated application", tenantNsName(user.compliantUsername))
-					proxyApp.Spec.DisplayName = changedDisplayName
-					err := proxyCl.Update(context.TODO(), proxyApp)
+					// Update application
+					proxyApp, err := awaitilities.Host().UpdateApplication(t, proxyCl, applicationName, tenantNsName(user.compliantUsername), func(app *appstudiov1.Application) {
+						app.Spec.DisplayName = changedDisplayName
+					})
 					require.NoError(t, err)


Not related to this PR, but I think that we can drop the "AppStudio API"-specific test cases. It's completely fine to test modification on standard k8s resources (eg. ConfigMap), no need to maintain the other API

Apart from that, there is actually no Application controller running that would watch/modify these resources. This means that we don't expect any conflicts for this resource and the update through the proxy should work fine without having the loop.

Thank you so much! I will remove the loop here. And take care of the "AppStudio API"-specific test cases in a different PR

test/e2e/parallel/socialevent_test.go

testsupport/spaceprovisionerconfig/spaceprovisionerconfig.go

testsupport/wait/host.go

rsoaresd · 2024-11-29T12:31:18Z

Thanks a lot for taking care of this. I believe that we could lower the amount of copy-pasting by expanding the Waiter generic type

toolchain-e2e/testsupport/wait/awaitility.go

Lines 669 to 675 in f22abed

    
           // Waiter is a helper struct for `wait.For()` that provides functions to query the cluster waiting 
        
           // for the results. 
        
           type Waiter[T client.Object] struct { 
        
           	await *Awaitility 
        
           	t     *testing.T 
        
           	gvk   schema.GroupVersionKind 
        
           }

cc @metlos
Something like this could work:

func (w *Waiter[T]) Update(status bool, objectName string, modify func(T)) (T, error) {
	var objectToReturn T
	err := wait.PollUntilContextTimeout(context.TODO(), w.await.RetryInterval, w.await.Timeout, true, func(ctx context.Context) (done bool, err error) {
		obj := &unstructured.Unstructured{}
		obj.SetGroupVersionKind(w.gvk)
		if err := w.await.Client.Get(context.TODO(), types.NamespacedName{Namespace: w.await.Namespace, Name: objectName}, obj); err != nil {
			return true, err
		}
		object, err := w.cast(obj)
		if err != nil {
			return false, fmt.Errorf("failed to cast the object to GVK %v: %w", w.gvk, err)
		}
		modify(object)
		if status {
			// Update the Status
			if err := w.await.Client.Status().Update(context.TODO(), object); err != nil {
				w.t.Logf("error updating '%v' Status '%s': %s. Will retry again...", w.gvk, objectName, err.Error())
				return false, err
			}
		} else {
			// Update the Spec
			if err := w.await.Client.Update(context.TODO(), object); err != nil {
				w.t.Logf("Error updating '%v' Spec '%s': %s. Will retry again...", w.gvk, objectName, err.Error())
				return false, err
			}
		}
		objectToReturn = object
		return true, nil
	})
	return objectToReturn, err
}

so you could then call it:

_, err = For(t, hostAwait.Awaitility, &toolchainv1alpha1.NSTemplateTier{}).
	Update(false, tier.NSTemplateTier.Name, func(nstt *toolchainv1alpha1.NSTemplateTier) {
		nstt.Spec = tier.NSTemplateTier.Spec
	})

or:

event, err = For(t, hostAwait.Awaitility, &toolchainv1alpha1.SocialEvent{}).
	Update(false, event.Name, func(ev *toolchainv1alpha1.SocialEvent) {
		ev.Spec.SpaceTier = "base"
	})

You could also add other methods with some syntactic sugar:

func (w *Waiter[T]) Update(objectName string, modify func(T)) (T, error) {
	return w.doUpdate(false, objectName, modify)
}

func (w *Waiter[T]) UpdateStatus(objectName string, modify func(T)) (T, error) {
	return w.doUpdate(true, objectName, modify)
}

func (w *Waiter[T]) doUpdate(status bool, objectName string, modify func(T)) (T, error) {
	var objectToReturn T
...

@MatousJobanek excellent idea!! 🚀 Should we move the Waiter generic type to toolchain-common or remain here?

MatousJobanek · 2024-11-29T16:24:25Z

as discussed in the KubeSaw sync call, it should stay here 👍

rsoaresd · 2024-12-02T12:29:48Z

as discussed in the KubeSaw sync call, it should stay here 👍

Perfect, thank you so much! I addressed for now only some cases to the pr to not be too heavy to review. I will clean up the other Update's functions in a separate PR once this one is merged

MatousJobanek

Nice, thanks for taking care of this, and thanks for implementing the generic approach 👍

testsupport/wait/awaitility.go

MatousJobanek · 2024-12-03T12:47:42Z

testsupport/wait/awaitility.go

+				return false, err
+			}
+		} else {
+			// Update the Spec
+			if err := w.await.Client.Update(context.TODO(), object); err != nil {
+				w.t.Logf("Error updating '%v' Spec '%s': %s. Will retry again...", w.gvk, objectName, err.Error())
+				return false, err


I just noticed that I made a mistake in my suggestion, we should not return an error here; otherwise, the poll loop immediately exists with an error:

Suggested change

return false, err

}

} else {

// Update the Spec

if err := w.await.Client.Update(context.TODO(), object); err != nil {

w.t.Logf("Error updating '%v' Spec '%s': %s. Will retry again...", w.gvk, objectName, err.Error())

return false, err

return false, nil

}

} else {

// Update the Spec

if err := w.await.Client.Update(context.TODO(), object); err != nil {

w.t.Logf("Error updating '%v' Spec '%s': %s. Will retry again...", w.gvk, objectName, err.Error())

return false, nil

is this true also for the status update faliure : https://github.com/codeready-toolchain/toolchain-e2e/pull/1074/files#diff-4abc90985c7fe1e595e0d6f5131e1f4757a6f00c9239f40ae09375d12f15e530R916

should we not return an error here as well and retry ?

Yes, it is addressed in line 916 of @MatousJobanek suggestion! Goinggggg!!

Locally, it passed for me. That's why I did not notice it. Sorry!

oh nice, sorry I've missed that from the suggestion, never mind 👍

sonarqubecloud · 2024-12-03T13:50:54Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
5.3% Duplication on New Code

See analysis details on SonarQube Cloud

mfrancisc

Great Job 👏
I like the new generic update functions 👍

I have only one minor comment/question.

test/e2e/parallel/space_test.go

openshift-ci · 2024-12-03T13:56:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MatousJobanek, mfrancisc, rsoaresd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [MatousJobanek,mfrancisc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rsoaresd · 2024-12-03T16:04:09Z

/retest

hitting known flaky test SANDBOX-837

loop update calls

519f88a

rsoaresd requested review from MatousJobanek, alexeykazakov, mfrancisc, rajivnathan, ranakan19 and xcoulon as code owners November 28, 2024 13:47

Merge branch 'master' into loop_update_calls

b8d7f77

MatousJobanek reviewed Nov 29, 2024

View reviewed changes

requested changes

88cfeb7

add generic update

b7cd532

Merge branch 'master' into loop_update_calls

99f41cb

rsoaresd requested a review from MatousJobanek December 2, 2024 12:30

MatousJobanek approved these changes Dec 3, 2024

View reviewed changes

testsupport/wait/awaitility.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved label Dec 3, 2024

MatousJobanek reviewed Dec 3, 2024

View reviewed changes

requested changes

8b4a141

mfrancisc approved these changes Dec 3, 2024

View reviewed changes

test/e2e/parallel/space_test.go Show resolved Hide resolved

rsoaresd merged commit 91404c7 into codeready-toolchain:master Dec 3, 2024
8 of 9 checks passed

	// Waiter is a helper struct for `wait.For()` that provides functions to query the cluster waiting
	// for the results.
	type Waiter[T client.Object] struct {
	await *Awaitility
	t *testing.T
	gvk schema.GroupVersionKind
	}

SANDBOX-838: loop update calls #1074

SANDBOX-838: loop update calls #1074

Uh oh!

Conversation

rsoaresd commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue ticket number and link

Uh oh!

rsoaresd commented Nov 28, 2024

Uh oh!

rsoaresd commented Nov 28, 2024

Uh oh!

MatousJobanek left a comment

Choose a reason for hiding this comment

Uh oh!

MatousJobanek Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

MatousJobanek Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

rsoaresd Nov 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rsoaresd commented Nov 29, 2024

Uh oh!

MatousJobanek commented Nov 29, 2024

Uh oh!

rsoaresd commented Dec 2, 2024

Uh oh!

MatousJobanek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MatousJobanek Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

mfrancisc Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

rsoaresd Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

mfrancisc Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 3, 2024

Quality Gate passed

Uh oh!

mfrancisc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci bot commented Dec 3, 2024

Uh oh!

rsoaresd commented Dec 3, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rsoaresd commented Nov 28, 2024 •

edited

Loading