Repository: kubernetes-sigs/security-profiles-operator
Pull request #1080 (2022-08-09 19:54:55)

Improving test flakiness
#### What type of PR is this?

/kind cleanup
/kind flake

#### What this PR does / why we need it:

Optimisations around flaky tests:
- Removes panic in Seccomp test when trying to access out of bounds index.
- Removes magic number when trying to reset function used to re-deploy namespaced operator.
- Segregate flaky tests into their own test, which does not fail the build on error.

#### Which issue(s) this PR fixes:

Relates to https://github.com/kubernetes-sigs/security-profiles-operator/issues/1087.

#### Does this PR have test?

N/A

#### Special notes for your reviewer:

The idea about segregating the flaky tests is to speed up the PR to merge time.
An alternative I was thinking, was to set it to happen nightly with a few auto-retries.

#### Does this PR introduce a user-facing change?

```release-note
NONE
```


k8s-ci-robot: Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with `/test all`
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/security-profiles-operator/pull/1080#" title="Author self-approved">pjbgf</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fsecurity-profiles-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/security-profiles-operator/blob/main/OWNERS)~~ [pjbgf]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov-commenter: # [Codecov](https://codecov.io/gh/kubernetes-sigs/security-profiles-operator/pull/1080?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#1080](https://codecov.io/gh/kubernetes-sigs/security-profiles-operator/pull/1080?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (4911066) into [main](https://codecov.io/gh/kubernetes-sigs/security-profiles-operator/commit/525990f99282ec1d4d7ecc8ac2398d04d5f2f36c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (525990f) will **not change** coverage.
> The diff coverage is `n/a`.

```diff
@@           Coverage Diff           @@
##             main    #1080   +/-   ##
=======================================
  Coverage   50.43%   50.43%           
=======================================
  Files          42       42           
  Lines        4761     4761           
=======================================
  Hits         2401     2401           
  Misses       2281     2281           
  Partials       79       79           
```

k8s-ci-robot: @pjbgf: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
JAORMX: /lgtm

Patch:
diff --git a/Makefile b/Makefile
index c070c373a..e751fc813 100644
--- a/Makefile
+++ b/Makefile
@@ -388,7 +388,15 @@ test-unit: $(BUILD_DIR) ## Run the unit tests
 
 .PHONY: test-e2e
 test-e2e: ## Run the end-to-end tests
-	CGO_LDFLAGS= $(GO) test -parallel 1 -timeout 80m -count=1 ./test/... -v
+	CGO_LDFLAGS= \
+	E2E_SKIP_FLAKY_TESTS=true \
+	$(GO) test -parallel 1 -timeout 60m -count=1 ./test/... -v
+
+.PHONY: test-flaky-e2e
+test-flaky-e2e: ## Only run the flaky end-to-end tests
+	CGO_LDFLAGS= \
+	E2E_SKIP_FLAKY_TESTS=false \
+	$(GO) test -parallel 1 -testify.m '^(TestSecurityProfilesOperator_Flaky)$$' -timeout 20m -count=1 ./test/... -v
 
 # Generate CRD manifests
 manifests: $(BUILD_DIR)/kubernetes-split-yaml $(BUILD_DIR)/kustomize


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index ccc44aef7..cc16f3a45 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -95,6 +95,11 @@ jobs:
           $RUN git config --global --add safe.directory /vagrant
       - name: Run E2E tests
         run: $RUN hack/ci/e2e-fedora.sh
+      - name: Run Flaky E2E tests
+        continue-on-error: true
+        run: $RUN hack/ci/e2e-fedora.sh
+        env:
+          E2E_TEST_FLAKY_TESTS_ONLY: true
       - name: Print generated RBAC rules
         run: $RUN hack/ci/print-rbac.sh
 
@@ -128,6 +133,11 @@ jobs:
           $RUN git config --global --add safe.directory /vagrant
       - name: Run E2E tests
         run: $RUN hack/ci/e2e-ubuntu.sh
+      - name: Run Flaky E2E tests
+        continue-on-error: true
+        run: $RUN hack/ci/e2e-ubuntu.sh
+        env:
+          E2E_TEST_FLAKY_TESTS_ONLY: true
 
   e2e-flatcar:
    needs: image
@@ -137,31 +147,36 @@ jobs:
    env:
      RUN: ./hack/ci/run-flatcar.sh
    steps:
