-
Notifications
You must be signed in to change notification settings - Fork 102
feat(conformance): Add HTTPRouteMultipleGatewaysDifferentPools test #838
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
base: main
Are you sure you want to change the base?
feat(conformance): Add HTTPRouteMultipleGatewaysDifferentPools test #838
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hi @SinaChavoshi. 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. |
/cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/lgtm |
/ok-to-test |
/unhold |
periodSeconds: 5 | ||
failureThreshold: 2 | ||
env: | ||
- name: POD_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this var being used? If not, +1 to remove it.
periodSeconds: 5 | ||
failureThreshold: 2 | ||
env: | ||
- name: POD_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this var being used? If not, +1 to remove it.
appBackendNamespace = "gateway-conformance-app-backend" | ||
infraNamespace = "gateway-conformance-infra" | ||
backendAppLabelKey = "app" | ||
|
||
primaryGatewayName = "conformance-gateway" | ||
routeForPrimaryGWName = "route-for-primary-gateway" | ||
primaryPoolName = "primary-pool" | ||
primaryBackendLabel = "inference-model-1" | ||
primaryRoutePath = "/test-primary-gateway" | ||
|
||
secondaryGatewayName = "conformance-secondary-gateway" | ||
routeForSecondaryGWName = "route-for-secondary-gateway" | ||
secondaryPoolName = "secondary-pool" | ||
secondaryBackendLabel = "inference-model-2" | ||
secondaryRoutePath = "/test-secondary-gateway" | ||
secondaryRouteHostname = "secondary.example.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conformance/resources/manifests/manifests.yaml
defines resources that will be used across the tests, correct? If so, most of these locally defined consts should be public scope in the conformance/resources
or conformance/resources/manifests
pkg and imported into this test.
apiVersion: networking.gke.io/v1 | ||
kind: HealthCheckPolicy | ||
metadata: | ||
name: primary-pool-health-check | ||
namespace: gateway-conformance-app-backend | ||
spec: | ||
targetRef: | ||
group: "inference.networking.x-k8s.io" | ||
kind: InferencePool | ||
name: primary-pool | ||
default: | ||
config: | ||
type: HTTP | ||
httpHealthCheck: | ||
requestPath: / | ||
port: 3000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HealthCheckPolicy is implementation-specific and should not be included in this test.
apiVersion: networking.gke.io/v1 | ||
kind: HealthCheckPolicy | ||
metadata: | ||
name: secondary-pool-health-check | ||
namespace: gateway-conformance-app-backend | ||
spec: | ||
targetRef: | ||
group: "inference.networking.x-k8s.io" | ||
kind: InferencePool | ||
name: secondary-pool | ||
default: | ||
config: | ||
type: HTTP | ||
httpHealthCheck: | ||
requestPath: / | ||
port: 3000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
namespace: gateway-conformance-app-backend | ||
spec: | ||
selector: | ||
app: "inference-model-2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: quoting is unneeded
@@ -0,0 +1,91 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is part of #834 and should be removed here, correct?
@@ -0,0 +1,183 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is part of #834 and should be removed here, correct?
conformance/utils/config/timing.go
Outdated
InferencePoolMustHaveConditionTimeout: 300 * time.Second, | ||
InferencePoolMustHaveConditionInterval: 10 * time.Second, | ||
GatewayObjectPollInterval: 5 * time.Second, | ||
HTTPRouteDeletionReconciliationTimeout: 5 * time.Second, | ||
HTTPRouteConditionTimeout: 300 * time.Second, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this condition timeout is the same as InferencePoolMustHaveConditionTimeout
, consider using a single var that represents both resources, i.e. ConditionTimeout
.
This PR introduces a new conformance test, HTTPRouteMultipleGatewaysDifferentPools, which validates a scenario where two distinct HTTPRoute resources, parented by different Gateway resources, successfully reference and route traffic to separate InferencePool backends.
local run results: ( Ran on commit 5990c51 )