AV-218868: Fix common failing unit tests #1541

ashishnayak96 · 2024-09-19T08:43:05Z

No description provided.

Dhivyaaj · 2024-09-19T08:43:15Z

No JIRA Ids found for the PR. Jira id is mandatory to update fix version in jira. Please update respective Jira id in PR title or commit message if the PR is intented for default branches of repo. For mandatory exemptions, comment trigger phrase 'skip jira-id-check' in PR. For manual trigger, comment trigger phrase 'run jira-id-check' in PR.

ashishnayak96 · 2024-09-19T08:48:03Z

build ako

ashishnayak96 · 2024-09-20T06:56:38Z

build ako

ashishnayak96 · 2024-09-23T06:08:20Z

build ako

ashishnayak96 · 2024-09-23T06:12:17Z

Common tests failing fixed till now:

TestFQDNsCountForAviInfraSettingWithLargeShardSize
TestCreateHostRuleBeforeIngress
TestCreateUpdateDeleteL7RuleInHostRule

ashishnayak96 · 2024-09-23T06:18:28Z

build ako

pkoshtavmware · 2024-09-23T06:29:46Z

build ako

akshayhavile · 2024-09-23T06:32:03Z

internal/status/crd_status.go

@@ -51,7 +54,7 @@ func UpdateHostRuleStatus(key string, hr *akov1beta1.HostRule, updateStatus Upda
 		"status": akov1beta1.HostRuleStatus(updateStatus),
 	})

-	_, err := lib.AKOControlConfig().V1beta1CRDClientset().AkoV1beta1().HostRules(hr.Namespace).Patch(context.TODO(), hr.Name, types.MergePatchType, patchPayload, metav1.PatchOptions{}, "status")
+	hrFromK8sClient, err := lib.AKOControlConfig().V1beta1CRDClientset().AkoV1beta1().HostRules(hr.Namespace).Patch(context.TODO(), hr.Name, types.MergePatchType, patchPayload, metav1.PatchOptions{}, "status")


initial comment: why are we retrying here as we are fetching from directly clientset so we should have updated content?

We are doing a patch through clientset which returns the updated result but we are not sure if the crd informer is also updated with the changes. So here, we are waiting for the informer to be updated with the patch

ashishnayak96 · 2024-09-24T09:13:20Z

build ako

aaha97 · 2024-09-24T10:09:42Z

build ako

jeyapradeen-avi · 2024-09-24T16:14:27Z

build ako

jeyapradeen-avi · 2024-09-24T16:24:01Z

build ako

jeyapradeen-avi · 2024-09-24T16:32:54Z

build ako

jeyapradeen-avi · 2024-09-24T16:45:07Z

build ako

jeyapradeen-avi · 2024-09-24T16:48:53Z

build ako

jeyapradeen-avi · 2024-09-24T17:09:48Z

build ako

jeyapradeen-avi · 2024-09-24T17:12:59Z

build ako

Dhivyaaj · 2024-09-25T09:50:55Z

No JIRA Ids found for the PR. Jira id is mandatory to update fix version in jira. Please update respective Jira id in PR title or commit message if the PR is intented for default branches of repo. For mandatory exemptions, comment trigger phrase 'skip jira-id-check' in PR. For manual trigger, comment trigger phrase 'run jira-id-check' in PR.

ashishnayak96 · 2024-09-25T09:51:20Z

build ako

ashishnayak96 · 2024-09-26T07:01:55Z

build ako

aaha97 · 2024-09-26T11:37:36Z

Makefile

-
-.PHONY: multi_tenancy
-multi_tenancy:
-	make -j 1 multitenancytests


why are we removing this target?

This is redundant to multitenancytests target

aaha97 · 2024-09-26T11:39:22Z

tests/integrationtest/lib.go

+	g := gomega.NewGomegaWithT(t)
+	g.Eventually(func() error {
+		ingClass, err := utils.GetInformers().IngressClassInformer.Lister().Get(ingclassName)
+		utils.AviLog.Infof("got ingclass %v", ingClass)


this log should be removed

ashishnayak96 · 2024-09-27T10:18:40Z

build ako

akshayhavile · 2024-09-30T06:29:06Z

internal/status/crd_status.go

@@ -64,6 +67,25 @@ func UpdateHostRuleStatus(key string, hr *akov1beta1.HostRule, updateStatus Upda
 		}
 		UpdateHostRuleStatus(key, updatedHr, updateStatus, retry+1)
 	}
+	// wait for crdinformer to be updated
+	constantBackoff := backoff.WithMaxRetries(backoff.NewConstantBackOff(500*time.Millisecond), 5)


This is being used in main processing path. It will impact each and every status update. Can it be optimized or moved to some other place?

It will only be impacted if it fails for the first time. We will wait for 500ms only when it is not updated. Best case is 0 ms. Worst case is 2.5s

Is this being done to make sure that informer cache is populated before AKO uses hostrule in graph layer, did we see any issue related to this and if that's the case why aren't we doing the same for other CRs?

as discussed, we will take for other CRs separately

akshayhavile · 2024-10-11T06:53:02Z

build ako

akshayhavile

LGTM. Need to re-run multiple times to verify its working fine.

ashishnayak96 · 2024-10-14T07:21:58Z

build ako

ashishnayak96 · 2024-10-14T07:25:22Z

build ako

ashishnayak96 · 2024-10-14T07:31:50Z

build ako

abhishekbsingh · 2024-10-14T09:33:00Z

internal/status/crd_status.go

+	err = backoff.Retry(operation, constantBackoff)
+	if err != nil {
+		utils.AviLog.Errorf("key: %s, msg: Hostrule %s/%s lister cache not updated with patch. error: [%s]", key, hr.Namespace, hr.Name, err.Error())
+	}



add a return or the success message gets printed as well

informer cache is internal for us, the kubernetes object has already been patched so successful message is also necessary

abhishekbsingh · 2024-10-14T09:33:44Z

tests/dedicatedvstests/l7_dedicated_crd_test.go

@@ -485,7 +485,7 @@ func TestFQDNsCountForAviInfraSettingWithDedicatedShardSize(t *testing.T) {
 	g.Eventually(func() bool {
 		_, found := mcache.VsCacheMeta.AviCacheGet(vsKey)
 		return found
-	}, 50*time.Second).Should(gomega.Equal(false))
+	}, 50*time.Second, 5*time.Second).Should(gomega.Equal(false))


why are we increasing the polling period from 10 ms to 5 seconds?

to decrease the load on cpu

ashishnayak96 · 2024-10-18T04:55:14Z

build ako

ashishnayak96 added 2 commits September 18, 2024 12:06

Fix Unit test

e6dea7d

Fix TestCreateHostRuleBeforeIngress

084ee65

vmwclabot added the cla-not-required label Sep 19, 2024

ashishnayak96 requested review from aaha97, akshayhavile, abhishekbsingh, arihantg, pkoshtavmware and DixitAakash September 20, 2024 04:42

Fix TestCreateUpdateDeleteL7RuleInHostRule

b7dfc0e

ashishnayak96 requested a review from apalsule September 23, 2024 06:08

akshayhavile reviewed Sep 23, 2024

View reviewed changes

Add buffer in tests

fb5615c

ashishnayak96 changed the title ~~Fix Unit tests~~ AV-218868: Fix common failing unit tests Sep 25, 2024

ashishnayak96 requested a review from akshayhavile September 26, 2024 10:31

ashishnayak96 assigned akshayhavile, aaha97, abhishekbsingh and pkoshtavmware Sep 26, 2024

aaha97 reviewed Sep 26, 2024

View reviewed changes

Address tests

20edf41

akshayhavile reviewed Sep 30, 2024

View reviewed changes

ashishnayak96 requested review from aaha97 and akshayhavile October 8, 2024 04:49

akshayhavile previously approved these changes Oct 11, 2024

View reviewed changes

Merge branch 'master' into uts

b28f5bb

ashishnayak96 dismissed akshayhavile’s stale review via f82d8db October 14, 2024 07:21

UT Fixes in restriciting fqdn to single namespaces

f82d8db

abhishekbsingh reviewed Oct 14, 2024

View reviewed changes

AV-218868: Fix common failing unit tests #1541

Are you sure you want to change the base?

AV-218868: Fix common failing unit tests #1541

Conversation

ashishnayak96 commented Sep 19, 2024

Dhivyaaj commented Sep 19, 2024

ashishnayak96 commented Sep 19, 2024

ashishnayak96 commented Sep 20, 2024

ashishnayak96 commented Sep 23, 2024

ashishnayak96 commented Sep 23, 2024

ashishnayak96 commented Sep 23, 2024

pkoshtavmware commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashishnayak96 commented Sep 24, 2024

aaha97 commented Sep 24, 2024

jeyapradeen-avi commented Sep 24, 2024

jeyapradeen-avi commented Sep 24, 2024

jeyapradeen-avi commented Sep 24, 2024

jeyapradeen-avi commented Sep 24, 2024

jeyapradeen-avi commented Sep 24, 2024

jeyapradeen-avi commented Sep 24, 2024

jeyapradeen-avi commented Sep 24, 2024

Dhivyaaj commented Sep 25, 2024

ashishnayak96 commented Sep 25, 2024

ashishnayak96 commented Sep 26, 2024

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

ashishnayak96 commented Sep 27, 2024

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

akshayhavile commented Oct 11, 2024

akshayhavile left a comment

Choose a reason for hiding this comment

ashishnayak96 commented Oct 14, 2024

ashishnayak96 commented Oct 14, 2024

ashishnayak96 commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishekbsingh Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashishnayak96 commented Oct 18, 2024

abhishekbsingh Oct 14, 2024 •

edited

Loading