operator: reduce Kubernetes API server traffic by jwilder · Pull Request #806 · Azure/unbounded

Jason Wilder (jwilder) · 2026-09-17T06:27:31Z

Summary

replace periodic full-set CRD maintenance with an event-driven reconciler for owned CRDs
avoid retrying deterministic override validation failures and skip unchanged Site status patches
watch useful net rollout readiness transitions and suppress unchanged server-side applies with a desired-payload hash

API impact

removes the healthy one-minute CRD apply/get cycle
avoids reconcile backoff for invalid override documents that require a user edit
avoids status writes when Site status is unchanged
turns repeated identical SSA writes into cache-backed reads after the first hash-establishing reconcile

Testing

make fmt
make test (go test -race ./...)
make lint
focused operator and component tests

Philip Lombardi (plombardi89)

Requesting changes because the new applied-hash label can make otherwise valid resources permanently fail to apply for a fixed desired payload. Inline comments also cover the live-read/cache mismatch, CRD defaulting defeating the skip check, and operational/testing follow-ups. Reviewed at c319812 against the pinned controller-runtime v0.25.0 and Kubernetes libraries v0.37.0. The cache-sizing, diagnostics, naming, and e2e comments are follow-ups, not additional demonstrated blocking failures. I have not run the kind-based e2e suite.

Philip Lombardi (plombardi89) · 2026-09-18T14:34:00Z

+
+	sum := sha256.Sum256(data)
+
+	return base64.RawURLEncoding.EncodeToString(sum[:]), nil


Blocking: base64url can produce an invalid Kubernetes label value. Kubernetes requires a nonempty label value to start and end with an alphanumeric character, but this encoding can start with - or _. For a uniformly distributed SHA-256 digest that is 2/64 of possible leading characters. The apiserver then rejects the apply, and retries cannot fix it because the same desired payload always produces the same invalid label. This affects every resource sent through ApplyObject, not just workloads.

Please use an encoding that is always label-safe, such as a truncated hex digest or an alphanumeric prefix on this base64url value. The test's ^[A-Za-z0-9_-]+$ check misses the start/end constraint; use Kubernetes' validation.IsValidLabelValue and include a case whose unprefixed hash starts with - or _. The relevant validation is in apimachinery v0.37.0, pkg/api/validate/content/kube.go:24-26,74-91.

Fixed. The hash now has an alphanumeric-safe representation and the unit test validates its Kubernetes label constraints. The real kind e2e also passes with the marker applied to resources.

Philip Lombardi (plombardi89) · 2026-09-18T14:34:00Z

