Surface OOMs in `mz_cluster_replica_statuses` #18796

umanwizard · 2023-04-17T14:07:21Z

When reporting a pod status change, check whether it's due to OOM, and report this in mz_cluster_replica_statuses

Tested manually. No automated tests because I don't know a way to force pods to OOM that doesn't take several minutes.

Partially fixes #18621

src/orchestrator-kubernetes/src/lib.rs

do-not-use-parker-timmerman · 2023-04-17T14:10:11Z

src/orchestrator/src/lib.rs

-    NotReady,
+    /// The inner element is `None` if the reason
+    /// is unknown
+    NotReady(Option<NotReadyReason>),


instead of Option<NotReadyReason> what do you think about adding an Unknown variant to NotReadyReason? It would remove one layer of nesting?

The "extra" layer of nesting matches the actual value we emit to SQL -- NULL corresponds to None, whereas an inhabited SQL value corresponds to Some.

Ahh that makes sense!

do-not-use-parker-timmerman · 2023-04-17T14:12:48Z

src/adapter/src/catalog/builtin.rs

        .with_column("status", ScalarType::String.nullable(false))
+        .with_column("reason", ScalarType::String.nullable(true))


Do these two need to be separate, or can they be folded in together? e.g. making them separate technically allows us to represent status: "ready" and reason: "OOMKilled", which should be an invalid state?

Indeed, but it also feels weird to have ready, not-ready-oom-killed, and not-ready-other as the possible values in a single column. The reason for non-readiness feels like a logically separate piece of information.

I was thinking you could distill the reasons a bit, i.e. today you could have one of three statuses:

ready

oom-killed

not-ready

Where oom-killed implies "not ready". I totally see where you're coming from though w.r.t. non-readiness being a logically separate piece of info, so I could go either way

do-not-use-parker-timmerman

Changes look good to me! At least from a Surfaces point of view

do-not-use-parker-timmerman · 2023-04-17T14:22:55Z

src/adapter/src/catalog/builtin.rs

        .with_column("status", ScalarType::String.nullable(false))
+        .with_column("reason", ScalarType::String.nullable(true))


I was thinking you could distill the reasons a bit, i.e. today you could have one of three statuses:

ready

oom-killed

not-ready

Where oom-killed implies "not ready". I totally see where you're coming from though w.r.t. non-readiness being a logically separate piece of info, so I could go either way

do-not-use-parker-timmerman · 2023-04-17T14:23:09Z

src/orchestrator/src/lib.rs

-    NotReady,
+    /// The inner element is `None` if the reason
+    /// is unknown
+    NotReady(Option<NotReadyReason>),


Ahh that makes sense!

teskje

The idea seems sound, I just have a couple improvement suggestions.

