Skip to content

Add loadtesting framework#3895

Open
SimonTheLeg wants to merge 2 commits intokcp-dev:mainfrom
SimonTheLeg:load-test-framework
Open

Add loadtesting framework#3895
SimonTheLeg wants to merge 2 commits intokcp-dev:mainfrom
SimonTheLeg:load-test-framework

Conversation

@SimonTheLeg
Copy link
Member

@SimonTheLeg SimonTheLeg commented Mar 10, 2026

Summary

This PR adds the loadtesting framework, considerations for review:

  • testing/example_test.go is a good starting point to see how all different aspects of the framework come together
  • Updates the Readme to describe the loadtest itself. Old Readme has been moved to Proposal.md
  • For the stats we just wrap around https://github.com/montanaflynn/stats, so we can give stats names for the output. But other than that I found it much better to use a well-tested, zero dependency library instead of us going around implementing our own P99 and so on functions
  • The framework is kept as minimal as possible, and takes some inspiration from clusterloader2 kubernetes performance testing https://github.com/kubernetes/perf-tests/tree/master/clusterloader2, especially the overall folder structure and the tuningset terminology. It does modernize some aspects of it by using Go iterators for the tuninsets themselves to control concurrency via yielding, which I think results in a bit cleaner code and error handling
  • I went against passing configurable options to the tests via flags and instead went with env variables. The idea behind this was mainly that a) we don't need to have custom TestMains everywhere to ensure centralized flags in setup.go + b) we can run things like go test ./... without all test packages requiring the same flags

What Type of PR Is This?

/kind feature

Release Notes

NONE

@kcp-ci-bot kcp-ci-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the DCO. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 10, 2026
@SimonTheLeg SimonTheLeg force-pushed the load-test-framework branch 4 times, most recently from 47f8c76 to ac3fd3a Compare March 10, 2026 16:35
Copy link
Member

@ntnn ntnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First quick look

Comment on lines +72 to +95
cfg := &Config{}
var missing []Capability

if slices.Contains(caps, KCPFrontProxyKubeconfig) {
v := os.Getenv(string(KCPFrontProxyKubeconfig))
if v == "" {
missing = append(missing, KCPFrontProxyKubeconfig)
} else {
cfg.FrontProxyKubeconfig = loadKubeconfig(t, v)
}
}

if slices.Contains(caps, KCPShardKubeconfig) {
v := os.Getenv(string(KCPShardKubeconfig))
if v == "" {
missing = append(missing, KCPShardKubeconfig)
} else {
cfg.ShardKubeconfig = loadKubeconfig(t, v)
}
}

if len(missing) > 0 {
t.Fatalf("Require: the following required environment variables are not set: %v", missing)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be short to just require.NotEmpty cfg.FrontProxyKubeconfig?

	if slices.Contains(caps, KCPFrontProxyKubeconfig) {
		cfg.FrontProxyKubeconfig = os.Getenv(string(KCPFrontProxyKubeconfig))
		require.NotEmpty(t, cfg.FrontProxyKubeconfig, "env var must be set", KCPFrontProxyKubeconfig))
	}

This should happen early enough that we don't need to check all settings first before failing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I really like the behavior that it immediately tells you all the variables that are missing. I expect over time for the list to grow quite large and when you run these tests as a pod somewhere, I think its convenient if the first error message just tells you everything.

What I could offer ot make it a bit more streamlined is something like this. Let me know if you have a strong preference on it over the other one.

	cfg := &Config{}
	var missing []Capability

	setters := map[Capability]func(*rest.Config){
		KCPFrontProxyKubeconfig: func(c *rest.Config) { cfg.FrontProxyKubeconfig = c },
		KCPShardKubeconfig:      func(c *rest.Config) { cfg.ShardKubeconfig = c },
	}

	for _, cap := range caps {
		v := os.Getenv(string(cap))
		if v == "" {
			missing = append(missing, cap)
			continue
		}
		setters[cap](loadKubeconfig(t, v))
	}

	require.Empty(t, missing, "the following required environment variables are not set: %v", missing)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use a combination of assert and a boolean:

	if slices.Contains(caps, KCPFrontProxyKubeconfig) {
		cfg.FrontProxyKubeconfig = os.Getenv(string(KCPFrontProxyKubeconfig))
		assert.NotEmpty(t, cfg.FrontProxyKubeconfig, "env var must be set", KCPFrontProxyKubeconfig))
		anyMissing = true
	}
	// ...
	require.False(t, anyMissing, "missing required components and/or settings")

Keeps it streamlined while staying flexible to check if e.g. additional tooling is present if the configuration requires it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry might be Monday brain: But this would not work for the happy path or?
If I have the env set correctly then assert is fine, but we still would set anyMissing to true, even though it is not missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah true :D I wasn't thinking correctly - but you can check on testing.T if it is already failed:
https://pkg.go.dev/testing#T.Failed

	if slices.Contains(caps, KCPFrontProxyKubeconfig) {
		cfg.FrontProxyKubeconfig = os.Getenv(string(KCPFrontProxyKubeconfig))
		assert.NotEmpty(t, cfg.FrontProxyKubeconfig, "env var must be set", KCPFrontProxyKubeconfig))
	}
	// ...
	require.False(t, t.Failed(), "missing required components and/or settings")

@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ntnn. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@SimonTheLeg
Copy link
Member Author

/hold there is currently an issue where the unit-tests are passing, but skipping the unit-tests of the framework. I'll look into it, but for now hold the PR so we don't accidentally merge/forget about it.

@kcp-ci-bot kcp-ci-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2026
@SimonTheLeg SimonTheLeg force-pushed the load-test-framework branch from 2f89683 to d757aca Compare March 12, 2026 15:58
@SimonTheLeg
Copy link
Member Author

/unhold
I think the E2E are currently borked by our recent rebase k8s 1.35 update, but previously skipped unit tests for the load testing framework are now correctly executed :)
So ready for review

@kcp-ci-bot kcp-ci-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2026
@ntnn ntnn added this to tbd Mar 13, 2026
@github-project-automation github-project-automation bot moved this to Backlog in tbd Mar 13, 2026
@ntnn ntnn moved this from Backlog to Reviewing in tbd Mar 13, 2026
On-behalf-of: SAP <simon.bein@sap.com>
Signed-off-by: Simon Bein <simontheleg@gmail.com>
@SimonTheLeg SimonTheLeg force-pushed the load-test-framework branch from d757aca to 63257cc Compare March 16, 2026 14:51
now that we also have it on main

On-behalf-of: SAP <simon.bein@sap.com>
Signed-off-by: Simon Bein <simontheleg@gmail.com>
@SimonTheLeg
Copy link
Member Author

/retest

@SimonTheLeg
Copy link
Member Author

/restest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has signed the DCO. kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants