Skip to content

⚠️ Improve priority and validation of input variables consumed in wait_for_interface_or_ip #788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

⚠️ Improve priority and validation of input variables consumed in wait_for_interface_or_ip #788

wants to merge 6 commits into from

Conversation

@mchiappero
Copy link

@mchiappero mchiappero commented Nov 14, 2025 •
edited
Loading

What this PR does / why we need it:
The current logic in wait_for_interface_or_ip deals with the input IP addresses or interfaces, according to the following order: PROVISIONING_IP, IRONIC_IP, PROVISIONING_INTERFACE. The improvements this PR introduces are:

  • make the test case for IRONIC_IP explicit rather than implicit, improving code readability before the following change
  • "group" PROVISIONING_IP and PROVISIONING_INTERFACE in terms of priority by prioritizing IRONIC_IP instead, resulting in the following order: IRONIC_IP, PROVISIONING_IP, PROVISIONING_INTERFACE
  • include input validation for the externally provided IRONIC_IP

What the PR does not address is logging a warning in case of multiple input variables set and one taking inevitably precedence over the another(s).

NOTE: based on PR #787

Checklist:

  • Documentation has been updated, if necessary.
  • Integration tests have been added, if necessary.

@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Nov 14, 2025

[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 rozzii for approval. For more information see the Code Review Process.

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

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 14, 2025
@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Nov 14, 2025

Hi @mchiappero. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 14, 2025
@mchiappero mchiappero mentioned this pull request Nov 14, 2025
Closed
@mchiappero mchiappero force-pushed the waitforiforip-improvements branch 3 times, most recently from 85381e6 to 4b626bc Compare November 14, 2025 17:37
tuminoid
tuminoid reviewed Nov 16, 2025
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Some minor nitting, and also please squash the commits.

As for the actual logic, I need another look with fresher eyes tomorrow.
/cc @Rozzii @dtantsur
PTAL.

@tuminoid
Copy link
Member

tuminoid commented Nov 16, 2025

/ok-to-test

@metal3-io-bot metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 16, 2025
@tuminoid
Copy link
Member

tuminoid commented Nov 16, 2025

Also, isn't this the same as #787 ?

@tuminoid tuminoid mentioned this pull request Nov 16, 2025
Open
1 task
@mchiappero
Copy link
Author

mchiappero commented Nov 17, 2025

Also, isn't this the same as #787 ?

The PRs are stacked, as far as I can see it's not possible to set a different branch, but I should have mentioned that in the description. Let me fix it... along with the proposed changes (thanks for reviewing!).

@mchiappero mchiappero mentioned this pull request Nov 17, 2025
Open
2 tasks
@mchiappero
Copy link
Author

mchiappero commented Nov 17, 2025

Some minor nitting, and also please squash the commits.

As for the actual logic, I need another look with fresher eyes tomorrow. /cc @Rozzii @dtantsur PTAL.

Sure. But before squashing I'd like to make sure the change is fully understood, though. By the way, IRONIC_IP is not documented in https://github.com/metal3-io/ironic-image/blob/main/README.md, while it probably should. Also, which variables take precedence is also not documented either, nor logged by the scripts. I guess maybe something to discuss at the weekly meeting. Let me know what you think!

@mchiappero
Simplify the setting of host_ip in ironic.conf
5a10f7e 
The value of host_ip is determined twice within the ironic.conf.j2 template
file, by means of a relatively hard to read set of conditions.

Avoid this duplication and improve readability by exporting the correct
value once in scripts/configure-ironic.sh. This also leave more room for
more complex evaluations should these be needed in the future (e.g.
IPv6)

Signed-off-by: Marco Chiappero <[email protected]>
@mchiappero mchiappero force-pushed the waitforiforip-improvements branch from 4b626bc to b26391c Compare November 17, 2025 20:00
@elfosardo
Copy link
Member

elfosardo commented Nov 18, 2025

/hold
let's hold on to this until we deal with #787

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 18, 2025
@mchiappero
Introduce an address parsing utility function
b5e013b 
Whether IRONIC_IP or PROVISIONIG_IP is provided to ironic-image, any
such address should be checked for validity and, if needed,
properly formatted. For this reason, this commit introduces
parse_ip_address to be consumed inside wait_for_interface_or_ip.

Signed-off-by: Marco Chiappero <[email protected]>
@mchiappero
Introduce get_interface_of_ip to look PROVISIONING_IP up
25de33b 
Add a new get_interface_of_ip function that returns the name of the
interface of a given IP, and use it to determine if the
PROVISIONING_IP address is actually present on a network interface.

This improves the code readability and enables additional debugging
output.

Signed-off-by: Marco Chiappero <[email protected]>
@mchiappero
Revert 2742439 being now redundant
3c0fbf5 
Commit 2742439 added logic to tentatively identify the interface name
in get_provisioning_interface if the PROVISIONING_IP is provided.
However the same process in then repeated in wait_for_interface_or_ip.

Signed-off-by: Marco Chiappero <[email protected]>
@mchiappero
Improve validation of the input variables in wait_for_interface_or_ip
bd05dc0 
Introduce an explicit check for IRONIC_IP, allowing
PROVISIONING_INTERFACE to be tested separately, to improve quality
and readability.

Moreover, whenever PROVISIONING_INTERFACE is not set by the user,
function get_provisioning_interface attempts to determine one, or
provide "provisioning" as default value. However this can cause
confusing errors down the line. So, also remove this default value
and fail gracefully, with proper logging, if the
PROVISIONING_INTERFACE value is not detected.

Signed-off-by: Marco Chiappero <[email protected]>
@mchiappero
Make IRONIC_IP higher priority than PROVISIONING_IP
a3971ce 
Up to now wait_for_interface_or_ip has parse the values in the
following order: PROVISIONING_IP, IRONIC_IP, PROVISIONING_INTERFACE.
However IRONIC_IP should likely be considered as overriding any
PROVISIONING_* value. Thus, make sure IRONIC_IP is evaluated at the
beginning of the chain.

Signed-off-by: Marco Chiappero <[email protected]>
@mchiappero mchiappero force-pushed the waitforiforip-improvements branch from b26391c to a3971ce Compare November 18, 2025 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tuminoid tuminoid tuminoid left review comments

@elfosardo elfosardo Awaiting requested review from elfosardo

@zaneb zaneb Awaiting requested review from zaneb

@dtantsur dtantsur Awaiting requested review from dtantsur

@Rozzii Rozzii Awaiting requested review from Rozzii

Assignees

No one assigned

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mchiappero @metal3-io-bot @tuminoid @elfosardo