Create paired upgrade tests #8158

mgencur · 2024-08-14T09:21:57Z

Related to knative-extensions/eventing-kafka-broker#4047

This brings the building blocks for creating paired upgrade tests in other repositories like eventing-kafka-broker.

Proposed Changes

Introduce type DurableFeature which holds two interconnected features: SetupF, VerifyF. This allows re-using existing Features created for reconciler-test framework, with slight modifications. The original feature should be split into two parts:
1. Setup feature - this feature does all the required setup for the feature, typically the Setup, Prerequisite phases of REKT tests
2. Verify feature - this feature does the verification, typically the Requirement and Assert phases of the REKT tests
The DurableFeature also holds the environment that was created at the beginning and thus allows re-using the same namespace (and resources) after upgrade/downgrade.
There are also a few decorators that will allow different types of tests.
- NewFeatureSmoke - this will create an object that will run same tests before upgrade, after upgrade, after downgrade. This will always include creating new resources (Setup), verifying resources (Assert), and deleting namespace (Teardown).
- NewFeatureOnlyUpgrade - will create resources before upgrade, verify and tear down after upgrade, there's no operation after downgrade.
- NewFeatureOnlyDowngrade - will create resources after upgrade, verify and tear down after downgrade, there's no operation at the beginning before upgrade.
- NewFeatureBothUpgradeDowngrade - this will create resources before upgrade, verify them after upgrade, and verify and tear down after downgrade.
There's also the type FeatureGroupWithUpgradeTests which just aggregates operations across a slice of features.

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Docs

codecov · 2024-08-14T09:31:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.45%. Comparing base (71d5d5f) to head (f430906).
Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8158      +/-   ##
==========================================
- Coverage   67.71%   67.45%   -0.26%     
==========================================
  Files         370      371       +1     
  Lines       17946    18039      +93     
==========================================
+ Hits        12152    12169      +17     
- Misses       5020     5091      +71     
- Partials      774      779       +5

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

mgencur · 2024-08-15T07:38:36Z

/test upgrade-tests

mgencur · 2024-08-15T08:32:34Z

Hmm, the eventing-webhook doesn't start after downgrade, I see these events:

knative-eventing   11m         Warning   Unhealthy                                pod/eventing-webhook-5c974857b-jjkjl                                           Readiness probe failed: Get "https://10.68.0.8:8443/": remote error: tls: unrecognized name
knative-eventing   6m56s       Normal    Killing                                  pod/eventing-webhook-5c974857b-jjkjl                                           Stopping container eventing-webhook
knative-eventing   6m38s       Warning   Unhealthy                                pod/eventing-webhook-5c974857b-jjkjl                                           Readiness probe failed: HTTP probe failed with statuscode: 503

mgencur · 2024-08-15T11:23:42Z

/test upgrade-tests

mgencur · 2024-08-15T12:58:43Z

The failures are caused by ContainerSource being deployed which brings SinkBinding and the test scripts try to scale down to 0 and back to 3, see #8161

mgencur · 2024-08-16T08:52:17Z

Added a workaround for #8161 which makes the tests pass.

Cali0707 · 2024-08-21T13:28:51Z

/assign

Cali0707

This seems like a huge upgrade on the existing upgrade tests. Maybe once we have this in eventing and are happy with the API we can also look into adding this into reconciler-test?

test/upgrade/upgrade.go

mgencur · 2024-08-22T05:51:54Z

This seems like a huge upgrade on the existing upgrade tests. Maybe once we have this in eventing and are happy with the API we can also look into adding this into reconciler-test?

Hmm. Possibly we could add it to reconciler-test. We could make it more configurable, for example these EnvOpts could be held as a field in DurableFeature so the user could use &DurableFeature{SetupF: setupF, VerifyF: verifyF, Global: glob, EnvOpts: myEnvOpts} to initialize them at the beginning according to their needs. Maybe I could do it in this PR already. I mean something like https://gist.github.com/mgencur/a1920035107833de278759fdc67d96de

test/upgrade/upgrade.go

mgencur · 2024-09-06T09:37:34Z

We could make it more configurable, for example these EnvOpts could be held as a field in DurableFeature

I did that and pushed another commit here.

Cali0707 · 2024-09-09T15:13:23Z

test/e2e-common.sh

+  # Workaround for https://github.com/knative/eventing/issues/8161
+  kubectl label namespace "${SYSTEM_NAMESPACE}" bindings.knative.dev/exclude=true --overwrite


Can we have some tracker to remove this workaround and/or note that this should be cleaned up on #8161 ?

I can add a note on #8161 as soon as this PR is merged. Doing it before it's merged seems too early as I can't properly reference this workaround.

Cali0707 · 2024-09-09T15:16:22Z

test/upgrade/upgrade_test.go

+	restConfig, err := pkgtest.Flags.ClientConfig.GetRESTConfig()
+	if err != nil {
+		log.Fatal("Error building client config: ", err)
+	}
+
+	// Getting the rest config explicitly and passing it further will prevent re-initializing the flagset
+	// in NewStandardGlobalEnvironment(). The upgrade tests use knative.dev/pkg/test which initializes the
+	// flagset as well.
+	global = environment.NewStandardGlobalEnvironment(func(cfg environment.Configuration) environment.Configuration {
+		cfg.Config = restConfig
+		return cfg
+	})


Maybe it is worth making rekt-based upgrade tests rather than adding these rekt tests into the existing upgrade tests? Sort of like #7626 (comment)

