Make routecontroller_test less hacky.

Rename reconcilePodCIDRs to reconcileNodeCIDRs.
Add comments and TODOs about using controller framework.

cluster/gce/util.sh

@@ -771,6 +771,8 @@ function kube-down {
 
   # Delete routes.
   local -a routes
+  # Clean up all routes w/ names like "<cluster-name>-<node-GUID>"
+  # e.g. "kubernetes-12345678-90ab-cdef-1234-567890abcdef"
   routes=( $(gcloud compute routes list --project "${PROJECT}" \
     --regexp "${INSTANCE_PREFIX}-.{8}-.{4}-.{4}-.{4}-.{12}" | awk 'NR >= 2 { print $1 }') )
   routes+=("${MASTER_NAME}")

pkg/cloudprovider/cloud.go

@@ -87,14 +87,21 @@ type Instances interface {
 
 // Route is a representation of an advanced routing rule.
 type Route struct {
-	Name            string
+	// Name is the name of the routing rule in the cloud-provider.
-	TargetInstance  string
+	Name string
+	// TargetInstance is the name of the instance as specified in routing rules
+	// for the cloud-provider (in gce: the Instance Name).
+	TargetInstance string
+	// Destination CIDR is the CIDR format IP range that this routing rule
+	// applies to.
 	DestinationCIDR string
-	Description     string
+	// Description is a free-form string. It can be useful for tagging Routes.
+	Description string
 }
 
 // Routes is an abstract, pluggable interface for advanced routing rules.
 type Routes interface {
+	// List all routes that match the filter
 	ListRoutes(filter string) ([]*Route, error)
 	// Create the described route
 	CreateRoute(route *Route) error

pkg/cloudprovider/nodecontroller/nodecontroller.go

@@ -148,10 +148,10 @@ func generateCIDRs(clusterCIDR *net.IPNet, num int) util.StringSet {
 	return res
 }
 
-// reconcilePodCIDRs looks at each node and assigns it a valid CIDR
+// reconcileNodeCIDRs looks at each node and assigns it a valid CIDR
 // if it doesn't currently have one.
-func (nc *NodeController) reconcilePodCIDRs(nodes *api.NodeList) {
+func (nc *NodeController) reconcileNodeCIDRs(nodes *api.NodeList) {
-	glog.V(4).Infof("Reconciling pods cidrs for %d nodes", len(nodes.Items))
+	glog.V(4).Infof("Reconciling cidrs for %d nodes", len(nodes.Items))
 	// TODO(roberthbailey): This seems inefficient. Why re-calculate CIDRs
 	// on each sync period?
 	availableCIDRs := generateCIDRs(nc.clusterCIDR, len(nodes.Items))
@@ -341,7 +341,9 @@ func (nc *NodeController) monitorNodeStatus() error {
 		return err
 	}
 	if nc.allocateNodeCIDRs {
-		nc.reconcilePodCIDRs(nodes)
+		// TODO (cjcullen): Use pkg/controller/framework to watch nodes and
+		// reduce lists/decouple this from monitoring status.
+		nc.reconcileNodeCIDRs(nodes)
 	}
 	for i := range nodes.Items {
 		var gracePeriod time.Duration

pkg/cloudprovider/routecontroller/routecontroller.go

@@ -62,6 +62,8 @@ func (rc *RouteController) reconcileNodeRoutes() error {
 	if err != nil {
 		return fmt.Errorf("error listing routes: %v", err)
 	}
+	// TODO (cjcullen): use pkg/controller/framework.NewInformer to watch this
+	// and reduce the number of lists needed.
 	nodeList, err := rc.kubeClient.Nodes().List(labels.Everything(), fields.Everything())
 	if err != nil {
 		return fmt.Errorf("error listing nodes: %v", err)

pkg/cloudprovider/routecontroller/routecontroller_test.go

@@ -168,9 +168,22 @@ func TestReconcile(t *testing.T) {
 		if err := rc.reconcile(testCase.nodes, testCase.initialRoutes); err != nil {
 			t.Errorf("%d. Error from rc.reconcile(): %v", i, err)
 		}
-		time.Sleep(10 * time.Millisecond)
+		var finalRoutes []*cloudprovider.Route
-		if finalRoutes, err := routes.ListRoutes(""); err != nil || !routeListEqual(finalRoutes, testCase.expectedRoutes) {
+		var err error
-			t.Errorf("%d. rc.reconcile() = %v, routes:\n%v\nexpected: nil, routes:\n%v\n", i, err, flatten(finalRoutes), flatten(testCase.expectedRoutes))
+		timeoutChan := time.After(50 * time.Millisecond)
+		tick := time.NewTicker(10 * time.Millisecond)
+		defer tick.Stop()
+	poll:
+		for {
+			select {
+			case <-tick.C:
+				if finalRoutes, err = routes.ListRoutes(""); err == nil && routeListEqual(finalRoutes, testCase.expectedRoutes) {
+					break poll
+				}
+			case <-timeoutChan:
+				t.Errorf("%d. rc.reconcile() = %v, routes:\n%v\nexpected: nil, routes:\n%v\n", i, err, flatten(finalRoutes), flatten(testCase.expectedRoutes))
+				break poll
+			}
 		}
 	}
 }