Add startup probe to CLI and VMDP download servers#2319
Add startup probe to CLI and VMDP download servers#2319openshift-merge-bot[bot] merged 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 36 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: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughCLI and VMDP server Deployment templates now include startup probes. Reconciliation independently backfills missing readiness, liveness, and startup probes on existing Deployments, updating only when changes are needed. New tests validate probe settings and preservation of existing configuration. ChangesServer Deployment Probe Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/controller/cli_download_controller_test.go`:
- Around line 17-31: Extend the startup-probe assertions in the tests at
internal/controller/cli_download_controller_test.go:17-31 and
internal/controller/vmdp_download_controller_test.go:17-31 to verify
InitialDelaySeconds and PeriodSeconds are both 5, alongside the existing
StartupProbe checks. Update the relevant test assertions in each file; no other
probe behavior needs to change.
- Around line 9-39: Add reconciliation tests at
internal/controller/cli_download_controller_test.go:9-39 covering
reconcileCLIResources with existing Deployments that have missing probes and
probes already set, asserting probes are backfilled without overwriting
pre-existing values. Add equivalent tests at
internal/controller/vmdp_download_controller_test.go:9-39 for
reconcileVMDPResources, covering both missing and pre-existing probes; retain
the existing deployment-construction assertions.
In `@internal/controller/cli_download_controller.go`:
- Around line 98-103: The probe backfill logic must target the named server
container instead of assuming index 0. In
internal/controller/cli_download_controller.go lines 98-103, update the
deployment reconciliation to locate oadp-cli-server by container name before
comparing or applying probes; likewise, in
internal/controller/vmdp_download_controller.go lines 93-98, locate
oadp-vmdp-server by name and use that container for the existing probe updates.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9800cd1-9c08-435c-bc34-086a5c2d1e02
📒 Files selected for processing (4)
internal/controller/cli_download_controller.gointernal/controller/cli_download_controller_test.gointernal/controller/vmdp_download_controller.gointernal/controller/vmdp_download_controller_test.go
|
/retest |
…-dev # Conflicts: # internal/controller/cli_download_controller.go # internal/controller/cli_download_controller_test.go # internal/controller/vmdp_download_controller.go # internal/controller/vmdp_download_controller_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/cli_download_controller_test.go (2)
282-284: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the expected reconciliation error at all four sites.
Each test currently treats any returned error as expected, which can hide failures in earlier reconciliation steps.
internal/controller/cli_download_controller_test.go#L282-L284: assert the intentional unregistered-route/console error.internal/controller/cli_download_controller_test.go#L323-L325: assert the intentional unregistered-route/console error.internal/controller/cli_download_controller_test.go#L379-L381: assert the intentional unregistered-route/console error.internal/controller/cli_download_controller_test.go#L421-L423: assert the intentional unregistered-route/console error.🤖 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 `@internal/controller/cli_download_controller_test.go` around lines 282 - 284, Update all four reconcileCLIResources test sites in internal/controller/cli_download_controller_test.go (lines 282-284, 323-325, 379-381, and 421-423) to assert that the returned error specifically identifies the intentional unregistered Route/ConsoleCLIDownload step. Continue allowing that expected error, but fail the tests for any earlier or unrelated reconciliation error.Source: Path instructions
430-431: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack update calls instead of
ResourceVersionhere.The fake client’s
ResourceVersionbehavior is an approximation and is unreliable for proving reconciliation skippedUpdate. Wrap/intercept the client and assert zero update calls, or use envtest instead of relying onbefore.ResourceVersion != after.ResourceVersion.🤖 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 `@internal/controller/cli_download_controller_test.go` around lines 430 - 431, Replace the ResourceVersion comparison in the reconciliation test with an assertion that the fake client received zero Update calls. Wrap or intercept the client used by the test to track updates, then verify no update occurred while preserving the existing reconciliation scenario.
🤖 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.
Outside diff comments:
In `@internal/controller/cli_download_controller_test.go`:
- Around line 282-284: Update all four reconcileCLIResources test sites in
internal/controller/cli_download_controller_test.go (lines 282-284, 323-325,
379-381, and 421-423) to assert that the returned error specifically identifies
the intentional unregistered Route/ConsoleCLIDownload step. Continue allowing
that expected error, but fail the tests for any earlier or unrelated
reconciliation error.
- Around line 430-431: Replace the ResourceVersion comparison in the
reconciliation test with an assertion that the fake client received zero Update
calls. Wrap or intercept the client used by the test to track updates, then
verify no update occurred while preserving the existing reconciliation scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2629fb3c-3f00-43dd-b9c3-0c76dd4d2ce6
📒 Files selected for processing (4)
internal/controller/cli_download_controller.gointernal/controller/cli_download_controller_test.gointernal/controller/vmdp_download_controller.gointernal/controller/vmdp_download_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/vmdp_download_controller_test.go
|
/retest |
|
@NicholasYancey: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
approved
|
/cherry-pick oadp-1.6 |
|
@NicholasYancey: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, NicholasYancey, sseago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@NicholasYancey: #2319 failed to apply on top of branch "oadp-1.6": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Why the changes were made
Adds a StartupProbe to the
oadp-cli-serverandoadp-vmdp-serverdeployments. This gives each container up to 60 seconds (periodSeconds: 5,failureThreshold: 12) to start up before the LivenessProbe begins evaluating it, preventing premature restarts on slow starts. Existing deployments without a StartupProbe are backfilled on reconcile so upgraded clusters get the fix automatically.How to test the changes made
Run the controller unit tests to verify the StartupProbe is set correctly on both deployments:
Then confirm the probe is present on a running cluster:
Summary by CodeRabbit