Merge pull request #819 from spencerhance/refactor-lb-gc

Refactor LB GC

pkg/composite/utils.go

@@ -46,52 +46,6 @@ func CreateKey(gceCloud *gce.Cloud, name string, scope meta.KeyType) (*meta.Key,
 	return nil, fmt.Errorf("invalid resource type: %s", scope)
 }
 
-// TODO: (shance) generate this
-// TODO: (shance) populate scope
-// TODO: (shance) figure out a better way to get version
-// ListAllUrlMaps() merges all configured List() calls into one list of composite UrlMaps.
-// This function combines both global and regional resources into one slice
-// so users must use ScopeFromSelfLink() to determine the correct scope
-// before issuing an API call.
-func ListAllUrlMaps(gceCloud *gce.Cloud) ([]*UrlMap, error) {
-	resultMap := map[string]*UrlMap{}
-
-	// TODO(shance): this needs to be replaced with 'scope' and not key
-	globalKey, err := CreateKey(gceCloud, "", meta.Global)
-	if err != nil {
-		return nil, err
-	}
-	regionalKey, err := CreateKey(gceCloud, "", meta.Regional)
-	if err != nil {
-		return nil, err
-	}
-
-	for _, vk := range []struct {
-		v meta.Version
-		k *meta.Key //TODO(shance): change this to scope
-	}{
-		{meta.VersionGA, globalKey},
-		{meta.VersionAlpha, regionalKey},
-	} {
-
-		list, err := ListUrlMaps(gceCloud, vk.k, vk.v)
-		if err != nil {
-			return nil, fmt.Errorf("error listing all urlmaps: %v", err)
-		}
-		for _, um := range list {
-			resultMap[um.SelfLink] = um
-		}
-	}
-
-	// Convert map to slice
-	result := []*UrlMap{}
-	for _, um := range resultMap {
-		result = append(result, um)
-	}
-
-	return result, nil
-}
-
 // TODO: (shance) generate this
 // TODO: (shance) populate scope
 // TODO: (shance) figure out a more accurate way to obtain version

pkg/loadbalancers/features/features.go

@@ -28,6 +28,8 @@ const (
 	FeatureL7ILB = "L7ILB"
 )
 
