From e16267a28eac2581b6719e476ed66b4789d6ce35 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 4 Jan 2017 16:58:07 -0800 Subject: [PATCH] Don't try to sync LBs that have no ports For example: an ingress with a single backend that happens to not be NodePort. In the most basic sense, this LB is broken. We could skip it entirely, but if there are any ports we can at least make progress. Log an event for missing NodePorts. If there are no node-ports, don't try to create it at all. --- controllers/gce/controller/controller.go | 52 +++++++++++++++++-- controllers/gce/controller/utils.go | 31 ----------- .../gce/loadbalancers/loadbalancers.go | 45 ++++++++++------ 3 files changed, 78 insertions(+), 50 deletions(-) diff --git a/controllers/gce/controller/controller.go b/controllers/gce/controller/controller.go index 9b69b1e6f..47d7e0604 100644 --- a/controllers/gce/controller/controller.go +++ b/controllers/gce/controller/controller.go @@ -290,11 +290,11 @@ func (lbc *LoadBalancerController) sync(key string) (err error) { } glog.V(3).Infof("Syncing %v", key) - paths, err := lbc.ingLister.List() + ingresses, err := lbc.ingLister.List() if err != nil { return err } - nodePorts := lbc.tr.toNodePorts(&paths) + nodePorts := lbc.tr.toNodePorts(&ingresses) lbNames := lbc.ingLister.Store.ListKeys() lbs, err := lbc.ListRuntimeInfo() if err != nil { @@ -320,8 +320,13 @@ func (lbc *LoadBalancerController) sync(key string) (err error) { // successful GC we know that there are no dangling cloud resources that // don't have an associated Kubernetes Ingress/Service/Endpoint. + allNodePorts := []int64{} + for _, lb := range lbs { + allNodePorts = append(allNodePorts, lb.NodePorts...) + } + lbNames := lbc.ingLister.Store.ListKeys() defer func() { - if deferErr := lbc.CloudClusterManager.GC(lbNames, nodePorts); deferErr != nil { + if deferErr := lbc.CloudClusterManager.GC(lbNames, allNodePorts); deferErr != nil { err = fmt.Errorf("Error during sync %v, error during GC %v", err, deferErr) } glog.V(3).Infof("Finished syncing %v", key) @@ -330,7 +335,7 @@ func (lbc *LoadBalancerController) sync(key string) (err error) { // Record any errors during sync and throw a single error at the end. This // allows us to free up associated cloud resources ASAP. var syncError error - if err := lbc.CloudClusterManager.Checkpoint(lbs, nodeNames, nodePorts); err != nil { + if err := lbc.CloudClusterManager.Checkpoint(lbs, nodeNames, allNodePorts); err != nil { // TODO: Implement proper backoff for the queue. eventMsg := "GCE" if utils.IsHTTPErrorCode(err, http.StatusForbidden) { @@ -427,10 +432,16 @@ func (lbc *LoadBalancerController) ListRuntimeInfo() (lbs []*loadbalancers.L7Run if err != nil { glog.Warningf("Cannot get certs for Ingress %v/%v: %v", ing.Namespace, ing.Name, err) } + nodePorts, missingPorts := lbc.getNodePorts(&ing) + if len(missingPorts) > 0 { + glog.Warningf("Ingress %v/%v is missing some NodePorts", ing.Namespace, ing.Name) + } + annotations := ingAnnotations(ing.ObjectMeta.Annotations) lbs = append(lbs, &loadbalancers.L7RuntimeInfo{ Name: k, TLS: tls, + NodePorts: nodePorts, AllowHTTP: annotations.allowHTTP(), StaticIPName: annotations.staticIPName(), }) @@ -438,6 +449,39 @@ func (lbc *LoadBalancerController) ListRuntimeInfo() (lbs []*loadbalancers.L7Run return lbs, nil } +func (lbc *LoadBalancerController) getNodePorts(ing *extensions.Ingress) ([]int64, []int64) { + nodePorts := []int64{} + missing := []int64{} + + defaultBackend := ing.Spec.Backend + if defaultBackend != nil { + port, err := lbc.tr.getServiceNodePort(*defaultBackend, ing.Namespace) + if err != nil { + lbc.recorder.Eventf(ing, api.EventTypeWarning, "Service", err.Error()) + missing = append(missing, int64(port)) + } else { + nodePorts = append(nodePorts, int64(port)) + } + } + for _, rule := range ing.Spec.Rules { + if rule.HTTP == nil { + glog.Errorf("Ignoring non http Ingress rule.") + continue + } + for _, path := range rule.HTTP.Paths { + port, err := lbc.tr.getServiceNodePort(path.Backend, ing.Namespace) + if err != nil { + lbc.recorder.Eventf(ing, api.EventTypeWarning, "Service", err.Error()) + missing = append(missing, int64(port)) + continue + } + nodePorts = append(nodePorts, int64(port)) + } + } + + return nodePorts, missing +} + // syncNodes manages the syncing of kubernetes nodes to gce instance groups. // The instancegroups are referenced by loadbalancer backends. func (lbc *LoadBalancerController) syncNodes(key string) error { diff --git a/controllers/gce/controller/utils.go b/controllers/gce/controller/utils.go index 930d6bffa..06187d3f4 100644 --- a/controllers/gce/controller/utils.go +++ b/controllers/gce/controller/utils.go @@ -331,37 +331,6 @@ func (t *GCETranslator) getServiceNodePort(be extensions.IngressBackend, namespa "Could not find matching nodeport from service.")} } -// toNodePorts converts a pathlist to a flat list of nodeports. -func (t *GCETranslator) toNodePorts(ings *extensions.IngressList) []int64 { - knownPorts := []int64{} - for _, ing := range ings.Items { - defaultBackend := ing.Spec.Backend - if defaultBackend != nil { - port, err := t.getServiceNodePort(*defaultBackend, ing.Namespace) - if err != nil { - glog.Infof("%v", err) - } else { - knownPorts = append(knownPorts, int64(port)) - } - } - for _, rule := range ing.Spec.Rules { - if rule.HTTP == nil { - glog.Errorf("Ignoring non http Ingress rule.") - continue - } - for _, path := range rule.HTTP.Paths { - port, err := t.getServiceNodePort(path.Backend, ing.Namespace) - if err != nil { - glog.Infof("%v", err) - continue - } - knownPorts = append(knownPorts, int64(port)) - } - } - } - return knownPorts -} - func getZone(n *api.Node) string { zone, ok := n.Labels[zoneKey] if !ok { diff --git a/controllers/gce/loadbalancers/loadbalancers.go b/controllers/gce/loadbalancers/loadbalancers.go index 9f98b389b..d645ff6a8 100644 --- a/controllers/gce/loadbalancers/loadbalancers.go +++ b/controllers/gce/loadbalancers/loadbalancers.go @@ -162,23 +162,36 @@ func (l *L7s) Delete(name string) error { // Sync loadbalancers with the given runtime info from the controller. func (l *L7s) Sync(lbs []*L7RuntimeInfo) error { - glog.V(3).Infof("Creating loadbalancers %+v", lbs) + glog.V(4).Infof("Syncing %d loadbalancers", len(lbs)) - if len(lbs) != 0 { - // Lazily create a default backend so we don't tax users who don't care - // about Ingress by consuming 1 of their 3 GCE BackendServices. This - // BackendService is GC'd when there are no more Ingresses. - if err := l.defaultBackendPool.Add(l.defaultBackendNodePort); err != nil { - return err - } - defaultBackend, err := l.defaultBackendPool.Get(l.defaultBackendNodePort) - if err != nil { - return err - } - l.glbcDefaultBackend = defaultBackend - } - // create new loadbalancers, validate existing + // Make sure there is at least one active port in the set of LBs, before we + // do any work. + validLBs := []*L7RuntimeInfo{} for _, ri := range lbs { + if len(ri.NodePorts) > 0 { + validLBs = append(validLBs, ri) + } + } + + glog.V(4).Infof("%d of %d loadbalancers are valid", len(validLBs), len(lbs)) + if len(validLBs) == 0 { + return nil + } + + // Lazily create a default backend so we don't tax users who don't care + // about Ingress by consuming 1 of their 3 GCE BackendServices. This + // BackendService is GC'd when there are no more Ingresses. + if err := l.defaultBackendPool.Add(l.defaultBackendNodePort); err != nil { + return err + } + defaultBackend, err := l.defaultBackendPool.Get(l.defaultBackendNodePort) + if err != nil { + return err + } + l.glbcDefaultBackend = defaultBackend + + // create new loadbalancers, validate existing + for _, ri := range validLBs { if err := l.Add(ri); err != nil { return err } @@ -244,6 +257,8 @@ type L7RuntimeInfo struct { Name string // IP is the desired ip of the loadbalancer, eg from a staticIP. IP string + // NodePorts is the set of ports being used by this L7. + NodePorts []int64 // TLS are the tls certs to use in termination. TLS *TLSCerts // AllowHTTP will not setup :80, if TLS is nil and AllowHTTP is set,