feat(daemonset): add restart and rollout-aware wait methods#2753
feat(daemonset): add restart and rollout-aware wait methods#2753rnetser wants to merge 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a DaemonSet compatibility shim, a new ChangesDaemonSet compatibility and rollout helpers
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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}
|
@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 |
This comment was marked as spam.
This comment was marked as spam.
…tures Preserve backward compatibility — keep original method signatures and ordering (wait_until_deployed, delete first after generated code). New methods (restart, wait_for_rollout) follow after. Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Signed-off-by: rnetser <rnetser@redhat.com>
Tests restart() and wait_for_rollout() via ocp_resources.daemon_set and ocp_resources.daemonset (deprecated shim) on a live cluster. Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev> Signed-off-by: rnetser <rnetser@redhat.com>
Signed-off-by: rnetser <rnetser@redhat.com>
Allow restart(wait_for_rollout=True) to automatically wait for the rollout to complete after triggering the restart. Addresses maintainer review feedback. Signed-off-by: rnetser <rnetser@redhat.com>
Add `or 0` null-safety to wait_until_deployed for consistency with wait_for_rollout. Log warning when resource not found in both wait methods instead of looping silently. Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev> Signed-off-by: rnetser <rnetser@redhat.com>
ResourceField wrapping an empty list is truthy — use len() == 0 to reliably detect non-existent resources. Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev> Signed-off-by: rnetser <rnetser@redhat.com>
Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev> Signed-off-by: rnetser <rnetser@redhat.com>
Remove len(sample.items) guard — the wrapper pattern relies on TimeoutSampler to handle non-existent resources via timeout. 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> Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev> Signed-off-by: rnetser <rnetser@redhat.com>
Replace self.api.get with field_selector by lambda: self.instance to match the standard wrapper pattern (if sample: instead of if sample.items:). Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Signed-off-by: rnetser <rnetser@redhat.com>
rnetser
force-pushed
the
feature/issue-2752-daemonset-restart-rollout
branch
from
cf7abf6 to
935161b
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
|
/verified |
Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Signed-off-by: rnetser <rnetser@redhat.com>
Signed-off-by: rnetser <rnetser@redhat.com>
|
/verified |
Co-authored-by: pi-coding-agent <noreply@pi-coding-agent.com> Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev> Signed-off-by: rnetser <rnetser@redhat.com>
|
/verified |
7 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