🐛 mitigate upgrade-e2e test flakiness #1627

perdasilva · 2025-01-15T16:15:35Z

Description

Mitigates the upgrade-e2e test flakiness caused by 🐛 catalog status is not updated during re-unpack after pod reset #1626 by waiting for the catalog's last unpacked to be after the catalogd deployment pod's creation time
Updates the upgrade-e2e test to also wait for the catalogd deployment to be up
Factors out the artifact collection code into a utils package and adds artifact collection to upgrade-e2e

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-01-15T16:15:53Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`a1a9777`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/678a2e03b446e60008b7b419
😎 Deploy Preview	https://deploy-preview-1627--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2025-01-15T16:32:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.72%. Comparing base (c8380d2) to head (a1a9777).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1627      +/-   ##
==========================================
+ Coverage   66.70%   66.72%   +0.02%     
==========================================
  Files          57       57              
  Lines        4595     4595              
==========================================
+ Hits         3065     3066       +1     
+ Misses       1304     1303       -1     
  Partials      226      226

Flag	Coverage Δ
e2e	`53.16% <ø> (-0.09%)`	⬇️
unit	`53.68% <ø> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LalatenduMohanty · 2025-01-15T16:53:35Z

We have #1550 for this issue as well

azych

lgtm

test/upgrade-e2e/post_upgrade_test.go

.github/workflows/e2e.yaml

camilamacedo86

I am OK with the changes
Thank you a lot for this contribution
+1 in the nit raised by @azych
we might want to address it follow and it across the whole project

@perdasilva could you please add in the description that this PR closes: #1550

/lgtm

LalatenduMohanty · 2025-01-16T12:20:20Z

@perdasilva How can I test this PR locally on my laptop? I have local kind setup created usng make run.

LalatenduMohanty · 2025-01-16T12:36:25Z

I deleted the kind setup and $ make test-upgrade-e2e should test this PR.

perdasilva · 2025-01-16T12:37:00Z

@perdasilva How can I test this PR locally on my laptop? I have local kind setup created usng make run.

make test-upgrade-e2e will exercise the upgrade test

if you want to step through it, it also has the recipe for it:

.PHONY: test-upgrade-e2e
test-upgrade-e2e: KIND_CLUSTER_NAME := operator-controller-upgrade-e2e
test-upgrade-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog
test-upgrade-e2e: export TEST_CLUSTER_EXTENSION_NAME := test-package
test-upgrade-e2e: kind-cluster run-latest-release image-registry pre-upgrade-setup docker-build kind-load kind-deploy post-upgrade-checks kind-clean #HELP Run upgrade e2e tests on a local kind cluster

LalatenduMohanty · 2025-01-16T12:56:12Z

I could reproduce the issue without your PR, however when I tested your PR it failed with

=== RUN   TestClusterExtensionAfterOLMUpgrade
    post_upgrade_test.go:31: Starting checks after OLM upgrade
    post_upgrade_test.go:36: Wait for catalogd deployment to be ready
    post_upgrade_test.go:121: Checking that the deployment is updated
    post_upgrade_test.go:137: Waiting for only one Pod to remain
    post_upgrade_test.go:40: Wait for operator-controller deployment to be ready
    post_upgrade_test.go:121: Checking that the deployment is updated
    post_upgrade_test.go:137: Waiting for only one Pod to remain
    post_upgrade_test.go:43: Reading logs to make sure that ClusterExtension was reconciled by operator-controller before we update it
    post_upgrade_test.go:55: Checking that the ClusterCatalog is unpacked
    post_upgrade_test.go:56: 
        	Error Trace:	/home/lmohanty/code/github.com/OLM-Upstream/operator-controller/test/upgrade-e2e/post_upgrade_test.go:62
        	            				/usr/local/go/src/runtime/asm_amd64.s:1700
        	Error:      	Expected nil, but got: &v1.Condition{Type:"Serving", Status:"True", ObservedGeneration:1, LastTransitionTime:time.Date(2025, time.January, 16, 7, 42, 20, 0, time.Local), Reason:"Available", Message:"Serving desired content from resolved source"}
    post_upgrade_test.go:56: 
        	Error Trace:	/home/lmohanty/code/github.com/OLM-Upstream/operator-controller/test/upgrade-e2e/post_upgrade_test.go:72
        	            				/usr/local/go/src/runtime/asm_amd64.s:1700
        	Error:      	Expected nil, but got: &v1.Condition{Type:"Progressing", Status:"True", ObservedGeneration:1, LastTransitionTime:time.Date(2025, time.January, 16, 7, 42, 20, 0, time.Local), Reason:"Succeeded", Message:"Successfully unpacked and stored content from resolved source"}
    post_upgrade_test.go:56: 
        	Error Trace:	/home/lmohanty/code/github.com/OLM-Upstream/operator-controller/test/upgrade-e2e/post_upgrade_test.go:56
        	Error:      	Condition never satisfied
        	Test:       	TestClusterExtensionAfterOLMUpgrade
--- FAIL: TestClusterExtensionAfterOLMUpgrade (66.42s)
FAIL
FAIL	github.com/operator-framework/operator-controller/test/upgrade-e2e	66.450s
FAIL
make: *** [Makefile:228: post-upgrade-checks] Error 1

perdasilva · 2025-01-16T12:58:21Z

I could reproduce the issue without your PR, however when I tested your PR it failed with

=== RUN   TestClusterExtensionAfterOLMUpgrade
    post_upgrade_test.go:31: Starting checks after OLM upgrade
    post_upgrade_test.go:36: Wait for catalogd deployment to be ready
    post_upgrade_test.go:121: Checking that the deployment is updated
    post_upgrade_test.go:137: Waiting for only one Pod to remain
    post_upgrade_test.go:40: Wait for operator-controller deployment to be ready
    post_upgrade_test.go:121: Checking that the deployment is updated
    post_upgrade_test.go:137: Waiting for only one Pod to remain
    post_upgrade_test.go:43: Reading logs to make sure that ClusterExtension was reconciled by operator-controller before we update it
    post_upgrade_test.go:55: Checking that the ClusterCatalog is unpacked
    post_upgrade_test.go:56: 
        	Error Trace:	/home/lmohanty/code/github.com/OLM-Upstream/operator-controller/test/upgrade-e2e/post_upgrade_test.go:62
        	            				/usr/local/go/src/runtime/asm_amd64.s:1700
        	Error:      	Expected nil, but got: &v1.Condition{Type:"Serving", Status:"True", ObservedGeneration:1, LastTransitionTime:time.Date(2025, time.January, 16, 7, 42, 20, 0, time.Local), Reason:"Available", Message:"Serving desired content from resolved source"}
    post_upgrade_test.go:56: 
        	Error Trace:	/home/lmohanty/code/github.com/OLM-Upstream/operator-controller/test/upgrade-e2e/post_upgrade_test.go:72
        	            				/usr/local/go/src/runtime/asm_amd64.s:1700
        	Error:      	Expected nil, but got: &v1.Condition{Type:"Progressing", Status:"True", ObservedGeneration:1, LastTransitionTime:time.Date(2025, time.January, 16, 7, 42, 20, 0, time.Local), Reason:"Succeeded", Message:"Successfully unpacked and stored content from resolved source"}
    post_upgrade_test.go:56: 
        	Error Trace:	/home/lmohanty/code/github.com/OLM-Upstream/operator-controller/test/upgrade-e2e/post_upgrade_test.go:56
        	Error:      	Condition never satisfied
        	Test:       	TestClusterExtensionAfterOLMUpgrade
--- FAIL: TestClusterExtensionAfterOLMUpgrade (66.42s)
FAIL
FAIL	github.com/operator-framework/operator-controller/test/upgrade-e2e	66.450s
FAIL
make: *** [Makefile:228: post-upgrade-checks] Error 1

yeah - seems to be due to some recent changes addressing some reviewer comments. Maybe we're finding out why !assert.NotNil was used...?

perdasilva · 2025-01-16T13:06:20Z

test/upgrade-e2e/post_upgrade_test.go

@@ -101,16 +101,47 @@ func TestClusterExtensionAfterOLMUpgrade(t *testing.T) {
 	require.EventuallyWithT(t, func(ct *assert.CollectT) {
 		assert.NoError(ct, c.Get(ctx, types.NamespacedName{Name: testClusterExtensionName}, &clusterExtension))
 		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled)
-		if !assert.NotNil(ct, cond) {


this will never return false - either it returns true or fails

Yup, I agree.

perdasilva · 2025-01-16T13:08:04Z

@azych I've changed it to assert.NotNil looking at the impl. it seem we either get true or a failure. So, I don't think the if made sense to begin with...?

camilamacedo86 · 2025-01-16T13:47:27Z

Hi @perdasilva,

I liked the other improvements you made. However, regarding the issue, I wanted to clarify something:

It seems like we are now allocating 1 minute for the load and 1 minute for the update. Wouldn’t that effectively result in a total increase of 2 minutes overall? Or did I misunderstand something?

Looking forward to your thoughts!

perdasilva · 2025-01-16T14:20:19Z

Hi @perdasilva,

I liked the other improvements you made. However, regarding the issue, I wanted to clarify something:

It seems like we are now allocating 1 minute for the load and 1 minute for the update. Wouldn’t that effectively result in a total increase of 2 minutes overall? Or did I misunderstand something?

Looking forward to your thoughts!

@camilamacedo86 Do you mean because we now wait for the catalogd deployment?
Aside from that, which I think is warranted after the mono-repo changes, I don't think there's any change in the time allocation itself. What this change does is ensure that the catalog has been unpacked and is serving before we wait for the update. I think this is still has the same intention as the original test. It's just that the guard for unpack would pass really quickly, since the conditions on the catalog don't change from the installation step. Does that make sense?

Anecdotally, looking at a few upgrade-e2e with and without this PR, there doesn't seem to be any meaningful change in runtimes

LalatenduMohanty

/lgtm

LalatenduMohanty · 2025-01-16T14:44:05Z

/hold for the #1627 (comment) . Feel free to remove the hold.

azych · 2025-01-16T15:20:42Z

@azych I've changed it to assert.NotNil looking at the impl. it seem we either get true or a failure. So, I don't think the if made sense to begin with...?

👍 I think your current change makes sense looking at the intention

camilamacedo86 · 2025-01-16T18:07:03Z

HI @LalatenduMohanty

Can we unhold this one?
Can we get it merged?

It is
/lgtm for me !!!

LalatenduMohanty · 2025-01-16T18:15:12Z

/hold cancel

Signed-off-by: Per Goncalves da Silva <[email protected]>

camilamacedo86 · 2025-01-17T11:20:57Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2025

perdasilva changed the title ~~🐛 Mitigate upgrde2e~~ 🐛 mitigate upgrade-e2e test flakiness Jan 15, 2025

perdasilva force-pushed the mitigate-upgrde2e branch from cad9a6d to 9ccf48d Compare January 15, 2025 16:20

perdasilva marked this pull request as ready for review January 15, 2025 16:50

perdasilva requested a review from a team as a code owner January 15, 2025 16:50

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2025

perdasilva mentioned this pull request Jan 15, 2025

Investigate Flakiness in Post-Upgrade Test Timing #1550

Open

azych approved these changes Jan 16, 2025

View reviewed changes

test/upgrade-e2e/post_upgrade_test.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jan 16, 2025

View reviewed changes

.github/workflows/e2e.yaml Show resolved Hide resolved

camilamacedo86 previously approved these changes Jan 16, 2025

View reviewed changes

openshift-ci bot assigned camilamacedo86 Jan 16, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2025

perdasilva dismissed camilamacedo86’s stale review via fb6f8b2 January 16, 2025 12:39

perdasilva force-pushed the mitigate-upgrde2e branch from 9ccf48d to fb6f8b2 Compare January 16, 2025 12:39

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2025

perdasilva force-pushed the mitigate-upgrde2e branch from fb6f8b2 to a8daf69 Compare January 16, 2025 12:47

perdasilva force-pushed the mitigate-upgrde2e branch from a8daf69 to 4eea23c Compare January 16, 2025 13:05

perdasilva commented Jan 16, 2025

View reviewed changes

LalatenduMohanty previously approved these changes Jan 16, 2025

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2025

LalatenduMohanty added this pull request to the merge queue Jan 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025

camilamacedo86 added this pull request to the merge queue Jan 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025

camilamacedo86 added this pull request to the merge queue Jan 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025

camilamacedo86 added this pull request to the merge queue Jan 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025

perdasilva added this pull request to the merge queue Jan 17, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2025

perdasilva force-pushed the mitigate-upgrde2e branch from 4eea23c to a6b249e Compare January 17, 2025 10:13

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2025

Per Goncalves da Silva added 2 commits January 17, 2025 11:16

add artifact collection to upgrade-e2e-tests

0695e25

Signed-off-by: Per Goncalves da Silva <[email protected]>

mitigate upgrade-e2e flakiness

a1a9777

Signed-off-by: Per Goncalves da Silva <[email protected]>

perdasilva dismissed LalatenduMohanty’s stale review via a1a9777 January 17, 2025 10:16

perdasilva force-pushed the mitigate-upgrde2e branch from a6b249e to a1a9777 Compare January 17, 2025 10:16

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2025

camilamacedo86 enabled auto-merge January 17, 2025 11:21

camilamacedo86 approved these changes Jan 17, 2025

View reviewed changes

camilamacedo86 added this pull request to the merge queue Jan 17, 2025

Merged via the queue into operator-framework:main with commit 7446060 Jan 17, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 mitigate upgrade-e2e test flakiness #1627

🐛 mitigate upgrade-e2e test flakiness #1627

perdasilva commented Jan 15, 2025 •

edited

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

LalatenduMohanty commented Jan 15, 2025

azych left a comment

camilamacedo86 left a comment •

edited

Loading

LalatenduMohanty commented Jan 16, 2025

LalatenduMohanty commented Jan 16, 2025

perdasilva commented Jan 16, 2025 •

edited

Loading

LalatenduMohanty commented Jan 16, 2025

perdasilva commented Jan 16, 2025

perdasilva Jan 16, 2025

LalatenduMohanty Jan 16, 2025

perdasilva commented Jan 16, 2025

camilamacedo86 commented Jan 16, 2025

perdasilva commented Jan 16, 2025 •

edited

Loading

LalatenduMohanty left a comment

LalatenduMohanty commented Jan 16, 2025

azych commented Jan 16, 2025

camilamacedo86 commented Jan 16, 2025

LalatenduMohanty commented Jan 16, 2025

camilamacedo86 commented Jan 17, 2025

🐛 mitigate upgrade-e2e test flakiness #1627

🐛 mitigate upgrade-e2e test flakiness #1627

Conversation

perdasilva commented Jan 15, 2025 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jan 15, 2025 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

LalatenduMohanty commented Jan 15, 2025

azych left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

LalatenduMohanty commented Jan 16, 2025

LalatenduMohanty commented Jan 16, 2025

perdasilva commented Jan 16, 2025 • edited Loading

LalatenduMohanty commented Jan 16, 2025

perdasilva commented Jan 16, 2025

perdasilva Jan 16, 2025

Choose a reason for hiding this comment

LalatenduMohanty Jan 16, 2025

Choose a reason for hiding this comment

perdasilva commented Jan 16, 2025

camilamacedo86 commented Jan 16, 2025

perdasilva commented Jan 16, 2025 • edited Loading

LalatenduMohanty left a comment

Choose a reason for hiding this comment

LalatenduMohanty commented Jan 16, 2025

azych commented Jan 16, 2025

camilamacedo86 commented Jan 16, 2025

LalatenduMohanty commented Jan 16, 2025

camilamacedo86 commented Jan 17, 2025

perdasilva commented Jan 15, 2025 •

edited

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

perdasilva commented Jan 16, 2025 •

edited

Loading

perdasilva commented Jan 16, 2025 •

edited

Loading