gcp-controller: don't make assumptions on PodCIDRs

The gcp-controller, to avoid issues with preemtible VMs, detect
when a VM has changed and delete all Pods associated to avoid
Kubernetes controllers think this was a restart, when it actually
was a completely replacement of the VM.

The heuristic for detecting a Node change are based on:
- the instance ID, that is unique for each instance
- the pod.Spec.PodCIDRs value

The logic for the PodCIDR value is not easy to generalize and current
implementation is based on a specific implementation and configuration.
In addition, the pod.Spec.PodCIDRs field is inmutable once set, so
it can only change if the VM is new, ie. the instanceID has changed, so
no need to duplicate the logic.

Change-Id: Ic39996f473682973c9ac6c81bfa10a14dd473b77
Signed-off-by: Antonio Ojea <[email protected]>

Author: aojea

cmd/gcp-controller-manager/node_csr_approver.go

@@ -963,27 +963,6 @@ func shouldDeleteNode(ctx *controllerContext, node *v1.Node, getInstance func(*c
 			return true, nil
 		}
 	}
-	// Newly created node might not have pod CIDR allocated yet.
-	if node.Spec.PodCIDR == "" {
-		klog.V(2).Infof("Node %q has empty podCIDR.", node.Name)
-		return false, nil
-	}
-	var unmatchedRanges []string
-	for _, networkInterface := range inst.NetworkInterfaces {
-		for _, r := range networkInterface.AliasIpRanges {
-			if node.Spec.PodCIDR == r.IpCidrRange {
-				klog.V(2).Infof("Instance %q has alias range that matches node's podCIDR.", inst.Name)
-				return false, nil
-			}
-			unmatchedRanges = append(unmatchedRanges, r.IpCidrRange)
-		}
-	}
-	if len(unmatchedRanges) != 0 {
-		klog.Warningf("Instance %q has alias range(s) %v and none of them match node's podCIDR %s, will trigger node deletion.", inst.Name, unmatchedRanges, node.Spec.PodCIDR)
-		return true, nil
-	}
-	// Instance with no alias range is route based, for which node object deletion is unnecessary.
-	klog.V(2).Infof("Instance %q has no alias range.", inst.Name)
 	return false, nil
 }
 

cmd/gcp-controller-manager/node_csr_approver_test.go

@@ -1252,53 +1252,6 @@ func TestShouldDeleteNode(t *testing.T) {
 		getInstanceErr error
 		expectedErr    error
 	}{
-		{
-			desc: "instance with 1 alias range and matches podCIDR",
-			ctx:  &controllerContext{},
-			node: &v1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "node-test",
-				},
-				Spec: v1.NodeSpec{
-					PodCIDR: "10.0.0.1/24",
-				},
-			},
-			instance: &compute.Instance{
-				NetworkInterfaces: []*compute.NetworkInterface{
-					{
-						AliasIpRanges: []*compute.AliasIpRange{
-							{
-								IpCidrRange: "10.0.0.1/24",
-							},
-						},
-					},
-				},
-			},
-		},
-		{
-			desc: "instance with 1 alias range doesn't match podCIDR",
-			ctx:  &controllerContext{},
-			node: &v1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "node-test",
-				},
-				Spec: v1.NodeSpec{
-					PodCIDR: "10.0.0.1/24",
-				},
-			},
-			instance: &compute.Instance{
-				NetworkInterfaces: []*compute.NetworkInterface{
-					{
-						AliasIpRanges: []*compute.AliasIpRange{
-							{
-								IpCidrRange: "10.0.0.2/24",
-							},
-						},
-					},
-				},
-			},
-			shouldDelete: true,
-		},
 		{
 			desc: "instance with 2 alias range and 1 matches podCIDR",
 			ctx:  &controllerContext{},
@@ -1399,13 +1352,15 @@ func TestShouldDeleteNode(t *testing.T) {
 		},
 	}
 	for _, c := range cases {
-		fakeGetInstance := func(_ *controllerContext, _ string) (*compute.Instance, error) {
-			return c.instance, c.getInstanceErr
-		}
-		shouldDelete, err := shouldDeleteNode(c.ctx, c.node, fakeGetInstance)
-		if err != c.expectedErr || shouldDelete != c.shouldDelete {
-			t.Errorf("%s: shouldDeleteNode=(%v, %v), want (%v, %v)", c.desc, shouldDelete, err, c.shouldDelete, c.expectedErr)
-		}
+		t.Run(c.desc, func(t *testing.T) {
+			fakeGetInstance := func(_ *controllerContext, _ string) (*compute.Instance, error) {
+				return c.instance, c.getInstanceErr
+			}
+			shouldDelete, err := shouldDeleteNode(c.ctx, c.node, fakeGetInstance)
+			if err != c.expectedErr || shouldDelete != c.shouldDelete {
+				t.Errorf("%s: shouldDeleteNode=(%v, %v), want (%v, %v)", c.desc, shouldDelete, err, c.shouldDelete, c.expectedErr)
+			}
+		})
 	}
 }