Skip to content
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

reduce goroutine leakage in test/integration/controlplane/transformation #111674

Closed
aramase opened this issue Aug 3, 2022 · 6 comments · Fixed by #111986
Closed

reduce goroutine leakage in test/integration/controlplane/transformation #111674

aramase opened this issue Aug 3, 2022 · 6 comments · Fixed by #111986
Assignees
Labels
sig/auth Categorizes an issue or PR as relevant to SIG Auth. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@aramase
Copy link
Member

aramase commented Aug 3, 2022

Following up on #111126 (comment) to shutdown the gRPC goroutines and reduce the number of leaked goroutines.

/sig auth

@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 3, 2022
@aojea
Copy link
Member

aojea commented Aug 4, 2022

/cc

@aojea
Copy link
Member

aojea commented Aug 5, 2022

@aramase if you are too busy, just add a description on where the problem is and, if possible, suggestions on how to fix it, there is always people wanting to help with these things

@sanwishe
Copy link
Contributor

sanwishe commented Aug 5, 2022

Comment #108483(comment) might help.

The goroutines are coming from the fact that we're not closing the kms transformer (grpc goroutines) that is being created.
We basically create a grpc service and never close that:

envelopeService, err = envelopeServiceFactory(provider.KMS.Endpoint, provider.KMS.Timeout.Duration)

Fixing would probably require changing transformer interfact to implement Close method or sth like that.

@aojea
Copy link
Member

aojea commented Aug 5, 2022

I was thinking in something like

diff --git a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope.go b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope.go
index 30795d41a87..d6792c73479 100644
--- a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope.go
+++ b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope.go
@@ -44,6 +44,8 @@ type Service interface {
        Decrypt(data []byte) ([]byte, error)
        // Encrypt bytes to a ciphertext.
        Encrypt(data []byte) ([]byte, error)
+       // Stop the service and close the connections.
+       Stop()
 }
 
 type envelopeTransformer struct {
diff --git a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope_test.go b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope_test.go
index 4f030772eb9..adc131bea1a 100644
--- a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/envelope_test.go
@@ -71,6 +71,9 @@ func (t *testEnvelopeService) Rotate() {
        t.keyVersion = strconv.FormatInt(int64(i+1), 10)
 }
 
+func (t *testEnvelopeService) Stop() {
+}
+
 func newTestEnvelopeService() *testEnvelopeService {
        return &testEnvelopeService{
                keyVersion: "1",
diff --git a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service.go b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service.go
index c5304cd09f2..189119f8d31 100644
--- a/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service.go
+++ b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/grpc_service.go
@@ -152,3 +152,9 @@ func (g *gRPCService) interceptor(
 
        return invoker(ctx, method, req, reply, cc, opts...)
 }
+
+func (g *gRPCService) Stop() {
+       g.mux.Lock()
+       defer g.mux.Unlock()
+       g.connection.Close()
+}

if someone wants to give it a shot, to hook the Close() with the Storage()

@sanwishe
Copy link
Contributor

sanwishe commented Aug 5, 2022

@aojea Thanks for your demo, And I would like to work on this.
/assign @sanwishe

@enj
Copy link
Member

enj commented Aug 18, 2022

/milestone v1.26
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 18, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/auth Categorizes an issue or PR as relevant to SIG Auth. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: Done
5 participants