Skip to content

Commit 5e73564

Browse files
zhoward-1claudeaustingreco
authored
docs: address post-merge review feedback from PR #1026 (#1074)
## Summary Addresses the four post-merge review comments left by @austingreco on #1026 after it was already merged. - **`ingester-internals.md`**: Added an overview paragraph before "Finalizer Implementation" explaining what the ingester is, restoring context that existed in the old `ingester-design.md` - **`ingester-configuration.md`**: Restored the full Pre-Existing Objects trade-off — explains orphan rows left behind when `kubectl delete` is used on objects without the finalizer, and documents both mitigations (backfill controller + API Server path), not just the workaround - **`notifications.md`**: Added a missing `## Enabling Notification Delivery` section that fixes three broken anchor links across the file (in the `:::caution` block, the `:::tip`, and Troubleshooting) - **`notifications.md`**: Removed the "Reference Files" section pointing users to internal Go source files — moved that context into the new operator-facing section above ## Test plan - [ ] Verify anchor links resolve: `#enabling-notification-delivery` appears three times in `notifications.md` and should now point to the new section - [ ] Confirm `ingester-internals.md` opens with an overview before jumping into finalizer details - [ ] Confirm `ingester-configuration.md` Pre-Existing Objects section explains the orphan row consequence and both mitigations 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Austin Greco <austingreco@gmail.com> Co-authored-by: Austin Greco <agreco@uber.com>
1 parent 3ea7c43 commit 5e73564

3 files changed

Lines changed: 26 additions & 9 deletions

File tree

docs/contributing/ingester-internals.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
This guide explains the internal design of the Ingester controller for developers who need to understand, extend, or modify the ingester code.
44

5+
## Overview
6+
7+
The Ingester is a generic Kubernetes controller that watches all 13 Michelangelo CRDs and durably syncs them into MySQL. Its purpose is to decouple metadata storage from etcd: Michelangelo's API Server and query layer can read from MySQL (faster, richer query capabilities) rather than depending solely on etcd. One `Reconciler` instance runs per CRD kind, watching only objects of that type and upserting them into a dedicated MySQL table on every create, update, or delete event.
8+
59
## Finalizer Implementation
610

711
The ingester uses Kubernetes finalizers to guarantee safe deletions: MySQL is always updated before an object is removed from etcd.

docs/operator-guides/ingester-configuration.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,9 +218,15 @@ Immutable objects (like completed PipelineRuns) no longer consume etcd memory.
218218

219219
### Pre-Existing Objects
220220

221-
Objects created before the ingester was enabled will NOT have the ingester finalizer. They are still synced to MySQL, but may not be intercepted during deletion via `kubectl delete`.
221+
Objects created before the ingester was enabled will NOT have the ingester finalizer. They are still synced to MySQL on the controller's first startup, but are not intercepted during deletion via `kubectl delete`. If a user deletes such an object directly through `kubectl`, Kubernetes removes it from etcd immediately — the ingester never sees the deletion — leaving behind an orphan row in MySQL with `delete_time IS NULL`. Over time this causes MySQL to drift out of sync with etcd.
222222

223-
**Workaround**: Ensure all deletions go through the API Server, which sets the deletion annotation that the ingester monitors.
223+
Two mitigations are available:
224+
225+
1. **Backfill controller (not yet implemented):** A periodic reconciliation controller that compares MySQL rows against etcd and soft-deletes any rows whose corresponding etcd object no longer exists. This is the cleanest long-term solution but requires additional engineering work.
226+
227+
2. **Require all deletes through the API Server:** The API Server sets the `michelangelo/Deleting` annotation instead of issuing a direct Kubernetes delete, which the ingester detects and handles correctly even without a finalizer. Enforce this operationally by restricting direct `kubectl delete` access to CRD objects.
228+
229+
Until a backfill controller is in place, option 2 is the recommended operational workaround.
224230

225231
### Schema Evolution
226232

docs/user-guides/notifications.md

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ Notifications are configured through YAML specs only. The Michelangelo Studio UI
99
:::
1010

1111
:::caution Operator Implementation Required
12-
Notification delivery is not enabled by default in open-source deployments. The notification workflow fires correctly when pipeline states change, but the email and Slack delivery activities are stubs that must be implemented by your platform operator. See the reference files below for implementation details.
12+
Notification delivery is not enabled by default in open-source deployments. The notification workflow fires correctly when pipeline states change, but the email and Slack delivery activities are stubs that must be implemented by your platform operator. See [Enabling Notification Delivery](#enabling-notification-delivery) below for details.
1313
:::
1414

1515
## When to Use Notifications
@@ -57,7 +57,7 @@ You can also combine both in a single spec — see the [full example](#full-exam
5757
Add a `notifications` field to any `PipelineRun`, `TriggerRun`, or `Pipeline` spec. Each entry in the list is one notification rule, and you can have as many rules as you need with different event types and destinations.
5858

5959
:::tip
60-
You can configure notifications in your YAML specs at any time. Messages will be delivered once your operator has enabled notification delivery.
60+
You can configure notifications in your YAML specs at any time. Messages will be delivered once your operator has [enabled notification delivery](#enabling-notification-delivery).
6161
:::
6262

6363
### Fields
@@ -195,18 +195,25 @@ To stop receiving notifications, remove the `notifications` block entirely and r
195195
- Double-check that your `notificationType`, `resourceType`, and `eventTypes` are valid values from the tables above. Typos in enum values will be silently ignored.
196196
- For email, verify the addresses in `emails` are correct.
197197
- For Slack, confirm the channel name in `slackDestinations` exists and is accessible by the platform.
198-
- Notification delivery depends on operator-level implementation. If everything looks correct in your spec but notifications still aren't arriving, check with your platform administrator.
198+
- Notification delivery depends on operator-level implementation. If everything looks correct in your spec but notifications still aren't arriving, check with your platform administrator — see [Enabling Notification Delivery](#enabling-notification-delivery) below.
199199

200200
**I'm only getting some of my notifications.**
201201

202202
- Make sure you've listed all the event types you care about. For example, if you want alerts on both success and failure, you need both `EVENT_TYPE_PIPELINE_RUN_STATE_SUCCEEDED` and `EVENT_TYPE_PIPELINE_RUN_STATE_FAILED` in your `eventTypes` list.
203203
- Each notification rule is independent. If you have separate rules for email and Slack, check that each one has the correct event types.
204204

205-
### Reference Files
205+
## Enabling Notification Delivery
206206

207-
- `go/worker/activities/notification/activities.go` — activity stubs with request types and integration comments
208-
- `go/worker/workflows/notification/workflows.go` — the workflow that invokes the activities
209-
- `go/base/notification/types/types.go` — message generation helpers
207+
Notification delivery is not active in open-source deployments by default. The notification workflow fires correctly when pipeline states change, but the email and Slack delivery steps are stubs that your platform operator must implement.
208+
209+
To enable delivery, an operator needs to provide concrete implementations for two activity stubs:
210+
211+
- **Email delivery** — implement the email send activity to connect to your SMTP provider or email API (e.g., SendGrid, SES).
212+
- **Slack delivery** — implement the Slack send activity to post messages to channels using the Slack API or incoming webhooks.
213+
214+
Once implemented and deployed, notifications configured in your specs will begin delivering automatically — no changes to your YAML are required.
215+
216+
Contact your platform administrator if notifications are configured correctly but messages are not arriving.
210217

211218
## What's Next
212219

0 commit comments

Comments
 (0)