Skip to content

Commit

Permalink
Ensure infrastructure is destroyed before deleting the node
Browse files Browse the repository at this point in the history 
If there were an edge case where we deleted an unreachable node and therefore removing any pod from etcd, but an stateful process were still running on the underlying host, we'll be immediately freeing up the pod name from the apiserver. "This would let the StatefulSet controller create a replacement Pod with that same identity; this can lead to the duplication of a still-running Pod, and if said Pod can still communicate with the other members of the StatefulSet, will violate the at most one semantics that StatefulSet is designed to guarantee."

To mitigate this it'd be safer to delete the node only after the owned infraMachine is gone. This would give stronger guarantees that the underlying host is destroyed and so there's no chance of a stateful "leaked" process still running.
#2565
  • Loading branch information
@enxebre
enxebre committed Mar 9, 2020
1 parent 88506f8 commit ba6758b
Show file tree
Hide file tree
Showing 2 changed files with 196 additions and 15 deletions.
38 changes: 23 additions & 15 deletions controllers/machine_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,8 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
logger := r.Log.WithValues("machine", m.Name, "namespace", m.Namespace)
logger = logger.WithValues("cluster", cluster.Name)

if err := r.isDeleteNodeAllowed(ctx, m); err != nil {
isDeleteNodeAllowed, err := r.isDeleteNodeAllowed(ctx, m)
if err != nil {
switch err {
case errNilNodeRef:
logger.Error(err, "Deleting node is not allowed")
Expand All @@ -271,7 +272,9 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
logger.Error(err, "IsDeleteNodeAllowed check failed")
return ctrl.Result{}, err
}
} else {
}

if isDeleteNodeAllowed {
// Drain node before deletion
if _, exists := m.ObjectMeta.Annotations[clusterv1.ExcludeNodeDrainingAnnotation]; !exists {
logger.Info("Draining node", "node", m.Status.NodeRef.Name)
Expand All @@ -281,6 +284,17 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
}
r.recorder.Eventf(m, corev1.EventTypeNormal, "SuccessfulDrainNode", "success draining Machine's node %q", m.Status.NodeRef.Name)
}
}

if ok, err := r.reconcileDeleteExternal(ctx, m); !ok || err != nil {
// Return early and don't remove the finalizer if we got an error or
// the external reconciliation deletion isn't ready.
return ctrl.Result{}, err
}

// We only delete the node after the underlying infrastructure is gone.
// https://github.com/kubernetes-sigs/cluster-api/issues/2565
if isDeleteNodeAllowed {
logger.Info("Deleting node", "node", m.Status.NodeRef.Name)

var deleteNodeErr error
Expand All @@ -296,28 +310,22 @@ func (r *MachineReconciler) reconcileDelete(ctx context.Context, cluster *cluste
}
}

if ok, err := r.reconcileDeleteExternal(ctx, m); !ok || err != nil {
// Return early and don't remove the finalizer if we got an error or
// the external reconciliation deletion isn't ready.
return ctrl.Result{}, err
}

controllerutil.RemoveFinalizer(m, clusterv1.MachineFinalizer)
return ctrl.Result{}, nil
}

// isDeleteNodeAllowed returns nil only if the Machine's NodeRef is not nil
// isDeleteNodeAllowed returns true only if the Machine's NodeRef is not nil
// and if the Machine is not the last control plane node in the cluster.
func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, machine *clusterv1.Machine) error {
func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, machine *clusterv1.Machine) (bool, error) {
// Cannot delete something that doesn't exist.
if machine.Status.NodeRef == nil {
return errNilNodeRef
return false, errNilNodeRef
}

// Get all of the machines that belong to this cluster.
machines, err := getActiveMachinesInCluster(ctx, r.Client, machine.Namespace, machine.Labels[clusterv1.ClusterLabelName])
if err != nil {
return err
return false, err
}

// Whether or not it is okay to delete the NodeRef depends on the
Expand All @@ -327,14 +335,14 @@ func (r *MachineReconciler) isDeleteNodeAllowed(ctx context.Context, machine *cl
case numControlPlaneMachines == 0:
// Do not delete the NodeRef if there are no remaining members of
// the control plane.
return errNoControlPlaneNodes
return false, errNoControlPlaneNodes
case numControlPlaneMachines == 1 && util.IsControlPlaneMachine(machine):
// Do not delete the NodeRef if this is the last member of the
// control plane.
return errLastControlPlaneNode
return false, errLastControlPlaneNode
default:
// Otherwise it is okay to delete the NodeRef.
return nil
return true, nil
}
}

