fix(export): add support in onGCE + no conn to metadata case#1021
fix(export): add support in onGCE + no conn to metadata case#1021
Conversation
e9db076 to
668618c
Compare
668618c to
da0746d
Compare
da0746d to
f606eed
Compare
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 <bwplotka@google.com>
f606eed to
e02408c
Compare
TheSpiritXIII
left a comment
There was a problem hiding this comment.
Thank you for the investigation and solid change!
| func TestFromFlags_NotOnGCE(t *testing.T) { | ||
| // Asserting there is actually no GCE underneath. | ||
| if metadata.OnGCE() { | ||
| t.Fatal("This test assumes we don't run on GCP") |
There was a problem hiding this comment.
Why? Do we expect to run in GCP? I don't think so, so let's fail fast (assert our assumption), WDYT? 🤔
| // calls will timeout correctly (we propagate context properly). | ||
| func TestTryPopulateUnspecifiedFromMetadata(t *testing.T) { | ||
| // Asserting there is actually no GCE underneath. | ||
| if metadata.OnGCE() { |
There was a problem hiding this comment.
Suggestion: What if we just mock the metadata server? e.g. we create an interface and pass that to tryPopulateUnspecifiedFromMetadata?
There was a problem hiding this comment.
Yea, what's the advantage of this versus what's now?
We can mock, but then we just test if we pass context correctly.
Here we test the metadata client to also pass context correctly through HTTP libs. I know we shouldn't generally test not our code.. but well, our product depends on this we just got regression because of that, thus let's maybe test this case?
| // is not accessible on GCP, because it was disabled (404 errors), or not accessible | ||
| // (connection refused, slow network, e.g. sandbox + metadata disabled) | ||
| // it's a noop. Make sure to pass context with a timeout. | ||
| func tryPopulateUnspecifiedFromMetadata(ctx context.Context, logger log.Logger, opts *export.ExporterOpts) { |
There was a problem hiding this comment.
Suggestion: Should we move this to a common utility, so we could reuse this for cmd/operator/main.go? Do we want to? We may suffer from the same problem there. In fact, we did see alerts based around the operator restarting too often at some point. May be the same or similar issue?
There was a problem hiding this comment.
Good idea!
Doubt we are affected in the same way, but would be a good practice and little harm! I want to keep this PR small for bugfix though, let me check if this can be still "small" with operator changes.
There was a problem hiding this comment.
Ok, I checked and indeed it would be nice, but I would postpone the operator (and rule-eval) changes until googleapis/google-cloud-go#10370 is merged (if anything). Will be easier. For now we could merge this and patch 0.12.0?
They are not directly affected to the known issue (you can't really move non collectors to sandbox nodes in GKE, for managed GMP)
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 <bwplotka@gmail.com>
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>
Signed-off-by: bwplotka <bwplotka@google.com>
|
I tested this PR on my repro cluster.
Metrics flow for 3 nodes as expected:
From this PR POV it's rdy to go I think |
TheSpiritXIII
left a comment
There was a problem hiding this comment.
I'm okay with all of your reasons. Thank you so much for the metadata library change! Much appreciated and great idea.
Signed-off-by: bwplotka <bwplotka@google.com>
bee321c to
9c49d91
Compare
|
🤗 |
Cherry-pick of 3 commits from #1021
Turns out @TheSpiritXIII [suggestion](#1021 (comment)) was worth doing 🙃
| // be used as crucial labels for export to work against GCM's Prometheus target. | ||
| // | ||
| // Set a hard time limit due to readiness and liveliness probes during this stage. | ||
| mctx, cancel := context.WithTimeout(context.Background(), metadataFetchTimeout) |
There was a problem hiding this comment.
nit: should this have been ctx instead of context.Background()?
| // of the metadata server. This is best-effort, so when the metadata server | ||
| // is not accessible on GCP, because it was disabled (404 errors), or not accessible | ||
| // (connection refused, slow network, e.g. sandbox + metadata disabled) | ||
| // it's a noop. Make sure to pass context with a timeout. |
There was a problem hiding this comment.
suggestion: Could we add a TODO here referencing we can update/slim this code down once googleapis/google-cloud-go#10370 is merged?
Fixes b/344740239 (edge case with GKE Metadata Server and GKE sandbox).
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