-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add dualstack support to the framework behind flag #101
Conversation
001d8aa
to
4f0707d
Compare
c9ac825
to
682e1db
Compare
cd91460
to
ad73eca
Compare
42e7a3d
to
dc31464
Compare
|
b879f09
to
6d66eaf
Compare
f53f369
to
07bfab4
Compare
32e2354
to
44dd2a4
Compare
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.
LGTM, but give it another local run against the dual-stack cluster to polish the issues like the one with incorrect deletion orders. Then merge the current master into this branch, and re-run the tests. I'll give it another look early tomorrow.
Oh, and please save the output of the local run - I'd like to give it a check in case we missed anything |
…ods, address other code improvement suggestions
…testcase, add enable_dualstack to client and server deployment
44dd2a4
to
dd087fb
Compare
All test suites against 1.65.x after rebase with latest main |
@purnesh42H noticed that the jobs you ran weren't using this framework branch. Note the jobs I started above: # [08:17:29 PDT] Parsed build request:
full_job_name: "grpc/java/v1.65.x/branch/psm-security"
scm_revision {
github_scm_revision {
commit_sha: "v1.65.x"
name: "grpc-java"
owner: "grpc"
repository: "grpc-java"
}
}
env_vars {
key: "TEST_DRIVER_BRANCH"
value: "add-dualstack-support"
}
env_vars {
key: "TEST_DRIVER_REPO_OWNER"
value: "purnesh42H"
} Also - no need to run |
Ah, my fault. For some reason I expected these env vars to show up in BUILD_CONFIG on this page: https://btx.cloud.google.com/invocations/88d2b23a-0c50-47f1-8b97-990496b008cd/details. The jobs your started look right. |
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.
All looks good, except I suspect ClientDeploymentArgs won't work as we want to. Could you apply/accept suggestions I made, and I'll take a stab at getting the deployment args right.
Co-authored-by: Sergii Tkachenko <[email protected]>
Co-authored-by: Sergii Tkachenko <[email protected]>
Co-authored-by: Sergii Tkachenko <[email protected]>
Co-authored-by: Sergii Tkachenko <[email protected]>
Co-authored-by: Sergii Tkachenko <[email protected]>
Co-authored-by: Sergii Tkachenko <[email protected]>
Co-authored-by: Sergii Tkachenko <[email protected]>
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.
Two items to address in the follow-up:
- Move server runner's enable_dualstack to
ServerDeploymentArgs
- Fix the issue with deployment_args not merged in suites that override
initKubernetesClientRunner
, see https://github.com/grpc/psm-interop/pull/101/files#r1688415338
Based on @larry-safran's proof of concept PR #88