feat(compute/metadata): add context for all functions/methods #10370
+320
−50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hi! The [GMP product got hit by the case](GoogleCloudPlatform/prometheus-engine#1021) where calls to metadata server were hanging "forever" (GKE sandbox with disabled metadata server). Thanks to recent work in this library, the clients support retry with backoff with timeouts. However, still a single call was taking ~14s and since we made multiple of those, it took down the service (readiness probe of 30s). We can hackly workaround things (as we did in [here](GoogleCloudPlatform/prometheus-engine@e02408c)), by copying some code around, but I think the above issue proves it would be helpful to actually add context to all metadata functions and methods. I saw googleapis#9733 we started small, but in this PR I actually spent 1h to add context to all things for consistency. I self-reviewed carefuly, and tried to be consistent and extra safe, but careful review is needed, given little testing. Some minor extra changes (we can split to new commit/PR): * Added comment around 14s worst case * Added comment about OnGCE semantics which was a bit surprising for us. * When reviewing tests, I updated one test for best practices. Alternative would be something like ``` // WithContext sets the context that will be used for all client methods that // does not support context. This means that e.g. the context passed in // GetWithContext will override the context in WithContext func (c *Client) WithContext(ctx context.Context) { c.ctx = ctx } ``` .. but the logic is not trivial and we introdue some state to the global variable (defaultClient), which can be source of problems if someone sets context for them, but another package in same binary resues it etc. Given it's trivial to add (and use) new methods, I just did it. Signed-off-by: bwplotka <[email protected]>
product-auto-label
bot
added
the
api: compute
bwplotka
added a commit
to GoogleCloudPlatform/prometheus-engine
that referenced
this pull request
Jun 12, 2024
Fixes b/344740239 (edge case with GKE Metadata Server and GKE sandbox). * Added debug logging. * Updated metadata deps to get googleapis/google-cloud-go#9733 & use timeout-ed context * Moved risky logic from FromFlags, see code comment why. * Added regession test. Regression test fails on prev flow (no context propagation)  ### Alternatives Everything we do in FromFlags or constructor is within readines period. We could consider moving potentially "slow" things on slow network or metadata srv to exporter.Run. This could be questionable as for GMP to work we at end need export functionality to work, so delaying that information or making it surface in separation to readiness might not be helpful. EDIT: Added PR against metadata so we can get rid of custom code in the future: googleapis/google-cloud-go#10370
codyoss
reviewed
Jun 12, 2024
As discussed in googleapis#10370 (comment) We can add this later. Signed-off-by: bwplotka <[email protected]>
codyoss
requested changes
Jun 27, 2024
Signed-off-by: bwplotka <[email protected]>
codyoss
approved these changes
Jun 28, 2024
codyoss
added
the
automerge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi!
The GMP product got hit by the case where calls to metadata server were hanging "forever" (GKE sandbox with disabled metadata server). Thanks to recent work in this library, the clients support retry with backoff with timeouts.
However, still a single call was taking ~14s and since we made multiple of those, it took down the service (readiness probe of 30s).
We can hackly workaround things (as we did in here), by copying some code around, but I think the above issue proves it would be helpful to actually add context to all metadata functions and methods.
I saw #9733 we started small, but in this PR I actually spent 1h to add context to all things for consistency.
I self-reviewed carefuly, and tried to be consistent and extra safe, but careful review is needed, given little testing.
Some minor extra changes (we can split to new commit/PR):
Alternative would be something like
.. but the logic is not trivial and we introdue some state to the global variable (defaultClient), which can be source of problems if someone sets context for them, but another package in same binary resues it etc.
Given it's trivial to add (and use) new methods, I just did it.
cc @codyoss