Merge pull request #60692 from adnavare/bug/60466

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>.

Cleanup the use of ExternalID as it is deprecated

The patch removes ExternalID usage from node_controller
and node_lifecycle_oontroller. The code instead uses InstanceID
which returns the cloud provider ID as well.

fixes #60466

pkg/cloudprovider/cloud.go

@@ -130,9 +130,6 @@ type Instances interface {
 	// from the node whose nodeaddresses are being queried. i.e. local metadata
 	// services cannot be used in this method to obtain nodeaddresses
 	NodeAddressesByProviderID(ctx context.Context, providerID string) ([]v1.NodeAddress, error)
-	// ExternalID returns the cloud provider ID of the node with the specified NodeName.
-	// Note that if the instance does not exist or is no longer running, we must return ("", cloudprovider.InstanceNotFound)
-	ExternalID(ctx context.Context, nodeName types.NodeName) (string, error)
 	// InstanceID returns the cloud provider ID of the node with the specified NodeName.
 	InstanceID(ctx context.Context, nodeName types.NodeName) (string, error)
 	// InstanceType returns the type of the specified instance.

pkg/cloudprovider/providers/aws/aws.go

@@ -1318,24 +1318,6 @@ func (c *Cloud) NodeAddressesByProviderID(ctx context.Context, providerID string
 	return extractNodeAddresses(instance)
 }
 
-// ExternalID returns the cloud provider ID of the node with the specified nodeName (deprecated).
-func (c *Cloud) ExternalID(ctx context.Context, nodeName types.NodeName) (string, error) {
-	if c.selfAWSInstance.nodeName == nodeName {
-		// We assume that if this is run on the instance itself, the instance exists and is alive
-		return c.selfAWSInstance.awsID, nil
-	}
-	// We must verify that the instance still exists
-	// Note that if the instance does not exist or is no longer running, we must return ("", cloudprovider.InstanceNotFound)
-	instance, err := c.findInstanceByNodeName(nodeName)
-	if err != nil {
-		return "", err
-	}
-	if instance == nil {
-		return "", cloudprovider.InstanceNotFound
-	}
-	return aws.StringValue(instance.InstanceId), nil
-}
-
 // InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.
 // If false is returned with no error, the instance will be immediately deleted by the cloud controller manager.
 func (c *Cloud) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {

pkg/cloudprovider/providers/azure/azure_instances.go

@@ -96,11 +96,6 @@ func (az *Cloud) NodeAddressesByProviderID(ctx context.Context, providerID strin
 	return az.NodeAddresses(ctx, name)
 }
 
-// ExternalID returns the cloud provider ID of the specified instance (deprecated).
-func (az *Cloud) ExternalID(ctx context.Context, name types.NodeName) (string, error) {
-	return az.InstanceID(ctx, name)
-}
-
 // InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.
 // If false is returned with no error, the instance will be immediately deleted by the cloud controller manager.
 func (az *Cloud) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {

pkg/cloudprovider/providers/cloudstack/cloudstack_instances.go

@@ -80,11 +80,6 @@ func (cs *CSCloud) nodeAddresses(instance *cloudstack.VirtualMachine) ([]v1.Node
 	return addresses, nil
 }
 
-// ExternalID returns the cloud provider ID of the specified instance (deprecated).
-func (cs *CSCloud) ExternalID(ctx context.Context, name types.NodeName) (string, error) {
-	return cs.InstanceID(ctx, name)
-}
-
 // InstanceID returns the cloud provider ID of the specified instance.
 func (cs *CSCloud) InstanceID(ctx context.Context, name types.NodeName) (string, error) {
 	instance, count, err := cs.client.VirtualMachine.GetVirtualMachineByName(

pkg/cloudprovider/providers/cloudstack/metadata.go

@@ -69,11 +69,6 @@ func (m *metadata) NodeAddressesByProviderID(ctx context.Context, providerID str
 	return nil, errors.New("NodeAddressesByProviderID not implemented")
 }
 
-// ExternalID returns the cloud provider ID of the specified instance (deprecated).
-func (m *metadata) ExternalID(ctx context.Context, name types.NodeName) (string, error) {
-	return m.InstanceID(ctx, name)
-}
-
 // InstanceID returns the cloud provider ID of the specified instance.
 func (m *metadata) InstanceID(ctx context.Context, name types.NodeName) (string, error) {
 	instanceID, err := m.get(metadataTypeInstanceID)

pkg/cloudprovider/providers/fake/fake.go

@@ -208,14 +208,6 @@ func (f *FakeCloud) NodeAddressesByProviderID(ctx context.Context, providerID st
 	return f.Addresses, f.Err
 }
 
-// ExternalID is a test-spy implementation of Instances.ExternalID.
-// It adds an entry "external-id" into the internal method call record.
-// It returns an external id to the mapped instance name, if not found, it will return "ext-{instance}"
-func (f *FakeCloud) ExternalID(ctx context.Context, nodeName types.NodeName) (string, error) {
-	f.addCall("external-id")
-	return f.ExtID[nodeName], f.Err
-}
-
 // InstanceID returns the cloud provider ID of the node with the specified Name.
 func (f *FakeCloud) InstanceID(ctx context.Context, nodeName types.NodeName) (string, error) {
 	f.addCall("instance-id")

pkg/cloudprovider/providers/openstack/openstack_instances.go

@@ -110,18 +110,6 @@ func (i *Instances) NodeAddressesByProviderID(ctx context.Context, providerID st
 	return addresses, nil
 }
 
-// ExternalID returns the cloud provider ID of the specified instance (deprecated).
-func (i *Instances) ExternalID(ctx context.Context, name types.NodeName) (string, error) {
-	srv, err := getServerByName(i.compute, name)
-	if err != nil {
-		if err == ErrNotFound {
-			return "", cloudprovider.InstanceNotFound
-		}
-		return "", err
-	}
-	return srv.ID, nil
-}
-
 // InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.
 // If false is returned with no error, the instance will be immediately deleted by the cloud controller manager.
 func (i *Instances) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {

pkg/cloudprovider/providers/ovirt/ovirt.go

@@ -196,16 +196,6 @@ func mapNodeNameToInstanceName(nodeName types.NodeName) string {
 	return string(nodeName)
 }
 
-// ExternalID returns the cloud provider ID of the specified node with the specified NodeName (deprecated).
-func (v *OVirtCloud) ExternalID(ctx context.Context, nodeName types.NodeName) (string, error) {
-	name := mapNodeNameToInstanceName(nodeName)
-	instance, err := v.fetchInstance(name)
-	if err != nil {
-		return "", err
-	}
-	return instance.UUID, nil
-}
-
 // InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.
 // If false is returned with no error, the instance will be immediately deleted by the cloud controller manager.
 func (v *OVirtCloud) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {

pkg/cloudprovider/providers/photon/photon.go

@@ -454,23 +454,6 @@ func getInstanceID(pc *PCCloud, name string) (string, error) {
 	return vmID, err
 }
 
-// ExternalID returns the cloud provider ID of the specified instance (deprecated).
-func (pc *PCCloud) ExternalID(ctx context.Context, nodeName k8stypes.NodeName) (string, error) {
-	name := string(nodeName)
-	if name == pc.localK8sHostname {
-		return pc.localInstanceID, nil
-	} else {
-		// We assume only master need to get InstanceID of a node other than itself
-		ID, err := getInstanceID(pc, name)
-		if err != nil {
-			glog.Errorf("Photon Cloud Provider: getInstanceID failed for ExternalID. Error[%v]", err)
-			return ID, err
-		} else {
-			return ID, nil
-		}
-	}
-}
-
 // InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.
 // If false is returned with no error, the instance will be immediately deleted by the cloud controller manager.
 func (pc *PCCloud) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {

pkg/cloudprovider/providers/photon/photon_test.go

@@ -133,22 +133,7 @@ func TestInstances(t *testing.T) {
 		t.Fatalf("Instances() returned false")
 	}
 
-	externalId, err := i.ExternalID(context.TODO(), NodeName)
-	if err != nil {
-		t.Fatalf("Instances.ExternalID(%s) failed: %s", testVM, err)
-	}
-	t.Logf("Found ExternalID(%s) = %s\n", testVM, externalId)
-
 	nonExistingVM := types.NodeName(rand.String(15))
-	externalId, err = i.ExternalID(context.TODO(), nonExistingVM)
-	if err == cloudprovider.InstanceNotFound {
-		t.Logf("VM %s was not found as expected\n", nonExistingVM)
-	} else if err == nil {
-		t.Fatalf("Instances.ExternalID did not fail as expected, VM %s was found", nonExistingVM)
-	} else {
-		t.Fatalf("Instances.ExternalID did not fail as expected, err: %v", err)
-	}
-
 	instanceId, err := i.InstanceID(context.TODO(), NodeName)
 	if err != nil {
 		t.Fatalf("Instances.InstanceID(%s) failed: %s", testVM, err)

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -577,11 +577,6 @@ func convertToK8sType(vmName string) k8stypes.NodeName {
 	return k8stypes.NodeName(vmName)
 }
 
-// ExternalID returns the cloud provider ID of the node with the specified Name (deprecated).
-func (vs *VSphere) ExternalID(ctx context.Context, nodeName k8stypes.NodeName) (string, error) {
-	return vs.InstanceID(ctx, nodeName)
-}
-
 // InstanceExistsByProviderID returns true if the instance with the given provider id still exists and is running.
 // If false is returned with no error, the instance will be immediately deleted by the cloud controller manager.
 func (vs *VSphere) InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error) {

pkg/cloudprovider/providers/vsphere/vsphere_test.go

@@ -174,22 +174,7 @@ func TestInstances(t *testing.T) {
 		t.Fatalf("CurrentNodeName() failed: %s", err)
 	}
 
-	externalID, err := i.ExternalID(context.TODO(), nodeName)
-	if err != nil {
-		t.Fatalf("Instances.ExternalID(%s) failed: %s", nodeName, err)
-	}
-	t.Logf("Found ExternalID(%s) = %s\n", nodeName, externalID)
-
 	nonExistingVM := types.NodeName(rand.String(15))
-	externalID, err = i.ExternalID(context.TODO(), nonExistingVM)
-	if err == cloudprovider.InstanceNotFound {
-		t.Logf("VM %s was not found as expected\n", nonExistingVM)
-	} else if err == nil {
-		t.Fatalf("Instances.ExternalID did not fail as expected, VM %s was found", nonExistingVM)
-	} else {
-		t.Fatalf("Instances.ExternalID did not fail as expected, err: %v", err)
-	}
-
 	instanceID, err := i.InstanceID(context.TODO(), nodeName)
 	if err != nil {
 		t.Fatalf("Instances.InstanceID(%s) failed: %s", nodeName, err)

pkg/controller/cloud/node_controller.go

@@ -417,11 +417,9 @@ func ensureNodeExistsByProviderIDOrExternalID(instances cloudprovider.Instances,
 	exists, err := instances.InstanceExistsByProviderID(context.TODO(), node.Spec.ProviderID)
 	if err != nil {
 		providerIDErr := err
-		_, err = instances.ExternalID(context.TODO(), types.NodeName(node.Name))
+		_, err = instances.InstanceID(context.TODO(), types.NodeName(node.Name))
+		//<anupn> Changing the check as InstanceID does not return error
 		if err == nil {
-			return true, nil
-		}
-		if err == cloudprovider.InstanceNotFound {
 			return false, nil
 		}
 

pkg/controller/cloud/node_controller_test.go

@@ -90,7 +90,7 @@ func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
 			providerIDErr:      errors.New("unimplemented"),
 			existsByNodeName:   true,
 			nodeNameErr:        nil,
-			expectedCalls:      []string{"instance-exists-by-provider-id", "external-id"},
+			expectedCalls:      []string{"instance-exists-by-provider-id", "instance-id"},
 			node: &v1.Node{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "node0",
@@ -106,7 +106,7 @@ func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
 			providerIDErr:      errors.New("unimplemented"),
 			existsByNodeName:   false,
 			nodeNameErr:        cloudprovider.InstanceNotFound,
-			expectedCalls:      []string{"instance-exists-by-provider-id", "external-id"},
+			expectedCalls:      []string{"instance-exists-by-provider-id", "instance-id"},
 			node: &v1.Node{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "node0",
@@ -138,7 +138,7 @@ func TestEnsureNodeExistsByProviderIDOrNodeName(t *testing.T) {
 				"expected exist by provider id to be `%t` but got `%t`",
 				tc.existsByProviderID, exists)
 
-			assert.False(t, tc.existsByNodeName && tc.existsByNodeName != 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,

pkg/controller/util/node/controller_utils.go

@@ -178,7 +178,7 @@ func ExistsInCloudProvider(cloud cloudprovider.Interface, nodeName types.NodeNam
 	if !ok {
 		return false, fmt.Errorf("%v", ErrCloudInstance)
 	}
-	if _, err := instances.ExternalID(context.TODO(), nodeName); err != nil {
+	if _, err := instances.InstanceID(context.TODO(), nodeName); err != nil {
 		if err == cloudprovider.InstanceNotFound {
 			return false, nil
 		}

pkg/kubelet/kubelet_node_status.go

@@ -302,8 +302,8 @@ func (kl *Kubelet) initialNode() (*v1.Node, error) {
 
 		// TODO(roberthbailey): Can we do this without having credentials to talk
 		// to the cloud provider?
-		// TODO: ExternalID is deprecated, we'll have to drop this code
-		externalID, err := instances.ExternalID(context.TODO(), kl.nodeName)
+		// ExternalID is deprecated, so ProviderID is retrieved using InstanceID
+		externalID, err := instances.InstanceID(context.TODO(), kl.nodeName)
 		if err != nil {
 			return nil, fmt.Errorf("failed to get external ID from cloud provider: %v", err)
 		}