From e6da5b9601738069e0a3f81aca6dcf4bb24a4b4f Mon Sep 17 00:00:00 2001 From: CJ Cullen Date: Wed, 20 May 2015 17:24:30 -0700 Subject: [PATCH] Make routecontroller_test less hacky. Rename reconcilePodCIDRs to reconcileNodeCIDRs. Add comments and TODOs about using controller framework. --- cluster/gce/util.sh | 2 ++ pkg/cloudprovider/cloud.go | 13 ++++++++++--- .../nodecontroller/nodecontroller.go | 10 ++++++---- .../routecontroller/routecontroller.go | 2 ++ .../routecontroller/routecontroller_test.go | 19 ++++++++++++++++--- 5 files changed, 36 insertions(+), 10 deletions(-) diff --git a/cluster/gce/util.sh b/cluster/gce/util.sh index efe5c10fc90..078743b9c46 100755 --- a/cluster/gce/util.sh +++ b/cluster/gce/util.sh @@ -771,6 +771,8 @@ function kube-down { # Delete routes. local -a routes + # Clean up all routes w/ names like "-" + # 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}") diff --git a/pkg/cloudprovider/cloud.go b/pkg/cloudprovider/cloud.go index bc487d7ec43..4739d4caafd 100644 --- a/pkg/cloudprovider/cloud.go +++ b/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 - TargetInstance string + // Name is the name of the routing rule in the cloud-provider. + 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 diff --git a/pkg/cloudprovider/nodecontroller/nodecontroller.go b/pkg/cloudprovider/nodecontroller/nodecontroller.go index 166522f2018..de74570bee1 100644 --- a/pkg/cloudprovider/nodecontroller/nodecontroller.go +++ b/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) { - glog.V(4).Infof("Reconciling pods cidrs for %d nodes", len(nodes.Items)) +func (nc *NodeController) reconcileNodeCIDRs(nodes *api.NodeList) { + 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 diff --git a/pkg/cloudprovider/routecontroller/routecontroller.go b/pkg/cloudprovider/routecontroller/routecontroller.go index 1ba08a7e63a..5cbe5859764 100644 --- a/pkg/cloudprovider/routecontroller/routecontroller.go +++ b/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) diff --git a/pkg/cloudprovider/routecontroller/routecontroller_test.go b/pkg/cloudprovider/routecontroller/routecontroller_test.go index baf68e2eebe..3d256b4b91b 100644 --- a/pkg/cloudprovider/routecontroller/routecontroller_test.go +++ b/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) - if finalRoutes, err := routes.ListRoutes(""); err != nil || !routeListEqual(finalRoutes, testCase.expectedRoutes) { - t.Errorf("%d. rc.reconcile() = %v, routes:\n%v\nexpected: nil, routes:\n%v\n", i, err, flatten(finalRoutes), flatten(testCase.expectedRoutes)) + var finalRoutes []*cloudprovider.Route + var err error + 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 + } } } }