feat(compute/metadata): add context aware functions #9733

codyoss · 2024-04-09T17:12:09Z

This change adds the minimal amount of context aware functionality
so that users can pass a context to all metadata requests. This
does not re-expose all the helper methods this package provides.
We can add context variants for all of these in the future and/or
if we ever create a v2 of this package.

Fixes: #4483

This change adds the minimal amount of context aware functionality so that users can pass a context to all metadata requests. This does not re-expose all the helper methods this package provides. We can add context varients for all of these in the future and/or if we ever create a v2 of this package. Fixes: googleapis#4483

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. ### 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. Signed-off-by: bwplotka <[email protected]>

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. 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]>

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]>

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

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. ### 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. Signed-off-by: bwplotka <[email protected]>

codyoss added 2 commits April 9, 2024 11:16

chore(compute/metadata): fixup warnings

feee882

codyoss requested a review from a team as a code owner April 9, 2024 17:12

Merge branch 'main' into ctx-metadata

b45709c

codyoss requested a review from quartzmo April 9, 2024 17:12

product-auto-label bot added the api: compute Issues related to the Compute Engine API. label Apr 9, 2024

codyoss added 2 commits April 15, 2024 11:12

Merge branch 'main' into ctx-metadata

d313308

Merge branch 'main' into ctx-metadata

9ca0351

codyoss added the automerge Merge the pull request once unit tests and other checks pass. label Apr 15, 2024

quartzmo approved these changes Apr 15, 2024

View reviewed changes

Merge branch 'main' into ctx-metadata

62243ec

gcf-merge-on-green bot merged commit e4eb5b4 into googleapis:main Apr 15, 2024
8 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 15, 2024

codyoss deleted the ctx-metadata branch April 15, 2024 19:05

release-please bot mentioned this pull request Apr 15, 2024

chore: release main #9710

Merged

bwplotka mentioned this pull request Jun 11, 2024

fix(export): add support in onGCE + no conn to metadata case GoogleCloudPlatform/prometheus-engine#1021

Merged

bwplotka mentioned this pull request Jun 12, 2024

feat(compute/metadata): add context for all functions/methods #10370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compute/metadata): add context aware functions #9733

feat(compute/metadata): add context aware functions #9733

codyoss commented Apr 9, 2024

feat(compute/metadata): add context aware functions #9733

feat(compute/metadata): add context aware functions #9733

Conversation

codyoss commented Apr 9, 2024