Merge pull request #63227 from karataliu/nodec

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix ensure by provider id

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #63226

**Special notes for your reviewer**:

cc @adnavare 

**Release note**:

```release-note
NONE
```

pkg/controller/cloud/node_controller.go

@@ -154,7 +154,7 @@ func (cnc *CloudNodeController) updateNodeAddress(node *v1.Node, instances cloud
 		return
 	}
 	// Node that isn't present according to the cloud provider shouldn't have its address updated
-	exists, err := ensureNodeExistsByProviderIDOrInstanceID(instances, node)
+	exists, err := ensureNodeExistsByProviderID(instances, node)
 	if err != nil {
 		// Continue to update node address when not sure the node is not exists
 		glog.Errorf("%v", err)
@@ -265,7 +265,7 @@ func (cnc *CloudNodeController) MonitorNode() {
 
 				// Check with the cloud provider to see if the node still exists. If it
 				// doesn't, delete the node immediately.
-				exists, err := ensureNodeExistsByProviderIDOrInstanceID(instances, node)
+				exists, err := ensureNodeExistsByProviderID(instances, node)
 				if err != nil {
 					glog.Errorf("Error getting data for node %s from cloud: %v", node.Name, err)
 					continue
@@ -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
+// ensureNodeExistsByProviderID checks if the instance exists by the provider id,
-func ensureNodeExistsByProviderIDOrInstanceID(instances cloudprovider.Instances, node *v1.Node) (bool, error) {
+// If provider id in spec is empty it calls instanceId with node name to get provider id
-	exists, err := instances.InstanceExistsByProviderID(context.TODO(), node.Spec.ProviderID)
+func ensureNodeExistsByProviderID(instances cloudprovider.Instances, node *v1.Node) (bool, error) {
+	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 TestEnsureNodeExistsByProviderID(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,28 @@ 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)
+			exists, err := ensureNodeExistsByProviderID(instances, tc.node)
-			assert.NoError(t, err)
+			assert.Equal(t, err, tc.providerIDErr)
+
 			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")
 		})
 	}