Skip to content

Set dhcp vars for dnsmasq via IRSO - #1702

Open
Sunnatillo wants to merge 1 commit into
metal3-io:mainfrom
Nordix:Sunnatillo/fix-dhcp-vars
Open

Set dhcp vars for dnsmasq via IRSO#1702
Sunnatillo wants to merge 1 commit into
metal3-io:mainfrom
Nordix:Sunnatillo/fix-dhcp-vars

Conversation

@Sunnatillo

@Sunnatillo Sunnatillo commented Jun 25, 2026

Copy link
Copy Markdown
Member

This PR sets dhcp_hosts and dhcp_ignore vars for dnsmasq via IRSO

@metal3-io-bot

metal3-io-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign elfosardo 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

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 25, 2026
@Sunnatillo

Sunnatillo commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

/test metal3-centos-e2e-integration-test-release-1-13 metal3-dev-env-integration-test-ubuntu-main

@Sunnatillo Sunnatillo changed the title Set dhcp_hosts and dhcp_ignore vars for dnsmasq via IRSO Set dhcp vars for dnsmasq via IRSO Jun 25, 2026
@Sunnatillo

Sunnatillo commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

/copilot-review

@metal3-io-bot
metal3-io-bot requested a review from Copilot June 25, 2026 10:57
@metal3-io-bot

metal3-io-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Copilot code review has been requested by @Sunnatillo. Please allow a few moments for the review to be added.

Copilot AI reviewed Jun 25, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates metal3-dev-env’s IRSO-based Ironic launch path to pass dnsmasq DHCP configuration (DHCP_HOSTS, DHCP_IGNORE) via the generated Ironic custom resource, rather than via the legacy environment-variable configmap flow used when launching Ironic without IRSO.

Changes:

  • Build YAML snippets for dnsmasq DHCP hosts and ignore settings from DHCP_HOSTS / DHCP_IGNORE.
  • Inject those settings into the generated Ironic CR under spec.networking.dhcp.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread 03_launch_mgmt_cluster.sh Outdated
Comment thread 03_launch_mgmt_cluster.sh
Comment thread 03_launch_mgmt_cluster.sh
@Sunnatillo

Sunnatillo commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

Test failed exacly where I was expecting, it means changes are working. I will update the PR.

@Sunnatillo
Sunnatillo force-pushed the Sunnatillo/fix-dhcp-vars branch from 95dc580 to 7231c2a Compare June 25, 2026 12:17
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 25, 2026
@Sunnatillo
Sunnatillo force-pushed the Sunnatillo/fix-dhcp-vars branch from 7231c2a to 59703be Compare June 25, 2026 12:24
@Sunnatillo

Sunnatillo commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

/test metal3-centos-e2e-integration-test-release-1-13 metal3-dev-env-integration-test-ubuntu-main metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

@Sunnatillo

Sunnatillo commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

/cc @lentzi90

@metal3-io-bot
metal3-io-bot requested a review from lentzi90 June 25, 2026 12:26
lentzi90
lentzi90 reviewed Jun 26, 2026
View reviewed changes

@lentzi90 lentzi90 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for taking care of this!
I think we can skip the parsing and formatting and just pass the variable through. What do you think about this?

Comment thread 03_launch_mgmt_cluster.sh Outdated
rangeBegin: "${CLUSTER_DHCP_RANGE_START}"
rangeEnd: "${CLUSTER_DHCP_RANGE_END}"
networkCIDR: "${BARE_METAL_PROVISIONER_NETWORK}"
networkCIDR: "${BARE_METAL_PROVISIONER_NETWORK}"${dhcp_yaml}

@lentzi90 lentzi90 Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is quite hard to read. We can actually do it much simpler.
IRSO does parse separate items into the ; separated string, but it is also valid to put that string directly.

Suggested change
networkCIDR: "${BARE_METAL_PROVISIONER_NETWORK}"${dhcp_yaml}
networkCIDR: "${BARE_METAL_PROVISIONER_NETWORK}"
hosts:
- "${DHCP_HOSTS}"
ignore:
- "${DHCP_IGNORE}"

@Sunnatillo Sunnatillo Jul 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, I agree to keep this simple.

@Sunnatillo Sunnatillo Jul 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have tried, but we have two different way of configuring it now. One via old ironic way, where we pass the variables which includes mac addresses separeted by semicolon, the new irso way we give list of mac addresses via yaml.

Could not find the better solution.

@lentzi90 lentzi90 Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry I must be missing something. What happens if you try my suggestion above? My understanding is that IRSO is backwards compatible with the old format. You should be able to use it as it is, without any parsing.

@Sunnatillo
Sunnatillo force-pushed the Sunnatillo/fix-dhcp-vars branch from 59703be to 0913353 Compare July 1, 2026 13:47
@metal3-io-bot metal3-io-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2026
@Sunnatillo

Sunnatillo commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

/copilot-review

@metal3-io-bot
metal3-io-bot requested a review from Copilot July 1, 2026 13:48
@metal3-io-bot

metal3-io-bot commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Copilot code review has been requested by @Sunnatillo. Please allow a few moments for the review to be added.

Copilot AI reviewed Jul 1, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread 03_launch_mgmt_cluster.sh Outdated
Comment thread vars.md
@Sunnatillo
Add setting DHCP_HOSTS and DHCP_IGNORE vars for dnsmasq via IRSO
2e0df5d 
Signed-off-by: Sunnatillo <sunnat.samadov@est.tech>
@Sunnatillo
Sunnatillo force-pushed the Sunnatillo/fix-dhcp-vars branch from 0913353 to 2e0df5d Compare July 1, 2026 16:01
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@adilGhaffarDev adilGhaffarDev Awaiting requested review from adilGhaffarDev
@kashifest kashifest Awaiting requested review from kashifest
@smoshiur1237 smoshiur1237 Awaiting requested review from smoshiur1237
+1 more reviewer
@lentzi90 lentzi90 lentzi90 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Sunnatillo @metal3-io-bot @lentzi90