Reduce goroutine leakage in test/integration/controlplane/transformation

[sanwishe]

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix leaking goroutines in test/integration/controlplane/transformation

Which issue(s) this PR fixes:

Fixes #111674

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @sanwishe. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[enj]

Instead of a manual stop method that is a no-op in many cases I think it would be better to have a context be used to cancel any implementations that explicitly need it.

[yangjunmyfm192085]

/ok-to-test

[yangjunmyfm192085]

ToDo can be deleted.

[aojea]

hopefully can be deleted soon, but there are still some other leaks #108483 (comment)

[aojea]

Instead of a manual stop method that is a no-op in many cases I think it would be better to have a context be used to cancel any implementations that explicitly need it.

@enj this is coming from #108483, it is not about context cancellation it is about storage shutdown, it needs a function that allows to stop and destroy all resources

[aojea]

does order matters?

[sanwishe]

It matters, and i have move it after client.close()

[aojea]

/assign @wojtek-t

he is more familiar with this code

[yangjunmyfm192085]

/retest

[enj]

Instead of a manual stop method that is a no-op in many cases I think it would be better to have a context be used to cancel any implementations that explicitly need it.

@enj this is coming from #108483, it is not about context cancellation it is about storage shutdown, it needs a function that allows to stop and destroy all resources

I understand what this code is trying to do. Contexts can be used to control lifetime, in the same way a manual stop method can. I do not want to expand the storage transformer interface with a method that one can forget to call. Instead, any implementation that needs any form of cleanup should take a context in its constructor and automatically perform said cleanup when the context is canceled.

[enj]

/assign

[249043822]

a kms transformer which uses a grpc-service but doen't provide a mechnism to close the connection of grpc-service , so we should assure connection close while apiserver shutdown. since the lifecycle of the transformer is consistent with apiserver, does listenning a shutdown signal(or a stopCh) for grpc-service closing more suitable than a context? @aojea @enj @liggitt

[wojtek-t]

a kms transformer which uses a grpc-service but doen't provide a mechnism to close the connection of grpc-service , so we should assure connection close while apiserver shutdown. since the lifecycle of the transformer is consistent with apiserver, does listenning a shutdown signal(or a stopCh) for grpc-service closing more suitable than a context? @aojea @enj @liggitt

Storage is being destroyed at the very end:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go#L464
We still don't have graceful shutdown of watches (which can use transformers), so we need to ensure that transformers won't get shutdown too early.

Instead, any implementation that needs any form of cleanup should take a context in its constructor and automatically perform said cleanup when the context is canceled.

@enj - I don't have very strong opinion on that and if we can make that work with contexts - I'm fine with it.
That said, currently transformers (in particular the KMS ones) are created very early, before even the apiserver config is created:

kubernetes/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

Line 245 in 18ce801

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

[it's called at the level of initializing options]
So this approach would require significant changes to be implemented.

OTOH, the current Destroy/Stop-like approach is relatively easy - we don't expect people to be doing it on their own. It's the responsibility of the server to do that on shutdown, and as such I don't think that the concern of someone forgetting to call it is a valid one.

[enj]

We already have existing bugs for CRDs not supporting encryption at rest because the wiring is incorrect so I don't buy the argument that "we won't forget because we only need to do it in one place in the API server" - nothing enforces that invariant. If we want to get the go routine number down before test freeze with this change, that is fine. I have to refactor a lot of this code for KMS v2 so I can clean it up then.

[wojtek-t]

We already have existing bugs for CRDs not supporting encryption at rest because the wiring is incorrect so I don't buy the argument that "we won't forget because we only need to do it in one place in the API server"

My point was that cost is not per transformer-author. We have a fixed cost to properly wire the Initialize/Destroy interface (and yes, I agree with you that it's more than one place, because e.g. we do that differently for CRDs vs built-ins).
But I disagree that the model with destroy is more complex - this is roughly symmetric. In one case (your proposal) we need to enforce proper initialization (also not exactly the same across CRDs and built-ins), in this case, we need to ensure proper deletion. I don't agree that one is visibly simpler than the other.

I don't know what your plans for KMSv2 are exactly and I admit that the additional interface method isn't perfect. But it has one advantage of forcing people to think about the lifecycle, which definitely isn't the case for many authors now (not specific to transformers - it's the same for many controllers, plugins, etc.).

I don't think it's super critical for 1.25, so if you are planning to changing that soon (in 1.26) I'm fine with waiting to see if we can make that work in a simpler way then. If this will take multiple releases to get there, I think we should try merging something like this (I didn't do detailed review of it - just skimmed it).

[enj]

KMS v2 is part of my day job so it will definitely get done 😉

@liggitt had expressed wanting this go routine bit fixed in 1.25 so this may be fine for now (I haven't looked thoroughly).

[fedebongio]

/triage accepted

[aojea]

KMS v2 is part of my day job so it will definitely get done 😉

nice

@wojtek-t can you take a look and judge if we should add it to 1.25, we are still on the testing freeze period https://www.kubernetes.dev/resources/release/#summary

[wojtek-t]

@wojtek-t can you take a look and judge if we should add it to 1.25, we are still on the testing freeze period

I took a deeper look into this PR and this LGTM.
I think that even if longer-term we decide to change the interface (as part of changes for KMSv2) and get rid of the Stop() method this is fine.
But given I don't know the exact plans for KMSv2 I would like to hear an ACK from @enj before approving.

From purely test-freeze perpective, that looks reasonable to merge.

[enj]

I have to do a lot of refactors to the KMS code to support the v2 feature set so this will not be a big issue for me to update. @wojtek-t you will have to approve the API server code changes.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, sanwishe
Once this PR has been reviewed and has the lgtm label, please ask for approval from wojtek-t by writing /assign @wojtek-t in a comment. For more information see:The Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/OWNERS [enj]
• staging/src/k8s.io/apiserver/pkg/storage/OWNERS
• test/OWNERS [enj]

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

[enj]

Test freeze for 1.25 was 18:00 PDT Tuesday 9th August 2022 so I think we have missed the window for 1.25...

[enj]

/lgtm cancel

(since we missed the deadline)

[enj]

I have opened #111986 to fix this bug without expanding the transformer interface.

[enj]

@sanwishe thanks for your contribution.

This issue was resolved in #111986

/close

[k8s-ci-robot]

@sanwishe: 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.

[k8s-ci-robot]

@enj: Closed this PR.

In response to this:

@sanwishe thanks for your contribution.

This issue was resolved in #111986

/close

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.