From 897998569fb4f8ec70dffed92dff8789e1c46c92 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Thu, 9 Jan 2020 11:41:37 -0800 Subject: [PATCH 1/3] :running: Use MachineList instead of Machine slice in KubeadmControlPlane This makes it easier to split apart "get all machines" from "filter on owned machines" functionality. Signed-off-by: Daniel Lipovetsky --- .../kubeadm_control_plane_controller.go | 37 +++++++------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go b/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go index c08088a4c4a7..f33e663c02de 100644 --- a/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go +++ b/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go @@ -165,15 +165,12 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, kcp *cont } // TODO: handle proper adoption of Machines - ownedMachines, err := r.getOwnedMachines( - ctx, - kcp, - types.NamespacedName{Namespace: cluster.Namespace, Name: cluster.Name}, - ) + allMachines, err := r.getMachines(ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: cluster.Name}) if err != nil { - logger.Error(err, "failed to get list of owned machines") + logger.Error(err, "Failed to get list of machines") return ctrl.Result{}, err } + ownedMachines := r.filterOwnedMachines(kcp, allMachines) // Always attempt to update status defer func() { @@ -228,7 +225,7 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, kcp *cont // Currently we are not handling upgrade, so treat all owned machines as one for now. // Once we start handling upgrade, we'll need to filter this list and act appropriately - numMachines := len(ownedMachines) + numMachines := len(ownedMachines.Items) desiredReplicas := int(*kcp.Spec.Replicas) switch { // We are creating the first replica @@ -272,16 +269,13 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, kcp *c // This is necessary for CRDs including scale subresources. kcp.Status.Selector = selector.String() - ownedMachines, err := r.getOwnedMachines( - ctx, - kcp, - types.NamespacedName{Namespace: cluster.Namespace, Name: cluster.Name}, - ) + allMachines, err := r.getMachines(ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: cluster.Name}) if err != nil { return errors.Wrap(err, "failed to get list of owned machines") } + ownedMachines := r.filterOwnedMachines(kcp, allMachines) - replicas := int32(len(ownedMachines)) + replicas := int32(len(ownedMachines.Items)) // TODO: take into account configuration hash once upgrades are in place kcp.Status.Replicas = replicas @@ -291,8 +285,8 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, kcp *c } readyMachines := int32(0) - for _, m := range ownedMachines { - + for i := range ownedMachines.Items { + m := &ownedMachines.Items[i] node, err := getMachineNode(ctx, remoteClient, m) if err != nil { return errors.Wrap(err, "failed to get referenced Node") @@ -526,22 +520,17 @@ func (r *KubeadmControlPlaneReconciler) getMachines(ctx context.Context, cluster return allMachines, nil } -func (r *KubeadmControlPlaneReconciler) getOwnedMachines(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane, clusterName types.NamespacedName) ([]*clusterv1.Machine, error) { - allMachines, err := r.getMachines(ctx, clusterName) - if err != nil { - return nil, err - } - - var ownedMachines []*clusterv1.Machine +func (r *KubeadmControlPlaneReconciler) filterOwnedMachines(kcp *controlplanev1.KubeadmControlPlane, allMachines *clusterv1.MachineList) *clusterv1.MachineList { + ownedMachines := &clusterv1.MachineList{} for i := range allMachines.Items { m := allMachines.Items[i] controllerRef := metav1.GetControllerOf(&m) if controllerRef != nil && controllerRef.Kind == "KubeadmControlPlane" && controllerRef.Name == kcp.Name { - ownedMachines = append(ownedMachines, &m) + ownedMachines.Items = append(ownedMachines.Items, m) } } - return ownedMachines, nil + return ownedMachines } func (r *KubeadmControlPlaneReconciler) reconcileExternalReference(ctx context.Context, cluster *clusterv1.Cluster, ref corev1.ObjectReference) error { From 769f14e6852d493df34f1e6243693c4c646498b4 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Thu, 9 Jan 2020 17:06:36 -0800 Subject: [PATCH 2/3] :runner: Add util function to list all machines for a cluster Signed-off-by: Daniel Lipovetsky --- util/util.go | 16 ++++++++++++ util/util_test.go | 62 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/util/util.go b/util/util.go index fba9f71a88ff..8279d9a9397f 100644 --- a/util/util.go +++ b/util/util.go @@ -69,6 +69,22 @@ func RandomString(n int) string { return string(result) } +// GetMachinesForCluster returns a list of machines associated with the cluster. +func GetMachinesForCluster(ctx context.Context, c client.Client, cluster *clusterv1.Cluster) (*clusterv1.MachineList, error) { + var machines clusterv1.MachineList + if err := c.List( + ctx, + &machines, + client.InNamespace(cluster.Namespace), + client.MatchingLabels{ + clusterv1.ClusterLabelName: cluster.Name, + }, + ); err != nil { + return nil, err + } + return &machines, nil +} + // GetControlPlaneMachines returns a slice containing control plane machines. func GetControlPlaneMachines(machines []*clusterv1.Machine) (res []*clusterv1.Machine) { for _, machine := range machines { diff --git a/util/util_test.go b/util/util_test.go index ee8aafa88cfc..82e395096c3c 100644 --- a/util/util_test.go +++ b/util/util_test.go @@ -380,3 +380,65 @@ func TestGetOwnerMachineSuccessByName(t *testing.T) { t.Fatal("expected a machine but got nil") } } + +func TestGetMachinesForCluster(t *testing.T) { + scheme := runtime.NewScheme() + if err := clusterv1.AddToScheme(scheme); err != nil { + t.Fatal("failed to register cluster api objects to scheme") + } + + cluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster", + Namespace: "my-ns", + }, + } + + machine := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-machine", + Namespace: cluster.Namespace, + Labels: map[string]string{ + clusterv1.ClusterLabelName: cluster.Name, + }, + }, + } + + machineDifferentClusterNameSameNamespace := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-machine", + Namespace: cluster.Namespace, + Labels: map[string]string{ + clusterv1.ClusterLabelName: "other-cluster", + }, + }, + } + + machineSameClusterNameDifferentNamespace := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-machine", + Namespace: "other-ns", + Labels: map[string]string{ + clusterv1.ClusterLabelName: cluster.Name, + }, + }, + } + + c := fake.NewFakeClientWithScheme( + scheme, + machine, + machineDifferentClusterNameSameNamespace, + machineSameClusterNameDifferentNamespace, + ) + + machines, err := GetMachinesForCluster(context.Background(), c, cluster) + if err != nil { + t.Fatalf("failed to get machines for cluster: %s", err) + } + if len(machines.Items) != 1 { + t.Fatalf("expected list to have one machine, found %d", len(machines.Items)) + } + if machines.Items[0].Labels[clusterv1.ClusterLabelName] != cluster.Name { + t.Fatalf("expected list to have machine %v, found %v", machine, machines.Items[0]) + } +} From 5e03d6d1053cd141970c63ca4365d3a84709d455 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Thu, 9 Jan 2020 12:51:08 -0800 Subject: [PATCH 3/3] :sparkles: Implement delete for KubeadmControlPlane Signed-off-by: Daniel Lipovetsky --- .../kubeadm_control_plane_controller.go | 70 +++++- .../kubeadm_control_plane_controller_test.go | 217 ++++++++++++++++++ 2 files changed, 278 insertions(+), 9 deletions(-) diff --git a/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go b/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go index f33e663c02de..7a7e351e591e 100644 --- a/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go +++ b/controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go @@ -31,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" - utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apiserver/pkg/storage/names" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" @@ -55,6 +54,12 @@ import ( "sigs.k8s.io/cluster-api/util/secret" ) +const ( + // DeleteRequeueAfter is how long to wait before checking again to see if + // all control plane machines have been deleted. + DeleteRequeueAfter = 30 * time.Second +) + // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;patch // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io;bootstrap.cluster.x-k8s.io;controlplane.cluster.x-k8s.io,resources=*,verbs=get;list;watch;create;update;patch;delete @@ -324,7 +329,7 @@ func (r *KubeadmControlPlaneReconciler) scaleUpControlPlane(ctx context.Context, } } - return utilerrors.NewAggregate(errs) + return kerrors.NewAggregate(errs) } func (r *KubeadmControlPlaneReconciler) initializeControlPlane(ctx context.Context, cluster *clusterv1.Cluster, kcp *controlplanev1.KubeadmControlPlane) error { @@ -371,7 +376,7 @@ func (r *KubeadmControlPlaneReconciler) cloneConfigsAndGenerateMachine(ctx conte errs = append(errs, errors.Wrap(err, "failed to cleanup generated resources")) } - return utilerrors.NewAggregate(errs) + return kerrors.NewAggregate(errs) } return nil @@ -394,7 +399,7 @@ func (r *KubeadmControlPlaneReconciler) cleanupFromGeneration(ctx context.Contex } } - return utilerrors.NewAggregate(errs) + return kerrors.NewAggregate(errs) } func (r *KubeadmControlPlaneReconciler) generateKubeadmConfig(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane, cluster *clusterv1.Cluster, spec *bootstrapv1.KubeadmConfigSpec) (*corev1.ObjectReference, error) { @@ -469,16 +474,63 @@ func generateKubeadmControlPlaneLabels(clusterName string) map[string]string { } // reconcileDelete handles KubeadmControlPlane deletion. -func (r *KubeadmControlPlaneReconciler) reconcileDelete(_ context.Context, kcp *controlplanev1.KubeadmControlPlane, logger logr.Logger) (ctrl.Result, error) { - err := errors.New("Not Implemented") +// The implementation does not take non-control plane workloads into +// consideration. This may or may not change in the future. Please see +// https://github.com/kubernetes-sigs/cluster-api/issues/2064 +func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, kcp *controlplanev1.KubeadmControlPlane, logger logr.Logger) (_ ctrl.Result, reterr error) { + // Fetch the Cluster. + cluster, err := util.GetOwnerCluster(ctx, r.Client, kcp.ObjectMeta) + if err != nil { + logger.Error(err, "Failed to retrieve owner Cluster from the API Server") + return ctrl.Result{}, err + } + if cluster == nil { + logger.Info("Cluster Controller has not yet set OwnerRef") + return ctrl.Result{}, nil + } + logger = logger.WithValues("cluster", cluster.Name) + // Fetch Machines + allMachines, err := util.GetMachinesForCluster(ctx, r.Client, cluster) if err != nil { - logger.Error(err, "Not Implemented") + logger.Error(err, "Failed to get list of machines") return ctrl.Result{}, err } + ownedMachines := r.filterOwnedMachines(kcp, allMachines) - controllerutil.RemoveFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer) - return ctrl.Result{}, nil + // Always attempt to update status + defer func() { + if err := r.updateStatus(ctx, kcp, cluster); err != nil { + logger.Error(err, "Failed to update status") + reterr = kerrors.NewAggregate([]error{reterr, err}) + } + }() + + // Verify that only control plane machines remain + if len(allMachines.Items) != len(ownedMachines.Items) { + err := errors.New("at least one machine is not owned by the control plane") + logger.Error(err, "Failed to delete the control plane") + return ctrl.Result{}, err + } + + // If no control plane machines remain, remove the finalizer + if len(ownedMachines.Items) == 0 { + controllerutil.RemoveFinalizer(kcp, controlplanev1.KubeadmControlPlaneFinalizer) + return ctrl.Result{}, nil + } + + // Delete control plane machines in parallel + var errs []error + for i := range ownedMachines.Items { + m := &ownedMachines.Items[i] + if err := r.Client.Delete(ctx, m); err != nil && !apierrors.IsNotFound(err) { + errs = append(errs, errors.Wrap(err, "failed to cleanup owned machines")) + } + } + if errs != nil { + return ctrl.Result{}, kerrors.NewAggregate(errs) + } + return ctrl.Result{RequeueAfter: DeleteRequeueAfter}, nil } func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context, clusterName types.NamespacedName, endpoint clusterv1.APIEndpoint, kcp *controlplanev1.KubeadmControlPlane) error { diff --git a/controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go b/controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go index a1464173efe4..d793149a0bdb 100644 --- a/controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go +++ b/controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go @@ -1328,3 +1328,220 @@ func TestCloneConfigsAndGenerateMachine(t *testing.T) { g.Expect(m.Spec.Bootstrap.ConfigRef.Kind).To(gomega.Equal("KubeadmConfig")) } } + +func TestReconcileControlPlaneDelete(t *testing.T) { + cluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "test", + }, + Spec: clusterv1.ClusterSpec{ + ControlPlaneEndpoint: clusterv1.APIEndpoint{ + Host: "test.local", + Port: 9999, + }, + ControlPlaneRef: &corev1.ObjectReference{ + Kind: "KubeadmControlPlane", + Namespace: "test", + Name: "kcp-foo", + APIVersion: controlplanev1.GroupVersion.String(), + }, + }, + } + + genericMachineTemplate := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "GenericMachineTemplate", + "apiVersion": "generic.io/v1", + "metadata": map[string]interface{}{ + "name": "infra-foo", + "namespace": cluster.Namespace, + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "hello": "world", + }, + }, + }, + }, + } + + t.Run("delete control plane machines", func(t *testing.T) { + g := gomega.NewWithT(t) + + kcp := &controlplanev1.KubeadmControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cluster.Namespace, + Name: "foo", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Cluster", + APIVersion: clusterv1.GroupVersion.String(), + Name: cluster.Name, + }, + }, + }, + Spec: controlplanev1.KubeadmControlPlaneSpec{ + InfrastructureTemplate: corev1.ObjectReference{ + Kind: genericMachineTemplate.GetKind(), + Namespace: genericMachineTemplate.GetNamespace(), + Name: genericMachineTemplate.GetName(), + APIVersion: genericMachineTemplate.GetAPIVersion(), + }, + KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{ + ClusterConfiguration: &kubeadmv1.ClusterConfiguration{}, + InitConfiguration: &kubeadmv1.InitConfiguration{}, + JoinConfiguration: &kubeadmv1.JoinConfiguration{}, + }, + Replicas: utilpointer.Int32Ptr(3), + }, + } + + machine := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-0", + Namespace: cluster.Namespace, + Labels: generateKubeadmControlPlaneLabels(cluster.Name), + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")), + }, + }, + } + + kcp.Default() + g.Expect(kcp.ValidateCreate()).To(gomega.Succeed()) + + g.Expect(clusterv1.AddToScheme(scheme.Scheme)).To(gomega.Succeed()) + g.Expect(bootstrapv1.AddToScheme(scheme.Scheme)).To(gomega.Succeed()) + g.Expect(controlplanev1.AddToScheme(scheme.Scheme)).To(gomega.Succeed()) + fakeClient := fake.NewFakeClientWithScheme( + scheme.Scheme, + kcp.DeepCopy(), + cluster.DeepCopy(), + machine.DeepCopy(), + genericMachineTemplate.DeepCopy(), + ) + log.SetLogger(klogr.New()) + + r := &KubeadmControlPlaneReconciler{ + Client: fakeClient, + Log: log.Log, + remoteClient: func(c client.Client, _ *clusterv1.Cluster, _ *runtime.Scheme) (client.Client, error) { + return c, nil + }, + recorder: record.NewFakeRecorder(32), + } + + // Create control plane machines + result, err := r.reconcile(context.Background(), kcp, r.Log) + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(result).To(gomega.Equal(ctrl.Result{})) + + g.Expect(kcp.Status.Replicas).To(gomega.BeEquivalentTo(3)) + + // Delete control plane machines and requeue, but do not remove finalizer + result, err = r.reconcileDelete(context.Background(), kcp, r.Log) + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(result).To(gomega.Equal(ctrl.Result{RequeueAfter: DeleteRequeueAfter})) + + g.Expect(kcp.Status.Replicas).To(gomega.BeEquivalentTo(0)) + g.Expect(kcp.Finalizers).To(gomega.Equal([]string{controlplanev1.KubeadmControlPlaneFinalizer})) + + // Remove finalizer + result, err = r.reconcileDelete(context.Background(), kcp, r.Log) + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(result).To(gomega.Equal(ctrl.Result{})) + + g.Expect(kcp.Status.Replicas).To(gomega.BeEquivalentTo(0)) + g.Expect(kcp.Finalizers).To(gomega.Equal([]string{})) + }) + + t.Run("fail to delete control plane machines because at least one machine is not owned by the control plane", func(t *testing.T) { + g := gomega.NewWithT(t) + + kcp := &controlplanev1.KubeadmControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cluster.Namespace, + Name: "foo", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Cluster", + APIVersion: clusterv1.GroupVersion.String(), + Name: cluster.Name, + }, + }, + }, + Spec: controlplanev1.KubeadmControlPlaneSpec{ + InfrastructureTemplate: corev1.ObjectReference{ + Kind: genericMachineTemplate.GetKind(), + Namespace: genericMachineTemplate.GetNamespace(), + Name: genericMachineTemplate.GetName(), + APIVersion: genericMachineTemplate.GetAPIVersion(), + }, + KubeadmConfigSpec: bootstrapv1.KubeadmConfigSpec{ + ClusterConfiguration: &kubeadmv1.ClusterConfiguration{}, + InitConfiguration: &kubeadmv1.InitConfiguration{}, + JoinConfiguration: &kubeadmv1.JoinConfiguration{}, + }, + Replicas: utilpointer.Int32Ptr(3), + }, + } + + machine := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo-0", + Namespace: cluster.Namespace, + Labels: generateKubeadmControlPlaneLabels(cluster.Name), + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")), + }, + }, + } + + workerMachine := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar-0", + Namespace: cluster.Namespace, + Labels: map[string]string{ + clusterv1.ClusterLabelName: cluster.Name, + }, + }, + } + + kcp.Default() + g.Expect(kcp.ValidateCreate()).To(gomega.Succeed()) + + g.Expect(clusterv1.AddToScheme(scheme.Scheme)).To(gomega.Succeed()) + g.Expect(bootstrapv1.AddToScheme(scheme.Scheme)).To(gomega.Succeed()) + g.Expect(controlplanev1.AddToScheme(scheme.Scheme)).To(gomega.Succeed()) + fakeClient := fake.NewFakeClientWithScheme( + scheme.Scheme, + kcp.DeepCopy(), + cluster.DeepCopy(), + machine.DeepCopy(), + workerMachine.DeepCopy(), + genericMachineTemplate.DeepCopy(), + ) + log.SetLogger(klogr.New()) + + r := &KubeadmControlPlaneReconciler{ + Client: fakeClient, + Log: log.Log, + remoteClient: func(c client.Client, _ *clusterv1.Cluster, _ *runtime.Scheme) (client.Client, error) { + return c, nil + }, + recorder: record.NewFakeRecorder(32), + } + + result, err := r.reconcile(context.Background(), kcp, r.Log) + g.Expect(err).NotTo(gomega.HaveOccurred()) + g.Expect(result).To(gomega.Equal(ctrl.Result{})) + + g.Expect(kcp.Status.Replicas).To(gomega.BeEquivalentTo(3)) + + result, err = r.reconcileDelete(context.Background(), kcp, r.Log) + g.Expect(err).Should(gomega.MatchError(gomega.ContainSubstring("at least one machine is not owned by the control plane"))) + g.Expect(result).To(gomega.Equal(ctrl.Result{})) + }) +}