From 1335e6e2d4f8c5e32acdbd73556833ff2650b4a0 Mon Sep 17 00:00:00 2001 From: Anup Navare Date: Thu, 1 Mar 2018 22:22:28 -0800 Subject: [PATCH] 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. --- pkg/cloudprovider/cloud.go | 3 --- pkg/cloudprovider/providers/aws/aws.go | 18 ------------------ .../providers/azure/azure_instances.go | 5 ----- .../cloudstack/cloudstack_instances.go | 5 ----- .../providers/cloudstack/metadata.go | 5 ----- pkg/cloudprovider/providers/fake/fake.go | 8 -------- .../providers/openstack/openstack_instances.go | 12 ------------ pkg/cloudprovider/providers/ovirt/ovirt.go | 10 ---------- pkg/cloudprovider/providers/photon/photon.go | 17 ----------------- .../providers/photon/photon_test.go | 15 --------------- pkg/cloudprovider/providers/vsphere/vsphere.go | 5 ----- .../providers/vsphere/vsphere_test.go | 15 --------------- pkg/controller/cloud/node_controller.go | 6 ++---- pkg/controller/cloud/node_controller_test.go | 6 +++--- pkg/controller/util/node/controller_utils.go | 2 +- pkg/kubelet/kubelet_node_status.go | 4 ++-- 16 files changed, 8 insertions(+), 128 deletions(-) diff --git a/pkg/cloudprovider/cloud.go b/pkg/cloudprovider/cloud.go index 9ca91ebf31..027a6262f8 100644 --- a/pkg/cloudprovider/cloud.go +++ b/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. diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index d7bd645249..55928ef109 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/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) { diff --git a/pkg/cloudprovider/providers/azure/azure_instances.go b/pkg/cloudprovider/providers/azure/azure_instances.go index 1f7daac0f2..fab721d3b7 100644 --- a/pkg/cloudprovider/providers/azure/azure_instances.go +++ b/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) { diff --git a/pkg/cloudprovider/providers/cloudstack/cloudstack_instances.go b/pkg/cloudprovider/providers/cloudstack/cloudstack_instances.go index cf8559a2b0..db8faa8071 100644 --- a/pkg/cloudprovider/providers/cloudstack/cloudstack_instances.go +++ b/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( diff --git a/pkg/cloudprovider/providers/cloudstack/metadata.go b/pkg/cloudprovider/providers/cloudstack/metadata.go index bffeb49cf9..70b289d8eb 100644 --- a/pkg/cloudprovider/providers/cloudstack/metadata.go +++ b/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) diff --git a/pkg/cloudprovider/providers/fake/fake.go b/pkg/cloudprovider/providers/fake/fake.go index 6b4ee68256..ed05b52321 100644 --- a/pkg/cloudprovider/providers/fake/fake.go +++ b/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") diff --git a/pkg/cloudprovider/providers/openstack/openstack_instances.go b/pkg/cloudprovider/providers/openstack/openstack_instances.go index 65176db458..43c60febde 100644 --- a/pkg/cloudprovider/providers/openstack/openstack_instances.go +++ b/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) { diff --git a/pkg/cloudprovider/providers/ovirt/ovirt.go b/pkg/cloudprovider/providers/ovirt/ovirt.go index 708358eec6..e18f008a73 100644 --- a/pkg/cloudprovider/providers/ovirt/ovirt.go +++ b/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) { diff --git a/pkg/cloudprovider/providers/photon/photon.go b/pkg/cloudprovider/providers/photon/photon.go index 424b6f91af..2bfdd639ab 100644 --- a/pkg/cloudprovider/providers/photon/photon.go +++ b/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) { diff --git a/pkg/cloudprovider/providers/photon/photon_test.go b/pkg/cloudprovider/providers/photon/photon_test.go index 6f13f479e3..f798f1cc21 100644 --- a/pkg/cloudprovider/providers/photon/photon_test.go +++ b/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) diff --git a/pkg/cloudprovider/providers/vsphere/vsphere.go b/pkg/cloudprovider/providers/vsphere/vsphere.go index a8606b297b..755cf9bf61 100644 --- a/pkg/cloudprovider/providers/vsphere/vsphere.go +++ b/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) { diff --git a/pkg/cloudprovider/providers/vsphere/vsphere_test.go b/pkg/cloudprovider/providers/vsphere/vsphere_test.go index 6e5e2b04f4..84b9664413 100644 --- a/pkg/cloudprovider/providers/vsphere/vsphere_test.go +++ b/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) diff --git a/pkg/controller/cloud/node_controller.go b/pkg/controller/cloud/node_controller.go index 9e732620e2..05036dfa5f 100644 --- a/pkg/controller/cloud/node_controller.go +++ b/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)) + // Changing the check as InstanceID does not return error if err == nil { - return true, nil - } - if err == cloudprovider.InstanceNotFound { return false, nil } diff --git a/pkg/controller/cloud/node_controller_test.go b/pkg/controller/cloud/node_controller_test.go index a8faf8352f..0bf7c503a7 100644 --- a/pkg/controller/cloud/node_controller_test.go +++ b/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, diff --git a/pkg/controller/util/node/controller_utils.go b/pkg/controller/util/node/controller_utils.go index b2c0ec23cf..9a719e0b26 100644 --- a/pkg/controller/util/node/controller_utils.go +++ b/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 } diff --git a/pkg/kubelet/kubelet_node_status.go b/pkg/kubelet/kubelet_node_status.go index 85e7ca7d17..e0a5e8378d 100644 --- a/pkg/kubelet/kubelet_node_status.go +++ b/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) }