fix: log warning when pod ports don't match InferencePool targetPorts#2582
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @hexfusion. 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 Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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
added
size/M
|
|
||
| _ = ds.PoolSet(context.Background(), fakeClient, poolutil.InferencePoolToEndpointPool(inferencePool)) | ||
| _ = ds.PodUpdateOrAddIfNotExist(pod1) | ||
| _ = ds.PodUpdateOrAddIfNotExist(context.Background(), pod1) |
There was a problem hiding this comment.
Q: other test used t.Context(), any preferrence to that vs contex.Background()?
There was a problem hiding this comment.
just following the style from the line above it, and what I saw in general seems to be a mixed usage. i do not have strong feeling as long as its serving same purpose. would you advise adjusting all instances?
There was a problem hiding this comment.
I see. Ultimately we would want consistency. I don't feel strongly enough to do it in this PR.
Will let others weigh in.
There was a problem hiding this comment.
no problem adjusting all tests to ensure consistency if we have consensus
| } | ||
| for _, pod := range test.storePods { | ||
| ds.PodUpdateOrAddIfNotExist(pod) | ||
| ds.PodUpdateOrAddIfNotExist(context.Background(), pod) |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve no need to block the PR on context consistency across tests. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hexfusion, nirrozenbaum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…kubernetes-sigs/gateway-api-inference-extension#2582) Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…kubernetes-sigs/gateway-api-inference-extension#2582) Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Adds a warning log when a pod matches an InferencePool's label selector but none of its container ports intersect with the pool's
targetPorts. Previously the pod was silently ignored with no diagnostic output.Also threads
ctxthroughPodUpdateOrAddIfNotExistto follow the controller-runtimelog.FromContext(ctx)pattern consistently.Which issue(s) this PR fixes:
Fixes #2562
Does this PR introduce a user-facing change?: