Use generated namespaces in e2e tests #3042

awgreene · 2023-09-21T21:53:36Z

This commit updates the OLM e2e test framework to create the test namespace if it does not already exist.

openshift-ci · 2023-09-21T21:53:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [awgreene]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

awgreene · 2023-09-22T13:19:50Z

Makefile

@@ -139,7 +139,7 @@ endif
 E2E_OPTS ?= $(if $(E2E_SEED),-seed '$(E2E_SEED)') $(if $(SKIP), -skip '$(SKIP)') $(if $(TEST),-focus '$(TEST)') $(if $(ARTIFACT_DIR), -output-dir $(ARTIFACT_DIR) -junit-report junit_e2e.xml) -flake-attempts $(E2E_FLAKE_ATTEMPTS) -nodes $(E2E_NODES) -timeout $(E2E_TIMEOUT) -v -randomize-suites -race -trace -progress
 E2E_INSTALL_NS ?= operator-lifecycle-manager
 E2E_CATALOG_NS ?= $(E2E_INSTALL_NS)
-E2E_TEST_NS ?= operators
+E2E_TEST_NS ?= nonexistent-namespace


This isn't technically necessary, but it allows us to ensure that the e2e tests work when a namespace is specified that doesn't exist.

In kcp, we generated unique names for our outermost container in all tests (which was a kcp workspace, but the equivalent here would be a namespace). How would you feel about every standalone test use its own dedicated unique namespace name?

I'm not opposed to that option, would we want to deprecate the ability to specify a testNamespace or update that feature to override those generated namespaces? Would we rather do this now or in a followup?

Followup is fine if you don't want to do it now, but I don't see any utility in supporting specifying a test namespace name.

The only reason it would be nice to allow specifying it is if you think that someone who is working on the test could hot-loop the test process and bypass whatever setup is needed by providing an existing namespace.

I like Steve's proposal, but I'm concerned about scope creep for the PR. How would we feel about focusing this PR on using generated namespaces for e2e tests. To do so we would:

Drop the first commit (which includes this change) from the PR

Create an issue to update how e2e tests consume the testNamespace parameter.

Not saying it's needed or useful, just pointing out that it's the only case where it might be useful.

Removed the first commit.

awgreene · 2023-09-22T13:21:03Z

test/e2e/e2e_test.go

 	var groups operatorsv1.OperatorGroupList
 	Expect(ctx.Ctx().Client().List(context.Background(), &groups, client.InNamespace(testNamespace))).To(Succeed())
 	if len(groups.Items) == 0 {
 		og := operatorsv1.OperatorGroup{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      "opgroup",
+				Name:      "global-operators",


Some of the tests expect the default operatorGroup in the testNamespace, so I've changed the name here. Alternatively, I could write the tests to use any operatorGroup in the namespace, but there are some assumptions about the default scope of that operatorGroup.

awgreene · 2023-09-22T13:24:35Z

test/e2e/util/e2e_client.go

@@ -59,6 +59,11 @@ func (m *E2EKubeClient) Reset() error {
 			break
 		}

+		// Allow resources to opt-out of cleanup at the end of the test.


Suggested change

// Allow resources to opt-out of cleanup at the end of the test.

// Allow resources to opt-out of being deleted by the e2e client.

In my recent work on e2es I've added $SKIP_CLEANUP checks to stop cleaning up - would that approach work here?

Good idea! If we decide to split this out as proposed here I'll share this idea in the ticket.

stevekuznetsov · 2023-09-26T14:41:23Z

test/e2e/csv_e2e_test.go

@@ -401,6 +401,10 @@ var _ = Describe("ClusterServiceVersion", func() {
 	})

 	Context("AllNamespaces OperatorGroup", func() {
+		BeforeEach(func() {
+			nsName := genName("csv-e2e-")
+			generatedNamespace = SetupGeneratedTestNamespace(nsName)


Why is this valid? Were the tests reusing one namespace in the past?

This is actually a no-op change, so I'm going to revert it with the exception of changing ns = to generatedNamespace =

stevekuznetsov · 2023-09-26T14:42:23Z

test/e2e/installplan_e2e_test.go

@@ -3320,15 +3320,6 @@ var _ = Describe("Install Plan", func() {
 	It("compresses installplan step resource manifests to configmap references", func() {
 		// Test ensures that all steps for index-based catalogs are references to configmaps. This avoids the problem
 		// of installplans growing beyond the etcd size limit when manifests are written to the ip status.
-
-		og := &operatorsv1.OperatorGroup{}


It's not clear why this block was removed?

I removed this as this code will create a fresh namespace with an operator group in it.

stevekuznetsov · 2023-09-26T14:44:02Z

test/e2e/installplan_e2e_test.go

-		}
-		_, err = crc.OperatorsV1().OperatorGroups(generatedNamespace.GetName()).Create(context.Background(), og, metav1.CreateOptions{})
-		Expect(err).ToNot(HaveOccurred())
+		// Update the existing OG to use the ServiceAccount


(just reviewing the last commit) really not clear where the existing OG comes from - why is it shared? Can it be test-local?

This line is ran before each it, which creates the namespace and operatorGroup, so we don't need to create new resources here.

stevekuznetsov · 2023-09-26T14:44:19Z

test/e2e/packagemanifest_e2e_test.go

-		BeforeEach(func() {
-			generatedNamespace = SetupGeneratedTestNamespace(genName("package-manifest-e2e"))
-		})
+		generatedNamespace = SetupGeneratedTestNamespace(genName("package-manifest-e2e"))


Why do we want one ns for all of them vs one per test?

Hmm I might be missing what you're trying to point out, could you describe what you mean by "all of them" vs "one per test"? Running the tests in this describe block shows that multiple namespaces are generated, specifically one for each test.

$ k get ns -w ... ... package-manifest-e2ect2wt Active 0s package-manifest-e2ehsr64 Active 0s package-manifest-e2erptpp Active 0s package-manifest-e2edgq5m Active 0s

I assumed that when you bubbled up from a BeforeEach() to a larger scope, the number of times that this line was called was reduced?

Silly Pseudo Code used to explain what is happening:

Describe("e2e suite")...{ BeforeEach{ fmt.Printf("Before\n) } AfterEach{ fmt.Printf("After\n) } It(1)...{ fmt.Printf("Test 1\n) } It(2)...{ fmt.Printf("Test 2\n) } }

If you run the tests above, you'd see:

Before

Test1

After

Before

Test2

After

This commit updates all of OLM e2e test suites to generate a namespace rather than using a hard coded test namespace passed into the test as a parameter. The motivation for this change include: 1. Decreasing the opportunity for tests to influence each other. Tests are not guaranteed to cleanup after themselves and may influence the results of other tests. 2. Some distributions of OLM run alongside other controllers which may revert changes to resources in predetermined namespaces. Signed-off-by: Alexander Greene <[email protected]>

awgreene · 2023-09-26T21:13:45Z

[FAIL] Fail Forward Upgrades when a CSV resource is in a failed state [It] eventually reports a successful state when using replaces

Worked locally, retesting

awgreene · 2023-09-27T02:50:56Z

[FAIL] Fail Forward Upgrades when a CSV resource is in a failed state [It] eventually reports a successful state when using skip ranges

New failure that didn't happen locally, retesting.

stevekuznetsov · 2023-09-27T12:43:52Z

@awgreene both of those are the known flakes that we tracked down, can sync on Slack for them but just re-test your heart away for now

ncdc · 2023-09-27T15:58:50Z

test/e2e/catalog_e2e_test.go

@@ -49,7 +49,7 @@ const (

 var _ = Describe("Starting CatalogSource e2e tests", func() {
 	var (
-		ns                  corev1.Namespace
+		generatedNamespace  corev1.Namespace


note for future reference: it is easier to review without a change like this. or potentially consider putting the rename in its own commit. in this particular case, i would have left the variable name at ns

awgreene · 2023-09-27T21:22:50Z

/hold, waiting on #3028

This commit reintroduces test documentation accidentally removed in operator-framework#3042 Signed-off-by: Alexander Greene <[email protected]>

This commit reintroduces test documentation accidentally removed in #3042 Signed-off-by: Alexander Greene <[email protected]>

This commit reintroduces test documentation accidentally removed in operator-framework/operator-lifecycle-manager#3042 Signed-off-by: Alexander Greene <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 543167908548535a76635659659b42272a949adc

This commit reintroduces test documentation accidentally removed in operator-framework/operator-lifecycle-manager#3042 Signed-off-by: Alexander Greene <[email protected]> Signed-off-by: Todd Short <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 543167908548535a76635659659b42272a949adc (cherry picked from commit 6eb08f5)

This commit reintroduces test documentation accidentally removed in operator-framework/operator-lifecycle-manager#3042 Signed-off-by: Alexander Greene <[email protected]> Signed-off-by: Todd Short <[email protected]> Upstream-repository: operator-lifecycle-manager Upstream-commit: 543167908548535a76635659659b42272a949adc (cherry picked from commit 6eb08f5) (cherry picked from commit 947d546)

openshift-ci bot requested review from ankitathomas and dinhxuanvu September 21, 2023 21:53

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023

awgreene force-pushed the fix-test-namspace-dne branch 3 times, most recently from ff1b8c2 to 303d610 Compare September 22, 2023 13:15

awgreene commented Sep 22, 2023

View reviewed changes

awgreene force-pushed the fix-test-namspace-dne branch from 303d610 to 3caed49 Compare September 22, 2023 13:22

awgreene commented Sep 22, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2023

awgreene force-pushed the fix-test-namspace-dne branch 2 times, most recently from 033b09f to 009b8dc Compare September 22, 2023 19:15

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2023

stevekuznetsov reviewed Sep 26, 2023

View reviewed changes

awgreene force-pushed the fix-test-namspace-dne branch from 72e569e to 6913daf Compare September 26, 2023 16:58

awgreene force-pushed the fix-test-namspace-dne branch from 6913daf to ba8c626 Compare September 26, 2023 16:59

awgreene changed the title ~~Update e2e to create test namespace if it does not exist~~ Use generated namespaces in e2e tests Sep 26, 2023

ncdc reviewed Sep 27, 2023

View reviewed changes

stevekuznetsov added this pull request to the merge queue Oct 4, 2023

Merged via the queue into operator-framework:master with commit 108d6db Oct 4, 2023

awgreene mentioned this pull request Oct 25, 2023

update 'CSV copy watching all namespaces' case #2946

Closed

awgreene added a commit to awgreene/operator-lifecycle-manager that referenced this pull request Nov 20, 2023

Add test comment back

69f802f

This commit reintroduces test documentation accidentally removed in operator-framework#3042 Signed-off-by: Alexander Greene <[email protected]>

awgreene mentioned this pull request Nov 20, 2023

Add test comment back #3118

Merged

11 tasks

github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2023

Add test comment back

5431679

This commit reintroduces test documentation accidentally removed in #3042 Signed-off-by: Alexander Greene <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use generated namespaces in e2e tests #3042

Use generated namespaces in e2e tests #3042

awgreene commented Sep 21, 2023

openshift-ci bot commented Sep 21, 2023

awgreene Sep 22, 2023

ncdc Sep 22, 2023

awgreene Sep 22, 2023

ncdc Sep 22, 2023

stevekuznetsov Sep 22, 2023

awgreene Sep 26, 2023

stevekuznetsov Sep 26, 2023

awgreene Sep 26, 2023

awgreene Sep 22, 2023

awgreene Sep 22, 2023

stevekuznetsov Sep 22, 2023

awgreene Sep 26, 2023

stevekuznetsov Sep 26, 2023

awgreene Sep 26, 2023

stevekuznetsov Sep 26, 2023

awgreene Sep 26, 2023

stevekuznetsov Sep 26, 2023

awgreene Sep 26, 2023

stevekuznetsov Sep 26, 2023

awgreene Sep 26, 2023

stevekuznetsov Sep 26, 2023

awgreene Sep 26, 2023

awgreene commented Sep 26, 2023

awgreene commented Sep 27, 2023

stevekuznetsov commented Sep 27, 2023

ncdc Sep 27, 2023

awgreene commented Sep 27, 2023

	// Allow resources to opt-out of cleanup at the end of the test.
	// Allow resources to opt-out of being deleted by the e2e client.

Use generated namespaces in e2e tests #3042

Use generated namespaces in e2e tests #3042

Conversation

awgreene commented Sep 21, 2023

openshift-ci bot commented Sep 21, 2023

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awgreene commented Sep 26, 2023

awgreene commented Sep 27, 2023

stevekuznetsov commented Sep 27, 2023

Choose a reason for hiding this comment

awgreene commented Sep 27, 2023