cleanup: refactor PodList calls to prepare for making pod metrics staleness configurable #1046

nayihz · 2025-06-23T12:33:04Z

k8s-ci-robot · 2025-06-23T12:33:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2025-06-23T12:34:17Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`c677334`
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/688c24d6f091430007cfbd0b
😎 Deploy Preview	https://deploy-preview-1046--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

nayihz · 2025-06-25T09:47:57Z

/cc @liu-cong

pkg/epp/backend/metrics/types.go

cmd/epp/runner/runner.go

pkg/epp/common/config/defaults.go

pkg/epp/server/runserver.go

pkg/epp/backend/metrics/logger.go

pkg/epp/scheduling/scheduler.go

nayihz · 2025-07-01T13:22:15Z

I found that it becomes very inconvenient to write unit tests after updating PodGetAll to PodGetAllWithFreshMetrics. But after reading the code in depth, I still couldn't come up with a good solution. Any ideas on this? @nirrozenbaum @liu-cong
https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/1046/files#diff-1b7741fc131b712835ea0040fe1dc86b62403c0b124f0d672ef8bfadb84d32d3R325-R328

https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/1046/files#diff-1b7741fc131b712835ea0040fe1dc86b62403c0b124f0d672ef8bfadb84d32d3R353

nirrozenbaum · 2025-07-01T14:10:56Z

I found that it becomes very inconvenient to write unit tests after updating PodGetAll to PodGetAllWithFreshMetrics. But after reading the code in depth, I still couldn't come up with a good solution. Any ideas on this? @nirrozenbaum @liu-cong https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/1046/files#diff-1b7741fc131b712835ea0040fe1dc86b62403c0b124f0d672ef8bfadb84d32d3R325-R328

https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/1046/files#diff-1b7741fc131b712835ea0040fe1dc86b62403c0b124f0d672ef8bfadb84d32d3R353

@nayihz I don’t want to nitpick too much, but to be honest I’m not sure why the interface change was required.
we have (today, before this PR) in datastore PodGetAll and PodList(predicate).
couldn’t we implement the “get pod with fresh metrics” with PodList(predicate == function to return only fresh pod)?

nirrozenbaum · 2025-07-01T14:15:49Z

I mean - to leave PodGetAll function as is.. and use the ListPod with that predicate only in the specific places it’s needed. would that help?

nayihz · 2025-07-02T02:45:15Z

to leave PodGetAll function as is.. and use the ListPod with that predicate only in the specific places it’s needed.

make sense to me.

nayihz · 2025-07-30T13:53:50Z

Most of the comments have been addressed. There are still two remaining comments that can be discussed further. @liu-cong

liu-cong · 2025-07-30T21:29:01Z

pkg/epp/datastore/datastore_test.go

 				},
 			},
+			storePods: []*corev1.Pod{pod1, pod2, pod3},
+			want:      []*backendmetrics.MetricsState{pod1Metrics, pod2Metrics}, // pod3 metrics were stale and should not be included.


The test is using a FakePodMetricsClient, you should be able to inject whatever test data you want. I don't think the current test is really testing what you want.

pkg/epp/saturationdetector/saturationdetector.go

nayihz · 2025-07-31T06:25:04Z

This PR involves changes to many files, so I kindly ask everyone to move forward as soon as possible. Otherwise, it's easy to encounter branch conflicts, which can sometimes take a lot of time to resolve.

liu-cong · 2025-07-31T20:51:06Z

Thank you for your patience @nayihz . I had two comments which are small I believe, I will lgtm once those are addressed. Thanks again!

nayihz · 2025-08-01T02:19:42Z

Oops, it conflicted again...😅

liu-cong · 2025-08-01T03:40:31Z

/lgtm

@nirrozenbaum or @kfswain mind taking another look? This PR became a pure refactor now, and the change of callers to take into consideration of the stale metrics will be in a follow up PR.

liu-cong · 2025-08-01T03:43:11Z

/hold

@nayihz Can you update the PR title to something like 'refactor PodList calls to prepare for making pod metrics staleness configurable"?

kfswain · 2025-08-01T16:27:16Z

/approve
/unhold

k8s-ci-robot · 2025-08-01T16:27:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kfswain, nayihz

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:

~~OWNERS~~ [kfswain]

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

ahg-g · 2025-08-03T20:02:04Z

pkg/epp/backend/metrics/types.go

@@ -28,16 +28,22 @@ import (
 	"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/backend"
 )

-func NewPodMetricsFactory(pmc PodMetricsClient, refreshMetricsInterval time.Duration) *PodMetricsFactory {
+var (
+	AllPodPredicate = func(PodMetrics) bool { return true }


typo? AllPodsPredicate?

ahg-g · 2025-08-03T20:06:49Z

pkg/epp/server/runserver.go

@@ -80,6 +81,7 @@ const (
 	DefaultCertPath                                 = ""                               // default for --cert-path
 	DefaultConfigFile                               = ""                               // default for --config-file
 	DefaultConfigText                               = ""                               // default for --config-text
+	DefaultMetricsStalenessThreshold                = 2 * time.Second


@liu-cong wasn't this originally 5 seconds (when it was named metricsValidityPeriod)?

Originally there is no threshold enforced (though we highlight metrics older than 5s in logs, never saw that happen though). This PR doesn't enforce the threshold either. But a followup PR is expected to enforce the threshold

nayihz · 2025-08-04T07:42:15Z

Create an issue to track this. @liu-cong
#1292

/lgtm

@nirrozenbaum or @kfswain mind taking another look? This PR became a pure refactor now, and the change of callers to take into consideration of the stale metrics will be in a follow up PR.

k8s-ci-robot added do-not-merge/work-in-progress cncf-cla: yes labels Jun 23, 2025

k8s-ci-robot requested review from ahg-g and robscott June 23, 2025 12:33

k8s-ci-robot added the size/L label Jun 23, 2025

nayihz force-pushed the feat_metric_stale_time branch from 12f8bfe to 2d42a53 Compare June 23, 2025 12:35

k8s-ci-robot added the needs-rebase label Jun 25, 2025

nayihz force-pushed the feat_metric_stale_time branch from a339897 to 1005486 Compare June 25, 2025 05:27

k8s-ci-robot removed the needs-rebase label Jun 25, 2025

nayihz force-pushed the feat_metric_stale_time branch from 1005486 to 9b1e7e2 Compare June 25, 2025 05:28

nayihz marked this pull request as ready for review June 25, 2025 09:22

k8s-ci-robot removed the do-not-merge/work-in-progress label Jun 25, 2025

k8s-ci-robot requested a review from danehans June 25, 2025 09:22

nayihz force-pushed the feat_metric_stale_time branch from 9b1e7e2 to 12943a7 Compare June 25, 2025 09:29

k8s-ci-robot requested a review from liu-cong June 25, 2025 09:48

liu-cong reviewed Jun 26, 2025

View reviewed changes

nayihz force-pushed the feat_metric_stale_time branch from 12943a7 to 518655c Compare June 29, 2025 07:11

nirrozenbaum reviewed Jun 29, 2025

View reviewed changes

pkg/epp/scheduling/scheduler.go Outdated Show resolved Hide resolved

nayihz force-pushed the feat_metric_stale_time branch from 518655c to e54be57 Compare June 29, 2025 13:31

k8s-ci-robot added the needs-rebase label Jun 30, 2025

nayihz force-pushed the feat_metric_stale_time branch 2 times, most recently from 6bde389 to bff4272 Compare July 2, 2025 02:38

k8s-ci-robot removed the needs-rebase label Jul 2, 2025

k8s-ci-robot added the needs-rebase label Jul 30, 2025

liu-cong reviewed Jul 30, 2025

View reviewed changes

nayihz force-pushed the feat_metric_stale_time branch from 48e5285 to 3dfb4d8 Compare July 31, 2025 03:22

k8s-ci-robot removed the needs-rebase label Jul 31, 2025

nayihz requested a review from liu-cong July 31, 2025 06:25

nayihz force-pushed the feat_metric_stale_time branch from 3dfb4d8 to ad0171a Compare August 1, 2025 02:07

k8s-ci-robot added the needs-rebase label Aug 1, 2025

feat: Make metrics stale time configurable

c677334

nayihz force-pushed the feat_metric_stale_time branch from ad0171a to c677334 Compare August 1, 2025 02:22

k8s-ci-robot removed the needs-rebase label Aug 1, 2025

k8s-ci-robot assigned liu-cong Aug 1, 2025

k8s-ci-robot added the lgtm label Aug 1, 2025

k8s-ci-robot added the do-not-merge/hold label Aug 1, 2025

nayihz changed the title ~~feat: Make metrics stale time configurable~~ cleanup: refactor PodList calls to prepare for making pod metrics staleness configurable Aug 1, 2025

k8s-ci-robot removed the do-not-merge/hold label Aug 1, 2025

k8s-ci-robot added the approved label Aug 1, 2025

k8s-ci-robot merged commit b1b43dc into kubernetes-sigs:main Aug 1, 2025
9 checks passed

nayihz deleted the feat_metric_stale_time branch August 2, 2025 07:16

ahg-g reviewed Aug 3, 2025

View reviewed changes

nayihz mentioned this pull request Aug 4, 2025

Allow specifying metricsStalenessThreshold in datastore.PodList predicate function #1292

Open

cleanup: refactor PodList calls to prepare for making pod metrics staleness configurable #1046

cleanup: refactor PodList calls to prepare for making pod metrics staleness configurable #1046

Uh oh!

Conversation

nayihz commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 23, 2025

Uh oh!

netlify bot commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for gateway-api-inference-extension ready!

Uh oh!

nayihz commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nayihz commented Jul 1, 2025

Uh oh!

nirrozenbaum commented Jul 1, 2025

Uh oh!

nirrozenbaum commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nayihz commented Jul 2, 2025

Uh oh!

nayihz commented Jul 30, 2025

Uh oh!

liu-cong Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nayihz commented Jul 31, 2025

Uh oh!

liu-cong commented Jul 31, 2025

Uh oh!

nayihz commented Aug 1, 2025

Uh oh!

liu-cong commented Aug 1, 2025

Uh oh!

liu-cong commented Aug 1, 2025

Uh oh!

kfswain commented Aug 1, 2025

Uh oh!

k8s-ci-robot commented Aug 1, 2025

Uh oh!

Uh oh!

ahg-g Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

ahg-g Aug 3, 2025

Choose a reason for hiding this comment

Uh oh!

liu-cong Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

nayihz commented Aug 4, 2025

Uh oh!

Uh oh!

nayihz commented Jun 23, 2025 •

edited

Loading

netlify bot commented Jun 23, 2025 •

edited

Loading

nirrozenbaum commented Jul 1, 2025 •

edited

Loading