MGMT-16814: Pass both IP family URLs to ironic agent

[carbonin]

When deploying a single stack spoke cluster from a dual-stack hub it's hard for us to determine what callback URLs we should send to the ironic agent. If we get the choice wrong the agent simply never registers and the ironic agent gets stuck trying to send data to ironic on the hub.

As of https://issues.redhat.com/browse/OCPBUGS-24579 the ironic agent can accept a comma-separated list of URLs and will select the correct one based on the host's networking situation at runtime.

This PR provides that comma-separated list of URLs to the ironic agent if the agent is of a version that includes the fix (currently OCP 4.16+).

As a side effect we also need to track the version of the ironic agent we're embedding. This is so that we don't provide a list of URLs to an agent that is only expecting a single one. This is simple enough for now, but may become more complicated as the fix is backported (we may need to track multiple releases for each Y-stream depending on how far back the fix goes). This also means that we need users to provide the version of overrides and the env-configurable defaults.

List all the issues related to this PR

https://issues.redhat.com/browse/MGMT-16814

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

I don't have the environment to test this, but QE is going to run my image through a test.

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see [CONTRIBUTING] guide)
• This change does not require a documentation update (docstring, docs, README, etc) - docs updated
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

[openshift-ci-robot]

@carbonin: This pull request references MGMT-16814 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set.

In response to this:

When deploying a single stack spoke cluster from a dual-stack hub it's hard for us to determine what callback URLs we should send to the ironic agent. If we get the choice wrong the agent simply never registers and the ironic agent gets stuck trying to send data to ironic on the hub.

As of https://issues.redhat.com/browse/OCPBUGS-24579 the ironic agent can accept a comma-separated list of URLs and will select the correct one based on the host's networking situation at runtime.

This PR provides that comma-separated list of URLs to the ironic agent if the agent is of a version that includes the fix (currently OCP 4.16+).

As a side effect we also need to track the version of the ironic agent we're embedding. This is so that we don't provide a list of URLs to an agent that is only expecting a single one. This is simple enough for now, but may become more complicated as the fix is backported (we may need to track multiple releases for each Y-stream depending on how far back the fix goes). This also means that we need users to provide the version of overrides and the env-configurable defaults.

List all the issues related to this PR

https://issues.redhat.com/browse/MGMT-16814

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

I don't have the environment to test this, but QE is going to run my image through a test.

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see [CONTRIBUTING] guide)
• This change does not require a documentation update (docstring, docs, README, etc) - docs updated
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin

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:

• OWNERS [carbonin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

Attention: Patch coverage is 88.23529% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 68.39%. Comparing base (ee0189b) to head (98910e7).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6047      +/-   ##
==========================================
- Coverage   68.39%   68.39%   -0.01%     
==========================================
  Files         239      239              
  Lines       35454    35472      +18     
==========================================
+ Hits        24248    24260      +12     
- Misses       9100     9104       +4     
- Partials     2106     2108       +2     

Files	Coverage Δ
...rnal/controller/controllers/infraenv_controller.go	64.13% <ø> (ø)
internal/ignition/ironic.go	100.00% <100.00%> (ø)
...ler/controllers/preprovisioningimage_controller.go	81.41% <87.50%> (+0.41%)	⬆️

... and 3 files with indirect coverage changes

[carbonin]

I'm a bit concerned about maintaining this ...

If, for example, the ironic fix gets backported to both 4.15 and 4.14 we'd need to maintain 2 different versions for this mode to be available.

I wonder if it would be acceptable to just fix the existing case that should have handled the dual-stack hub + ipv6-only spoke (I'm only assuming there is a bug there because there is no must-gather).
My guess is that since we're using the machine networks to determine if the cluster is dual-stack we're running into an issue because machine networks wouldn't be set before nodes are discovered with baremetal spokes, right?

@eranco74 @avishayt what do you think?

[carbonin]

/hold

See previous comment for reasoning.

[openshift-ci]

@carbonin: 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/edge-e2e-ai-operator-disconnected-capi	98910e7	link	false	/test edge-e2e-ai-operator-disconnected-capi
ci/prow/e2e-agent-compact-ipv4	98910e7	link	false	/test e2e-agent-compact-ipv4

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

[carbonin]

Closing this in favor of #6048.