Skip to content

Commit 4f4fa72

Browse files
authored
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.
1 parent 27c861b commit 4f4fa72

File tree

7 files changed

+46
-20
lines changed

7 files changed

+46
-20
lines changed

.github/workflows/nfr.yml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,15 +160,15 @@ jobs:
160160
- name: Open a PR with the results
161161
uses: peter-evans/create-pull-request@70a41aba780001da0a30141984ae2a0c95d8704e # v6.0.2
162162
with:
163-
commit-message: NFR Test Results for NGF version ${{ inputs.version }}
163+
commit-message: NFR Test Results for NGF version ${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '(Plus)' || ''}}
164164
author: ${{ github.actor }} <${{ github.actor_id }}+${{ github.actor }}@users.noreply.github.com>
165-
branch: tests/nfr-tests-${{ inputs.version }}
165+
branch: tests/nfr-tests-${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '-plus' || ''}}
166166
delete-branch: true
167-
title: NFR Test Results for NGF version ${{ inputs.version }}
167+
title: NFR Test Results for NGF version ${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '(Plus)' || ''}}
168168
add-paths: |
169169
tests/results/
170170
body: |
171-
Update with NFR test results for NGF version ${{ inputs.version }}
171+
Update with NFR test results for NGF version ${{ inputs.version }}${{ inputs.nginx_plus == 'true' && '(Plus)' || ''}}
172172
- Auto-generated by the NFR tests workflow run ${{ github.run_id }}
173173
- Tests ran using Docker image tag ${{ inputs.image_tag }}
174174
- ${{ inputs.test_label }} test(s) ran

tests/Makefile

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

tests/framework/results.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,16 @@ func CreateResultsFile(filename string) (*os.File, error) {
3232
return outFile, nil
3333
}
3434

