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.

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

FR: Emit warnings when a user attempts to use DirectPath but environment or client is incorrectly configured #2164

Closed
frankyn opened this issue Oct 16, 2023 · 4 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@frankyn
Copy link
Member

frankyn commented Oct 16, 2023

Is your feature request related to a problem? Please describe.
When a user tries to use DirectPath for GCS; there are two "flags" to enable: attemptDirectPath() and attemptDirectPathXds() (env var equivalents exist as well). During development of gRPC API support in java-storage; the implementation only used attemptDirectPath() but failed to notify us that the flag attemptDirectPathXds() was also needed. The result was ~2 weeks of debugging why java-storage was using Cloud Path when attemptDirectPath() was used.

More context is provided by @mohanli-ml from the gRPC team:

attemptDirectPath is originally created for DirectPath gRPCLB (gRPCLB is the name resolver). Then DirectPath xDS was developed (Traffic Director is the name resolver) and we decide to migrate DirectPath gRPCLB to DirectPath xDS. To differentiate the two DirectPath infrastructures, attemptDirectPathXds option/env were created:

  • If attemptDirectPath is NOT set, CloudPath will be used;
  • If attemptDirectPath is set AND attemptDirectPathXds option/env is NOT set, DirectPath gRPCLB is used; (for GCS it will use Cloud Path)
  • If attemptDirectPath is set AND attemptDirectPathXds option/env is set, DirectPath xDS is used.

Currently the migration is still ongoing as CBT still has DirectPath gRPCLB traffic (CBT's timeline is Q4). After that, I will cleanup DirectPath gRPCLB code in the gax library. For example, attemptDirectPath will be removed and we just need to keep attemptDirectPathXds option/env.

Describe the solution you'd like

When a user attempts DirectPath using attemptDirectPath() or/and attemptDirectPathXds(); there should be warnings emitted if the attempt failed to use DirectPath. Otherwise, the attempt behaves like a blackbox unless a user enables verbose logging to inspect the CIDR block being used by remote IP addresses. Additionally, there are other conditions where DirectPath may fail:

  • Directpath attempted without GCE Credentials
  • Directpath attempted outside of GCE

It would be great to extend this methodology to future conditions that could cause CloudPath to be used when attempting DirectPath but it's hard to predict or prepare ahead of time.

Describe alternatives you've considered

  1. Relying on GCS support to help unblock customers is an alternative through internal dashboards but does not scale.
  2. Doing nothing is also not acceptable from a usability perspective because there is no signal for a customer to debug the issue.
@mohanli-ml
Copy link
Contributor

mohanli-ml commented Oct 16, 2023

Thanks Frank.

The key point to properly generate the WARNING log without spamming/misleading customers is to correctly decide whether a customer want to use DirectPath. Intuitively, we decide by if a customer uses attemptDirectPath() and/or attemptDirectPathXds(), as you suggested. However, this strategy brings difficulty to CBT/CS customers because CBT/CS Java/Go client libraries set attemptDirectPath() to true by default. So image CBT customer who is not using GCP and have no sense of DirectPath, the customer will be confused by the DirectPath WARNING logs.

So my suggestion is that let's only emit WARNING logs when attemptDirectPathXds() is set, no matter if attemptDirectPath() is set or not. In this case, we may miss the case where attemptDirectPath() is set but attemptDirectPathXds() is not set. However:

  • For GCS, this is not feasible in practice because GCS client library combines attemptDirectPath() and attemptDirectPathXds() as one option to GCS customers;
  • For CBT/CS, this does not mean customers want to use DirectPath, as attemptDirectPath() is set by the client library (and traffic will fallback because of ACL) instead of customers.

@blakeli0 blakeli0 added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Oct 17, 2023
@blakeli0
Copy link
Collaborator

For GCS, this is not feasible in practice because GCS client library combines attemptDirectPath() and attemptDirectPathXds() as one option to GCS customers

@frankyn How does GCS expose direct path options to customers?

@frankyn
Copy link
Member Author

frankyn commented Oct 17, 2023

@frankyn How does GCS expose direct path options to customers?

Here's how it's exposed in java-storage:
https://github.com/googleapis/java-storage/blob/main/samples/snippets/src/main/java/com/example/storage/QuickstartGrpcDpSample.java

For google-cloud-go storage: enabling uses header support:
https://pkg.go.dev/cloud.google.com/go/storage#hdr-Experimental_gRPC_API

image

@blakeli0
Copy link
Collaborator

fixed by #2105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

3 participants