Skip to content

Adds Initial e2e Tests and Tooling #217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 28, 2025

Conversation

danehans
Copy link
Contributor

Adds initial e2e tests and tooling. The e2e tests can be run by using the test-e2e target. The "HF_TOKEN" environment variable must be set to your Hugging Face access token (with access to Llama2 model) before running the tests.

  • api/v1alpha1/inferencemodel_types.go and api/v1alpha1/inferencepool_types.go: Adds constants to define resource and kind.
  • Bumps go deps.
  • pkg/crd/install.go: Adds public function for installing CRDs. Used by e2e suite to setup test infra.
  • pkg/crd/install_test.go and pkg/crd/mocks/mock_client.go: Adds unit tests and mock client for InstallCRDs function.
  • test/consts/consts.go: Defines constants used by tests.
  • test/e2e/e2e_suite_test.go: Creates initial test suite, test infra, utility functions, etc.
  • test/e2e/e2e_test.go: Adds initial e2e test by creating an InferenceModel and ensuring that requests are properly load balanced across the target models.
  • test/utils/resources.go: Utility functions for managing Kubernetes resources used for e2e tests.
  • test/utils/utils.go: Provides utility functions for working with test resources.
  • test/utils/wrappers.go: Provides wrappers for working with test resources.

Fixes #77

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 23, 2025
Copy link

netlify bot commented Jan 23, 2025

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

Name Link
🔨 Latest commit 2319c1f
🔍 Latest deploy log https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67992fcb606c100008ac33a9
😎 Deploy Preview https://deploy-preview-217--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

One concern I have is that we are coding a lot of things that could have been done easier with kubectl and a shell script, specifically: deploying the crds, the envoy proxy and the epp. What I am thinking is that why not have a script that deploys everything as described in our guide, then in go we verify that everything is up and running (via a BeforeSuite) and runs the test client.

There are a couple of advantages to this approach:

  1. significantly simplify the golang code
  2. reduce duplication since we will be reusing the manifests we have for the user guide
  3. practically this e2e test is a test for the guide as well!

what do you think?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2025
pkg/README.md Outdated

```sh
kubectl apply -f config/crd/bases
Replace `$HF_TOKEN` in `./manifests/vllm/deployment.yaml` with your Hugging Face secret and then deploy the sample vLLM deployment.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step was updated since the e2e test suite now deploys vLLM test infra resources from this manifest by:

  • Reading the manifest file into memory
  • Reading the HF_TOKEN env var
  • Replacing $HF_TOKEN with the HF_TOKEN env var in the in-memory manifest
  • Deploying the resources

```bash
kubectl apply -f ./manifests/inferencepool-with-model.yaml
kubectl apply -f ./manifests/inferencemodel.yaml
```
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the InferencePool CR is now part of the ext-proxy deployment. The InferenceModel had to be split from the InferencePool since e2e tests will create InferenceModels for each test case. The InferenceModel could have its own manifest but IMHO it makes sense to bundle with the ext-proc since the ext-proc is specifically configured for this InferencePool.


1. **Deploy Ext-Proc**
1. **Deploy the Inference Extension and InferencePool**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned above, the InferencePool is now bundled with the ext-proc since the ext-proc is specifically configured for this pool.

labels:
app: vllm
stringData:
token: $HF_TOKEN
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2e will replace this variable and the POC instructions have been updated accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have this Secret resource in a separate yaml so that the guide doesn't require the user to clone the repo to test it out.

@danehans
Copy link
Contributor Author

@ahg-g I updated the e2e test suite to use a "golden manifest" approach. This approach provides the following benefits:

  • Single Flow & Simplified Maintenance: A single test suite that runs both the “deploy” and “test” steps, making it simpler to maintain and debug.
  • Deterministic, Self-Contained Tests: The test harness ensures that each test run has the same environment—no reliance on manual or separate script-driven steps.
  • Better Control of Race Conditions & Ordering: Runs custom wait logic (or test verifications) between each resource’s creation. Scripts typically proceed line by line without advanced logic unless heavily scripted.
  • Native Go Logging, Assertions, & Error Handling: Failures integrate seamlessly with the test harness (e.g., Ginkgo, testing.T), producing richer debug output.

Copy link
Contributor

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, can we add a readme how to run this test manually pls?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2025
@danehans danehans requested a review from ahg-g January 28, 2025 17:47
labels:
app: vllm
stringData:
token: $HF_TOKEN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have this Secret resource in a separate yaml so that the guide doesn't require the user to clone the repo to test it out.

@ahg-g
Copy link
Contributor

ahg-g commented Jan 28, 2025

Amazing!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2025
@k8s-ci-robot k8s-ci-robot merged commit 16b95a2 into kubernetes-sigs:main Jan 28, 2025
8 checks passed
@danehans danehans deleted the issue_77 branch January 28, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup at least one e2e test
3 participants