conformance tests for percentage-based request mirroring #3508

LiorLieberman · 2024-12-17T02:29:28Z

What type of PR is this?
/kind test
/area conformance

What this PR does / why we need it:
Conformance tests for #3171 plus small updates to the GEP.
Tested with istio 1.24.1

Note: I also added a test case for fraction but it looks like it has an issue currently with the implementation I used (istio).

The specific test is failing, (which is good) so I think its in a good merge state. I also opened istio/istio#54357.

Mesh tests to follow

Which issue(s) this PR fixes:
Fixes #3171

Does this PR introduce a user-facing change?:

NONE

/cc @jakebennert @robscott

LiorLieberman · 2024-12-17T03:30:53Z

/cc @jakebennert @robscott

k8s-ci-robot · 2024-12-17T03:30:56Z

@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: jakebennert.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jakebennert @robscott

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.

robscott

Thanks @LiorLieberman! This is really well done

conformance/tests/httproute-request-percentage-mirror.go

geps/gep-3171/index.md

robscott · 2024-12-18T07:18:20Z

/cc @dprotaso

LiorLieberman · 2024-12-20T01:40:35Z

/cc @kflynn

LiorLieberman · 2025-01-07T03:43:11Z

ping @kflynn @dprotaso
/cc @mikemorris

mlavacca

Thanks for the PR, @LiorLieberman!

pkg/features/httproute.go

conformance/tests/httproute-request-percentage-mirror.go

mlavacca · 2025-01-10T14:18:41Z

conformance/tests/httproute-request-percentage-mirror.go

+				http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expected)
+
+				// Override to not have more requests than expected
+				suite.TimeoutConfig.RequiredConsecutiveSuccesses = 1


This change will affect other tests as well. Why do we need to do so?

oh man.. there is a whole story behind this change.
Essentially since I test mirroring, I started to see more requests that I had actually sent. so the mirror expectations were totally wrong. I learned to find this config the hard way..

Any other suggestion here if this is not desired?

yes, this var defaults to 3:

gateway-api/conformance/utils/config/timeout.go

Line 128 in 082cc90

RequiredConsecutiveSuccesses: 3,

Patching this in place is problematic, as it changes the user configuration and affects many other tests as well. Can we instead copy the whole TimeoutConfig to a new var and change that to our desired value? It should be feasible, as http.MakeRequestAndExpectEventuallyConsistentResponse accepts a Config as a parameter.

conformance/tests/httproute-request-percentage-mirror.go

k8s-ci-robot · 2025-01-10T22:19:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LiorLieberman
Once this PR has been reviewed and has the lgtm label, please assign danwinship for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

LiorLieberman · 2025-01-13T19:22:18Z

@sayboras Can you help testing if CIlium passes this test?

/cc @sayboras

k8s-ci-robot · 2025-01-13T19:22:21Z

@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: sayboras.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@sayboras Can you help testing if CIlium passes this test?

/cc @sayboras

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.

mlavacca · 2025-01-23T10:50:55Z

conformance/tests/httproute-request-percentage-mirror.go

@@ -0,0 +1,234 @@
+/*
+Copyright 2024 The Kubernetes Authors.


Suggested change

Copyright 2024 The Kubernetes Authors.

Copyright 2025 The Kubernetes Authors.

mlavacca · 2025-01-23T10:51:37Z

conformance/tests/httproute-request-percentage-mirror.go

+const (
+	concurrentRequests    = 10
+	tolerancePercentage   = 5.0
+	totalRequests         = 500.0


nit: I think we can make this an int

mlavacca · 2025-01-23T11:05:03Z

conformance/tests/httproute-request-percentage-mirror.go

+				http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expected)
+
+				// Override to not have more requests than expected
+				suite.TimeoutConfig.RequiredConsecutiveSuccesses = 1


yes, this var defaults to 3:

gateway-api/conformance/utils/config/timeout.go

Line 128 in 082cc90

RequiredConsecutiveSuccesses: 3,

Patching this in place is problematic, as it changes the user configuration and affects many other tests as well. Can we instead copy the whole TimeoutConfig to a new var and change that to our desired value? It should be feasible, as http.MakeRequestAndExpectEventuallyConsistentResponse accepts a Config as a parameter.

mlavacca · 2025-01-23T11:06:08Z

conformance/tests/httproute-request-percentage-mirror.go

+					wg.Wait()
+					if err := testMirroredRequestsDistribution(t, suite, expected, timeNow); err != nil {
+						t.Logf("Traffic distribution test failed (%d/%d): %s", k+1, numDistributionChecks, err)
+						time.Sleep(2 * time.Second)


Why do we need such a sleep here?

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 17, 2024

k8s-ci-robot requested review from michaelbeaumont and mlavacca December 17, 2024 02:29

k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 17, 2024

LiorLieberman force-pushed the percent-based-mirror branch 3 times, most recently from 117f34b to d63225c Compare December 17, 2024 03:12

LiorLieberman changed the title ~~initial conformance tests for percent based mirroring~~ conformance tests for percentage-based request mirroring Dec 17, 2024

conformance tests for percentage-based request mirroring

15db255

LiorLieberman force-pushed the percent-based-mirror branch from d63225c to 15db255 Compare December 17, 2024 03:30

k8s-ci-robot requested a review from robscott December 17, 2024 03:30

robscott reviewed Dec 18, 2024

View reviewed changes

k8s-ci-robot requested a review from dprotaso December 18, 2024 07:18

LiorLieberman force-pushed the percent-based-mirror branch from 8ed94ce to aa266b3 Compare December 18, 2024 17:59

replace multi-mirror test cases

63803d9

LiorLieberman force-pushed the percent-based-mirror branch from aa266b3 to 63803d9 Compare December 18, 2024 18:01

k8s-ci-robot requested a review from kflynn December 20, 2024 01:40

k8s-ci-robot requested a review from mikemorris January 7, 2025 03:43

mlavacca reviewed Jan 10, 2025

View reviewed changes

add TODOs with issues

d74a070

LiorLieberman force-pushed the percent-based-mirror branch from 3b693c1 to d74a070 Compare January 10, 2025 22:19

LiorLieberman requested a review from mlavacca January 10, 2025 22:19

fix linting issues and add sleep

082cc90

LiorLieberman requested a review from robscott January 15, 2025 22:26

mlavacca reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conformance tests for percentage-based request mirroring #3508

conformance tests for percentage-based request mirroring #3508

LiorLieberman commented Dec 17, 2024 •

edited

Loading

LiorLieberman commented Dec 17, 2024

k8s-ci-robot commented Dec 17, 2024

robscott left a comment

robscott commented Dec 18, 2024

LiorLieberman commented Dec 20, 2024

LiorLieberman commented Jan 7, 2025

mlavacca left a comment

mlavacca Jan 10, 2025

LiorLieberman Jan 10, 2025

mlavacca Jan 23, 2025

k8s-ci-robot commented Jan 10, 2025

LiorLieberman commented Jan 13, 2025

k8s-ci-robot commented Jan 13, 2025

mlavacca Jan 23, 2025

mlavacca Jan 23, 2025

mlavacca Jan 23, 2025

mlavacca Jan 23, 2025

	Copyright 2024 The Kubernetes Authors.
	Copyright 2025 The Kubernetes Authors.

conformance tests for percentage-based request mirroring #3508

Are you sure you want to change the base?

conformance tests for percentage-based request mirroring #3508

Conversation

LiorLieberman commented Dec 17, 2024 • edited Loading

LiorLieberman commented Dec 17, 2024

k8s-ci-robot commented Dec 17, 2024

robscott left a comment

Choose a reason for hiding this comment

robscott commented Dec 18, 2024

LiorLieberman commented Dec 20, 2024

LiorLieberman commented Jan 7, 2025

mlavacca left a comment

Choose a reason for hiding this comment

mlavacca Jan 10, 2025

Choose a reason for hiding this comment

LiorLieberman Jan 10, 2025

Choose a reason for hiding this comment

mlavacca Jan 23, 2025

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 10, 2025

LiorLieberman commented Jan 13, 2025

k8s-ci-robot commented Jan 13, 2025

mlavacca Jan 23, 2025

Choose a reason for hiding this comment

mlavacca Jan 23, 2025

Choose a reason for hiding this comment

mlavacca Jan 23, 2025

Choose a reason for hiding this comment

mlavacca Jan 23, 2025

Choose a reason for hiding this comment

LiorLieberman commented Dec 17, 2024 •

edited

Loading