Skip to content

NO-ISSUE: Merge upstream 2026 04 13#831

Open
iurygregory wants to merge 31 commits into
openshift:mainfrom
iurygregory:merge-upstream-2026-04-13
Open

NO-ISSUE: Merge upstream 2026 04 13#831
iurygregory wants to merge 31 commits into
openshift:mainfrom
iurygregory:merge-upstream-2026-04-13

Conversation

@iurygregory
Copy link
Copy Markdown

@iurygregory iurygregory commented Apr 13, 2026

Summary by CodeRabbit

  • New Features

    • Added a standalone networking service entrypoint with JSON‑RPC auth, optional TLS, configurable switch drivers and driver config dir, plus environment-variable overrides.
    • Added startup/config helpers and defaults (including NUMWORKERS and DHCP control).
  • Documentation

    • Expanded README with networking env vars and detailed build instructions for custom sources and applying upstream patches.
  • Bug Fixes

    • Improved iPXE IPA asset URL handling and tightened Apache access control for IPA image files.

renovate Bot and others added 30 commits March 16, 2026 00:36
…ck-ironic-digest

🌱 Update openstack-ironic digest to 4e0c244 (main)
…ols-82.x

🌱 Update dependency setuptools to v82.0.1 (main)
Combine two COPY instructions targeting /bin/ into one, and merge
the database setup and non-root user configuration RUN blocks into
a single heredoc. This reduces the final stage from 14 to 12 layers.

Signed-off-by: Riccardo Pittau <elfosardo@gmail.com>
Signed-off-by: PDCuong <pdcuong11@gmail.com>
EOL releases cannot be compared anymore, so they'll produce 404s.
Release notes should not be touched afterwards, so there is no fixing
the links either.

Signed-off-by: Tuomo Tanskanen <tuomo.tanskanen@est.tech>
Signed-off-by: Tuomo Tanskanen <tuomo.tanskanen@est.tech>
✨ Make ipa-collect-lldp and parse-lldp hook optional in ironic.conf.j2
…gnores

🌱 ignore github release comparison links in Lychee
🌱 Reduce Dockerfile layers by merging redundant steps
Replace 6 separate COPY instructions for ironic-config/ files with a
single COPY of the entire directory to a staging path, distributing
files to their destinations via cp in the RUN block. This reduces the
final image by 5 layers.

Signed-off-by: Riccardo Pittau <elfosardo@gmail.com>
The regex-based <Directory> directives for redfish, ilo, and images
paths require a trailing slash, failing to match requests without one.
This allows access control bypass when the parent directory has a
permissive Require directive.

Make the trailing slash optional with /? in all three affected
templates: apache2-ipxe.conf.j2, apache2-vmedia.conf.j2, and
httpd.conf.j2.

Signed-off-by: Riccardo Pittau <elfosardo@gmail.com>
The ! exclusion syntax is only valid for ProxyPass, not
ProxyPassReverse. The invalid directive causes Apache to reject the
vhost configuration with an "invalid URL" error. The preceding
ProxyPass "/images" ! already prevents /images from being proxied.

Signed-off-by: Riccardo Pittau <elfosardo@gmail.com>
…ck-ironic-digest

🌱 Update openstack-ironic digest to f700d3d (main)
When IPXE_TLS_SETUP=true the cleartext httpd vhost blocks
ironic-python-agent files via a FilesMatch deny rule, and the
iPXE TLS vhost denies the entire /shared/html/images/ directory.

