diff --git a/pkg/controller/cloud/node_controller.go b/pkg/controller/cloud/node_controller.go index c10f69bb5d6..5ecf29a8357 100644 --- a/pkg/controller/cloud/node_controller.go +++ b/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 -func ensureNodeExistsByProviderIDOrInstanceID(instances cloudprovider.Instances, node *v1.Node) (bool, error) { - exists, err := instances.InstanceExistsByProviderID(context.TODO(), node.Spec.ProviderID) - if err != nil { - providerIDErr := err - _, err = instances.InstanceID(context.TODO(), types.NodeName(node.Name)) - //TODO(anupn): Changing the check as InstanceID does not return error - if err == nil { - return false, nil +// ensureNodeExistsByProviderID checks if the instance exists by the provider id, +// If provider id in spec is empty it calls instanceId with node name to get provider id +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 { + return false, err } - return false, fmt.Errorf("InstanceExistsByProviderID: Error fetching by providerID: %v Error fetching by NodeName: %v", providerIDErr, err) + if providerID == "" { + glog.Warningf("Cannot find valid providerID for node name %q, assuming non existence", node.Name) + return false, nil + } } - return exists, nil + return instances.InstanceExistsByProviderID(context.TODO(), providerID) } func getNodeAddressesByProviderIDOrName(instances cloudprovider.Instances, node *v1.Node) ([]v1.NodeAddress, error) { diff --git a/pkg/controller/cloud/node_controller_test.go b/pkg/controller/cloud/node_controller_test.go index 019b68d6a1b..89fb565e717 100644 --- a/pkg/controller/cloud/node_controller_test.go +++ b/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", - existsByProviderID: false, - providerIDErr: errors.New("unimplemented"), - existsByNodeName: true, + testName: "exists by instance id", + existsByProviderID: true, + providerIDErr: nil, + 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) - assert.NoError(t, err) + exists, err := ensureNodeExistsByProviderID(instances, tc.node) + 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, - "expected exist by provider id to be `%t` but got `%t`", + assert.Equal(t, tc.expectedNodeExists, exists, + "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") }) }