35+
// CreateResultsFilename returns the name of the results file.
36+
func CreateResultsFilename(ext, base string, plusEnabled bool) string {
37+
name := fmt.Sprintf("%s.%s", base, ext)
38+
if plusEnabled {
39+
name = fmt.Sprintf("%s-plus.%s", base, ext)
40+
}
41+
42+
return name
43+
}
44+
3545
// WriteSystemInfoToFile writes the cluster system info to the given file.
3646
func WriteSystemInfoToFile(file *os.File, ci ClusterInfo, plus bool) error {
3747
clusterType := "Local"

tests/suite/dataplane_perf_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ var _ = Describe("Dataplane performance", Ordered, Label("nfr", "performance"),
6969
resultsDir, err := framework.CreateResultsDir("dp-perf", version)
7070
Expect(err).ToNot(HaveOccurred())
7171

72-
filename := filepath.Join(resultsDir, fmt.Sprintf("%s.md", version))
72+
filename := filepath.Join(resultsDir, framework.CreateResultsFilename("md", version, *plusEnabled))
7373
outFile, err = framework.CreateResultsFile(filename)
7474
Expect(err).ToNot(HaveOccurred())
7575
Expect(framework.WriteSystemInfoToFile(outFile, clusterInfo, *plusEnabled)).To(Succeed())

tests/suite/longevity_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ var _ = Describe("Longevity", Label("longevity-setup", "longevity-teardown"), fu
6565
resultsDir, err := framework.CreateResultsDir("longevity", version)
6666
Expect(err).ToNot(HaveOccurred())
6767

68-
filename := filepath.Join(resultsDir, fmt.Sprintf("%s.md", version))
68+
filename := filepath.Join(resultsDir, framework.CreateResultsFilename("md", version, *plusEnabled))
6969
resultsFile, err := framework.CreateResultsFile(filename)
7070
Expect(err).ToNot(HaveOccurred())
7171
defer resultsFile.Close()

tests/suite/system_suite_test.go

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,15 @@ var (
4747
)
4848
k8sVersion = flag.String("k8s-version", "latest", "Version of k8s being tested on")
4949
// Configurable NGF installation variables. Helm values will be used as defaults if not specified.
50-
ngfImageRepository = flag.String("ngf-image-repo", "", "Image repo for NGF control plane")
51-
nginxImageRepository = flag.String("nginx-image-repo", "", "Image repo for NGF data plane")
52-
imageTag = flag.String("image-tag", "", "Image tag for NGF images")
53-
versionUnderTest = flag.String("version-under-test", "", "Version of NGF that is being tested")
54-
imagePullPolicy = flag.String("pull-policy", "", "Image pull policy for NGF images")
55-
serviceType = flag.String("service-type", "NodePort", "Type of service fronting NGF to be deployed")
56-
isGKEInternalLB = flag.Bool("is-gke-internal-lb", false, "Is the LB service GKE internal only")
57-
plusEnabled = flag.Bool("plus-enabled", false, "Is NGINX Plus enabled")
50+
ngfImageRepository = flag.String("ngf-image-repo", "", "Image repo for NGF control plane")
51+
nginxImageRepository = flag.String("nginx-image-repo", "", "Image repo for NGF data plane")
52+
nginxPlusImageRepository = flag.String("nginx-plus-image-repo", "", "Image repo for NGF N+ data plane")
53+
imageTag = flag.String("image-tag", "", "Image tag for NGF images")
54+
versionUnderTest = flag.String("version-under-test", "", "Version of NGF that is being tested")
55+
imagePullPolicy = flag.String("pull-policy", "", "Image pull policy for NGF images")
56+
serviceType = flag.String("service-type", "NodePort", "Type of service fronting NGF to be deployed")
57+
isGKEInternalLB = flag.Bool("is-gke-internal-lb", false, "Is the LB service GKE internal only")
58+
plusEnabled = flag.Bool("plus-enabled", false, "Is NGINX Plus enabled")
5859
)
5960

6061
var (
@@ -153,6 +154,9 @@ func setup(cfg setupConfig, extraInstallArgs ...string) {
153154
if !strings.HasPrefix(cfg.chartPath, "oci://") {
154155
installCfg.NgfImageRepository = *ngfImageRepository
155156
installCfg.NginxImageRepository = *nginxImageRepository
157+
if *plusEnabled && cfg.nfr {
158+
installCfg.NginxImageRepository = *nginxPlusImageRepository
159+
}
156160
installCfg.ImageTag = *imageTag
157161
installCfg.ImagePullPolicy = *imagePullPolicy
158162
}

tests/suite/upgrade_test.go

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
6868
resultsDir, err = framework.CreateResultsDir("ngf-upgrade", version)
6969
Expect(err).ToNot(HaveOccurred())
7070

71-
filename := filepath.Join(resultsDir, fmt.Sprintf("%s.md", version))
71+
filename := filepath.Join(resultsDir, framework.CreateResultsFilename("md", version, *plusEnabled))
7272
resultsFile, err = framework.CreateResultsFile(filename)
7373
Expect(err).ToNot(HaveOccurred())
7474
Expect(framework.WriteSystemInfoToFile(resultsFile, clusterInfo, *plusEnabled)).To(Succeed())
@@ -81,16 +81,22 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
8181
})
8282

8383
It("upgrades NGF with zero downtime", func() {
84+
nginxImage := *nginxImageRepository
85+
if *plusEnabled {
86+
nginxImage = *nginxPlusImageRepository
87+
}
88+
8489
cfg := framework.InstallationConfig{
8590
ReleaseName: releaseName,
8691
Namespace: ngfNamespace,
8792
ChartPath: localChartPath,
8893
NgfImageRepository: *ngfImageRepository,
89-
NginxImageRepository: *nginxImageRepository,
94+
NginxImageRepository: nginxImage,
9095
ImageTag: *imageTag,
9196
ImagePullPolicy: *imagePullPolicy,
9297
ServiceType: *serviceType,
9398
IsGKEInternalLB: *isGKEInternalLB,
99+
Plus: *plusEnabled,
94100
}
95101

96102
type metricsResults struct {
@@ -157,7 +163,7 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
157163
Expect(encoder.Encode(&res)).To(Succeed())
158164
}
159165

160-
csvName := fmt.Sprintf("%s.csv", scheme)
166+
csvName := framework.CreateResultsFilename("csv", scheme, *plusEnabled)
161167
filename := filepath.Join(resultsDir, csvName)
162168
csvFile, err := framework.CreateResultsFile(filename)
163169
Expect(err).ToNot(HaveOccurred())
@@ -166,7 +172,8 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
166172
Expect(err).ToNot(HaveOccurred())
167173
csvFile.Close()
168174

169-
output, err := framework.GeneratePNG(resultsDir, csvName, fmt.Sprintf("%s.png", scheme))
175+
pngName := framework.CreateResultsFilename("png", scheme, *plusEnabled)
176+
output, err := framework.GeneratePNG(resultsDir, csvName, pngName)
170177
Expect(err).ToNot(HaveOccurred(), string(output))
171178

172179
metricsCh <- &metricsRes
@@ -246,7 +253,12 @@ var _ = Describe("Upgrade testing", Label("nfr", "upgrade"), func() {
246253

247254
Expect(framework.WriteResults(resultsFile, res.metrics)).To(Succeed())
248255

249-
_, err = fmt.Fprintf(resultsFile, "```\n\n![%[1]v.png](%[1]v.png)\n", res.scheme)
256+
link := fmt.Sprintf("\n\n![%[1]v.png](%[1]v.png)\n", res.scheme)
257+
if *plusEnabled {
258+
link = fmt.Sprintf("\n\n![%[1]v-plus.png](%[1]v-plus.png)\n", res.scheme)
259+
}
260+
261+
_, err = fmt.Fprintf(resultsFile, "```%s", link)
250262
Expect(err).ToNot(HaveOccurred())
251263
}
252264
})

0 commit comments

Comments
 (0)