From 050b583b96fe7892b251381b3791c32293ff00b1 Mon Sep 17 00:00:00 2001 From: Dong Liu Date: Fri, 27 Apr 2018 11:26:40 +0800 Subject: [PATCH 1/3] Fix ensure by provider id --- pkg/controller/cloud/node_controller.go | 23 ++++--- pkg/controller/cloud/node_controller_test.go | 65 ++++++++++++-------- 2 files changed, 54 insertions(+), 34 deletions(-) diff --git a/pkg/controller/cloud/node_controller.go b/pkg/controller/cloud/node_controller.go index c10f69bb5d6..94b794f78d3 100644 --- a/pkg/controller/cloud/node_controller.go +++ b/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) - 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 + 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..96a8a36d6f2 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 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", - 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,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) - assert.NoError(t, err) + 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, - "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") }) } From f0046a71949b6908908f4b597b67c8eeccb27f1f Mon Sep 17 00:00:00 2001 From: Dong Liu Date: Sat, 28 Apr 2018 13:43:29 +0800 Subject: [PATCH 2/3] Rename func to ensureNodeExistsByProviderID --- pkg/controller/cloud/node_controller.go | 10 +++++----- pkg/controller/cloud/node_controller_test.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/controller/cloud/node_controller.go b/pkg/controller/cloud/node_controller.go index 94b794f78d3..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,9 +438,9 @@ func excludeTaintFromList(taints []v1.Taint, toExclude v1.Taint) []v1.Taint { return newTaints } -// 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) { +// 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 diff --git a/pkg/controller/cloud/node_controller_test.go b/pkg/controller/cloud/node_controller_test.go index 96a8a36d6f2..d26401383ba 100644 --- a/pkg/controller/cloud/node_controller_test.go +++ b/pkg/controller/cloud/node_controller_test.go @@ -41,7 +41,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestEnsureNodeExistsByProviderIDOrInstanceID(t *testing.T) { +func TestEnsureNodeExistsByProviderID(t *testing.T) { testCases := []struct { testName string @@ -149,7 +149,7 @@ func TestEnsureNodeExistsByProviderIDOrInstanceID(t *testing.T) { } instances, _ := fc.Instances() - exists, err := ensureNodeExistsByProviderIDOrInstanceID(instances, tc.node) + exists, err := ensureNodeExistsByProviderID(instances, tc.node) if tc.providerIDErr == nil { assert.NoError(t, err) } From 9a6319b23cf1a2b21154ff3603df1b4fb2c5670b Mon Sep 17 00:00:00 2001 From: Dong Liu Date: Thu, 3 May 2018 10:22:20 +0800 Subject: [PATCH 3/3] Update error assertation --- pkg/controller/cloud/node_controller_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/controller/cloud/node_controller_test.go b/pkg/controller/cloud/node_controller_test.go index d26401383ba..89fb565e717 100644 --- a/pkg/controller/cloud/node_controller_test.go +++ b/pkg/controller/cloud/node_controller_test.go @@ -150,9 +150,8 @@ func TestEnsureNodeExistsByProviderID(t *testing.T) { instances, _ := fc.Instances() exists, err := ensureNodeExistsByProviderID(instances, tc.node) - if tc.providerIDErr == nil { - 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)