fix(failuredomain): Set a has if none set

Author: thunderboltsid

pkg/controllers/failuredomainrollout/controller.go

@@ -52,31 +52,40 @@ func (r *Reconciler) SetupWithManager(
 
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	logger := ctrl.LoggerFrom(ctx).WithValues("cluster", req.NamespacedName)
+	logger.Info("Starting failure domain rollout reconciliation")
 
 	var cluster clusterv1.Cluster
 	if err := r.Get(ctx, req.NamespacedName, &cluster); err != nil {
 		if apierrors.IsNotFound(err) {
-			logger.V(5).Info("Cluster not found, skipping reconciliation")
+			logger.Info("Cluster not found, skipping reconciliation")
 			return ctrl.Result{}, nil
 		}
 		return ctrl.Result{}, fmt.Errorf("failed to get Cluster %s: %w", req.NamespacedName, err)
 	}
 
+	logger.Info(
+		"Successfully retrieved cluster",
+		"hasTopology",
+		cluster.Spec.Topology != nil,
+		"failureDomainCount",
+		len(cluster.Status.FailureDomains),
+	)
+
 	// Skip if cluster is not using topology
 	if cluster.Spec.Topology == nil {
-		logger.V(5).Info("Cluster is not using topology, skipping reconciliation")
+		logger.Info("Cluster is not using topology, skipping reconciliation")
 		return ctrl.Result{}, nil
 	}
 
 	// Skip if cluster doesn't have a control plane reference
 	if cluster.Spec.ControlPlaneRef == nil {
-		logger.V(5).Info("Cluster has no control plane reference, skipping reconciliation")
+		logger.Info("Cluster has no control plane reference, skipping reconciliation")
 		return ctrl.Result{}, nil
 	}
 
 	// Skip if cluster doesn't have failure domains
 	if len(cluster.Status.FailureDomains) == 0 {
-		logger.V(5).Info("Cluster has no failure domains, skipping reconciliation")
+		logger.Info("Cluster has no failure domains, skipping reconciliation")
 		return ctrl.Result{}, nil
 	}
 
@@ -88,13 +97,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}
 	if err := r.Get(ctx, kcpKey, &kcp); err != nil {
 		if apierrors.IsNotFound(err) {
-			logger.V(5).Info("KubeAdmControlPlane not found, skipping reconciliation")
+			logger.Info("KubeAdmControlPlane not found, skipping reconciliation")
 			return ctrl.Result{}, nil
 		}
 		return ctrl.Result{}, fmt.Errorf("failed to get KubeAdmControlPlane %s: %w", kcpKey, err)
 	}
 
+	logger.Info("Successfully retrieved KubeAdmControlPlane", "kcpName", kcp.Name, "replicas", kcp.Spec.Replicas)
+
 	// Check if we need to trigger a rollout
+	logger.Info("Evaluating rollout necessity")
 	needsRollout, reason, err := r.shouldTriggerRollout(ctx, &cluster, &kcp)
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to determine if rollout is needed: %w", err)
@@ -117,14 +129,36 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		// Store the current failure domain hash
 		fdHash := r.calculateFailureDomainHash(cluster.Status.FailureDomains)
 		kcpCopy.Annotations[FailureDomainHashAnnotation] = fdHash
+		logger.Info("Updating KubeAdmControlPlane with rollout trigger", "failureDomainHash", fdHash)
 
 		if err := r.Update(ctx, kcpCopy); err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to update KubeAdmControlPlane %s: %w", kcpKey, err)
 		}
 
 		logger.Info("Successfully triggered rollout", "rolloutAfter", now.Format(time.RFC3339))
+	} else {
+		logger.Info("No rollout needed", "reason", "failure domain configuration is stable")
+
+		// Check if we need to store the hash for the first time (when no previous hash exists)
+		currentHash := r.calculateFailureDomainHash(cluster.Status.FailureDomains)
+		_, hasStoredHash := kcp.Annotations[FailureDomainHashAnnotation]
+
+		if !hasStoredHash {
+			logger.Info("Storing initial failure domain hash", "failureDomainHash", currentHash)
+			kcpCopy := kcp.DeepCopy()
+			if kcpCopy.Annotations == nil {
+				kcpCopy.Annotations = make(map[string]string)
+			}
+			kcpCopy.Annotations[FailureDomainHashAnnotation] = currentHash
+
+			if err := r.Update(ctx, kcpCopy); err != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to store initial failure domain hash %s: %w", kcpKey, err)
+			}
+			logger.Info("Successfully stored initial failure domain hash")
+		}
 	}
 
+	logger.Info("Completed failure domain rollout reconciliation")
 	return ctrl.Result{}, nil
 }
 
@@ -138,54 +172,71 @@ func (r *Reconciler) shouldTriggerRollout(
 
 	// Calculate current failure domain hash
 	currentFDHash := r.calculateFailureDomainHash(cluster.Status.FailureDomains)
+	logger.Info("Calculated current failure domain hash", "hash", currentFDHash)
 
 	// Get the stored hash from annotations
 	storedFDHash, hasStoredHash := kcp.Annotations[FailureDomainHashAnnotation]
+	logger.Info("Retrieved stored failure domain hash", "hash", storedFDHash, "hasStoredHash", hasStoredHash)
 
 	// If no stored hash, this is the first time - store the hash but don't trigger rollout
 	if !hasStoredHash {
-		logger.V(5).Info("No previous failure domain hash found, storing current hash")
+		logger.Info("No previous failure domain hash found, storing current hash")
 		return false, "", nil
 	}
 
 	// If hashes are the same, no changes detected
 	if currentFDHash == storedFDHash {
-		logger.V(5).Info("No failure domain changes detected")
+		logger.Info("No failure domain changes detected")
 		return false, "", nil
 	}
 
+	logger.Info(
+		"Failure domain hash changed, analyzing impact",
+		"currentHash",
+		currentFDHash,
+		"storedHash",
+		storedFDHash,
+	)
+
 	// Get the current KCP machines to understand current placement
 	currentlyUsedFailureDomains, err := r.getCurrentlyUsedFailureDomains(ctx, cluster)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to get currently used failure domains: %w", err)
 	}
 
-	logger.V(5).Info("Analyzing failure domain changes",
+	logger.Info("Analyzing failure domain changes",
 		"currentlyUsedFailureDomains", currentlyUsedFailureDomains,
 		"availableFailureDomains", getAvailableFailureDomains(cluster.Status.FailureDomains))
 
 	// Check if any currently used failure domain is disabled or removed
+	logger.Info("Checking if used failure domains are still valid")
 	for _, usedFD := range currentlyUsedFailureDomains {
 		if fd, exists := cluster.Status.FailureDomains[usedFD]; !exists {
+			logger.Info("Found removed failure domain in use", "failureDomain", usedFD)
 			return true, fmt.Sprintf("failure domain %s is removed", usedFD), nil
 		} else if !fd.ControlPlane {
+			logger.Info("Found disabled failure domain in use", "failureDomain", usedFD)
 			return true, fmt.Sprintf("failure domain %s is disabled for control plane", usedFD), nil
 		}
 	}
+	logger.Info("All used failure domains are still valid")
 
 	// Check if the distribution could be improved
 	availableFDs := getAvailableFailureDomains(cluster.Status.FailureDomains)
+	logger.Info("Checking if distribution can be improved", "availableFailureDomains", availableFDs)
 	shouldImprove, err := r.shouldImproveDistribution(ctx, cluster, kcp, availableFDs)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to check distribution improvement: %w", err)
 	}
 	if shouldImprove {
+		logger.Info("Distribution can be improved, triggering rollout")
 		return true, "failure domain distribution could be improved for better fault tolerance", nil
 	}
+	logger.Info("Distribution is already optimal, no improvement needed")
 
 	// If we reach here, failure domains changed but no meaningful impact
 	// (e.g., new failure domains added but existing ones still valid)
-	logger.V(5).Info("Failure domains changed but no meaningful impact detected")
+	logger.Info("Failure domains changed but no meaningful impact detected")
 	return false, "", nil
 }
 
