Repository: kubermatic/machine-controller
Pull request #813 (2020-08-23 16:32:50)

Delete Nodes by NodeRef and compare node and machine names when getting a Hetzner node
**What this PR does / why we need it**:

This PR attempts to fix the bug which occurs when a Machine is rolled-out and the new Machine has the same IP addresses as the old one. It could happen that the machine-controller sets the NodeOwner label on the old Node object to the UID of the new Machine. That can break the logic for removing the old Node object, as it depends on the NodeOwner label having the UID of the Machine that's being deleted.

There are two fixes:

* The getNode function which is used when adding the NodeRef now confirms that the instance and node names match for **Hetzner Machines/instances**. Previously, we only checked IP addresses, so if we a new Machine has the same addresses as the old one, it can adopt a node that's to-be-deleted
  * This assumes the name of the Node object will always be the same as the Machine name.
    * The Machine name is **not always** same as the Node name. For example, on AWS, the Machine name is based on the MachineDeployment name, while the Node name is the internal DNS name.
  * On providers with CCM (internal or external), that's not a problem because CCM is responsible for setting the ProviderID on the Node object. If the ProviderID is present, it will be used by the `getNode` function instead of IP addresses
* The deleteNodeForMachine function now uses NodeRef if it's present. We use NodeRef for all other things when deleting a machine, such as evicting the node and deleting the cloud provider instance
  * The old logic (based on the NodeOwner label) is kept in the place but it's used only as a fall-back if the NodeRef is not present

**Optional Release Note**:
```release-note
* Delete the Node object by NodeRef if it's present
* Compare node and instances names when applying the NodeOwner label for Hetzner machines/instances
```


