Skip to content

e2e: add e2e test for MergeGateways feature#2665

Merged
guydc merged 33 commits intoenvoyproxy:mainfrom
shawnh2:merge-gateways-e2e-test
Apr 24, 2024
Merged

e2e: add e2e test for MergeGateways feature#2665
guydc merged 33 commits intoenvoyproxy:mainfrom
shawnh2:merge-gateways-e2e-test

Conversation

@shawnh2
Copy link
Copy Markdown
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
add e2e test for merge gateways feature
1f1dedf 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@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
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

cnvergence
cnvergence reviewed Feb 21, 2024
View reviewed changes
@shawnh2
address comments
f7161d9 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
Copy link
Copy Markdown
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
Copy Markdown
Member

Xunzhuo commented Feb 23, 2024

#2672 is merged, plz go ahead, thanks.

@shawnh2
Merge branch 'main' of github.com:envoyproxy/gateway into merge-gatew…
ae3a2d0 
…ays-e2e-test
@shawnh2
add e2e test for conflicted merge gateways case
5ea21e5 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
add supportedKinds for listener status
4ef1232 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Feb 24, 2024 •
edited
Loading

/retest

all the unstable conformance tests will be investigated via #2269

@shawnh2
resolve conflicts
893f5ee 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
Merge branch 'main' of github.com:envoyproxy/gateway into merge-gatew…
f5cfbb0 
…ays-e2e-test
@shawnh2
abstract new e2e test for gateway class as a function
8df3a0d 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2 shawnh2 mentioned this pull request Feb 28, 2024
Closed
@shawnh2
Copy link
Copy Markdown
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
@arkodg arkodg mentioned this pull request Feb 28, 2024
Closed
arkodg
arkodg previously approved these changes Mar 8, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

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 <shawnhxh@outlook.com>
@shawnh2
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@arkodg arkodg Apr 10, 2024

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
Copy Markdown
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
arkodg
arkodg reviewed Apr 10, 2024
View reviewed changes
@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 <shawnhxh@outlook.com>
@shawnh2
address comments
6a32018 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
sort imports for envoy_shutdown and setup suite for merge-gateways
eb7629a 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
fix lint
a4517cc 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@shawnh2
Merge branch 'main' into merge-gateways-e2e-test
50512dc
@shawnh2
fix gen-check
29e77ca 
Signed-off-by: shawnh2 <shawnhxh@outlook.com>
@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 <shawnhxh@outlook.com>
@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 <shawnhxh@outlook.com>
@shawnh2
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

zirain commented Apr 24, 2024

/retest

3 similar comments
@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Apr 24, 2024

raise #3262 to track flaky e2e test: EnvoyShutdown

@shawnh2
Copy link
Copy Markdown
Contributor Author

shawnh2 commented Apr 24, 2024

/retest

@guydc guydc merged commit cc8a86e into envoyproxy:main Apr 24, 2024
@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

@zirain zirain zirain approved these changes

@arkodg arkodg arkodg approved these changes

@guydc guydc guydc approved these changes

@cnvergence cnvergence Awaiting requested review from cnvergence

+1 more reviewer

@liorokman liorokman liorokman left review comments

Reviewers whose approvals may not affect merge requirements

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