Fix ensure by provider id

pkg/controller/cloud/node_controller.go

@@ -438,21 +438,24 @@ func excludeTaintFromList(taints []v1.Taint, toExclude v1.Taint) []v1.Taint {
 	return newTaints
 }
 
-// ensureNodeExistsByProviderIDOrInstanceID first checks if the instance exists by the provider id and then by calling instance id with node name
+// ensureNodeExistsByProviderIDOrInstanceID checks if the instance exists by the provider id,
+// If provider id is empty it calls instanceId with node name to get provider id
 func ensureNodeExistsByProviderIDOrInstanceID(instances cloudprovider.Instances, node *v1.Node) (bool, error) {
-	exists, err := instances.InstanceExistsByProviderID(context.TODO(), node.Spec.ProviderID)
+	providerID := node.Spec.ProviderID
+	if providerID == "" {
+		var err error
+		providerID, err = instances.InstanceID(context.TODO(), types.NodeName(node.Name))
 		if err != nil {
-		providerIDErr := err
+			return false, err
-		_, err = instances.InstanceID(context.TODO(), types.NodeName(node.Name))
+		}
-		//TODO(anupn): Changing the check as InstanceID does not return error
+
-		if err == nil {
+		if providerID == "" {
+			glog.Warningf("Cannot find valid providerID for node name %q, assuming non existence", node.Name)
 			return false, nil
 		}
-
-		return false, fmt.Errorf("InstanceExistsByProviderID: Error fetching by providerID: %v Error fetching by NodeName: %v", providerIDErr, err)
 	}
 
-	return exists, nil
+	return instances.InstanceExistsByProviderID(context.TODO(), providerID)
 }
 
 func getNodeAddressesByProviderIDOrName(instances cloudprovider.Instances, node *v1.Node) ([]v1.NodeAddress, error) {

pkg/controller/cloud/node_controller_test.go

@@ -41,13 +41,14 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
+func TestEnsureNodeExistsByProviderIDOrInstanceID(t *testing.T) {
 
 	testCases := []struct {
 		testName           string
 		node               *v1.Node
 		expectedCalls      []string
-		existsByNodeName   bool
+		expectedNodeExists bool
+		hasInstanceID      bool
 		existsByProviderID bool
 		nodeNameErr        error
 		providerIDErr      error
@@ -56,9 +57,10 @@ func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
 			testName:           "node exists by provider id",
 			existsByProviderID: true,
 			providerIDErr:      nil,
-			existsByNodeName:   false,
+			hasInstanceID:      true,
 			nodeNameErr:        errors.New("unimplemented"),
 			expectedCalls:      []string{"instance-exists-by-provider-id"},
+			expectedNodeExists: true,
 			node: &v1.Node{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "node0",
@@ -72,9 +74,10 @@ func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
 			testName:           "does not exist by provider id",
 			existsByProviderID: false,
 			providerIDErr:      nil,
-			existsByNodeName:   false,
+			hasInstanceID:      true,
 			nodeNameErr:        errors.New("unimplemented"),
 			expectedCalls:      []string{"instance-exists-by-provider-id"},
+			expectedNodeExists: false,
 			node: &v1.Node{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "node0",
@@ -85,28 +88,41 @@ func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
 			},
 		},
 		{
-			testName:           "node exists by node name",
+			testName:           "exists by instance id",
-			existsByProviderID: false,
+			existsByProviderID: true,
-			providerIDErr:      errors.New("unimplemented"),
+			providerIDErr:      nil,
-			existsByNodeName:   true,
+			hasInstanceID:      true,
 			nodeNameErr:        nil,
-			expectedCalls:      []string{"instance-exists-by-provider-id", "instance-id"},
+			expectedCalls:      []string{"instance-id", "instance-exists-by-provider-id"},
+			expectedNodeExists: true,
 			node: &v1.Node{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "node0",
 				},
-				Spec: v1.NodeSpec{
+			},
-					ProviderID: "node0",
+		},
+		{
+			testName:           "does not exist by no instance id",
+			existsByProviderID: true,
+			providerIDErr:      nil,
+			hasInstanceID:      false,
+			nodeNameErr:        cloudprovider.InstanceNotFound,
+			expectedCalls:      []string{"instance-id"},
+			expectedNodeExists: false,
+			node: &v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "node0",
 				},
 			},
 		},
 		{
-			testName:           "does not exist by node name",
+			testName:           "provider id returns error",
 			existsByProviderID: false,
 			providerIDErr:      errors.New("unimplemented"),
-			existsByNodeName:   false,
+			hasInstanceID:      true,
 			nodeNameErr:        cloudprovider.InstanceNotFound,
-			expectedCalls:      []string{"instance-exists-by-provider-id", "instance-id"},
+			expectedCalls:      []string{"instance-exists-by-provider-id"},
+			expectedNodeExists: false,
 			node: &v1.Node{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "node0",
@@ -121,28 +137,29 @@ func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.testName, func(t *testing.T) {
 			fc := &fakecloud.FakeCloud{
-				Exists:             tc.existsByNodeName,
 				ExistsByProviderID: tc.existsByProviderID,
 				Err:                tc.nodeNameErr,
 				ErrByProviderID:    tc.providerIDErr,
 			}
 
+			if tc.hasInstanceID {
+				fc.ExtID = map[types.NodeName]string{
+					types.NodeName(tc.node.Name): "provider-id://a",
+				}
+			}
+
 			instances, _ := fc.Instances()
 			exists, err := ensureNodeExistsByProviderIDOrInstanceID(instances, tc.node)
+			if tc.providerIDErr == nil {
 				assert.NoError(t, err)
+			}
 			assert.EqualValues(t, tc.expectedCalls, fc.Calls,
 				"expected cloud provider methods `%v` to be called but `%v` was called ",
 				tc.expectedCalls, fc.Calls)
 
-			assert.False(t, tc.existsByProviderID && tc.existsByProviderID != exists,
+			assert.Equal(t, tc.expectedNodeExists, exists,
-				"expected exist by provider id to be `%t` but got `%t`",
+				"expected exists to be `%t` but got `%t`",
 				tc.existsByProviderID, exists)
-
-			assert.False(t, tc.existsByNodeName && tc.existsByNodeName == exists,
-				"expected exist by node name to be `%t` but got `%t`", tc.existsByNodeName, exists)
-
-			assert.False(t, !tc.existsByNodeName && !tc.existsByProviderID && exists,
-				"node is not supposed to exist")
 		})
 	}