Repository navigation
CLO-188 Add events and more metrics to orchestratord (attempt 3) - #39030
Closed
Alphadelta14 wants to merge 8 commits into
Closed
Alphadelta14 wants to merge 8 commits into
Alphadelta14 wants to merge 8 commits into
Conversation
Co-authored-by: claude <noreply@anthropic.com>
Losing the leadership lease aborts the in-flight reconciliations, which drops any live step guard. Defaulting a dropped guard to `failed` counted a lease handoff as a reconciliation failure, on a process that stays up and keeps exposing the counter. A `Drop` cannot tell a cancelled future from a `?` early return, so the default now says only what is known: the step did not conclude. `orchestratord_reconciliations_total` remains the failure signal, since it is recorded from the reconciler's actual `Result` and a cancelled pass never reaches it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reconciliation is a handful of Kubernetes API calls. Measured on a settled operator, steps run 6ms to 90ms, the two that touch a generation's resources run 230ms to 420ms, and a whole pass runs 58ms to 350ms. The power-of-two spacing of `histogram_seconds_buckets` put twenty buckets across that band, which is resolution no operator question asks for. Both histograms now share one order-of-magnitude ladder, so a step's latency still reads against the whole pass it belongs to. That is 216 series on the leader replica rather than 552. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Kubernetes aggregates repeats of an event into one object carrying a count, and `kube`'s recorder keys that aggregation on everything about an event except its note, re-sending the cached note on a hit. Sharing one recorder across the process therefore pinned a failing resource's event to whichever error came first. The aggregation entry expires six minutes after its last use, but each republish refreshes it, and `k8s_controller`'s default backoff tops out at retrying every 128 to 256 seconds, so the entry of a resource that keeps failing never ages out. A Materialize applied before its backend secret exists first fails looking the secret up and then fails validating the license key, and the event went on reporting the lookup, under a count and a timestamp that both made it look current. `Publisher` now aggregates only while the note is unchanged, keeping the recorder that filed a resource's last event under a reason and reaching for a fresh one when the note differs. Repeats of one message still collapse into one event, which is what keeps a backoff from burying a resource, while a changed message files a new event. A pass that succeeds forgets what the resource reported, so the state is bounded by the resources currently reporting something. The new `failure-events` workflow drives an environment through two different failures in a row and requires the note to follow. It asserts that the note changed rather than what it says, so it does not encode either error's wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The status conditions record only the phase an environment is in now, so a rollout that has moved on leaves no account of the phases it passed through. Report each transition as a Kubernetes event, which the monitoring stack persists as a structured log. `update_status` is the single place a condition is written, and it already returns early unless the status substantively moved, comparing everything but the timestamp. That makes it an exact edge detector, so a reconciler that re-runs on every watch event still files one event per transition without tracking any state of its own. The event carries the condition's own reason and message, so `Applying`, `ReadyToPromote`, `Promoting`, `Applied`, `WaitingForApproval`, `RolloutTimeout` and `FailedDeploy` are one vocabulary across the status, `kubectl describe`, and a dashboard. Publishing happens after the status write lands, so no event claims a transition that failed to persist. Failures now report twice, once here with the phase and once from the reconciliation wrapper with the cause; the reasons tell them apart. `workflow_manually_promote` drives a promotion through every phase, so it asserts the vocabulary rather than that any event at all was published. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The severity of a lifecycle event was taken from the condition's status, so every reason carrying `UpToDate=False` reported as a warning. Three reasons carry it: `FailedDeploy` and `RolloutTimeout`, which are failures, and `WaitingForApproval`, which is not. That last one is the operator doing exactly what it was configured to do, holding a detected change until someone requests the rollout. Flagging it teaches people to ignore warnings on Materialize resources, which costs them the two that mean something. The rule moves into `transition_event_type`, which special-cases that reason and otherwise falls back to the status, so a failure reason added later still reports as a warning without having to be named there. A unit test pins the severity of every condition the controller writes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This reverts commit 8698e04, which does not build: it points `Observed::publish_failure` back at a `recorder` field that no longer exists, since the aggregation fix replaced it with `publisher`. The commit reapplies the free `publish` function verbatim, stale doc comment included, and leaves it beside the `Publisher` that superseded it with no callers. That is the rebase conflict resolved toward the wrong side rather than an intended change, so reverting it restores the file to its parent exactly. Reinstating that path would also undo the fix for the aggregation bug QA reported on the parent branch: publishing every failure through one shared recorder pins a failing resource's event note to whichever error came first, for as long as the failure lasts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 21, 2026
Alphadelta14
requested review from
SangJunBak and
jubrad
and
a lite review from Copilot
September 21, 2026 15:41
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Address reconciliation cleanup and event-publication behavior, and stabilize the event assertions.
Review effort: Lite
Findings: None
What changed in this PR
Adds Kubernetes lifecycle events and reconciliation metrics to orchestratord, with supporting tests, RBAC, documentation, and CI coverage.
Changes:
- Adds aggregated success and failure events.
- Instruments reconciliation and controller steps with metrics.
- Adds integration tests, permissions, metric documentation, and nightly CI coverage.
| File | Summary |
|---|---|
test/orchestratord/mzcompose.py |
Adds event integration tests. |
src/orchestratord/src/reconcile.rs |
Implements shared events, metrics, and reconciliation instrumentation. |
src/orchestratord/src/metrics.rs |
Registers reconciliation metrics. |
src/orchestratord/src/lib.rs |
Exposes reconciliation functionality. |
src/orchestratord/src/controller/materialize.rs |
Adds lifecycle events and step instrumentation. |
src/orchestratord/src/controller/console.rs |
Adds step instrumentation. |
src/orchestratord/src/controller/balancer.rs |
Adds step instrumentation. |
src/orchestratord/src/bin/orchestratord.rs |
Wires observability components into controllers. |
misc/helm-charts/operator/tests/clusterrole_test.yaml |
Tests event permissions. |
misc/helm-charts/operator/templates/clusterrole.yaml |
Grants event permissions. |
doc/user/data/metrics.yml |
Documents new metrics. |
ci/nightly/pipeline.template.yml |
Adds nightly event-testing coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
doy-materialize
approved these changes
Sep 22, 2026
doy-materialize
left a comment
doy-materialize
left a comment
Contributor
There was a problem hiding this comment.
this overall looks good - we should consider moving the event reporting half of this up into the k8s-controller crate at some point, although that can happen later.
(still would like to make a gentle request to try to keep prs smaller than this in the future(: )
| } | ||
|
|
||
| // NOTE: `error_action` is deliberately not delegated. Its signature names | ||
| // `k8s_controller`'s error type, which that crate does not export, so no |
Contributor
There was a problem hiding this comment.
hmmm, we should fix this (i'm surprised that rust/clippy doesn't warn about this honestly), but it doesn't have to block this pr
| K: Resource, | ||
| K::DynamicType: Default, | ||
| { | ||
| event.note = event.note.take().map(|note| truncate_note(¬e)); |
Contributor
There was a problem hiding this comment.
should this happen after the recorder_for call, so that the deduplication happens against the pre-truncated note?
alex-hunt-materialize
left a comment
alex-hunt-materialize
left a comment
Contributor
There was a problem hiding this comment.
Most of this probably belongs in our k8s-controller crate.
| // | ||
| // The exception is the pass that gives a new resource its first status, | ||
| // which carries no conditions yet and so reports no phase. | ||
| let condition = status.conditions.first().cloned(); |
Contributor
There was a problem hiding this comment.
I think we might currently have only one condition, but we should not assume that. If we later add another one, nothing points us back here to update this line. We should specifically find the one with type UpToDate.
| /// | ||
| /// [`Publisher::forget`] bounds this to the resources whose most recent | ||
| /// reconciliation reported something. | ||
| published: Mutex<BTreeMap<(String, String), (String, Recorder)>>, |
Contributor
There was a problem hiding this comment.
Can we get named types for the keys and values of this map? Or at least a line in the comment saying it's Mutex<BTreeMap<(Uid, Reason), (Note, Recorder)>>? We have three strings, and it's unclear what they are even with the large comments above them.
Member
Author
|
Alphadelta14
marked this pull request as draft
September 30, 2026 22:04
Member
Author
|
Superseded by #39426, which reimplements this on top of the reconciliation metrics and events now provided by k8s-controller 0.13.0. |
Alphadelta14
added a commit
that referenced
this pull request
Oct 1, 2026
### Motivation Part of [CLO-188](https://linear.app/materializeinc/issue/CLO-188). This reimplements #39030 on top of k8s-controller 0.13.0, which now provides reconciliation metrics and Kubernetes events itself (MaterializeInc/k8s-controller#52). The metric names, steps and lifecycle events are the same; orchestratord no longer carries its own reconciliation wrapper and event publisher. ### Description - **Dependency:** bumps `k8s-controller` to 0.13.0 with its `prometheus` feature. - **Metrics:** registers k8s-controller's Prometheus metrics under the `orchestratord` prefix and reports every controller's passes to them: - `orchestratord_reconciliations_total{controller, phase, outcome}` - `orchestratord_reconciliation_duration_seconds{controller, phase}` - `orchestratord_reconciliation_steps_total{controller, step, outcome}` - `orchestratord_reconciliation_step_duration_seconds{controller, step}` - **Steps:** the Materialize, Balancer and Console reconcilers mark named steps through the `TraceMetadata` each pass receives, with the same step names as #39030. - **Failure events:** each controller gets its own `EventRecorder`, so the event's reporting controller identifies it: `orchestratord.materialize.cloud/materialize`, `/balancer` or `/console`, with the pod name as the instance. A failed pass publishes a `ReconcileFailed` warning event on the resource, with the error and its causes as the note. - **Lifecycle events:** the Materialize controller publishes each `UpToDate` transition as an event once the status is written. The condition's reason and message become the event's. It's a warning when the environment is not up to date, except while waiting for approval. - **RBAC:** the operator's ClusterRole gains `create` and `patch` on `events.k8s.io` events. - **Metrics catalog:** `mz-metrics-catalog` only saw `metric!` invocations, so it now also recognizes `PrometheusMetrics::new("<namespace>")`. It documents those metrics by constructing them and reading back their descriptions, as it already does for tokio's runtime metrics. It now depends on `k8s-controller` and `k8s-openapi`, which makes the catalog lint slower to build. Differences from #39030 that reviewers may notice: - The failure event reason is k8s-controller's `ReconcileFailed`, not `ReconciliationFailed`. - The phase label is `phase` rather than `event_type`. - Each controller reports as itself instead of all sharing `orchestratord.materialize.cloud`. - An identical lifecycle event repeated within 10 minutes increments the existing event's count instead of creating a new event. The messages include the generation number, so this only applies when a transition really repeats. ### Verification - `mz-metrics-catalog`: a test that `PrometheusMetrics` constructions are documented with the right names and labels. - `mz-orchestratord`: a test of which lifecycle transitions are published as warnings. - Helm: the ClusterRole test asserts the events permission. - `test/orchestratord`: - A new `failure-events` workflow, with a nightly step. It drives an environment through two different failures, a missing backend secret and then an invalid license key, and checks that the `ReconcileFailed` event's note follows the current cause. - `manually-promote` now checks that each rollout phase was published as an event. - Deployments expected to fail (`post_run_check` with `expect_fail`) now also check for a `ReconcileFailed` event. ### Release notes This release will publish Kubernetes events on Materialize, Balancer and Console resources when the operator fails to reconcile them, and on each Materialize rollout phase, so `kubectl describe` shows why a resource isn't progressing. The operator's ClusterRole now includes `create` and `patch` on `events.k8s.io` events. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This replaces my unapproved stack of #38529 and #38537 which have been sitting for a few weeks.
Description
Add Warning and Normal events to orchestratord on lifecycle transitions.
Provide additional metrics to track reconciliations and stalls.
Verification
This has been running in my self-managed cluster for almost a month and has a full dashboard built around it.
I validated it with both the success cases and failure cases.