Expand Down
173 changes: 173 additions & 0 deletions controllers/machine_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,3 +784,176 @@ func Test_clusterToActiveMachines(t *testing.T) {
g.Expect(got).To(Equal(tt.want))
}
}

func TestIsDeleteNodeAllowed(t *testing.T) {
testCases := []struct {
name string
machine *clusterv1.Machine
expected bool
expectedError error
}{
{
name: "machine without nodeRef",
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
},
Spec: clusterv1.MachineSpec{
ClusterName: "test-cluster",
InfrastructureRef: corev1.ObjectReference{},
Bootstrap: clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
},
Status: clusterv1.MachineStatus{},
},
expected: false,
expectedError: errNilNodeRef,
},
{
name: "no control plane members",
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
},
Spec: clusterv1.MachineSpec{
ClusterName: "test-cluster",
InfrastructureRef: corev1.ObjectReference{},
Bootstrap: clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
},
Status: clusterv1.MachineStatus{
NodeRef: &corev1.ObjectReference{
Name: "test",
},
},
},
expected: false,
expectedError: errNoControlPlaneNodes,
},
{
name: "is last control plane members",
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
clusterv1.MachineControlPlaneLabelName: "",
},
Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
},
Spec: clusterv1.MachineSpec{
ClusterName: "test-cluster",
InfrastructureRef: corev1.ObjectReference{},
Bootstrap: clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
},
Status: clusterv1.MachineStatus{
NodeRef: &corev1.ObjectReference{
Name: "test",
},
},
},
expected: false,
expectedError: errLastControlPlaneNode,
},
{
name: "has nodeRef and control plane is healthy",
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
},
Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
},
Spec: clusterv1.MachineSpec{
ClusterName: "test-cluster",
InfrastructureRef: corev1.ObjectReference{},
Bootstrap: clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
},
Status: clusterv1.MachineStatus{
NodeRef: &corev1.ObjectReference{
Name: "test",
},
},
},
expected: true,
expectedError: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

m1 := &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "cp1",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
},
Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
},
Spec: clusterv1.MachineSpec{
ClusterName: "test-cluster",
InfrastructureRef: corev1.ObjectReference{},
Bootstrap: clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
},
Status: clusterv1.MachineStatus{
NodeRef: &corev1.ObjectReference{
Name: "test1",
},
},
}
m2 := &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "cp2",
Namespace: "default",
Labels: map[string]string{
clusterv1.ClusterLabelName: "test",
},
Finalizers: []string{clusterv1.MachineFinalizer, metav1.FinalizerDeleteDependents},
},
Spec: clusterv1.MachineSpec{
ClusterName: "test-cluster",
InfrastructureRef: corev1.ObjectReference{},
Bootstrap: clusterv1.Bootstrap{Data: pointer.StringPtr("data")},
},
Status: clusterv1.MachineStatus{
NodeRef: &corev1.ObjectReference{
Name: "test2",
},
},
}
// For isDeleteNodeAllowed to be true we assume a healthy control plane.
if tc.expected {
m1.Labels[clusterv1.MachineControlPlaneLabelName] = ""
m2.Labels[clusterv1.MachineControlPlaneLabelName] = ""
}

mr := &MachineReconciler{
Client: fake.NewFakeClientWithScheme(
scheme.Scheme,
tc.machine,
m1,
m2,
),
Log: log.Log,
scheme: scheme.Scheme,
}

isDeleteNodeAllowed, err := mr.isDeleteNodeAllowed(context.TODO(), tc.machine);
g.Expect(isDeleteNodeAllowed).To(Equal(tc.expected))
if tc.expectedError == nil {
g.Expect(err).To(BeNil())
} else {
g.Expect(err).To(Equal(tc.expectedError))
}

})
}
}

0 comments on commit ba6758b

Please sign in to comment.