From 302431d6b1adc698ac59de8f6936dbc1ed539987 Mon Sep 17 00:00:00 2001 From: haijianyang Date: Fri, 29 Mar 2024 03:07:23 -0400 Subject: [PATCH 1/3] Refactor IPPool selection code --- controllers/elfmachine_controller.go | 10 +++ controllers/elfmachine_controller_test.go | 2 + pkg/ipam/ipam.go | 2 +- pkg/ipam/metal3io/ipam.go | 78 +++++++++++++++-------- 4 files changed, 63 insertions(+), 29 deletions(-) diff --git a/controllers/elfmachine_controller.go b/controllers/elfmachine_controller.go index abf6029..3f442ab 100644 --- a/controllers/elfmachine_controller.go +++ b/controllers/elfmachine_controller.go @@ -336,6 +336,16 @@ func (r *ElfMachineReconciler) reconcileDeviceIPAddress(ctx *context.MachineCont return ctrl.Result{}, nil } +// getIPPool returns the specified IPPool. +// +// getIPPool selects IPPool according to the following priorities: +// (1) Without ip-pool-name +// 1. select IPPool with `is-default`` label from elfMachine.Namespace +// 2. select IPPool with `is-default` label from default namespace +// (2) With ip-pool-name(from AddressesFromPools or ElfMachineTemplate) +// 1. select IPPool using the specified ip-pool-name and ip-pool-namespace +// 2. select the IPPool named ip-pool-name in the default namespace + func (r *ElfMachineReconciler) getIPPool(ctx *context.MachineContext, device capev1.NetworkDeviceSpec) (ipam.IPPool, error) { poolMatchLabels := make(map[string]string) // Prefer IPPool of device. Only Metal3 IPPool is supported now. diff --git a/controllers/elfmachine_controller_test.go b/controllers/elfmachine_controller_test.go index e36ee01..3b87814 100644 --- a/controllers/elfmachine_controller_test.go +++ b/controllers/elfmachine_controller_test.go @@ -226,9 +226,11 @@ var _ = Describe("ElfMachineReconciler", func() { It("should wait for IP when IPClaim without IP", func() { ctrlutil.AddFinalizer(elfMachine, MachineStaticIPFinalizer) + metal3IPPool.Namespace = ipam.DefaultIPPoolNamespace metal3IPPool.Labels = map[string]string{ ipam.ClusterIPPoolGroupKey: "ip-pool-group", ipam.ClusterNetworkNameKey: "ip-pool-vm-network", + ipam.DefaultIPPoolKey: "true", } elfMachineTemplate.Labels = metal3IPPool.Labels metal3IPClaim, metal3IPAddress = fake.NewMetal3IPObjects(metal3IPPool, ipamutil.GetFormattedClaimName(elfMachine.Namespace, elfMachine.Name, 0)) diff --git a/pkg/ipam/ipam.go b/pkg/ipam/ipam.go index 4c66e9c..c8009f5 100644 --- a/pkg/ipam/ipam.go +++ b/pkg/ipam/ipam.go @@ -29,6 +29,6 @@ const ( // Default IPPool. const ( - DefaultIPPoolNamespace = "cape-system" + DefaultIPPoolNamespace = "default" DefaultIPPoolKey = "ippool.cluster.x-k8s.io/is-default" ) diff --git a/pkg/ipam/metal3io/ipam.go b/pkg/ipam/metal3io/ipam.go index c33d268..a13614a 100644 --- a/pkg/ipam/metal3io/ipam.go +++ b/pkg/ipam/metal3io/ipam.go @@ -130,26 +130,28 @@ func (m *Metal3IPAM) ReleaseIPs(ctx goctx.Context, owner metav1.Object, pool ipa func (m *Metal3IPAM) GetAvailableIPPool(ctx goctx.Context, poolMatchLabels map[string]string, clusterMeta metav1.ObjectMeta) (ipam.IPPool, error) { poolNamespace := getIPPoolNamespace(poolMatchLabels, clusterMeta) + poolName := poolMatchLabels[ipam.ClusterIPPoolNameKey] - // if the specific ip-pool name is provided use that to get the ip-pool - var ipPool ipamv1.IPPool - if poolName, ok := poolMatchLabels[ipam.ClusterIPPoolNameKey]; ok && poolName != "" { - if err := m.Get(ctx, apitypes.NamespacedName{ - Namespace: poolNamespace, - Name: poolName, - }, &ipPool); err != nil { - if apierrors.IsNotFound(err) { - return nil, nil - } - - return nil, errors.Wrapf(err, "failed to get IPPool %s/%s", poolNamespace, poolName) - } - - return toIPPool(ipPool), nil + // 1. If the specific ip-pool name is provided use that to get the ip-pool. + if poolName != "" { + return m.getIPPoolByName(ctx, poolNamespace, poolName) } - matchLabels := map[string]string{} + // 2. Otherwise use default ip-pool + + ipPoolList := &ipamv1.IPPoolList{} + if err := m.List( + ctx, + ipPoolList, + client.InNamespace(clusterMeta.Namespace), + client.MatchingLabels(map[string]string{ipam.DefaultIPPoolKey: "true"})); err != nil { + return nil, err + } + if len(ipPoolList.Items) > 0 { + return toIPPool(ipPoolList.Items[0]), nil + } + matchLabels := map[string]string{ipam.DefaultIPPoolKey: "true"} // use labels 'ip-pool-group' & 'network-name' to select the ip-pool if label, ok := poolMatchLabels[ipam.ClusterIPPoolGroupKey]; ok && label != "" { matchLabels[ipam.ClusterIPPoolGroupKey] = label @@ -158,30 +160,50 @@ func (m *Metal3IPAM) GetAvailableIPPool(ctx goctx.Context, poolMatchLabels map[s matchLabels[ipam.ClusterNetworkNameKey] = label } - // use default ip-pool - if len(matchLabels) == 0 { - poolNamespace = ipam.DefaultIPPoolNamespace - matchLabels[ipam.DefaultIPPoolKey] = "true" - } - - ipPoolList := &ipamv1.IPPoolList{} + ipPoolList = &ipamv1.IPPoolList{} if err := m.List( ctx, ipPoolList, - client.InNamespace(poolNamespace), + client.InNamespace(ipam.DefaultIPPoolNamespace), client.MatchingLabels(matchLabels)); err != nil { return nil, err } if len(ipPoolList.Items) == 0 { - m.logger.Info("failed to get a matching IPPool") return nil, nil } - ipPool = ipPoolList.Items[0] - m.logger.Info(fmt.Sprintf("IPPool %s is available", ipPool.Name)) + return toIPPool(ipPoolList.Items[0]), nil +} + +// getIPPoolByName returns the IPPool with the specified name. +// +// If IPPool is not found in the specified namespace, will try to find from the +// default namespace. +func (m *Metal3IPAM) getIPPoolByName(ctx goctx.Context, poolNamespace, poolName string) (ipam.IPPool, error) { + var ipPool ipamv1.IPPool + err := m.Get(ctx, apitypes.NamespacedName{ + Namespace: poolNamespace, + Name: poolName, + }, &ipPool) + if err == nil { + return toIPPool(ipPool), nil + } else if !apierrors.IsNotFound(err) { + return nil, errors.Wrapf(err, "failed to get IPPool %s/%s", poolNamespace, poolName) + } + + // Try the ip-pool default namespace. + err = m.Get(ctx, apitypes.NamespacedName{ + Namespace: ipam.DefaultIPPoolNamespace, + Name: poolName, + }, &ipPool) + if err == nil { + return toIPPool(ipPool), nil + } else if apierrors.IsNotFound(err) { + return nil, nil + } - return toIPPool(ipPool), nil + return nil, errors.Wrapf(err, "failed to get IPPool %s/%s", poolNamespace, poolName) } func (m *Metal3IPAM) getIPClaim(ctx goctx.Context, pool ipam.IPPool, claimName string) (*ipamv1.IPClaim, error) { From 7a77cc913061d78034a64438d03dac9aa5427ff2 Mon Sep 17 00:00:00 2001 From: haijianyang Date: Tue, 2 Apr 2024 02:03:53 -0400 Subject: [PATCH 2/3] test --- controllers/elfmachine_controller.go | 10 ++--- pkg/ipam/metal3io/ipam.go | 64 +++++++++++++++------------- 2 files changed, 39 insertions(+), 35 deletions(-) diff --git a/controllers/elfmachine_controller.go b/controllers/elfmachine_controller.go index 3f442ab..17d22ef 100644 --- a/controllers/elfmachine_controller.go +++ b/controllers/elfmachine_controller.go @@ -340,12 +340,12 @@ func (r *ElfMachineReconciler) reconcileDeviceIPAddress(ctx *context.MachineCont // // getIPPool selects IPPool according to the following priorities: // (1) Without ip-pool-name -// 1. select IPPool with `is-default`` label from elfMachine.Namespace -// 2. select IPPool with `is-default` label from default namespace +// 1. select IPPool with `is-default“ label from elfMachine.Namespace +// 2. select IPPool with `is-default` label from default namespace +// // (2) With ip-pool-name(from AddressesFromPools or ElfMachineTemplate) -// 1. select IPPool using the specified ip-pool-name and ip-pool-namespace -// 2. select the IPPool named ip-pool-name in the default namespace - +// 1. select IPPool using the specified ip-pool-name and ip-pool-namespace +// 2. select the IPPool named ip-pool-name in the default namespace func (r *ElfMachineReconciler) getIPPool(ctx *context.MachineContext, device capev1.NetworkDeviceSpec) (ipam.IPPool, error) { poolMatchLabels := make(map[string]string) // Prefer IPPool of device. Only Metal3 IPPool is supported now. diff --git a/pkg/ipam/metal3io/ipam.go b/pkg/ipam/metal3io/ipam.go index a13614a..4ea50b3 100644 --- a/pkg/ipam/metal3io/ipam.go +++ b/pkg/ipam/metal3io/ipam.go @@ -138,7 +138,41 @@ func (m *Metal3IPAM) GetAvailableIPPool(ctx goctx.Context, poolMatchLabels map[s } // 2. Otherwise use default ip-pool + return m.getIPPoolByLabels(ctx, poolMatchLabels, clusterMeta) +} + +// getIPPoolByName returns the IPPool with the specified name. +// +// If IPPool is not found in the specified namespace, will try to find from the +// default namespace. +func (m *Metal3IPAM) getIPPoolByName(ctx goctx.Context, poolNamespace, poolName string) (ipam.IPPool, error) { + var ipPool ipamv1.IPPool + err := m.Get(ctx, apitypes.NamespacedName{ + Namespace: poolNamespace, + Name: poolName, + }, &ipPool) + if err == nil { + return toIPPool(ipPool), nil + } else if !apierrors.IsNotFound(err) { + return nil, errors.Wrapf(err, "failed to get IPPool %s/%s", poolNamespace, poolName) + } + + // Try the ip-pool default namespace. + err = m.Get(ctx, apitypes.NamespacedName{ + Namespace: ipam.DefaultIPPoolNamespace, + Name: poolName, + }, &ipPool) + if err == nil { + return toIPPool(ipPool), nil + } else if apierrors.IsNotFound(err) { + return nil, nil + } + + return nil, errors.Wrapf(err, "failed to get IPPool %s/%s", poolNamespace, poolName) +} +// getIPPoolByLabels returns the IPPool with the specified labels. +func (m *Metal3IPAM) getIPPoolByLabels(ctx goctx.Context, poolMatchLabels map[string]string, clusterMeta metav1.ObjectMeta) (ipam.IPPool, error) { ipPoolList := &ipamv1.IPPoolList{} if err := m.List( ctx, @@ -176,36 +210,6 @@ func (m *Metal3IPAM) GetAvailableIPPool(ctx goctx.Context, poolMatchLabels map[s return toIPPool(ipPoolList.Items[0]), nil } -// getIPPoolByName returns the IPPool with the specified name. -// -// If IPPool is not found in the specified namespace, will try to find from the -// default namespace. -func (m *Metal3IPAM) getIPPoolByName(ctx goctx.Context, poolNamespace, poolName string) (ipam.IPPool, error) { - var ipPool ipamv1.IPPool - err := m.Get(ctx, apitypes.NamespacedName{ - Namespace: poolNamespace, - Name: poolName, - }, &ipPool) - if err == nil { - return toIPPool(ipPool), nil - } else if !apierrors.IsNotFound(err) { - return nil, errors.Wrapf(err, "failed to get IPPool %s/%s", poolNamespace, poolName) - } - - // Try the ip-pool default namespace. - err = m.Get(ctx, apitypes.NamespacedName{ - Namespace: ipam.DefaultIPPoolNamespace, - Name: poolName, - }, &ipPool) - if err == nil { - return toIPPool(ipPool), nil - } else if apierrors.IsNotFound(err) { - return nil, nil - } - - return nil, errors.Wrapf(err, "failed to get IPPool %s/%s", poolNamespace, poolName) -} - func (m *Metal3IPAM) getIPClaim(ctx goctx.Context, pool ipam.IPPool, claimName string) (*ipamv1.IPClaim, error) { var ipClaim ipamv1.IPClaim if err := m.Client.Get(ctx, apitypes.NamespacedName{ From b66f9b57f76c9efd0e4022d836db5646d9c85cde Mon Sep 17 00:00:00 2001 From: haijianyang Date: Tue, 9 Apr 2024 00:11:00 -0400 Subject: [PATCH 3/3] Add unit tests --- controllers/elfmachine_controller_test.go | 56 ++++++++++++++++++----- controllers/suite_test.go | 20 ++++++++ 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/controllers/elfmachine_controller_test.go b/controllers/elfmachine_controller_test.go index 3b87814..b1581b1 100644 --- a/controllers/elfmachine_controller_test.go +++ b/controllers/elfmachine_controller_test.go @@ -26,7 +26,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" capev1 "github.com/smartxworks/cluster-api-provider-elf/api/v1beta1" - capecontext "github.com/smartxworks/cluster-api-provider-elf/pkg/context" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -39,7 +38,6 @@ import ( ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/config" - "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/context" "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/ipam" "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/ipam/metal3io" ipamutil "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/ipam/util" @@ -347,26 +345,60 @@ var _ = Describe("ElfMachineReconciler", func() { } ctrlContext := newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate, metal3IPPool, metal3IPClaim, metal3IPAddress) fake.InitOwnerReferences(ctrlContext, elfCluster, cluster, elfMachine, machine) - machineContext := &context.MachineContext{ - IPAMService: metal3io.NewIpam(ctrlContext.Client, ctrlContext.Logger), - MachineContext: &capecontext.MachineContext{ - ControllerContext: ctrlContext, - Cluster: cluster, - Machine: machine, - ElfMachine: elfMachine, - Logger: ctrlContext.Logger, - }, - } + machineContext := newMachineContext(ctrlContext, metal3io.NewIpam(ctrlContext.Client, ctrlContext.Logger), cluster, machine, elfMachine) reconciler := &ElfMachineReconciler{ControllerContext: ctrlContext} ipPool, err := reconciler.getIPPool(machineContext, elfMachine.Spec.Network.Devices[0]) Expect(err).ToNot(HaveOccurred()) Expect(ipPool.GetNamespace()).To(Equal(metal3IPPool.Namespace)) Expect(ipPool.GetName()).To(Equal(metal3IPPool.Name)) + metal3IPPool.Namespace = ipam.DefaultIPPoolNamespace + ctrlContext = newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate, metal3IPPool, metal3IPClaim, metal3IPAddress) + fake.InitOwnerReferences(ctrlContext, elfCluster, cluster, elfMachine, machine) + machineContext = newMachineContext(ctrlContext, metal3io.NewIpam(ctrlContext.Client, ctrlContext.Logger), cluster, machine, elfMachine) + reconciler = &ElfMachineReconciler{ControllerContext: ctrlContext} + ipPool, err = reconciler.getIPPool(machineContext, elfMachine.Spec.Network.Devices[0]) + Expect(err).ToNot(HaveOccurred()) + Expect(ipPool.GetName()).To(Equal(metal3IPPool.Name)) + elfMachine.Spec.Network.Devices[0].AddressesFromPools[0].Name = "notfound" ipPool, err = reconciler.getIPPool(machineContext, elfMachine.Spec.Network.Devices[0]) Expect(err).ToNot(HaveOccurred()) Expect(ipPool).To(BeNil()) }) + + It("should get the default ip-pool", func() { + metal3IPPool.Namespace = cluster.Namespace + metal3IPPool.Labels = map[string]string{ipam.DefaultIPPoolKey: "true"} + ctrlContext := newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate, metal3IPPool, metal3IPClaim, metal3IPAddress) + fake.InitOwnerReferences(ctrlContext, elfCluster, cluster, elfMachine, machine) + machineContext := newMachineContext(ctrlContext, metal3io.NewIpam(ctrlContext.Client, ctrlContext.Logger), cluster, machine, elfMachine) + reconciler := &ElfMachineReconciler{ControllerContext: ctrlContext} + ipPool, err := reconciler.getIPPool(machineContext, elfMachine.Spec.Network.Devices[0]) + Expect(err).ToNot(HaveOccurred()) + Expect(ipPool.GetNamespace()).To(Equal(metal3IPPool.Namespace)) + Expect(ipPool.GetName()).To(Equal(metal3IPPool.Name)) + + metal3IPPool.Namespace = ipam.DefaultIPPoolNamespace + metal3IPPool.Labels = map[string]string{ipam.DefaultIPPoolKey: "true"} + ctrlContext = newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate, metal3IPPool, metal3IPClaim, metal3IPAddress) + fake.InitOwnerReferences(ctrlContext, elfCluster, cluster, elfMachine, machine) + machineContext = newMachineContext(ctrlContext, metal3io.NewIpam(ctrlContext.Client, ctrlContext.Logger), cluster, machine, elfMachine) + reconciler = &ElfMachineReconciler{ControllerContext: ctrlContext} + ipPool, err = reconciler.getIPPool(machineContext, elfMachine.Spec.Network.Devices[0]) + Expect(err).ToNot(HaveOccurred()) + Expect(ipPool.GetNamespace()).To(Equal(metal3IPPool.Namespace)) + Expect(ipPool.GetName()).To(Equal(metal3IPPool.Name)) + + metal3IPPool.Namespace = "notfoud" + metal3IPPool.Labels = map[string]string{ipam.DefaultIPPoolKey: "true"} + ctrlContext = newCtrlContexts(elfCluster, cluster, elfMachine, machine, elfMachineTemplate, metal3IPPool, metal3IPClaim, metal3IPAddress) + fake.InitOwnerReferences(ctrlContext, elfCluster, cluster, elfMachine, machine) + machineContext = newMachineContext(ctrlContext, metal3io.NewIpam(ctrlContext.Client, ctrlContext.Logger), cluster, machine, elfMachine) + reconciler = &ElfMachineReconciler{ControllerContext: ctrlContext} + ipPool, err = reconciler.getIPPool(machineContext, elfMachine.Spec.Network.Devices[0]) + Expect(err).ToNot(HaveOccurred()) + Expect(ipPool).To(BeNil()) + }) }) }) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index b2934e9..d7df0a8 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -25,12 +25,16 @@ import ( ipamv1 "github.com/metal3-io/ip-address-manager/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + capev1 "github.com/smartxworks/cluster-api-provider-elf/api/v1beta1" capecontext "github.com/smartxworks/cluster-api-provider-elf/pkg/context" "k8s.io/klog/v2" + capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" ctrllog "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/context" + "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/ipam" ipamutil "github.com/smartxworks/cluster-api-provider-elf-static-ip/pkg/ipam/util" "github.com/smartxworks/cluster-api-provider-elf-static-ip/test/fake" "github.com/smartxworks/cluster-api-provider-elf-static-ip/test/helpers" @@ -111,3 +115,19 @@ func setMetal3IPForClaim(ipClaim *ipamv1.IPClaim, ip *ipamv1.IPAddress) { ref := ipamutil.GetObjRef(ip) ipClaim.Status.Address = &ref } + +func newMachineContext( + ctrlContext *capecontext.ControllerContext, + ipamService ipam.IPAddressManager, + cluster *capiv1.Cluster, machine *capiv1.Machine, elfMachine *capev1.ElfMachine) *context.MachineContext { + return &context.MachineContext{ + IPAMService: ipamService, + MachineContext: &capecontext.MachineContext{ + ControllerContext: ctrlContext, + Cluster: cluster, + Machine: machine, + ElfMachine: elfMachine, + Logger: ctrlContext.Logger, + }, + } +}