+var GAResourceVersions = NewResourceVersions()
+
 // ResourceVersions allows you to define all the versions required for each resource
 // for a feature.
 type ResourceVersions struct {

pkg/loadbalancers/features/features_test.go

@@ -75,7 +75,7 @@ var (
 	}
 
 	fakeFeatureToVersions = map[string]*ResourceVersions{
-		fakeGaFeature:              NewResourceVersions(),
+		fakeGaFeature:              GAResourceVersions,
 		fakeAlphaFeature:           &fakeAlphaFeatureVersions,
 		fakeBetaFeature:            &fakeBetaFeatureVersions,
 		fakeAlphaFeatureUrlMapOnly: &fakeAlphaFeatureUrlMapOnlyVersions,
@@ -180,7 +180,7 @@ func TestVersionsFromFeatures(t *testing.T) {
 	}{
 		{
 			desc:     "No Features",
-			expected: NewResourceVersions(),
+			expected: GAResourceVersions,
 		},
 		{
 			desc:     "Alpha features",

pkg/loadbalancers/interfaces.go

@@ -18,14 +18,16 @@ package loadbalancers
 
 import (
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"k8s.io/ingress-gce/pkg/composite"
+	"k8s.io/ingress-gce/pkg/loadbalancers/features"
 )
 
 // LoadBalancerPool is an interface to manage the cloud resources associated
 // with a gce loadbalancer.
 type LoadBalancerPool interface {
 	Ensure(ri *L7RuntimeInfo) (*L7, error)
-	Delete(name string, version meta.Version, scope meta.KeyType) error
+	Delete(name string, versions *features.ResourceVersions, scope meta.KeyType) error
 	GC(names []string) error
 	Shutdown() error
-	List() ([]string, []meta.KeyType, error)
+	List(key *meta.Key, version meta.Version) ([]*composite.UrlMap, error)
 }

pkg/loadbalancers/l7.go

@@ -223,10 +223,9 @@ func (l *L7) GetIP() string {
 // Cleanup deletes resources specific to this l7 in the right order.
 // forwarding rule -> target proxy -> url map
 // This leaves backends and health checks, which are shared across loadbalancers.
-func (l *L7) Cleanup() error {
+func (l *L7) Cleanup(versions *features.ResourceVersions) error {
 	var key *meta.Key
 	var err error
-	versions := l.Versions()
 
 	fwName := l.namer.ForwardingRule(l.Name, utils.HTTPProtocol)
 	klog.V(2).Infof("Deleting global forwarding rule %v", fwName)

pkg/loadbalancers/l7s.go

@@ -72,7 +72,7 @@ func (l *L7s) Ensure(ri *L7RuntimeInfo) (*L7, error) {
 }
 
 // Delete deletes a load balancer by name.
-func (l *L7s) Delete(name string, version meta.Version, scope meta.KeyType) error {
+func (l *L7s) Delete(name string, versions *features.ResourceVersions, scope meta.KeyType) error {
 	lb := &L7{
 		runtimeInfo: &L7RuntimeInfo{Name: name},
 		Name:        l.namer.LoadBalancer(name),
@@ -82,74 +82,93 @@ func (l *L7s) Delete(name string, version meta.Version, scope meta.KeyType) erro
 	}
 
 	klog.V(3).Infof("Deleting lb %v", lb.Name)
-	if err := lb.Cleanup(); err != nil {
+
+	if err := lb.Cleanup(versions); err != nil {
 		return err
 	}
 	return nil
 }
 
-// List returns a list of names of L7 resources, by listing all URL maps and
-// deriving the Loadbalancer name from the URL map name
-func (l *L7s) List() ([]string, []meta.KeyType, error) {
-	var names []string
-	var scopes []meta.KeyType
-
-	var urlMaps []*composite.UrlMap
-	var err error
-	if flags.F.EnableL7Ilb {
-		urlMaps, err = composite.ListAllUrlMaps(l.cloud)
-	} else {
-		urlMaps, err = composite.ListUrlMaps(l.cloud, meta.GlobalKey(""), meta.VersionGA)
-	}
+// List returns a list of urlMaps (the top level LB resource) that belong to the cluster
+func (l *L7s) List(key *meta.Key, version meta.Version) ([]*composite.UrlMap, error) {
+	var result []*composite.UrlMap
+	urlMaps, err := composite.ListUrlMaps(l.cloud, key, version)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	for _, um := range urlMaps {
 		if l.namer.NameBelongsToCluster(um.Name) {
-			nameParts := l.namer.ParseName(um.Name)
-			l7Name := l.namer.LoadBalancerFromLbName(nameParts.LbName)
-			names = append(names, l7Name)
-			scope, err := composite.ScopeFromSelfLink(um.SelfLink)
-			if err != nil {
-				return nil, nil, err
-			}
-			scopes = append(scopes, scope)
+			result = append(result, um)
 		}
 	}
 
-	return names, scopes, nil
+	return result, nil
 }
 
 // GC garbage collects loadbalancers not in the input list.
+// TODO(shance): Update to handle regional and global LB with same name
 func (l *L7s) GC(names []string) error {
 	klog.V(2).Infof("GC(%v)", names)
 
 	knownLoadBalancers := sets.NewString()
 	for _, n := range names {
 		knownLoadBalancers.Insert(l.namer.LoadBalancer(n))
 	}
-	pool, scopes, err := l.List()
+
+	// GC L7-ILB LBs if enabled
+	if flags.F.EnableL7Ilb {
+		key, err := composite.CreateKey(l.cloud, "", meta.Regional)
+		if err != nil {
+			return fmt.Errorf("error getting regional key: %v", err)
+		}
+		urlMaps, err := l.List(key, features.L7ILBVersions().UrlMap)
+		if err != nil {
+			return fmt.Errorf("error listing regional LBs: %v", err)
+		}
+
+		if err := l.gc(urlMaps, knownLoadBalancers, features.L7ILBVersions()); err != nil {
+			return fmt.Errorf("error gc-ing regional LBs: %v", err)
+		}
+	}
+
+	// TODO(shance): fix list taking a key
+	urlMaps, err := l.List(meta.GlobalKey(""), meta.VersionGA)
 	if err != nil {
-		return err
+		return fmt.Errorf("error listing global LBs: %v", err)
+	}
+
+	if errors := l.gc(urlMaps, knownLoadBalancers, features.GAResourceVersions); errors != nil {
+		return fmt.Errorf("error gcing global LBs: %v", errors)
 	}
 
+	return nil
+}
+
+// gc is a helper for GC
+// TODO(shance): get versions from description
+func (l *L7s) gc(urlMaps []*composite.UrlMap, knownLoadBalancers sets.String, versions *features.ResourceVersions) []error {
+	var errors []error
+
 	// Delete unknown loadbalancers
-	for i, name := range pool {
-		if knownLoadBalancers.Has(name) {
+	for _, um := range urlMaps {
+		nameParts := l.namer.ParseName(um.Name)
+		l7Name := l.namer.LoadBalancerFromLbName(nameParts.LbName)
+
+		if knownLoadBalancers.Has(l7Name) {
+			klog.V(3).Infof("Load balancer %v is still valid, not GC'ing", l7Name)
 			continue
 		}
-		klog.V(2).Infof("GCing loadbalancer %v", name)
 
-		version := meta.VersionGA
-		// TODO: (shance) figure out a cleaner way to determine this
-		// Regional resources are alpha only
-		if scopes[i] != meta.Global {
-			version = meta.VersionAlpha
+		scope, err := composite.ScopeFromSelfLink(um.SelfLink)
+		if err != nil {
+			errors = append(errors, fmt.Errorf("error getting scope from self link for urlMap %v: %v", um, err))
+			continue
 		}
 
-		if err := l.Delete(name, version, scopes[i]); err != nil {
-			return err
+		klog.V(2).Infof("GCing loadbalancer %v", l7Name)
+		if err := l.Delete(l7Name, versions, scope); err != nil {
+			errors = append(errors, fmt.Errorf("error deleting loadbalancer %q", l7Name))
 		}
 	}
 	return nil