Also note that in mz_cluster_replica_statuses you will need to be fast/lucky to see the "reason" since the replica will be restarting quickly (unless it's crashlooping). Is this relation one of the "retained metrics" ones?

src/adapter/src/catalog.rs

src/orchestrator-kubernetes/src/lib.rs

src/orchestrator/src/lib.rs

teskje · 2023-04-17T14:26:31Z

src/adapter/src/notice.rs

+            AdapterNotice::ClusterReplicaStatusChanged { status, .. } => {
+                match status {
+                    ServiceStatus::NotReady(None) => Some("The cluster replica may be restarting or going offline.".into()),
+                    ServiceStatus::NotReady(Some(NotReadyReason::OOMKilled)) => Some("The cluster replica may have run out of memory and been killed.".into()),


Why do you use "may" here? If k8s says the replica was OOM-killed, is there a chance that this is not correct?

I don't know -- I doubt it! I was just trying to match the language in the other similar notice (the NotReady(None) case). Probably we should let @mjibson or someone else on Surfaces decide what the correct language is here for both cases.

src/adapter/src/catalog/builtin_table_updates.rs

umanwizard · 2023-04-17T14:38:08Z

@teskje

Is this relation one of the "retained metrics" ones?

No, but as a follow-up PR, we will make it one (assuming the memory requirements are acceptable -- I'll do some math first and post it in the comment of that PR).

For more details see the discussion here.

benesch · 2023-04-17T16:17:47Z

Tested manually. No automated tests because I don't know a way to force pods to OOM that doesn't take several minutes.

Eep, hang on! This needs a test, or it is virtually certain to regress. Let's figure out how to write one, possibly involving the QA team if necessary.

umanwizard · 2023-04-17T17:04:29Z

Yes, Philip has already helped me find something, so I'm planning to add a test today

…

On Mon, Apr 17, 2023 at 12:17 PM Nikhil Benesch ***@***.***> wrote: Tested manually. No automated tests because I don't know a way to force pods to OOM that doesn't take several minutes. Eep, hang on! This needs a test, or it is virtually certain to regress. Let's figure out how to write one, possibly involving the QA team if necessary. — Reply to this email directly, view it on GitHub <#18796 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOOGJ4IU3L6KKNR3JDBFA3XBVULNANCNFSM6AAAAAAXBHW6QE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

umanwizard · 2023-04-17T20:38:14Z

test Pr is here: #18806

Retroactively adding a release note and patching the docs after #18796. --------- Co-authored-by: umanwizard <brennan@umanwizard.com>

Surface OOMs in mz_cluster_replica_statuses

834c001

umanwizard requested a review from teskje April 17, 2023 14:07

umanwizard requested review from benesch and a team as code owners April 17, 2023 14:07

umanwizard requested a review from a team April 17, 2023 14:08

do-not-use-parker-timmerman reviewed Apr 17, 2023

View reviewed changes

Remove stray code

85f7379

do-not-use-parker-timmerman approved these changes Apr 17, 2023

View reviewed changes

teskje reviewed Apr 17, 2023

View reviewed changes

Address comments

1ca6783

umanwizard merged commit 9679528 into MaterializeInc:main Apr 17, 2023

materialize-bot mentioned this pull request Apr 21, 2023

release: v0.52.0 required reviews #18907

Closed

12 tasks

This was referenced Apr 27, 2023

doc/user: polish v0.52 release notes #19000

Merged

doc/user: add release note about OOM observability #19032

Merged

morsapaes added a commit that referenced this pull request Apr 28, 2023

doc/user: add release note about OOM observability (#19032)

450c24d

Retroactively adding a release note and patching the docs after #18796. --------- Co-authored-by: umanwizard <brennan@umanwizard.com>

morsapaes mentioned this pull request May 31, 2023

integrations: expand the metrics in Datadog and Grafana MaterializeInc/demos#75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surface OOMs in `mz_cluster_replica_statuses` #18796

Surface OOMs in `mz_cluster_replica_statuses` #18796

umanwizard commented Apr 17, 2023 •

edited

Loading

do-not-use-parker-timmerman Apr 17, 2023

umanwizard Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

umanwizard Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

do-not-use-parker-timmerman left a comment

do-not-use-parker-timmerman Apr 17, 2023

do-not-use-parker-timmerman Apr 17, 2023

teskje left a comment

teskje Apr 17, 2023

umanwizard Apr 17, 2023

umanwizard commented Apr 17, 2023

benesch commented Apr 17, 2023

umanwizard commented Apr 17, 2023 via email

umanwizard commented Apr 17, 2023

		.with_column("status", ScalarType::String.nullable(false))
		.with_column("reason", ScalarType::String.nullable(true))

Surface OOMs in mz_cluster_replica_statuses #18796

Surface OOMs in mz_cluster_replica_statuses #18796

Conversation

umanwizard commented Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

do-not-use-parker-timmerman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teskje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umanwizard commented Apr 17, 2023

benesch commented Apr 17, 2023

umanwizard commented Apr 17, 2023 via email

umanwizard commented Apr 17, 2023

Surface OOMs in `mz_cluster_replica_statuses` #18796

Surface OOMs in `mz_cluster_replica_statuses` #18796

umanwizard commented Apr 17, 2023 •

edited

Loading