refactor: Replace ListerWatcher with ListerWatcherWithContext #2724

onasser1 · 2025-08-09T15:33:17Z

What this PR does / why we need it:
ListerWatcher is deprecated and we need to move KSM code to use instead ListerWatcherWithContext
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
N/A
**Which issue(s) this PR fixes:
Fixes #2721

k8s-ci-robot · 2025-08-09T15:33:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: onasser1
Once this PR has been reviewed and has the lgtm label, please assign dgrisonnet for approval. For more information see the 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:

OWNERS

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

k8s-ci-robot · 2025-08-09T15:33:26Z

This issue is currently awaiting triage.

If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

k8s-ci-robot · 2025-08-09T15:33:26Z

Welcome @onasser1!

It looks like this is your first PR to kubernetes/kube-state-metrics 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-state-metrics has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

onasser1 · 2025-08-09T15:37:42Z

/hold

onasser1 · 2025-08-09T15:53:15Z

internal/store/builder.go

+	listWatchWithContextFunc func(kubeClient clientset.Interface, ns string, fieldSelector string) cache.ListerWatcherWithContext,
 	useAPIServerCache bool, objectLimit int64,
 ) []cache.Store {
 	metricFamilies = generator.FilterFamilyGenerators(b.familyGeneratorFilter, metricFamilies)
 	composedMetricGenFuncs := generator.ComposeMetricGenFuncs(metricFamilies)
 	familyHeaders := generator.ExtractMetricFamilyHeaders(metricFamilies)
+	var listerWatcher func(kubeClient clientset.Interface, ns string, fieldSelector string) cache.ListerWatcher


That listerWatcher func will be removed (I prevent startReflector from raising errors)
should be replaced with listWatchWithContextFunc. further information I will explain at startReflector function below

onasser1 · 2025-08-09T17:06:32Z

I haven't fixed/worked on tests yet, and still doing some changes so I'd prefer to keep this as a draft until I finish my changes and I'd mention you for a review, so you can ignore this for couple of days. I'd revisit this tomorrow.

onasser1 · 2025-08-11T16:16:18Z

internal/store/builder.go

-	instrumentedListWatch := watch.NewInstrumentedListerWatcher(listWatcher, b.listWatchMetrics, reflect.TypeOf(expectedType).String(), useAPIServerCache, objectLimit)
-	reflector := cache.NewReflectorWithOptions(sharding.NewShardedListWatch(b.shard, b.totalShards, instrumentedListWatch), expectedType, store, cache.ReflectorOptions{ResyncPeriod: 0})
+	instrumentedListWatchWithContext := watch.NewInstrumentedListerWatcher(listWatcherWithContext, b.listWatchMetrics, reflect.TypeOf(expectedType).String(), useAPIServerCache, objectLimit)
+	reflector := cache.NewReflectorWithOptions(sharding.NewShardedListWatch(b.shard, b.totalShards, instrumentedListWatchWithContext), expectedType, store, cache.ReflectorOptions{ResyncPeriod: 0})


issue:
newReflectorWithOptions which is used from client-go is taking a ListerWatcher as an argument and inside its logic converts it to ListerWatcherWithContext
but we need to send ListerWatcherWithContext from the beginning without the need of a cast.
https://github.com/kubernetes/client-go/blob/v0.33.3/tools/cache/reflector.go#L259

This is a good catch. Do you want to work with client-go folks to update the implementation to support ListerWatcherWithContext as well?

Absolutely Yes! but my question is should we update the parameter list to receive ListerWatcherWithContext but wouldn't this make a dependency conflict? Or we should support another version of the function with different parameter list as we would do an override.
or Let's move this question to a new issue at client-go repo? WDYT?

Since lister watcher without context is deprecated, I think we need a new NewReflector function in client-go. Yeah please open up an issue with client-go

is it deprecated?

https://github.com/kubernetes/kubernetes/blob/17d6c9c551f917b63797cb1cfe9bbc4b01ea6a43/staging/src/k8s.io/client-go/tools/cache/listwatch.go#L105-L111

If this is not possible, can I know why? thanks 😄

There were too many existing APIs and structs which just pass through a ListerWatcher, without actually doing anything with it. Changing all of those to have variants with ListWatcherWithContext would have been a much bigger change, without some actual benefits.

@pohly As Manuel Ruger seems kinda busy, can we move forward with this solution you proposed? What's your opinion?
If we can proceed with this, I'll wait for the approval from manuel with yours too.

I'd prefer to revisit the underlying assumption of this PR ("ListerWatcher is deprecated", which is not true) and instead address whatever linter warnings the current code gets without changing APIs.

Thanks Patrick, I've reached out to Manuel, but he is busy with the new release of KSM as it's the current priority, I think we will revisit this once he has capacity to review.

onasser1 · 2025-08-11T16:17:06Z

@dgrisonnet @rexagod Hi 👋🏻 I need help here, I guess I messed things up 🤔

onasser1 · 2025-08-11T16:19:44Z

Hi @mrueg 👋🏻 can you help me in your free time?
this is an initial solution, if you have a better solution please let me know and I can start working on it.

assu-2000 · 2025-08-24T09:40:09Z

@dgrisonnet @rexagod Hi 👋🏻 I need help here, I guess I messed things up 🤔

Hi @onasser1, you definitely haven’t messed anything up. I’d recommend taking some time to get familiar with the codebase, that will help you build confidence and make it easier to contribute. I also suggest joining the bi-weekly meeting, where you can ask any questions you might have and get valuable insights into the code. That way, you’ll avoid falling into the trap. You’re already doing amazing work ,keep it up!

Btw, maintainers and reviewers are at bandwidth, so please understand if they can’t always respond within your expected timeline.

onasser1 · 2025-08-24T11:10:23Z

@assu-2000 thank you for kind words 😃 I plan to do this actually.

k8s-ci-robot requested review from dgrisonnet and rexagod August 9, 2025 15:33

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 9, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2025

onasser1 changed the title ~~Replace ListerWatcher with ListerWatcherWithContext~~ refactor: Replace ListerWatcher with ListerWatcherWithContext Aug 9, 2025

Replace ListerWatcher with ListerWatcherWithContext

4a84bff

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

onasser1 commented Aug 9, 2025

View reviewed changes

onasser1 changed the title ~~refactor: Replace ListerWatcher with ListerWatcherWithContext~~ refactor: Draft - Replace ListerWatcher with ListerWatcherWithContext Aug 9, 2025

Fix unused ctx

aad019e

onasser1 changed the title ~~refactor: Draft - Replace ListerWatcher with ListerWatcherWithContext~~ refactor:Replace ListerWatcher with ListerWatcherWithContext Aug 10, 2025

onasser1 marked this pull request as draft August 10, 2025 06:21

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2025

onasser1 changed the title ~~refactor:Replace ListerWatcher with ListerWatcherWithContext~~ refactor: Replace ListerWatcher with ListerWatcherWithContext Aug 10, 2025

onasser1 marked this pull request as ready for review August 11, 2025 16:14

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2025

k8s-ci-robot requested a review from logicalhan August 11, 2025 16:14

onasser1 commented Aug 11, 2025

View reviewed changes

Convert ListerWatcher to ListerWatcherWithContext

a9067ff

onasser1 mentioned this pull request Aug 12, 2025

Support ListerWatcherWithContext in parameters list in startReflector kubernetes/kubernetes#133542

Open

onasser1 changed the title ~~refactor: Replace ListerWatcher with ListerWatcherWithContext~~ WIP: refactor: Replace ListerWatcher with ListerWatcherWithContext Aug 19, 2025

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 19, 2025

onasser1 mentioned this pull request Aug 20, 2025

Support ListerWatcherWithContext in parameters list in startReflector kubernetes/kubernetes#133567

Open

onasser1 changed the title ~~WIP: refactor: Replace ListerWatcher with ListerWatcherWithContext~~ refactor: Replace ListerWatcher with ListerWatcherWithContext Aug 25, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2025

rexagod added this to SIG Instrumentation Aug 26, 2025

github-project-automation bot moved this to Needs Triage in SIG Instrumentation Aug 26, 2025

refactor: Replace ListerWatcher with ListerWatcherWithContext #2724

Are you sure you want to change the base?

refactor: Replace ListerWatcher with ListerWatcherWithContext #2724

Conversation

onasser1 commented Aug 9, 2025

Uh oh!

k8s-ci-robot commented Aug 9, 2025

Uh oh!

k8s-ci-robot commented Aug 9, 2025

Uh oh!

k8s-ci-robot commented Aug 9, 2025

Uh oh!

onasser1 commented Aug 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onasser1 commented Aug 9, 2025

Uh oh!

onasser1 Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onasser1 Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onasser1 commented Aug 11, 2025

Uh oh!

onasser1 commented Aug 11, 2025

Uh oh!

assu-2000 commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

onasser1 commented Aug 24, 2025

Uh oh!

Uh oh!

onasser1 Aug 11, 2025 •

edited

Loading

pohly Aug 21, 2025 •

edited

Loading

onasser1 Aug 25, 2025 •

edited

Loading

assu-2000 commented Aug 24, 2025 •

edited

Loading