inspector.ipxe.j2 renders all IPA kernel/ramdisk URLs against
IRONIC_HTTP_URL (http://host:80), so they become unreachable
during TLS-enabled inspection boot.

Introduce IRONIC_IPA_BASE_URL in ironic-common.sh that resolves
to the iPXE TLS endpoint when TLS is active, and falls back to
IRONIC_HTTP_URL otherwise. Use it in inspector.ipxe.j2 for all
IPA artifact URLs. Add a targeted FilesMatch grant on the iPXE
TLS vhost so IPA files are served while the broader images/
deny stays in effect.

Signed-off-by: Riccardo Pittau <elfosardo@gmail.com>
…ructions

🌱 Consolidate ironic-config COPY instructions into single layer
…railing-slash

🐛 Fix Directory regex to match paths without trailing slash
…reverse-exclusion

🐛 Drop invalid ProxyPassReverse exclusion for /images
…s-urls

🐛 Fix inspector.ipxe IPA URLs unreachable under iPXE TLS
Integrates Bare Metal Operator's e2e test suite to test ironic-image
with live-ISO provisioning scenarios. Uses Method 1 from issue metal3-io#758:
clones BMO at release v0.12.1, builds custom ironic image, and runs
tests via BMO's ci-e2e.sh script.

Fixes: metal3-io#758
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Abhishek Bongale <abhishekbongale@outlook.com>
…ck-ironic-digest

🌱 Update openstack-ironic digest to eda12b9 (main)
In support of: metal3-io/metal3-docs#586

Add the ironic-networking service container entry point and
configuration to enable standalone network management of baremetal
nodes via networking-generic-switch (NGS). This allows Ironic to
manage switch port configurations during node provisioning, cleaning,
inspection, servicing, and rescuing operations.

Changes:
- Add runironic-networking entry point and configure-ironic-networking
  script with associated environment variable defaults
- Add ironic-networking.conf.j2 Jinja2 configuration template
- Add NGS_SOURCE and INSTALL_NGS build arguments to Dockerfile and
  build-wheels.sh for optional networking-generic-switch installation
- Update ironic.conf.j2 to conditionally enable the ironic-networking
  network interface and configure JSON-RPC for the networking service
- Update auth-common.sh to accept a configurable config group name so
  it can be reused for ironic_networking_json_rpc authentication
- Add inspection hook local-link-connection and force_dhcp setting
  when networking is enabled
- Document new environment variables in README.md
- Update hack/prepare-irso-tests.sh for NGS source builds

Signed-off-by: Allain Legacy <alegacy@redhat.com>
…orking

✨ Add support for ironic-networking standalone service
Follow-up to PR metal3-io#931 to address review comments about unquoted
parameter expansions. Wraps all variable assignments using
${VAR:-default} in double quotes to prevent word splitting and
globbing on the expanded result.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Allain Legacy <alegacy@redhat.com>
…orking

🌱 Fix unquoted parameter expansions in shell scripts
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 13, 2026
@openshift-ci-robot
Copy link
Copy Markdown

@iurygregory: This pull request explicitly references no jira issue.

Details

In 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 openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 13, 2026

Walkthrough

Adds standalone ironic-networking support: docs, Jinja2 templates, and shell scripts to render networking configs, set environment variables, configure JSON‑RPC auth, and run the ironic-networking daemon; also minor Apache and iPXE template adjustments and small edits to existing helpers.

Changes

Cohort / File(s) Summary
Documentation
README.md
Documented new runironic-networking entrypoint, environment variables for overriding ironic-networking behavior, and build-time options for custom sources and patch application.
New Config Template
ironic-config/ironic-networking.conf.j2
Adds INI template for ironic-networking ([DEFAULT], [ironic_networking_json_rpc], optional [ssl], [ironic_networking]) with env-driven RPC, TLS, and switch-driver settings.
Modified Config Templates
ironic-config/ironic.conf.j2, ironic-config/inspector.ipxe.j2, ironic-config/apache2-ipxe.conf.j2
ironic.conf.j2: conditionally enable ironic-networking, add JSON-RPC/endpoint stanzas and conditional inspector hooks/params. inspector.ipxe.j2: switch URL base to IRONIC_IPA_BASE_URL and adjust boot args. apache2-ipxe.conf.j2: allow access to ironic-python-agent.* under images.
Shell Helpers — Common
scripts/ironic-common.sh
Quoted IRONIC_USE_MARIADB default, added IRONIC_FORCE_DHCP, and set IRONIC_IPA_BASE_URL (based on IPXE_TLS_SETUP) inside wait_for_interface_or_ip().
Shell Helpers — Networking Common
scripts/ironic-networking-common.sh
New file exporting networking-related env vars (enable flag, driver config dir, RPC host/port, enabled switch drivers, switch config path).
Shell Helpers — Auth
scripts/auth-common.sh
configure_json_rpc_auth() now always runs and accepts an optional GROUP parameter (defaults to json_rpc) to target different config sections.
Service Init & Configure
scripts/configure-ironic.sh, scripts/configure-ironic-networking.sh
configure-ironic.sh now sources networking common and conditionally configures JSON‑RPC auth per service. New configure-ironic-networking.sh renders ironic-networking.conf, configures auth for ironic_networking_json_rpc, updates NO_PROXY, and ensures driver config dir.
Service Runner
scripts/runironic-networking
New executable that sources configure script, wires certificate-restart handling, and execs /usr/bin/ironic-networking with --config-file and --config-dir.
Other
.lycheeignore
Added ignore pattern for upstream compare URLs.

Sequence Diagram(s)

sequenceDiagram
    participant Entrypoint as runironic-networking
    participant Configure as configure-ironic-networking.sh
    participant Render as Jinja2 Renderer
    participant Auth as configure_json_rpc_auth
    participant Service as /usr/bin/ironic-networking
    participant JSONRPC as ironic-networking JSON-RPC

    Entrypoint->>Configure: source configure script
    Configure->>Render: render ironic-networking.conf.j2 -> /etc/ironic/ironic.conf
    Configure->>Auth: configure_json_rpc_auth("ironic_networking_json_rpc")
    Configure->>Service: exec /usr/bin/ironic-networking --config-file --config-dir
    Service->>JSONRPC: bind to host/port from config
    JSONRPC-->>Service: accept connections from switch drivers / clients
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 8 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is vague and generic, using a pattern (NO-ISSUE: Merge upstream YYYY MM DD) that provides minimal meaningful information about the specific changes beyond indicating an upstream merge. Consider a more descriptive title that highlights the primary change (e.g., 'Add ironic-networking service support and update upstream dependencies') to help reviewers understand the key modifications at a glance.
✅ Passed checks (8 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Stable And Deterministic Test Names ✅ Passed The custom check for stable Ginkgo test names is not applicable; this repository uses Python unittest, not Ginkgo, and the PR modifies only shell scripts, Jinja2 templates, and documentation.
Test Structure And Quality ✅ Passed No Ginkgo test files exist in this repository or PR; check is not applicable.
Microshift Test Compatibility ✅ Passed PR adds no Ginkgo e2e tests; contains only documentation, configuration templates, and shell scripts. Check not applicable; passes by default.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests added; repository contains image build configs, templates, and scripts only.
Topology-Aware Scheduling Compatibility ✅ Passed The custom check for topology-aware scheduling compatibility is not applicable to this pull request as it modifies only container image configurations, not Kubernetes resources.
Ote Binary Stdout Contract ✅ Passed OTE Binary Stdout Contract applies to Go/Ginkgo test binaries communicating via JSON on stdout. This PR contains only shell scripts, Jinja2 templates, and documentation with no Go code or test runner code.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The custom check for IPv6 and disconnected network test compatibility is not applicable to this PR. The openshift/ironic-image repository is a Metal3 Ironic container image repository containing shell scripts, configuration templates, and documentation—not a Kubernetes e2e test suite. No Ginkgo test files exist in this repository or were added in this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@openshift-ci openshift-ci Bot requested review from bfournie and derekhiggins April 13, 2026 20:01
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 13, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iurygregory

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

The pull request process is described 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
README.md (1)

175-175: Keep the heading style consistent with the rest of the README.

These new ATX headings introduce the markdownlint MD003 warnings already reported for this file.

Also applies to: 220-220, 251-251

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 175, The new ATX heading "## Ironic Networking" uses a
different heading style than the rest of the README; update this heading (and
the other occurrences with the same text) to match the repository's existing
heading style (e.g., convert to the setext underline form or the same number of
leading # characters used elsewhere), ensuring MD003 consistency across the file
by using the same heading syntax for "Ironic Networking" and the similar
headings at the other locations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ironic-config/inspector.ipxe.j2`:
- Around line 46-59: Remove the merge conflict markers and unify the conditional
so that when env.DEPLOY_KERNEL_URL / env.DEPLOY_RAMDISK_URL are defined you use
those (with the replace(file_url_prefix, env.IRONIC_HTTP_URL + '/') logic and
derive ipa_ramdisk_name from env.DEPLOY_RAMDISK_URL.split('/')[-1]), otherwise
fall back to env.IRONIC_IPA_BASE_URL for ipa_kernel and ipa_ramdisk; ensure
ipa_ramdisk_name is set to "ironic-python-agent.initramfs" in the fallback and
that no "<<<<<<<", "=======", or ">>>>>>>" lines remain in the template.

In `@scripts/configure-ironic-networking.sh`:
- Line 3: Remove unconditional xtrace: replace "set -euxo pipefail" with "set
-euo pipefail" and make -x opt-in; either enable tracing only when a DEBUG/TRACE
env var is set (e.g., if [ -n "$DEBUG" ]; then set -x; fi) or explicitly disable
tracing around sensitive operations by adding "set +x" before sourcing the auth
helpers and before configuring JSON-RPC auth and restore it afterward if needed.
Target the top-level "set -euxo pipefail" and the sections that source auth
helpers and configure JSON-RPC auth to avoid leaking secrets.

---

Nitpick comments:
In `@README.md`:
- Line 175: The new ATX heading "## Ironic Networking" uses a different heading
style than the rest of the README; update this heading (and the other
occurrences with the same text) to match the repository's existing heading style
(e.g., convert to the setext underline form or the same number of leading #
characters used elsewhere), ensuring MD003 consistency across the file by using
the same heading syntax for "Ironic Networking" and the similar headings at the
other locations.
🪄 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: Pro Plus

Run ID: be734137-2fd9-4f89-b134-7e9701e7634f

📥 Commits

Reviewing files that changed from the base of the PR and between 208ccd1 and 39c8e18.

📒 Files selected for processing (11)
  • README.md
  • ironic-config/apache2-ipxe.conf.j2
  • ironic-config/inspector.ipxe.j2
  • ironic-config/ironic-networking.conf.j2
  • ironic-config/ironic.conf.j2
  • scripts/auth-common.sh
  • scripts/configure-ironic-networking.sh
  • scripts/configure-ironic.sh
  • scripts/ironic-common.sh
  • scripts/ironic-networking-common.sh
  • scripts/runironic-networking

Comment thread ironic-config/inspector.ipxe.j2 Outdated
Comment thread scripts/configure-ironic-networking.sh
Conflicts:
 - ironic-config/inspector.ipxe.j2: preserve original OCP
@iurygregory iurygregory force-pushed the merge-upstream-2026-04-13 branch from 39c8e18 to c94d4ad Compare April 13, 2026 21:20
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ironic-config/inspector.ipxe.j2 (1)

49-60: ⚠️ Potential issue | 🔴 Critical

Remove the extra {% endif %} on line 49 that causes the {% else %} on line 60 to be orphaned.

The {% endif %} on line 49 closes the if env.DEPLOY_KERNEL_BY_ARCH is defined block from line 9 prematurely, leaving the {% else %} on line 60 without a matching if, which prevents the template from parsing.

🧪 Verification output

The template contains unbalanced control flow blocks with an orphan else at line 60, confirming the issue.

🩹 Suggested fix
 :fallback
 echo Booting fallback IPA for ${buildarch}
 set ipa_kernel {{ env.IRONIC_HTTP_URL }}/images/ironic-python-agent.kernel
 set ipa_ramdisk {{ env.IRONIC_HTTP_URL }}/images/ironic-python-agent.initramfs
 set ipa_ramdisk_name ironic-python-agent.initramfs
-{%- endif %}
 goto boot
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ironic-config/inspector.ipxe.j2` around lines 49 - 60, There is an extra "{%
endif %}" that prematurely closes the "if env.DEPLOY_KERNEL_BY_ARCH is defined"
block and leaves the "{% else %}" orphaned; remove the stray "{% endif %}" so
the "{% else %}" pairs with the original "if" (ensure the block surrounding
kernel_cmdline('${ipa_kernel}', '${ipa_ramdisk_name}') and the subsequent
:boot/:retry_boot logic remains inside the intended conditional structure).
♻️ Duplicate comments (1)
ironic-config/inspector.ipxe.j2 (1)

61-74: ⚠️ Potential issue | 🔴 Critical

Still hardcoded to the default HTTP images in the single-arch path.

scripts/ironic-common.sh:195-245 still populates DEPLOY_KERNEL_URL / DEPLOY_RAMDISK_URL for this branch, but Line 73 and Line 74 ignore the computed ipa_kernel / ipa_ramdisk values and always boot the default IRONIC_HTTP_URL images. That also bypasses IRONIC_IPA_BASE_URL from scripts/ironic-common.sh:143-156, so custom images and TLS iPXE setups regress here.

🧪 Verification script
#!/bin/bash
python - <<'PY'
from pathlib import Path
lines = Path("ironic-config/inspector.ipxe.j2").read_text().splitlines()
segment = "\n".join(lines[60:75])

hardcoded = (
    "kernel_cmdline(env.IRONIC_HTTP_URL + '/images/ironic-python-agent.kernel'" in segment
    and "initrd --timeout 60000 {{ env.IRONIC_HTTP_URL }}/images/ironic-python-agent.initramfs" in segment
)
uses_precomputed = (
    "kernel_cmdline(ipa_kernel" in segment
    and "initrd --timeout 60000 {{ ipa_ramdisk }}" in segment
)

print(f"hardcoded_http_defaults={hardcoded}")
print(f"uses_precomputed_urls={uses_precomputed}")

if hardcoded and not uses_precomputed:
    raise SystemExit(1)
PY
🩹 Suggested fix
 {%- set file_url_prefix = 'file:///shared/html/' -%}
 {%- if env.DEPLOY_KERNEL_URL is defined -%}
-{%- set ipa_kernel = env.DEPLOY_KERNEL_URL | replace(file_url_prefix, env.IRONIC_HTTP_URL + '/') -%}
-{%- set ipa_ramdisk = env.DEPLOY_RAMDISK_URL | replace(file_url_prefix, env.IRONIC_HTTP_URL + '/') -%}
+{%- set ipa_kernel = env.DEPLOY_KERNEL_URL | replace(file_url_prefix, env.IRONIC_IPA_BASE_URL + '/') -%}
+{%- set ipa_ramdisk = env.DEPLOY_RAMDISK_URL | replace(file_url_prefix, env.IRONIC_IPA_BASE_URL + '/') -%}
 {%- else -%}
-{%- set ipa_kernel = env.IRONIC_HTTP_URL + '/images/ironic-python-agent.kernel' -%}
-{%- set ipa_ramdisk = env.IRONIC_HTTP_URL + '/images/ironic-python-agent.initramfs' -%}
+{%- set ipa_kernel = env.IRONIC_IPA_BASE_URL + '/images/ironic-python-agent.kernel' -%}
+{%- set ipa_ramdisk = env.IRONIC_IPA_BASE_URL + '/images/ironic-python-agent.initramfs' -%}
 {%- endif %}
 :retry_boot
 imgfree
 # NOTE(dtantsur): keep inspection kernel params in [mdns]params in
 # ironic-inspector-image and configuration in configure-ironic.sh
-{{ kernel_cmdline(env.IRONIC_HTTP_URL + '/images/ironic-python-agent.kernel', 'ironic-python-agent.initramfs') }}
-initrd --timeout 60000 {{ env.IRONIC_HTTP_URL }}/images/ironic-python-agent.initramfs || goto retry_boot
+{{ kernel_cmdline(ipa_kernel, ipa_ramdisk.split('/')[-1]) }}
+initrd --timeout 60000 {{ ipa_ramdisk }} || goto retry_boot
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ironic-config/inspector.ipxe.j2` around lines 61 - 74, The template currently
ignores the precomputed ipa_kernel and ipa_ramdisk variables and hardcodes
images to env.IRONIC_HTTP_URL; update the boot lines to use the computed
ipa_kernel and ipa_ramdisk instead: replace the kernel_cmdline(...) call to pass
ipa_kernel (not env.IRONIC_HTTP_URL + '/images/...') and change the initrd line
to reference {{ ipa_ramdisk }} (instead of {{ env.IRONIC_HTTP_URL
}}/images/ironic-python-agent.initramfs) so DEPLOY_KERNEL_URL/DEPLOY_RAMDISK_URL
and IRONIC_IPA_BASE_URL are honored; ensure you keep the existing timeout/goto
retry_boot behavior.
🧹 Nitpick comments (1)
README.md (1)

270-272: Optional wording cleanup for readability.

Line 270–272 repeats “you want to”; consider tightening phrasing to reduce repetition.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 270 - 272, The description for **refspec** repeats
"you want to"; edit the sentence to remove redundancy by rephrasing it to
something like "for example, to apply the patch at
<https://review.opendev.org/c/openstack/ironic/+/800084>" so the clause reads
smoothly and avoids the repeated phrase; update the text that starts with
"**refspec** is the gerrit refspec of the patch we want to test, for example if
you want to apply the patch at" to the tightened phrasing using **refspec** as
the subject.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Around line 180-181: Update the README entry for IRONIC_NETWORKING_ENABLED to
state that while the env var defaults to false, the standalone networking
entrypoint runironic-networking (invoked by
scripts/configure-ironic-networking.sh) force-sets
IRONIC_NETWORKING_ENABLED=true, so operators should not rely on the env var
alone when using that entrypoint; mention both the default behavior and the
override performed by configure-ironic-networking.sh/runironic-networking.
- Line 175: The heading "Ironic Networking" is written as an ATX heading (##)
but the repository expects setext style per markdownlint (MD003); replace the
"## Ironic Networking" line with a setext H2 by making the title line "Ironic
Networking" and adding a following underline of hyphens (-----) to form a setext
heading, and do the same conversion for the other new ATX headings called out in
the review so all new headings use setext style.

---

Outside diff comments:
In `@ironic-config/inspector.ipxe.j2`:
- Around line 49-60: There is an extra "{% endif %}" that prematurely closes the
"if env.DEPLOY_KERNEL_BY_ARCH is defined" block and leaves the "{% else %}"
orphaned; remove the stray "{% endif %}" so the "{% else %}" pairs with the
original "if" (ensure the block surrounding kernel_cmdline('${ipa_kernel}',
'${ipa_ramdisk_name}') and the subsequent :boot/:retry_boot logic remains inside
the intended conditional structure).

---

Duplicate comments:
In `@ironic-config/inspector.ipxe.j2`:
- Around line 61-74: The template currently ignores the precomputed ipa_kernel
and ipa_ramdisk variables and hardcodes images to env.IRONIC_HTTP_URL; update
the boot lines to use the computed ipa_kernel and ipa_ramdisk instead: replace
the kernel_cmdline(...) call to pass ipa_kernel (not env.IRONIC_HTTP_URL +
'/images/...') and change the initrd line to reference {{ ipa_ramdisk }}
(instead of {{ env.IRONIC_HTTP_URL }}/images/ironic-python-agent.initramfs) so
DEPLOY_KERNEL_URL/DEPLOY_RAMDISK_URL and IRONIC_IPA_BASE_URL are honored; ensure
you keep the existing timeout/goto retry_boot behavior.

---

Nitpick comments:
In `@README.md`:
- Around line 270-272: The description for **refspec** repeats "you want to";
edit the sentence to remove redundancy by rephrasing it to something like "for
example, to apply the patch at
<https://review.opendev.org/c/openstack/ironic/+/800084>" so the clause reads
smoothly and avoids the repeated phrase; update the text that starts with
"**refspec** is the gerrit refspec of the patch we want to test, for example if
you want to apply the patch at" to the tightened phrasing using **refspec** as
the subject.
🪄 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: Pro Plus

Run ID: e8fc564b-e5ca-4d97-84f0-1e5146070fb4

📥 Commits

Reviewing files that changed from the base of the PR and between 39c8e18 and c94d4ad.

📒 Files selected for processing (12)
  • .lycheeignore
  • README.md
  • ironic-config/apache2-ipxe.conf.j2
  • ironic-config/inspector.ipxe.j2
  • ironic-config/ironic-networking.conf.j2
  • ironic-config/ironic.conf.j2
  • scripts/auth-common.sh
  • scripts/configure-ironic-networking.sh
  • scripts/configure-ironic.sh
  • scripts/ironic-common.sh
  • scripts/ironic-networking-common.sh
  • scripts/runironic-networking
✅ Files skipped from review due to trivial changes (4)
  • .lycheeignore
  • scripts/runironic-networking
  • scripts/ironic-networking-common.sh
  • ironic-config/ironic-networking.conf.j2
🚧 Files skipped from review as they are similar to previous changes (5)
  • ironic-config/apache2-ipxe.conf.j2
  • scripts/configure-ironic.sh
  • scripts/auth-common.sh
  • scripts/configure-ironic-networking.sh
  • scripts/ironic-common.sh

Comment thread README.md
`true` will make the server enforce its cipher list ordering for TLS version
up to 1.2, defaults to `false`

## Ironic Networking
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use consistent heading style to satisfy markdownlint (MD003).

These new headings are ATX while the file’s configured style expects setext, so this will keep lint warnings active.

Also applies to: 220-220, 251-251

🧰 Tools
🪛 markdownlint-cli2 (0.22.0)

[warning] 175-175: Heading style
Expected: setext; Actual: atx

(MD003, heading-style)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 175, The heading "Ironic Networking" is written as an ATX
heading (##) but the repository expects setext style per markdownlint (MD003);
replace the "## Ironic Networking" line with a setext H2 by making the title
line "Ironic Networking" and adding a following underline of hyphens (-----) to
form a setext heading, and do the same conversion for the other new ATX headings
called out in the review so all new headings use setext style.

Comment thread README.md
Comment on lines +180 to +181
- `IRONIC_NETWORKING_ENABLED` - Enable standalone networking service
(default `false`)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clarify IRONIC_NETWORKING_ENABLED behavior for the standalone entrypoint.

Line 180 says the service is enabled via env var (default false), but scripts/configure-ironic-networking.sh (Line 7) force-sets IRONIC_NETWORKING_ENABLED=true when runironic-networking is used. Please document that distinction to avoid operator confusion.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 180 - 181, Update the README entry for
IRONIC_NETWORKING_ENABLED to state that while the env var defaults to false, the
standalone networking entrypoint runironic-networking (invoked by
scripts/configure-ironic-networking.sh) force-sets
IRONIC_NETWORKING_ENABLED=true, so operators should not rely on the env var
alone when using that entrypoint; mention both the default behavior and the
override performed by configure-ironic-networking.sh/runironic-networking.

@MahnoorAsghar
Copy link
Copy Markdown

MahnoorAsghar commented Apr 17, 2026

/retest
Failures look unrelated to this particular PR

MahnoorAsghar pushed a commit to MahnoorAsghar/ironic-image that referenced this pull request Apr 17, 2026
…ck-ironic-digest

🌱 Update openstack-ironic digest to 04aa88a (main)
Copy link
Copy Markdown

@elfosardo elfosardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we missing changes in the Dockerfile.ocp?

@iurygregory
Copy link
Copy Markdown
Author

/retest

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 24, 2026

@iurygregory: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6 c94d4ad link true /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-metal-ipi-bm c94d4ad link true /test e2e-metal-ipi-bm
ci/prow/e2e-metal-ipi-virtualmedia c94d4ad link true /test e2e-metal-ipi-virtualmedia

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@dtantsur
Copy link
Copy Markdown
Member

https://github.com/openshift/openstack-networking-generic-switch now exists and can be used here

@elfosardo
Copy link
Copy Markdown

please be aware that if you add openstack-networking-generic-switch to requirements.cachito you will have to modify also:
tools/check-requirements.sh
.github/workflows/update-requirements.yml

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants