UPSTREAM: <carry>: openshift: drop machine annotation linkage

This changes the mapping between a node and a machine to rely on the
presence of the ProviderID rather than the "machine" annotation
currently added by the nodelink-controller.

Note: this should not merge until we update the AWS, libvirt and
kubemark actuators - they should ensure that ProviderID is set, if not
already, on the machine object.

cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_controller.go

@@ -33,7 +33,7 @@ import (
 )
 
 const (
-	nodeProviderIDIndex = "openshiftmachineapi-nodeProviderIDIndex"
+	machineProviderIDIndex = "openshiftmachineapi-machineProviderIDIndex"
 )
 
 // machineController watches for Nodes, Machines, MachineSets and
@@ -53,9 +53,12 @@ type machineController struct {
 
 type machineSetFilterFunc func(machineSet *v1beta1.MachineSet) error
 
-func indexNodeByNodeProviderID(obj interface{}) ([]string, error) {
-	if node, ok := obj.(*apiv1.Node); ok {
-		return []string{node.Spec.ProviderID}, nil
+func indexMachineByProviderID(obj interface{}) ([]string, error) {
+	if machine, ok := obj.(*v1beta1.Machine); ok {
+		if machine.Spec.ProviderID != nil && *machine.Spec.ProviderID != "" {
+			return []string{*machine.Spec.ProviderID}, nil
+		}
+		return []string{}, nil
 	}
 	return []string{}, nil
 }
@@ -155,7 +158,7 @@ func (c *machineController) run(stopCh <-chan struct{}) error {
 // node.Spec.ProviderID cannot be found or if the node has no machine
 // annotation. A DeepCopy() of the object is returned on success.
 func (c *machineController) findMachineByNodeProviderID(node *apiv1.Node) (*v1beta1.Machine, error) {
-	objs, err := c.nodeInformer.GetIndexer().ByIndex(nodeProviderIDIndex, node.Spec.ProviderID)
+	objs, err := c.machineInformer.Informer().GetIndexer().ByIndex(machineProviderIDIndex, node.Spec.ProviderID)
 	if err != nil {
 		return nil, err
 	}
@@ -167,16 +170,12 @@ func (c *machineController) findMachineByNodeProviderID(node *apiv1.Node) (*v1be
 		return nil, fmt.Errorf("internal error; expected len==1, got %v", n)
 	}
 
-	node, ok := objs[0].(*apiv1.Node)
+	machine, ok := objs[0].(*v1beta1.Machine)
 	if !ok {
-		return nil, fmt.Errorf("internal error; unexpected type %T", node)
-	}
-
-	if machineName, found := node.Annotations[machineAnnotationKey]; found {
-		return c.findMachine(machineName)
+		return nil, fmt.Errorf("internal error; unexpected type %T", machine)
 	}
 
-	return nil, nil
+	return machine.DeepCopy(), nil
 }
 
 // findNodeByNodeName find the Node object keyed by node.Name. Returns
@@ -247,12 +246,10 @@ func newMachineController(
 	nodeInformer := kubeInformerFactory.Core().V1().Nodes().Informer()
 	nodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{})
 
-	indexerFuncs := cache.Indexers{
-		nodeProviderIDIndex: indexNodeByNodeProviderID,
-	}
-
-	if err := nodeInformer.GetIndexer().AddIndexers(indexerFuncs); err != nil {
-		return nil, fmt.Errorf("cannot add indexers: %v", err)
+	if err := machineInformer.Informer().GetIndexer().AddIndexers(cache.Indexers{
+		machineProviderIDIndex: indexMachineByProviderID,
+	}); err != nil {
+		return nil, fmt.Errorf("cannot add machine indexer: %v", err)
 	}
 
 	return &machineController{

cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_controller_test.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	fakekube "k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/pointer"
 )
 
 type testControllerShutdownFunc func()
@@ -200,9 +201,6 @@ func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference)
 		},
 		ObjectMeta: v1.ObjectMeta{
 			Name: fmt.Sprintf("%s-%s-node-%d", namespace, owner.Name, i),
-			Annotations: map[string]string{
-				machineAnnotationKey: fmt.Sprintf("%s/%s-%s-machine-%d", namespace, namespace, owner.Name, i),
-			},
 		},
 		Spec: apiv1.NodeSpec{
 			ProviderID: fmt.Sprintf("%s-%s-nodeid-%d", namespace, owner.Name, i),
@@ -222,6 +220,9 @@ func makeLinkedNodeAndMachine(i int, namespace string, owner v1.OwnerReference)
 				UID:  owner.UID,
 			}},
 		},
+		Spec: v1beta1.MachineSpec{
+			ProviderID: pointer.StringPtr(fmt.Sprintf("%s-%s-nodeid-%d", namespace, owner.Name, i)),
+		},
 		Status: v1beta1.MachineStatus{
 			NodeRef: &apiv1.ObjectReference{
 				Kind: node.Kind,
@@ -406,7 +407,8 @@ func TestControllerFindMachineByNodeProviderID(t *testing.T) {
 	defer stop()
 
 	// Test #1: Verify node can be found because it has a
-	// ProviderID value and a machine annotation.
+	// ProviderID value and the machine has a matching ProviderID
+	// value.
 	machine, err := controller.findMachineByNodeProviderID(testConfig.nodes[0])
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -418,7 +420,7 @@ func TestControllerFindMachineByNodeProviderID(t *testing.T) {
 		t.Fatalf("expected machines to be equal - expected %+v, got %+v", testConfig.machines[0], machine)
 	}
 
-	// Test #2: Verify node is not found if it has a non-existent ProviderID
+	// Test #2: Verify machine is not found if node has no ProviderID
 	node := testConfig.nodes[0].DeepCopy()
 	node.Spec.ProviderID = ""
 	nonExistentMachine, err := controller.findMachineByNodeProviderID(node)
@@ -429,11 +431,11 @@ func TestControllerFindMachineByNodeProviderID(t *testing.T) {
 		t.Fatal("expected find to fail")
 	}
 
-	// Test #3: Verify node is not found if the stored object has
-	// no "machine" annotation
-	node = testConfig.nodes[0].DeepCopy()
-	delete(node.Annotations, machineAnnotationKey)
-	if err := controller.nodeInformer.GetStore().Update(node); err != nil {
+	// Test #3: Verify machine is not found if underlying machine
+	// has no ProviderID value.
+	machine = testConfig.machines[0].DeepCopy()
+	machine.Spec.ProviderID = nil
+	if err := controller.machineInformer.Informer().GetStore().Update(machine); err != nil {
 		t.Fatalf("unexpected error updating node, got %v", err)
 	}
 	nonExistentMachine, err = controller.findMachineByNodeProviderID(testConfig.nodes[0])

cluster-autoscaler/cloudprovider/openshiftmachineapi/machineapi_nodegroup.go

@@ -29,7 +29,6 @@ import (
 
 const (
 	machineDeleteAnnotationKey = "machine.openshift.io/cluster-api-delete-machine"
-	machineAnnotationKey       = "machine.openshift.io/machine"
 	debugFormat                = "%s (min: %d, max: %d, replicas: %d)"
 )