diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index c27c7441..24546efb 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -177,6 +177,8 @@ nodeSelector: | `servers.audit.tls.secretNameOverride` | Override Secret name for audit TLS cert/key | `-audit-server-cert` | | `controllerManager.additionalSensitiveResources` | Extra Secret-shaped resource types encrypted as `resource` or `group/resource` | `[]` | | `controllerManager.allowInsecureGitHTTP` | Permit credentials with `http://` GitProvider URLs for trusted in-cluster development Git servers | `false` | +| `controllerManager.branchWorkerQueueDepth` | Event queue depth per branch worker. A full queue drops the write (`git_queue_drops_total`) instead of stalling the watch path, so the live attributed commit is lost even though convergence heals the mirror. Costs queue depth × payload of pod memory, which `branchBufferMaxSize` does **not** cover | `1000` | +| `controllerManager.branchBufferMaxSize` | Maximum in-memory event buffer per branch worker. Bounds the open commit window plus the writes retained for replay until a push succeeds | `8Mi` | | `auditService.type` | Service type for the dedicated audit Service | `NodePort` | | `auditService.nodePort` | Fixed NodePort for the audit Service when `auditService.type=NodePort` | `30444` | | `auditService.clusterIP` | Optional fixed ClusterIP for the dedicated audit Service | `""` | diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index cb50b80b..32febd0f 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -105,6 +105,8 @@ spec: {{- if .Values.controllerManager.allowInsecureGitHTTP }} - --allow-insecure-git-http {{- end }} + - --branch-worker-queue-depth={{ int .Values.controllerManager.branchWorkerQueueDepth }} + - --branch-buffer-max-size={{ .Values.controllerManager.branchBufferMaxSize }} {{- if .Values.logging.level }} - --zap-log-level={{ .Values.logging.level }} {{- end }} diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json index 59fc1385..ac111d82 100644 --- a/charts/gitops-reverser/values.schema.json +++ b/charts/gitops-reverser/values.schema.json @@ -103,6 +103,15 @@ "allowInsecureGitHTTP": { "type": "boolean", "description": "Permit credentials with http:// GitProvider URLs for trusted in-cluster development Git servers." + }, + "branchWorkerQueueDepth": { + "type": "integer", + "minimum": 1, + "description": "Event queue depth per branch worker. A full queue drops the write (git_queue_drops_total) rather than stalling the watch path, losing that live attributed commit. Costs queue depth x payload of pod memory, which branchBufferMaxSize does not cover." + }, + "branchBufferMaxSize": { + "type": "string", + "description": "Maximum in-memory event buffer per branch worker, as a Kubernetes resource quantity (e.g. 8Mi). Bounds the open commit window plus the writes retained for replay until a push succeeds." } } }, diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index d951207a..86cc4066 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -64,6 +64,28 @@ controllerManager: # Permit credentials with http:// GitProvider URLs. This is only for trusted # in-cluster development Git servers; production remotes should use HTTPS or SSH. allowInsecureGitHTTP: false + # Event queue depth per branch worker. One branch worker is shared by every GitTarget + # writing to one (GitProvider, branch), so pointing another GitTarget at a branch + # divides the headroom of a worker already in use. + # + # A full queue DROPS the write and counts git_queue_drops_total, rather than stalling + # the watch path behind a slow remote. Convergence heals the mirror afterwards, but the + # live attributed commit for a dropped write never happens — so raise this if + # git_queue_drops_total is ever non-zero. Size it so a bounded burst cannot overrun it: + # roughly (concurrent writers) x (GitTargets sharing the branch worker). Note that an + # administrative burst (an unpaced `kubectl delete` of many objects) is far spikier than + # the steady-state traffic the depth gauge shows you. + # + # Costs pod memory ON TOP of branchBufferMaxSize, which does NOT cover this queue: that + # cap is accounted only after an item leaves it. Budget queue depth x serialized size + # x ~6 (a queued object is held as an unstructured map), per SATURATED worker — about + # 5Mi at this default for ~800-byte resources, but the multiplier is on the payload, so + # size it against your own if you mirror megabyte-sized ConfigMaps or Secrets. + branchWorkerQueueDepth: 1000 + # Maximum in-memory event buffer per branch worker, as a Kubernetes resource quantity. + # Bounds the open commit window plus the writes retained for replay until a push + # succeeds; tripping it finalizes the window early, ignoring the commit cadence. + branchBufferMaxSize: 8Mi # cert-manager issuer shared by every certificate the chart mints. One self-signed CA # backs them all, so the issuer is genuinely cross-cutting and lives here; each server diff --git a/cmd/main.go b/cmd/main.go index a61f6cc4..b2fd8ef8 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -131,7 +131,7 @@ func main() { workerManager := git.NewWorkerManager( mgr.GetClient(), ctrl.Log.WithName("worker-manager"), - cfg.branchBufferMaxBytes, + cfg.branchWorkerLimits, cfg.sensitiveResources, ) workerManager.SetSSHHostKeyConfig(cfg.sshHostKeys) @@ -451,11 +451,11 @@ type appConfig struct { // check: the in-memory fact transport only works when the audit receiver and the resolver are // the same process, and that has to fail loudly rather than degrade into silent attribution // loss. - replicaCount int - branchBufferMaxBytes int64 - sensitiveResources types.SensitiveResourcePolicy - sshHostKeys git.SSHHostKeyConfig - credentialPolicy git.CredentialTransportPolicy + replicaCount int + branchWorkerLimits git.BranchWorkerLimits + sensitiveResources types.SensitiveResourcePolicy + sshHostKeys git.SSHHostKeyConfig + credentialPolicy git.CredentialTransportPolicy // sourceClusterQPS / sourceClusterBurst bound the rate at which the operator talks to a // source cluster reached through a GitTarget.spec.kubeConfig. A remote is reached over a // network the in-cluster config is not, so it carries client-side throttling by default. @@ -614,7 +614,19 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { var branchBufferMaxSizeFlag string fs.StringVar(&branchBufferMaxSizeFlag, "branch-buffer-max-size", branchBufferMaxSizeStr, "Maximum in-memory event buffer per branch worker, as a Kubernetes resource quantity "+ - "(e.g. 8Mi, 1Gi; default 8Mi). Bounds pod memory under bursty workloads; not user-facing.") + "(e.g. 8Mi, 1Gi; default 8Mi). Bounds the open commit window plus the writes retained "+ + "for replay until a push succeeds. It does NOT bound the event queue, which is "+ + "accounted only after a dequeue: see --branch-worker-queue-depth.") + fs.IntVar(&cfg.branchWorkerLimits.QueueDepth, "branch-worker-queue-depth", + git.DefaultBranchWorkerQueueDepth, + fmt.Sprintf("Event queue depth per branch worker (default %d). ", git.DefaultBranchWorkerQueueDepth)+ + "A branch worker is shared by every "+ + "GitTarget writing to one (GitProvider, branch), and a full queue DROPS the write "+ + "(git_queue_drops_total) rather than stalling the watch path behind a slow remote, "+ + "losing that live attributed commit even though convergence later heals the mirror. "+ + "Size it so a bounded burst cannot overrun it: roughly (concurrent writers) x "+ + "(GitTargets sharing the branch worker). Costs queue depth x payload of pod memory "+ + "ON TOP of --branch-buffer-max-size, which does not cover this queue.") var additionalSensitiveResources string fs.StringVar( &additionalSensitiveResources, @@ -674,10 +686,18 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { if err != nil { return appConfig{}, fmt.Errorf("invalid --branch-buffer-max-size %q: %w", branchBufferMaxSizeFlag, err) } - cfg.branchBufferMaxBytes, _ = bufferQuantity.AsInt64() - if cfg.branchBufferMaxBytes <= 0 { + cfg.branchWorkerLimits.MaxBufferBytes, _ = bufferQuantity.AsInt64() + if cfg.branchWorkerLimits.MaxBufferBytes <= 0 { return appConfig{}, fmt.Errorf("--branch-buffer-max-size must be > 0, got %s", branchBufferMaxSizeFlag) } + // Refused rather than defaulted: 0 is a channel with no buffer, which drops every write + // that does not find the loop already waiting. An operator who typed it meant something + // else, and the symptom -- near-total silent loss under any load -- reads like a bug in + // the watch path rather than like the number they set. + if cfg.branchWorkerLimits.QueueDepth <= 0 { + return appConfig{}, fmt.Errorf( + "--branch-worker-queue-depth must be > 0, got %d", cfg.branchWorkerLimits.QueueDepth) + } cfg.sensitiveResources, err = types.ParseSensitiveResourcePolicy(additionalSensitiveResources) if err != nil { diff --git a/cmd/main_branch_worker_limits_test.go b/cmd/main_branch_worker_limits_test.go new file mode 100644 index 00000000..3bcedaf4 --- /dev/null +++ b/cmd/main_branch_worker_limits_test.go @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "flag" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/git" +) + +func parseLimitsArgs(t *testing.T, args ...string) (appConfig, error) { + t.Helper() + return parseFlagsWithArgs(flag.NewFlagSet("manager", flag.ContinueOnError), args) +} + +// The queue depth was a compile-time constant, and the deployments that overrun it are the +// ones whose shape the constant was never sized against: a branch worker is shared by every +// GitTarget on a (GitProvider, branch), so the headroom an operator needs is a property of +// their cluster, not of this binary. Assert the knob exists and carries a value through. +func TestParseFlags_BranchWorkerQueueDepthIsConfigurable(t *testing.T) { + cfg, err := parseLimitsArgs(t, "--branch-worker-queue-depth=4096") + require.NoError(t, err) + require.Equal(t, 4096, cfg.branchWorkerLimits.QueueDepth) +} + +func TestParseFlags_BranchWorkerLimitsDefault(t *testing.T) { + cfg, err := parseLimitsArgs(t) + require.NoError(t, err) + require.Equal(t, git.DefaultBranchWorkerQueueDepth, cfg.branchWorkerLimits.QueueDepth, + "an unset queue size must take the package default, not zero") + require.Equal(t, git.DefaultBranchBufferMaxBytes, cfg.branchWorkerLimits.MaxBufferBytes) +} + +// Zero is refused rather than defaulted. An unbuffered channel drops every write that does +// not find the event loop already waiting, which presents as near-total silent loss in the +// watch path rather than as the number the operator typed. +func TestParseFlags_BranchWorkerQueueDepthRejectsNonPositive(t *testing.T) { + for _, arg := range []string{"--branch-worker-queue-depth=0", "--branch-worker-queue-depth=-1"} { + _, err := parseLimitsArgs(t, arg) + require.ErrorContains(t, err, "--branch-worker-queue-depth must be > 0", "arg %q", arg) + } +} + +// The depth is deliberately flag-only, with no environment fallback. Every install path +// this repo ships configures the manager by argv -- the chart renders the flag +// unconditionally, and the config/ manifests pass args too -- so an env seam here would be +// a second configuration path that no install could actually reach, and one whose parse +// order could fail a startup the explicit flag had already settled. Asserted so it does not +// quietly come back. +func TestParseFlags_BranchWorkerQueueDepthIgnoresEnvironment(t *testing.T) { + t.Setenv("BRANCH_WORKER_QUEUE_DEPTH", "2500") + cfg, err := parseLimitsArgs(t) + require.NoError(t, err) + require.Equal(t, git.DefaultBranchWorkerQueueDepth, cfg.branchWorkerLimits.QueueDepth, + "the depth is flag-only; an env var must not set it") + + // An unparseable value cannot fail a startup, because nothing reads it. + t.Setenv("BRANCH_WORKER_QUEUE_DEPTH", "not-a-number") + cfg, err = parseLimitsArgs(t, "--branch-worker-queue-depth=77") + require.NoError(t, err) + require.Equal(t, 77, cfg.branchWorkerLimits.QueueDepth) +} diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go index bf5cee0a..2b068fea 100644 --- a/cmd/main_chart_args_test.go +++ b/cmd/main_chart_args_test.go @@ -11,6 +11,8 @@ import ( "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" "sigs.k8s.io/yaml" + + "github.com/ConfigButler/gitops-reverser/internal/git" ) // chartInputs are read by the test process itself so `go test` records them as cache @@ -69,9 +71,13 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantAuditTLSCert string wantHTTP2 bool wantGitHTTP bool + wantQueueDepth int + wantBufferBytes int64 }{ "chart defaults": { - wantKeyPrefix: "gitops-reverser", + wantKeyPrefix: "gitops-reverser", + wantQueueDepth: git.DefaultBranchWorkerQueueDepth, + wantBufferBytes: git.DefaultBranchBufferMaxBytes, }, // servers.enableHTTP2 was a value no template read: setting it changed nothing, while // its comment promised an HTTP/2 Rapid-Reset mitigation. Assert it reaches the binary. @@ -124,6 +130,18 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantKeyPrefix: "gitops-reverser", wantGitHTTP: true, }, + // The branch-worker capacity knobs exist BECAUSE one compile-time value cannot be + // right for every deployment, so an operator raising them is the whole point of the + // flags: render an override and confirm it survives the trip to the binary. + "raised branch-worker capacity": { + setValues: []string{ + "controllerManager.branchWorkerQueueDepth=4000", + "controllerManager.branchBufferMaxSize=32Mi", + }, + wantKeyPrefix: "gitops-reverser", + wantQueueDepth: 4000, + wantBufferBytes: 32 * 1024 * 1024, + }, } for name, tc := range tests { @@ -150,6 +168,12 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { "servers.enableHTTP2 must reach the binary, and default to off") require.Equal(t, tc.wantGitHTTP, cfg.credentialPolicy.AllowInsecureGitHTTP, "controllerManager.allowInsecureGitHTTP must reach the binary") + if tc.wantQueueDepth != 0 { + require.Equal(t, tc.wantQueueDepth, cfg.branchWorkerLimits.QueueDepth, + "controllerManager.branchWorkerQueueDepth must reach the binary") + require.Equal(t, tc.wantBufferBytes, cfg.branchWorkerLimits.MaxBufferBytes, + "controllerManager.branchBufferMaxSize must reach the binary") + } }) } } diff --git a/docs/interpreting-metrics.md b/docs/interpreting-metrics.md index 42b84dbc..a2b66fa9 100644 --- a/docs/interpreting-metrics.md +++ b/docs/interpreting-metrics.md @@ -209,6 +209,44 @@ boundary, the commit, the push. Background: | `placement_kustomization_entries_total` | counter | `outcome`, `gittarget_namespace`, `gittarget_name` | `added` / `no_change` / `failed`. | | `git_resync_failures_total` | counter | `gittarget_namespace`, `gittarget_name` | Rule-change resyncs whose apply failed **after** enqueue. | +### Sizing the branch worker queue against `git_queue_drops_total` + +`git_queue_drops_total{kind="write"}` is the one loss the end state hides. The enqueue onto a +branch worker is deliberately non-blocking, so a full queue throws the write away rather than +stalling the watch path behind a slow remote; the next resync's mark-and-sweep then heals the +mirror, and the target goes on reporting `RenderMatchesLive=True`. What does not come back is the +live, attributed commit for the dropped write — the change happened, and the history does not say +so. Treat any sustained increment as a signal to raise +`controllerManager.branchWorkerQueueDepth`. + +**The depth gauge will not warn you first.** `git_queue_depth` shows the traffic you have, and the +bursts that overrun the queue are usually the traffic you do not: an unpaced `kubectl delete` of a +few hundred objects arrives orders of magnitude faster than the application writes the same objects +did. A workload that sits comfortably at a fraction of the depth can still drop under an +administrative one. + +**Size it by burst, not by rate.** A queue slot holds one write request, so what has to fit is the +number of concurrent write requests a bounded burst can produce — roughly +`(concurrent writers) × (GitTargets sharing the branch worker)`. A burst that cannot exceed the +depth cannot drop at all, whatever its arrival shape. Note the second factor: workers are keyed by +`(GitProvider namespace, GitProvider name, branch)`, so pointing a second GitTarget at a branch +halves the headroom of a worker already in use, and nothing in the API surface says so. Confirm +which targets share one worker by joining on `git_branch_targets` — several targets against a +single `git_queue_depth` series is one worker serving all of them. + +**What it costs.** `--branch-buffer-max-size` does **not** cover this queue. That cap bounds the +open commit window plus the writes retained for replay until a push succeeds, and it is accounted +only once an item *leaves* the queue — so queue depth × payload is additional pod memory, on top of +it. + +Budget it as **queue depth × serialized size × ~6**. A queued object is held as an unstructured +map, which measures around six times the bytes it serializes to; the channel itself is negligible +beside that. So a full queue of 1000 at the ~800-byte resources of a small CRD is about 5 MiB per +saturated worker, against a 1Gi default limit — but the multiplier is on the payload, so a folder +mirroring megabyte-sized ConfigMaps or Secrets reaches gigabytes at the same depth. Size against +your own payloads, and against how many workers could saturate at once: the figure is per worker, +and workers are per `(GitProvider, branch)`. + ### Outcomes are classified, not ranked Several values above are the pipeline working, and a dashboard that paints every non-happy outcome diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index ee95515d..a2656eb7 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -99,7 +99,7 @@ var _ = BeforeSuite(func() { // Initialize WorkerManager for new architecture workerManager := git.NewWorkerManager( - mgr.GetClient(), logf.Log.WithName("worker-manager"), 0, types.SensitiveResourcePolicy{}, + mgr.GetClient(), logf.Log.WithName("worker-manager"), git.BranchWorkerLimits{}, types.SensitiveResourcePolicy{}, ) err = mgr.Add(workerManager) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 93748784..6e810fb3 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -32,9 +32,6 @@ import ( ) const ( - // branchWorkerQueueSize is the size of the event queue for each branch worker. - branchWorkerQueueSize = 100 - // metadataCacheDuration is how long metadata is considered fresh before re-fetching. // This optimization prevents redundant Git fetches when multiple GitTargets // share the same branch and reconcile within a short time window. @@ -226,23 +223,20 @@ const ( windowFinalizeReasonShutdown windowFinalizeReason = "shutdown" ) -// NewBranchWorker creates a worker for a (provider, branch) combination. -// Pass 0 (or a negative value) for branchBufferMaxBytes to use -// DefaultBranchBufferMaxBytes. +// NewBranchWorker creates a worker for a (provider, branch) combination. A zero +// BranchWorkerLimits gets the defaults for both knobs; see BranchWorkerLimits. func NewBranchWorker( client client.Client, log logr.Logger, providerName, providerNamespace string, branch string, writer *contentWriter, - branchBufferMaxBytes int64, + limits BranchWorkerLimits, ) *BranchWorker { if writer == nil { writer = newContentWriter(itypes.SensitiveResourcePolicy{}) } - if branchBufferMaxBytes <= 0 { - branchBufferMaxBytes = DefaultBranchBufferMaxBytes - } + limits = limits.withDefaults() return &BranchWorker{ GitProviderRef: providerName, GitProviderNamespace: providerNamespace, @@ -254,9 +248,9 @@ func NewBranchWorker( "branch", branch, ), contentWriter: writer, - eventQueue: make(chan WorkItem, branchWorkerQueueSize), + eventQueue: make(chan WorkItem, limits.QueueDepth), pendingResyncs: make(map[resyncKey]*pendingResync), - branchBufferMaxBytes: branchBufferMaxBytes, + branchBufferMaxBytes: limits.MaxBufferBytes, } } @@ -734,7 +728,7 @@ func (w *BranchWorker) processEvents() { loop := newBranchWorkerEventLoop(w, DefaultCommitWindow) w.Log.Info("Branch worker event loop configured", "defaultCommitWindow", DefaultCommitWindow.String(), - "queueSize", cap(w.eventQueue), + "queueDepth", cap(w.eventQueue), "branchBufferMaxBytes", w.branchBufferMaxBytes) loop.run() } diff --git a/internal/git/branch_worker_credread_test.go b/internal/git/branch_worker_credread_test.go index a201465b..bbdd1933 100644 --- a/internal/git/branch_worker_credread_test.go +++ b/internal/git/branch_worker_credread_test.go @@ -73,7 +73,7 @@ func TestCommitPendingWrites_ResolvesCredentialsOncePerPushCycle(t *testing.T) { }). Build() - worker := NewBranchWorker(countingClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(countingClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = context.Background() // First commit of the cycle: fetches the remote tip, so it resolves credentials exactly once. diff --git a/internal/git/branch_worker_loop_test.go b/internal/git/branch_worker_loop_test.go index bcba6850..69533edf 100644 --- a/internal/git/branch_worker_loop_test.go +++ b/internal/git/branch_worker_loop_test.go @@ -45,7 +45,7 @@ func TestCommitWindowFor_DefaultsAndParsing(t *testing.T) { target("negative", ptrString("-2s")), target("garbage", ptrString("not-a-duration")), ).Build() - w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, 0) + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{}) ctx := t.Context() for _, tc := range []struct { @@ -163,15 +163,72 @@ func TestNewBranchWorker_DefaultsBufferCap(t *testing.T) { require.NoError(t, configv1alpha3.AddToScheme(scheme)) c := fake.NewClientBuilder().WithScheme(scheme).Build() - w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, 0) + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{}) assert.Equal(t, DefaultBranchBufferMaxBytes, w.branchBufferMaxBytes) - w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, 4096) + w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{MaxBufferBytes: 4096}) assert.Equal(t, int64(4096), w.branchBufferMaxBytes) - w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, -7) + w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{MaxBufferBytes: -7}) assert.Equal(t, DefaultBranchBufferMaxBytes, w.branchBufferMaxBytes, "non-positive override falls back to default") } +// The queue depth is the drop boundary, so a configured depth that did not reach the +// channel would be a knob that reads back correctly and changes nothing — the failure the +// operator cannot see, because the symptom (dropped writes) is what they set it to prevent. +func TestNewBranchWorker_QueueDepthReachesTheChannel(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configv1alpha3.AddToScheme(scheme)) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{}) + assert.Equal(t, DefaultBranchWorkerQueueDepth, cap(w.eventQueue)) + + w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: 7}) + assert.Equal(t, 7, cap(w.eventQueue)) + + w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: -1}) + assert.Equal(t, DefaultBranchWorkerQueueDepth, cap(w.eventQueue), + "non-positive override falls back to default") +} + +// The two knobs are independent: setting one must not silently reset the other, since an +// operator raising the queue has no reason to restate a buffer cap they are happy with. +func TestNewBranchWorker_LimitsAreIndependent(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configv1alpha3.AddToScheme(scheme)) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: 3}) + assert.Equal(t, 3, cap(w.eventQueue)) + assert.Equal(t, DefaultBranchBufferMaxBytes, w.branchBufferMaxBytes) + + w = NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{MaxBufferBytes: 4096}) + assert.Equal(t, DefaultBranchWorkerQueueDepth, cap(w.eventQueue)) + assert.Equal(t, int64(4096), w.branchBufferMaxBytes) +} + +// A configured depth must move the drop boundary with it, which is the behaviour the knob +// is FOR: accept exactly the configured number of pending writes, and drop the next. +func TestBranchWorker_QueueDepthBoundsAcceptedWrites(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configv1alpha3.AddToScheme(scheme)) + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + const depth = 5 + w := NewBranchWorker(c, logr.Discard(), "p", "ns", "main", nil, BranchWorkerLimits{QueueDepth: depth}) + + // The worker is never started, so nothing drains: the queue fills to exactly its depth. + for i := range depth { + assert.Truef(t, w.enqueueRequest(&WriteRequest{}), "write %d must fit the configured depth", i) + } + assert.False(t, w.enqueueRequest(&WriteRequest{}), "the write past the depth must be dropped") + assert.Equal(t, int64(depth), w.inflightItems.Load(), + "a dropped write must not be counted in flight") +} + func ptrString(s string) *string { return &s } diff --git a/internal/git/branch_worker_metrics_test.go b/internal/git/branch_worker_metrics_test.go index 49cdf6bb..011ac4a0 100644 --- a/internal/git/branch_worker_metrics_test.go +++ b/internal/git/branch_worker_metrics_test.go @@ -36,7 +36,7 @@ func newMetricsTestWorker() *BranchWorker { Log: logr.Discard(), ctx: context.Background(), contentWriter: newContentWriter(itypes.SensitiveResourcePolicy{}), - eventQueue: make(chan WorkItem, branchWorkerQueueSize), + eventQueue: make(chan WorkItem, DefaultBranchWorkerQueueDepth), branchBufferMaxBytes: DefaultBranchBufferMaxBytes, } } diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index ee9eab10..cd26f63d 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -160,7 +160,7 @@ func setupCommitPushSplitWorker(t *testing.T) (*BranchWorker, *git.Repository, s provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = ctx return worker, serverRepo, remoteURL } diff --git a/internal/git/branch_worker_test.go b/internal/git/branch_worker_test.go index 40f1d4d3..d4786d5f 100644 --- a/internal/git/branch_worker_test.go +++ b/internal/git/branch_worker_test.go @@ -60,7 +60,7 @@ func TestBranchWorker_EmptyRepository(t *testing.T) { _ = configv1alpha3.AddToScheme(scheme) client := fake.NewClientBuilder().WithScheme(scheme).Build() logger := logr.Discard() - worker := NewBranchWorker(client, logger, "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(client, logger, "test-repo", "default", "main", nil, BranchWorkerLimits{}) // Create a GitProvider in the fake client pointing to our empty repo repoConfig := &configv1alpha3.GitProvider{ @@ -92,7 +92,7 @@ func TestBranchWorker_IdentityFields(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - worker := NewBranchWorker(client, log, "my-repo", "my-namespace", "develop", nil, 0) + worker := NewBranchWorker(client, log, "my-repo", "my-namespace", "develop", nil, BranchWorkerLimits{}) if worker.GitProviderRef != "my-repo" { t.Errorf("Expected GitProviderRef 'my-repo', got %q", worker.GitProviderRef) @@ -127,7 +127,7 @@ func TestBranchWorker_EnsurePathBootstrapped_EmptyPathCreatesTemplate(t *testing require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/prod") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) @@ -176,7 +176,7 @@ func TestBranchWorker_EnsurePathBootstrapped_NonEmptyPathBootstrapsMissingFiles( require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/prod") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) @@ -215,7 +215,7 @@ func TestBranchWorker_EnsurePathBootstrapped_NoEncryptionSkipsSOPSConfig(t *test require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithoutEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/dev") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -258,7 +258,7 @@ func TestBranchWorker_EnsurePathBootstrapped_ExistingFileNotOverwritten(t *testi require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/prod") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/prod", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -291,7 +291,7 @@ func TestBranchWorker_EnsurePathBootstrapped_EnableEncryptionLaterAddsSOPSConfig require.NoError(t, k8sClient.Create(ctx, provider)) createTargetWithoutEncryption(ctx, t, k8sClient, "bootstrap-target", "default", "test-repo", "main", "clusters/dev") - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -346,7 +346,7 @@ func TestBranchWorker_EnsurePathBootstrapped_InvalidEncryptionSecretSkipsSOPSCon }, ) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -391,7 +391,7 @@ func TestBranchWorker_EnsurePathBootstrapped_MissingSOPSKeySkipsSOPSConfig(t *te }, ) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -463,7 +463,7 @@ func TestBranchWorker_EnsurePathBootstrapped_RendersAllResolvedRecipients(t *tes } require.NoError(t, k8sClient.Create(ctx, target)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) require.NoError(t, worker.EnsurePathBootstrapped("clusters/dev", "bootstrap-target", "default")) repoPath := worker.repoPathForRemote(remoteURL) @@ -510,7 +510,7 @@ func TestBranchWorker_CommitAndPushRequest_PreparesRepositoryBeforeFirstWrite(t provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = ctx request := &WriteRequest{ @@ -605,7 +605,7 @@ func TestBranchWorker_CommitAndPushRequest_NewBranchStartsFromLatestMain(t *test provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "feature", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "feature", nil, BranchWorkerLimits{}) worker.ctx = ctx // Pre-create a stale local checkout while remote main is still at commit A. @@ -744,7 +744,7 @@ func TestBranchWorker_CommitAndPushRequest_UsesProviderCommitterAndTargetMessage } require.NoError(t, k8sClient.Create(ctx, target)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = ctx request := &WriteRequest{ @@ -834,7 +834,7 @@ func TestBranchWorker_CommitAndPushRequest_UsesBatchTemplateForAtomicRequest(t * } require.NoError(t, k8sClient.Create(ctx, target)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = ctx request := &WriteRequest{ @@ -957,7 +957,7 @@ func TestBranchWorker_CommitAndPushRequest_SignsCommitWhenConfigured(t *testing. } require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = ctx request := &WriteRequest{ @@ -1054,7 +1054,7 @@ func TestBranchWorker_CommitAndPushRequest_SkipsWriteWhenSigningSecretIsInvalid( } require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = ctx request := &WriteRequest{ diff --git a/internal/git/commit_request_attach_test.go b/internal/git/commit_request_attach_test.go index 97ed4abc..c013a691 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -415,7 +415,7 @@ func TestAttach_FinalizeFailureResolvesFailed(t *testing.T) { provider.Namespace = "default" require.NoError(t, k8sClient.Create(ctx, provider)) - worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), "test-repo", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = ctx createPlainGitTarget(t, worker, "team-a", "team-a") diff --git a/internal/git/helpers_test.go b/internal/git/helpers_test.go index 2e548ba2..53e27de3 100644 --- a/internal/git/helpers_test.go +++ b/internal/git/helpers_test.go @@ -216,7 +216,7 @@ func newTestBranchWorker( }) k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() - worker := NewBranchWorker(k8sClient, logr.Discard(), providerName, "default", branch, nil, 0) + worker := NewBranchWorker(k8sClient, logr.Discard(), providerName, "default", branch, nil, BranchWorkerLimits{}) worker.ctx = context.Background() return worker, nil } diff --git a/internal/git/pending_writes_test.go b/internal/git/pending_writes_test.go index f48615f0..b038c525 100644 --- a/internal/git/pending_writes_test.go +++ b/internal/git/pending_writes_test.go @@ -78,7 +78,7 @@ func TestPendingWrites_ResolvesEncryptionOncePerUniqueTarget(t *testing.T) { Build() counting := &countingClient{Client: baseClient} - worker := NewBranchWorker(counting, logr.Discard(), providerName, "default", "main", nil, 0) + worker := NewBranchWorker(counting, logr.Discard(), providerName, "default", "main", nil, BranchWorkerLimits{}) worker.ctx = context.Background() events := []Event{ diff --git a/internal/git/prune_replay_test.go b/internal/git/prune_replay_test.go index 16ec63b0..0b5c31bf 100644 --- a/internal/git/prune_replay_test.go +++ b/internal/git/prune_replay_test.go @@ -75,7 +75,7 @@ func replayWorker(t *testing.T, objects []client.Object, fns *interceptor.Funcs) if fns != nil { builder = builder.WithInterceptorFuncs(*fns) } - worker := NewBranchWorker(builder.Build(), logr.Discard(), "provider", "default", "main", nil, 0) + worker := NewBranchWorker(builder.Build(), logr.Discard(), "provider", "default", "main", nil, BranchWorkerLimits{}) worker.ctx = context.Background() return worker } diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index 76b91709..ef0021ce 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -21,6 +21,62 @@ import ( // --branch-buffer-max-size (8Mi by default). const DefaultBranchBufferMaxBytes int64 = 8 * 1024 * 1024 +// DefaultBranchWorkerQueueDepth is the default depth of each branch worker's event +// queue. Operators override it via --branch-worker-queue-depth. +// +// It is sized so that a bounded administrative burst cannot overrun it, because a burst +// that stays under the queue depth cannot drop at ALL, whatever its arrival shape. The +// sizing input is not an event rate: a slot holds one WriteRequest, so what must fit is +// the number of concurrent write requests a burst can produce, roughly +// (concurrent writers) x (GitTargets sharing this branch worker). Note the second factor: +// workers are keyed by (provider namespace, provider name, branch), so pointing another +// GitTarget at a branch divides the per-target headroom of a worker already in use. +// +// 1000 replaces an earlier 100, which a realistic workload cleared and a scripted one did +// not: a 200-participant demo across two GitTargets peaked at depth 44, and then deleting +// those 203 objects unpaced -- hundreds of requests per second rather than the ballots' +// ~3/s -- dropped 20 writes. Convergence still healed the mirror, but a dropped write is a +// LIVE, ATTRIBUTED commit that never happened, so the history is missing something the +// end state cannot show. +const DefaultBranchWorkerQueueDepth = 1000 + +// BranchWorkerLimits bounds one branch worker's memory. The two knobs cover different +// stages of the same pipeline and neither substitutes for the other, which is why they +// travel together. +type BranchWorkerLimits struct { + // MaxBufferBytes caps totalRetainedBytes: the open commit window plus the writes + // committed locally and retained for replay until a push succeeds. Tripping it + // finalizes the window early, ignoring the commit cadence. + MaxBufferBytes int64 + + // QueueDepth is the depth of the event queue, and it is a HARD drop boundary: the + // enqueue is deliberately non-blocking, so a full queue throws the item away and + // counts git_queue_drops_total rather than stalling the watch path behind a slow + // remote. + // + // MaxBufferBytes does NOT cover this queue. That cap is accounted only once the loop + // DEQUEUES an item, so whatever is still on the channel is bounded by count alone and + // costs memory ON TOP of it. The channel itself is trivial (a WorkItem is three + // pointers); what it retains is not, because each live-event item holds a sanitized + // object as an unstructured map, measured at roughly SIX times the bytes it serializes + // to. Budget QueueDepth x serialized size x ~6 per SATURATED worker: ~5MiB at the + // default depth for ~800-byte resources, but the multiplier rides the payload, so a + // folder mirroring megabyte-sized ConfigMaps reaches gigabytes at the same depth. + QueueDepth int +} + +// withDefaults fills in the zero value of each knob, so a caller that cares about one +// need not restate the other. +func (l BranchWorkerLimits) withDefaults() BranchWorkerLimits { + if l.MaxBufferBytes <= 0 { + l.MaxBufferBytes = DefaultBranchBufferMaxBytes + } + if l.QueueDepth <= 0 { + l.QueueDepth = DefaultBranchWorkerQueueDepth + } + return l +} + // WorkerManager manages BranchWorkers. // Creates workers per (repo, branch), shared by multiple GitDestinations. // Implements controller-runtime's Runnable interface for lifecycle management. @@ -28,8 +84,8 @@ type WorkerManager struct { Client client.Client Log logr.Logger - branchBufferMaxBytes int64 - sensitiveResources types.SensitiveResourcePolicy + limits BranchWorkerLimits + sensitiveResources types.SensitiveResourcePolicy mu sync.RWMutex // lifecycleMu serialises creating and stopping workers, so one BranchKey never has two live @@ -72,25 +128,21 @@ type WorkerManager struct { renderFidelityGate *RenderFidelityGate } -// NewWorkerManager creates a new worker manager. -// branchBufferMaxBytes bounds each worker's combined buffer + unpushed-events -// memory. Pass 0 (or a negative value) to use DefaultBranchBufferMaxBytes. +// NewWorkerManager creates a new worker manager. limits bounds every worker this manager +// creates; a zero value of either knob takes that knob's default. func NewWorkerManager( client client.Client, log logr.Logger, - branchBufferMaxBytes int64, + limits BranchWorkerLimits, sensitiveResources types.SensitiveResourcePolicy, ) *WorkerManager { - if branchBufferMaxBytes <= 0 { - branchBufferMaxBytes = DefaultBranchBufferMaxBytes - } return &WorkerManager{ - Client: client, - Log: log, - branchBufferMaxBytes: branchBufferMaxBytes, - sensitiveResources: sensitiveResources, - workers: make(map[BranchKey]*BranchWorker), - renderFidelityGate: NewRenderFidelityGate(), + Client: client, + Log: log, + limits: limits.withDefaults(), + sensitiveResources: sensitiveResources, + workers: make(map[BranchKey]*BranchWorker), + renderFidelityGate: NewRenderFidelityGate(), } } @@ -209,7 +261,7 @@ func (m *WorkerManager) EnsureWorker( providerNamespace, branch, newContentWriter(m.sensitiveResources), - m.branchBufferMaxBytes, + m.limits, ) // Inject the resolver before Start: the field is read only by the event-loop // goroutine Start spawns, so setting it here (under m.mu, before that goroutine diff --git a/internal/git/worker_manager_test.go b/internal/git/worker_manager_test.go index 830c2999..b04f6571 100644 --- a/internal/git/worker_manager_test.go +++ b/internal/git/worker_manager_test.go @@ -28,7 +28,7 @@ import ( // deletes have no resource index to target. func TestWorkerManager_SetMapperInjectsIntoWorkers(t *testing.T) { client := fake.NewClientBuilder().WithScheme(setupScheme()).Build() - manager := NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, logr.Discard(), BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) mapper := typeset.NewSnapshotRegistry(typeset.Snapshot{}) manager.SetMapper(mapper) @@ -103,7 +103,7 @@ func TestWorkerManagerRegisterTarget(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - manager := NewWorkerManager(client, log, 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, log, BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -158,7 +158,7 @@ func TestWorkerManagerMultipleTargetsSameBranch(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - manager := NewWorkerManager(client, log, 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, log, BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -212,7 +212,7 @@ func TestWorkerManagerDifferentBranches(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - manager := NewWorkerManager(client, log, 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, log, BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -271,7 +271,7 @@ func TestWorkerManagerUnregisterTarget(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - manager := NewWorkerManager(client, log, 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, log, BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -341,7 +341,7 @@ func TestWorkerManagerConcurrentRegistration(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - manager := NewWorkerManager(client, log, 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, log, BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -392,7 +392,7 @@ func TestWorkerManagerGetNonexistentWorker(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - manager := NewWorkerManager(client, log, 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, log, BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) worker, exists := manager.GetWorkerForTarget("nonexistent", "default", "main") if exists { @@ -409,7 +409,7 @@ func TestWorkerManagerUnregisterNonexistent(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() log := logr.Discard() - manager := NewWorkerManager(client, log, 0, types.SensitiveResourcePolicy{}) + manager := NewWorkerManager(client, log, BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) // Unregister should be idempotent and not error err := manager.UnregisterTarget("nonexistent", "default", "repo1", "gitops-system", "main") diff --git a/internal/watch/event_router_test.go b/internal/watch/event_router_test.go index 9bdb0736..0c2e095f 100644 --- a/internal/watch/event_router_test.go +++ b/internal/watch/event_router_test.go @@ -46,7 +46,12 @@ func saveAttach(gitTargetName, gitTargetNamespace string) git.AttachCommitReques func TestServiceCommitRequest_GitTargetNotFound(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, nil, client, logr.Discard()) @@ -59,7 +64,12 @@ func TestServiceCommitRequest_GitTargetNotFound(t *testing.T) { func TestRouteEvent_NoWorker(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, nil, client, logr.Discard()) err := router.RouteEvent("provider", "team-a", "main", git.Event{Operation: "UPDATE"}) @@ -71,7 +81,12 @@ func TestRouteEvent_NoWorker(t *testing.T) { func TestGitTargetEventStreamRegistry(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, nil, client, logr.Discard()) gitDest := types.NewResourceReference("team-a-config", "team-a") stream := reconcile.NewGitTargetEventStream(gitDest.Name, gitDest.Namespace, &recordingEnqueuer{}, logr.Discard()) @@ -93,7 +108,12 @@ func TestEnqueueScopedResync_ReportsMissingWorker(t *testing.T) { }, } client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(gitTarget).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, nil, client, logr.Discard()) resultCh, enqueued, err := router.enqueueScopedResync( @@ -122,7 +142,12 @@ func TestEnqueueScopedResync_ReportsMissingWorker(t *testing.T) { func TestEnqueueScopedResync_ReportsGoneGitTargetAsTerminal(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() // no GitTarget exists - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, nil, client, logr.Discard()) _, enqueued, err := router.enqueueScopedResync( @@ -149,7 +174,12 @@ func TestEnqueueScopedResync_ReportsGoneGitTargetAsTerminal(t *testing.T) { func TestDrainScopedResync_TreatsSupersededAsSuccess(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, nil, client, logr.Discard()) resultCh := make(chan git.ResyncResult, 1) @@ -177,7 +207,12 @@ func TestDrainScopedResync_TreatsSupersededAsSuccess(t *testing.T) { func TestDrainScopedResync_CompletesSuccessfulResult(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, &Manager{Log: logr.Discard()}, client, logr.Discard()) resultCh := make(chan git.ResyncResult, 1) resultCh <- git.ResyncResult{Stats: git.ResyncStats{Created: 1}} @@ -207,7 +242,12 @@ func TestDrainScopedResync_CompletesSuccessfulResult(t *testing.T) { func TestDrainScopedResync_RefusalMarksGitPathRefused(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) mgr := &Manager{Log: logr.Discard()} router := NewEventRouter(workerManager, mgr, client, logr.Discard()) @@ -281,7 +321,12 @@ func TestServiceCommitRequest_NoWorkerResolvesNoOpenWindow(t *testing.T) { }, } client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(gitTarget).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) router := NewEventRouter(workerManager, nil, client, logr.Discard()) @@ -306,7 +351,12 @@ func TestServiceCommitRequest_RegisteredWorkerResolvesNoOpenWindow(t *testing.T) }, } client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(provider, gitTarget).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -348,7 +398,12 @@ func TestServiceCommitRequest_RegisteredWorkerResolvesNoOpenWindow(t *testing.T) func TestDrainScopedResync_QueueFullIsDrainedNotOrphaned(t *testing.T) { scheme := eventRouterScheme(t) client := fake.NewClientBuilder().WithScheme(scheme).Build() - workerManager := git.NewWorkerManager(client, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + client, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) mgr := &Manager{Log: logr.Discard()} router := NewEventRouter(workerManager, mgr, client, logr.Discard()) diff --git a/internal/watch/git_path_acceptance_test.go b/internal/watch/git_path_acceptance_test.go index 5cbeb195..c6827a4c 100644 --- a/internal/watch/git_path_acceptance_test.go +++ b/internal/watch/git_path_acceptance_test.go @@ -127,7 +127,12 @@ func TestReportGitPathRefusal_SatisfiesWorkerManagerReporter(t *testing.T) { } func TestRenderFidelityStatus_ReducesTheCurrentPlansScopes(t *testing.T) { - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) manager := &Manager{Log: logr.Discard()} manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) target := types.NewResourceReference("podinfo-test", "team-a") @@ -172,7 +177,12 @@ func (m *Manager) restartAllFidelityScopes( // now a cell — so a stream retired by a served-version change lands squarely on the live cell's // scope instead of missing it. func TestTargetWatchStream_CarriesTheRevisionItWasStartedWith(t *testing.T) { - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) manager := &Manager{Log: logr.Discard()} manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) target := types.NewResourceReference("podinfo-test", "team-a") @@ -198,7 +208,12 @@ func TestTargetWatchStream_CarriesTheRevisionItWasStartedWith(t *testing.T) { } func TestReportGitPathRefusal_RenderFidelityKeepsGitPathAccepted(t *testing.T) { - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) manager := &Manager{Log: logr.Discard()} manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) target := types.NewResourceReference("podinfo-test", "team-a") @@ -229,7 +244,12 @@ func requireGitPathEvent(t *testing.T, events <-chan event.GenericEvent) { // or an unknown target, and the caller used to discard that answer without a word — so a scope // could owe a report for ever with nothing anywhere saying why. func TestMarkRenderFidelityScopeClean_NamesAResultTheGateWouldNotTake(t *testing.T) { - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) log, lines := recordingLogger() manager := &Manager{Log: log} manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) @@ -253,7 +273,12 @@ func TestMarkRenderFidelityScopeClean_NamesAResultTheGateWouldNotTake(t *testing // gate always issues a non-zero revision, so a stream reporting under zero was started without // one; its result is unusable and its scope keeps owing a report. func TestMarkRenderFidelityScopeClean_NamesAReportWithNoRevision(t *testing.T) { - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) log, lines := recordingLogger() manager := &Manager{Log: log} manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) @@ -273,7 +298,12 @@ func TestMarkRenderFidelityScopeClean_NamesAReportWithNoRevision(t *testing.T) { // published. Publishing what the GATE currently says instead of what this drain saw removes the // race. func TestRenderFidelityStatus_PublishesTheCurrentStatusNotTheObservedOne(t *testing.T) { - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) manager := &Manager{Log: logr.Discard()} manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) target := types.NewResourceReference("podinfo-test", "team-a") diff --git a/internal/watch/owner_test.go b/internal/watch/owner_test.go index 4a5f10e5..5fc6d80c 100644 --- a/internal/watch/owner_test.go +++ b/internal/watch/owner_test.go @@ -281,7 +281,12 @@ func TestOwner_AFailedPassLeavesTheRunningPlanUntouched(t *testing.T) { m.discoveryClient = func() (apiResourceDiscovery, error) { return nil, errors.New("discovery is unavailable") } - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) m.EventRouter = NewEventRouter(workerManager, m, nil, logr.Discard()) ref := types.NewResourceReference("target", "team-a").WithUID("uid-1") @@ -434,7 +439,12 @@ func TestOwner_AStreamOutlivesThePassThatStartedIt(t *testing.T) { m := ownerTestManager() m.watchLifetime.Store(&lifetime) - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) m.EventRouter = NewEventRouter(workerManager, m, nil, logr.Discard()) gitDest := types.NewResourceReference("target", "team-a") @@ -519,7 +529,12 @@ func TestOwner_AFailedSharedRefreshAsksForAnother(t *testing.T) { // `timed_out` pass outcome the metrics doc tells operators to alert on could never be emitted. func TestOwner_APassThatIsOutOfTimeStopsAndSaysSo(t *testing.T) { m := ownerTestManager() - workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + workerManager := git.NewWorkerManager( + nil, + logr.Discard(), + git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}, + ) m.EventRouter = NewEventRouter(workerManager, m, nil, logr.Discard()) ref := types.NewResourceReference("target", "team-a").WithUID("uid-1")