feat: reusable e2e test setups by dattito · Pull Request #73 · openmcp-project/openmcp-testing

dattito · 2026-05-22T08:50:11Z

What this PR does / why we need it:
This PR adds the possibility to reuse platform-, onboarding- and MCP-clusters created during local e2e tests.
This should enhance the developer experience with faster iteration cycles.

Some design-decisions / information:

This feature should be non-breaking. I say it should, because I had to adjust some functions to be idempotent. E.g. CreateIgnoreAlreadyExists() to CreateOrUpdate(). I don't expect this to be breaking in any setups, but it is not guaranteed. It is however important as my design expects a complete idempotent Setup() of the test-environment and the individual tests (more in the next bullet point)
To keep the logic for this as straight forward as possible, I decided to implement the "reusability" as the absence of a Teardown(). And on the next run, the idempotent Setup() step will just pick up the still-existing cluster, and will directly fulfill it's assess steps.
For Setup() to be idempotent, the initial cluster name of the platform cluster needs to be hardcoded instead of randomly generated as it is now. I set it per default to "e2e-platform", and it is adjustable. Not that this way multiple e2e-tests cannot run in parallel. I don't think that is a use-case anyway.
The shared test-environment setup in main_test.go can use the new "re-use" logic without any code changes. It is activated purely with the E2E_REUSE_CLUSTER=true env variable.
A lot of time is also spent on the Setup() part of the individual tests, as I noticed that often e.g. ManagedControlPlane resources are spawned here (and deleted again in Teardown()). With the correct setup (which most tests should already have, but it is not guaranteed: At the end of the test, the MCP has to be as pure as it was after the creation!), these ManagedControlPlanes are also reusable by just not calling these Teardown() parts when reuse mode is active. However, this has to be implemented in every test itself and cannot be passed down to this library, as it doesn't have control of the Teardown() function here. However, a if !setup.IsReuseMode() { helper has been created to make this easy. An example has been added to the "demo" e2e test of this repo.

Which issue(s) this PR fixes:
Fixes #36 .

Special notes for your reviewer:

I successfully tested it with the service-provider-flux e2e tests. As noted above, for the individual tests MCP clusters to also be reusable, the Teardown() function has to be adjusted. This has to be done in a separate PR.
I splitted this feature into multiple commits, that logically build-up. I hope this makes a review easier and less messy.

Release note:

ability to reuse clusters of e2e tests for faster local dev iterations

christophrj

@dattito Thanks a lot for your contribution and the detailed description. This already looks great. Please have a look at my comments and let me know your thoughts on this.

christophrj · 2026-05-27T15:00:49Z

+// The intent is to make install paths idempotent so that the test harness can
+// be re-run against an existing cluster (see E2E_REUSE_CLUSTER) and propagate
+// spec changes (e.g. a new image) without erroring on AlreadyExists.
+func CreateOrUpdate(ctx context.Context, cfg *envconf.Config, obj k8s.Object) error {


have you tried using controller-runtime createOrUpdate instead? I mean by creating a client.Client based on the klient.Client cfg. It might look a little strange to but would save you the custom implementation.

Yes that is a good idea. I was worried about the "merge" part there, but in the end I also have implemented my self too.

I will replace it.

Maybe controller-runtime's createOrUpdate is just not a good fit with the e2e decoder/klient and untyped/unstructured objects. At least the way it is implemented right now is pretty confusing and uncommon imo.

But having looked at it again I was wondering if it could be simplified with the existing decoder functions by just replacing the decoder.CreateIgnoreAlreadyExists with a delegate to either decoder.UpdateHandler or decoder.CreateHanlder? What do you think?

christophrj · 2026-05-27T15:19:17Z

 		Setup(s.installServiceProviders()).
-		Finish(s.cleanup(kindConfig, operatorTemplate)).
-		Finish(envfuncs.DestroyCluster(platformClusterName))
+		Setup(s.rolloutOnReuseDeployments())


I'm not sure if I totally understand or missed the reason that it has to be this way but let's discuss :) I am wondering if it wouldn't be easier to reuse the existing LoadImageToCluster flag of each *Setup struct instead of adding another RolloutOnReuse flag.

My sugesstion is that you don't have this additional Setup(s.rolloutOnReuseDeployment()) step but rather adjust the install* steps to be a installOrRedeploy similar to your createNamespace adjustment. Inside the install* functions: If LoadImageToCluster is true and the related ServiceProvider/ClusterProvider/PlatformService resource already exists, delete the resource, wait and recreate it. If LoadImageToCluster is false and the resource already exists, just update it in case the provided image has been changed. This scenario should trigger a regular update that is picked up the openmcp-operator similar to what we do in a real environment.

This way the deployment is still managed by the openmcp-operator with init jobs and run deployments being created properly. This would also make sure that any updated init (job) functionality of that provider is applied as part of the reload.

Yes that sounds good. I was worried about this needing a lot of time, but I tested it and it doesn't use much time (+ ~5 seconds per run).

You introduced all the required parts but didn't change the orchestration. Can't you drop rollout.go and just use CreateOrUpdate inside the already existing functions (installOpenMCPOperator, installClusterProvider, installPlatformServices and installServiceProviders)?

On-behalf-of: David Siregar <david.siregar@sap.com> Signed-off-by: David Siregar <david.siregar@sap.com>

