🏃 Refactor/cleanup load balancer related code

docs/mover.md

@@ -3,9 +3,8 @@
 **Table of Contents**  *generated with [DocToc](https://github.com/thlorenz/doctoc)*
 
 - [Pre-condition](#pre-condition)
-- [Install openstack providers into target cluster](#install-openstack-providers-into-target-cluster)
-- [Move objects in `bootstrap` cluster into `target` cluster.](#move-objects-in-bootstrap-cluster-into-target-cluster)
-- [Create secret in `target` cluster](#create-secret-in-target-cluster)
+- [Install OpenStack Cluster API provider into target cluster](#install-openstack-cluster-api-provider-into-target-cluster)
+- [Move objects from `bootstrap` cluster into `target` cluster.](#move-objects-from-bootstrap-cluster-into-target-cluster)
 - [Check cluster status](#check-cluster-status)
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->

pkg/cloud/services/compute/instance.go

@@ -142,10 +142,9 @@ func (s *Service) InstanceCreate(openStackCluster *infrav1.OpenStackCluster, mac
 
 	out, err := createInstance(s, clusterName, input)
 	if err != nil {
-		record.Warnf(openStackMachine, "FailedCreateServer", "Failed to create server %s: %v", out.Name, err)
+		record.Warnf(openStackMachine, "FailedCreateServer", "Failed to create server %s: %v", input.Name, err)
 		return nil, err
 	}
-
 	record.Eventf(openStackMachine, "SuccessfulCreateServer", "Created server %s with id %s", out.Name, out.ID)
 	return out, nil
 }
@@ -158,16 +157,16 @@ func createInstance(is *Service, clusterName string, i *infrav1.Instance) (*infr
 	}
 
 	accessIPv4 := ""
-	nets := i.Networks
+	networkList := i.Networks
 	portsList := []servers.Network{}
-	for _, net := range *nets {
-		net := net
-		if net.ID == "" {
+	for _, network := range *networkList {
+		network := network
+		if network.ID == "" {
 			return nil, fmt.Errorf("no network was found or provided. Please check your machine configuration and try again")
 		}
 		allPages, err := ports.List(is.networkClient, ports.ListOpts{
 			Name:      i.Name,
-			NetworkID: net.ID,
+			NetworkID: network.ID,
 		}).AllPages()
 		if err != nil {
 			return nil, fmt.Errorf("searching for existing port for server: %v", err)
@@ -179,7 +178,7 @@ func createInstance(is *Service, clusterName string, i *infrav1.Instance) (*infr
 		var port ports.Port
 		if len(portList) == 0 {
 			// create server port
-			port, err = createPort(is, clusterName, i.Name, &net, i.SecurityGroups)
+			port, err = createPort(is, clusterName, i.Name, &network, i.SecurityGroups)
 			if err != nil {
 				return nil, fmt.Errorf("failed to create port err: %v", err)
 			}
@@ -331,12 +330,12 @@ func GetIPFromInstance(v servers.Server) (map[string]string, error) {
 		if err != nil {
 			return nil, fmt.Errorf("extract IP from instance err: %v", err)
 		}
-		var networks []interface{}
-		err = json.Unmarshal(list, &networks)
+		var networkList []interface{}
+		err = json.Unmarshal(list, &networkList)
 		if err != nil {
 			return nil, fmt.Errorf("extract IP from instance err: %v", err)
 		}
-		for _, network := range networks {
+		for _, network := range networkList {
 			var netInterface networkInterface
 			b, _ := json.Marshal(network)
 			err = json.Unmarshal(b, &netInterface)
@@ -444,22 +443,22 @@ func getSecurityGroups(is *Service, securityGroupParams []infrav1.SecurityGroupP
 
 func getServerNetworks(networkClient *gophercloud.ServiceClient, networkParams []infrav1.NetworkParam) ([]infrav1.Network, error) {
 	var nets []infrav1.Network
-	for _, net := range networkParams {
-		opts := networks.ListOpts(net.Filter)
-		opts.ID = net.UUID
+	for _, networkParam := range networkParams {
+		opts := networks.ListOpts(networkParam.Filter)
+		opts.ID = networkParam.UUID
 		ids, err := networking.GetNetworkIDsByFilter(networkClient, &opts)
 		if err != nil {
 			return nil, err
 		}
 		for _, netID := range ids {
-			if net.Subnets == nil {
+			if networkParam.Subnets == nil {
 				nets = append(nets, infrav1.Network{
 					ID: netID,
 				})
 				continue
 			}
 
-			for _, subnet := range net.Subnets {
+			for _, subnet := range networkParam.Subnets {
 				subnetOpts := subnets.ListOpts(subnet.Filter)
 				subnetOpts.ID = subnet.UUID
 				subnetOpts.NetworkID = netID

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -42,15 +42,15 @@ const (
 
 func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackCluster, clusterName string) error {
 	loadBalancerName := getLoadBalancerName(clusterName)
-	s.logger.Info("Reconciling loadbalancer", "name", loadBalancerName)
+	s.logger.Info("Reconciling load balancer", "name", loadBalancerName)
 
 	// lb
 	lb, err := checkIfLbExists(s.loadbalancerClient, loadBalancerName)
 	if err != nil {
 		return err
 	}
 	if lb == nil {
-		s.logger.Info("Creating loadbalancer", "name", loadBalancerName)
+		s.logger.Info("Creating load balancer", "name", loadBalancerName)
 		lbCreateOpts := loadbalancers.CreateOpts{
 			Name:        loadBalancerName,
 			VipSubnetID: openStackCluster.Status.Network.Subnet.ID,
@@ -67,11 +67,11 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
 		return err
 	}
 
-	fip := openStackCluster.Spec.ControlPlaneEndpoint.Host
+	floatingIPAddress := openStackCluster.Spec.ControlPlaneEndpoint.Host
 	if openStackCluster.Spec.APIServerFloatingIP != "" {
-		fip = openStackCluster.Spec.APIServerFloatingIP
+		floatingIPAddress = openStackCluster.Spec.APIServerFloatingIP
 	}
-	fp, err := s.networkingService.GetOrCreateFloatingIP(openStackCluster, fip)
+	fp, err := s.networkingService.GetOrCreateFloatingIP(openStackCluster, floatingIPAddress)
 	if err != nil {
 		return err
 	}
@@ -90,7 +90,7 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
 			return err
 		}
 		if listener == nil {
-			s.logger.Info("Creating lb listener", "name", lbPortObjectsName)
+			s.logger.Info("Creating load balancer listener", "name", lbPortObjectsName)
 			listenerCreateOpts := listeners.CreateOpts{
 				Name:           lbPortObjectsName,
 				Protocol:       "TCP",
@@ -116,7 +116,7 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
 			return err
 		}
 		if pool == nil {
-			s.logger.Info("Creating lb pool", "name", lbPortObjectsName)
+			s.logger.Info("Creating load balancer pool", "name", lbPortObjectsName)
 			poolCreateOpts := pools.CreateOpts{
 				Name:       lbPortObjectsName,
 				Protocol:   "TCP",
@@ -138,7 +138,7 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
 			return err
 		}
 		if monitor == nil {
-			s.logger.Info("Creating lb monitor", "name", lbPortObjectsName)
+			s.logger.Info("Creating load balancer monitor", "name", lbPortObjectsName)
 			monitorCreateOpts := monitors.CreateOpts{
 				Name:       lbPortObjectsName,
 				PoolID:     pool.ID,
@@ -182,7 +182,7 @@ func (s *Service) ReconcileLoadBalancerMember(openStackCluster *infrav1.OpenStac
 	}
 
 	loadBalancerName := getLoadBalancerName(clusterName)
-	s.logger.Info("Reconciling loadbalancer", "name", loadBalancerName)
+	s.logger.Info("Reconciling load balancer", "name", loadBalancerName)
 
 	lbID := openStackCluster.Status.Network.APIServerLoadBalancer.ID
 	portList := []int{int(openStackCluster.Spec.ControlPlaneEndpoint.Port)}
@@ -196,7 +196,7 @@ func (s *Service) ReconcileLoadBalancerMember(openStackCluster *infrav1.OpenStac
 			return err
 		}
 		if pool == nil {
-			return errors.New("loadbalancer pool does not exist yet")
+			return errors.New("load balancer pool does not exist yet")
 		}
 
 		lbMember, err := checkIfLbMemberExists(s.loadbalancerClient, pool.ID, name)
@@ -211,7 +211,7 @@ func (s *Service) ReconcileLoadBalancerMember(openStackCluster *infrav1.OpenStac
 				return nil
 			}
 
-			s.logger.Info("Deleting lb member (because the IP of the machine changed)", "name", name)
+			s.logger.Info("Deleting load balancer member (because the IP of the machine changed)", "name", name)
 
 			// lb member changed so let's delete it so we can create it again with the correct IP
 			err = waitForLoadBalancerActive(s.logger, s.loadbalancerClient, lbID)
@@ -228,7 +228,7 @@ func (s *Service) ReconcileLoadBalancerMember(openStackCluster *infrav1.OpenStac
 			}
 		}
 
-		s.logger.Info("Creating lb member", "name", name)
+		s.logger.Info("Creating load balancer member", "name", name)
 
 		// if we got to this point we should either create or re-create the lb member
 		lbMemberOpts := pools.CreateMemberOpts{
@@ -263,7 +263,7 @@ func (s *Service) DeleteLoadBalancer(openStackCluster *infrav1.OpenStackCluster,
 	deleteOpts := loadbalancers.DeleteOpts{
 		Cascade: true,
 	}
-	s.logger.Info("Deleting loadbalancer", "name", loadBalancerName)
+	s.logger.Info("Deleting load balancer", "name", loadBalancerName, "cascade", deleteOpts.Cascade)
 	err = loadbalancers.Delete(s.loadbalancerClient, lb.ID, deleteOpts).ExtractErr()
 	if err != nil {
 		record.Warnf(openStackCluster, "FailedDeleteLoadBalancer", "Failed to delete load balancer %s with id %s: %v", lb.Name, lb.ID, err)
@@ -280,7 +280,7 @@ func (s *Service) DeleteLoadBalancerMember(openStackCluster *infrav1.OpenStackCl
 	}
 
 	loadBalancerName := getLoadBalancerName(clusterName)
-	s.logger.Info("Reconciling loadbalancer", "name", loadBalancerName)
+	s.logger.Info("Reconciling load balancer", "name", loadBalancerName)
 
 	lbID := openStackCluster.Status.Network.APIServerLoadBalancer.ID
 
@@ -295,7 +295,7 @@ func (s *Service) DeleteLoadBalancerMember(openStackCluster *infrav1.OpenStackCl
 			return err
 		}
 		if pool == nil {
-			s.logger.Info("Pool does not exist", "name", lbPortObjectsName)
+			s.logger.Info("Load balancer pool does not exist", "name", lbPortObjectsName)
 			continue
 		}
 
@@ -313,7 +313,7 @@ func (s *Service) DeleteLoadBalancerMember(openStackCluster *infrav1.OpenStackCl
 			}
 			err = pools.DeleteMember(s.loadbalancerClient, pool.ID, lbMember.ID).ExtractErr()
 			if err != nil {
-				return fmt.Errorf("error deleting lbmember: %s", err)
+				return fmt.Errorf("error deleting load balancer member: %s", err)
 			}
 			err = waitForLoadBalancerActive(s.logger, s.loadbalancerClient, lbID)
 			if err != nil {
@@ -412,7 +412,7 @@ var backoff = wait.Backoff{
 
 // Possible LoadBalancer states are documented here: https://developer.openstack.org/api-ref/network/v2/?expanded=show-load-balancer-status-tree-detail#load-balancer-statuses
 func waitForLoadBalancerActive(logger logr.Logger, client *gophercloud.ServiceClient, id string) error {
-	logger.Info("Waiting for loadbalancer", "id", id, "targetStatus", "ACTIVE")
+	logger.Info("Waiting for load balancer", "id", id, "targetStatus", "ACTIVE")
 	return wait.ExponentialBackoff(backoff, func() (bool, error) {
 		lb, err := loadbalancers.Get(client, id).Extract()
 		if err != nil {
@@ -423,7 +423,7 @@ func waitForLoadBalancerActive(logger logr.Logger, client *gophercloud.ServiceCl
 }
 
 func waitForListener(logger logr.Logger, client *gophercloud.ServiceClient, id, target string) error {
-	logger.Info("Waiting for listener", "id", id, "targetStatus", target)
+	logger.Info("Waiting for load balancer listener", "id", id, "targetStatus", target)
 	return wait.ExponentialBackoff(backoff, func() (bool, error) {
 		_, err := listeners.Get(client, id).Extract()
 		if err != nil {

pkg/cloud/services/loadbalancer/service.go

@@ -40,7 +40,7 @@ func NewService(client *gophercloud.ProviderClient, clientOpts *clientconfig.Cli
 		Region: clientOpts.RegionName,
 	})
 	if err != nil {
-		return nil, fmt.Errorf("failed to create loadbalancer service client: %v", err)
+		return nil, fmt.Errorf("failed to create load balancer service client: %v", err)
 	}
 
 	networkingService, err := networking.NewService(client, clientOpts, logger)

pkg/cloud/services/networking/floatingip.go

@@ -30,28 +30,29 @@ import (
 func (s *Service) GetOrCreateFloatingIP(openStackCluster *infrav1.OpenStackCluster, ip string) (*floatingips.FloatingIP, error) {
 	var fp *floatingips.FloatingIP
 	var err error
+	var fpCreateOpts floatingips.CreateOpts
+
 	if ip != "" {
 		fp, err = checkIfFloatingIPExists(s.client, ip)
 		if err != nil {
 			return nil, err
 		}
-	}
-	if fp == nil {
-		fpCreateOpts := &floatingips.CreateOpts{
-			FloatingNetworkID: openStackCluster.Status.ExternalNetwork.ID,
-		}
-		if ip != "" {
-			// only admin can add ip address
-			fpCreateOpts.FloatingIP = ip
-		}
-		fp, err = floatingips.Create(s.client, fpCreateOpts).Extract()
-		if err != nil {
-			record.Warnf(openStackCluster, "FailedCreateFloatingIP", "Failed to create floating IP %s: %v", fp.FloatingIP, err)
-			return nil, err
+		if fp != nil {
+			return fp, nil
 		}
-		record.Eventf(openStackCluster, "SuccessfulCreateFloatingIP", "Created floating IP %s with id %s", fp.FloatingIP, fp.ID)
+		// only admin can add ip address
+		fpCreateOpts.FloatingIP = ip
+	}
+
+	fpCreateOpts.FloatingNetworkID = openStackCluster.Status.ExternalNetwork.ID
 
+	fp, err = floatingips.Create(s.client, fpCreateOpts).Extract()
+	if err != nil {
+		record.Warnf(openStackCluster, "FailedCreateFloatingIP", "Failed to create floating IP %s: %v", ip, err)
+		return nil, err
 	}
+
+	record.Eventf(openStackCluster, "SuccessfulCreateFloatingIP", "Created floating IP %s with id %s", fp.FloatingIP, fp.ID)
 	return fp, nil
 }
 
@@ -93,18 +94,18 @@ var backoff = wait.Backoff{
 }
 
 func (s *Service) AssociateFloatingIP(openStackCluster *infrav1.OpenStackCluster, fp *floatingips.FloatingIP, portID string) error {
-	s.logger.Info("Associating floating IP", "IP", fp.FloatingIP)
+	s.logger.Info("Associating floating IP", "id", fp.ID, "ip", fp.FloatingIP)
 	fpUpdateOpts := &floatingips.UpdateOpts{
 		PortID: &portID,
 	}
-	fp, err := floatingips.Update(s.client, fp.ID, fpUpdateOpts).Extract()
+	_, err := floatingips.Update(s.client, fp.ID, fpUpdateOpts).Extract()
 	if err != nil {
 		record.Warnf(openStackCluster, "FailedAssociateFloatingIP", "Failed to associate floating IP %s with port %s: %v", fp.FloatingIP, portID, err)
 		return err
 	}
 	record.Eventf(openStackCluster, "SuccessfulAssociateFloatingIP", "Associated floating IP %s with port %s", fp.FloatingIP, portID)
 
-	s.logger.Info("Waiting for floatingIP", "id", fp.ID, "targetStatus", "ACTIVE")
+	s.logger.Info("Waiting for floating IP", "id", fp.ID, "targetStatus", "ACTIVE")
 
 	return wait.ExponentialBackoff(backoff, func() (bool, error) {
 		fp, err := floatingips.Get(s.client, fp.ID).Extract()

test/e2e/suites/e2e/e2e_test.go

@@ -165,7 +165,7 @@ var _ = Describe("e2e tests", func() {
 			configCluster.Flavor = shared.FlavorDefault
 			_ = createCluster(ctx, configCluster, specName)
 
-			shared.Byf("Creating Machine Deployment with invalid subnet ID")
+			shared.Byf("Creating Machine Deployment with invalid subnet id")
 			md1Name := clusterName + "-md-1"
 			framework.CreateMachineDeployment(ctx, framework.CreateMachineDeploymentInput{
 				Creator:                 e2eCtx.Environment.BootstrapClusterProxy.GetClient(),