From 9698890407616229ce53c6ce309cde66acf800fd Mon Sep 17 00:00:00 2001 From: Jont828 Date: Wed, 25 Oct 2023 20:01:40 -0400 Subject: [PATCH] Reuse DockerMachine list and add just in time propogation for annotation --- .../dockermachinepool_controller_phases.go | 69 ++++--- .../dockermachinepool_controller_test.go | 170 ------------------ .../controllers/dockermachine_controller.go | 4 - 3 files changed, 45 insertions(+), 198 deletions(-) delete mode 100644 test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_test.go diff --git a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go b/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go index 5d3dafbd2467..daa4bf6cd58f 100644 --- a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_phases.go +++ b/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, diff --git a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_test.go b/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_test.go deleted file mode 100644 index 0b89bd78a187..000000000000 --- a/test/infrastructure/docker/exp/internal/controllers/dockermachinepool_controller_test.go +++ /dev/null @@ -1,170 +0,0 @@ -/* -Copyright 2023 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// Package controllers implements controller functionality. -package controllers - -// import ( -// "context" -// "fmt" -// "testing" - -// . "github.com/onsi/gomega" -// corev1 "k8s.io/api/core/v1" -// metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -// "k8s.io/utils/pointer" -// "sigs.k8s.io/controller-runtime/pkg/client" -// "sigs.k8s.io/controller-runtime/pkg/client/fake" - -// clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" -// expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" -// infrav1 "sigs.k8s.io/cluster-api/test/infrastructure/docker/api/v1beta1" -// ) - -// const ( -// clusterName = "test-cluster" -// ) - -// func TestDeleteMachinePoolMachine(t *testing.T) { -// defaultMachinePool := &expv1.MachinePool{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "machinepool-test", -// Namespace: metav1.NamespaceDefault, -// Labels: map[string]string{ -// clusterv1.ClusterNameLabel: clusterName, -// }, -// }, -// Spec: expv1.MachinePoolSpec{ -// ClusterName: clusterName, -// Replicas: pointer.Int32(2), -// Template: clusterv1.MachineTemplateSpec{ -// Spec: clusterv1.MachineSpec{ -// Bootstrap: clusterv1.Bootstrap{ -// ConfigRef: &corev1.ObjectReference{ -// APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", -// Kind: "BootstrapConfig", -// Name: "bootstrap-config1", -// }, -// }, -// InfrastructureRef: corev1.ObjectReference{ -// APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", -// Kind: "InfrastructureConfig", -// Name: "infra-config1", -// }, -// }, -// }, -// }, -// } - -// machine := &clusterv1.Machine{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "machine1", -// Namespace: metav1.NamespaceDefault, -// Labels: map[string]string{ -// clusterv1.ClusterNameLabel: clusterName, -// clusterv1.MachinePoolNameLabel: "machinepool-test", -// }, -// }, -// Spec: clusterv1.MachineSpec{ -// ClusterName: clusterName, -// InfrastructureRef: corev1.ObjectReference{ -// APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", -// Kind: "DockerMachine", -// Name: "docker-machine", -// Namespace: metav1.NamespaceDefault, -// }, -// }, -// } - -// dockerMachine := &infrav1.DockerMachine{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "docker-machine", -// Namespace: metav1.NamespaceDefault, -// OwnerReferences: []metav1.OwnerReference{}, -// }, -// } - -// testCases := []struct { -// name string -// dockerMachine *infrav1.DockerMachine -// machine *clusterv1.Machine -// machinePool *expv1.MachinePool -// expectedError string -// }{ -// { -// name: "delete owner Machine for DockerMachine", -// dockerMachine: dockerMachine.DeepCopy(), -// machine: machine.DeepCopy(), -// machinePool: defaultMachinePool.DeepCopy(), -// expectedError: "", -// }, -// { -// name: "delete DockerMachine directly when MachinePool is not found", -// dockerMachine: dockerMachine.DeepCopy(), -// machine: nil, -// machinePool: nil, -// expectedError: "", -// }, -// { -// name: "return error to requeue when owner Machine doesn't exist but MachinePool does", -// dockerMachine: dockerMachine.DeepCopy(), -// machine: nil, -// machinePool: defaultMachinePool.DeepCopy(), -// expectedError: fmt.Sprintf("DockerMachine %s/%s has no owner Machine, will reattempt deletion once owner Machine is present", dockerMachine.Namespace, dockerMachine.Name), -// }, -// } - -// for _, tc := range testCases { -// t.Run(tc.name, func(t *testing.T) { -// g := NewWithT(t) - -// objs := []client.Object{tc.dockerMachine} -// if tc.machine != nil { -// objs = append(objs, tc.machine) -// } -// mp := expv1.MachinePool{} -// if tc.machinePool != nil { -// objs = append(objs, tc.machinePool) -// mp = *tc.machinePool -// } - -// r := &DockerMachinePoolReconciler{ -// Client: fake.NewClientBuilder().WithObjects(objs...).Build(), -// } - -// if tc.machine != nil { -// machine := &clusterv1.Machine{} -// g.Expect(r.Client.Get(context.Background(), client.ObjectKeyFromObject(tc.machine), machine)).To(Succeed()) -// tc.dockerMachine.OwnerReferences = append(tc.dockerMachine.OwnerReferences, *metav1.NewControllerRef(machine, machine.GroupVersionKind())) -// } - -// err := r.deleteMachinePoolMachine(context.Background(), *tc.dockerMachine, mp) -// if tc.expectedError != "" { -// g.Expect(err).To(HaveOccurred()) -// g.Expect(err.Error()).To(ContainSubstring(tc.expectedError)) -// } else { -// g.Expect(err).NotTo(HaveOccurred()) - -// // If Machine exists, expect delete call to go to Machine, which triggers DockerMachine deletion eventually (though we're not running it here). -// if tc.machine != nil { -// g.Expect(r.Client.Get(context.Background(), client.ObjectKeyFromObject(tc.machine), tc.machine)).ToNot(Succeed()) -// } else { // Otherwise, expect delete call to go to DockerMachine directly. -// g.Expect(r.Client.Get(context.Background(), client.ObjectKeyFromObject(tc.dockerMachine), tc.dockerMachine)).ToNot(Succeed()) -// } -// } -// }) -// } -// } diff --git a/test/infrastructure/docker/internal/controllers/dockermachine_controller.go b/test/infrastructure/docker/internal/controllers/dockermachine_controller.go index 273a51c851f3..ec6ac826b8e3 100644 --- a/test/infrastructure/docker/internal/controllers/dockermachine_controller.go +++ b/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 {