🌱 Small refactoring in the ironic startup scripts and template #787
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
metal3-io-bot
added
the
needs-ok-to-test
|
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 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. |
metal3-io-bot
added
the
size/M
|
/ok-to-test |
metal3-io-bot
added
ok-to-test
Rozzii
left a comment
Rozzii
left a comment
There was a problem hiding this comment.
Thanks for the refactor, I also not a fun of complex jinja templates so I support the idea but I would like to point out a few shellcheck issues and nits.
mchiappero
force-pushed
the
ironic-refactoring
branch
from
284ed37 to
b81d0f9
Compare
mchiappero
force-pushed
the
ironic-refactoring
branch
2 times, most recently
from
5eaafa0 to
8e987bf
Compare
|
Same as #788 ? |
mchiappero
force-pushed
the
ironic-refactoring
branch
from
8e987bf to
50b86cd
Compare
scripts/ironic-common.sh
Outdated
|
|
||
| if [[ "$#" -gt 2 ]]; then | ||
| echo "ERROR: ${FUNCNAME[0]}: too many parameters" >&2 | ||
| return 1 |
There was a problem hiding this comment.
not sure we want a return here, let's just use exit ?
maybe also for the other return statements
unless you really want to capture the exit code, then use echo, but I don't see the point
There was a problem hiding this comment.
Whether to exit or not is not a decision to be made at this scope. The caller could, and probably should, use exit, but there is no homogeneous error handling in general across these scripts.
There was a problem hiding this comment.
Sorry, I thought it was a different error test... Yes we could exit here in this case since it would be an invalid use of the function rather than invalid input. I don't mind, it's an unfortunate side effect of using the wrong tool for the job, it doesn't make much of a difference.
scripts/ironic-common.sh
Outdated
| local PARSED_IP | ||
| PARSED_IP="$(parse_ip_address "${PROVISIONING_IP}")" | ||
| if [[ -z "${IRONIC_IP}" ]]; then | ||
| return 1 |
There was a problem hiding this comment.
we should probably provide an error message here ?
There was a problem hiding this comment.
By the way, we could wrap it in a "parse_or_die" sort of function, and avoid the test here.
There was a problem hiding this comment.
By the way, we could wrap it in a "parse_or_die" sort of function, and avoid the test here.
E.g.:
parse_ip_address_or_die()
{
local RESULT
RESULT="$(parse_ip_address $@)"
if [[ -z "${RESULT}" ]]; then
echo "ERROR: \"$@\" is not a valid IP address, failed to start ironic"
exit 1
fi
echo "${RESULT}"
}
3c0fbf5 to
0979ec7
Compare
|
I think the code now is a good shape, let me know how we can unblock this, if there is any other change you would like to see. |
|
/retest |
|
/retest |
|
Sorry, the grep bit in get_interface_of_ip is wrong for some reason, just noticing now. Let me fix it. |
Sorry for the delay, I'll change the grep command a bit, but I have tested the current version against a number of different containers, from SUSE images to Ubuntu without any problem. Let's see... |
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]>
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
force-pushed
the
ironic-refactoring
branch
from
0979ec7 to
354f16c
Compare
|
/retest |
|
/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main |
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]>
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
force-pushed
the
ironic-refactoring
branch
from
354f16c to
06ff57e
Compare
|
/retest |
|
/test metal3-centos-e2e-integration-test-main |
|
@elfosardo Please let me know if you are happy with the changes or if there are still open points. Thank you! |
5 participants
What this PR does / why we need it:
This PR introduces a set of code refactoring, aiming at making the code more readable but extracting common logic that may become useful with future code additions. Most notably:
In the future a matching utility function to retrieve the (first) IP address assigned to an interface will also be added, but implies more changes and will be introduced along with other functional changes.
Checklist: