OCPBUGS-25599: Reconcile AgentMachines for unbound Agents (#95)

AgentMachines need to be reconciled if they don't yet have an Agent, and
and a valid Agent is modified. Otherwise AgentMachines that don't yet
have an Agent will not be reconciled when an Agent becomes available.

Author: avishayt

controllers/agentmachine_controller.go

@@ -340,7 +340,7 @@ func (r *AgentMachineReconciler) findAgent(ctx context.Context, log logrus.Field
 
 	// Find an agent that is unbound and whose validations pass
 	for i := 0; i < len(agents.Items) && foundAgent == nil; i++ {
-		if isValidAgent(&agents.Items[i], agentMachine) {
+		if isValidAgent(&agents.Items[i]) {
 			foundAgent = &agents.Items[i]
 			log.Infof("Found agent to associate with AgentMachine: %s/%s", foundAgent.Namespace, foundAgent.Name)
 			err = r.updateFoundAgent(ctx, log, agentMachine, foundAgent, clusterDeploymentRef, machineConfigPool, ignitionTokenSecretRef)
@@ -511,7 +511,7 @@ func (r *AgentMachineReconciler) processBootstrapDataSecret(ctx context.Context,
 	return machineConfigPool, ignitionTokenSecretRef, nil
 }
 
-func isValidAgent(agent *aiv1beta1.Agent, agentMachine *capiproviderv1.AgentMachine) bool {
+func isValidAgent(agent *aiv1beta1.Agent) bool {
 	if !agent.Spec.Approved {
 		return false
 	}
@@ -675,31 +675,33 @@ func (r *AgentMachineReconciler) mapMachineToAgentMachine(ctx context.Context, m
 	return []reconcile.Request{}
 }
 
-// SetupWithManager sets up the controller with the Manager.
-func (r *AgentMachineReconciler) SetupWithManager(mgr ctrl.Manager, agentNamespace string) error {
-	mapAgentToAgentMachine := func(ctx context.Context, agent client.Object) []reconcile.Request {
-		log := r.Log.WithFields(
-			logrus.Fields{
-				"agent":           agent.GetName(),
-				"agent_namespace": agent.GetNamespace(),
-			})
+func (r *AgentMachineReconciler) mapAgentToAgentMachine(ctx context.Context, a client.Object) []reconcile.Request {
+	log := r.Log.WithFields(
+		logrus.Fields{
+			"agent":           a.GetName(),
+			"agent_namespace": a.GetNamespace(),
+		})
 
-		namespace, ok := agent.GetAnnotations()[AgentMachineRefNamespace]
-		if !ok {
-			return make([]reconcile.Request, 0)
-		}
+	amList := &capiproviderv1.AgentMachineList{}
+	opts := &client.ListOptions{}
+	mappedAgent := false
 
-		amList := &capiproviderv1.AgentMachineList{}
-		opts := &client.ListOptions{
-			Namespace: namespace,
-		}
+	// The annotation will only be present on an Agent that's mapped to an AgentMachine
+	namespace, ok := a.GetAnnotations()[AgentMachineRefNamespace]
+	if ok {
+		opts.Namespace = namespace
+		mappedAgent = true
+	}
 
-		if err := r.List(ctx, amList, opts); err != nil {
-			log.WithError(err).Error("failed to list agent machines")
-			return []reconcile.Request{}
-		}
+	if err := r.List(ctx, amList, opts); err != nil {
+		log.WithError(err).Error("failed to list agent machines")
+		return []reconcile.Request{}
+	}
 
-		reply := make([]reconcile.Request, 0, len(amList.Items))
+	reply := make([]reconcile.Request, 0, len(amList.Items))
+	if mappedAgent {
+		// If the Agent is mapped to an AgentMachine, return only that AgentMachine
+		agent := a
 		for _, agentMachine := range amList.Items {
 			if agentMachine.Status.AgentRef != nil &&
 				agentMachine.Status.AgentRef.Namespace == agent.GetNamespace() &&
@@ -711,12 +713,34 @@ func (r *AgentMachineReconciler) SetupWithManager(mgr ctrl.Manager, agentNamespa
 				break
 			}
 		}
-		return reply
+	} else {
+		// If the Agent isn't mapped to an AgentMachine and it's "valid" for use, return any AgentMachines
+		// that don't have an Agent mapped to them yet
+		agent := &aiv1beta1.Agent{}
+		if err := r.Get(ctx, types.NamespacedName{Name: a.GetName(), Namespace: a.GetNamespace()}, agent); err != nil {
+			return []reconcile.Request{}
+		}
+		if !isValidAgent(agent) {
+			return []reconcile.Request{}
+		}
+		for _, agentMachine := range amList.Items {
+			if agentMachine.Status.AgentRef == nil {
+				reply = append(reply, reconcile.Request{NamespacedName: types.NamespacedName{
+					Namespace: agentMachine.Namespace,
+					Name:      agentMachine.Name,
+				}})
+			}
+		}
 	}
 
+	return reply
+}
+
+// SetupWithManager sets up the controller with the Manager.
+func (r *AgentMachineReconciler) SetupWithManager(mgr ctrl.Manager, agentNamespace string) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&capiproviderv1.AgentMachine{}).
-		Watches(&aiv1beta1.Agent{}, handler.EnqueueRequestsFromMapFunc(mapAgentToAgentMachine)).
+		Watches(&aiv1beta1.Agent{}, handler.EnqueueRequestsFromMapFunc(r.mapAgentToAgentMachine)).
 		Watches(&clusterv1.Machine{}, handler.EnqueueRequestsFromMapFunc(r.mapMachineToAgentMachine)).
 		Complete(r)
 }

controllers/agentmachine_controller_test.go

@@ -154,9 +154,10 @@ func newAgentMachine(name, namespace string, spec capiproviderv1.AgentMachineSpe
 func newAgent(name, namespace string, spec aiv1beta1.AgentSpec) *aiv1beta1.Agent {
 	return &aiv1beta1.Agent{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
-			Namespace: namespace,
-			Labels:    make(map[string]string),
+			Name:        name,
+			Namespace:   namespace,
+			Labels:      make(map[string]string),
+			Annotations: make(map[string]string),
 		},
 		Spec: spec,
 		Status: aiv1beta1.AgentStatus{
@@ -642,3 +643,68 @@ var _ = Describe("mapMachineToAgentMachine", func() {
 		Expect(amr.mapMachineToAgentMachine(ctx, &machine)).To(BeEmpty())
 	})
 })
+
+var _ = Describe("mapAgentToAgentMachine", func() {
+	var (
+		c             client.Client
+		amr           *AgentMachineReconciler
+		ctx           = context.Background()
+		testNamespace = "test-namespace"
+	)
+
+	BeforeEach(func() {
+		c = fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+		amr = &AgentMachineReconciler{
+			Client:      c,
+			Scheme:      scheme.Scheme,
+			Log:         logrus.New(),
+			AgentClient: c,
+		}
+	})
+
+	It("returns a request for the related agent machine", func() {
+		agent := newAgent("agent", testNamespace, aiv1beta1.AgentSpec{})
+		agent.ObjectMeta.Annotations[AgentMachineRefNamespace] = testNamespace
+		Expect(c.Create(ctx, agent)).To(BeNil())
+		agentMachine, _ := newAgentMachine("agentMachine-1", testNamespace, capiproviderv1.AgentMachineSpec{}, ctx, c)
+		agentMachine.Status.AgentRef = &capiproviderv1.AgentReference{Namespace: testNamespace, Name: "agent"}
+		Expect(c.Create(ctx, agentMachine)).To(Succeed())
+
+		requests := amr.mapAgentToAgentMachine(ctx, agent)
+		Expect(len(requests)).To(Equal(1))
+
+		agentMachineKey := types.NamespacedName{
+			Name:      "agentMachine-1",
+			Namespace: testNamespace,
+		}
+		Expect(requests[0].NamespacedName).To(Equal(agentMachineKey))
+	})
+
+	It("returns nothing if the agent isn't valid", func() {
+		agent := newAgent("agent", testNamespace, aiv1beta1.AgentSpec{Approved: true})
+		agent.Status.Conditions = append(agent.Status.Conditions, v1.Condition{Type: aiv1beta1.BoundCondition, Status: boolToConditionStatus(false)})
+		agent.Status.Conditions = append(agent.Status.Conditions, v1.Condition{Type: aiv1beta1.ValidatedCondition, Status: boolToConditionStatus(false)})
+		Expect(c.Create(ctx, agent)).To(BeNil())
+		agentMachine1, _ := newAgentMachine("agentMachine-1", testNamespace, capiproviderv1.AgentMachineSpec{}, ctx, c)
+		Expect(c.Create(ctx, agentMachine1)).To(Succeed())
+		agentMachine2, _ := newAgentMachine("agentMachine-2", testNamespace, capiproviderv1.AgentMachineSpec{}, ctx, c)
+		Expect(c.Create(ctx, agentMachine2)).To(Succeed())
+
+		requests := amr.mapAgentToAgentMachine(ctx, agent)
+		Expect(len(requests)).To(Equal(0))
+	})
+
+	It("returns unmatched agent machines if no match is found", func() {
+		agent := newAgent("agent", testNamespace, aiv1beta1.AgentSpec{Approved: true})
+		agent.Status.Conditions = append(agent.Status.Conditions, v1.Condition{Type: aiv1beta1.BoundCondition, Status: boolToConditionStatus(false)})
+		agent.Status.Conditions = append(agent.Status.Conditions, v1.Condition{Type: aiv1beta1.ValidatedCondition, Status: boolToConditionStatus(true)})
+		Expect(c.Create(ctx, agent)).To(BeNil())
+		agentMachine1, _ := newAgentMachine("agentMachine-1", testNamespace, capiproviderv1.AgentMachineSpec{}, ctx, c)
+		Expect(c.Create(ctx, agentMachine1)).To(Succeed())
+		agentMachine2, _ := newAgentMachine("agentMachine-2", testNamespace, capiproviderv1.AgentMachineSpec{}, ctx, c)
+		Expect(c.Create(ctx, agentMachine2)).To(Succeed())
+
+		requests := amr.mapAgentToAgentMachine(ctx, agent)
+		Expect(len(requests)).To(Equal(2))
+	})
+})