Skip to content

Commit

Permalink
feat(compute/metadata): add context for all functions/methods (#10370)
Browse files Browse the repository at this point in the history
Hi!

The [GMP product got hit by the case](https://togithub.com/GoogleCloudPlatform/prometheus-engine/pull/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](https://togithub.com/GoogleCloudPlatform/prometheus-engine/pull/1021/commits/e02408cf56ecce7a949b425245d133ced42635c9)), 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 https://togithub.com/googleapis/google-cloud-go/pull/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.

cc @codyoss
  • Loading branch information
bwplotka committed Jul 1, 2024
1 parent 6e16c64 commit 66b8efe
Show file tree
Hide file tree
Showing 2 changed files with 320 additions and 50 deletions.
Loading

0 comments on commit 66b8efe

Please sign in to comment.