cc @pierDipi

The existing upgrade tests are actually based on https://github.com/knative/pkg/tree/main/test/upgrade package. This package is used in nearly all Knative repos - Serving, Eventing, EKB, operator.
My changes bring a glue between these upgrade tests and REKT framework. It is still possible to use the existing upgrade test framework and re-use REKT features that are already defined elsewhere as building blocks. The only necessity is to split the feature into two smaller features (SetupF, VerifyF) so that the upgrade framework can run individual parts as needed.
Regarding #7626 (comment) , I can see that as a suggestion to use REKT-based tests instead of re-using custom legacy code that was implemented earlier. My PR does that - it allows re-using REKT-based tests (features), with minimal changes.
It would be possible to move the types from upgrade.go (DurableFeature, FeatureWithUpgradeTests, etc.) to reconciler-test and be re-used in Eventing, EKB. Right now this code is here in Eventing.

I don't have ideas how to create specific "rekt-based upgrade tests" without re-implementing a lot of stuff from knative/pkg.

Cali0707

/lgtm

knative-prow · 2024-09-10T09:51:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, mgencur

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:

~~test/OWNERS~~ [Cali0707]

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

mgencur · 2024-09-10T14:32:18Z

/retest-required

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 14, 2024

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2024

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 14, 2024

knative-prow bot requested review from evankanderson and matzew August 14, 2024 09:22

knative-prow bot added the area/test-and-release Test infrastructure, tests or release label Aug 14, 2024

Re-use channel.ChannelChain reconciler-test feature in upgrade tests

174bef9

mgencur force-pushed the paired_upgrade_tests_main branch from cd0052c to 174bef9 Compare August 14, 2024 09:25

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2024

mgencur added 7 commits August 14, 2024 13:30

Fix glob -> global

2e2c82a

Remove redundant feature name from test name

cd83115

Remove Setup/Verify from test name

0e5e4c5

Use slices.Concat from Golang 1.22

09c64d9

Include setupEnv and setupCtx in DurableFeature

ce075ed

Remove most pointers

18b9f9f

Refactor and use FeatureGroupWithUpgradeTests

999379b

mgencur mentioned this pull request Aug 15, 2024

[TEST ONLY] Checking post-downgrade failure in eventing-webhook #8160

Closed

5 tasks

mgencur mentioned this pull request Aug 15, 2024

eventing-webhook mutates itself when SinkBinding is present #8161

Open

Try workaround for issues/8161

6e2d381

mgencur force-pushed the paired_upgrade_tests_main branch from 7297183 to 6e2d381 Compare August 16, 2024 07:37

mgencur changed the title ~~[WIP] Create paired upgrade tests~~ Create paired upgrade tests Aug 16, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2024

knative-prow bot assigned Cali0707 Aug 21, 2024

Cali0707 reviewed Aug 21, 2024

View reviewed changes

test/upgrade/upgrade.go Outdated Show resolved Hide resolved

test/upgrade/upgrade.go Outdated Show resolved Hide resolved

Renames

b762deb

mgencur commented Aug 22, 2024

View reviewed changes

test/upgrade/upgrade.go Show resolved Hide resolved

Expose EnvOpts and make DurableFeature more configurable

f430906

Cali0707 reviewed Sep 9, 2024

View reviewed changes

Cali0707 approved these changes Sep 10, 2024

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2024

knative-prow bot merged commit 229446d into knative:main Sep 10, 2024
31 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create paired upgrade tests #8158

Create paired upgrade tests #8158

mgencur commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

mgencur commented Aug 15, 2024

mgencur commented Aug 15, 2024

mgencur commented Aug 15, 2024

mgencur commented Aug 15, 2024 •

edited

Loading

mgencur commented Aug 16, 2024

Cali0707 commented Aug 21, 2024

Cali0707 left a comment

mgencur commented Aug 22, 2024 •

edited

Loading

mgencur commented Sep 6, 2024

Cali0707 Sep 9, 2024

mgencur Sep 10, 2024

Cali0707 Sep 9, 2024

mgencur Sep 10, 2024 •

edited

Loading

Cali0707 left a comment

knative-prow bot commented Sep 10, 2024

mgencur commented Sep 10, 2024

		# Workaround for https://github.com/knative/eventing/issues/8161
		kubectl label namespace "${SYSTEM_NAMESPACE}" bindings.knative.dev/exclude=true --overwrite

Create paired upgrade tests #8158

Create paired upgrade tests #8158

Conversation

mgencur commented Aug 14, 2024 • edited Loading

Proposed Changes

Pre-review Checklist

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

mgencur commented Aug 15, 2024

mgencur commented Aug 15, 2024

mgencur commented Aug 15, 2024

mgencur commented Aug 15, 2024 • edited Loading

mgencur commented Aug 16, 2024

Cali0707 commented Aug 21, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

mgencur commented Aug 22, 2024 • edited Loading

mgencur commented Sep 6, 2024

Cali0707 Sep 9, 2024

Choose a reason for hiding this comment

mgencur Sep 10, 2024

Choose a reason for hiding this comment

Cali0707 Sep 9, 2024

Choose a reason for hiding this comment

mgencur Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Sep 10, 2024

mgencur commented Sep 10, 2024

mgencur commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

mgencur commented Aug 15, 2024 •

edited

Loading

mgencur commented Aug 22, 2024 •

edited

Loading

mgencur Sep 10, 2024 •

edited

Loading