-     - uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
-     - name: Vagrant box version
-       id: vagrant-box
-       run: |
-         echo "::set-output name=version::$(curl -s  https://stable.release.flatcar-linux.net/amd64-usr/current/flatcar_production_vagrant.json | jq '.versions[0].version' | tr -d '".')"
-       shell: bash
-     - uses: actions/cache@a7c34adf76222e77931dedbf4a45b2e4648ced19
-       with:
-         path: |
-           ~/.vagrant.d/boxes
-         key: e2e-flatcar-${{ steps.vagrant-box.outputs.version }}-${{ hashFiles('hack/ci/Vagrantfile-flatcar') }}
-         restore-keys: e2e-flatcar-${{ steps.vagrant-box.outputs.version }}-
-     - name: Upgrade vagrant box
-       run: |
-         ln -sf hack/ci/Vagrantfile-flatcar Vagrantfile
-         vagrant box update
-     - uses: actions/download-artifact@fb598a63ae348fa914e94cd0ff38f362e927b741
-       with:
-         name: image
-         path: .
-     - name: Boot Virtual Machine
-       run: make vagrant-up-flatcar
-     - name: Show environment information
-       run: |
-         $RUN kubectl wait --for=condition=ready --timeout=600s node localhost
-         $RUN kubectl get nodes -o wide
-     - name: Run E2E tests
-       run: $RUN hack/ci/e2e-flatcar-dev-container.sh
+      - uses: actions/checkout@2541b1294d2704b0964813337f33b291d3f8596b
+      - name: Vagrant box version
+        id: vagrant-box
+        run: |
+          echo "::set-output name=version::$(curl -s  https://stable.release.flatcar-linux.net/amd64-usr/current/flatcar_production_vagrant.json | jq '.versions[0].version' | tr -d '".')"
+        shell: bash
+      - uses: actions/cache@a7c34adf76222e77931dedbf4a45b2e4648ced19
+        with:
+          path: |
+            ~/.vagrant.d/boxes
+          key: e2e-flatcar-${{ steps.vagrant-box.outputs.version }}-${{ hashFiles('hack/ci/Vagrantfile-flatcar') }}
+          restore-keys: e2e-flatcar-${{ steps.vagrant-box.outputs.version }}-
+      - name: Upgrade vagrant box
+        run: |
+          ln -sf hack/ci/Vagrantfile-flatcar Vagrantfile
+          vagrant box update
+      - uses: actions/download-artifact@fb598a63ae348fa914e94cd0ff38f362e927b741
+        with:
+          name: image
+          path: .
+      - name: Boot Virtual Machine
+        run: make vagrant-up-flatcar
+      - name: Show environment information
+        run: |
+          $RUN kubectl wait --for=condition=ready --timeout=600s node localhost
+          $RUN kubectl get nodes -o wide
+      - name: Run E2E tests
+        run: $RUN hack/ci/e2e-flatcar-dev-container.sh
+      - name: Run Flaky E2E tests
+        continue-on-error: true
+        run: $RUN hack/ci/e2e-ubuntu.sh
+        env:
+          E2E_TEST_FLAKY_TESTS_ONLY: true
diff --git a/hack/ci/e2e-fedora.sh b/hack/ci/e2e-fedora.sh
index bc04bff8f..31645b78a 100755
--- a/hack/ci/e2e-fedora.sh
+++ b/hack/ci/e2e-fedora.sh
@@ -21,9 +21,14 @@ export E2E_TEST_LOG_ENRICHER=true
 export E2E_TEST_LABEL_POD_DENIALS=true
 export E2E_TEST_BPF_RECORDER=true
 export E2E_TEST_PROFILE_RECORDING=true
+export E2E_TEST_FLAKY_TESTS_ONLY=${E2E_TEST_FLAKY_TESTS_ONLY:-false}
 
 # These are already tested in the standard e2e test.
 # No need to test them here.
 export E2E_TEST_SECCOMP=false
 
-make test-e2e
+if "${E2E_TEST_FLAKY_TESTS_ONLY}"; then
+    make test-flaky-e2e
+else
+    make test-e2e
+fi
diff --git a/hack/ci/e2e-flatcar.sh b/hack/ci/e2e-flatcar.sh
index ecb15344f..246888313 100755
--- a/hack/ci/e2e-flatcar.sh
+++ b/hack/ci/e2e-flatcar.sh
@@ -22,6 +22,7 @@ export E2E_TEST_SELINUX=false
 export E2E_TEST_LOG_ENRICHER=false
 export E2E_TEST_BPF_RECORDER=true
 export E2E_TEST_PROFILE_RECORDING=false
+export E2E_TEST_FLAKY_TESTS_ONLY=${E2E_TEST_FLAKY_TESTS_ONLY:-false}
 
 export HOSTFS_DEV_MOUNT_PATH="/hostfs"
 export NODE_ROOTFS_PREFIX=$HOSTFS_DEV_MOUNT_PATH
@@ -38,4 +39,8 @@ export PATH="$HOSTFS_DEV_MOUNT_PATH/opt/bin:$PATH"
 export KUBECONFIG=$HOSTFS_DEV_MOUNT_PATH/etc/kubernetes/admin.conf
 alias k=kubectl
 
-make test-e2e
+if "${E2E_TEST_FLAKY_TESTS_ONLY}"; then
+    make test-flaky-e2e
+else
+    make test-e2e
+fi
diff --git a/hack/ci/e2e-ubuntu.sh b/hack/ci/e2e-ubuntu.sh
index 580aa8ac6..a09813ccd 100755
--- a/hack/ci/e2e-ubuntu.sh
+++ b/hack/ci/e2e-ubuntu.sh
@@ -18,5 +18,10 @@ set -euo pipefail
 export E2E_CLUSTER_TYPE=vanilla
 export E2E_TEST_LOG_ENRICHER=true
 export E2E_TEST_SECCOMP=false
+export E2E_TEST_FLAKY_TESTS_ONLY=${E2E_TEST_FLAKY_TESTS_ONLY:-false}
 
-make test-e2e
+if "${E2E_TEST_FLAKY_TESTS_ONLY}"; then
+    make test-flaky-e2e
+else
+    make test-e2e
+fi
diff --git a/test/e2e_flaky_test.go b/test/e2e_flaky_test.go
new file mode 100644
index 000000000..9e2c479f5
--- /dev/null
+++ b/test/e2e_flaky_test.go
@@ -0,0 +1,79 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package e2e_test
+
+func (e *e2e) TestSecurityProfilesOperator_Flaky() {
+	if e.skipFlakyTests {
+		e.T().Skip("Skipping flaky tests")
+		return
+	}
+
+	e.waitForReadyPods()
+
+	// Deploy prerequisites
+	e.deployCertManager()
+
+	// Deploy the operator
+	e.deployOperator(e.operatorManifest)
+
+	// Retrieve the inputs for the test cases
+	nodes := e.getWorkerNodes()
+
+	// Execute the test cases. Each test case should cleanup on its own and
+	// leave a working operator behind.
+	e.logf("testing cluster-wide operator")
+	testCases := []testCase{
+		{
+			"Seccomp: Metrics",
+			e.testCaseSeccompMetrics,
+		},
+	}
+	for _, testCase := range testCases {
+		tc := testCase
+		e.Run("cluster-wide: "+tc.description, func() {
+			tc.fn(nodes)
+		})
+	}
+
+	// TODO(jaosorior): Re-introduce this to the namespaced tests once we
+	// fix the issue with the certs.
+	e.Run("cluster-wide: Seccomp: Verify profile binding", func() {
+		e.testCaseSeccompProfileBinding(nodes)
+	})
+
+	e.Run("cluster-wide: Seccomp: Verify profile recording logs", func() {
+		e.testCaseProfileRecordingStaticPodLogs()
+		e.testCaseProfileRecordingMultiContainerLogs()
+		e.testCaseProfileRecordingSpecificContainerLogs()
+		e.testCaseProfileRecordingDeploymentLogs()
+	})
+
+	e.Run("cluster-wide: Seccomp: Verify profile recording bpf", func() {
+		e.testCaseBpfRecorderKubectlRun()
+		e.testCaseBpfRecorderStaticPod()
+		e.testCaseBpfRecorderMultiContainer()
+		e.testCaseBpfRecorderDeployment()
+		e.testCaseBpfRecorderParallel()
+		e.testCaseBpfRecorderSelectContainer()
+	})
+
+	// Clean up cluster-wide deployment to prepare for namespace deployment
+	e.cleanupOperator(e.operatorManifest)
+	e.run("git", "checkout", e.operatorManifest)
+
+	e.testNamespacedOperator(namespaceManifest, testNamespace, testCases, nodes)
+}
diff --git a/test/e2e_test.go b/test/e2e_test.go
index 1b4f45662..f11220d13 100644
--- a/test/e2e_test.go
+++ b/test/e2e_test.go
@@ -88,10 +88,6 @@ func (e *e2e) TestSecurityProfilesOperator() {
 			"Seccomp: Delete profiles",
 			e.testCaseDeleteProfiles,
 		},
-		{
-			"Seccomp: Metrics",
-			e.testCaseSeccompMetrics,
-		},
 		{
 			"Seccomp: Re-deploy the operator",
 			e.testCaseReDeployOperator,
@@ -132,12 +128,6 @@ func (e *e2e) TestSecurityProfilesOperator() {
 		})
 	}
 
-	// TODO(jaosorior): Re-introduce this to the namespaced tests once we
-	// fix the issue with the certs.
-	e.Run("cluster-wide: Seccomp: Verify profile binding", func() {
-		e.testCaseSeccompProfileBinding(nodes)
-	})
-
 	e.Run("cluster-wide: Selinux: Verify profile binding", func() {
 		e.testCaseSelinuxProfileBinding(nodes)
 	})
@@ -149,28 +139,12 @@ func (e *e2e) TestSecurityProfilesOperator() {
 		e.testCaseProfileRecordingDeploymentHook()
 	})
 
-	e.Run("cluster-wide: Seccomp: Verify profile recording logs", func() {
-		e.testCaseProfileRecordingStaticPodLogs()
-		e.testCaseProfileRecordingMultiContainerLogs()
-		e.testCaseProfileRecordingSpecificContainerLogs()
-		e.testCaseProfileRecordingDeploymentLogs()
-	})
-
 	e.Run("cluster-wide: Selinux: Verify SELinux profile recording logs", func() {
 		e.testCaseProfileRecordingStaticPodSELinuxLogs()
 		e.testCaseProfileRecordingMultiContainerSELinuxLogs()
 		e.testCaseProfileRecordingSelinuxDeploymentLogs()
 	})
 
-	e.Run("cluster-wide: Seccomp: Verify profile recording bpf", func() {
-		e.testCaseBpfRecorderKubectlRun()
-		e.testCaseBpfRecorderStaticPod()
-		e.testCaseBpfRecorderMultiContainer()
-		e.testCaseBpfRecorderDeployment()
-		e.testCaseBpfRecorderParallel()
-		e.testCaseBpfRecorderSelectContainer()
-	})
-
 	e.Run("cluster-wide: Same profile in multiple namespaces", func() {
 		e.testCaseSameProfileMultipleNs()
 	})
@@ -188,9 +162,12 @@ func (e *e2e) testNamespacedOperator(manifest, namespace string, testCases []tes
 	}
 
 	e.logf("testing namespace operator")
-
-	// Use namespace manifests for redeploy test
-	testCases[5].fn = e.testCaseReDeployNamespaceOperator
+	for _, tc := range testCases {
+		// Replace re-deploy the operator with a namespaced alternative.
+		if tc.description == "Seccomp: Re-deploy the operator" {
+			tc.fn = e.testCaseReDeployNamespaceOperator
+		}
+	}
 
 	// Deploy the namespace operator
 	e.kubectl("create", "namespace", namespace)
@@ -359,8 +336,10 @@ func (e *e2e) getSeccompProfileNodeStatus(
 	)
 	secpolNodeStatusList := &secprofnodestatusv1alpha1.SecurityProfileNodeStatusList{}
 	e.Nil(json.Unmarshal([]byte(seccompProfileNodeStatusJSON), secpolNodeStatusList))
-	e.Equal(len(secpolNodeStatusList.Items), 1)
-	return &secpolNodeStatusList.Items[0]
+	if e.Equal(len(secpolNodeStatusList.Items), 1) {
+		return &secpolNodeStatusList.Items[0]
+	}
+	return nil
 }
 
 func (e *e2e) getAllSeccompProfileNodeStatuses(
diff --git a/test/suite_test.go b/test/suite_test.go
index b60364a7d..20f65ab1e 100644
--- a/test/suite_test.go
+++ b/test/suite_test.go
@@ -50,6 +50,7 @@ var (
 	envSkipBuildImages              = os.Getenv("E2E_SKIP_BUILD_IMAGES")
 	envTestImage                    = os.Getenv("E2E_SPO_IMAGE")
 	spodConfig                      = os.Getenv("E2E_SPOD_CONFIG")
+	envSkipFlakyTests               = os.Getenv("E2E_SKIP_FLAKY_TESTS")
 	envSkipNamespacedTests          = os.Getenv("E2E_SKIP_NAMESPACED_TESTS")
 	envSelinuxTestsEnabled          = os.Getenv("E2E_TEST_SELINUX")
 	envLogEnricherTestsEnabled      = os.Getenv("E2E_TEST_LOG_ENRICHER")
@@ -92,6 +93,7 @@ type e2e struct {
 	testProfileRecording  bool
 	bpfRecorderEnabled    bool
 	skipNamespacedTests   bool
+	skipFlakyTests        bool
 	testWebhookConfig     bool
 	singleNodeEnvironment bool
 	logger                logr.Logger
@@ -157,6 +159,11 @@ func TestSuite(t *testing.T) {
 		skipNamespacedTests = false
 	}
 
+	skipFlakyTests, err := strconv.ParseBool(envSkipFlakyTests)
+	if err != nil {
+		skipFlakyTests = true
+	}
+
 	testWebhookConfig, err := strconv.ParseBool(envWebhookConfigTestsEnabled)
 	if err != nil {
 		testWebhookConfig = true
@@ -189,6 +196,7 @@ func TestSuite(t *testing.T) {
 				skipNamespacedTests:  skipNamespacedTests,
 				operatorManifest:     operatorManifest,
 				testWebhookConfig:    testWebhookConfig,
+				skipFlakyTests:       skipFlakyTests,
 			},
 			"", "",
 		})
@@ -220,6 +228,7 @@ func TestSuite(t *testing.T) {
 				skipNamespacedTests:  skipNamespacedTests,
 				operatorManifest:     operatorManifest,
 				testWebhookConfig:    testWebhookConfig,
+				skipFlakyTests:       skipFlakyTests,
 			},
 			skipBuildImages,
 			skipPushImages,
@@ -246,6 +255,7 @@ func TestSuite(t *testing.T) {
 				skipNamespacedTests:  skipNamespacedTests,
 				operatorManifest:     operatorManifest,
 				testWebhookConfig:    testWebhookConfig,
+				skipFlakyTests:       skipFlakyTests,
 				// NOTE(jaosorior): Our current vanilla jobs are
 				// single-node only.
 				singleNodeEnvironment: true,
diff --git a/test/tc_allowed_syscalls_test.go b/test/tc_allowed_syscalls_test.go
index 2d2aa59fb..4a4cb568a 100644
--- a/test/tc_allowed_syscalls_test.go
+++ b/test/tc_allowed_syscalls_test.go
@@ -80,7 +80,9 @@ func (e *e2e) testCaseAllowedSyscallsValidation(nodes []string) {
 			e.verifyCRDProfileContent(node, sp)
 
 			spns := e.getSeccompProfileNodeStatus(name, namespace, node)
-			e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+			if e.NotNil(spns) {
+				e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+			}
 		}
 
 		for _, name := range deniedProfileNames {
@@ -119,7 +121,9 @@ func (e *e2e) testCaseAllowedSyscallsChange(nodes []string) {
 		e.verifyCRDProfileContent(node, sp)
 
 		spns := e.getSeccompProfileNodeStatus(name, namespace, node)
-		e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+		if e.NotNil(spns) {
+			e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+		}
 	}
 
 	// Remove a syscall form allowed syscall list in order to invalidate the seccomp profile. The operator
diff --git a/test/tc_default_profiles_test.go b/test/tc_default_profiles_test.go
index 84f678854..1904119bc 100644
--- a/test/tc_default_profiles_test.go
+++ b/test/tc_default_profiles_test.go
@@ -68,7 +68,9 @@ func (e *e2e) testCaseDefaultAndExampleProfiles(nodes []string) {
 			e.verifyCRDProfileContent(node, sp)
 
 			spns := e.getSeccompProfileNodeStatus(name, namespace, node)
-			e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+			if e.NotNil(spns) {
+				e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+			}
 		}
 
 		// Example profile verification
@@ -83,7 +85,9 @@ func (e *e2e) testCaseDefaultAndExampleProfiles(nodes []string) {
 			e.verifyCRDProfileContent(node, sp)
 
 			spns := e.getSeccompProfileNodeStatus(name, namespace, node)
-			e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+			if e.NotNil(spns) {
+				e.Equal(spns.Status, secprofnodestatusv1alpha1.ProfileStateInstalled)
+			}
 		}
 	}
 }