Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add e2e test of Grafana dashboard #71

Merged
merged 3 commits into from
Jul 21, 2022
Merged

Add e2e test of Grafana dashboard #71

merged 3 commits into from
Jul 21, 2022

Conversation

heanlan
Copy link
Contributor

@heanlan heanlan commented Jul 2, 2022 •
edited
Loading

This commit adds e2e test of pre-built Grafana dashboards, by:

  1. Get dashboard JSON by uid
  2. Read dashboard JSON file and get the queries
  3. Execute the queries and check whether the expected contents is
    contained in the result data frame

Given we cannot access Grafana through nodePort on Kind cluster,
before sending request to Grafana backend HTTP API, we firstly
port-forward Grafana Service port to a local port.

Fixes: #52

Signed-off-by: heanlan hanlan@vmware.com

@heanlan
Copy link
Contributor Author

heanlan commented Jul 2, 2022

Trying to gather some preliminary feedback about the testcase design and test approach, before adding all the testcases for all queries. Thanks

@heanlan heanlan force-pushed the grafana-e2e branch 2 times, most recently from 2efa196 to 9f23d3e Compare July 2, 2022 06:19
@heanlan heanlan mentioned this pull request Jul 2, 2022
Closed
yuntanghsu
yuntanghsu reviewed Jul 6, 2022
View reviewed changes
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
@heanlan
Copy link
Contributor Author

heanlan commented Jul 12, 2022

Given the current testcase code takes a big code block, I thought about whether we want to move the Grafana test to another file. The thing is, the actual test part takes very short time(0.32s) to finish. The test setup time is relatively very long. If we want to separate it into another test to make it looks better, it must sacrifice the overall e2e test running time. What's your opinion on that?

--- PASS: TestFlowVisibility (243.58s)
--- PASS: TestFlowVisibility/IPv4 (104.92s)
--- PASS: TestFlowVisibility/IPv4/IntraNodeFlows (14.54s)
--- PASS: TestFlowVisibility/IPv4/IntraNodeDenyConnIngressANP (6.64s)
--- PASS: TestFlowVisibility/IPv4/IntraNodeDenyConnEgressANP (2.49s)
--- PASS: TestFlowVisibility/IPv4/IntraNodeDenyConnNP (8.62s)
--- PASS: TestFlowVisibility/IPv4/InterNodeFlows (14.03s)
--- PASS: TestFlowVisibility/IPv4/InterNodeDenyConnIngressANP (7.08s)
--- PASS: TestFlowVisibility/IPv4/InterNodeDenyConnEgressANP (2.46s)
--- PASS: TestFlowVisibility/IPv4/InterNodeDenyConnNP (8.62s)
--- PASS: TestFlowVisibility/IPv4/ToExternalFlows (8.85s)
--- PASS: TestFlowVisibility/IPv4/LocalServiceAccess (13.84s)
--- PASS: TestFlowVisibility/IPv4/RemoteServiceAccess (14.26s)
--- PASS: TestFlowVisibility/IPv4/Grafana (0.32s)

dreamtalen
dreamtalen reviewed Jul 12, 2022
View reviewed changes
Copy link
Contributor

@dreamtalen dreamtalen left a comment

Choose a reason for hiding this comment

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

Looks like queryList accounts for most of lines, how about sepearting those test data into files and import them during test running?
This will keep Grafana test inside this test file to avoid long setup time and keep code clean.

yanjunz97
yanjunz97 reviewed Jul 13, 2022
View reviewed changes
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

I agree to Yongming that repeating the same setting up is not recommended. We can move some grafana related functions and test data initialization to another file and use them in flowvisibility_test.go. The overall test logic looks good to me, only some nits.

test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
@heanlan
Copy link
Contributor Author

heanlan commented Jul 13, 2022

/theia-test-e2e

@salv-orlando salv-orlando added this to the 0.2 milestone Jul 14, 2022
yuntanghsu
yuntanghsu reviewed Jul 14, 2022
View reviewed changes
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
@heanlan
Copy link
Contributor Author

heanlan commented Jul 14, 2022

/theia-test-e2e

@heanlan heanlan requested a review from yuntanghsu July 14, 2022 22:38
@heanlan heanlan force-pushed the grafana-e2e branch 2 times, most recently from b927d89 to 0e61601 Compare July 15, 2022 22:35
dreamtalen
dreamtalen reviewed Jul 15, 2022
View reviewed changes
test/e2e/framework.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
test/e2e/flowvisibility_test.go Outdated Show resolved Hide resolved
@yuntanghsu
Copy link
Contributor

LGTM

@heanlan
Add e2e test of Grafana dashboard
84861ac 
This commit adds e2e test of pre-built Grafana dashboards, by:
1. Get dashboard JSON by uid
2. Read dashboard JSON file and get the queries
3. Execute the queries and check whether the expected contents is
contained in the result data frame

Given we cannot access Grafana through nodePort on Kind cluster,
before sending request to Grafana backend HTTP API, we firstly
port-forward Grafana Service port to a local port.

Signed-off-by: heanlan <hanlan@vmware.com>
@heanlan
Pull flow-aggregator image from docker hub
9cd932e 
Signed-off-by: heanlan <hanlan@vmware.com>
@heanlan
Edit deny NP queries
8a80235 
Previously in the networkpolicy_dashboard throughput of deny NPs
queries we require AVG(throughput) > 0, it turns out to be almost
all the deny connection have throughput = 0. That is because they
only send one packet, and the byte counts is too small. In the
throughput calculation, the very small byte counts yields a 0
throughput as the integer result. Thus, we change AVG(throughput)
to SUM(octetDeltaCount), which effectively help us avoid the issue.

Signed-off-by: heanlan <hanlan@vmware.com>
dreamtalen
dreamtalen approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@dreamtalen dreamtalen left a comment

Choose a reason for hiding this comment

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

LGTM

@heanlan
Copy link
Contributor Author

heanlan commented Jul 21, 2022

/theia-test-e2e

@heanlan heanlan merged commit fd5795a into antrea-io:main Jul 21, 2022
@heanlan heanlan deleted the grafana-e2e branch July 21, 2022 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamtalen dreamtalen dreamtalen approved these changes

@salv-orlando salv-orlando Awaiting requested review from salv-orlando

@yanjunz97 yanjunz97 Awaiting requested review from yanjunz97

@yuntanghsu yuntanghsu Awaiting requested review from yuntanghsu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Theia v0.2 release
Development

Successfully merging this pull request may close these issues.

e2e test coverage for Grafana dashboards
5 participants
@heanlan @yuntanghsu @dreamtalen @yanjunz97 @salv-orlando