-
Notifications
You must be signed in to change notification settings - Fork 107
E2E tests for long_tcp_conns metrics and accesslogs #1330
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?
Conversation
@@ -0,0 +1,25 @@ | |||
//go:build integ |
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.
Maybe we could put this test in the baseline directly, just like L4 telemetry test case, ref: https://github.com/kmesh-net/kmesh/blob/main/test/e2e/baseline_test.go#L780
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.
ok @YaoZengzeng i will do that, thanks for the help
getting these warnings for all the files in e2e directory in vs-code, also the go-linting and code-navigation Is not working in e2e directory. @YaoZengzeng |
i think these comments at the above are the reason, what they do |
ok got it, they are required to separate unit and integration tests |
@YaoZengzeng i want to deploy
for long_conn testing, how can I do this |
You should use the deployed test application as in other test cases. Deploy ref: https://github.com/kmesh-net/kmesh/blob/main/test/e2e/main_test.go#L146 Using method ref: https://github.com/kmesh-net/kmesh/blob/main/test/e2e/baseline_test.go#L780 It seems that there is no direct way to specify a long connection, but maybe we can simulate it by specifying a number of reqs and delay of each req, ref: https://github.com/istio/istio/blob/master/tests/integration/ambient/baseline_test.go#L3316 We use the same e2e testing framework as istio. You can check if there are more use cases to ref in it. |
there should be only one single request which last for long time and should exchange data |
is it possible to deploy custom images in istio test framework @YaoZengzeng |
@YaoZengzeng can u check my approach |
@YaoZengzeng @hzxuzhonghu can u review the test |
2ee32da
to
18c4f29
Compare
|
@hzxuzhonghu can u review the test |
Codecov ReportAll modified and coverable lines are covered by tests ✅
see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Timeout: time.Second, | ||
Check: check.OK(), | ||
To: localDst, | ||
NewConnectionPerRequest: false, |
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.
I think this setting will not take effect, ref:
https://github.com/istio/istio/blob/master/pkg/test/framework/components/echo/calloptions.go#L151
https://github.com/istio/istio/blob/master/pkg/test/framework/components/echo/calloptions.go#L329
You should use HTTP and set a sufficiently long delay for each request:
https://github.com/kmesh-net/kmesh/blob/main/test/e2e/baseline_test.go#L931
test/e2e/baseline_test.go
Outdated
stc.Logf("prometheus query: %#v", query) | ||
err := retry.Until(func() bool { | ||
stc.Logf("sending call from %q to %q", deployName(localSrc), localDst.Config().Service) | ||
localSrc.CallOrFail(stc, opt) |
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.
In addition this test wants to get metrics during the long connection process? However this function call is synchronous, and when it returns, the connection has been terminated.
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.
what about running this function in another go routine ??
@YaoZengzeng can u check now ??, i think now things can work |
@YaoZengzeng why this, I have used HTTP now |
test/e2e/baseline_test.go
Outdated
Port: echo.Port{Name: "http"}, | ||
Scheme: scheme.TCP, | ||
Count: 20, | ||
Timeout: 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.
It should be bigger, the delay is three seconds
test/e2e/baseline_test.go
Outdated
localSrc := src | ||
opt := echo.CallOptions{ | ||
Port: echo.Port{Name: "http"}, | ||
Scheme: scheme.TCP, |
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 should be HTTP as well, I think.
@YaoZengzeng done the changes |
/retest |
6ad1730
to
a66aa0f
Compare
if i comment observe_data, then also tests are working fine, hence we have to change somthing |
multiple request are send paralllely |
i dont think it is reusing the socket |
Check: check.OK(), | ||
HTTP: echo.HTTP{Path: "/?delay=3s"}, | ||
To: localDst, | ||
NewConnectionPerRequest: false, |
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 flag will only take effect for HTTP2/3 but not HTTP1.1, ref: https://github.com/istio/istio/blob/master/pkg/test/echo/server/forwarder/http.go#L133
Also metioned here: https://github.com/istio/istio/blob/master/tests/integration/ambient/baseline_test.go#L345
So use HTTP2 instead, https://github.com/istio/istio/blob/master/tests/integration/ambient/baseline_test.go#L346
Signed-off-by: Yash Patel <yp969803@gmail.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
i want to run
in this test how to do it @hzxuzhonghu @YaoZengzeng |
What type of PR is this?
/kind feature
What this PR does / why we need it:
E2E tests for long_tcp_conns metrics and accesslogs
Which issue(s) this PR fixes:
Fixes #1322
Special notes for your reviewer:
Does this PR introduce a user-facing change?: