Reuse DockerMachine list and add just in time propogation for annotation

test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go

@@ -111,9 +111,9 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 		return err
 	}
 
-	dockerMachineSet := make(map[string]struct{})
+	dockerMachineSet := make(map[string]infrav1.DockerMachine)
 	for _, dockerMachine := range dockerMachineList.Items {
-		dockerMachineSet[dockerMachine.Name] = struct{}{}
+		dockerMachineSet[dockerMachine.Name] = dockerMachine
 	}
 
 	labelFilters := map[string]string{dockerMachinePoolLabel: dockerMachinePool.Name}
@@ -127,22 +127,19 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 	for _, machine := range externalMachines {
 		if _, ok := dockerMachineSet[machine.Name()]; !ok {
 			log.V(2).Info("Creating a new DockerMachine for Docker container", "container", machine.Name())
-			if err := r.createDockerMachine(ctx, machine.Name(), cluster, machinePool, dockerMachinePool); err != nil {
+			dockerMachine, err := r.createDockerMachine(ctx, machine.Name(), cluster, machinePool, dockerMachinePool)
+			if err != nil {
 				return errors.Wrap(err, "failed to create a new docker machine")
 			}
+
+			dockerMachineSet[dockerMachine.Name] = *dockerMachine
 		} else {
 			log.V(4).Info("DockerMachine already exists, nothing to do", "name", machine.Name())
 		}
 	}
 
-	// Refresh list
-	dockerMachineList, err = getDockerMachines(ctx, r.Client, *cluster, *machinePool, *dockerMachinePool)
-	if err != nil {
-		return err
-	}
-
 	// Delete any DockerMachines that correspond to an outdated Docker container.
-	for _, dockerMachine := range dockerMachineList.Items {
+	for _, dockerMachine := range dockerMachineSet {
 		externalMachine, err := docker.NewMachine(ctx, cluster, dockerMachine.Name, labelFilters)
 		if err != nil {
 			return err
@@ -152,13 +149,9 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 			if err := r.deleteMachinePoolMachine(ctx, dockerMachine, *machinePool); err != nil {
 				return err
 			}
-		}
-	}
 
-	// Refresh list
-	dockerMachineList, err = getDockerMachines(ctx, r.Client, *cluster, *machinePool, *dockerMachinePool)
-	if err != nil {
-		return err
+			delete(dockerMachineSet, dockerMachine.Name)
+		}
 	}
 
 	externalMachines, err = docker.ListMachinesByCluster(ctx, cluster, labelFilters)
@@ -172,7 +165,7 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 	}
 
 	// Delete any DockerMachines that correspond to a deleted Docker container.
-	for _, dockerMachine := range dockerMachineList.Items {
+	for _, dockerMachine := range dockerMachineSet {
 		externalMachine, err := docker.NewMachine(ctx, cluster, dockerMachine.Name, labelFilters)
 		if err != nil {
 			return err
@@ -182,17 +175,16 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 			if err := r.deleteMachinePoolMachine(ctx, dockerMachine, *machinePool); err != nil {
 				return err
 			}
+
+			delete(dockerMachineSet, dockerMachine.Name)
 		}
 	}
 
-	// Refresh list
-	dockerMachineList, err = getDockerMachines(ctx, r.Client, *cluster, *machinePool, *dockerMachinePool)
+	dockerMachines, err := r.propogateMachineDeleteAnnotation(ctx, cluster, machinePool, dockerMachinePool)
 	if err != nil {
 		return err
 	}
 
-	dockerMachines := dockerMachineList.Items
-
 	// Delete any DockerMachines for excess replicas.
 
 	// Sort priority delete to the front of the list
@@ -216,7 +208,7 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 
 // createDockerMachine creates a DockerMachine to represent a Docker container in a DockerMachinePool.
 // These DockerMachines have the clusterv1.ClusterNameLabel and clusterv1.MachinePoolNameLabel to support MachinePool Machines.
-func (r *DockerMachinePoolReconciler) createDockerMachine(ctx context.Context, name string, cluster *clusterv1.Cluster, machinePool *expv1.MachinePool, dockerMachinePool *infraexpv1.DockerMachinePool) error {
+func (r *DockerMachinePoolReconciler) createDockerMachine(ctx context.Context, name string, cluster *clusterv1.Cluster, machinePool *expv1.MachinePool, dockerMachinePool *infraexpv1.DockerMachinePool) (*infrav1.DockerMachine, error) {
 	log := ctrl.LoggerFrom(ctx)
 
 	labels := map[string]string{
@@ -248,10 +240,10 @@ func (r *DockerMachinePoolReconciler) createDockerMachine(ctx context.Context, n
 	log.V(2).Info("Creating DockerMachine", "dockerMachine", dockerMachine.Name)
 
 	if err := r.Client.Create(ctx, dockerMachine); err != nil {
-		return errors.Wrap(err, "failed to create dockerMachine")
+		return nil, errors.Wrap(err, "failed to create dockerMachine")
 	}
 
-	return nil
+	return dockerMachine, nil
 }
 
 // deleteMachinePoolMachine attempts to delete a DockerMachine and its associated owner Machine if it exists.
@@ -274,6 +266,35 @@ func (r *DockerMachinePoolReconciler) deleteMachinePoolMachine(ctx context.Conte
 	return nil
 }
 
+// propogateMachineDeleteAnnotation returns the DockerMachines for a MachinePool and for each DockerMachine, it copies the owner
+// Machine's delete annotation to each DockerMachine if it's present. This is done just in time to ensure that the annotations are
+// up to date when we sort for DockerMachine deletion.
+func (r *DockerMachinePoolReconciler) propogateMachineDeleteAnnotation(ctx context.Context, cluster *clusterv1.Cluster, machinePool *expv1.MachinePool, dockerMachinePool *infraexpv1.DockerMachinePool) ([]infrav1.DockerMachine, error) {
+	_ = ctrl.LoggerFrom(ctx)
+
+	dockerMachineList, err := getDockerMachines(ctx, r.Client, *cluster, *machinePool, *dockerMachinePool)
+	if err != nil {
+		return nil, err
+	}
+
+	dockerMachines := []infrav1.DockerMachine{}
+	for _, dockerMachine := range dockerMachineList.Items {
+		machine, err := util.GetOwnerMachine(ctx, r.Client, dockerMachine.ObjectMeta)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error getting owner Machine for DockerMachine %s/%s", dockerMachine.Namespace, dockerMachine.Name)
+		}
+		if machine != nil && machine.Annotations != nil {
+			if _, hasDeleteAnnotation := machine.Annotations[clusterv1.DeleteMachineAnnotation]; hasDeleteAnnotation {
+				dockerMachine.Annotations[clusterv1.DeleteMachineAnnotation] = machine.Annotations[clusterv1.DeleteMachineAnnotation]
+			}
+		}
+
+		dockerMachines = append(dockerMachines, dockerMachine)
+	}
+
+	return dockerMachines, nil
+}
+
 // isMachineMatchingInfrastructureSpec returns true if the Docker container image matches the custom image in the DockerMachinePool spec.
 func isMachineMatchingInfrastructureSpec(_ context.Context, machine *docker.Machine, machinePool *expv1.MachinePool, dockerMachinePool *infraexpv1.DockerMachinePool) bool {
 	// NOTE: With the current implementation we are checking if the machine is using a kindest/node image for the expected version,

test/infrastructure/docker/internal/controllers/dockermachine_controller.go

@@ -153,10 +153,6 @@ func (r *DockerMachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, nil
 	}
 
-	if _, hasDeleteAnnotation := machine.Annotations[clusterv1.DeleteMachineAnnotation]; hasDeleteAnnotation {
-		dockerMachine.Annotations[clusterv1.DeleteMachineAnnotation] = machine.Annotations[clusterv1.DeleteMachineAnnotation]
-	}
-
 	// Create a helper for managing the docker container hosting the machine.
 	externalMachine, err := docker.NewMachine(ctx, cluster, dockerMachine.Name, nil)
 	if err != nil {