New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: testing: make apiserver-based tests context-aware #124093
base: master
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Thu Mar 28 02:11:27 UTC 2024. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
27a68ae introduced context support for events. Creating an event broadcaster with context makes tests more resilient against leaking goroutines when that context gets canceled at the end of a test and enables per-test output via ktesting. The context could get passed to the constructor. A cleaner solution is to enhance context support for the apiserver and then pass the context into the controller's run method. This ripples up the call stack to all places which start an apiserver.
27a68ae introduced context support for events. Creating an event broadcaster with context makes tests more resilient against leaking goroutines when that context gets canceled at the end of a test and enables per-test output via ktesting. The New method already had a context, therefore no API changes are needed.
This enables per-test output. test/e2e_node/runner/remote/run_remote.go no longer needs to add klog flags itself, ktesting takes care of that.
049eb4f
to
0bb3047
Compare
@@ -100,23 +102,15 @@ type TestServerInstanceOptions struct { | |||
|
|||
// TestServer return values supplied by kube-test-ApiServer | |||
type TestServer struct { | |||
ClientConfig *restclient.Config // Rest client config | |||
ktesting.TContext | |||
ClientConfig *restclient.Config // Rest client config, also available via TContext.RESTConfig() (= TestServer.RESTConfig()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mengjiao-liu, @aojea : style question...
My goal is to let StartTestServer
handle the TContext and client set creation and tear-down. That makes it possible to remove some common boilerplate code that appears in all integration tests.
Should that TContext get returned inline as part of TestServer (as shown above) or as a separate return value?
When it's inline, test code looks like this:
func TestEndpointUpdates(t *testing.T) {
// Disable ServiceAccount admission plugin as we don't have serviceaccount controller running.
server := kubeapiservertesting.StartTestServerOrDie(t, nil, []string{"--disable-admission-plugins=ServiceAccount"}, framework.SharedEtcd())
client := server.Client()
informers := informers.NewSharedInformerFactory(client, 0)
tCtx := server.TContext
epController := endpoint.NewEndpointController(
tCtx,
...
diff --git a/test/integration/endpoints/endpoints_test.go b/test/integration/endpoints/endpoints_test.go
index e1aee518c02..3cebed7b394 100644
--- a/test/integration/endpoints/endpoints_test.go
+++ b/test/integration/endpoints/endpoints_test.go
@@ -28,7 +28,6 @@ import (
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/informers"
- clientset "k8s.io/client-go/kubernetes"
kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing"
"k8s.io/kubernetes/pkg/controller/endpoint"
"k8s.io/kubernetes/test/integration/framework"
@@ -38,17 +37,10 @@ import (
func TestEndpointUpdates(t *testing.T) {
// Disable ServiceAccount admission plugin as we don't have serviceaccount controller running.
server := kubeapiservertesting.StartTestServerOrDie(t, nil, []string{"--disable-admission-plugins=ServiceAccount"}, framework.SharedEtcd())
- defer server.TearDownFn()
-
- client, err := clientset.NewForConfig(server.ClientConfig)
- if err != nil {
- t.Fatalf("Error creating clientset: %v", err)
- }
-
+ client := server.Client()
informers := informers.NewSharedInformerFactory(client, 0)
- tCtx := ktesting.Init(t)
- defer tCtx.Cancel("test completed")
+ tCtx := server.TContext
epController := endpoint.NewEndpointController(
tCtx,
informers.Core().V1().Pods(),
When it's separate:
func TestEndpointUpdates(t *testing.T) {
// Disable ServiceAccount admission plugin as we don't have serviceaccount controller running.
tCtx, server := kubeapiservertesting.StartTestServerOrDie(t, nil, []string{"--disable-admission-plugins=ServiceAccount"}, framework.SharedEtcd())
client := tCtx.Client()
informers := informers.NewSharedInformerFactory(client, 0)
epController := endpoint.NewEndpointController(
tCtx,
...
I'm leaning towards separate return values. The only downside is that all callers need to be updated, but I want to do that anyway.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This enables per-test output.
Special notes for your reviewer:
test/e2e_node/runner/remote/run_remote.go no longer needs to
add klog flags itself, ktesting takes care of that.
Does this PR introduce a user-facing change?