Move Node.ExternalID to CloudNodes()

pkg/cloudprovider/controller/nodecontroller.go

@@ -77,13 +77,20 @@ func NewNodeController(
 
 // Run creates initial node list and start syncing instances from cloudprovider if any.
 // It also starts syncing cluster node status.
+// 1. RegisterNodes() is called only once to register all initial nodes (from cloudprovider
+//    or from command line flag). To make cluster bootstrap faster, node controller populates
+//    node addresses.
+// 2. SyncCloud() is called periodically (if enabled) to sync instances from cloudprovider.
+//    Node created here will only have specs.
+// 3. SyncNodeStatus() is called periodically (if enabled) to sync node status for nodes in
+//    k8s cluster.
 func (s *NodeController) Run(period time.Duration, syncNodeList, syncNodeStatus bool) {
 	// Register intial set of nodes with their status set.
 	var nodes *api.NodeList
 	var err error
 	if s.isRunningCloudProvider() {
 		if syncNodeList {
-			nodes, err = s.CloudNodes()
+			nodes, err = s.GetCloudNodesWithSpec()
 			if err != nil {
 				glog.Errorf("Error loading initial node from cloudprovider: %v", err)
 			}
@@ -91,7 +98,7 @@ func (s *NodeController) Run(period time.Duration, syncNodeList, syncNodeStatus
 			nodes = &api.NodeList{}
 		}
 	} else {
-		nodes, err = s.StaticNodes()
+		nodes, err = s.GetStaticNodesWithSpec()
 		if err != nil {
 			glog.Errorf("Error loading initial static nodes: %v", err)
 		}
@@ -162,11 +169,7 @@ func (s *NodeController) RegisterNodes(nodes *api.NodeList, retryCount int, retr
 
 // SyncCloud synchronizes the list of instances from cloudprovider to master server.
 func (s *NodeController) SyncCloud() error {
-	matches, err := s.CloudNodes()
-	if err != nil {
-		return err
-	}
-	matches, err = s.PopulateIPs(matches)
+	matches, err := s.GetCloudNodesWithSpec()
 	if err != nil {
 		return err
 	}
@@ -271,12 +274,6 @@ func (s *NodeController) PopulateIPs(nodes *api.NodeList) (*api.NodeList, error)
 			} else {
 				node.Status.HostIP = hostIP.String()
 			}
-			instanceID, err := instances.ExternalID(node.Name)
-			if err != nil {
-				glog.Errorf("error getting instance id for %s: %v", node.Name, err)
-			} else {
-				node.Spec.ExternalID = instanceID
-			}
 		}
 	} else {
 		for i := range nodes.Items {
@@ -393,9 +390,10 @@ func (s *NodeController) deletePods(nodeID string) error {
 	return nil
 }
 
-// StaticNodes constructs and returns api.NodeList for static nodes. If error
-// occurs, an empty NodeList will be returned with a non-nil error info.
-func (s *NodeController) StaticNodes() (*api.NodeList, error) {
+// GetStaticNodesWithSpec constructs and returns api.NodeList for static nodes. If error
+// occurs, an empty NodeList will be returned with a non-nil error info. The
+// method only constructs spec fields for nodes.
+func (s *NodeController) GetStaticNodesWithSpec() (*api.NodeList, error) {
 	result := &api.NodeList{}
 	for _, nodeID := range s.nodes {
 		node := api.Node{
@@ -407,9 +405,10 @@ func (s *NodeController) StaticNodes() (*api.NodeList, error) {
 	return result, nil
 }
 
-// CloudNodes constructs and returns api.NodeList from cloudprovider. If error
-// occurs, an empty NodeList will be returned with a non-nil error info.
-func (s *NodeController) CloudNodes() (*api.NodeList, error) {
+// GetCloudNodesWithSpec constructs and returns api.NodeList from cloudprovider. If error
+// occurs, an empty NodeList will be returned with a non-nil error info. The
+// method only constructs spec fields for nodes.
+func (s *NodeController) GetCloudNodesWithSpec() (*api.NodeList, error) {
 	result := &api.NodeList{}
 	instances, ok := s.cloud.Instances()
 	if !ok {
@@ -432,6 +431,12 @@ func (s *NodeController) CloudNodes() (*api.NodeList, error) {
 		if resources != nil {
 			node.Spec.Capacity = resources.Capacity
 		}
+		instanceID, err := instances.ExternalID(node.Name)
+		if err != nil {
+			glog.Errorf("error getting instance id for %s: %v", node.Name, err)
+		} else {
+			node.Spec.ExternalID = instanceID
+		}
 		result.Items = append(result.Items, node)
 	}
 	return result, nil

pkg/cloudprovider/controller/nodecontroller_test.go

@@ -247,7 +247,7 @@ func TestRegisterNodes(t *testing.T) {
 	}
 }
 
-func TestCreateStaticNodes(t *testing.T) {
+func TestCreateGetStaticNodesWithSpec(t *testing.T) {
 	table := []struct {
 		machines      []string
 		expectedNodes *api.NodeList
@@ -289,7 +289,7 @@ func TestCreateStaticNodes(t *testing.T) {
 
 	for _, item := range table {
 		nodeController := NewNodeController(nil, "", item.machines, &api.NodeResources{}, nil, nil, 10, time.Minute)
-		nodes, err := nodeController.StaticNodes()
+		nodes, err := nodeController.GetStaticNodesWithSpec()
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 		}
@@ -299,7 +299,7 @@ func TestCreateStaticNodes(t *testing.T) {
 	}
 }
 
-func TestCreateCloudNodes(t *testing.T) {
+func TestCreateGetCloudNodesWithSpec(t *testing.T) {
 	resourceList := api.ResourceList{
 		api.ResourceCPU:    *resource.NewMilliQuantity(1000, resource.DecimalSI),
 		api.ResourceMemory: *resource.NewQuantity(3000, resource.DecimalSI),
@@ -350,7 +350,7 @@ func TestCreateCloudNodes(t *testing.T) {
 
 	for _, item := range table {
 		nodeController := NewNodeController(item.fakeCloud, ".*", nil, &api.NodeResources{}, nil, nil, 10, time.Minute)
-		nodes, err := nodeController.CloudNodes()
+		nodes, err := nodeController.GetCloudNodesWithSpec()
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 		}