Skip to content

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

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

Merged
openshift-merge-bot merged 1 commit into from
Sep 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/hive-integration/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,19 @@ This can be found using the following command:
oc adm release info --image-for=ironic-agent <hub-release-image>
```

### Ironic Agent callback IP family

If the hub cluster is a dual stack cluster the preprovisioning image controller needs to pick which IP family to provide to the ironic agent for callback purposes.

This is done using the following priority system:

1. Use the IP family from the `infraenv.agent-install.openshift.io/ip-family` annotation
- This can be `v4`, `v6`, or `v4,v6` depending on what the hosts in this infraenv support
- Use this option when using late binding or when the following behavior doesn't work for your use case
2. IP family of the cluster associated with the InfraEnv
- If the InfraEnv has a cluster associated with it that cluster's networking will be checked to determine the correct IP family to use
3. Default to the primary IP family of the hub cluster

[ZTP converged flow](ZTP_converged_flow.png)

## Working with mirror registry
Expand Down
13 changes: 9 additions & 4 deletions internal/controller/controllers/infraenv_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,15 @@ import (
"sigs.k8s.io/controller-runtime/pkg/source"
)

const defaultRequeueAfterPerRecoverableError = 2 * bminventory.WindowBetweenRequestsInSeconds
const InfraEnvFinalizerName = "infraenv." + aiv1beta1.Group + "/ai-deprovision"
const EnableIronicAgentAnnotation = "infraenv." + aiv1beta1.Group + "/enable-ironic-agent"
const ironicAgentImageOverrideAnnotation = "infraenv." + aiv1beta1.Group + "/ironic-agent-image-override"
const (
defaultRequeueAfterPerRecoverableError = 2 * bminventory.WindowBetweenRequestsInSeconds
InfraEnvFinalizerName = "infraenv." + aiv1beta1.Group + "/ai-deprovision"
EnableIronicAgentAnnotation = "infraenv." + aiv1beta1.Group + "/enable-ironic-agent"
ironicAgentImageOverrideAnnotation = "infraenv." + aiv1beta1.Group + "/ironic-agent-image-override"
infraEnvIPFamilyAnnotation = "infraenv." + aiv1beta1.Group + "/ip-family"
ipv4Family = "v4"
ipv6Family = "v6"
)

type InfraEnvConfig struct {
ImageType models.ImageType `envconfig:"ISO_IMAGE_TYPE" default:"minimal-iso"`
Expand Down
20 changes: 13 additions & 7 deletions internal/controller/controllers/preprovisioningimage_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ func (r *PreprovisioningImageReconciler) getIronicConfig(ctx context.Context, lo
}

if iccConfig.IronicBaseURL == "" && iccConfig.IronicInspectorBaseUrl == "" {
err := r.fillIronicServiceURLs(ctx, infraEnvInternal, iccConfig)
err := r.fillIronicServiceURLs(ctx, infraEnv, infraEnvInternal, iccConfig)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -566,18 +566,23 @@ func (r *PreprovisioningImageReconciler) AddIronicAgentToInfraEnv(ctx context.Co
return updated, nil
}

func (r *PreprovisioningImageReconciler) getIPFamilyForInfraEnv(ctx context.Context, infraEnv *common.InfraEnv) (v4 bool, v6 bool, err error) {
if infraEnv.ClusterID == "" {
func (r *PreprovisioningImageReconciler) getIPFamilyForInfraEnv(ctx context.Context, infraEnv *aiv1beta1.InfraEnv, internalInfraEnv *common.InfraEnv) (v4 bool, v6 bool, err error) {
ipFamilyAnnotation := infraEnv.GetAnnotations()[infraEnvIPFamilyAnnotation]
if ipFamilyAnnotation != "" {
families := strings.Split(ipFamilyAnnotation, ",")
return funk.Contains(families, ipv4Family), funk.Contains(families, ipv6Family), nil
}
if internalInfraEnv.ClusterID == "" {

@adriengentil adriengentil Sep 10, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why use internalInfraEnv here instead of infraEnv as before? It's because of the late binding?

@carbonin carbonin Sep 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I changed the name to be more consistent with the other functions that have both.
Now internalInfraEnv is the db object and infraEnv is the CR.

In this case I needed both because infraEnv has the annotations.

@adriengentil adriengentil Sep 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh right! I missed that infraEnv was referring to the DB object 🤦‍♂️, it's a very good change 👍

return false, false, fmt.Errorf("cannot find address family for non-bound infraEnv")
}
cluster, err := r.Installer.GetClusterInternal(ctx, installer.V2GetClusterParams{ClusterID: infraEnv.ClusterID})
cluster, err := r.Installer.GetClusterInternal(ctx, installer.V2GetClusterParams{ClusterID: internalInfraEnv.ClusterID})
if err != nil {
return false, false, err
}
return network.GetConfiguredAddressFamilies(cluster)
}

func (r *PreprovisioningImageReconciler) fillIronicServiceURLs(ctx context.Context, infraEnv *common.InfraEnv, iccConfig *ICCConfig) error {
func (r *PreprovisioningImageReconciler) fillIronicServiceURLs(ctx context.Context, infraEnv *aiv1beta1.InfraEnv, internalInfraEnv *common.InfraEnv, iccConfig *ICCConfig) error {
ironicIPs, inspectorIPs, err := r.BMOUtils.GetIronicIPs()
if err != nil {
return err
Expand All @@ -589,9 +594,10 @@ func (r *PreprovisioningImageReconciler) fillIronicServiceURLs(ctx context.Conte
inspectorURL := getUrlFromIP(inspectorIPs[0])

if len(ironicIPs) > 1 {
v4, v6, err := r.getIPFamilyForInfraEnv(ctx, infraEnv)
v4, v6, err := r.getIPFamilyForInfraEnv(ctx, infraEnv, internalInfraEnv)
r.Log.Debugf("infraEnv IP families: v4: %t, v6: %t", v4, v6)
if err != nil {
r.Log.WithError(err).Warnf("failed to determine IP family for infraEnv %s", infraEnv.ID)
r.Log.WithError(err).Warnf("failed to determine IP family for infraEnv %s", internalInfraEnv.ID)
} else if !v4 && v6 {
// spoke is single stack v6 so take v6 hub address
ironicURL = getUrlFromIP(ironicIPs[1])
Expand Down
27 changes: 26 additions & 1 deletion internal/controller/controllers/preprovisioningimage_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ var _ = Describe("PreprovisioningImage reconcile", func() {
Expect(*internalIgnitionConfig).Should(ContainSubstring(ironicServiceIPs[0]))
Expect(*internalIgnitionConfig).Should(ContainSubstring(ironicInspectorIPs[0]))
}).Return(
&common.InfraEnv{InfraEnv: models.InfraEnv{ClusterID: clusterID, ID: &infraEnvID, DownloadURL: downloadURL, CPUArchitecture: infraEnvArch}, GeneratedAt: strfmt.DateTime(time.Now())}, nil).Times(1)
&common.InfraEnv{InfraEnv: models.InfraEnv{ID: &infraEnvID, DownloadURL: downloadURL, CPUArchitecture: infraEnvArch}, GeneratedAt: strfmt.DateTime(time.Now())}, nil).Times(1)
mockCRDEventsHandler.EXPECT().NotifyInfraEnvUpdates(infraEnv.Name, infraEnv.Namespace).Times(1)
mockBMOUtils.EXPECT().getICCConfig(gomock.Any()).Times(1).Return(nil, errors.Errorf("ICC configuration is not available"))
res, err := pr.Reconcile(ctx, newPreprovisioningImageRequest(ppi))
Expand Down Expand Up @@ -233,6 +233,31 @@ var _ = Describe("PreprovisioningImage reconcile", func() {
Expect(c.Get(ctx, key, infraEnv)).To(BeNil())
Expect(infraEnv.ObjectMeta.Annotations[EnableIronicAgentAnnotation]).To(Equal("true"))
})
It("Adds the ironic IPv6 address to the ignition when the infraEnv has no cluster and is annotated for IPv6", func() {
metav1.SetMetaDataAnnotation(&infraEnv.ObjectMeta, infraEnvIPFamilyAnnotation, ipv6Family)
Expect(c.Create(ctx, infraEnv)).To(BeNil())
backendInfraEnv.ClusterID = ""
mockInstallerInternal.EXPECT().GetInfraEnvByKubeKey(gomock.Any()).Return(backendInfraEnv, nil)
mockInstallerInternal.EXPECT().UpdateInfraEnvInternal(gomock.Any(), gomock.Any(), gomock.Any()).
Do(func(ctx context.Context, params installer.UpdateInfraEnvParams, internalIgnitionConfig *string) {
Expect(params.InfraEnvID).To(Equal(*backendInfraEnv.ID))
Expect(*internalIgnitionConfig).Should(ContainSubstring(url.QueryEscape(ironicServiceIPs[1])))

@paul-maidment paul-maidment Sep 5, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I made the assumption here that ironicServiceIPs[0] is an IPv4 address and that ironicServiceIPs[1] is an IPv6 address. Though it's not directly obvious at first glance in the test.

Could we have a comment here that points out that we are using IPv6 addresses here?

@carbonin carbonin Sep 6, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The test name is "Adds the ironic IPv6 address to the ignition when the infraEnv has no cluster and is annotated for IPv6" do you really think more commentary is required?

Also the ironicServiceIPs variable is just a hardcoded list here

@paul-maidment paul-maidment Sep 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I saw the title of the test and I knew of the hard coded IP slice.

I thought it would be more convenient for future readers of this code to not have to look up the hard coded list. Instead of a comment, the code could be updated to make things read more nicely.

I mean something like this...with the appropriate changes to where the original slices are referenced, it doesn't make a massive difference but will make it extremely obvious when the v4 or v6 IP is referenced.

var (
		...
		...
		ipV4IP                = "198.51.100.1"
		ipV6IP                = "2001:db8::dead:beee"
		ironicServiceIPs      = []string{ipV4IP, ipV6IP}
		ironicInspectorIPs    = []string{ipV4IP, ipV6IP}
		...
		...
)

Expect(*internalIgnitionConfig).Should(ContainSubstring(url.QueryEscape(ironicInspectorIPs[1])))
}).Return(
&common.InfraEnv{InfraEnv: models.InfraEnv{ID: &infraEnvID, DownloadURL: downloadURL, CPUArchitecture: infraEnvArch}, GeneratedAt: strfmt.DateTime(time.Now())}, nil).Times(1)
mockCRDEventsHandler.EXPECT().NotifyInfraEnvUpdates(infraEnv.Name, infraEnv.Namespace).Times(1)
mockBMOUtils.EXPECT().getICCConfig(gomock.Any()).Times(1).Return(nil, errors.Errorf("ICC configuration is not available"))
res, err := pr.Reconcile(ctx, newPreprovisioningImageRequest(ppi))
Expect(err).To(BeNil())
Expect(res).To(Equal(ctrl.Result{}))

key := types.NamespacedName{
Namespace: testNamespace,
Name: "testInfraEnv",
}
Expect(c.Get(ctx, key, infraEnv)).To(BeNil())
Expect(infraEnv.ObjectMeta.Annotations[EnableIronicAgentAnnotation]).To(Equal("true"))
})
It("Wait for InfraEnv cool down", func() {
infraEnv.Status.ISODownloadURL = downloadURL
infraEnv.Status.CreatedTime = &metav1.Time{Time: time.Now()}
Expand Down