Repository navigation
Retry creates and updates rejected right after the CRD is created (OwnerReferencesPermissionEnforcement) - #2103
Conversation
…sionEnforcement For a short window after the CHI CRD is created (observed 5-23s), the OwnerReferencesPermissionEnforcement admission plugin rejects creates carrying a blockOwnerDeletion owner reference with "cannot find RESTMapping", because its cached RESTMapper does not know the kind yet. The preliminary common ConfigMaps failed (logged only), leaving pods in Init until after the 300s StatefulSet wait, and a failed per-host ConfigMap aborted the reconcile with nothing to requeue it. Retry ConfigMap, Service, PDB, Secret and StatefulSet creates on that error and on transient API errors via CreateWithRetry (1s doubling, capped at 10s, ~35s total), sharing the loop with GetWithRetry. Any other Forbidden stays terminal. Fixes Altinity#2094
…rmissionEnforcement CHK objects get the same blockOwnerDeletion owner reference (Kind ClickHouseKeeperInstallation), so a CHK applied right after its CRD is created hits the same admission window. controller-runtime's back-off does not cover it: reconcileCRAuxObjectsPreliminary only logs common ConfigMap failures, so nothing is returned to requeue on. Wrap the CHK ConfigMap, PDB, Secret, Service and StatefulSet creates in CreateWithRetry, as for CHI.
On kind the CHK fix needed 6 of 7 attempts: the admission window closed during a 10s back-off. Allow 10 retries (1s, 2s, 4s, 8s, then 10s) so a slower apiserver does not run out.
Slach
left a comment
Slach
left a comment
There was a problem hiding this comment.
Review done with a multi-model panel (rejudge), with findings checked against the code.
The fix works for #2094 and the GetWithRetry → retryOn refactor behaves the same as before (5 attempts, backoff still copied by value). Object reuse across attempts is safe (controller-runtime Create doesn't decode into the object on error), and skipping PVC/Event creates is correct.
The main concern is the scope of the retry budget: the 75s schedule applies to every transient create error, not only the owner-ref admission window. Since CHI reconciles run on one worker by default, this can block other CHIs for minutes. It also nests inside the 50-attempt status-update loop through statusUpdateCM. Details are in the inline comments.
| func (c *ConfigMap) Create(ctx context.Context, cm *core.ConfigMap) (*core.ConfigMap, error) { | ||
| ctx = k8sCtx(ctx) | ||
| return c.kubeClient.CoreV1().ConfigMaps(cm.Namespace).Create(ctx, cm, controller.NewCreateOptions()) | ||
| return commonKube.CreateWithRetry(ctx, func() (*core.ConfigMap, error) { |
There was a problem hiding this comment.
Medium: this wrapper is also used by the best-effort status ConfigMap path in cr.go, which multiplies the retry budget.
CR.statusUpdateCM (pkg/controller/chi/kube/cr.go:286-293) does Update, then on NotFound calls cmm.Create → this CreateWithRetry. Its error is explicitly discarded (_ = c.statusUpdateCM(ctx, cm) in statusUpdate), but:
- Each call can now take up to ~75-82s, and it runs inside
statusUpdateRetry(50 attempts, 1s sleep). In the worst case that's 50 × ~76s ≈ 1 hour of a blocked CHI worker on a best-effort write.StatusUpdateruns on abort/failure paths and once per host in the STS reconciler. - Ordering: the CM create runs after
c.Get(ctx, namespace, name)and beforestatusUpdateCR. That stretches the read-modify-write window on the CR by the whole retry budget, which makesUpdateStatus409 conflicts more likely, and each conflict pays the create retry again.
Suggestion: use the raw (unwrapped, or owner-ref-only short) create in statusUpdateCM, and/or write the CM after statusUpdateCR.
There was a problem hiding this comment.
Fixed. statusUpdateCM now does a single create with no retry. It's best effort and gets rewritten on every status update.
| func (c *STS) Create(ctx context.Context, statefulSet *apps.StatefulSet) (*apps.StatefulSet, error) { | ||
| ctx = k8sCtx(ctx) | ||
| return c.kubeClient.AppsV1().StatefulSets(statefulSet.Namespace).Create(ctx, statefulSet, controller.NewCreateOptions()) | ||
| return commonKube.CreateWithRetry(ctx, func() (*apps.StatefulSet, error) { |
There was a problem hiding this comment.
Low-medium: Update paths that newly add a blockOwnerDeletion ownerRef hit the same Forbidden and aren't wrapped.
OwnerReferencesPermissionEnforcement checks ownerRefs that are newly added compared with the stored object. That happens on Update too. Examples: adopting a pre-existing Service/ConfigMap with the generated name, upgrading from a version or config without ownerRefs, or a CR recreated with a new UID while its children survive. During the same discovery window, STS.Update / ConfigMap.Update / Service.Update (chi and chk) get the same cannot find RESTMapping Forbidden with no retry.
For STS this is destructive: an update error leads to ErrCRUDRecreate → recreateStatefulSet (statefulset-reconciler.go:296-330, unless onUpdateFailure: abort), so the STS and its pods get deleted over an error that clears by itself in seconds.
Suggestion: wrap Update with an owner-ref-pending-only retry (not the transient one, because Update has conflict semantics). At minimum, don't map this Forbidden to recreate.
Address review on Altinity#2103: - CreateWithRetry retries only IsOwnerRefAdmissionPending. Transient errors on Create are returned as before, so a broken webhook or busy apiserver no longer holds the reconcile worker for ~75s. Admission rejects before anything is stored, so a retry cannot meet an AlreadyExists from its own earlier attempt. - Create the status ConfigMap once, without retry: it is best effort, rewritten on every status update, and sits inside statusUpdateRetry's 50-attempt loop. - Require both "cannot set blockOwnerDeletion" and "cannot find RESTMapping". - Name the retried condition in logs, log the first retry at V(1), and log "succeeded after N attempts". - Test the production budget, the Cap path, classifier negatives, and that transient Create errors are not retried.
OwnerReferencesPermissionEnforcement checks blockOwnerDeletion owner references newly added on Update too. When children exist without owner references as the CRD is created, the first write is an Update, which failed outright: the CHI pod started on empty ConfigMaps and only recovered on a later reconcile, and a failed STS or Service update leads to a recreate. - Add UpdateWithRetry (the same owner-ref-only retry) and use it for ConfigMap, Service, PDB, Secret and StatefulSet updates in both CHI and CHK. The rejected request stores nothing, so the retry cannot hit a Conflict of its own making. - The status ConfigMap writes its update and create once, with no retry, so statusUpdateRetry's loop does not repeat the wait.
matthewhudsonedb
changed the title
Fixes #2094
Right after the CHI or CHK CRD is created, clusters with OwnerReferencesPermissionEnforcement (OpenShift turns it on) reject the operator's writes for a short time with cannot find RESTMapping. Every object the operator creates has an owner reference with blockOwnerDeletion: true, so they all hit it. The plugin also checks owner references that an update adds to an object that didn't have them before. The rejection clears by itself after about 5-25 seconds, but the operator doesn't try again:
The common ConfigMaps fail and the error is only logged. Pods can't start because they mount those ConfigMaps, and nothing creates them until after the 300s StatefulSet wait.
If a per-host object fails, the CHI ends up Aborted and stays that way.
If the children already exist without owner references, the first write is an update. When that fails, the pod starts on empty ConfigMaps, and a failed StatefulSet or Service update makes the operator delete and recreate it.
This retries creates and updates when they hit that error, the same way GetWithRetry already retries reads. It waits 1s, 2s, 4s, 8s, then 10s between tries, for about 75s in total. Only this rejection is retried. Any other error, including temporary network or server errors, fails straight away as before. The rejection happens before anything is saved, so a retry can't hit AlreadyExists or a conflict caused by its own earlier attempt.
It covers ConfigMap, Service, PDB, Secret and StatefulSet creates and updates for both CHI and CHK. PVCs aren't included because the operator doesn't give them an owner reference. The status ConfigMap is written once with no retry, because it's best effort, gets rewritten on every status update, and already sits inside a 50-attempt retry loop.
I tested it on kind with --enable-admission-plugins=NodeRestriction,OwnerReferencesPermissionEnforcement, applying the CRDs, the operator and a 1-node CHI or CHK at the same time. A background loop of owner-referenced requests kept the admission cache warm, as on a busy cluster. Without it, the rejection doesn't show up on an idle kind cluster. The rejection lasted about 16-26s in each run.
Completedafter 54sAbortedafter about 5.5 minCompletedafter 61sIn the CHK run the ConfigMap needed 6 tries before it went through, which is why the retry window is about 75s rather than something shorter.