Skip to content

Add api listener test for k8s#26993

Closed
ericgribkoff wants to merge 1 commit intogrpc:masterfrom
ericgribkoff:api_listener_port
Closed

Add api listener test for k8s#26993
ericgribkoff wants to merge 1 commit intogrpc:masterfrom
ericgribkoff:api_listener_port

Conversation

@ericgribkoff
Copy link
Copy Markdown
Contributor

This ports the api listener test case to k8s. I noticed that the current implementation of this test case in run_xds_tests.py seems to have what amounts to an unconditional 10 minute wait here - I'm not quite sure what's going on with the verify_attempts calculation in Python, as it seems to come out to equal 60, which I can only reproduce in a repl with int(_WAIT_FOR_URL_MAP_PATCH_SEC / (10.0 * qps) * qps), e.g., with a floating point value for _NUM_TEST_RPCS. Regardless, from the logs it's clearly executing the loop 60 times at 10 seconds per call, resulting in the overall test case typically running 18-20 minutes. Since this isn't actually doing anything but (attempting) to wait for TD propagation - and we can't tell the difference between not-having-propagated and the test passing - I just shortened this loop considerably here. We should be able to properly verify that the propagation has occurred via the CSDS service, which is left as a TODO.

I did not have time to finish the forwarding-rule* or metadata-filter test cases, so just sending what I have here.

@ericgribkoff ericgribkoff added the release notes: no Indicates if PR should not be in release notes label Aug 12, 2021
@YifeiZhuang
Copy link
Copy Markdown
Member

Thanks @ericgribkoff

unconditional 10 minute wait here

It looks a bug regardless of the _WAIT_FOR_URL_MAP_PATCH_SEC timeout config changes previously. We should return early when successful. I will fix in the current test framework, similar to what you've done here.

@YifeiZhuang
Copy link
Copy Markdown
Member

Thanks @ericgribkoff

unconditional 10 minute wait here

It looks a bug regardless of the _WAIT_FOR_URL_MAP_PATCH_SEC timeout config changes previously. We should return early when successful. I will fix in the current test framework, similar to what you've done here.

Actually it looks this verification was on purpose. It should verify that after deleting first set of map+tp+fr, traffic still goes to the original backends, but, we do not know whether this was because TD didn't propagate, or because they propagated and grpc is doing the right thing. So, we waited long enough and assume it is propagated, therefore we know that creating the second api listener is safe. And yes, the verification should probably be polling for TD propagation instead of waiting for their SLA.

@YifeiZhuang
Copy link
Copy Markdown
Member

@ericgribkoff can I have write permission to this branch, I have changes ready to complete this PR.

@ericgribkoff
Copy link
Copy Markdown
Contributor Author

@YifeiZhuang The "allow edits by maintainers" box is already checked (https://screenshot.googleplex.com/AYUDXfQMuyzHpei). Is there something else you'd like me to enable?

@YifeiZhuang
Copy link
Copy Markdown
Member

@YifeiZhuang The "allow edits by maintainers" box is already checked (https://screenshot.googleplex.com/AYUDXfQMuyzHpei). Is there something else you'd like me to enable?

hmmm, i don't know.
It says that If checked, users with write access to grpc/grpc can add new commits to your branch
But I still get this when i push to your branch

To https://github.com/ericgribkoff/grpc.git
 ! [remote rejected]       eric/api_listener_port -> eric/api_listener_port (permission denied)
error: failed to push some refs to 'https://github.com/ericgribkoff/grpc.git'

I switched to #27534, just to be faster.

@stale
Copy link
Copy Markdown

stale bot commented Jan 3, 2022

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions!

@stale stale bot closed this Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disposition/stale release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants