client-go: support `Shutdown()` for metadata and dynamic informers #114434

howardjohn · 2022-12-12T23:24:36Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Followup to #112200, specifically #112200 (comment).

Special notes for your reviewer:

Does this PR introduce a user-facing change?

client-go: metadatainformer and dynamicinformer `SharedInformerFactory`s now supports waiting for goroutines during shutdown

Followup to kubernetes#112200, specifically kubernetes#112200 (comment).

howardjohn · 2022-12-12T23:24:51Z

cc @pohly

apelisse · 2022-12-13T15:42:33Z

cc @alexzielenski

apelisse · 2022-12-13T15:42:39Z

/triage accepted

apelisse · 2022-12-13T15:46:45Z

staging/src/k8s.io/client-go/metadata/metadatainformer/interface.go

@@ -27,6 +27,7 @@ type SharedInformerFactory interface {
 	Start(stopCh <-chan struct{})
 	ForResource(gvr schema.GroupVersionResource) informers.GenericInformer
 	WaitForCacheSync(stopCh <-chan struct{}) map[schema.GroupVersionResource]bool
+	Shutdown()


naive by-stander question, why do we need a shutdown when starts already takes a stopCh?

The shutdown additionally actually waits for everything to terminate is the difference

It would make sense to copy the documentation from https://github.com/kubernetes/kubernetes/pull/112200/files, then someone (like @apelisse) who looks at this interface can see how to use it without having to know that some other, similar interface elsewhere explains it.

Done, copy+pasted from your PR. thanks

I'm not super excited about this API, one needs to keep two handles to shutdown a factory (the stopCh, AND the factory which is error-prone and smelly). I can also call shutdown on something that hasn't been created. I can shutdown but forget to close the channel (it will hang forever?)

I had called the new function WaitForShutdown initially and then there were concerns about that name that led to a renaming 🤷

See #112200 (comment)

Maybe "RejectNewAndWaitForShutdown" at least captures what it actually does :(

Apologies for not spending more time on that review :(

Normally we call the method "Run" instead of "Start" and you just wait for it to exit.

I agree that the most consistent thing to do here would be to expose a Run which blocks. Start could be refactored to call Run for backwards compatibility. Is that something people are interested in?

Alex, defining a Run sounds good to me. It's not clear without digging in if it is good to make one function call the other or not.

We should definitely leave the existing Start functions as-is behavior-wise to not break people.

Yeah, I think we can define Run to call Start and the now DOA Shutdown, that'd be a good API with minimal change.

howardjohn · 2022-12-13T16:09:43Z

/retest

pohly · 2022-12-14T16:30:39Z

/retest

howardjohn · 2023-01-19T18:48:26Z

/retest

howardjohn · 2023-02-16T17:31:59Z

Given #114496 is closed can we move forward with this for consistency?

alexzielenski · 2023-02-16T23:59:12Z

since this is the direction we are going for this API, this is looks good to me. I checked the changes against the original, and they are mirrored

/lgtm
/assign @lavalamp

k8s-ci-robot · 2023-02-16T23:59:18Z

LGTM label has been added.

Git tree hash: 5e03ccac06ade04754f48cb0bf5388bc4bc4d3c4

staging/src/k8s.io/client-go/dynamic/dynamicinformer/informer.go

staging/src/k8s.io/client-go/metadata/metadatainformer/informer.go

lavalamp · 2023-02-24T22:26:31Z

/approve

for nits:
/hold

k8s-ci-robot · 2023-02-24T22:26:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, lavalamp

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]

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

howardjohn · 2023-02-28T22:36:46Z

/hold cancel

lavalamp · 2023-02-28T22:41:42Z

/label tide/merge-method-squash
/lgtm

k8s-ci-robot · 2023-02-28T22:41:49Z

LGTM label has been added.

Git tree hash: 1c72024159e4b60f5c53439f1d3f6121b8ed4d3a

k8s-triage-robot · 2023-03-01T02:54:42Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2023-03-01T06:03:42Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

…ubernetes#114434) * client-go: support `Shutdown()` for metadata and dynamic informers Followup to kubernetes#112200, specifically kubernetes#112200 (comment). * add comments * Defer lock

client-go: support Shutdown() for metadata and dynamic informers

42a0ffe

Followup to kubernetes#112200, specifically kubernetes#112200 (comment).

k8s-ci-robot requested review from apelisse and deads2k December 12, 2022 23:25

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 12, 2022

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 Dec 13, 2022

apelisse reviewed Dec 13, 2022

View reviewed changes

add comments

019a4d4

alexzielenski mentioned this pull request Dec 14, 2022

[WIP] add Run to Informer Factories #114496

Closed

k8s-ci-robot assigned lavalamp and alexzielenski Feb 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 16, 2023

lavalamp reviewed Feb 24, 2023

View reviewed changes

staging/src/k8s.io/client-go/dynamic/dynamicinformer/informer.go Outdated Show resolved Hide resolved

lavalamp reviewed Feb 24, 2023

View reviewed changes

staging/src/k8s.io/client-go/metadata/metadatainformer/informer.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2023

Defer lock

c087b10

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2023

k8s-ci-robot requested review from alexzielenski and lavalamp February 28, 2023 22:36

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2023

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 28, 2023

k8s-ci-robot merged commit b99fe0d into kubernetes:master Mar 1, 2023

k8s-ci-robot added this to the v1.27 milestone Mar 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client-go: support `Shutdown()` for metadata and dynamic informers #114434

client-go: support `Shutdown()` for metadata and dynamic informers #114434

howardjohn commented Dec 12, 2022

howardjohn commented Dec 12, 2022

apelisse commented Dec 13, 2022

apelisse commented Dec 13, 2022

apelisse Dec 13, 2022

howardjohn Dec 13, 2022

pohly Dec 14, 2022

howardjohn Dec 14, 2022

apelisse Dec 14, 2022

pohly Dec 14, 2022

lavalamp Dec 14, 2022

alexzielenski Dec 14, 2022

lavalamp Dec 14, 2022

apelisse Dec 14, 2022

howardjohn commented Dec 13, 2022

pohly commented Dec 14, 2022

howardjohn commented Jan 19, 2023

howardjohn commented Feb 16, 2023

alexzielenski commented Feb 16, 2023 •

edited

k8s-ci-robot commented Feb 16, 2023

lavalamp commented Feb 24, 2023

k8s-ci-robot commented Feb 24, 2023

howardjohn commented Feb 28, 2023

lavalamp commented Feb 28, 2023

k8s-ci-robot commented Feb 28, 2023

k8s-triage-robot commented Mar 1, 2023

k8s-triage-robot commented Mar 1, 2023

client-go: support Shutdown() for metadata and dynamic informers #114434

client-go: support Shutdown() for metadata and dynamic informers #114434

Conversation

howardjohn commented Dec 12, 2022

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

howardjohn commented Dec 12, 2022

apelisse commented Dec 13, 2022

apelisse commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn commented Dec 13, 2022

pohly commented Dec 14, 2022

howardjohn commented Jan 19, 2023

howardjohn commented Feb 16, 2023

alexzielenski commented Feb 16, 2023 • edited

k8s-ci-robot commented Feb 16, 2023

lavalamp commented Feb 24, 2023

k8s-ci-robot commented Feb 24, 2023

howardjohn commented Feb 28, 2023

lavalamp commented Feb 28, 2023

k8s-ci-robot commented Feb 28, 2023

k8s-triage-robot commented Mar 1, 2023

k8s-triage-robot commented Mar 1, 2023

client-go: support `Shutdown()` for metadata and dynamic informers #114434

client-go: support `Shutdown()` for metadata and dynamic informers #114434

alexzielenski commented Feb 16, 2023 •

edited