fix metric scrape port not updated when inference pool target port updated #417

Kuromesi · 2025-02-27T07:19:57Z

Currently the scrape port is stored in PodMetrics, which should be updated when targetPortNumber of inferencePool changed.

Also, currently the datastore stores all PodMetrics and return a reference of it when we try to read which in my opinion is not safe. This may cause inconsistency if we try to read and directly modify it in a goroutine. I think we should always return a copy and let the datastore control all write actions.

k8s-ci-robot · 2025-02-27T07:20:07Z

Hi @Kuromesi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2025-02-27T07:20:14Z

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

Name	Link
🔨 Latest commit	`e36719e`
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67c15c11794eed0008bb6fa2
😎 Deploy Preview	https://deploy-preview-417--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-02-27T15:45:04Z

pkg/epp/controller/inferencepool_reconciler.go

@@ -85,6 +85,13 @@ func (c *InferencePoolReconciler) updateDatastore(ctx context.Context, newPool *
 		//    the ones that may have existed already to the store.
 		c.Datastore.PodResyncAll(ctx, c.Client)
 	}
+	if oldPool != nil && newPool.Spec.TargetPortNumber != oldPool.Spec.TargetPortNumber {


I actually thinkw we should follow what we do here: https://github.com/Kuromesi/gateway-api-inference-extension/blob/186ac42b10c1c884cb024dc8de8f6c14d76b20be/pkg/epp/handlers/request.go#L111

This allows there to be a single source of truth (the pool port number) and accessing it simplifies keeping everything in sync.

If we make port number a param in: https://github.com/Kuromesi/gateway-api-inference-extension/blob/186ac42b10c1c884cb024dc8de8f6c14d76b20be/pkg/epp/datastore/types.go#L79-L80

We actually are able to remove the ScrapPort field entirely, and not need to worry about this func. Which I think simplifies the codebase quite a bit. WDYT?

I actually thinkw we should follow what we do here: https://github.com/Kuromesi/gateway-api-inference-extension/blob/186ac42b10c1c884cb024dc8de8f6c14d76b20be/pkg/epp/handlers/request.go#L111

+1

ahg-g · 2025-02-27T16:12:45Z

Also, currently the datastore stores all PodMetrics and return a reference of it when we try to read which in my opinion is not safe. This may cause inconsistency if we try to read and directly modify it in a goroutine. I think we should always return a copy and let the datastore control all write actions.

Not copying is by design, we list the pods frequently during probing, cloning frequently is going to be expensive at scale. The update there is strictly on the metrics status part, which should only be updated by the metrics probing routine.

I would keep this as is and add a giant comment on what parts of the PodMetric are updated when.

ahg-g · 2025-02-27T16:09:06Z

pkg/epp/datastore/datastore.go

-	// Update pod properties if anything changed.
-	existing.(*PodMetrics).Pod = new.Pod
-	return false
+func (ds *datastore) PodUpdate(f PodUpdateFunc, predicates ...PodListPredicate) {


where do we use this?

Kuromesi · 2025-02-27T23:23:10Z

Thanks guys! I agree, I'll revert my changes and repush a new commit.

…dated Signed-off-by: Kuromesi <blackfacepan@163.com>

hzxuzhonghu

/lgtm

But better add a unit test

ahg-g · 2025-02-28T01:10:22Z

pkg/epp/backend/provider.go

@@ -121,7 +121,8 @@ func (p *Provider) refreshMetricsOnce(logger logr.Logger) error {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			updated, err := p.pmc.FetchMetrics(ctx, existing)
+			pool, _ := p.datastore.PoolGet()


we need to check for the error here and skip updating if the pool wasn't set yet.

ahg-g · 2025-02-28T01:12:36Z

pkg/epp/backend/vllm/metrics.go

 ) (*datastore.PodMetrics, error) {
 	logger := log.FromContext(ctx)
 	loggerDefault := logger.V(logutil.DEFAULT)

 	// Currently the metrics endpoint is hard-coded, which works with vLLM.
 	// TODO(https://github.com/kubernetes-sigs/gateway-api-inference-extension/issues/16): Consume this from InferencePool config.
-	url := existing.BuildScrapeEndpoint()
+	url := existing.Address + ":" + strconv.Itoa(int(port))


we need to set the scrape path, /metrics

Sorry my bad.

Signed-off-by: Kuromesi <blackfacepan@163.com>

Kuromesi · 2025-02-28T01:56:12Z

/lgtm

But better add a unit test

Thanks, I'll see what I can do for the ut!

ahg-g · 2025-02-28T03:18:27Z

/ok-to-test

Signed-off-by: Kuromesi <blackfacepan@163.com>

Kuromesi · 2025-02-28T03:46:56Z

@ahg-g Sorry I got another small question, do you think it is more reasonable if we store the scrapePort and scrapePath in the metrics client?

type PodMetricsClientImpl struct{
	scrapePort int32
	scrapePath string
}
func (p *PodMetricsClientImpl) UpdateScrapeOptions(port int32, path string) {
	p.scrapePort = port
	p.scrapePath = path
}

So that we don't have to pass the port every time and keep the FetchMetrics tidy? 🤔

ahg-g · 2025-02-28T04:30:34Z

pkg/epp/backend/vllm/metrics.go

 ) (*datastore.PodMetrics, error) {
 	logger := log.FromContext(ctx)
 	loggerDefault := logger.V(logutil.DEFAULT)

 	// Currently the metrics endpoint is hard-coded, which works with vLLM.
 	// TODO(https://github.com/kubernetes-sigs/gateway-api-inference-extension/issues/16): Consume this from InferencePool config.
-	url := existing.BuildScrapeEndpoint()
+	url := existing.Address + ":" + strconv.Itoa(int(port)) + "/metrics"


I tested the PR, scraping failed. You need to add "http://" +, otherwise the scraping fails.

pkg/epp/backend/provider.go

ahg-g · 2025-02-28T05:29:44Z

/approve
/lgtm
/hold

If you can address one last comment, then that would be great. otherwise feel free to unhold and we can address in a followup.

k8s-ci-robot · 2025-02-28T05:29:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, Kuromesi

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~~ [ahg-g]

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-02-28T06:30:54Z

pkg/epp/backend/vllm/metrics.go

+func (p *PodMetricsClientImpl) UpdateScrapeOptions(port int32, path string) {
+	p.scrapePort = port
+	p.scrapePath = path
+}


what are those for?

Sorry I accidentally push some local changes...

Signed-off-by: Kuromesi <blackfacepan@163.com>

ahg-g · 2025-02-28T15:07:25Z

/hold cancel
/lgtm

Thanks

…dated (kubernetes-sigs#417) * fix metric scrape port not updated when inference pool target port updated Signed-off-by: Kuromesi <blackfacepan@163.com> * bug fix Signed-off-by: Kuromesi <blackfacepan@163.com> * fix ut Signed-off-by: Kuromesi <blackfacepan@163.com> * add log Signed-off-by: Kuromesi <blackfacepan@163.com> --------- Signed-off-by: Kuromesi <blackfacepan@163.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 27, 2025

k8s-ci-robot requested review from kfswain and robscott February 27, 2025 07:20

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 27, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 27, 2025

Kuromesi force-pushed the poolfix branch from 7945724 to baf6c37 Compare February 27, 2025 08:59

kfswain reviewed Feb 27, 2025

View reviewed changes

ahg-g reviewed Feb 27, 2025

View reviewed changes

fix metric scrape port not updated when inference pool target port up…

0789e84

…dated Signed-off-by: Kuromesi <blackfacepan@163.com>

Kuromesi force-pushed the poolfix branch from 186ac42 to 0789e84 Compare February 28, 2025 00:55

hzxuzhonghu reviewed Feb 28, 2025

View reviewed changes

k8s-ci-robot assigned hzxuzhonghu Feb 28, 2025

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

ahg-g reviewed Feb 28, 2025

View reviewed changes

bug fix

3dee812

Signed-off-by: Kuromesi <blackfacepan@163.com>

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

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 28, 2025

fix ut

67d0654

Signed-off-by: Kuromesi <blackfacepan@163.com>

ahg-g reviewed Feb 28, 2025

View reviewed changes

pkg/epp/backend/provider.go 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 28, 2025

k8s-ci-robot assigned ahg-g Feb 28, 2025

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

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 28, 2025

ahg-g reviewed Feb 28, 2025

View reviewed changes

add log

e36719e

Signed-off-by: Kuromesi <blackfacepan@163.com>

Kuromesi force-pushed the poolfix branch from 368f0c3 to e36719e Compare February 28, 2025 06:47

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 28, 2025

k8s-ci-robot merged commit 0d08a07 into kubernetes-sigs:main Feb 28, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix metric scrape port not updated when inference pool target port updated #417

fix metric scrape port not updated when inference pool target port updated #417

Kuromesi commented Feb 27, 2025

k8s-ci-robot commented Feb 27, 2025

netlify bot commented Feb 27, 2025 •

edited

Loading

kfswain Feb 27, 2025

ahg-g Feb 27, 2025

ahg-g commented Feb 27, 2025

ahg-g Feb 27, 2025

Kuromesi commented Feb 27, 2025

hzxuzhonghu left a comment

ahg-g Feb 28, 2025

ahg-g Feb 28, 2025

Kuromesi Feb 28, 2025

Kuromesi commented Feb 28, 2025

ahg-g commented Feb 28, 2025

Kuromesi commented Feb 28, 2025

ahg-g Feb 28, 2025 •

edited

Loading

Kuromesi Feb 28, 2025

ahg-g commented Feb 28, 2025

k8s-ci-robot commented Feb 28, 2025

ahg-g Feb 28, 2025

Kuromesi Feb 28, 2025

ahg-g commented Feb 28, 2025

fix metric scrape port not updated when inference pool target port updated #417

fix metric scrape port not updated when inference pool target port updated #417

Conversation

Kuromesi commented Feb 27, 2025

k8s-ci-robot commented Feb 27, 2025

netlify bot commented Feb 27, 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

ahg-g commented Feb 27, 2025

Choose a reason for hiding this comment

Kuromesi commented Feb 27, 2025

hzxuzhonghu left a 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

Kuromesi commented Feb 28, 2025

ahg-g commented Feb 28, 2025

Kuromesi commented Feb 28, 2025

ahg-g Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Feb 28, 2025

k8s-ci-robot commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Feb 28, 2025

netlify bot commented Feb 27, 2025 •

edited

Loading

ahg-g Feb 28, 2025 •

edited

Loading