Add doc comments

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

@@ -237,20 +237,29 @@ func (r *DockerMachinePoolReconciler) reconcileNormal(ctx context.Context, clust
 		machinePool.Spec.Replicas = pointer.Int32(1)
 	}
 
+	// First, reconcile the Docker containers, but do not delete any as we need to delete the Machine to ensure node cordon/drain.
+	// Similarly, providers implementing MachinePool Machines will need to reconcile their analagous infrastructure instances (aside
+	// from deletion) before reconciling InfraMachinePoolMachines.
 	if err := r.reconcileDockerContainers(ctx, cluster, machinePool, dockerMachinePool); err != nil {
 		return ctrl.Result{}, err
 	}
 
+	// Second, reconcile the DockerMachines once the Docker containers are created. This means creating a DockerMachine for each newly
+	// created Docker container as well as handling container deletion. Instead of deleting an infrastructure instance directly, we want to reach up
+	// to pull the associated DockerMachine and its owner Machine and call the deletion the owner Machine, which will trigger cordon and drain as well
+	// as delete the DockerMachine, which then deletes the Docker container.
+	// Similarly, providers will need to create InfraMachines for each instance, and instead of deleting instances directly, delete the associated Machine.
 	if err := r.reconcileDockerMachines(ctx, cluster, machinePool, dockerMachinePool); err != nil {
 		return ctrl.Result{}, err
 	}
 
+	// Refresh the list of DockerMachines to ensure the provider IDs are up to date.
 	dockerMachineList, err := getDockerMachines(ctx, r.Client, *cluster, *machinePool, *dockerMachinePool)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// Derive info from DockerMachines
