openshift / assisted-service

Apache License 2.0
109 stars 209 forks source link

MGMT-18510: Add an annotation to override ironic IP family to provide in dual-stack hubs #6686

Closed carbonin closed 1 day ago

carbonin commented 3 weeks ago

Currently when a hub cluster is dual-stack the only way the preprovisioning image controller can determine which callback IP family to provide to the ironic agent is to use the cluster reference in the infraenv. If there is no cluster reference (for late binding cases) then the controller always provides the primary IP family of the hub cluster.

This prevents users from using late-binding to deploy ipv6 only hosts from an ipv4-primary dual-stack hub (or vice versa).

This commit adds an annotation on the infraenv that can be used to override the ip family used when discovering hosts with a particular discovery image. infraenv.agent-install.openshift.io/ip-family can be set to v4, v6, or v4,v6 to indicate to the controller which family should be used.

In newer OCP versions the ironic agent supports multiple callback URLs in its config, but the infrastructure-operator still supports running on hubs that don't have this change so this annotation is still required for those situations.

List all the issues related to this PR

Partially resolves https://issues.redhat.com/browse/MGMT-18510

What environments does this code impact?

How was this code tested?

Unit tests only - I don't have a dual-stack hub cluster to test this on

Checklist

Reviewers Checklist

openshift-ci-robot commented 3 weeks ago

@carbonin: This pull request references MGMT-18510 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.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/assisted-service/pull/6686): >Currently when a hub cluster is dual-stack the only way the preprovisioning image controller can determine which callback IP family to provide to the ironic agent is to use the cluster reference in the infraenv. If there is no cluster reference (for late binding cases) then the controller always provides the primary IP family of the hub cluster. > >This prevents users from using late-binding to deploy ipv6 only hosts from an ipv4-primary dual-stack hub (or vice versa). > >This commit adds an annotation on the infraenv that can be used to override the ip family used when discovering hosts with a particular discovery image. `infraenv.agent-install.openshift.io/ip-family` can be set to `v4`, `v6`, or `v4,v6` to indicate to the controller which family should be used. > >In newer OCP versions the ironic agent supports multiple callback URLs in its config, but the infrastructure-operator still supports running on hubs that don't have this change so this annotation is still required for those situations. > >## List all the issues related to this PR > >Partially resolves https://issues.redhat.com/browse/MGMT-18510 > >- [ ] New Feature >- [x] Enhancement >- [x] Bug fix >- [ ] Tests >- [ ] Documentation >- [ ] CI/CD > >## What environments does this code impact? > >- [ ] Automation (CI, tools, etc) >- [ ] Cloud >- [x] Operator Managed Deployments >- [ ] None > >## How was this code tested? > >Unit tests only - I don't have a dual-stack hub cluster to test this on > > > >- [ ] assisted-test-infra environment >- [ ] dev-scripts environment >- [ ] Reviewer's test appreciated >- [x] Waiting for CI to do a full test run >- [ ] Manual (Elaborate on how it was tested) >- [ ] No tests needed > >## Checklist > >- [x] Title and description added to both, commit and PR. >- [x] Relevant issues have been associated (see [CONTRIBUTING] guide) >- [ ] This change does not require a documentation update (docstring, `docs`, README, etc) - docs added >- [x] 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](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.19%. Comparing base (c5cda7e) to head (2efed43). Report is 25 commits behind head on master.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/openshift/assisted-service/pull/6686/graphs/tree.svg?width=650&height=150&src=pr&token=YOR4NSSOXQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)](https://app.codecov.io/gh/openshift/assisted-service/pull/6686?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) ```diff @@ Coverage Diff @@ ## master #6686 +/- ## ========================================== + Coverage 68.64% 70.19% +1.55% ========================================== Files 246 246 Lines 36860 38714 +1854 ========================================== + Hits 25301 27175 +1874 + Misses 9307 9274 -33 - Partials 2252 2265 +13 ``` | [Files with missing lines](https://app.codecov.io/gh/openshift/assisted-service/pull/6686?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) | Coverage Δ | | |---|---|---| | [...rnal/controller/controllers/infraenv\_controller.go](https://app.codecov.io/gh/openshift/assisted-service/pull/6686?src=pr&el=tree&filepath=internal%2Fcontroller%2Fcontrollers%2Finfraenv_controller.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift#diff-aW50ZXJuYWwvY29udHJvbGxlci9jb250cm9sbGVycy9pbmZyYWVudl9jb250cm9sbGVyLmdv) | `64.13% <ø> (ø)` | | | [...ler/controllers/preprovisioningimage\_controller.go](https://app.codecov.io/gh/openshift/assisted-service/pull/6686?src=pr&el=tree&filepath=internal%2Fcontroller%2Fcontrollers%2Fpreprovisioningimage_controller.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift#diff-aW50ZXJuYWwvY29udHJvbGxlci9jb250cm9sbGVycy9wcmVwcm92aXNpb25pbmdpbWFnZV9jb250cm9sbGVyLmdv) | `81.81% <100.00%> (+0.24%)` | :arrow_up: | ... and [9 files with indirect coverage changes](https://app.codecov.io/gh/openshift/assisted-service/pull/6686/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)
carbonin commented 6 days ago

@adriengentil can you take a look at this one when you get a chance?

openshift-ci[bot] commented 1 day ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, 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](https://github.com/openshift/assisted-service/blob/master/OWNERS)~~ [adriengentil,carbonin] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci-robot commented 1 day ago

/retest-required

Remaining retests: 0 against base HEAD 48d04f6ad16616f0639bc28780842dc689715e18 and 2 for PR HEAD 2efed43109c4e3c1bd5713790e73980efe1cfa3e in total

openshift-ci[bot] commented 1 day ago

@carbonin: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-bot commented 1 day ago

[ART PR BUILD NOTIFIER]

Distgit: ose-agent-installer-api-server This PR has been included in build ose-agent-installer-api-server-container-v4.18.0-202409120210.p0.gf2f02f5.assembly.stream.el9. All builds following this will include this PR.