Use dedicated namespace for e2e test code #661
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @rramkumar1. 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/L
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -0,0 +1,126 @@ | |||
| apiVersion: inference.networking.x-k8s.io/v1alpha2 | |||
There was a problem hiding this comment.
This looks like a duplication of the existing inferencepool-resources.yaml, excluding the namespace field.
I would prefer if we can remove the namespace field from all resources in the inferencepool-resources.yaml file and specify the namespace in the e2e test (as env var as specified in other comment).
there is a big value in testing e2e our "Getting Started" files, so we are self testing the public documentation.
I would avoid having duplications as much as possible.
There was a problem hiding this comment.
How would you suggest getting the env var parsed in the YAML file?
There was a problem hiding this comment.
I suppose I could use a similar approach to the one you described above. However, that might complicate the getting started guide? How would the user of the guide make sure the env var was parsed in the file?
There was a problem hiding this comment.
in the e2e a similar approach would work. in getting started guide I’m not sure we have an agreement of the other maintainers about being able to choose the namespace (I’m in favor of it).
if there is an agreement, you can use envsubst command to replace env var with it’s value.
for example -
envsubst < myfile.yaml | kubectl apply -f -
this discussion is relevant only for the subject serviceaccount in the ClusterRoleBinding, as the namespace in the Service and Deployment resources can be just removed and then you can decide the ns during e2e test.
There was a problem hiding this comment.
Should we tackle this in a follow-up then? If we change the getting started file in this PR its going to expand the scope considerably as we now need to update the guide with the envvar substitution command.
cc @danehans
There was a problem hiding this comment.
If we change the getting started file in this PR
+1 on not changing the getting started guide in this PR.
There was a problem hiding this comment.
there is a big value in testing e2e our "Getting Started" files, so we are self testing the public documentation.
I agree with this point. We may need to consider 2 different e2e tests, 1 for testing the quickstart and another that is more programmatic to support various inference use cases. Maybe we track this in a separate issue?
There was a problem hiding this comment.
I prefer to move away from yaml files and just create those objects in code,,, the envoy one is complex, so I don't mind it, but other than that I think it is more manageable to have the test create those objects in code.
There was a problem hiding this comment.
oh, well, creating the pool requires also creating all the epp and the rbac resources, so it is also complex...
rramkumar1
force-pushed
the
e2e-improvements
branch
from
ea6558e to
d5978ba
Compare
|
@rramkumar1 CI is failing due to: test/e2e/epp/e2e_suite_test.go:59:98: `overriden` is a misspelling of `overridden` (misspell)
// defaultNsName is the default name of the Namespace used for tests. The namespace used can be overriden by the E2E_NS environment variable.
^
make: *** [Makefile:144: ci-lint] Error 1 |
rramkumar1
force-pushed
the
e2e-improvements
branch
from
d5978ba to
503394a
Compare
Fixed. |
rramkumar1
force-pushed
the
e2e-improvements
branch
from
503394a to
fc47c08
Compare
| @@ -0,0 +1,126 @@ | |||
| apiVersion: inference.networking.x-k8s.io/v1alpha2 | |||
There was a problem hiding this comment.
there is a big value in testing e2e our "Getting Started" files, so we are self testing the public documentation.
I agree with this point. We may need to consider 2 different e2e tests, 1 for testing the quickstart and another that is more programmatic to support various inference use cases. Maybe we track this in a separate issue?
rramkumar1
force-pushed
the
e2e-improvements
branch
from
fc47c08 to
b1efa3b
Compare
| @@ -109,6 +115,8 @@ var _ = ginkgo.BeforeSuite(func() { | |||
| }) | |||
|
|
|||
| func setupInfra() { | |||
| createNamespace(cli, nsName) | |||
There was a problem hiding this comment.
can you add the namespace deletion to AfterSuite cleanup?
all resources that are created in the test should be deleted at the end of it.
There was a problem hiding this comment.
I believe it's already done here:
I do observe the NS being cleaned up.
|
/lgtm Thanks! |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, rramkumar1 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
Fixed #227
/assign @danehans
See results of E2E run below using the CPU-based vLLM: