fix(export): add support in onGCE + no conn to metadata case #1021

bwplotka · 2024-06-11T11:30:26Z

Fixes b/344740239 (edge case with GKE Metadata Server and GKE sandbox).

Added debug logging.
Updated metadata deps to get feat(compute/metadata): add context aware functions 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

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>

TheSpiritXIII

Thank you for the investigation and solid change!

TheSpiritXIII · 2024-06-11T20:29:06Z

pkg/export/setup/setup_test.go

+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")


Suggestion: t.Skip?

Why? Do we expect to run in GCP? I don't think so, so let's fail fast (assert our assumption), WDYT? 🤔

TheSpiritXIII · 2024-06-11T20:30:13Z

pkg/export/setup/setup_test.go

+// calls will timeout correctly (we propagate context properly).
+func TestTryPopulateUnspecifiedFromMetadata(t *testing.T) {
+	// Asserting there is actually no GCE underneath.
+	if metadata.OnGCE() {


Suggestion: What if we just mock the metadata server? e.g. we create an interface and pass that to tryPopulateUnspecifiedFromMetadata?

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?

TheSpiritXIII · 2024-06-11T20:31:56Z

pkg/export/setup/setup.go

+// 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) {


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?

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.

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>

bwplotka · 2024-06-12T08:18:00Z

I tested this PR on my repro cluster.

std node: works
sandbox node: works
no metadata node: works (we pass data via flags so it's fine)
sandbox, no metadata: collector runs, but export is unauth as expected

Metrics flow for 3 nodes as expected:

From this PR POV it's rdy to go I think

bwplotka · 2024-06-12T08:27:32Z

After I did https://cloud.google.com/stackdriver/docs/managed-prometheus/setup-managed#explicit-credentials all works:

TheSpiritXIII

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>

bwplotka · 2024-06-12T12:20:50Z

🤗

Cherry-pick of 3 commits from #1021

@TheSpiritXIII

Turns out @TheSpiritXIII [suggestion](#1021 (comment)) was worth doing 🙃

pintohutch · 2024-06-14T14:30:01Z

pkg/export/setup/setup.go

+			// 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)


nit: should this have been ctx instead of context.Background()?

pintohutch · 2024-06-14T14:44:48Z

pkg/export/setup/setup.go

+// 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.


suggestion: Could we add a TODO here referencing we can update/slim this code down once googleapis/google-cloud-go#10370 is merged?

bwplotka force-pushed the bwplotka/sandbox-meta-fix branch from e9db076 to 668618c Compare June 11, 2024 17:41

bwplotka changed the title ~~fix(export): make FromFlags & metadata call more reliable and observable~~ fix(export): add support in onGCE + no conn to metadata case Jun 11, 2024

bwplotka marked this pull request as ready for review June 11, 2024 17:43

bwplotka force-pushed the bwplotka/sandbox-meta-fix branch from 668618c to da0746d Compare June 11, 2024 17:44

bwplotka requested review from TheSpiritXIII and pintohutch June 11, 2024 19:32

bwplotka force-pushed the bwplotka/sandbox-meta-fix branch from da0746d to f606eed Compare June 11, 2024 19:47

bwplotka force-pushed the bwplotka/sandbox-meta-fix branch from f606eed to e02408c Compare June 11, 2024 19:56

TheSpiritXIII requested changes Jun 11, 2024

View reviewed changes

bwplotka mentioned this pull request Jun 12, 2024

feat(compute/metadata): add context for all functions/methods googleapis/google-cloud-go#10370

Open

bwplotka requested a review from TheSpiritXIII June 12, 2024 08:02

chore: add extra log line to export setup

e772a66

Signed-off-by: bwplotka <bwplotka@google.com>

TheSpiritXIII approved these changes Jun 12, 2024

View reviewed changes

chore: add CLI parameter for timeout

9c49d91

Signed-off-by: bwplotka <bwplotka@google.com>

bwplotka force-pushed the bwplotka/sandbox-meta-fix branch from bee321c to 9c49d91 Compare June 12, 2024 12:20

bwplotka merged commit 054f28c into main Jun 12, 2024
27 checks passed

gmp-bot mentioned this pull request Jun 11, 2024

chore(main): release 0.12.0-rc.1 #876

Closed

bwplotka mentioned this pull request Jun 12, 2024

fix(export): add support in onGCE + no conn to metadata case #1023

Merged

bwplotka added a commit that referenced this pull request Jun 12, 2024

fix(export): add support in onGCE + no conn to metadata case (#1023)

79b11af

Cherry-pick of 3 commits from #1021

bwplotka mentioned this pull request Jun 12, 2024

fix: make sure tests are not failing on cloudtop #1025

Merged

bwplotka added a commit that referenced this pull request Jun 12, 2024

fix: make sure tests are not failing on cloudtop (#1025)

1df4974

Turns out @TheSpiritXIII [suggestion](#1021 (comment)) was worth doing 🙃

pintohutch reviewed Jun 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(export): add support in onGCE + no conn to metadata case #1021

fix(export): add support in onGCE + no conn to metadata case #1021

bwplotka commented Jun 11, 2024 •

edited

TheSpiritXIII left a comment

TheSpiritXIII Jun 11, 2024

bwplotka Jun 12, 2024

TheSpiritXIII Jun 11, 2024

bwplotka Jun 12, 2024 •

edited

TheSpiritXIII Jun 11, 2024

bwplotka Jun 12, 2024 •

edited

bwplotka Jun 12, 2024 •

edited

bwplotka commented Jun 12, 2024 •

edited

bwplotka commented Jun 12, 2024

TheSpiritXIII left a comment

bwplotka commented Jun 12, 2024

pintohutch Jun 14, 2024

pintohutch Jun 14, 2024

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

bwplotka commented Jun 11, 2024 • edited

Alternatives

TheSpiritXIII left a comment

Choose a reason for hiding this comment

TheSpiritXIII Jun 11, 2024

Choose a reason for hiding this comment

bwplotka Jun 12, 2024

Choose a reason for hiding this comment

TheSpiritXIII Jun 11, 2024

Choose a reason for hiding this comment

bwplotka Jun 12, 2024 • edited

Choose a reason for hiding this comment

TheSpiritXIII Jun 11, 2024

Choose a reason for hiding this comment

bwplotka Jun 12, 2024 • edited

Choose a reason for hiding this comment

bwplotka Jun 12, 2024 • edited

Choose a reason for hiding this comment

bwplotka commented Jun 12, 2024 • edited

bwplotka commented Jun 12, 2024

TheSpiritXIII left a comment

Choose a reason for hiding this comment

bwplotka commented Jun 12, 2024

pintohutch Jun 14, 2024

Choose a reason for hiding this comment

pintohutch Jun 14, 2024

Choose a reason for hiding this comment

bwplotka commented Jun 11, 2024 •

edited

bwplotka Jun 12, 2024 •

edited

bwplotka Jun 12, 2024 •

edited

bwplotka Jun 12, 2024 •

edited

bwplotka commented Jun 12, 2024 •

edited