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

e2e: add e2e test for MergeGateways feature #2665

Merged
merged 33 commits into from
Apr 24, 2024
Merged

e2e: add e2e test for MergeGateways feature #2665

merged 33 commits into from
Apr 24, 2024

Conversation

shawnh2
Copy link
Contributor

@shawnh2 shawnh2 commented Feb 21, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #2029

@shawnh2 shawnh2 requested a review from a team as a code owner February 21, 2024 10:06
@shawnh2 shawnh2 requested a review from cnvergence February 21, 2024 10:06
@codecov Codecov
Copy link

codecov bot commented Feb 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.60%. Comparing base (29946b0) to head (c3df6a1).
Report is 92 commits behind head on main.

❗ Current head c3df6a1 differs from pull request most recent head 0b4509a. Consider uploading reports for the commit 0b4509a to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2665      +/-   ##
==========================================
- Coverage   66.51%   64.60%   -1.92%     
==========================================
  Files         161      122      -39     
  Lines       22673    21147    -1526     
==========================================
- Hits        15080    13661    -1419     
+ Misses       6720     6638      -82     
+ Partials      873      848      -25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

liorokman
liorokman reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@liorokman liorokman left a comment

Choose a reason for hiding this comment

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

LGTM

Maybe also add a test-case where there is some conflict between the gateways?

@shawnh2
Copy link
Contributor Author

shawnh2 commented Feb 21, 2024

LGTM

Maybe also add a test-case where there is some conflict between the gateways?

good point

@shawnh2 shawnh2 mentioned this pull request Feb 21, 2024
Closed
cnvergence
cnvergence reviewed Feb 21, 2024
View reviewed changes
Copy link
Member

@cnvergence cnvergence left a comment

Choose a reason for hiding this comment

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

looks good, I am happy to see it!

@shawnh2
address comments
f7161d9 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
Copy link
Contributor Author

shawnh2 commented Feb 21, 2024 •
edited
Loading

LGTM

Maybe also add a test-case where there is some conflict between the gateways?

there is an issue while implementing this test, and it seems related to #2668

@Xunzhuo
Copy link
Member

Xunzhuo commented Feb 23, 2024

#2672 is merged, plz go ahead, thanks.

@shawnh2
Copy link
Contributor Author

shawnh2 commented Feb 24, 2024 •
edited
Loading

/retest

all the unstable conformance tests will be investigated via #2269

@shawnh2 shawnh2 mentioned this pull request Feb 28, 2024
Closed
@shawnh2
Copy link
Contributor Author

shawnh2 commented Feb 28, 2024 •
edited
Loading

hold again before resolving #2520 and #2724

@shawnh2 shawnh2 added the hold do not merge label Feb 28, 2024
arkodg
arkodg reviewed Feb 28, 2024
View reviewed changes
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
@arkodg arkodg mentioned this pull request Feb 28, 2024
Closed
arkodg
arkodg previously approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@zirain
Copy link
Contributor

zirain commented Mar 8, 2024

/retest

@shawnh2 shawnh2 removed the hold do not merge label Mar 8, 2024
@shawnh2
update code
8647f62 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 10, 2024

hey @shawnh2 is this one ready for review ?

Yes. But this test seems a little unstable.

arkodg
arkodg reviewed Apr 10, 2024
View reviewed changes
test/e2e/testdata/basic-merge-gateways.yaml
namespace: gateway-conformance-infra
spec:
gatewayClassName: merge-gateways
listeners:
Copy link
Contributor

Choose a reason for hiding this comment

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

is the instability because you are using the same listener name and port b/w merged-gateway-4 & merged-gateway-3
cc @cnvergence

Copy link
Contributor Author

@shawnh2 shawnh2 Apr 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the merged-gateway-4 is deliberately collide with merged-gateway-3. the test case for merged-gateway-4 is to make sure the right status will be surfaced if the merge gateway is conflict with another.

arkodg
arkodg reviewed Apr 10, 2024
View reviewed changes
tools/make/kube.mk Outdated Show resolved Hide resolved
@arkodg arkodg requested a review from cnvergence April 10, 2024 10:49
shawnh2 added 10 commits April 14, 2024 21:32
@shawnh2
resolve conflicts
e822a61 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
address comments
6a32018 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
sort imports for envoy_shutdown and setup suite for merge-gateways
eb7629a 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
fix lint
a4517cc 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
Merge branch 'main' into merge-gateways-e2e-test
50512dc
@shawnh2
fix gen-check
29e77ca 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
Merge branch 'main' of github.com:envoyproxy/gateway into merge-gatew…
0402ca5 
…ays-e2e-test
@shawnh2
change kube.mk merge gateways path
e97db3d 
Signed-off-by: shawnh2 <[email protected]>
@shawnh2
Merge branch 'main' of github.com:envoyproxy/gateway into merge-gatew…
3131079 
…ays-e2e-test
@shawnh2
only setting up necessary arguments for merged gateway e2e test to be…
2c65b0b 
… able to run

Signed-off-by: shawnh2 <[email protected]>
@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 21, 2024 •
edited
Loading

hi @arkodg, I think this PR is good to go. If we running into any flakiness caused by this e2e test case, we can submit an issue to report anytime :)

arkodg
arkodg approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@zirain
Copy link
Contributor

zirain commented Apr 24, 2024

/retest

3 similar comments
@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 24, 2024

raise #3262 to track flaky e2e test: EnvoyShutdown

@shawnh2
Copy link
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@guydc guydc merged commit cc8a86e into envoyproxy:main Apr 24, 2024
17 checks passed
@shawnh2 shawnh2 deleted the merge-gateways-e2e-test branch April 24, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liorokman liorokman liorokman left review comments

@guydc guydc guydc approved these changes

@arkodg arkodg arkodg approved these changes

@zirain zirain zirain approved these changes

@cnvergence cnvergence Awaiting requested review from cnvergence

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add E2E for the MergeGateways feature
7 participants
@shawnh2 @Xunzhuo @zirain @arkodg @liorokman @guydc @cnvergence