+	current := &unstructured.Unstructured{}
+	current.SetGroupVersionKind(desired.GroupVersionKind())
+
+	key := client.ObjectKeyFromObject(desired)
+	if err := e.Client.Get(ctx, key, current); err == nil &&
+		current.GetLabels()[AppliedHashLabel] == hash && desiredFieldsMatch(desired.Object, current.Object) {


The lookup here bypasses the manager cache. current is an *unstructured.Unstructured, while the manager's default client has CacheOptions{Unstructured: false}. In controller-runtime v0.25.0, see pkg/cluster/cluster.go:206-210 and pkg/client/client.go:311-314. This manager does not override that option.

Consequently an unchanged resource costs a live GET instead of an apply, and a cache/hash miss costs a GET plus an apply. Avoiding SSA processing can still help, but this does not implement the advertised cache-backed reads or eliminate the per-object API request. Please use an intentional cached read path, with appropriate cache scope, or correct the documentation/PR claim and measure the GET-versus-apply tradeoff. A fake-client apply-count test cannot detect this distinction.

Fixed by configuring the manager delegated client with client.CacheOptions{Unstructured: true}. The no-op lookup now uses the manager cache rather than silently falling through to a live unstructured GET.

Philip Lombardi (plombardi89) · 2026-09-18T14:34:01Z

+	return apiequality.Semantic.DeepEqual(current.Labels, wanted.Labels) &&
+		apiequality.Semantic.DeepEqual(current.Annotations, wanted.Annotations) &&
+		apiequality.Semantic.DeepEqual(current.Spec, wanted.Spec)


Server defaulting prevents this equality check from recognizing an unchanged CRD. For example, the embedded Site CRD omits spec.conversion, but the apiserver defaults it to {strategy: None} (apiextensions-apiserver v0.37.0, pkg/apis/apiextensions/v1/defaults.go:49-53). Converting the manifest with FromUnstructured does not apply those defaults, so current.Spec and wanted.Spec differ even in a healthy cluster and each delivered event takes the apply path.

Whole-map equality on labels/annotations also treats harmless third-party metadata as drift, even though this SSA payload does not remove independently owned keys. Please account for API defaults and compare the intended metadata rather than requiring exact map equality. Add coverage with a server-defaulted CRD and an extra user annotation; the existing test seeds the fake client directly from the desired manifest and misses both cases.

Fixed. CRDs now carry a digest of the embedded desired payload; server-defaulted fields and independently owned metadata no longer defeat the skip. Added coverage for a defaulted CRD with an extra third-party annotation.

Philip Lombardi (plombardi89) · 2026-09-18T14:34:01Z

+
+	return ctrl.NewControllerManagedBy(mgr).
+		Named("owned-crd").
+		For(&apiextensionsv1.CustomResourceDefinition{}, builder.WithPredicates(predicate.Funcs{


Operational follow-up: this caches all CRDs, not just the owned set. The owned predicate filters events after the informer has listed/cached objects. DefaultNamespaces in the manager does not restrict cluster-scoped CRDs, so this introduces a full-schema LIST/watch/cache for every CRD in the cluster, replacing the previous direct maintenance of the operator's fixed set.

Please measure the startup and resident-memory impact on a CRD-heavy cluster and either document the tradeoff or scope the cache. This is an unmeasured scaling risk, not a demonstrated OOM. If scoping by an ownership label, account for repairing removal of that label; simply hiding unlabeled objects from the cache can undermine drift repair.

Agreed this remains a tradeoff: the CRD watch caches all cluster CRDs because name predicates filter after the informer. I have not claimed this is scoped away. The cluster already grants list/watch and typical CRD counts are bounded, but a label-scoped cache would undermine repair after label removal. I am leaving measurement/scoping as a follow-up rather than introducing a correctness gap here.

Philip Lombardi (plombardi89) · 2026-09-18T14:34:01Z

+	current.SetGroupVersionKind(desired.GroupVersionKind())
+
+	key := client.ObjectKeyFromObject(desired)
+	if err := e.Client.Get(ctx, key, current); err == nil &&


Diagnostics follow-up: falling back to apply on a failed read is reasonable, but non-NotFound errors disappear entirely when the apply succeeds. A persistent GET failure would silently disable this optimization and add failed reads on every pass. Consider a low-verbosity diagnostic or metric for non-NotFound read failures while preserving the authoritative-write fallback. This is an observability suggestion, not a reason to fail reconciliation on a cache miss.

The manager now caches unstructured objects, so these normal reads no longer produce live GET failures. Read errors still intentionally fall through to the authoritative apply.

Philip Lombardi (plombardi89) · 2026-09-18T14:34:01Z

-	if !strings.Contains(err.Error(), "0 workload(s) left unchanged") {
-		t.Fatalf("error = %v, want it to report that nothing was withheld", err)
+	if err != nil {
+		t.Fatalf("rejected entry should wait for the ConfigMap watch: %v", err)


Test-maintenance follow-up: this assertion now requires success, but the enclosing test is still named TestRejectedEntryFailsThePassEvenWhenNothingIsWithheld. Please rename it to describe waiting for an input change without returning an error. The reconcileExpectingError helper in the e2e suite is also now used for successful passes; a neutral name would avoid suggesting the opposite contract to future readers.

Fixed. Renamed the unit test to describe waiting for an input change and renamed the e2e helper to the neutral reconcileOnce.

Philip Lombardi (plombardi89) · 2026-09-18T14:34:01Z


-	if err := f.reconcileExpectingError(ctx, t); err == nil {
-		t.Fatal("an invalid document must fail the pass so it requeues")
+	if err := f.reconcileExpectingError(ctx, t); err != nil {


Verification follow-up: this file is guarded by //go:build e2e, so the listed make test / go test -race ./... run does not compile or execute these changed assertions. Please run the operator e2e suite with its real apiserver fixture and record that result, or explicitly note that it was not run. This matters particularly for this PR because fake clients do not exercise real label validation and server defaulting, which are implicated in the apply/hash changes. The revised assertion itself looks consistent with the intended override behavior.

Verified by CI after the fixes: operator reaper e2e (kind) passed at head 0436671, including the real-apiserver override, label validation, SSA ownership, and drift scenarios. The updated branch also passes the full race suite and lint locally.

Jason Wilder (jwilder) requested a review from a team September 17, 2026 06:27

Jason Wilder (jwilder) added 6 commits September 17, 2026 23:13

operator: reduce idle API server traffic

38e1ca5

operator: watch net rollout readiness transitions

8021e7c

operator: skip unchanged server-side applies

15c4efe

operator: preserve user annotations with apply hashes

09f0733

operator: encode apply hashes as label values

6c1e41b

operator: verify desired fields before skipping apply

c319812

Jason Wilder (jwilder) force-pushed the optimize/unbounded-operator-api-calls branch from 0436671 to c319812 Compare September 17, 2026 23:15

Philip Lombardi (plombardi89) approved these changes Sep 18, 2026

View reviewed changes

Philip Lombardi (plombardi89) requested changes Sep 18, 2026

View reviewed changes

operator: address API optimization review

42cc612


		sum := sha256.Sum256(data)

		return base64.RawURLEncoding.EncodeToString(sum[:]), nil

Conversation

Jason Wilder (jwilder) commented Sep 17, 2026

Summary

API impact

Testing

Uh oh!

Philip Lombardi (plombardi89) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants