OCPEDGE-2196: feat: add support for TNF agent based install #1798
Conversation
eggfoobar
changed the title
|
/retest-required |
agent/roles/manifests/templates/install-config_baremetal_yaml.j2
Outdated
Show resolved
Hide resolved
agent/roles/manifests/templates/install-config_baremetal_yaml.j2
Outdated
Show resolved
Hide resolved
eggfoobar
force-pushed
the
add-agent-based-tnf-support
branch
from
9f88a96 to
3fd5bcd
Compare
|
/retest-required |
1 similar comment
|
/retest-required |
| # and used as reference for TNF configurations. | ||
| export ENABLE_TWO_NODE_FENCING="" | ||
| if [[ ${NUM_ARBITERS} -eq 0 ]] && [[ ${NUM_MASTERS} -eq 2 ]]; then | ||
| export ENABLE_TWO_NODE_FENCING="true" |
There was a problem hiding this comment.
This is a little bit confusing for me. Below there is already a logic to enable the 2n fendince when the AGENT_E2E_TEST_SCENARIO is TNF, which makes sense to me.
Here I was expecting something more like
export ENABLE_TWO_NODE_FENCING=${ENABLE_TWO_NODE_FENCING:"false"}
Either to avoid an unset var either to allow the user to override it if required (usually when not using AGENT_E2E_TEST_SCENARIO).
There was a problem hiding this comment.
Ah sorry, I had forgotten another place I wanted to use this, I replaced the if logic in the ipi flow as well, so this logic just sets the correct var for that flow. As well we explicitly set the var in the agent flow.
I updated the code to allow the user to change it if in the future we want to force fencing credentials being present, but otherwise this will just be on auto pilot
There was a problem hiding this comment.
Ok, it wasn't clear to me that the same config var was used in both the bm IPI workflow and the agent one. A better way to highlight that could be to add a check, ie something like if [[ -z ${AGENT_E2E_TEST_SCENARIO} ]]; then (it may also prevent weird mistakes in case both vars were set and also AGENT_E2E_TEST_SCENARIO)
There was a problem hiding this comment.
Oh good catch, updated the code to also check for that so it only gets checked when AGENT_E2E_TEST_SCENARIO is not set, and allows the agent flow to dictate that themselves, thanks @andfasano !
eggfoobar
force-pushed
the
add-agent-based-tnf-support
branch
2 times, most recently
from
73cd1c3 to
1a138da
Compare
|
/retest-required |
|
/retest |
eggfoobar
force-pushed
the
add-agent-based-tnf-support
branch
from
1a138da to
f9d5f98
Compare
|
/retest-required |
eggfoobar
force-pushed
the
add-agent-based-tnf-support
branch
2 times, most recently
from
418427f to
a4301f5
Compare
added ENABLE_TWO_NODE_FENCING internal variable to help identify fencing for templating. added fencing block in jinja baremetal install config Signed-off-by: ehila <[email protected]>
eggfoobar
force-pushed
the
add-agent-based-tnf-support
branch
from
a4301f5 to
506935e
Compare
|
/retest |
|
@eggfoobar: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| fencing: | ||
| credentials: | ||
| {% for hostname in hostnames %} | ||
| - hostname: {{hostname}} |
There was a problem hiding this comment.
pretty sure we also allow disabling certificate verification.
| @@ -1,4 +1,9 @@ | |||
| {% import 'net_macros.yaml' as net %} | |||
| {% set hostnames = agent_nodes_hostnames.split(',') %} | |||
| {% set bmc_addresses = agent_nodes_bmc_addresses.split(',') %} | |||
There was a problem hiding this comment.
One thing we don't have a 1/1 mapping for is ipmi support for BMC.
This is supported for provisioning via the baremetal operator via ironic, but is NOT supported for TNF.
I know it looks like duplicate information, but I would avoid reusing the same variables as bmc used for the baremetalhost definitions because we don't have feature parity and may need to support a divergent feature set from the provisioning side.
3 participants
No description provided.