Repository navigation
fix(k8s): keep the annotations of the Sentinel Deployment on update - #332
Conversation
The desired Sentinel Deployment leaves out fields that the API server defaults, for example the probe periods. Thus the operator updates the Deployment in each reconcile. The update had no annotations, so it removed deployment.kubernetes.io/revision, and the API server incremented the generation. The wait of the master replacement for the Sentinel rollout counts per generation, so it started again in each reconcile. The soak showed its Info line in each reconcile, and the 10-minute bound was never reached. The update keeps the stored annotations, so the generation changes only with a change of the spec. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4mmyo3sqLwf23m5FE2nBE
An update of a Deployment without annotations sends no empty map, as before the merge of the stored annotations. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4mmyo3sqLwf23m5FE2nBE
…ed one The test fails when the merge gives the stored annotations priority. The comments state why a comparison of the annotations is safe and what the update count of the Sentinel rollout test checks. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4mmyo3sqLwf23m5FE2nBE
|
@codex review Generated by Claude Code |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Changes proposed on the PR:
CreateOrUpdateDeploymentkeeps the stored annotations. The API server increments the generation of a Deployment when its annotations change. The operator updates the Sentinel Deployment in each reconcile, because the generated spec leaves out API defaults such as the probe periods. Each update removeddeployment.kubernetes.io/revisionand started a further generation. The master replacement waits for the Sentinel rollout per generation, so the wait started again in each reconcile. The soak showed the Info line in each reconcile (redis-chain-62-sent, 16 lines in 21 seconds), and the 10-minute bound of the wait was never reached. A desired annotation replaces a stored annotation with the same key.createOrUpdategets an optionalkeephook for this merge.The operator still sends a no-op update of the Sentinel Deployment in each reconcile, because
deploymentUpToDatedoes not normalize the API defaults. This is a separate change.Tests:
TestUpdateRedisesPodsBoundsTheSentinelRolloutWaitOverReconciles(the realEnsureof the Sentinel Deployment against a fake API server with the defaults and the generation rule, then the gate, over 5 reconciles: Info once, the start of the wait stays, Warning and master delete after 10 minutes) andTestDeploymentServiceUpdateKeepsTheStoredAnnotationsfail on main.TestDeploymentServiceUpdateWritesTheDesiredAnnotationsfails when the merge gives the stored annotations priority. Not tested on a real API server; the defaults and the generation rule come from the Kubernetes source.Generated by Claude Code