diff --git a/docs/TODO.md b/docs/TODO.md index 765141dc..761e989f 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -20,7 +20,8 @@ Nothing open. The 0.54.0 release PR's changelog was checked against what ships o discard or race an open commit window or pending live edits ([render-fidelity.md](design/support-boundary/render-fidelity.md) §6b). - [ ] **Watch the held applier**, so a switch-on or a `flux resume` is undone at once instead of - within the one-minute `heldClaimSteadyInterval`. Also makes ask #31 below exact. + within the one-minute `heldClaimSteadyInterval`. Also makes `claimed_resource_reconciled_total` + (ask #31) count at the moment of the resume. [applier-claim-held-applier-watch.md](design/applier-claim-held-applier-watch.md). - [ ] **F3: a Flux fetch is made before the hold is checked**, so a `Kustomization` resumed by hand applies the fetched artifact at once (`internal/applier/flux.go`, `Request`). @@ -51,18 +52,11 @@ Nothing open. The 0.54.0 release PR's changelog was checked against what ships o Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-asks.md). -- [ ] **#32**: a troubleshooting row in [bi-directional-flux.md](bi-directional-flux.md) for a - controller that replaces the `Kustomization`'s whole spec with an Update. Docs only. -- [ ] **#31**: a metric for an apply the claim did not start; today it is only the - `ClaimedResourceReconciled` Event, which the API server aggregates and expires. - [ ] **#34**: an e2e with an in-cluster applier and a remote source cluster, provoking one apply window collision. -- [ ] **#23**: verify with their reproduction that #431 fixed deletion-as-intent naming the - finalizing controller instead of the deleter. -- [ ] **#11**: one YAML encoder for `[CREATE]` and `[UPDATE]` bodies (indent churn). -- [ ] **#28**: `--watch-namespaces`, then an owner selector, so two instances can share one - config-plane cluster. It is the same mechanism HA needs. - [ ] **#10**: consider the namespace when placement follows siblings. Cosmetic. +- **#28** (two instances sharing one cluster) is deferred, not open work: one instance serving many + namespaces is the tenancy model, and the restart cost behind the ask is the durable queue's. ## Attribution diff --git a/docs/bi-directional-argocd.md b/docs/bi-directional-argocd.md index fd1ae1f3..fa0c7941 100644 --- a/docs/bi-directional-argocd.md +++ b/docs/bi-directional-argocd.md @@ -39,8 +39,8 @@ for each of these, and for the settings that look like a fix and are not. server-side apply under its own field manager, `gitops-reverser`, and touches nothing else of `automated`, so `prune` and `selfHeal` stay as they are. Switching automated sync on again (Argo CD's *Enable Auto-Sync*, `kubectl edit`) is undone within a minute, with a -`ClaimedResourceReconciled` Event; a sync Argo CD started in that minute runs to its end, -unchecked against the branch. +`ClaimedResourceReconciled` Event, and counted in `claimed_resource_reconciled_total`; a sync Argo CD +started in that minute runs to its end, unchecked against the branch. Deleting the applier claim, or no longer naming it from any `GitTarget`, hands the switch back as it was: syncing on its own if it did before, manual if it was. The applier claim records which in `status.previousSuspendState` when it first switches it off: `NotSuspended` for automated sync on, diff --git a/docs/bi-directional-flux.md b/docs/bi-directional-flux.md index ff529e1d..6e436f0c 100644 --- a/docs/bi-directional-flux.md +++ b/docs/bi-directional-flux.md @@ -477,8 +477,9 @@ tip, the `Kustomization`s GitOps Reverser holds ignore the new artifact, and one ## Overriding by hand - **`flux resume ks checkout`** applies the `Kustomization` at once, unchecked. Within a minute the - applier claim suspends it again, records a `ClaimedResourceReconciled` Event, and shows the apply as an - `Unresolved` operation that completes once nothing it started is running. It does so every time. + applier claim suspends it again, records a `ClaimedResourceReconciled` Event, counts it in + `claimed_resource_reconciled_total`, and shows the apply as an `Unresolved` operation that completes + once nothing it started is running. It does so every time. - **`flux reconcile ks checkout`** does nothing visible while the `Kustomization` is suspended: Flux marks the request handled without applying. - **`flux suspend ks checkout`** is taken back with a token of GitOps Reverser's. @@ -513,6 +514,7 @@ flux get ks checkout -n flux-system | --- | --- | --- | | `FluxClaim` `Configured=False`, `ApplierNotFound` | No `Kustomization` with that namespace and name in the applier claim's cluster, or Flux is not installed there | Create it, or recreate the applier claim with the right `kustomizationRef`; an omitted namespace is the applier claim's | | `FluxClaim` `Configured=False`, `SuspendOwnedElsewhere` | The manifest that applies the `Kustomization` sets `spec.suspend` | Remove the field from that manifest | +| Repeated `ClaimedResourceReconciled` Events (a rising `claimed_resource_reconciled_total`), each followed by applier claim `OperationUnresolved` | A controller writes the `Kustomization` with an Update of its whole spec that omits `spec.suspend`, such as `controllerutil.CreateOrUpdate` with a spec it builds from scratch. An Update takes no ownership of a field it removes, so `SuspendOwnedElsewhere` cannot report it, and Flux applies unchecked on each of that controller's writes until the applier claim suspends it again | Make that controller copy `spec.suspend` from the object it read into the spec it writes, or apply with server-side apply and leave the field out | | `ApplierClaimReady=False`, `SourceMismatch`, or applier claim `UnsupportedSource` | The message names the broken [`GitRepository` rule](#the-gitrepository) | Fix the `GitRepository`; the applier claim then adopts the `Kustomization`, and the apply that was waiting is asked for | | `FluxClaim` `Configured=False`, `ApplierNamespaceNotAllowed` | The `Kustomization` is in a namespace the grants do not give, such as `flux-system` | Add it to `applierNamespaces` in the `KubeProvider`'s `spec.allowedApplierClaims` (Helm: `kubeProvider.allowedApplierClaims`) | | `FluxClaim` `Configured=Unknown`, `NotBound` | A `GitTarget` names the applier claim, but cannot be judged yet: its `GitProvider` or `KubeProvider` does not exist | Create the missing object; the message names it | diff --git a/docs/future/gitops-api-open-asks.md b/docs/future/gitops-api-open-asks.md index bbc76734..a6eb4671 100644 --- a/docs/future/gitops-api-open-asks.md +++ b/docs/future/gitops-api-open-asks.md @@ -5,39 +5,18 @@ Status: **open asks from a production user**, condensed on 2026-10-24 from their left out: #30 shipped as `GitReceiver` (#432), #33 went with the prefetch in #425, #22 is fixed (`pkg/manifestanalyzer/repo.go`), #26 and #27 shipped (`git_branch_targets`, `commit_requests_total`), #25 shipped in 0.44.0 (#343), and #6 is answered by immutability that -fails at admission with "delete and recreate". Kind and field names are updated to `main` -(`FluxClaim` for `FluxTrigger`, `KubeProvider` for `ClusterProvider`, `ClaimedResourceReconciled` -for `ResumedElsewhere`). - -Their suggested order, as of revision 15: #32, then #31, then #34 when the e2e budget allows; -then #23, #11 and #10; #28 when two owners share a cluster. - -## 32. A troubleshooting row: something replaces the `Kustomization`'s whole spec - -**Ask.** One row in [the Flux guide](../bi-directional-flux.md)'s troubleshooting table: repeated -`ClaimedResourceReconciled` Events mean a controller writes the `Kustomization` with a full-spec -Update that omits `spec.suspend`; make it carry the field across. - -**Why.** The guide covers a manifest that *sets* `spec.suspend` (`SuspendOwnedElsewhere`). The more -common case in practice is a controller that never sets the field and replaces the whole spec -(`controllerutil.CreateOrUpdate` with a spec it builds from scratch). An Update that removes a field -another manager owns takes no ownership of it, so `SuspendOwnedElsewhere` can never fire: the -symptom is a resume Event plus an `OperationUnresolved` on every reconcile of that controller, and -Flux applying unchecked until the claim suspends it again. They reproduced it on purpose. The fix on -their side was three lines; finding it from the guide alone would not have been obvious. - -## 31. Count "resumed elsewhere" in a metric, not only in an `Event` - -**Ask.** A counter for an apply the claim did not start (the resume, and the `OperationUnresolved` -that follows it), labeled like `apply_requests_total`. Possibly also a timestamp in the claim's -status. - -**Why.** A resume behind the operator's back is the one way left for Flux to apply unchecked, so it -is what they most want to alarm on. Today it is only an Event, and an Event is a poor alarm source: -the API server aggregates identical ones (three resumes in three minutes showed as one Event with -`count: 3`) and drops them after an hour. They also saw the detection time vary from 2s to about -75s, which is the claim's steady poll; a watch on the held applier would close that window. That -watch is designed in [watching the held applier](../design/applier-claim-held-applier-watch.md). +fails at admission with "delete and recreate". Since revision 15: #32 is a row in +[the Flux guide](../bi-directional-flux.md#troubleshooting); #31 is +`claimed_resource_reconciled_total`, labeled by applier kind and claim; ask #23 is pinned at the +fact index, for a finalizer cleared by a PATCH or an UPDATE, in either order +(`TestFactIndex_AFinalizerClearedByAControllerNamesTheDeleter`); and #11 shipped in 0.41.0 (#290): +create and update write through one encoder, `internal/yamlstyle`, and a test fails on a second +one. A file GitOps Reverser did not write is re-indented once, on its first edit. Kind and field +names are updated to `main` (`FluxClaim` for `FluxTrigger`, `KubeProvider` for `ClusterProvider`, +`ClaimedResourceReconciled` for `ResumedElsewhere`). + +Their suggested order, as of revision 15: #34 when the e2e budget allows; then #10; #28 when two +owners share a cluster. #28 is deferred: see below for why. ## 34. An e2e for an in-cluster applier with a remote source cluster @@ -51,10 +30,11 @@ in it, but never saw the apply window withhold anything (`apply_window_collision zero), so the part that reads `managedFields` on the remote object is untested in this topology. An e2e that provokes one collision would settle it. -Two smaller things they noted: `OperationUnresolved` is recorded three times for one operation (on -the claim, the `GitTarget` and the `WatchRule`), and `apply_requests_total{outcome="sent"}` was -said to count both "the applier was asked" and "nothing needed asking". (On `main` an apply that is -not needed is never sent, so it is not counted; check whether that answers them.) +Two smaller things they noted, both answered. `OperationUnresolved` on the `GitTarget` and the +`WatchRule` is not recorded again: each object's readiness takes the reason of the one it depends +on (`ApplierClaimReady` on the `GitTarget`, `GitTargetReady` on the rule), so one operation shows on +three objects by design. And `apply_requests_total{outcome="sent"}` counts only an applier that was +asked: an apply that is not needed is never sent, so it is not counted. ## 28. Two operator instances cannot share one config-plane cluster @@ -66,13 +46,10 @@ exactly one instance reconciles a given `GitTarget`. Smallest useful first, and 2. **An explicit owner selector**, a label selector or a `spec.instance`-style field paired with an `--instance-name` flag, which covers the cluster-scoped kinds too. -**Why.** It is the same mechanism the chart's replica guard -(`charts/gitops-reverser/templates/validate-replica-count.yaml`) says is missing for HA: "Standby -replicas need ownership coordination first, so that two instances never write the same GitTarget." -They hit it moving onto a cluster that already runs an instance for its owner's own workloads: two -populations, different source clusters, providers, credentials and namespaces, and still both -instances would reconcile both sets. RBAC is no workaround: controller-runtime issues a cluster-wide -List, so a narrowed ClusterRole fails the cache sync and the manager exits. +**Why.** They hit it moving onto a cluster that already runs an instance for its owner's own +workloads: two populations, different source clusters, providers, credentials and namespaces, and +still both instances would reconcile both sets. RBAC is no workaround: controller-runtime issues a +cluster-wide List, so a narrowed ClusterRole fails the cache sync and the manager exits. **What sharing costs them**, for calibration: one Valkey holding both populations' attribution facts; one upgrade cadence; one RBAC posture that is the union of both; and, customer-visible, a @@ -80,32 +57,25 @@ restart for one owner's reasons costs the other's users their commit message, be `RenderMatchesLive` starts `Unknown` after a restart and an open window is dropped. They are not blocked: one shared instance works. -## 23. Deletion-as-intent picks the wrong actor when a controller clears the finalizer - -**Probably fixed by #431; verify with their reproduction.** #431 credits a terminating object's -removal to its deletion evidence first. - -**Ask.** Attribute a deletion-as-intent `DELETE` to the actor who **set** `deletionTimestamp`, not -to whoever makes the next modification while it is set. - -**Why.** Measured on `v0.40.1`: a human deletes a CR that their operator finalizes, and the commit -names the operator's service account. The operator's finalizer-removal `UPDATE` is itself a -modification of a terminating object, so it renders as the `DELETE` and wins the commit; the human's -own delete arrives a moment later and folds to a no-op because the file is already gone. The more -finalizers an object has, the more likely a controller's cleanup write is observed first. - -**Reproduction** (about 90 seconds): create a namespaced CR with a finalizer, mirror it with a -`ClusterWatchRule`, delete it as a human, let the controller clear the finalizer, read the commit. - -## 11. Serialize `[CREATE]` and `[UPDATE]` bodies identically - -**Ask.** One encoder for both paths. - -**Why.** A create rendered list items at 2-space indent, an update at 4, so every install churned -all the lines of a list to carry one field. Unverified since `v0.35.0`, but the two paths still -reach different encoders (`pkg/manifestanalyzer` renders via JSON to YAML; `internal/git/manifestedit` -uses `yaml.v3` with `SetIndent(2)`). Cosmetic, but it makes `git diff` on the mirror useless for -seeing what changed. +**Deferred, not built.** Step 1 was built and dropped before merge (#445). It removed two of the +four costs above, the shared Valkey and the restart reaching the other owner's users. It could not +remove the other two: + +- **One upgrade cadence stays.** The CRDs are cluster-scoped, so both instances run against one + schema, and an API break (we make them without migrations) breaks whichever instance lags. +- **One RBAC posture stays.** The ClusterRole is still cluster-wide. + +It is also not the HA mechanism the chart's replica guard +(`charts/gitops-reverser/templates/validate-replica-count.yaml`) asks for. Two instances serving +disjoint namespaces never contend for a `GitTarget`; two replicas of one instance would, and need +leader or ownership coordination instead. Against that, it adds a lasting cost: a second tenancy +model beside one instance serving many namespaces through grants (`consumerNamespaces`, +cross-namespace provider references). Every cross-namespace reference gains a "namespace this +instance does not serve" failure, and every cluster-scoped kind needs its own ownership check. + +The cost their users see is the restart dropping an open window. That is better fixed inside one +instance, for every installation: the durable pending writes in [the TODO](../TODO.md). A second +owner who needs its own upgrade cadence or RBAC needs its own cluster; #28 would give it neither. ## 10. Sibling inference can place an object in another namespace's directory diff --git a/docs/interpreting-metrics.md b/docs/interpreting-metrics.md index 3cd870ec..5099d583 100644 --- a/docs/interpreting-metrics.md +++ b/docs/interpreting-metrics.md @@ -837,6 +837,7 @@ else pushed. See [who applies this folder](configuration.md#who-applies-this-fol | --- | --- | --- | --- | | `apply_requests_total` | counter | `applier_kind`, `reason`, `outcome` | One per attempt to one applier. `applier_kind` is `kustomization` (a Flux `Kustomization`) or `application` (an Argo CD `Application`); `reason` is `ForeignCommit` / `Refusal` / `Repair` / `CatchUp`, for either kind (`CatchUp` adopts an object the API created in the folder, or converges once when the worker starts and once when a `GitTarget` first names the applier claim); `outcome` is `sent` (the applier was asked and the ask landed; it says nothing about how the apply went. What needs no apply, a converge of the commit the applier last applied or a branch the remote does not carry, the branch worker never sends, so it is not counted), `failed` (could not ask, including an applier that applies another repository or branch, an `Application` with automated sync on, or one whose running sync did not end within 5 minutes; the branch worker keeps the reason pending and decides the retry after a flush, so a failure that persists keeps counting at its backoff) or `dropped` (no `GitTarget` may still ask for that reason: each was deleted or recreated, moved to another branch or applier, is no longer admitted by its `KubeProvider`, or withdrew its consent to a `Refusal`) or `stale` (asked nothing because the branch tip moved before the ask landed; the worker decides again at once, at the new tip, and frequent stale attempts mean the branch keeps moving within an attempt's lifetime) | | `apply_revision_mismatches_total` | counter | `applier_kind` | One per apply operation that succeeded on another revision than the commit GitOps Reverser checked. A `Kustomization` applies whatever its `GitRepository` artifact holds when it reconciles; the artifact is read again just before the resume, so this is somebody fetching the source in between (a person, or the source's own interval). The apply window still withholds a late write over a newer edit; a steady rate means something else keeps fetching the source | +| `claimed_resource_reconciled_total` | counter | `applier_kind`, `claim_namespace`, `claim_name` | One per apply an applier claim did not start: a held `Kustomization` resumed (`flux resume`, or a controller writing its spec without `spec.suspend`), or a held `Application`'s automated sync switched on, by somebody else. From then until the claim takes it back, on its next look within a minute, the applier applies whatever its source holds, unchecked against the branch. The claim records a `ClaimedResourceReconciled` Event for each as well, but the API server folds identical Events into one and expires them, so alert on this counter. A `Kustomization` resumed so also shows the apply as an `Unresolved` operation on the claim | | `apply_window_collisions_total` | counter | `provider_namespace`, `provider_name`, `branch`, `outcome` | One per write by the applier, while an apply it was asked for runs, to an object the API side edited after the flush before that request: the apply arriving late over a newer edit. `outcome` is `withheld` (proven to be the applier's write, by its field manager alone, with an entry as new as the edit or newer: `kustomize-controller`, or `argocd-controller` / `argocd-application-controller`; so not mirrored: Git keeps the API's value, and a `Repair` of the applier follows, which restores it on the cluster) or `ambiguous` (the applier and another field manager wrote in the same second, so it was mirrored as ever; if it was the apply, the revert is in Git as the applier's commit). An applier entry older than the edit is not counted: it is the API's undo of an edit, mirrored | | `apply_echoes_total` | counter | `provider_namespace`, `provider_name`, `branch`, `outcome` | One per write by the applier's own field managers alone, judged by the apply window: an apply GitOps Reverser asked for landing, object by object. `outcome` is `not_mirrored` (the applier wrote the object last, at or after the earliest send of the apply windows still open, and nobody edited it in the window: the apply itself, whose content is the commit Git already holds, so it is not mirrored) or `late` (the applier's managers alone wrote the object last with no apply window open: an echo arriving more than 10 seconds after its operation completed, or an apply somebody else started; it is mirrored as an edit). `not_mirrored` rises with every apply and needs no action. A steady `late` rate means applies keep landing outside the windows, and such a write can revert a newer Git-side change to the object: look for a person or a tool applying the folder by hand (`flux reconcile`, a manual sync), or for watch events reaching the worker long after the apply | | `apply_ownership_conflicts_total` | counter | `provider_namespace`, `provider_name`, `branch` | One per object another applier owns that the branch worker kept out of a coordinated folder: a live write not mirrored, or a snapshot object neither upserted nor swept. The owner is read from Flux's owner labels (`kustomize.toolkit.fluxcd.io/name` and `/namespace`) or Argo CD's tracking annotation (`argocd.argoproj.io/tracking-id`). Any non-zero rate is a misconfiguration: another `Kustomization` or `Application` applies objects into a namespace the folder mirrors, and two appliers applying one object fight each other. The worker logs each object once, naming both appliers; narrow the `WatchRule`, or move the object to one applier | @@ -866,6 +867,9 @@ sum by (receiver_namespace, receiver_name) ( # Repair that follows; ambiguous ones may have put the applier's revert in Git. sum by (provider_namespace, provider_name, branch, outcome) ( increase(gitopsreverser_apply_window_collisions_total[1h])) > 0 +# An applier that somebody else made apply, by claim: each was unchecked against the branch. +sum by (applier_kind, claim_namespace, claim_name) ( + increase(gitopsreverser_claimed_resource_reconciled_total[1h])) > 0 # An applier stuck mid-apply: nothing more is asked of it until its operation completes. max by (provider_namespace, provider_name, branch) (gitopsreverser_apply_operations_unresolved) > 0 # Applier writes mirrored as edits because no apply window was open. A steady rate can revert @@ -1350,6 +1354,7 @@ sum by (outcome) (rate(gitopsreverser_secret_encryptions_total[5m])) | `(time() - …_fact_follower_last_success_timestamp_seconds > 600) or (…_transport_info == 1 unless on() …_fact_follower_last_success_timestamp_seconds)`, `for: 10m` | The fact follower is wedged; attribution is degrading to committer-authored cluster-wide. Both arms are needed — see below. | | `rate(gitopsreverser_git_resync_failures_total[15m]) > 0` sustained | Background resyncs are not committing; the folder relies on steady-state events to catch up. | | `increase(gitopsreverser_apply_ownership_conflicts_total[1h]) > 0` | Another `Kustomization` or `Application` applies objects into a coordinated folder; they are kept out of Git, and the log names both appliers. | +| `increase(gitopsreverser_claimed_resource_reconciled_total[1h]) > 0` | Somebody else made a held applier apply, unchecked against the branch; the labels name the applier claim. A steady rate on one claim is a controller that rewrites the `Kustomization` without `spec.suspend`. | | `increase(gitopsreverser_stale_snapshots_total{outcome="used"}[1h]) > 0` | An applier has been failing for at least 5 minutes, so a resync decided a snapshot taken while Git was ahead and reverted part of the unapplied commit. | | `gitopsreverser_api_catalog_group_versions{state="degraded"} > 0` | Part of the API surface is hidden behind a broken APIService. | | `time() - gitopsreverser_watch_plan_oldest_dirty_since_timestamp_seconds > 120`, `for: 5m` | A GitTarget cannot be planned — its source cluster or catalog is unreachable — and its mirror is not converging. The `time() -` is not optional: the gauge is a Unix timestamp, so a bare `> 120` is true of every instant since 1970 and would fire on ordinary planning. | diff --git a/internal/controller/argocdclaim_hold.go b/internal/controller/argocdclaim_hold.go index 909d37d2..ed6923ce 100644 --- a/internal/controller/argocdclaim_hold.go +++ b/internal/controller/argocdclaim_hold.go @@ -163,6 +163,7 @@ func (r *ApplierClaimReconciler) switchOff( r.event(claim, corev1.EventTypeNormal, EventReasonAdopted, "switched automated sync of Application %s off: from now on GitOps Reverser starts every sync", ref) case state.On: + recordClaimedResourceReconciled(ctx, applierKindApplication, claim) r.event(claim, corev1.EventTypeNormal, EventReasonClaimedResourceReconciled, "automated sync of Application %s was switched on outside GitOps Reverser, which switched it off "+ "again; a sync Argo CD started meanwhile was not checked against the branch", ref) diff --git a/internal/controller/argocdclaim_hold_test.go b/internal/controller/argocdclaim_hold_test.go index 83e52d2b..1474526b 100644 --- a/internal/controller/argocdclaim_hold_test.go +++ b/internal/controller/argocdclaim_hold_test.go @@ -24,6 +24,7 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/applier" "github.com/ConfigButler/gitops-reverser/internal/applier/argosim" "github.com/ConfigButler/gitops-reverser/internal/applier/fluxsim" + "github.com/ConfigButler/gitops-reverser/internal/telemetry" ) var checkoutApplication = k8stypes.NamespacedName{Namespace: "argocd", Name: "checkout"} @@ -147,9 +148,13 @@ func TestArgoCDClaim_SwitchesAutomatedSyncOff(t *testing.T) { // switched on outside GitOps Reverser is switched off again, every time, with a Normal // ClaimedResourceReconciled Event each time. func TestArgoCDClaim_AutomatedSyncSwitchedOnIsSwitchedOffAgain(t *testing.T) { + reader, err := telemetry.InitTestExporter() + require.NoError(t, err) f := newArgoHoldFixture(t, nil, applierArgoApplication(nil)) reconcileClaim(t, f.r) f.events() + _, counted := claimedResourceReconciledCount(reader, "application") + assert.False(t, counted, "an adoption is no sync somebody else started") for round := range 2 { f.switchOn(t) @@ -163,6 +168,8 @@ func TestArgoCDClaim_AutomatedSyncSwitchedOnIsSwitchedOffAgain(t *testing.T) { require.Len(t, events, 1, "an Event every time") assert.True(t, strings.HasPrefix(events[0], "Normal ClaimedResourceReconciled automated sync of Application "+ "argocd/checkout was switched on outside GitOps Reverser, which switched it off again"), events[0]) + count, _ := claimedResourceReconciledCount(reader, "application") + assert.Equal(t, int64(round+1), count, "counted every time, where the Event is aggregated") } } diff --git a/internal/controller/fluxclaim_hold.go b/internal/controller/fluxclaim_hold.go index a17adee3..2499e45f 100644 --- a/internal/controller/fluxclaim_hold.go +++ b/internal/controller/fluxclaim_hold.go @@ -8,6 +8,8 @@ import ( "sync" "time" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" @@ -22,6 +24,7 @@ import ( configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/applier" "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/telemetry" ) // A FluxClaim holds its Kustomization suspended while idle; the branch workers resume it for each @@ -274,7 +277,7 @@ func (r *ApplierClaimReconciler) hold( if err != nil { return keptCondition(claim.Status.Conditions, ApplierClaimConditionInControl, err.Error()) } - r.suspended(claim, token, wasInControl(claim) || held.inFlight()) + r.suspended(ctx, claim, token, wasInControl(claim) || held.inFlight()) return suspending(fmt.Sprintf("Kustomization %s is being suspended", ref)) case !held.suspension.Held || adopt: return r.takeOverSuspend(ctx, st, claim, held) @@ -415,13 +418,16 @@ func suspendState(suspended bool) configbutleraiv1alpha3.SuspendState { // suspended records a Kustomization just suspended: an adoption, or, for one held before, an // external resume, recorded as an Unresolved operation that completes once token is handled. -func (r *ApplierClaimReconciler) suspended(claim *configbutleraiv1alpha3.FluxClaim, token string, resumed bool) { +func (r *ApplierClaimReconciler) suspended( + ctx context.Context, claim *configbutleraiv1alpha3.FluxClaim, token string, resumed bool, +) { ref := claim.Applier() if !resumed { r.event(claim, corev1.EventTypeNormal, EventReasonAdopted, "suspended Kustomization %s: from now on GitOps Reverser starts every apply it makes", ref) return } + recordClaimedResourceReconciled(ctx, applierKindKustomization, claim) r.event(claim, corev1.EventTypeNormal, EventReasonClaimedResourceReconciled, "Kustomization %s was resumed outside GitOps Reverser, which suspended it again; the apply "+ "that resume started was not checked against the branch", ref) @@ -905,6 +911,25 @@ func (r *ApplierClaimReconciler) event(object client.Object, eventType, reason, } } +// applier_kind label values of claimed_resource_reconciled_total, as apply_requests_total spells them. +const ( + applierKindKustomization = "kustomization" + applierKindApplication = "application" +) + +// recordClaimedResourceReconciled counts an apply the applier claim did not start, beside its +// ClaimedResourceReconciled Event, which the API server aggregates and expires. +func recordClaimedResourceReconciled(ctx context.Context, applierKind string, claim client.Object) { + if telemetry.ClaimedResourceReconciledTotal == nil { + return + } + telemetry.ClaimedResourceReconciledTotal.Add(ctx, 1, metric.WithAttributes( + attribute.String("applier_kind", applierKind), + attribute.String("claim_namespace", claim.GetNamespace()), + attribute.String("claim_name", claim.GetName()), + )) +} + // publishAlsoAppliedBy sets the applier claim's status.source.alsoAppliedBy: the objects applying its // GitRepository in the applier's cluster, other than its own Kustomization, that no FluxClaim on the // same KubeProvider holds. A listing that fails publishes none. Information only (decision 2): it never diff --git a/internal/controller/fluxclaim_hold_test.go b/internal/controller/fluxclaim_hold_test.go index 9641e718..e11b7434 100644 --- a/internal/controller/fluxclaim_hold_test.go +++ b/internal/controller/fluxclaim_hold_test.go @@ -13,6 +13,7 @@ import ( "github.com/fluxcd/pkg/apis/meta" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -29,6 +30,7 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/applier" "github.com/ConfigButler/gitops-reverser/internal/applier/fluxsim" "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/telemetry" ) var checkoutKustomization = k8stypes.NamespacedName{Namespace: "flux-system", Name: "checkout"} @@ -115,9 +117,13 @@ func TestFluxClaim_AdoptsItsKustomization(t *testing.T) { // Kustomization is suspended again with a token, every time, with a Normal Event each time, and // recorded as an Unresolved operation that completes once nothing that resume started is running. func TestFluxClaim_AnExternalResumeIsSuspendedAgain(t *testing.T) { + reader, err := telemetry.InitTestExporter() + require.NoError(t, err) f := newHoldFixture(t, testFluxClaim(claimLocalProvider)) reconcileClaim(t, f.r) f.events() + _, counted := claimedResourceReconciledCount(reader, "kustomization") + assert.False(t, counted, "an adoption is no apply somebody else started") for round := range 2 { patch := []byte(`{"spec":{"suspend":false}}`) @@ -139,6 +145,8 @@ func TestFluxClaim_AnExternalResumeIsSuspendedAgain(t *testing.T) { require.Len(t, events, 1, "an Event every time") assert.True(t, strings.HasPrefix(events[0], "Normal ClaimedResourceReconciled "+ "Kustomization flux-system/checkout was resumed outside GitOps Reverser"), events[0]) + count, _ := claimedResourceReconciledCount(reader, "kustomization") + assert.Equal(t, int64(round+1), count, "counted every time, where the Event is aggregated") got := reconcileClaim(t, f.r).(*configbutleraiv1alpha3.FluxClaim) require.NotNil(t, got.Status.Operation) @@ -151,6 +159,14 @@ func TestFluxClaim_AnExternalResumeIsSuspendedAgain(t *testing.T) { assert.Len(t, f.sim.Applied(), 2, "each resume applied once, unchecked; that is what the record says") } +// claimedResourceReconciledCount reads claimed_resource_reconciled_total for the test claim. +func claimedResourceReconciledCount(reader *sdkmetric.ManualReader, applierKind string) (int64, bool) { + return telemetry.CollectInt64Sum(reader, "gitopsreverser_claimed_resource_reconciled_total", + map[string]string{ + "applier_kind": applierKind, "claim_namespace": claimKey.Namespace, "claim_name": claimKey.Name, + }) +} + // TestFluxClaim_LeavesABranchWorkersOperationAlone: a worker records its operation before it // resumes the Kustomization, so a resumed Kustomization with an operation of the worker's in flight // is that operation, not an external resume; the worker suspends it again itself. diff --git a/internal/queue/fact_index_removal_cause_test.go b/internal/queue/fact_index_removal_cause_test.go index fa3e163a..adbd2662 100644 --- a/internal/queue/fact_index_removal_cause_test.go +++ b/internal/queue/fact_index_removal_cause_test.go @@ -124,19 +124,56 @@ func TestFactIndex_RemovalNamesAnRVOnlyWriteAtItsOwnVersion(t *testing.T) { require.Equal(t, "bob", resolution.Fact.Author) } -// For a terminating object the fact at its version can be the finalizer PATCH, which says who cleared -// a finalizer rather than who asked for the deletion. Only a deletion counts. +// For a terminating object the fact at its version can be the finalizer PATCH or UPDATE, which says +// who cleared a finalizer rather than who asked for the deletion. Only a deletion counts. An operator +// clearing its finalizer with a whole-object Update (controllerutil.RemoveFinalizer, then Update) is +// the common case. func TestFactIndex_RemovalOfATerminatingObjectNeedsTheDeletion(t *testing.T) { - const route = "removal-terminating" - h := newFactIndexHarness(t, FactIndexConfig{}) - h.publish(factIndexTestStream(route), verbFact("finalizer-controller", "11", "patch")) - h.waitForFacts(1) + for _, verb := range []string{"patch", "update"} { + t.Run(verb, func(t *testing.T) { + route := "removal-terminating-" + verb + h := newFactIndexHarness(t, FactIndexConfig{}) + h.publish(factIndexTestStream(route), verbFact("finalizer-controller", "11", verb)) + h.waitForFacts(1) - h.absent(removalQuery(route, true)) + h.absent(removalQuery(route, true)) - h.publish(factIndexTestStream(route), verbFact("carol", "11", "delete")) - resolution := h.resolve(removalQuery(route, true)) - require.Equal(t, "carol", resolution.Fact.Author, "the deletion names the initiator") + h.publish(factIndexTestStream(route), verbFact("carol", "11", "delete")) + resolution := h.resolve(removalQuery(route, true)) + require.Equal(t, "carol", resolution.Fact.Author, "the deletion names the initiator") + }) + } +} + +// The reported case, in both orders: Carol deletes a resource her operator finalizes, and the +// operator's service account clears the finalizer, the write at the removal's own version. Whichever +// fact the index files first, the removal names Carol, at once. +func TestFactIndex_AFinalizerClearedByAControllerNamesTheDeleter(t *testing.T) { + for _, verb := range []string{"patch", "update"} { + for name, controllerFirst := range map[string]bool{"controller first": true, "deletion first": false} { + t.Run(verb+" "+name, func(t *testing.T) { + route := "removal-finalizer-" + verb + if controllerFirst { + route += "-controller-first" + } + h := newFactIndexHarness(t, FactIndexConfig{}) + deletion := verbFact("carol", "10", "delete") + clearing := verbFact("system:serviceaccount:team-a:operator", "11", verb) + facts := []AuthorFact{deletion, clearing} + if controllerFirst { + facts = []AuthorFact{clearing, deletion} + } + h.publish(factIndexTestStream(route), facts...) + h.waitForFacts(len(facts)) + + started := time.Now() + resolution := h.resolve(removalQuery(route, true)) + require.Equal(t, AttributionDeleteSticky, resolution.Result) + require.Equal(t, "carol", resolution.Fact.Author) + require.Less(t, time.Since(started), time.Second) + }) + } + } } // Bob relabels the object out and Carol deletes it, and both facts arrive in ONE audit batch, Carol's diff --git a/internal/telemetry/apply_requests.go b/internal/telemetry/apply_requests.go index ccda08ae..a759cbe0 100644 --- a/internal/telemetry/apply_requests.go +++ b/internal/telemetry/apply_requests.go @@ -36,6 +36,17 @@ import "go.opentelemetry.io/otel/metric" // each GitTarget carry that identity instead. var ApplyRequestsTotal metric.Int64Counter +// ClaimedResourceReconciledTotal counts the applies an applier claim did not start, labelled by +// {applier_kind, claim_namespace, claim_name}: a held Kustomization resumed, or a held Application's +// automated sync switched on, by somebody else. The applier may apply unchecked from then until the +// claim takes it back, which it does on its next look, within a minute. The ClaimedResourceReconciled +// Event on the claim says the same, but the API server aggregates identical Events and expires them, +// so alert on this instead. A Kustomization resumed so also shows its apply as an Unresolved operation. +// +// The claim is named: it holds exactly one applier, so the label is its identity and stays bounded +// by the claims that exist. +var ClaimedResourceReconciledTotal metric.Int64Counter + // ApplyRevisionMismatchesTotal counts apply operations that succeeded on another revision than the // commit GitOps Reverser checked, labelled by {applier_kind}. A Flux Kustomization applies whatever // its GitRepository's artifact holds when it reconciles; the artifact is read again just before the diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 18351c63..1e496393 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -608,6 +608,7 @@ func registerCounters() error { {"gitopsreverser_watch_plan_triggers_total", &WatchPlanTriggersTotal}, {"gitopsreverser_apply_requests_total", &ApplyRequestsTotal}, {"gitopsreverser_apply_revision_mismatches_total", &ApplyRevisionMismatchesTotal}, + {"gitopsreverser_claimed_resource_reconciled_total", &ClaimedResourceReconciledTotal}, {"gitopsreverser_apply_window_collisions_total", &ApplyWindowCollisionsTotal}, {"gitopsreverser_apply_echoes_total", &ApplyEchoesTotal}, {"gitopsreverser_stale_snapshots_total", &StaleSnapshotsTotal},