Invoke crio dedup if enabled at startup#6266
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ngopalak-redhat The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThis PR adds CRI-O dedup boot-time wiring: a default-disabled env file, a guarded shell script, a systemd oneshot unit, and an extended-privilege test for the disabled path. ChangesCRI-O Dedup Boot Flow
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
templates/common/_base/files/crio-dedup.yaml (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
systemctl is-activefor the running-service check.
systemctl status ... >/dev/null 2>&1conflates several distinct non-zero exit states (dead, failed, not-found);systemctl is-active --quietis the purpose-built, scripting-friendly check.♻️ Proposed refactor
- if systemctl status crio.service >/dev/null 2>&1; then + if systemctl is-active --quiet crio.service; then echo "CRI-O service is already running, skipping dedup" exit 0 fi🤖 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 `@templates/common/_base/files/crio-dedup.yaml` around lines 18 - 22, The CRI-O running-service check in the dedup script should use the purpose-built active-state probe instead of the broader status command. Update the service check in the script that handles the crio.service guard to use systemctl is-active --quiet so it only treats an actually running service as active, while preserving the existing skip-and-exit behavior in the same control flow.templates/common/_base/units/crio-dedup.service.yaml (1)
6-7: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUnnecessary network dependency for a local dedup operation.
crio dedupdeduplicates local image storage and doesn't need network; requiringnetwork-online.targetbefore this unit (which itself gatescrio.serviceviaBefore=) can add avoidable boot latency if network-online is slow to reach.🤖 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 `@templates/common/_base/units/crio-dedup.service.yaml` around lines 6 - 7, Remove the unnecessary network gating from the crio dedup unit so it no longer waits on network-online.target before starting. Update the crio-dedup.service.yaml unit stanza around the dedup service definition to drop the Wants=network-online.target dependency and adjust the After= ordering so crio dedup only keeps the firstboot-osupdate.target relationship and other required local startup ordering.
🤖 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 `@templates/common/_base/files/crio-dedup.yaml`:
- Line 6: The dedup script is too strict because `set -e` in `crio-dedup.yaml`
lets `/usr/bin/crio dedup` failures fail the unit and block `crio.service`
startup. Update the `crio-dedup` script to make the dedup step best-effort,
either by removing the fail-fast behavior or explicitly swallowing non-zero exit
codes around the dedup command so the unit always succeeds. Keep the change
localized to the dedup logic and ensure the service can continue even when dedup
fails.
---
Nitpick comments:
In `@templates/common/_base/files/crio-dedup.yaml`:
- Around line 18-22: The CRI-O running-service check in the dedup script should
use the purpose-built active-state probe instead of the broader status command.
Update the service check in the script that handles the crio.service guard to
use systemctl is-active --quiet so it only treats an actually running service as
active, while preserving the existing skip-and-exit behavior in the same control
flow.
In `@templates/common/_base/units/crio-dedup.service.yaml`:
- Around line 6-7: Remove the unnecessary network gating from the crio dedup
unit so it no longer waits on network-online.target before starting. Update the
crio-dedup.service.yaml unit stanza around the dedup service definition to drop
the Wants=network-online.target dependency and adjust the After= ordering so
crio dedup only keeps the firstboot-osupdate.target relationship and other
required local startup ordering.
🪄 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: bfc6f723-b29c-48e0-b034-69dfa76fb7f5
📒 Files selected for processing (3)
templates/common/_base/files/crio-dedup-enabled.yamltemplates/common/_base/files/crio-dedup.yamltemplates/common/_base/units/crio-dedup.service.yaml
|
/cherry-pick release-4.20 |
|
@ngopalak-redhat: 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. |
83bf244 to
1cf21dc
Compare
- What I did
Run cri-o dedup at startup if enabled
cc: @asahay19 @haircommander
- How to verify it
- Description for the changelog
Summary by CodeRabbit