kubermatic-bot: @xmudrii: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-machine-controller-test | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-test/1297805680220049409) | `/test pull-machine-controller-test`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=813)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
kubermatic-bot: @xmudrii: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-machine-controller-test | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-test/1297805680220049409) | `/test pull-machine-controller-test`
pull-machine-controller-e2e-azure | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-azure/1297805680207466496) | `/test pull-machine-controller-e2e-azure`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=813)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
kubermatic-bot: @xmudrii: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-machine-controller-test | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-test/1297805680220049409) | `/test pull-machine-controller-test`
pull-machine-controller-e2e-azure | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-azure/1297805680207466496) | `/test pull-machine-controller-e2e-azure`
pull-machine-controller-e2e-aws | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-aws/1297805680224243714) | `/test pull-machine-controller-e2e-aws`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=813)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
kubermatic-bot: @xmudrii: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-machine-controller-test | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-test/1297805680220049409) | `/test pull-machine-controller-test`
pull-machine-controller-e2e-azure | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-azure/1297805680207466496) | `/test pull-machine-controller-e2e-azure`
pull-machine-controller-e2e-aws | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-aws/1297805680224243714) | `/test pull-machine-controller-e2e-aws`
pull-machine-controller-e2e-aws-ebs-encryption-enabled | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-aws-ebs-encryption-enabled/1297805680241020930) | `/test pull-machine-controller-e2e-aws-ebs-encryption-enabled`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=813)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
xmudrii: /assign @kron4eg @xrstf 
kubermatic-bot: @xmudrii: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-machine-controller-test | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-test/1297805680220049409) | `/test pull-machine-controller-test`
pull-machine-controller-e2e-azure | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-azure/1297805680207466496) | `/test pull-machine-controller-e2e-azure`
pull-machine-controller-e2e-aws | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-aws/1297805680224243714) | `/test pull-machine-controller-e2e-aws`
pull-machine-controller-e2e-aws-ebs-encryption-enabled | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-aws-ebs-encryption-enabled/1297805680241020930) | `/test pull-machine-controller-e2e-aws-ebs-encryption-enabled`
pull-machine-controller-lint | 04c2128cb42b638a9a383b568bccec89ce9e8db7 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-lint/1297873123487518720) | `/test pull-machine-controller-lint`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=813)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
kubermatic-bot: @xmudrii: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-machine-controller-e2e-azure | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-azure/1297805680207466496) | `/test pull-machine-controller-e2e-azure`
pull-machine-controller-e2e-aws | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-aws/1297805680224243714) | `/test pull-machine-controller-e2e-aws`
pull-machine-controller-e2e-aws-ebs-encryption-enabled | 7c1fd61eab7b14d35c531c0fb0c9091c05eb2406 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-aws-ebs-encryption-enabled/1297805680241020930) | `/test pull-machine-controller-e2e-aws-ebs-encryption-enabled`
pull-machine-controller-e2e-openstack | a4be49125efab3fe886837a5446c7cc4a8c97135 | [link](https://public-prow.loodse.com/view/gs/prow-dev-public-data/pr-logs/pull/kubermatic_machine-controller/813/pull-machine-controller-e2e-openstack/1297875267070464003) | `/test pull-machine-controller-e2e-openstack`

[Full PR test history](https://public-prow.loodse.com/pr-history?org=kubermatic&repo=machine-controller&pr=813)

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
xmudrii: /retest
irozzo-1A: /approve
kubermatic-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubermatic/machine-controller/pull/813#issuecomment-680874056" title="Approved">irozzo-1A</a>*, *<a href="https://github.com/kubermatic/machine-controller/pull/813#" title="Author self-approved">xmudrii</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubermatic%2Fmachine-controller).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubermatic/machine-controller/blob/master/OWNERS)~~ [irozzo-1A,xmudrii]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
irozzo-1A: /lgtm
kubermatic-bot: LGTM label has been added.  <details>Git tree hash: 875072471baa1beb0bda5c2628e99d3abcea4841</details>
xmudrii: /hold cancel

Patch:
diff --git a/pkg/controller/machine/machine.go b/pkg/controller/machine/machine.go
index 8eba5d56..25cdeecb 100644
--- a/pkg/controller/machine/machine.go
+++ b/pkg/controller/machine/machine.go
@@ -608,19 +608,42 @@ func (r *Reconciler) deleteCloudProviderInstance(prov cloudprovidertypes.Provide
 }
 
 func (r *Reconciler) deleteNodeForMachine(machine *clusterv1alpha1.Machine) error {
-	selector, err := labels.Parse(NodeOwnerLabelName + "=" + string(machine.UID))
-	if err != nil {
-		return fmt.Errorf("failed to parse label selector: %v", err)
-	}
-	listOpts := &ctrlruntimeclient.ListOptions{LabelSelector: selector}
-	nodes := &corev1.NodeList{}
-	if err := r.client.List(r.ctx, nodes, listOpts); err != nil {
-		return fmt.Errorf("failed to list nodes: %v", err)
-	}
+	// If there's NodeRef on the Machine object, remove the Node by using the
+	// value of the NodeRef. If there's no NodeRef, try to find the Node by
+	// listing nodes using the NodeOwner label selector.
+	if machine.Status.NodeRef != nil {
+		objKey := ctrlruntimeclient.ObjectKey{Name: machine.Status.NodeRef.Name}
+		node := &corev1.Node{}
+		if err := r.client.Get(r.ctx, objKey, node); err != nil {
+			return fmt.Errorf("failed to get node %s: %v", machine.Status.NodeRef.Name, err)
+		}
+
+		if err := r.client.Delete(r.ctx, node); err != nil {
+			if !kerrors.IsNotFound(err) {
+				return err
+			}
+			klog.V(2).Infof("node %q does not longer exist for machine %q", machine.Status.NodeRef.Name, machine.Spec.Name)
+		}
+	} else {
+		selector, err := labels.Parse(NodeOwnerLabelName + "=" + string(machine.UID))
+		if err != nil {
+			return fmt.Errorf("failed to parse label selector: %v", err)
+		}
+		listOpts := &ctrlruntimeclient.ListOptions{LabelSelector: selector}
+		nodes := &corev1.NodeList{}
+		if err := r.client.List(r.ctx, nodes, listOpts); err != nil {
+			return fmt.Errorf("failed to list nodes: %v", err)
+		}
+		if len(nodes.Items) == 0 {
+			// We just want log that we didn't found the node. We don't want to
+			// return here, as we want to remove finalizers at the end.
+			klog.V(3).Infof("No node found for the machine %s", machine.Spec.Name)
+		}
 
-	for _, node := range nodes.Items {
-		if err := r.client.Delete(r.ctx, &node); err != nil {
-			return err
+		for _, node := range nodes.Items {
+			if err := r.client.Delete(r.ctx, &node); err != nil {
+				return err
+			}
 		}
 	}
 
@@ -888,8 +911,31 @@ func (r *Reconciler) getNode(instance instance.Instance, provider providerconfig
 				return node.DeepCopy(), true, nil
 			}
 		}
+		// If we were unable to find Node by ProviderID, fallback to IP address matching.
+		// This usually happens if there's no CCM deployed in the cluster.
+		//
+		// This mechanism is not always reliable, as providers reuse the IP addresses after
+		// some time.
+		//
+		// If we rollout a Machine, it can happen that a new instance has the same
+		// IP addresses as the instance that has just been deleted. If machine-controller
+		// processes the new Machine before removing the old Machine and the corresponding
+		// Node object, machine-controller could update the NodeOwner label on the old Node
+		// object to point to the Machine that just got created, as IP addresses would match.
+		// This causes machine-controller to fail to delete the old Node object, which could
+		// then cause cluster stability issues in some cases.
 		for _, nodeAddress := range node.Status.Addresses {
 			for instanceAddress := range instance.Addresses() {
+				// We observed that the issue described above happens often on Hetzner.
+				// As we know that the Node and the instance name will always be same
+				// on Hetzner, we can use it as an additional check to prevent this
+				// issue.
+				// TODO: We should do this for other providers, but there are providers where
+				// the node and the instance names will not match, so it requires further
+				// investigation (e.g. AWS).
+				if provider == providerconfigtypes.CloudProviderHetzner && node.Name != instance.Name() {
+					continue
+				}
 				if nodeAddress.Address == instanceAddress {
 					return node.DeepCopy(), true, nil
 				}


Test patch:
diff --git a/pkg/controller/machine/machine_test.go b/pkg/controller/machine/machine_test.go
index 42661ba0..24ba69cd 100644
--- a/pkg/controller/machine/machine_test.go
+++ b/pkg/controller/machine/machine_test.go
@@ -26,6 +26,7 @@ import (
 
 	clusterv1alpha1 "github.com/kubermatic/machine-controller/pkg/apis/cluster/v1alpha1"
 	"github.com/kubermatic/machine-controller/pkg/cloudprovider/instance"
+	cloudprovidertypes "github.com/kubermatic/machine-controller/pkg/cloudprovider/types"
 	providerconfigtypes "github.com/kubermatic/machine-controller/pkg/providerconfig/types"
 
 	corev1 "k8s.io/api/core/v1"
@@ -36,6 +37,7 @@ import (
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog"
+	ctrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client"
 	ctrlruntimefake "sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
@@ -99,7 +101,8 @@ func TestController_GetNode(t *testing.T) {
 	node1 := getTestNode("1", "aws")
 	node2 := getTestNode("2", "openstack")
 	node3 := getTestNode("3", "")
-	nodeList := []*corev1.Node{&node1, &node2, &node3}
+	node4 := getTestNode("4", "hetzner")
+	nodeList := []*corev1.Node{&node1, &node2, &node3, &node4}
 
 	tests := []struct {
 		name     string
@@ -149,6 +152,38 @@ func TestController_GetNode(t *testing.T) {
 			err:      nil,
 			instance: &fakeInstance{id: "3", addresses: map[string]corev1.NodeAddressType{"172.16.1.3": corev1.NodeInternalIP}},
 		},
+		{
+			name:     "hetzner node found by internal ip",
+			provider: "hetzner",
+			resNode:  &node3,
+			exists:   true,
+			err:      nil,
+			instance: &fakeInstance{id: "3", name: "node3", addresses: map[string]corev1.NodeAddressType{"192.168.1.3": corev1.NodeInternalIP}},
+		},
+		{
+			name:     "hetzner node found by external ip",
+			provider: "hetzner",
+			resNode:  &node3,
+			exists:   true,
+			err:      nil,
+			instance: &fakeInstance{id: "3", name: "node3", addresses: map[string]corev1.NodeAddressType{"172.16.1.3": corev1.NodeExternalIP}},
+		},
+		{
+			name:     "hetzner node not found - node and instance names mismatch",
+			provider: "hetzner",
+			resNode:  nil,
+			exists:   false,
+			err:      nil,
+			instance: &fakeInstance{id: "3", name: "instance3", addresses: map[string]corev1.NodeAddressType{"192.168.1.3": corev1.NodeInternalIP}},
+		},
+		{
+			name:     "hetzner node found by provider id",
+			provider: "hetzner",
+			resNode:  &node4,
+			exists:   true,
+			err:      nil,
+			instance: &fakeInstance{id: "4", addresses: map[string]corev1.NodeAddressType{"": ""}},
+		},
 	}
 
 	for _, test := range tests {
@@ -427,3 +462,137 @@ func TestControllerShouldEvict(t *testing.T) {
 		})
 	}
 }
+
+func TestControllerDeleteNodeForMachine(t *testing.T) {
+	machineUID := types.UID("test-1")
+
+	tests := []struct {
+		name             string
+		machine          *clusterv1alpha1.Machine
+		nodes            []runtime.Object
+		err              error
+		shouldDeleteNode string
+	}{
+		{
+			name: "delete node by nodeRef",
+			machine: &clusterv1alpha1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "machine-1",
+					Finalizers: []string{"machine-node-delete-finalizer"},
+				},
+				Status: clusterv1alpha1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{Name: "node-1"},
+				},
+			},
+			nodes: []runtime.Object{&corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node-0",
+				}}, &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node-1",
+				}},
+			},
+			err:              nil,
+			shouldDeleteNode: "node-1",
+		},
+		{
+			name: "delete node by NodeOwner label",
+			machine: &clusterv1alpha1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "machine-1",
+					Finalizers: []string{"machine-node-delete-finalizer"},
+					UID:        machineUID,
+				},
+				Status: clusterv1alpha1.MachineStatus{},
+			},
+			nodes: []runtime.Object{
+				&corev1.Node{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "node-0",
+						Labels: map[string]string{
+							NodeOwnerLabelName: string(machineUID),
+						},
+					},
+				},
+				&corev1.Node{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "node-1",
+					},
+				},
+			},
+			err:              nil,
+			shouldDeleteNode: "node-0",
+		},
+		{
+			name: "no node should be deleted",
+			machine: &clusterv1alpha1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "machine-1",
+					Finalizers: []string{"machine-node-delete-finalizer"},
+					UID:        machineUID,
+				},
+				Status: clusterv1alpha1.MachineStatus{},
+			},
+			nodes: []runtime.Object{
+				&corev1.Node{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "node-0",
+					},
+				},
+				&corev1.Node{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "node-1",
+					},
+				},
+			},
+			err:              nil,
+			shouldDeleteNode: "",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			objects := []runtime.Object{test.machine}
+			objects = append(objects, test.nodes...)
+
+			client := ctrlruntimefake.NewFakeClient(objects...)
+
+			ctx := context.TODO()
+			providerData := &cloudprovidertypes.ProviderData{
+				Ctx:    ctx,
+				Update: cloudprovidertypes.GetMachineUpdater(ctx, client),
+				Client: client,
+			}
+
+			reconciler := &Reconciler{
+				client:       client,
+				recorder:     &record.FakeRecorder{},
+				providerData: providerData,
+			}
+
+			err := reconciler.deleteNodeForMachine(test.machine)
+			if diff := deep.Equal(err, test.err); diff != nil {
+				t.Errorf("expected to get %v instead got: %v", test.err, err)
+			}
+			if err != nil {
+				return
+			}
+
+			if test.shouldDeleteNode != "" {
+				err = client.Get(ctx, types.NamespacedName{Name: test.shouldDeleteNode}, &corev1.Node{})
+				if !kerrors.IsNotFound(err) {
+					t.Errorf("expected node %q to be deleted, but got: %v", test.shouldDeleteNode, err)
+				}
+			} else {
+				nodes := &corev1.NodeList{}
+				err = client.List(ctx, nodes, &ctrlruntimeclient.ListOptions{})
+				if err != nil {
+					t.Errorf("error listing nodes: %v", err)
+				}
+				if len(test.nodes) != len(nodes.Items) {
+					t.Errorf("expected %d nodes, but got %d", len(test.nodes), len(nodes.Items))
+				}
+			}
+		})
+	}
+}