Skip to content

daemon: skip nice/ionice for extensions image pull during firstboot#6139

Draft
sdodson wants to merge 1 commit into
openshift:mainfrom
sdodson:worktree-firstboot-foreground-podman
Draft

daemon: skip nice/ionice for extensions image pull during firstboot#6139
sdodson wants to merge 1 commit into
openshift:mainfrom
sdodson:worktree-firstboot-foreground-podman

Conversation

@sdodson
Copy link
Copy Markdown
Member

@sdodson sdodson commented Jun 3, 2026

During firstboot no workloads are running on the node yet, so throttling podman to idle I/O and low CPU priority only slows down provisioning. Thread a firstBoot flag from update() down through applyOSChanges/applyLayeredOSChanges/ExtractExtensionsImage to pullExtensionsImage and podmanCopy, switching from RunExtBackground to RunExt when firstBoot is true.

Summary by CodeRabbit

  • Bug Fixes

    • Improved extension image extraction handling during OS updates.
    • Extension image extraction on layered-core systems is now properly controlled during initial boot.
  • Tests

    • Updated tests to reflect changes to extension image extraction logic.

During firstboot no workloads are running on the node yet, so
throttling podman to idle I/O and low CPU priority only slows down
provisioning. Thread a firstBoot flag from update() down through
applyOSChanges/applyLayeredOSChanges/ExtractExtensionsImage to
pullExtensionsImage and podmanCopy, switching from RunExtBackground
to RunExt when firstBoot is true.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@openshift-merge-bot
Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5b1eb03a-8dbe-4c49-9d68-28d003c44866

📥 Commits

Reviewing files that changed from the base of the PR and between 1d96927 and 67fe742.

📒 Files selected for processing (2)
  • pkg/daemon/update.go
  • pkg/daemon/update_test.go

Walkthrough

The PR adds background execution control for extensions image operations on CoreOS systems. Image extraction functions now accept a background flag to run podman pull/copy operations in the background or foreground. The OS update flow threads a firstBoot boolean through orchestration functions to compute the background mode as !firstBoot, preventing background extraction during initial system boot while allowing it on subsequent updates.

Changes

Extensions Image Extraction Background Control

Layer / File(s) Summary
Image extraction functions with background flag
pkg/daemon/update.go
pullExtensionsImage and podmanCopy now accept a background boolean and conditionally execute via RunExtBackground or RunExt. The exported ExtractExtensionsImage method signature updated to accept and forward background to both functions.
OS update orchestration with first-boot threading
pkg/daemon/update.go
applyOSChanges and applyLayeredOSChanges now accept and thread a firstBoot parameter. Extensions extraction on layered systems uses background = !firstBoot, preventing background mode during first boot.
Call sites propagating first-boot parameter
pkg/daemon/update.go
Daemon.update and HyperShift-specific update path now pass firstBoot (or false for HyperShift) into applyOSChanges on both normal and rollback invocations.
Test update for podmanCopy signature
pkg/daemon/update_test.go
TestPodmanCopy_CreateContainerCall updated to pass the new background boolean argument when invoking podmanCopy.

🎯 3 (Moderate) | ⏱️ ~18 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Container-Privileges ❌ Error PR introduces privileged container execution (--privileged, --pid=host, --net=host) in InplaceUpdateViaNewContainer function without explicit security justification in the custom check scope. Add security documentation or risk assessment for privileged container usage in OS update flows, or use alternative non-privileged update mechanisms where possible.
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ❓ Inconclusive The custom check specifies review of "Ginkgo test code," but pkg/daemon/update_test.go uses standard Go testing (testing.T), not Ginkgo (no Describe/It/BeforeEach blocks). The check is not applicable. Clarify if the check applies only to test/extended-priv Ginkgo tests or if standard Go unit tests should also be reviewed using adapted criteria (e.g., TestName instead of It blocks, t.Run instead of BeforeEach).
✅ Passed checks (12 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: skipping nice/ionice (I/O and CPU throttling) for extensions image pull during firstboot, which is the primary focus of threading the firstBoot flag through the call chain.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed This codebase uses standard Go testing, not Ginkgo. All test names are static, descriptive, and follow Go conventions; no dynamic content found.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests were added. The PR only modifies standard Go unit tests in update_test.go, so MicroShift compatibility check is not applicable.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests are added in this PR. Changes are limited to production code (pkg/daemon/update.go) and unit tests (pkg/daemon/update_test.go). The custom check is not applicable.
Topology-Aware Scheduling Compatibility ✅ Passed PR modifies only daemon Go code (pkg/daemon/update.go, update_test.go) for image extraction logic; no Kubernetes manifests, deployment specs, or scheduling constraints are introduced.
Ote Binary Stdout Contract ✅ Passed PR introduces no stdout writes in process-level code. Changes are isolated to internal daemon functions that are not called by OTE binary, so no OTE Binary Stdout Contract violation occurs.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No Ginkgo e2e tests are added in this PR. Changes are limited to pkg/daemon/update.go (main code) and pkg/daemon/update_test.go (standard Go unit tests), neither containing Ginkgo e2e test patterns.
No-Weak-Crypto ✅ Passed No weak cryptography (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto, or secret comparisons found. Changes only add execution control flags.
No-Sensitive-Data-In-Logs ✅ Passed No sensitive data exposed in logs. New background and firstBoot parameters are never logged; image URL logging follows existing codebase patterns.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 3, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 3, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sdodson
Once this PR has been reviewed and has the lgtm label, please assign pablintino for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant