Migrate to consolidated e2e test suite#760
Conversation
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
3d2cb38 to
40d97af
Compare
gger ci Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
|
/ok-to-test |
|
🚀 E2E tests triggered by /ok-to-test |
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
a6175b4 to
04e081e
Compare
| else | ||
| log_success "Successfully pulled image '$WVA_IMAGE_REPO:$WVA_IMAGE_TAG' from registry" | ||
| # Try to pull the image, or use local image if pull fails | ||
| if ! docker pull "$WVA_IMAGE_REPO:$WVA_IMAGE_TAG"; then |
There was a problem hiding this comment.
what's the usecase for pulling wva image from a (I suppose) remote repository?
There was a problem hiding this comment.
This is for testing a released version without building locally, if I understand your question correctly.
| ) | ||
|
|
||
| // Constants for MetricsAvailable condition | ||
| // Note: Reasons should match api/v1alpha1 constants for consistency |
There was a problem hiding this comment.
what about moving the 4 constants below to llmdVariantAutoscalingV1alpha1? Less code that way
| ) | ||
|
|
||
| // CreateHPA creates a HorizontalPodAutoscaler resource for WVA integration | ||
| func CreateHPA( |
There was a problem hiding this comment.
People familiar with the k8s API verbs would expect Create to fail if the object already exists. It may be a source of confusion. What about having (at least) 3 functions: CreateHPA, DeleteHPA and EnsureHPA? The 2 first functions match the semantic of the k8s API verbs, and the third one is a convenient function calling lower-level function. If you agree then make sure to also update the other builder functions
lionelvillard
left a comment
There was a problem hiding this comment.
LGTM, thanks! I added minor comments.
| // InferencePool compatibility via llm-d.ai/model-pool label. | ||
| // This function is idempotent: it will delete any existing deployment with the same name | ||
| // before creating a new one to handle leftover resources from previous test runs. | ||
| func CreateModelService(ctx context.Context, k8sClient *kubernetes.Clientset, namespace, name, poolName, modelID string, useSimulator bool, maxNumSeqs int) error { |
There was a problem hiding this comment.
I would put this builder function in model_service_builder.go for consistency reason.
| } | ||
|
|
||
| // LoadConfigFromEnv reads e2e test configuration from environment variables | ||
| func LoadConfigFromEnv() E2EConfig { |
There was a problem hiding this comment.
maybe consider return a pointer? Not very important.
There was a problem hiding this comment.
yes, the struct isn't large enough to justify a pointer. Better keep the current code simple and clear
|
|
||
| // Helper functions for environment variable parsing | ||
|
|
||
| func getEnv(key, defaultValue string) string { |
There was a problem hiding this comment.
I'm not 100% sure but all getEnvXX function can be replaced by one generic function
There was a problem hiding this comment.
yes, avoiding verbosity there, and the intent is to keep it clear and explicit.
| // Feature gate defaults | ||
| ScaleToZeroEnabled: getEnvBool("SCALE_TO_ZERO_ENABLED", false), | ||
|
|
||
| // EPP defaults |
There was a problem hiding this comment.
I would be cleaner (IMO) to create "stacks" (InferencePool, VA, ModelService, etc..) when running the tests, to not duplicate this logic. Eventually e2e tests may be deploying helm releases, one per stack. This is beyond the scope of this PR, just something to keep in mind.
There was a problem hiding this comment.
Yes, good suggestion.
asm582
left a comment
There was a problem hiding this comment.
/lgtm
We need to evaluate the use of hermetic tests and remove redundant tests in the subsequent PRs.
|
Thanks for the review, merging this pr to unblock other work. Kept a note of some of the relevant comments. Will address them in subsequent pr. |
Summary
Consolidated e2e test suite (
test/e2e/) infrastructuretest-e2e-smoke-with-setupautomatically on PRs with code changestest-e2eandtest-e2e-openshiftMakefile targets