helm: add NAMESPACE env var to EPP deployment for pool namespace #1610
Conversation
k8s-ci-robot
added
the
kind/feature
✅ 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
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @jyizheng. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
size/XS
| pullPolicy: Always | ||
| extProcPort: 9002 | ||
| env: [] | ||
| # Environment variables for the EPP container. |
There was a problem hiding this comment.
pls move this to https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/config/charts/inferencepool/templates/epp-deployment.yaml and remove the flag we set at https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/config/charts/inferencepool/templates/epp-deployment.yaml#L28
There was a problem hiding this comment.
I suggest adding the flag back so that helm can work with older versions of EPP, or a different distro of EPP such as inference scheduler, which may not have implemented the NAMESPACE env var fallback. Sent a PR #1633
There was a problem hiding this comment.
One can set the flag explicitly in https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/config/charts/inferencepool/values.yaml#L34, right?
There was a problem hiding this comment.
Yes this PR is fully optional, but I have heard multiple people (me included) using latest helm and got EPP crashlooping due to missing this. And it's not super obvious (the error will be some SA permission error due to the mismatched namespace). I think this is a nice to have even if we have the NAMESPACE var.
jyizheng
force-pushed
the
add-namespace-env-to-helm
branch
from
a8a66f2 to
14a884e
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, jyizheng 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
There is an open issue for it
Which issue(s) this PR fixes:
#1569
Fixes #
Does this PR introduce a user-facing change?: