Merge pull request #113 from leaseweb/lb_network_bug_fixes

Loadbalancer rule assignment fixes

Author: hrak

internal/controllers/cloudstackfailuredomain_controller.go

@@ -234,7 +234,7 @@ func (r *CloudStackFailureDomainReconciler) reconcileNormal(ctx context.Context,
 // GenerateIsolatedNetwork creates a CloudStackIsolatedNetwork object in the cluster that is owned by the CloudStackFailureDomain.
 func (r *CloudStackFailureDomainReconciler) GenerateIsolatedNetwork(ctx context.Context, scope *scope.FailureDomainScope) error {
 	lowerName := strings.ToLower(scope.NetworkName())
-	metaName := fmt.Sprintf("%s-%s", scope.KubernetesClusterName(), lowerName)
+	metaName := scope.IsolatedNetworkName()
 	ownerGVK := scope.OwnerGVK()
 	csIsoNet := &infrav1.CloudStackIsolatedNetwork{}
 	csIsoNet.ObjectMeta = metav1.ObjectMeta{

internal/controllers/cloudstackmachine_controller.go

@@ -898,7 +898,7 @@ func (r *CloudStackMachineReconciler) SetupWithManager(ctx context.Context, mgr
 						}
 
 						// Only trigger a CloudStackMachine reconcile if the loadbalancer rules changed.
-						return len(oldCSIsoNet.Status.LoadBalancerRuleIDs) != len(newCSIsoNet.Status.LoadBalancerRuleIDs)
+						return !cmp.Equal(oldCSIsoNet.Status.LoadBalancerRuleIDs, newCSIsoNet.Status.LoadBalancerRuleIDs)
 					},
 				},
 			),

pkg/cloud/isolated_network.go

@@ -778,8 +778,22 @@ func (c *client) ReconcileLoadBalancer(
 	return nil
 }
 
+// isLBRuleNotFound checks if an error from the CloudStack API indicates that a load balancer rule does not exist.
+func isLBRuleNotFound(err error) bool {
+	if err == nil {
+		return false
+	}
+	msg := strings.ToLower(err.Error())
+
+	return strings.Contains(msg, "no match found") ||
+		strings.Contains(msg, "unable to find") ||
+		strings.Contains(msg, "does not exist") ||
+		strings.Contains(msg, "entity does not exist")
+}
+
 // AssignVMToLoadBalancerRules assigns a VM to the load balancing rules listed in isoNet.Status.LoadBalancerRuleIDs,
 // if not already assigned. It returns a bool indicating whether an instance was actually assigned, and an error in case an error occurred.
