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

PWX-37601: Using informer cache's event handling instead of native k8s watchers for pod monitoring and extender metrics. #1795

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

diptiranjanpx
Copy link
Contributor

Signed-Off-By: Diptiranjan

What type of PR is this?

improvement

What this PR does / why we need it:
Reducing the load by not registering for watchers with kube-api-server for pod updates.
Instead informer cache's event handling has been used.

Does this PR change a user-facing CRD or CLI?:
no

Is a release note needed?:

…s watchers for pod monitoring and extender metrics.
Copy link
Contributor

@pp511 pp511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. One nit and one clarification. Please fix the build failures.

@@ -36,6 +38,9 @@ type SharedInformerCache interface {

// ListTransformedPods lists the all the Pods from the cache after applying TransformFunc
ListTransformedPods() (*corev1.PodList, error)

// WatchPods watches registers the pod event handler with the informer cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit..watches registers?
I think need to remove watches or need to reword it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modified

Comment on lines +202 to +206
log.Warnf("Shared informer cache has not been initialized, using watch for pod monitor.")
if err := core.Instance().WatchPods("", fn, metav1.ListOptions{}); err != nil {
log.Errorf("failed to watch pods for health monitoring due to: %v", err)
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering in what scenario will we enter inside this function? I don't think we ever will? It's still ok to keep it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly not required, I also think as a failsafe , let's keep it.

@pp511 pp511 self-requested a review June 12, 2024 07:49
Copy link
Contributor

@pp511 pp511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 12.12121% with 29 lines in your changes missing coverage. Please review.

Project coverage is 38.09%. Comparing base (b8f6264) to head (d688267).

Files Patch % Lines
pkg/extender/extender.go 11.76% 12 Missing and 3 partials ⚠️
pkg/monitor/monitor.go 12.50% 11 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1795      +/-   ##
==========================================
- Coverage   38.15%   38.09%   -0.06%     
==========================================
  Files          51       51              
  Lines       11443    11470      +27     
==========================================
+ Hits         4366     4370       +4     
- Misses       6688     6707      +19     
- Partials      389      393       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@diptiranjanpx diptiranjanpx merged commit e7a4826 into master Jun 13, 2024
3 of 5 checks passed
diptiranjanpx added a commit that referenced this pull request Jun 13, 2024
…s watchers for pod monitoring and extender metrics. (#1795)

* PWX-37601: Using informer cache's event handling instead of native k8s watchers for pod monitoring and extender metrics.

* Generated the mock files.
lalat-das pushed a commit that referenced this pull request Jun 16, 2024
…s watchers for pod monitoring and extender metrics. (#1795)

* PWX-37601: Using informer cache's event handling instead of native k8s watchers for pod monitoring and extender metrics.

* Generated the mock files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants