feat(reconciler): add per-record auto-apply policy hook (OBS-2988)#539
feat(reconciler): add per-record auto-apply policy hook (OBS-2988)#539unlisted wants to merge 4 commits into
Conversation
Vulnerability Scan: Passed — diode-authImage:
Commit: f114219 |
Vulnerability Scan: Passed — diode-ingesterImage: No vulnerabilities found. Commit: f114219 |
Vulnerability Scan: Passed — diode-reconcilerImage: No vulnerabilities found. Commit: f114219 |
|
Go test coverage
Total coverage: 52.7% |
There was a problem hiding this comment.
Pull request overview
This PR introduces a per-record auto-apply hook to the reconciler’s plan-only ingestion log processing flow, allowing specific planned change sets to bypass the OPEN review queue and be applied immediately (without changing the existing tenant-level AUTO_APPLY_CHANGESETS processor selection).
Changes:
- Add
AutoApplyPolicy+WithAutoApplyPolicy(...)functional option toIngestionLogProcessor, and invoke it afterBulkPlan. - Extend
IngestionProcessorOpswithApplyChangeSetForLog, with an OSS implementation delegating toBulkPlanApplyfor single items. - Add tests covering the policy match/no-match paths and ensuring apply + metrics behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| diode-server/reconciler/ops.go | Adds ApplyChangeSetForLog delegating to BulkPlanApply for single-item apply. |
| diode-server/reconciler/mocks/ingestionprocessorops.go | Updates mock ops interface to include ApplyChangeSetForLog. |
| diode-server/reconciler/ingestion_processor.go | Extends IngestionProcessorOps interface with ApplyChangeSetForLog. |
| diode-server/reconciler/ingestion_log_processor.go | Adds AutoApplyPolicy option + invokes apply path on policy match. |
| diode-server/reconciler/ingestion_log_processor_autoapply_test.go | Adds unit tests verifying apply invocation + metric recording behavior. |
Files not reviewed (1)
- diode-server/reconciler/mocks/ingestionprocessorops.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
unlisted
requested review from
MicahParks,
grant-nbl,
jajeffries,
leoparente,
manrodrigues,
marc-barry,
mfiedorowicz and
paulstuart
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f86d4cd0e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mfiedorowicz
left a comment
mfiedorowicz
left a comment
There was a problem hiding this comment.
Reviewed the auto-apply hook. Structure is clean — AutoApplyPolicy + WithAutoApplyPolicy functional option are idiomatic, and "ship the hook only, wire the predicate on the diode-pro side" is a good way to land it. One main change plus a safety caveat. (Overlaps the Copilot/Codex threads; consolidating into a single recommendation.)
1. Drop ApplyChangeSetForLog entirely; call BulkPlanApply with the matched item(s) in processBatch. The method is a thin pass-through to BulkPlanApply([]{item}) that collapses the result into error/nil — and that collapse is what produces the metric bug below (a NO_CHANGES outcome returns nil and gets counted as a successful apply of N changes). AutoApplyProcessor.processBatch already consumes BulkPlanApply results directly with the right PlanErr / ApplyErr / changes > 0 switch — the plan-only processor can do the same. That removes the misleading name, the ignored *changeset.ChangeSet param, the extra IngestionProcessorOps method, and its mock, and fixes the metric for free. Worth extracting the shared result→metrics handling into one helper both processors call. (Simplest: BulkPlanApply([]{item}) per matched record; even better: collect the matched items and issue one BulkPlanApply after the loop.) A dedicated single-item method earns its place only once a real apply-only endpoint (no re-plan) exists.
2. The re-plan divergence is a safety caveat, not just perf. The policy is evaluated against the BulkPlan change set, but the apply re-plans and applies a possibly-different set. For an auto-apply gate, a divergent re-plan can apply something the policy never approved. Either document this as an accepted interim limitation or close it with the deferred apply-only endpoint (there's no interception point while BulkPlanApply does plan+apply in one server call).
Nit: the tests sync with time.Sleep(200ms) after go p.Start — flaky under load; a channel fired from a mock would be deterministic.
| if p.autoApplyPolicy != nil && result.ChangeSet != nil && | ||
| p.autoApplyPolicy(item.IngestionLog, result.ChangeSet) { | ||
| applyResults := p.ops.BulkPlanApply(ctx, []ops.QueuedIngestionLog{item}, branchID) | ||
| if len(applyResults) == 0 { |
There was a problem hiding this comment.
Acknowledged — this is valid. The consequences are housekeeping rather than correctness: the ingestion log state is overwritten to its terminal value by the second persist, and readers use latest-wins (ORDER BY cs.id DESC LIMIT 1), so the superseded plan change_set is shadowed — it just occupies a retention slot. This can't be resolved from the caller side, since the policy needs the plan output and BulkPlan persists internally; properly fixing it (along with the re-plan/policy-divergence threads) requires a dedicated apply-only netbox-plugin endpoint that applies the already-persisted change set without re-planning.
unlisted
left a comment
unlisted
left a comment
There was a problem hiding this comment.
@mfiedorowicz addressed your review across df449d2, 6d740b2, 7c07bdd:
1. Drop ApplyChangeSetForLog — done in df449d2: method, interface entry, and mock removed; processBatch consumes BulkPlanApplyResult directly with the PlanErr / ApplyErr / changes > 0 switch, which also fixes the NO_CHANGES metric miscount. 6d740b2 then takes your "even better" variant: matched items are collected during the plan loop and applied with a single BulkPlanApply call after it — one round-trip per batch instead of one per matched record.
2. Re-plan divergence — documented as an accepted interim limitation at the apply call site in 6d740b2, to be closed properly by the deferred apply-only endpoint.
Nit (test sync) — 7c07bdd drops the time.Sleep(200ms) in favor of channels closed from the final expected mock call in each flow (RecordChangeSetApply / RecordChangeSetCreate); verified stable with -count=5 -race.
unlisted
force-pushed
the
OBS-2988-cf-auto-apply
branch
from
7c07bdd to
39425ca
Compare
Summary
AutoApplyPolicytype andWithAutoApplyPolicy(...)functional option onIngestionLogProcessor. Attaches a per-record predicate that fast-paths a planned change set past the OPEN review queue, layered on top of the existing tenant-levelAutoApplyChangesetsconfig — widens auto-apply for matches, never restricts.ApplyChangeSetForLogtoIngestionProcessorOps. OSS impl delegates toBulkPlanApplywith a single-item batch (re-plans before applying; one wasted round-trip per matched record). Acceptable for current volume — a dedicated/applyendpoint on the NetBox plugin can replace the re-plan later.processBatch: afterBulkPlan, items with a non-nil change set are passed to the policy; matches go throughApplyChangeSetForLog, non-matches stay in OPEN.This PR ships only the hook. No policy is attached at the OSS call site (
cmd/reconciler/main.gois unchanged) — the concrete predicate and wiring are the diode-pro side of OBS-2988 and will follow as a separate PR:ShouldAutoApplypredicate matching pure-createextras.customfieldchange setsWithAutoApplyPolicy(ShouldAutoApply)attached at the diode-proNewIngestionLogProcessorcall sitereplacedirective against this branch while iterating; that'll be removed and therequirepin bumped once this landsRelates to OBS-2988.
Test plan
go build ./...cleango test ./reconciler/...green — including newingestion_log_processor_autoapply_test.gocovering:ApplyChangeSetForLogcalled + apply-success metric recordedApplyChangeSetForLogNOT called, row left in OPEN