Merge pull request kubernetes#93582 from andrewsykim/instance-v2-fixes

Mark cloud provider InstanceV2 as experimental and remove provider ID references

Author: k8s-ci-robot

staging/src/k8s.io/cloud-provider/cloud.go

@@ -49,8 +49,11 @@ type Interface interface {
 	LoadBalancer() (LoadBalancer, bool)
 	// Instances returns an instances interface. Also returns true if the interface is supported, false otherwise.
 	Instances() (Instances, bool)
-	// InstancesV2 is an implementation for instances only used by cloud node-controller now.
+	// InstancesV2 is an implementation for instances and should only be implemented by external cloud providers.
+	// Implementing InstancesV2 is behaviorally identical to Instances but is optimized to significantly reduce
+	// API calls to the cloud provider when registering and syncing nodes.
 	// Also returns true if the interface is supported, false otherwise.
+	// WARNING: InstancesV2 is an experimental interface and is subject to change in v1.20.
 	InstancesV2() (InstancesV2, bool)
 	// Zones returns a zones interface. Also returns true if the interface is supported, false otherwise.
 	Zones() (Zones, bool)
@@ -189,15 +192,20 @@ type Instances interface {
 	InstanceShutdownByProviderID(ctx context.Context, providerID string) (bool, error)
 }
 
-// InstancesV2 is an abstract, pluggable interface for sets of instances.
-// Unlike Instances, it is only used by cloud node-controller now.
+// InstancesV2 is an abstract, pluggable interface for cloud provider instances.
+// Unlike the Instances interface, it is designed for external cloud providers and should only be used by them.
+// WARNING: InstancesV2 is an experimental interface and is subject to change in v1.20.
 type InstancesV2 interface {
-	// InstanceExistsByProviderID returns true if the instance for the given provider exists.
-	InstanceExistsByProviderID(ctx context.Context, providerID string) (bool, error)
-	// InstanceShutdownByProviderID returns true if the instance is shutdown in cloudprovider.
-	InstanceShutdownByProviderID(ctx context.Context, providerID string) (bool, error)
-	// InstanceMetadataByProviderID returns the instance's metadata.
-	InstanceMetadataByProviderID(ctx context.Context, providerID string) (*InstanceMetadata, error)
+	// InstanceExists returns true if the instance for the given node exists according to the cloud provider.
+	// Use the node.name or node.spec.providerID field to find the node in the cloud provider.
+	InstanceExists(ctx context.Context, node *v1.Node) (bool, error)
+	// InstanceShutdown returns true if the instance is shutdown according to the cloud provider.
+	// Use the node.name or node.spec.providerID field to find the node in the cloud provider.
+	InstanceShutdown(ctx context.Context, node *v1.Node) (bool, error)
+	// InstanceMetadata returns the instance's metadata. The values returned in InstanceMetadata are
+	// translated into specific fields in the Node object on registration.
+	// Use the node.name or node.spec.providerID field to find the node in the cloud provider.
+	InstanceMetadata(ctx context.Context, node *v1.Node) (*InstanceMetadata, error)
 }
 
 // Route is a representation of an advanced routing rule.
@@ -265,12 +273,25 @@ type PVLabeler interface {
 	GetLabelsForVolume(ctx context.Context, pv *v1.PersistentVolume) (map[string]string, error)
 }
 
-// InstanceMetadata contains metadata about the specific instance.
+// InstanceMetadata contains metadata about a specific instance.
+// Values returned in InstanceMetadata are translated into specific fields in Node.
 type InstanceMetadata struct {
-	// ProviderID is provider's id that instance belongs to.
+	// ProviderID is a unique ID used to idenfitify an instance on the cloud provider.
+	// The ProviderID set here will be set on the node's spec.providerID field.
+	// The provider ID format can be set by the cloud provider but providers should
+	// ensure the format does not change in any incompatible way.
+	//
+	// The provider ID format used by existing cloud provider has been:
+	//    <provider-name>://<instance-id>
+	// Existing providers setting this field should preserve the existing format
+	// currently being set in node.spec.providerID.
 	ProviderID string
-	// Type is instance's type.
-	Type string
+	// InstanceType is the instance's type.
+	// The InstanceType set here will be set using the following labels on the node object:
+	//   * node.kubernetes.io/instance-type=<instance-type>
+	//   * beta.kubernetes.io/instance-type=<instance-type> (DEPRECATED)
+	InstanceType string
 	// NodeAddress contains information for the instance's address.
+	// The node addresses returned here will be set on the node's status.addresses field.
 	NodeAddresses []v1.NodeAddress
 }

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

@@ -108,7 +108,9 @@ func NewCloudNodeController(
 	klog.Infof("Sending events to api server.")
 	eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")})
 
-	if _, ok := cloud.Instances(); !ok {
+	_, instancesSupported := cloud.Instances()
+	_, instancesV2Supported := cloud.InstancesV2()
+	if !instancesSupported && !instancesV2Supported {
 		return nil, errors.New("cloud provider does not support instances")
 	}
 
@@ -225,7 +227,7 @@ func (cnc *CloudNodeController) updateNodeAddress(ctx context.Context, node *v1.
 
 	instanceMetadataGetter := func(providerID string, node *v1.Node) (*cloudprovider.InstanceMetadata, error) {
 		if instancesV2, ok := cnc.cloud.InstancesV2(); instancesV2 != nil && ok {
-			return instancesV2.InstanceMetadataByProviderID(ctx, providerID)
+			return instancesV2.InstanceMetadata(ctx, node)
 		}
 
 		// If InstancesV2 not implement, use Instances.
@@ -435,9 +437,9 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider(ctx context.Co
 		providerID = node.Spec.ProviderID
 	}
 
-	instanceMetadataGetter := func(providerID string, nodeName string) (*cloudprovider.InstanceMetadata, error) {
+	instanceMetadataGetter := func(providerID string, nodeName string, node *v1.Node) (*cloudprovider.InstanceMetadata, error) {
 		if instancesV2, ok := cnc.cloud.InstancesV2(); instancesV2 != nil && ok {
-			return instancesV2.InstanceMetadataByProviderID(ctx, providerID)
+			return instancesV2.InstanceMetadata(ctx, node)
 		}
 
 		// If InstancesV2 not implement, use Instances.
@@ -454,12 +456,12 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider(ctx context.Co
 			return nil, err
 		}
 		return &cloudprovider.InstanceMetadata{
-			Type:          instanceType,
+			InstanceType:  instanceType,
 			NodeAddresses: nodeAddresses,
 		}, nil
 	}
 
-	instanceMeta, err := instanceMetadataGetter(providerID, node.Name)
+	instanceMeta, err := instanceMetadataGetter(providerID, node.Name, node)
 	if err != nil {
 		return nil, err
 	}
@@ -470,15 +472,15 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider(ctx context.Co
 		return nil, errors.New("failed to find kubelet node IP from cloud provider")
 	}
 
-	if instanceMeta.Type != "" {
-		klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelInstanceType, instanceMeta.Type)
-		klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelInstanceTypeStable, instanceMeta.Type)
+	if instanceMeta.InstanceType != "" {
+		klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelInstanceType, instanceMeta.InstanceType)
+		klog.V(2).Infof("Adding node label from cloud provider: %s=%s", v1.LabelInstanceTypeStable, instanceMeta.InstanceType)
 		nodeModifiers = append(nodeModifiers, func(n *v1.Node) {
 			if n.Labels == nil {
 				n.Labels = map[string]string{}
 			}
-			n.Labels[v1.LabelInstanceType] = instanceMeta.Type
-			n.Labels[v1.LabelInstanceTypeStable] = instanceMeta.Type
+			n.Labels[v1.LabelInstanceType] = instanceMeta.InstanceType
+			n.Labels[v1.LabelInstanceTypeStable] = instanceMeta.InstanceType
 		})
 	}
 

staging/src/k8s.io/cloud-provider/controllers/nodelifecycle/node_lifecycle_controller.go

@@ -85,7 +85,9 @@ func NewCloudNodeLifecycleController(
 		return nil, errors.New("no cloud provider provided")
 	}
 
-	if _, ok := cloud.Instances(); !ok {
+	_, instancesSupported := cloud.Instances()
+	_, instancesV2Supported := cloud.InstancesV2()
+	if !instancesSupported && !instancesV2Supported {
 		return nil, errors.New("cloud provider does not support instances")
 	}
 
@@ -118,12 +120,6 @@ func (c *CloudNodeLifecycleController) Run(stopCh <-chan struct{}) {
 // or shutdown. If deleted, it deletes the node resource. If shutdown it
 // applies a shutdown taint to the node
 func (c *CloudNodeLifecycleController) MonitorNodes() {
-	instances, ok := c.cloud.Instances()
-	if !ok {
-		utilruntime.HandleError(fmt.Errorf("failed to get instances from cloud provider"))
-		return
-	}
-
 	nodes, err := c.nodeLister.List(labels.Everything())
 	if err != nil {
 		klog.Errorf("error listing nodes from cache: %s", err)
@@ -148,7 +144,7 @@ func (c *CloudNodeLifecycleController) MonitorNodes() {
 
 		// At this point the node has NotReady status, we need to check if the node has been removed
 		// from the cloud provider. If node cannot be found in cloudprovider, then delete the node
-		exists, err := ensureNodeExistsByProviderID(context.TODO(), instances, node)
+		exists, err := ensureNodeExistsByProviderID(context.TODO(), c.cloud, node)
 		if err != nil {
 			klog.Errorf("error checking if node %s exists: %v", node.Name, err)
 			continue
@@ -195,6 +191,10 @@ func (c *CloudNodeLifecycleController) MonitorNodes() {
 
 // shutdownInCloudProvider returns true if the node is shutdown on the cloud provider
 func shutdownInCloudProvider(ctx context.Context, cloud cloudprovider.Interface, node *v1.Node) (bool, error) {
+	if instanceV2, ok := cloud.InstancesV2(); ok {
+		return instanceV2.InstanceShutdown(ctx, node)
+	}
+
 	instances, ok := cloud.Instances()
 	if !ok {
 		return false, errors.New("cloud provider does not support instances")
@@ -210,7 +210,16 @@ func shutdownInCloudProvider(ctx context.Context, cloud cloudprovider.Interface,
 
 // 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(ctx context.Context, instances cloudprovider.Instances, node *v1.Node) (bool, error) {
+func ensureNodeExistsByProviderID(ctx context.Context, cloud cloudprovider.Interface, node *v1.Node) (bool, error) {
+	if instanceV2, ok := cloud.InstancesV2(); ok {
+		return instanceV2.InstanceExists(ctx, node)
+	}
+
+	instances, ok := cloud.Instances()
+	if !ok {
+		return false, errors.New("instances interface not supported in the cloud provider")
+	}
+
 	providerID := node.Spec.ProviderID
 	if providerID == "" {
 		var err error