@@ -200,11 +251,13 @@ func (r *Reconciler) shouldImproveDistribution(
 
 	// Skip if no replicas specified
 	if kcp.Spec.Replicas == nil {
+		logger.Info("No replicas specified, skipping distribution analysis")
 		return false, nil
 	}
 
 	replicas := int(*kcp.Spec.Replicas)
 	availableCount := len(availableFDs)
+	logger.Info("Starting distribution analysis", "replicas", replicas, "availableFailureDomains", availableCount)
 
 	// Get actual machine distribution
 	var machines clusterv1.MachineList
@@ -224,12 +277,27 @@ func (r *Reconciler) shouldImproveDistribution(
 	}
 
 	currentUsedCount := len(currentDistribution)
+	logger.Info(
+		"Current failure domain distribution",
+		"distribution",
+		currentDistribution,
+		"usedFailureDomains",
+		currentUsedCount,
+	)
 
 	// If we have more available FDs than we're currently using, we might be able to improve
 	if availableCount > currentUsedCount {
+		logger.Info("More failure domains available than currently used, analyzing improvement potential")
 		// Calculate ideal distribution: spread replicas as evenly as possible
 		baseReplicasPerFD := replicas / availableCount
 		extraReplicas := replicas % availableCount
+		logger.Info(
+			"Calculated ideal distribution",
+			"baseReplicasPerFD",
+			baseReplicasPerFD,
+			"extraReplicas",
+			extraReplicas,
+		)
 
 		// Check if current distribution is suboptimal
 		// Ideal: some FDs have (baseReplicasPerFD + 1), others have baseReplicasPerFD
@@ -241,7 +309,7 @@ func (r *Reconciler) shouldImproveDistribution(
 		// Check if any FD has more than the ideal maximum
 		for fd, count := range currentDistribution {
 			if count > maxIdealPerFD {
-				logger.V(5).Info("Found suboptimal distribution",
+				logger.Info("Found suboptimal distribution",
 					"failureDomain", fd,
 					"currentCount", count,
 					"maxIdeal", maxIdealPerFD,
@@ -300,7 +368,7 @@ func (r *Reconciler) shouldImproveDistribution(
 		if canImprove {
 			for _, fd := range availableFDs {
 				if _, used := currentDistribution[fd]; !used {
-					logger.V(5).Info("Found unused failure domain that could improve distribution",
+					logger.Info("Found unused failure domain that could improve distribution",
 						"unusedFailureDomain", fd,
 						"currentDistribution", currentDistribution,
 						"minCount", minCount,
@@ -312,8 +380,11 @@ func (r *Reconciler) shouldImproveDistribution(
 				}
 			}
 		}
+	} else {
+		logger.Info("Using all available failure domains or distribution cannot be improved")
 	}
 
+	logger.Info("No distribution improvement possible")
 	return false, nil
 }
 
@@ -322,6 +393,8 @@ func (r *Reconciler) getCurrentlyUsedFailureDomains(
 	ctx context.Context,
 	cluster *clusterv1.Cluster,
 ) ([]string, error) {
+	logger := ctrl.LoggerFrom(ctx).WithValues("cluster", client.ObjectKeyFromObject(cluster))
+
 	var machines clusterv1.MachineList
 	if err := r.List(ctx, &machines, client.InNamespace(cluster.Namespace), client.MatchingLabels{
 		clusterv1.ClusterNameLabel:         cluster.Name,
@@ -330,6 +403,8 @@ func (r *Reconciler) getCurrentlyUsedFailureDomains(
 		return nil, fmt.Errorf("failed to list control plane machines: %w", err)
 	}
 
+	logger.Info("Retrieved control plane machines", "machineCount", len(machines.Items))
+
 	usedFailureDomains := make(map[string]struct{})
 	for _, machine := range machines.Items {
 		if machine.Spec.FailureDomain != nil {
@@ -341,6 +416,7 @@ func (r *Reconciler) getCurrentlyUsedFailureDomains(
 	for fd := range usedFailureDomains {
 		result = append(result, fd)
 	}
+	logger.Info("Identified currently used failure domains", "usedFailureDomains", result)
 	return result, nil
 }
 

pkg/controllers/failuredomainrollout/doc.go

@@ -15,6 +15,6 @@
 //
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters,verbs=get;list;watch
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters/status,verbs=get
-// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=kubeadmcontrolplanes,verbs=get;list;watch;update;patch
+// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=kubeadmcontrolplanes,verbs=get;list;watch;update;patch
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines,verbs=get;list;watch
 package failuredomainrollout