…ReuseMode) On-behalf-of: David Siregar <david.siregar@sap.com> Signed-off-by: David Siregar <david.siregar@sap.com>

On-behalf-of: David Siregar <david.siregar@sap.com> Signed-off-by: David Siregar <david.siregar@sap.com>

Signed-off-by: David Siregar <david.siregar@sap.com>

…e SP Signed-off-by: David Siregar <david.siregar@sap.com>

dattito · 2026-05-29T12:58:34Z

Thanks for the review @christophrj. I adjusted the code based on your comments, and I also rebased on main.

christophrj · 2026-06-01T10:01:41Z

 			{
 				Name:  "crossplane",
 				Image: "ghcr.io/openmcp-project/images/service-provider-crossplane:v0.4.1",
+				// To iterate on a locally-built same-tag image of this SP,


I would prefer to not add the documentation here but where it is actually happening which is in the bootstrap functions. From a user/export perspective the LoadImageToCluster has not changed.

christophrj · 2026-06-01T10:03:55Z

+	// additionally deletes and re-creates this PlatformService CR so the
+	// operator runs its full reconciliation again (init job + run deployment
+	// + Ready), picking up the freshly-loaded image. This flag is the
+	// entry point for the same-tag local-rebuild dev loop.


see my earlier comment: I would prefer to not add the documentation here but where it is actually happening which is in the bootstrap functions. From a user/export perspective the LoadImageToCluster has not changed.

christophrj · 2026-06-01T10:04:00Z

 	Image    string
 	WaitOpts []wait.Option
-	// LoadImageToCluster allows using local images that have to be loaded into the kind cluster
+	// LoadImageToCluster, when true, loads the local image into the kind


see my earlier comment: I would prefer to not add the documentation here but where it is actually happening which is in the bootstrap functions. From a user/export perspective the LoadImageToCluster has not changed.

christophrj · 2026-06-01T10:04:08Z

 	Image    string
 	WaitOpts []wait.Option
-	// LoadImageToCluster allows using local images that have to be loaded into the kind cluster
+	// LoadImageToCluster, when true, loads the local image into the kind


see my earlier comment: I would prefer to not add the documentation here but where it is actually happening which is in the bootstrap functions. From a user/export perspective the LoadImageToCluster has not changed.

christophrj · 2026-06-01T12:25:34Z

 	PlatformName string
 	WaitOpts     []wait.Option
-	// LoadImageToCluster allows using local images that have to be loaded into the kind cluster
+	// LoadImageToCluster, when true, loads the local image into the kind


same as the earlier documentation comments

christophrj · 2026-06-01T13:35:39Z

 		Setup(s.installServiceProviders()).
-		Finish(s.cleanup(kindConfig, operatorTemplate)).
-		Finish(envfuncs.DestroyCluster(platformClusterName))
+		Setup(s.rolloutOnReuseDeployments())


You introduced all the required parts but didn't change the orchestration. Can't you drop rollout.go and just use CreateOrUpdate inside the already existing functions (installOpenMCPOperator, installClusterProvider, installPlatformServices and installServiceProviders)?

christophrj · 2026-06-01T13:42:13Z

+// The intent is to make install paths idempotent so that the test harness can
+// be re-run against an existing cluster (see E2E_REUSE_CLUSTER) and propagate
+// spec changes (e.g. a new image) without erroring on AlreadyExists.
+func CreateOrUpdate(ctx context.Context, cfg *envconf.Config, obj k8s.Object) error {


Maybe controller-runtime's createOrUpdate is just not a good fit with the e2e decoder/klient and untyped/unstructured objects. At least the way it is implemented right now is pretty confusing and uncommon imo.

But having looked at it again I was wondering if it could be simplified with the existing decoder functions by just replacing the decoder.CreateIgnoreAlreadyExists with a delegate to either decoder.UpdateHandler or decoder.CreateHanlder? What do you think?

dattito requested a review from christophrj as a code owner May 22, 2026 08:50

christophrj requested changes May 27, 2026

View reviewed changes

dattito added 5 commits May 29, 2026 14:58

feat(resources): CreateOrUpdate helper for idempotent installs

a53f16f

On-behalf-of: David Siregar <david.siregar@sap.com> Signed-off-by: David Siregar <david.siregar@sap.com>

feat(setup): reuse-cluster mode (E2E_REUSE_CLUSTER + stable name + Is…

92de6b3

…ReuseMode) On-behalf-of: David Siregar <david.siregar@sap.com> Signed-off-by: David Siregar <david.siregar@sap.com>

test(e2e): reuse-mode regression Assess + gated MCP teardown

bee3da7

On-behalf-of: David Siregar <david.siregar@sap.com> Signed-off-by: David Siregar <david.siregar@sap.com>

feat(setup): LoadImageToCluster reuse-mode rollout (delete-and-recreate)

8fa948e

Signed-off-by: David Siregar <david.siregar@sap.com>

test(e2e): document LoadImageToCluster reuse-mode opt-in on crossplan…

76f8523

…e SP Signed-off-by: David Siregar <david.siregar@sap.com>

dattito force-pushed the feat/reusable-e2e-test-setups branch from 4e9ca8a to 76f8523 Compare May 29, 2026 12:58

dattito requested a review from christophrj May 29, 2026 12:58

christophrj reviewed Jun 1, 2026

View reviewed changes

Conversation

dattito commented May 22, 2026

Uh oh!

christophrj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dattito commented May 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants