Fix automated NFR issues with OSS vs Plus (#1721)

Problem:
- All results files used the same name
- The PR branch was the same for both OSS and Plus
- Plus image was not used when running tests

Solution:
- Separate OSS and Plus runs to use correct images, write to separate files, and use a separate branch.

Author: sjberman

.github/workflows/nfr.yml

@@ -160,15 +160,15 @@ jobs:
     - name: Open a PR with the results
       uses: peter-evans/create-pull-request@70a41aba780001da0a30141984ae2a0c95d8704e # v6.0.2
       with:
-        commit-message: NFR Test Results for NGF version ${{ inputs.version }}
+        commit-message: NFR Test Results for NGF version ${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '(Plus)' || ''}}
         author: ${{ github.actor }} <${{ github.actor_id }}+${{ github.actor }}@users.noreply.github.com>
-        branch: tests/nfr-tests-${{ inputs.version }}
+        branch: tests/nfr-tests-${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '-plus' || ''}}
         delete-branch: true
-        title: NFR Test Results for NGF version ${{ inputs.version }}
+        title: NFR Test Results for NGF version ${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '(Plus)' || ''}}
         add-paths: |
           tests/results/
         body: |
-          Update with NFR test results for NGF version ${{ inputs.version }}
+          Update with NFR test results for NGF version ${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '(Plus)' || ''}}
           - Auto-generated by the NFR tests workflow run ${{ github.run_id }}
           - Tests ran using Docker image tag ${{ inputs.image_tag }}
           - ${{ inputs.test_label }} test(s) ran

tests/Makefile

@@ -96,7 +96,7 @@ stop-longevity-test: ## Stops the longevity test and collects results
 .vm-nfr-test: ## Runs the NFR tests on the GCP VM (called by `nfr-test`)
 	go test -v ./suite -ginkgo.label-filter "nfr" $(GINKGO_FLAGS) -ginkgo.v -args --gateway-api-version=$(GW_API_VERSION) \
 		--gateway-api-prev-version=$(GW_API_PREV_VERSION) --image-tag=$(TAG) --version-under-test=$(NGF_VERSION) \
-		--plus-enabled=$(PLUS_ENABLED) --ngf-image-repo=$(PREFIX) --nginx-image-repo=$(NGINX_PREFIX) \
+		--plus-enabled=$(PLUS_ENABLED) --ngf-image-repo=$(PREFIX) --nginx-image-repo=$(NGINX_PREFIX) --nginx-plus-image-repo=$(NGINX_PLUS_PREFIX) \
 		--pull-policy=$(PULL_POLICY) --k8s-version=$(K8S_VERSION) --service-type=$(GW_SERVICE_TYPE) \
 		--is-gke-internal-lb=$(GW_SVC_GKE_INTERNAL)
 

tests/framework/results.go

@@ -32,6 +32,16 @@ func CreateResultsFile(filename string) (*os.File, error) {
 	return outFile, nil
 }
 
+// CreateResultsFilename returns the name of the results file.
+func CreateResultsFilename(ext, base string, plusEnabled bool) string {
+	name := fmt.Sprintf("%s.%s", base, ext)
+	if plusEnabled {
+		name = fmt.Sprintf("%s-plus.%s", base, ext)
+	}
+
+	return name
+}
+
 // WriteSystemInfoToFile writes the cluster system info to the given file.
 func WriteSystemInfoToFile(file *os.File, ci ClusterInfo, plus bool) error {
 	clusterType := "Local"

tests/suite/dataplane_perf_test.go

@@ -69,7 +69,7 @@ var _ = Describe("Dataplane performance", Ordered, Label("nfr", "performance"),
 		resultsDir, err := framework.CreateResultsDir("dp-perf", version)
 		Expect(err).ToNot(HaveOccurred())
 
-		filename := filepath.Join(resultsDir, fmt.Sprintf("%s.md", version))
+		filename := filepath.Join(resultsDir, framework.CreateResultsFilename("md", version, *plusEnabled))
 		outFile, err = framework.CreateResultsFile(filename)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(framework.WriteSystemInfoToFile(outFile, clusterInfo, *plusEnabled)).To(Succeed())

tests/suite/longevity_test.go

@@ -65,7 +65,7 @@ var _ = Describe("Longevity", Label("longevity-setup", "longevity-teardown"), fu
 		resultsDir, err := framework.CreateResultsDir("longevity", version)
 		Expect(err).ToNot(HaveOccurred())
 
-		filename := filepath.Join(resultsDir, fmt.Sprintf("%s.md", version))
+		filename := filepath.Join(resultsDir, framework.CreateResultsFilename("md", version, *plusEnabled))
 		resultsFile, err := framework.CreateResultsFile(filename)
 		Expect(err).ToNot(HaveOccurred())
 		defer resultsFile.Close()

tests/suite/system_suite_test.go

@@ -47,14 +47,15 @@ var (
 	)
 	k8sVersion = flag.String("k8s-version", "latest", "Version of k8s being tested on")
 	// Configurable NGF installation variables. Helm values will be used as defaults if not specified.
-	ngfImageRepository   = flag.String("ngf-image-repo", "", "Image repo for NGF control plane")
-	nginxImageRepository = flag.String("nginx-image-repo", "", "Image repo for NGF data plane")
-	imageTag             = flag.String("image-tag", "", "Image tag for NGF images")
-	versionUnderTest     = flag.String("version-under-test", "", "Version of NGF that is being tested")
-	imagePullPolicy      = flag.String("pull-policy", "", "Image pull policy for NGF images")
-	serviceType          = flag.String("service-type", "NodePort", "Type of service fronting NGF to be deployed")
-	isGKEInternalLB      = flag.Bool("is-gke-internal-lb", false, "Is the LB service GKE internal only")
-	plusEnabled          = flag.Bool("plus-enabled", false, "Is NGINX Plus enabled")
+	ngfImageRepository       = flag.String("ngf-image-repo", "", "Image repo for NGF control plane")
+	nginxImageRepository     = flag.String("nginx-image-repo", "", "Image repo for NGF data plane")
+	nginxPlusImageRepository = flag.String("nginx-plus-image-repo", "", "Image repo for NGF N+ data plane")
+	imageTag                 = flag.String("image-tag", "", "Image tag for NGF images")
+	versionUnderTest         = flag.String("version-under-test", "", "Version of NGF that is being tested")
+	imagePullPolicy          = flag.String("pull-policy", "", "Image pull policy for NGF images")
+	serviceType              = flag.String("service-type", "NodePort", "Type of service fronting NGF to be deployed")
+	isGKEInternalLB          = flag.Bool("is-gke-internal-lb", false, "Is the LB service GKE internal only")
+	plusEnabled              = flag.Bool("plus-enabled", false, "Is NGINX Plus enabled")
 )
 
 var (
@@ -153,6 +154,9 @@ func setup(cfg setupConfig, extraInstallArgs ...string) {
 	if !strings.HasPrefix(cfg.chartPath, "oci://") {
 		installCfg.NgfImageRepository = *ngfImageRepository
 		installCfg.NginxImageRepository = *nginxImageRepository
+		if *plusEnabled && cfg.nfr {
+			installCfg.NginxImageRepository = *nginxPlusImageRepository
+		}
 		installCfg.ImageTag = *imageTag
 		installCfg.ImagePullPolicy = *imagePullPolicy
 	}

tests/suite/upgrade_test.go

@@ -68,7 +68,7 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
 		resultsDir, err = framework.CreateResultsDir("ngf-upgrade", version)
 		Expect(err).ToNot(HaveOccurred())
 
-		filename := filepath.Join(resultsDir, fmt.Sprintf("%s.md", version))
+		filename := filepath.Join(resultsDir, framework.CreateResultsFilename("md", version, *plusEnabled))
 		resultsFile, err = framework.CreateResultsFile(filename)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(framework.WriteSystemInfoToFile(resultsFile, clusterInfo, *plusEnabled)).To(Succeed())
@@ -81,16 +81,22 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
 	})
 
 	It("upgrades NGF with zero downtime", func() {
+		nginxImage := *nginxImageRepository
+		if *plusEnabled {
+			nginxImage = *nginxPlusImageRepository
+		}
+
 		cfg := framework.InstallationConfig{
 			ReleaseName:          releaseName,
 			Namespace:            ngfNamespace,
 			ChartPath:            localChartPath,
 			NgfImageRepository:   *ngfImageRepository,
-			NginxImageRepository: *nginxImageRepository,
+			NginxImageRepository: nginxImage,
 			ImageTag:             *imageTag,
 			ImagePullPolicy:      *imagePullPolicy,
 			ServiceType:          *serviceType,
 			IsGKEInternalLB:      *isGKEInternalLB,
+			Plus:                 *plusEnabled,
 		}
 
 		type metricsResults struct {
@@ -157,7 +163,7 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
 					Expect(encoder.Encode(&res)).To(Succeed())
 				}
 
-				csvName := fmt.Sprintf("%s.csv", scheme)
+				csvName := framework.CreateResultsFilename("csv", scheme, *plusEnabled)
 				filename := filepath.Join(resultsDir, csvName)
 				csvFile, err := framework.CreateResultsFile(filename)
 				Expect(err).ToNot(HaveOccurred())
@@ -166,7 +172,8 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
 				Expect(err).ToNot(HaveOccurred())
 				csvFile.Close()
 
-				output, err := framework.GeneratePNG(resultsDir, csvName, fmt.Sprintf("%s.png", scheme))
+				pngName := framework.CreateResultsFilename("png", scheme, *plusEnabled)
+				output, err := framework.GeneratePNG(resultsDir, csvName, pngName)
 				Expect(err).ToNot(HaveOccurred(), string(output))
 
 				metricsCh <- &metricsRes
@@ -246,7 +253,12 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
 
 			Expect(framework.WriteResults(resultsFile, res.metrics)).To(Succeed())
 
-			_, err = fmt.Fprintf(resultsFile, "```\n\n![%[1]v.png](%[1]v.png)\n", res.scheme)
+			link := fmt.Sprintf("\n\n![%[1]v.png](%[1]v.png)\n", res.scheme)
+			if *plusEnabled {
+				link = fmt.Sprintf("\n\n![%[1]v-plus.png](%[1]v-plus.png)\n", res.scheme)
+			}
+
+			_, err = fmt.Fprintf(resultsFile, "```%s", link)
 			Expect(err).ToNot(HaveOccurred())
 		}
 	})