feat(daemonset): add restart and rollout-aware wait methods#2753
feat(daemonset): add restart and rollout-aware wait methods#2753rnetser wants to merge 4 commits into
Conversation
Add restart() method that patches the pod template with a kubectl.kubernetes.io/restartedAt annotation, triggering a rolling restart (equivalent to `oc rollout restart`). Add wait_for_rollout() method that checks observedGeneration, updatedNumberScheduled, and numberAvailable to reliably wait for a DaemonSet rollout to complete (unlike wait_until_deployed which only checks numberReady and is racy after restarts). Closes #2752 Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds ChangesDaemonSet restart and rollout wait
Estimated code review effort: 2 (Simple) | ~12 minutes Related issues: Suggested labels: enhancement, tests Suggested reviewers: RedHatQE/openshift-python-wrapper maintainers 🐰 A daemon set stirs beneath the hood, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
PR Summary by QodoAdd DaemonSet restart() and rollout-aware wait_for_rollout()
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ocp_resources/daemonset.py (1)
28-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate TimeoutSampler construction between
wait_until_deployedandwait_for_rollout.Both methods build an identical
TimeoutSampler(samewait_timeout,sleep,exceptions_dict,func,field_selector,namespace) and only differ in the loop condition. Consider extracting a small private helper (e.g._daemonset_samples(timeout)) to avoid duplication and keep the two wait strategies in sync going forward.Also applies to: 63-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ocp_resources/daemonset.py` around lines 28 - 42, Both wait methods are duplicating the same TimeoutSampler setup, so extract the shared construction into a small private helper on DaemonSet (for example, a helper used by wait_until_deployed and wait_for_rollout) and have both methods reuse it. Keep the existing loop-specific readiness checks in each method, but move the common sampler arguments and api.get call into the helper so the two wait strategies stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ocp_resources/daemonset.py`:
- Around line 63-97: Update the docstring for DaemonSet.wait_for_rollout to
match the actual readiness check used by the implementation. The current wording
says the updated pods are “ready,” but the method returns based on
status.numberAvailable in the rollout condition, so revise the text to describe
availability rather than readiness and keep the explanation aligned with the
observedGeneration, updatedNumberScheduled, and numberAvailable checks.
---
Nitpick comments:
In `@ocp_resources/daemonset.py`:
- Around line 28-42: Both wait methods are duplicating the same TimeoutSampler
setup, so extract the shared construction into a small private helper on
DaemonSet (for example, a helper used by wait_until_deployed and
wait_for_rollout) and have both methods reuse it. Keep the existing
loop-specific readiness checks in each method, but move the common sampler
arguments and api.get call into the helper so the two wait strategies stay
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 524c442e-078d-4619-9c84-af4d42c7516a
📒 Files selected for processing (2)
ocp_resources/daemonset.pytests/test_daemonset.py
Code Review by Qodo
Context used 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
- Guard against missing/None status during early reconciliation - Handle zero-desired DaemonSets (no matching nodes) without timeout - Use `or 0` fallbacks for optional status fields - Fix docstring: "ready" → "available" - Add tests for status-missing and zero-desired edge cases Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
Yes — the rollout logic now looks consistent with the intended behavior.
So from my side, this looks resolved as a design choice rather than a bug. |
|
Tip For best results, initiate chat on the files or code changes.
|
|
@qodo-code-review Please re-evaluate all remaining sticky findings against the current code. |
I re-checked the current code, and both remaining sticky findings are still present.
No remaining finding is fully addressed yet, so I didn’t remove any. |
|
@qodo-code-review Please re-evaluate all remaining sticky findings against the current code. |
I re-checked the remaining sticky findings against the current code. Keep
Remove
If you want, I can also help turn either of these into a concrete fix suggestion. |
|
@coderabbitai review |
|
@qodo-code-review Findings 3 and 4 about numberAvailable vs numberReady were confirmed as intentional design decisions in your previous response (you said 'the rollout logic now looks consistent with the intended behavior' and 'numberAvailable is the right signal'). Please remove these findings as they are resolved by design - the issue spec was updated to document this choice. |
✅ Action performedReview finished.
|
Done — I dismissed finding 3 and finding 4 as |
|
Maintenance triage note: most code checks are green, but this PR is still blocked by the external Please check the |
The kubernetes DynamicClient.patch() requires `name` in the body or as a parameter. Include `metadata.name` in the resource_dict to fix ValueError on restart(). Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Signed-off-by: rnetser <rnetser@redhat.com>
Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
/verified |
5 participants
What
Add two new methods to the
DaemonSetclass:restart()— Patches the pod template with akubectl.kubernetes.io/restartedAtannotation, triggering a rolling restart (equivalent tooc rollout restart).wait_for_rollout()— Waits until the DaemonSet rollout is complete by checking:observedGeneration == metadata.generation(controller has seen latest spec)updatedNumberScheduled == desiredNumberScheduled(all pods updated)numberAvailable == desiredNumberScheduled(all pods available, respectingminReadySeconds)Why
wait_until_deployedonly checksdesiredNumberScheduled == numberReady, which is racy after a rollout restart — old pods are still ready so it returns immediately before any pod is replaced. This forced consumers to shell out tooc rollout restart+oc rollout status.Tests
5 unit tests covering:
updatedNumberScheduled < desired)observedGeneration != generation)numberAvailable < desiredwhilenumberReady == desired)Closes #2752
Summary by CodeRabbit
New Features
Tests