From b59ef519d7683b386754f0b0e0567b3c565eb6f6 Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 29 May 2024 14:26:08 -0600 Subject: [PATCH 01/10] add graceful-recovery test to pipeline --- .github/workflows/functional.yml | 1 - tests/suite/graceful_recovery_test.go | 14 ++++++++++---- tests/suite/system_suite_test.go | 7 ++++--- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/.github/workflows/functional.yml b/.github/workflows/functional.yml index 4cec276d40..8e3ab3b115 100644 --- a/.github/workflows/functional.yml +++ b/.github/workflows/functional.yml @@ -119,7 +119,6 @@ jobs: ngf_tag=${{ steps.ngf-meta.outputs.version }} make test${{ inputs.image == 'plus' && '-with-plus' || ''}} PREFIX=${ngf_prefix} TAG=${ngf_tag} GINKGO_LABEL=telemetry working-directory: ./tests - - name: Run functional tests run: | ngf_prefix=ghcr.io/nginxinc/nginx-gateway-fabric diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index b4148039e4..3be3f98c63 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -28,7 +28,7 @@ const ( // Since checkContainerLogsForErrors may experience interference from previous tests (as explained in the function // documentation), this test is recommended to be run separate from other nfr tests. -var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recovery"), func() { +var _ = Describe("Graceful Recovery test", Ordered, Label("functional", "graceful-recovery"), func() { files := []string{ "graceful-recovery/cafe.yaml", "graceful-recovery/cafe-secret.yaml", @@ -42,10 +42,10 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov }, } - teaURL := "https://cafe.example.com/tea" - coffeeURL := "http://cafe.example.com/coffee" + teaURI := "/tea" + coffeeURI := "http://cafe.example.com/coffee" - var ngfPodName string + var ngfPodName, teaURL, coffeeURL string BeforeAll(func() { // this test is unique in that it will check the entire log of both ngf and nginx containers @@ -60,6 +60,12 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov Expect(podNames).To(HaveLen(1)) ngfPodName = podNames[0] + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + teaURL = fmt.Sprintf("http://cafe.example.com:%d", port) + teaURI + coffeeURL = fmt.Sprintf("http://cafe.example.com:%d", port) + coffeeURI }) BeforeEach(func() { diff --git a/tests/suite/system_suite_test.go b/tests/suite/system_suite_test.go index e8a7ea9876..3ae31c4afa 100644 --- a/tests/suite/system_suite_test.go +++ b/tests/suite/system_suite_test.go @@ -259,8 +259,8 @@ var _ = BeforeSuite(func() { "upgrade", // - running upgrade test (this test will deploy its own version) "longevity-teardown", // - running longevity teardown (deployment will already exist) "telemetry", // - running telemetry test (NGF will be deployed as part of the test) - "graceful-recovery", // - running graceful recovery test (this test will deploy its own version) - "scale", // - running scale test (this test will deploy its own version) + // "graceful-recovery", // - running graceful recovery test (this test will deploy its own version) + "scale", // - running scale test (this test will deploy its own version) } for _, s := range skipSubstrings { if strings.Contains(labelFilter, s) { @@ -299,6 +299,7 @@ func isNFR(labelFilter string) bool { strings.Contains(labelFilter, "longevity") || strings.Contains(labelFilter, "performance") || strings.Contains(labelFilter, "upgrade") || - strings.Contains(labelFilter, "graceful-recovery") || + // graceful-recovery tests are treated as functional tests to be run in the pipeline + // strings.Contains(labelFilter, "graceful-recovery") || strings.Contains(labelFilter, "scale") } From 15ad48cd295fd679b5cd878763cf3cc8f27aa2fd Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 29 May 2024 16:00:50 -0600 Subject: [PATCH 02/10] fix url for routes --- tests/suite/graceful_recovery_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 3be3f98c63..37077e47ac 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -42,10 +42,12 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("functional", "gracefu }, } - teaURI := "/tea" - coffeeURI := "http://cafe.example.com/coffee" + baseHttpURL := "http://cafe.example.com" + baseHttpsURL := "https://cafe.example.com" + teaURL := baseHttpsURL + "/tea" + coffeeURL := baseHttpURL + "/coffee" - var ngfPodName, teaURL, coffeeURL string + var ngfPodName string BeforeAll(func() { // this test is unique in that it will check the entire log of both ngf and nginx containers @@ -60,12 +62,12 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("functional", "gracefu Expect(podNames).To(HaveLen(1)) ngfPodName = podNames[0] - port := 80 if portFwdPort != 0 { - port = portFwdPort + coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHttpURL, portFwdPort) + } + if portFwdHTTPSPort != 0 { + teaURL = fmt.Sprintf("%s:%d/tea", baseHttpsURL, portFwdHTTPSPort) } - teaURL = fmt.Sprintf("http://cafe.example.com:%d", port) + teaURI - coffeeURL = fmt.Sprintf("http://cafe.example.com:%d", port) + coffeeURI }) BeforeEach(func() { From 4c82953346c49e36572285331a0abd5c5bf67a61 Mon Sep 17 00:00:00 2001 From: Saloni Date: Fri, 31 May 2024 16:03:21 -0600 Subject: [PATCH 03/10] add extra lines and remove comment --- .github/workflows/functional.yml | 1 + tests/suite/system_suite_test.go | 5 +---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/functional.yml b/.github/workflows/functional.yml index 8e3ab3b115..4cec276d40 100644 --- a/.github/workflows/functional.yml +++ b/.github/workflows/functional.yml @@ -119,6 +119,7 @@ jobs: ngf_tag=${{ steps.ngf-meta.outputs.version }} make test${{ inputs.image == 'plus' && '-with-plus' || ''}} PREFIX=${ngf_prefix} TAG=${ngf_tag} GINKGO_LABEL=telemetry working-directory: ./tests + - name: Run functional tests run: | ngf_prefix=ghcr.io/nginxinc/nginx-gateway-fabric diff --git a/tests/suite/system_suite_test.go b/tests/suite/system_suite_test.go index 3ae31c4afa..c44b2e05eb 100644 --- a/tests/suite/system_suite_test.go +++ b/tests/suite/system_suite_test.go @@ -259,8 +259,7 @@ var _ = BeforeSuite(func() { "upgrade", // - running upgrade test (this test will deploy its own version) "longevity-teardown", // - running longevity teardown (deployment will already exist) "telemetry", // - running telemetry test (NGF will be deployed as part of the test) - // "graceful-recovery", // - running graceful recovery test (this test will deploy its own version) - "scale", // - running scale test (this test will deploy its own version) + "scale", // - running scale test (this test will deploy its own version) } for _, s := range skipSubstrings { if strings.Contains(labelFilter, s) { @@ -299,7 +298,5 @@ func isNFR(labelFilter string) bool { strings.Contains(labelFilter, "longevity") || strings.Contains(labelFilter, "performance") || strings.Contains(labelFilter, "upgrade") || - // graceful-recovery tests are treated as functional tests to be run in the pipeline - // strings.Contains(labelFilter, "graceful-recovery") || strings.Contains(labelFilter, "scale") } From 1f6e6b41be6651cf79e2aef836e72c1f21ab2c64 Mon Sep 17 00:00:00 2001 From: Saloni Date: Fri, 31 May 2024 16:23:09 -0600 Subject: [PATCH 04/10] fix test for nplus scenarios --- tests/suite/graceful_recovery_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 37077e47ac..a48e6e11c4 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -280,7 +280,9 @@ func checkContainerLogsForErrors(ngfPodName string) { &core.PodLogOptions{Container: ngfContainerName}, ) Expect(err).ToNot(HaveOccurred()) - Expect(logs).ToNot(ContainSubstring("\"level\":\"error\""), logs) + if !*plusEnabled { + Expect(logs).ToNot(ContainSubstring("\"level\":\"error\""), logs) + } } func checkLeaderLeaseChange(originalLeaseName string) error { From 6ede9e437df3334dbd1aaa664bc11087708d6a31 Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 3 Jun 2024 12:07:53 -0600 Subject: [PATCH 05/10] check log lines for specific error --- tests/suite/graceful_recovery_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index a48e6e11c4..0efbcb1e93 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -280,8 +280,13 @@ func checkContainerLogsForErrors(ngfPodName string) { &core.PodLogOptions{Container: ngfContainerName}, ) Expect(err).ToNot(HaveOccurred()) - if !*plusEnabled { - Expect(logs).ToNot(ContainSubstring("\"level\":\"error\""), logs) + + for _, line := range strings.Split(logs, "\n") { + if *plusEnabled && strings.Contains(line, "[error]") { + Expect(line).To(ContainSubstring("Usage reporting must be enabled when using NGINX Plus"), line) + } else { + Expect(line).ToNot(ContainSubstring("[error]"), line) + } } } From c2f1e3dd945e40d31a476aeff10920d565b60b7e Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 3 Jun 2024 12:54:07 -0600 Subject: [PATCH 06/10] fix error check --- tests/suite/graceful_recovery_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 0efbcb1e93..0d3af3d342 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -282,10 +282,10 @@ func checkContainerLogsForErrors(ngfPodName string) { Expect(err).ToNot(HaveOccurred()) for _, line := range strings.Split(logs, "\n") { - if *plusEnabled && strings.Contains(line, "[error]") { + if *plusEnabled && strings.Contains(line, "\"level\":\"error\"") { Expect(line).To(ContainSubstring("Usage reporting must be enabled when using NGINX Plus"), line) } else { - Expect(line).ToNot(ContainSubstring("[error]"), line) + Expect(line).ToNot(ContainSubstring("\"level\":\"error\""), line) } } } From f66d2e032bf99b1ebce34331aa8d4d53d04c38c7 Mon Sep 17 00:00:00 2001 From: Saloni Date: Tue, 4 Jun 2024 10:52:13 -0600 Subject: [PATCH 07/10] steps to reproduce intermittent error --- tests/framework/resourcemanager.go | 50 +++++++++++++++++++++++++++ tests/suite/graceful_recovery_test.go | 6 ++-- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/tests/framework/resourcemanager.go b/tests/framework/resourcemanager.go index 193556f9b3..b08c4c43bd 100644 --- a/tests/framework/resourcemanager.go +++ b/tests/framework/resourcemanager.go @@ -683,3 +683,53 @@ func countNumberOfReadyParents(parents []v1.RouteParentStatus) int { return readyCount } + +func (rm *ResourceManager) WaitForAppsToBeReadyWithPodCount(namespace string, podCount int) error { + ctx, cancel := context.WithTimeout(context.Background(), rm.TimeoutConfig.CreateTimeout) + defer cancel() + + return rm.WaitForAppsToBeReadyWithCtxWithPodCount(ctx, namespace, podCount) +} + +func (rm *ResourceManager) WaitForAppsToBeReadyWithCtxWithPodCount(ctx context.Context, namespace string, podCount int) error { + if err := rm.WaitForPodsToBeReadyWithCount(ctx, namespace, podCount); err != nil { + return err + } + + if err := rm.waitForHTTPRoutesToBeReady(ctx, namespace); err != nil { + return err + } + + if err := rm.waitForGRPCRoutesToBeReady(ctx, namespace); err != nil { + return err + } + + return rm.waitForGatewaysToBeReady(ctx, namespace) +} + +// WaitForPodsToBeReady waits for all Pods in the specified namespace to be ready or +// until the provided context is cancelled. +func (rm *ResourceManager) WaitForPodsToBeReadyWithCount(ctx context.Context, namespace string, count int) error { + return wait.PollUntilContextCancel( + ctx, + 500*time.Millisecond, + true, /* poll immediately */ + func(ctx context.Context) (bool, error) { + var podList core.PodList + if err := rm.K8sClient.List(ctx, &podList, client.InNamespace(namespace)); err != nil { + return false, err + } + + var podsReady int + for _, pod := range podList.Items { + for _, cond := range pod.Status.Conditions { + if cond.Type == core.PodReady && cond.Status == core.ConditionTrue { + podsReady++ + } + } + } + + return podsReady == count, nil + }, + ) +} diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 0d3af3d342..41f490855e 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -73,7 +73,7 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("functional", "gracefu BeforeEach(func() { Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReadyWithPodCount(ns.Name, 2)).To(Succeed()) Eventually( func() error { @@ -107,7 +107,7 @@ func runRecoveryTest(teaURL, coffeeURL, ngfPodName, containerName string, files ) if containerName != nginxContainerName { - // Since we have already deployed resources and ran resourceManager.WaitForAppsToBeReady(ns.Name) earlier, + // Since we have already deployed resources and ran resourceManager.WaitForAppsToBeReadyWithPodCount earlier, // we know that the applications are ready at this point. This could only be the case if NGF has written // statuses, which could only be the case if NGF has the leader lease. Since there is only one instance // of NGF in this test, we can be certain that this is the correct leaseholder name. @@ -146,7 +146,7 @@ func runRecoveryTest(teaURL, coffeeURL, ngfPodName, containerName string, files Should(Succeed()) Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReadyWithPodCount(ns.Name, 2)).To(Succeed()) Eventually( func() error { From 3939f43b18e4153b7d7bd621429c66fa25f47dbd Mon Sep 17 00:00:00 2001 From: Saloni Date: Tue, 4 Jun 2024 16:31:40 -0600 Subject: [PATCH 08/10] bypass existing upstream error in pipeline --- tests/suite/graceful_recovery_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 41f490855e..c0d1892f81 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -270,7 +270,10 @@ func checkContainerLogsForErrors(ngfPodName string) { Expect(line).ToNot(ContainSubstring("[alert]"), line) Expect(line).ToNot(ContainSubstring("[emerg]"), line) if strings.Contains(line, "[error]") { - Expect(line).To(ContainSubstring("connect() failed (111: Connection refused)"), line) + expectedError1 := "connect() failed (111: Connection refused)" + //FIXME(salonichf5) remove this error message check when https://github.com/nginxinc/nginx-gateway-fabric/issues/2090 is completed. + expectedError2 := "no live upstreams while connecting to upstream" + Expect(line).To(Or(Equal(expectedError1), Equal(expectedError2))) } } From 19e3c07d55e50eee15531d298f65145c837275e1 Mon Sep 17 00:00:00 2001 From: Saloni Date: Tue, 4 Jun 2024 16:36:30 -0600 Subject: [PATCH 09/10] correct spelling mistake --- tests/framework/resourcemanager.go | 6 +++--- tests/suite/graceful_recovery_test.go | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/framework/resourcemanager.go b/tests/framework/resourcemanager.go index b08c4c43bd..1fb5bbbb61 100644 --- a/tests/framework/resourcemanager.go +++ b/tests/framework/resourcemanager.go @@ -307,7 +307,7 @@ func (rm *ResourceManager) WaitForAppsToBeReady(namespace string) error { } // WaitForAppsToBeReadyWithCtx waits for all apps in the specified namespace to be ready or -// until the provided context is cancelled. +// until the provided context is canceled. func (rm *ResourceManager) WaitForAppsToBeReadyWithCtx(ctx context.Context, namespace string) error { if err := rm.WaitForPodsToBeReady(ctx, namespace); err != nil { return err @@ -325,7 +325,7 @@ func (rm *ResourceManager) WaitForAppsToBeReadyWithCtx(ctx context.Context, name } // WaitForPodsToBeReady waits for all Pods in the specified namespace to be ready or -// until the provided context is cancelled. +// until the provided context is canceled. func (rm *ResourceManager) WaitForPodsToBeReady(ctx context.Context, namespace string) error { return wait.PollUntilContextCancel( ctx, @@ -708,7 +708,7 @@ func (rm *ResourceManager) WaitForAppsToBeReadyWithCtxWithPodCount(ctx context.C } // WaitForPodsToBeReady waits for all Pods in the specified namespace to be ready or -// until the provided context is cancelled. +// until the provided context is canceled. func (rm *ResourceManager) WaitForPodsToBeReadyWithCount(ctx context.Context, namespace string, count int) error { return wait.PollUntilContextCancel( ctx, diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index c0d1892f81..890c192c2b 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -42,10 +42,10 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("functional", "gracefu }, } - baseHttpURL := "http://cafe.example.com" - baseHttpsURL := "https://cafe.example.com" - teaURL := baseHttpsURL + "/tea" - coffeeURL := baseHttpURL + "/coffee" + baseHTTPURL := "http://cafe.example.com" + baseHTTPSURL := "https://cafe.example.com" + teaURL := baseHTTPSURL + "/tea" + coffeeURL := baseHTTPURL + "/coffee" var ngfPodName string @@ -63,10 +63,10 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("functional", "gracefu ngfPodName = podNames[0] if portFwdPort != 0 { - coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHttpURL, portFwdPort) + coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHTTPURL, portFwdPort) } if portFwdHTTPSPort != 0 { - teaURL = fmt.Sprintf("%s:%d/tea", baseHttpsURL, portFwdHTTPSPort) + teaURL = fmt.Sprintf("%s:%d/tea", baseHTTPSURL, portFwdHTTPSPort) } }) From 2d4d527f3a9cf6fee78ffce4d82e71a12ecea583 Mon Sep 17 00:00:00 2001 From: Saloni Date: Tue, 4 Jun 2024 18:25:34 -0600 Subject: [PATCH 10/10] fix lint errors --- tests/framework/resourcemanager.go | 6 +++++- tests/suite/graceful_recovery_test.go | 5 +++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/framework/resourcemanager.go b/tests/framework/resourcemanager.go index 1fb5bbbb61..c7c231c69b 100644 --- a/tests/framework/resourcemanager.go +++ b/tests/framework/resourcemanager.go @@ -691,7 +691,11 @@ func (rm *ResourceManager) WaitForAppsToBeReadyWithPodCount(namespace string, po return rm.WaitForAppsToBeReadyWithCtxWithPodCount(ctx, namespace, podCount) } -func (rm *ResourceManager) WaitForAppsToBeReadyWithCtxWithPodCount(ctx context.Context, namespace string, podCount int) error { +func (rm *ResourceManager) WaitForAppsToBeReadyWithCtxWithPodCount( + ctx context.Context, + namespace string, + podCount int, +) error { if err := rm.WaitForPodsToBeReadyWithCount(ctx, namespace, podCount); err != nil { return err } diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 890c192c2b..19cdc8caa1 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -271,9 +271,10 @@ func checkContainerLogsForErrors(ngfPodName string) { Expect(line).ToNot(ContainSubstring("[emerg]"), line) if strings.Contains(line, "[error]") { expectedError1 := "connect() failed (111: Connection refused)" - //FIXME(salonichf5) remove this error message check when https://github.com/nginxinc/nginx-gateway-fabric/issues/2090 is completed. + // FIXME(salonichf5) remove this error message check + // when https://github.com/nginxinc/nginx-gateway-fabric/issues/2090 is completed. expectedError2 := "no live upstreams while connecting to upstream" - Expect(line).To(Or(Equal(expectedError1), Equal(expectedError2))) + Expect(line).To(Or(ContainSubstring(expectedError1), ContainSubstring(expectedError2))) } }