-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(compute/metadata): add context for all functions/methods #10370
Conversation
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 <bwplotka@gmail.com>
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) ![image](https://github.com/GoogleCloudPlatform/prometheus-engine/assets/6950331/3c0d797a-66c3-4a82-8156-72e798171e71) ### 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
As discussed in googleapis#10370 (comment) We can add this later. Signed-off-by: bwplotka <bwplotka@gmail.com>
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.
Just one small nit, or else it LGTM
Signed-off-by: bwplotka <bwplotka@gmail.com>
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
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