+// If a load balancer rule no longer exists in CloudStack, it is skipped.
 func (c *client) AssignVMToLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) (bool, error) {
 	var assigned, found bool
 	assigned = false
@@ -790,6 +804,14 @@ func (c *client) AssignVMToLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedN
 		lbRuleInstances, err := c.cs.LoadBalancer.ListLoadBalancerRuleInstances(
 			c.cs.LoadBalancer.NewListLoadBalancerRuleInstancesParams(lbRuleID))
 		if err != nil {
+			if isLBRuleNotFound(err) {
+				// The load balancer rule no longer exists in CloudStack. Skip it; the
+				// isolated network controller will re-create it on its next reconciliation.
+				record.Warnf(isoNet, "LoadBalancerRuleNotFound",
+					"Load balancer rule %s no longer exists, skipping assignment for instance %s", lbRuleID, instanceID)
+
+				continue
+			}
 			c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
 
 			return false, err
@@ -822,13 +844,22 @@ func (c *client) AssignVMToLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedN
 
 // RemoveVMFromLoadBalancerRules removes a VM from the load balancing rules listed in isoNet.Status.LoadBalancerRuleIDs,
 // if not already removed. It returns a bool indicating whether an instance was actually removed, and an error in case an error occurred.
+// If a load balancer rule no longer exists in CloudStack, it is skipped (the VM is implicitly not assigned).
 func (c *client) RemoveVMFromLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork, instanceID string) (bool, error) {
 	for _, lbRuleID := range isoNet.Status.LoadBalancerRuleIDs {
 		// Check that the instance isn't already in LB rotation.
 		found := false
 		lbRuleInstances, err := c.cs.LoadBalancer.ListLoadBalancerRuleInstances(
 			c.cs.LoadBalancer.NewListLoadBalancerRuleInstancesParams(lbRuleID))
 		if err != nil {
+			if isLBRuleNotFound(err) {
+				// The load balancer rule no longer exists in CloudStack, so the VM
+				// is implicitly not assigned to it. Skip it.
+				record.Warnf(isoNet, "LoadBalancerRuleNotFound",
+					"Load balancer rule %s no longer exists, skipping removal for instance %s", lbRuleID, instanceID)
+
+				continue
+			}
 			c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
 
 			return false, err

pkg/cloud/isolated_network_test.go

@@ -534,6 +534,55 @@ var _ = Describe("Network", func() {
 			Ω(assigned).Should(BeFalse())
 			Ω(err).ShouldNot(HaveOccurred())
 		})
+
+		It("Skips non-existent LB rule during assignment", func() {
+			dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"stale-rule-id"}
+			lbip := &csapi.ListLoadBalancerRuleInstancesParams{}
+			lbs.EXPECT().NewListLoadBalancerRuleInstancesParams(dummies.CSISONet1.Status.LoadBalancerRuleIDs[0]).
+				Return(lbip)
+			lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(nil, errors.New("No match found for stale-rule-id"))
+
+			assigned, err := client.AssignVMToLoadBalancerRules(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)
+			Ω(assigned).Should(BeFalse())
+			Ω(err).ShouldNot(HaveOccurred())
+		})
+
+		It("Skips non-existent LB rule and assigns to remaining rules", func() {
+			dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"stale-rule-id", "valid-rule-id"}
+			lbip := &csapi.ListLoadBalancerRuleInstancesParams{}
+			albp := &csapi.AssignToLoadBalancerRuleParams{}
+			gomock.InOrder(
+				lbs.EXPECT().NewListLoadBalancerRuleInstancesParams("stale-rule-id").
+					Return(lbip),
+				lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).
+					Return(nil, errors.New("entity does not exist")),
+
+				lbs.EXPECT().NewListLoadBalancerRuleInstancesParams("valid-rule-id").
+					Return(lbip),
+				lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).
+					Return(&csapi.ListLoadBalancerRuleInstancesResponse{}, nil),
+				lbs.EXPECT().NewAssignToLoadBalancerRuleParams("valid-rule-id").Return(albp),
+				lbs.EXPECT().AssignToLoadBalancerRule(albp).Return(&csapi.AssignToLoadBalancerRuleResponse{Success: true}, nil),
+			)
+
+			assigned, err := client.AssignVMToLoadBalancerRules(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)
+			Ω(assigned).Should(BeTrue())
+			Ω(err).ShouldNot(HaveOccurred())
+		})
+	})
+
+	Context("Remove VM from Load Balancer rule", func() {
+		It("Skips non-existent LB rule during removal", func() {
+			dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"stale-rule-id"}
+			lbip := &csapi.ListLoadBalancerRuleInstancesParams{}
+			lbs.EXPECT().NewListLoadBalancerRuleInstancesParams("stale-rule-id").
+				Return(lbip)
+			lbs.EXPECT().ListLoadBalancerRuleInstances(lbip).Return(nil, errors.New("Unable to find uuid for id stale-rule-id"))
+
+			removed, err := client.RemoveVMFromLoadBalancerRules(dummies.CSISONet1, *dummies.CSMachine1.Spec.InstanceID)
+			Ω(removed).Should(BeFalse())
+			Ω(err).ShouldNot(HaveOccurred())
+		})
 	})
 
 	Context("load balancer rule does not exist", func() {

pkg/scope/machinescope.go

@@ -204,12 +204,14 @@ func (s *MachineScope) NetworkType() string {
 
 // IsolatedNetwork returns the isolated network of the machine.
 func (s *MachineScope) IsolatedNetwork(ctx context.Context) (*infrav1.CloudStackIsolatedNetwork, error) {
+	if s.CloudStackIsolatedNetwork != nil && s.CloudStackIsolatedNetwork.Name != "" {
+		return s.CloudStackIsolatedNetwork, nil
+	}
+
 	isonet := &infrav1.CloudStackIsolatedNetwork{}
-	if s.CloudStackIsolatedNetwork == nil || s.CloudStackIsolatedNetwork.Name == "" {
-		err := s.client.Get(ctx, client.ObjectKey{Name: s.IsolatedNetworkName(), Namespace: s.Namespace()}, isonet)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to get isolated network with name %s", s.IsolatedNetworkName())
-		}
+	err := s.client.Get(ctx, client.ObjectKey{Name: s.IsolatedNetworkName(), Namespace: s.Namespace()}, isonet)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get isolated network with name %s", s.IsolatedNetworkName())
 	}
 
 	return isonet, nil