Conversation
|
Warning Review limit reached
Next review available in: 33 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 YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe DaemonSet install container now sets Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @gcoon151. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![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)
controllers/daemonset_reconcile.go (1)
463-471: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the peer-pods environment contract.
Test that
EnablePeerPods=trueproduces exactlyPEER_PODS=true, while disabled mode omits it; also cover the shell guard with unset, false, and true values.🤖 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 `@controllers/daemonset_reconcile.go` around lines 463 - 471, Add regression tests around the DaemonSet reconciliation logic that builds kataInstallEnv, verifying EnablePeerPods=true adds exactly PEER_PODS=true and disabled mode omits the variable. Also test the kata-osbuilder.sh guard behavior for PEER_PODS unset, false, and true, preserving the expected skip behavior only for the true value.
🤖 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 `@scripts/kata-install/osc-kata-install.sh`:
- Around line 283-293: Guard the fresh-install postinstall scriptlet flow near
the existing postinstall/posttrans handling so peer-pods do not invoke
kata-osbuilder.sh before reaching the later guard. Preserve execution of
required SELinux and other non-image-building scriptlets, and gate only the
scriptlet or operation that builds the host-kernel-derived VM image; use the
existing PEER_PODS check consistently with the guarded block.
---
Nitpick comments:
In `@controllers/daemonset_reconcile.go`:
- Around line 463-471: Add regression tests around the DaemonSet reconciliation
logic that builds kataInstallEnv, verifying EnablePeerPods=true adds exactly
PEER_PODS=true and disabled mode omits the variable. Also test the
kata-osbuilder.sh guard behavior for PEER_PODS unset, false, and true,
preserving the expected skip behavior only for the true value.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: bf3a9ccf-78bd-4cad-8d89-6fa1d1b29a4b
📒 Files selected for processing (2)
controllers/daemonset_reconcile.goscripts/kata-install/osc-kata-install.sh
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
| // Inform the install script whether this is a peer-pods cluster. | ||
| // kata-osbuilder.sh (local kata initrd build) must be skipped on peer-pods | ||
| // nodes — the initrd is not used there and the build fails on RHCOS workers. | ||
| if r.kataConfig.Spec.EnablePeerPods { |
There was a problem hiding this comment.
@gcoon151 Discussing this with @snir911 he points out that in public cloud installations we want both kata and kata-remote to work together. So you should not key off the kata config enabling peer pods, but on the fact that the current runtime class is remote.
Rescinding my earlier approval based on this.
c3d
added
needs-ok-to-test
|
Question : I am not 100% familiar with how this is working. I.e: if we merge this, do we need to release the whole operator again, or can we "just" rebuild and push the daemonset image? |
187d1b8 to
bc31eb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/kata-install/osc-kata-install.sh`:
- Around line 312-317: Update the outer condition around the kata-osbuilder
generation block to guard on the PEER_PODS setting rather than CLOUD_PROVIDER.
Ensure non-peer-pods deployments, including IBM Cloud, run the existing
kernel/initrd existence check and kata-osbuilder.sh flow, while peer-pods
deployments skip it.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: def837fc-7da2-4b44-b8e7-a98d71800b21
📒 Files selected for processing (2)
controllers/daemonset_reconcile.goscripts/kata-install/osc-kata-install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/daemonset_reconcile.go
| if [[ "${CLOUD_PROVIDER:-}" != "ibmcloud" ]]; then | ||
| if ! chroot /host test -e /var/cache/kata-containers/osbuilder-images/kata.kernel; then | ||
| echo "kata VM kernel/initrd missing, running kata-osbuilder.sh" | ||
| chroot /host mkdir -p /var/cache/kata-containers/osbuilder-images | ||
| chroot /host /usr/libexec/kata-containers/osbuilder/kata-osbuilder.sh | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard on PEER_PODS, not CLOUD_PROVIDER.
controllers/daemonset_reconcile.go sets PEER_PODS=true when peer pods are enabled, but this block ignores it. As written, IBM Cloud non-peer-pods skip required local initrd generation, while peer-pods deployments on other providers still run kata-osbuilder.sh.
Proposed fix
-if [[ "${CLOUD_PROVIDER:-}" != "ibmcloud" ]]; then
+if [[ "${PEER_PODS:-}" != "true" ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ "${CLOUD_PROVIDER:-}" != "ibmcloud" ]]; then | |
| if ! chroot /host test -e /var/cache/kata-containers/osbuilder-images/kata.kernel; then | |
| echo "kata VM kernel/initrd missing, running kata-osbuilder.sh" | |
| chroot /host mkdir -p /var/cache/kata-containers/osbuilder-images | |
| chroot /host /usr/libexec/kata-containers/osbuilder/kata-osbuilder.sh | |
| fi | |
| if [[ "${PEER_PODS:-}" != "true" ]]; then | |
| if ! chroot /host test -e /var/cache/kata-containers/osbuilder-images/kata.kernel; then | |
| echo "kata VM kernel/initrd missing, running kata-osbuilder.sh" | |
| chroot /host mkdir -p /var/cache/kata-containers/osbuilder-images | |
| chroot /host /usr/libexec/kata-containers/osbuilder/kata-osbuilder.sh | |
| 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 `@scripts/kata-install/osc-kata-install.sh` around lines 312 - 317, Update the
outer condition around the kata-osbuilder generation block to guard on the
PEER_PODS setting rather than CLOUD_PROVIDER. Ensure non-peer-pods deployments,
including IBM Cloud, run the existing kernel/initrd existence check and
kata-osbuilder.sh flow, while peer-pods deployments skip it.
|
Since only the osc-rpm-install pod was crashing I think you could update just that code inside there and fix this and release that container. I updated the PR to only change behavior on ibmcloud. It's a short term fix. I'll try to PR up the additional longer term ideal fixes (like addressing the strip issue). |
On IBM Cloud, kata-remote (peer-pods) is the only runtime in use. The local kata VM initrd built by kata-osbuilder.sh is not needed — kata-agent runs inside a remote peer pod VSI managed by cloud-api-adaptor, not on the worker node. The repair block in install_kata() calls kata-osbuilder.sh when /var/cache/kata-containers/osbuilder-images/kata.kernel is absent. On IBM Cloud peer-pods workers this path is never populated, so the check fires on every pod restart, calls kata-osbuilder.sh, and crashes because strip(1) corrupts the dynamically linked kata-agent binary shipped in kata-containers-3.25.0-7.rhaos4.20/21.el9. The result is permanent CrashLoopBackOff and KataConfig stuck InProgress with readyNodeCount 0. Guard the repair block with CLOUD_PROVIDER != ibmcloud. This variable is already injected into the DaemonSet env by the controller; no controller change is needed. Mixed kata+kata-remote clusters on other providers are unaffected. Confirmed broken: OCP 4.20 and 4.21, IBM Cloud VPC HyperShift, OSC 1.13.1, enablePeerPods: true. Workaround (sentinel kata.kernel file on each node) unblocked all nodes; readyNodeCount reached 3 within 90 seconds. This fix makes the workaround unnecessary. Introduced in: fa5d05e daemonset: use rpm-ostree --apply-live to avoid node reboots Known limitations and follow-on work: - This guard is IBM Cloud-specific. AWS and Azure peer-pods deployments will hit the same crash if they adopt the DaemonSet install path. A provider-agnostic fix requires resolving the two issues below. - kata-osbuilder-generate.service at boot also calls kata-osbuilder.sh and hits the same strip crash on IBM Cloud nodes. The kata RuntimeClass exists but its initrd is never built. Fix: ship kata-agent pre-stripped in the RPM, or make strip failure non-fatal in rootfs.sh (kata-containers/kata-containers). - The kata and kata-nvidia-gpu RuntimeClasses are created unconditionally even on pure peer-pods clusters where they are never used. Long term, the controller should not create local kata infrastructure when enablePeerPods is true and no local kata runtime is needed. Signed-off-by: Gerald Coon <grcoon@us.ibm.com>
bc31eb9 to
b9f37f2
Compare
snir911
left a comment
snir911
left a comment
There was a problem hiding this comment.
LGTM, thanks, I'll create the DS image and test
|
Looks good to me. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
LGTM but I think at this point long term we should then support two flags in kataconfig, one for kata and one for peer-pods. So far we always supported both in all environments except IBM cloud. We cannot have one disabling the other, they should be two separate switches |
|
@gcoon151: 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. |
5 participants
Problem
When
KataConfig.spec.enablePeerPods: true, theinstall_kata()repair block inosc-kata-install.shunconditionally callskata-osbuilder.shon every pod restart. On peer-pods clusters/var/cache/kata-containers/osbuilder-images/kata.kernelis intentionally never populated —kata-remoteuses a remote VM image managed bycloud-api-adaptor, not a local kata initrd. So the missing-file check always fires,kata-osbuilder.shis called on every restart of every node, and the pods crash-loop permanently.Downstream, the crash is triggered by
strip(1)inrootfs.shcorrupting thekata-agentbinary (dynamically linked PIE, not pre-stripped inkata-containers-3.25.0-7.rhaos4.21.el9). But the root cause is that osbuilder should never run on peer-pods workers at all.Introduced in: fa5d05e (daemonset: use rpm-ostree --apply-live to avoid node reboots)
Confirmed broken: OCP 4.21.18, IBM Cloud VPC HyperShift, OSC 1.13.1,
enablePeerPods: trueImpact: Every peer-pods OSC deployment on 1.13.x — IBM Cloud, AWS, Azure —
KataConfignever reaches installed state.Fix
Two minimal changes:
controllers/daemonset_reconcile.go— injectPEER_PODS=trueinto the kata-install DaemonSet env whenEnablePeerPodsis set (the controller already branches on this field)scripts/kata-install/osc-kata-install.sh— guard the osbuilder repair block with${PEER_PODS:-}so the local initrd build is skipped entirely on peer-pods nodesTesting
Workaround equivalent to this fix (sentinel
kata.kernelfile on each node) was validated on the live broken cluster:osc-rpm-installpods reachedRunning, 0 restartsKataConfig.status.readyNodeCount: 3,InProgress: Falsewithin 90 secondsopenshift-sandboxed-containers-monitorpods (previouslyCreateContainerError) reachedRunningTest matrix:
enablePeerPods: false, local kataenablePeerPods: true, peer-podsenablePeerPods: true, node rebootSigned-off-by: Gerald Coon grcoon@us.ibm.com