introduce a general purpose pod scraper and add models scraper #783

nirrozenbaum · 2025-05-05T12:36:34Z

This PR lays the ground for pod scraper extensibility (part of the data layer extensibility).
it's still not pluggable or extensible, but the PR shows a clear way for doing that (see in main.go the initialization of the PodScraperFactory).
the PR converts the existing metrics scraper to the new format and adds models scraper for /v1/models according to OpenAI models API schema.

pay attention that the intention in the models scraper is NOT multiple base models.
it is intended for a single base model, BUT with a different set of LoRA adapters on different pods.

still WIP, but pushing a PR to start collecting feedback on the general direction.

EDIT:
the PR is now ready for final review after fixing all tests and finalizing the structures.
two non blocking points that will be handled in a follow up PR:

pod scraper should not start a go routine per scraper. all scrapers should consolidate to a single go routine per pod.
when reading ScrapedData using the GetData() function from PodInfo, current code requires type infer. need to think if this can be cleaner.

k8s-ci-robot · 2025-05-05T12:36:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nirrozenbaum
Once this PR has been reviewed and has the lgtm label, please assign terrytangyuan 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

netlify · 2025-05-05T12:36:59Z

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

Name	Link
🔨 Latest commit	`77bc048`
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/681b2ef7a7384200086913df
😎 Deploy Preview	https://deploy-preview-783--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 site configuration.

kfswain · 2025-05-05T14:58:41Z

cmd/epp/main.go

 	"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling"
 	runserver "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/server"
 	"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/util/logging"
 )

 const (
 	defaultMetricsEndpoint = "/metrics"
+	modelsScrapeInterval   = 30 * time.Second


nit: if we are moving these already, should we make them configurable? That's okay if we want to hold off on doing that, we already have a breathtaking amount of config.

yes, it should go to config. if there is no objection I'll do in a follow up PR.
prefer to finalize this PR and get it merged, otherwise I'll hit rebase issues every day (since it touches many files).

kfswain · 2025-05-05T15:06:11Z

cmd/epp/main.go

@@ -38,16 +39,20 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/filters"
 	"sigs.k8s.io/gateway-api-inference-extension/internal/runnable"
-	backendmetrics "sigs.k8s.io/gateway-api-inference-extension/pkg/epp/backend/metrics"
+	"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/backend/metrics"


I wonder if we should refer to this package as metricscollection or metricsingress to make clear the difference between the collector package and the metric emission/egress package

yes. this is a bit confusing.
I left the namings as it was, but I agree.
I'm suggesting to postpone the a follow up due to the PR size and the large number of files it touches.

docs/proposals/003-model-server-protocol/README.md

pkg/epp/backend/metrics/logger.go

kfswain · 2025-05-05T15:45:20Z

pkg/epp/backend/metrics/metrics_scraper.go

 	var errs error
-	updated := existing.Clone()
+	updated := s.existingMetrics.Clone().(*Metrics)


why do we need to type infer?

I see that this has a generic return type, I assume b/c of the way the interface was defined. I wonder if we can convert the clone func to a generic. It seems odd to have to type infer on a func calledClone as that implies it wasn't quite a clone.

yes, wrote in other comment as well.
still not 100% sure about the ScrapeResult which needs to go through another iteration for sure.
just wanted to make progress with this PR.

pkg/epp/backend/scrapers/pod_scraper.go

kfswain · 2025-05-05T18:12:50Z

pkg/epp/scheduling/plugins/scorer/kvcache.go

@@ -29,7 +30,8 @@ func (ss *KVCacheScorer) Name() string {
 func (ss *KVCacheScorer) Score(ctx *types.SchedulingContext, pods []types.Pod) map[types.Pod]float64 {
 	scores := make(map[types.Pod]float64, len(pods))
 	for _, pod := range pods {
-		scores[pod] = 1 - pod.GetMetrics().KVCacheUsagePercent
+		podMetrics := pod.GetData()[metrics.MetricsDataKey].(*metrics.Metrics)


nit: this seems like the primary way to access data; GetData()[metrics.MetricsDataKey].(*metrics.Metrics) seems like a code smell to me. Non-blocking, we can noodle on this overtime, but something seems amiss

yes, agreed.
this needs to be better defined.
I'm still not 100% about the ScrapeResult which is used as general purpose return value from Scrape.
eventually when accessing the data, we need to know the type for accessing the specific fields.

I'm still trying to think about a better way, but considering the fact that most part are generalized, I wanted to make progress and try to finalize this in next iteration of the pod scrapers.

nirrozenbaum · 2025-05-06T08:05:13Z

@kfswain got an excellent pointer from Abdullah in the Prefix scorer PR (was send in a different context), which seems very related to this PR.

Kube-scheduler has an interface called CycleState that serves as an abstract parameters map that is passed to plugins during the scheduling cycle.
See CycleState concept in kube-scheduler: https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/cycle_state.go#L32 and https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/interface.go#L556

the reason for using an abstract map is the ability to be pluggable & extensible.

this is kind of identical to the ScrapeResult interface with the Clone function. Pay attention that everywhere where the CycleState is used is doing a type infer, cause it's a general purpose parameters map. similarly, in this PR, ScrapeResult is a general purpose result of scrape, and when one wants to use it, there is a need for type infer.

few usage examples in kube-scheduler plugins (with type infer):

LMKWYT.

kfswain · 2025-05-06T15:29:35Z

@kfswain got an excellent pointer from Abdullah in the Prefix scorer PR (was send in a different context), which seems very related to this PR.

Kube-scheduler has an interface called CycleState that serves as an abstract parameters map that is passed to plugins during the scheduling cycle. See CycleState concept in kube-scheduler: https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/cycle_state.go#L32 and https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/interface.go#L556

the reason for using an abstract map is the ability to be pluggable & extensible.

this is kind of identical to the ScrapeResult interface with the Clone function. Pay attention that everywhere where the CycleState is used is doing a type infer, cause it's a general purpose parameters map. similarly, in this PR, ScrapeResult is a general purpose result of scrape, and when one wants to use it, there is a need for type infer.

few usage examples in kube-scheduler plugins (with type infer):

https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/examples/multipoint/multipoint.go#L81

https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go#L214-L224

https://github.com/kubernetes/kubernetes/blob/master/pkg/scheduler/framework/plugins/nodeaffinity/node_affinity.go#L341-L350

LMKWYT.

Yeah, im totally with you there, I'm just suggesting we use a generic to genericize the type infer and hide it from usage as much as we can. See my play example here: https://go.dev/play/p/FAE2ZIH_xRt

Signed-off-by: Nir Rozenbaum <[email protected]>

includes adapting the changes in all tests Signed-off-by: Nir Rozenbaum <[email protected]>

nirrozenbaum · 2025-05-07T10:43:00Z

the PR is now ready for final review after fixing all tests and finalizing the structures.
two non blocking points that will be handled in a follow up PR:

pod scraper should not start a go routine per scraper. all scrapers should consolidate to a single go routine per pod.
when reading ScrapedData using the GetData() function from PodInfo, current code requires type infer. need to think if this can be cleaner.

updated these points in the PR description as well.

k8s-ci-robot · 2025-05-07T19:31:31Z

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

nirrozenbaum · 2025-05-12T05:26:26Z

For easier and simpler review, will break this PR into a series of smaller PRs.
can be closed for now.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 5, 2025

k8s-ci-robot requested review from ahg-g and robscott May 5, 2025 12:36

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 5, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2025

kfswain reviewed May 5, 2025

View reviewed changes

nirrozenbaum force-pushed the model-scraper branch from aa301d2 to 0be970f Compare May 6, 2025 17:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2025

nirrozenbaum added 2 commits May 7, 2025 11:39

updated moder server protocal with Models api

b2ca65f

Signed-off-by: Nir Rozenbaum <[email protected]>

initial model scraper implenetation, still not used in datastore

afffca0

Signed-off-by: Nir Rozenbaum <[email protected]>

nirrozenbaum force-pushed the model-scraper branch from a559f7b to bd8fa4c Compare May 7, 2025 08:50

general purpose pod scraper and implementation of models scraper.

77bc048

includes adapting the changes in all tests Signed-off-by: Nir Rozenbaum <[email protected]>

nirrozenbaum force-pushed the model-scraper branch from 5c04425 to 77bc048 Compare May 7, 2025 09:59

nirrozenbaum changed the title ~~[WIP] introducing a general purpose pod scraper and implement models scraper~~ introduce a general purpose pod scraper and add models scraper May 7, 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 May 7, 2025

nirrozenbaum requested a review from kfswain May 7, 2025 10:43

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2025

nirrozenbaum closed this May 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce a general purpose pod scraper and add models scraper #783

introduce a general purpose pod scraper and add models scraper #783

nirrozenbaum commented May 5, 2025 •

edited

Loading

k8s-ci-robot commented May 5, 2025

netlify bot commented May 5, 2025 •

edited

Loading

kfswain May 5, 2025

nirrozenbaum May 5, 2025

kfswain May 5, 2025

kfswain May 5, 2025

nirrozenbaum May 5, 2025 •

edited

Loading

kfswain May 5, 2025

kfswain May 5, 2025

nirrozenbaum May 5, 2025

kfswain May 5, 2025

nirrozenbaum May 5, 2025

nirrozenbaum commented May 6, 2025 •

edited

Loading

kfswain commented May 6, 2025

nirrozenbaum commented May 7, 2025

k8s-ci-robot commented May 7, 2025

nirrozenbaum commented May 12, 2025

introduce a general purpose pod scraper and add models scraper #783

introduce a general purpose pod scraper and add models scraper #783

Conversation

nirrozenbaum commented May 5, 2025 • edited Loading

k8s-ci-robot commented May 5, 2025

netlify bot commented May 5, 2025 • edited Loading

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

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

nirrozenbaum May 5, 2025 • edited Loading

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

nirrozenbaum commented May 6, 2025 • edited Loading

kfswain commented May 6, 2025

nirrozenbaum commented May 7, 2025

k8s-ci-robot commented May 7, 2025

nirrozenbaum commented May 12, 2025

nirrozenbaum commented May 5, 2025 •

edited

Loading

netlify bot commented May 5, 2025 •

edited

Loading

nirrozenbaum May 5, 2025 •

edited

Loading

nirrozenbaum commented May 6, 2025 •

edited

Loading