+	// Derive providerIDList from the provider ID on each DockerMachine if it exists. The providerID is set by the DockerMachine controller.
 	dockerMachinePool.Spec.ProviderIDList = []string{}
 	for _, dockerMachine := range dockerMachineList.Items {
 		if dockerMachine.Spec.ProviderID != nil {

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

@@ -27,6 +27,7 @@ import (
 	"github.com/blang/semver/v4"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/kind/pkg/cluster/constants"
 
@@ -43,6 +44,8 @@ import (
 // reconcileDockerContainers manages the Docker containers for a MachinePool such that it
 // - Ensures the number of up-to-date Docker containers is equal to the MachinePool's desired replica count.
 // - Does not delete any containers as that must be triggered in reconcileDockerMachines to ensure node cordon/drain.
+//
+// Providers should similarly create their infrastructure instances and reconcile any additional logic.
 func (r *DockerMachinePoolReconciler) reconcileDockerContainers(ctx context.Context, cluster *clusterv1.Cluster, machinePool *expv1.MachinePool, dockerMachinePool *infraexpv1.DockerMachinePool) error {
 	log := ctrl.LoggerFrom(ctx)
 
@@ -101,6 +104,12 @@ func createDockerContainer(ctx context.Context, name string, cluster *clusterv1.
 }
 
 // reconcileDockerMachines creates and deletes DockerMachines to match the MachinePool's desired number of replicas and infrastructure spec.
+// It is responsible for
+// - Ensuring each Docker container has an associated DockerMachine by creating one if it doesn't already exist.
+// - Ensuring that deletion for Docker container happens by calling delete on the associated Machine so that the node is cordoned/drained and the infrastructure is cleaned up.
+// - Deleting DockerMachines referencing a container whose Kubernetes version or custom image no longer matches the spec.
+// - Deleting DockerMachines that correspond to a deleted/non-existent Docker container.
+// -
 func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Context, cluster *clusterv1.Cluster, machinePool *expv1.MachinePool, dockerMachinePool *infraexpv1.DockerMachinePool) error {
 	log := ctrl.LoggerFrom(ctx)
 
@@ -118,12 +127,15 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 
 	labelFilters := map[string]string{dockerMachinePoolLabel: dockerMachinePool.Name}
 
+	// List the Docker containers. This corresponds to a InfraMachinePool instance for providers.
 	externalMachines, err := docker.ListMachinesByCluster(ctx, cluster, labelFilters)
 	if err != nil {
 		return errors.Wrapf(err, "failed to list all machines in the cluster")
 	}
 
+	// Step 1:
 	// Create a DockerMachine for each Docker container so we surface the information to the user. Use the same name as the Docker container for the Docker Machine for ease of lookup.
+	// Providers should iterate through their infrastructure instances and ensure that each instance has a corresponding InfraMachine.
 	for _, machine := range externalMachines {
 		if _, ok := dockerMachineSet[machine.Name()]; !ok {
 			log.V(2).Info("Creating a new DockerMachine for Docker container", "container", machine.Name())
@@ -138,7 +150,9 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 		}
 	}
 
-	// Delete any DockerMachines that correspond to an outdated Docker container.
+	// Step 2:
+	// Delete any DockerMachines that correspond to an outdated Docker container, meaning the K8s version or custom image of the container no longer matches the spec.
+	// If the InfraMachinePool spec changes, providers should delete InfraMachines (and owner Machines) for an instance that no longer matches the spec.
 	for _, dockerMachine := range dockerMachineSet {
 		externalMachine, err := docker.NewMachine(ctx, cluster, dockerMachine.Name, labelFilters)
 		if err != nil {
@@ -164,7 +178,10 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 		externalMachineSet[externalMachine.Name()] = struct{}{}
 	}
 
+	// Step 3:
 	// Delete any DockerMachines that correspond to a deleted Docker container.
+	// Providers should iterate through the InfraMachines to ensure each one still corresponds to an existing infrastructure instance.
+	// This allows the InfraMachine (and owner Machine) to be deleted and avoid hanging resources when a user deletes an instance out-of-band.
 	for _, dockerMachine := range dockerMachineSet {
 		externalMachine, err := docker.NewMachine(ctx, cluster, dockerMachine.Name, labelFilters)
 		if err != nil {
@@ -180,14 +197,20 @@ func (r *DockerMachinePoolReconciler) reconcileDockerMachines(ctx context.Contex
 		}
 	}
 
+	// Step 4:
+	// This handles the scale down/excess replicas case. Delete DockerMachines until the desired amount of replicas is reached such that
+	// any DockerMachine whose owner Machine contains the clusterv1.DeleteMachineAnnotation is deleted first.
+
+	// For each DockerMachine, fetch the owner Machine and copy the clusterv1.DeleteMachineAnnotation to the DockerMachine if it exists before sorting the DockerMachines.
+	// This is done just before sorting to guarantee we have the latest copy of the Machine annotations.
 	dockerMachines, err := r.propogateMachineDeleteAnnotation(ctx, cluster, machinePool, dockerMachinePool)
 	if err != nil {
 		return err
 	}
 
-	// Delete any DockerMachines for excess replicas.
-
-	// Sort priority delete to the front of the list
+	// Sort priority delete to the front of the list.
+	// If providers already have a sorting order for instance deletion, i.e. oldest first or newest first, the clusterv1.DeleteMachineAnnotation must take priority.
+	// For example, if deleting by oldest, we expect the InfraMachines with clusterv1.DeleteMachineAnnotation to be deleted first followed by the oldest, and the second oldest, etc.
 	sort.SliceStable(dockerMachines, func(i, j int) bool {
 		_, iHasAnnotation := dockerMachines[i].Annotations[clusterv1.DeleteMachineAnnotation]
 		return iHasAnnotation
@@ -247,15 +270,25 @@ func (r *DockerMachinePoolReconciler) createDockerMachine(ctx context.Context, n
 }
 
 // deleteMachinePoolMachine attempts to delete a DockerMachine and its associated owner Machine if it exists.
+
 func (r *DockerMachinePoolReconciler) deleteMachinePoolMachine(ctx context.Context, dockerMachine infrav1.DockerMachine, machinePool expv1.MachinePool) error {
-	_ = ctrl.LoggerFrom(ctx)
+	log := ctrl.LoggerFrom(ctx)
+
 	machine, err := util.GetOwnerMachine(ctx, r.Client, dockerMachine.ObjectMeta)
-	// Not found doesn't return an error, so we need to check for nil.
 	if err != nil {
 		return errors.Wrapf(err, "error getting owner Machine for DockerMachine %s/%s", dockerMachine.Namespace, dockerMachine.Name)
 	}
+	// util.GetOwnerMachine() returns a nil Machine without error if there is no Machine kind in the ownerRefs, so we must verify that machine is not nil.
 	if machine == nil {
-		// TODO: Add note
+		log.V(2).Info("No owner Machine exists for DockerMachine", dockerMachine, klog.KObj(&dockerMachine))
+
+		// If the DockerMachine does not have an owner Machine, do not attempt to delete the DockerMachine as the MachinePool controller will create the
+		// Machine and we want to let it catch up. If we are too hasty to delete, that introduces a race condition where the DockerMachine could be deleted
+		// just as the Machine comes online.
+
+		// In the case where the MachinePool is being deleted and the Machine will never come online, the DockerMachine will be deleted via its ownerRef to the
+		// DockerMachinePool, so that is covered as well.
+
 		return nil
 	} else {
 		if err := r.Client.Delete(ctx, machine); err != nil {