From 64919225622acba5de4333875d8fce657d08710b Mon Sep 17 00:00:00 2001 From: Angus Lees Date: Tue, 2 Jun 2015 13:42:32 +1000 Subject: [PATCH 1/4] Catch 404 and return exists=false from GetTCPLoadBalancer Previouly getVipByName treated 404 like any other unexpected error return and passed it up the chain. This caused the "if ErrNotFound then exists=false" logic in GetTCPLoadBalancer to never fire. This change teaches getVipByName to return ErrNotFound on a 404 server response. --- pkg/cloudprovider/openstack/openstack.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/cloudprovider/openstack/openstack.go b/pkg/cloudprovider/openstack/openstack.go index 2415114bddb..0ef02cb72df 100644 --- a/pkg/cloudprovider/openstack/openstack.go +++ b/pkg/cloudprovider/openstack/openstack.go @@ -463,6 +463,11 @@ func getVipByName(client *gophercloud.ServiceClient, name string) (*vips.Virtual return true, nil }) if err != nil { + if e, ok := err.(*gophercloud.UnexpectedResponseCodeError); ok { + if e.Actual == http.StatusNotFound { + return nil, ErrNotFound + } + } return nil, err } From 785a775777cc6d4fab14415e0c776f91d8550221 Mon Sep 17 00:00:00 2001 From: Angus Lees Date: Tue, 2 Jun 2015 14:00:25 +1000 Subject: [PATCH 2/4] Actually delete LBaaS monitors after disassociating Partially addresses issue #8352 --- pkg/cloudprovider/openstack/openstack.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cloudprovider/openstack/openstack.go b/pkg/cloudprovider/openstack/openstack.go index 0ef02cb72df..8707434052a 100644 --- a/pkg/cloudprovider/openstack/openstack.go +++ b/pkg/cloudprovider/openstack/openstack.go @@ -691,8 +691,8 @@ func (lb *LoadBalancer) EnsureTCPLoadBalancerDeleted(name, region string) error if poolExists { for _, monId := range pool.MonitorIDs { - // TODO(#8352): Delete the monitor, don't just disassociate it. pools.DisassociateMonitor(lb.network, pool.ID, monId) + monitors.Delete(monId) } pools.Delete(lb.network, pool.ID) } From 9394635cc0a5049509673412c95af8509acffec5 Mon Sep 17 00:00:00 2001 From: Angus Lees Date: Tue, 2 Jun 2015 17:26:03 +1000 Subject: [PATCH 3/4] Make EnsureTCPLoadBalancerDeleted idempotent This change allows EnsureTCPLoadBalancerDeleted to be called repeatedly to reattempt deleting objects that may have failed on a previous run. Specifically, if the VIP is already deleted, then an attempt is made to lookup the pool by name. Returns success when both the VIP and pool are not found. Fixes #8352 --- pkg/cloudprovider/openstack/openstack.go | 122 ++++++++++++++++------- 1 file changed, 84 insertions(+), 38 deletions(-) diff --git a/pkg/cloudprovider/openstack/openstack.go b/pkg/cloudprovider/openstack/openstack.go index 8707434052a..fb6030cc499 100644 --- a/pkg/cloudprovider/openstack/openstack.go +++ b/pkg/cloudprovider/openstack/openstack.go @@ -443,6 +443,46 @@ func (os *OpenStack) TCPLoadBalancer() (cloudprovider.TCPLoadBalancer, bool) { return &LoadBalancer{network, compute, os.lbOpts}, true } +func isNotFound(err error) bool { + e, ok := err.(*gophercloud.UnexpectedResponseCodeError) + return ok && e.Actual == http.StatusNotFound +} + +func getPoolByName(client *gophercloud.ServiceClient, name string) (*pools.Pool, error) { + opts := pools.ListOpts{ + Name: name, + } + pager := pools.List(client, opts) + + poolList := make([]pools.Pool, 0, 1) + + err := pager.EachPage(func(page pagination.Page) (bool, error) { + p, err := pools.ExtractPools(page) + if err != nil { + return false, err + } + poolList = append(poolList, p...) + if len(poolList) > 1 { + return false, ErrMultipleResults + } + return true, nil + }) + if err != nil { + if isNotFound(err) { + return nil, ErrNotFound + } + return nil, err + } + + if len(poolList) == 0 { + return nil, ErrNotFound + } else if len(poolList) > 1 { + return nil, ErrMultipleResults + } + + return &poolList[0], nil +} + func getVipByName(client *gophercloud.ServiceClient, name string) (*vips.VirtualIP, error) { opts := vips.ListOpts{ Name: name, @@ -463,10 +503,8 @@ func getVipByName(client *gophercloud.ServiceClient, name string) (*vips.Virtual return true, nil }) if err != nil { - if e, ok := err.(*gophercloud.UnexpectedResponseCodeError); ok { - if e.Actual == http.StatusNotFound { - return nil, ErrNotFound - } + if isNotFound(err) { + return nil, ErrNotFound } return nil, err } @@ -656,45 +694,53 @@ func (lb *LoadBalancer) UpdateTCPLoadBalancer(name, region string, hosts []strin func (lb *LoadBalancer) EnsureTCPLoadBalancerDeleted(name, region string) error { glog.V(4).Infof("EnsureTCPLoadBalancerDeleted(%v, %v)", name, region) - // TODO(#8352): Because we look up the pool using the VIP object, if the VIP - // is already gone we can't attempt to delete the pool. We should instead - // continue even if the VIP doesn't exist and attempt to delete the pool by - // name. - vip, vipErr := getVipByName(lb.network, name) - if vipErr == ErrNotFound { - return nil - } else if vipErr != nil { - return vipErr - } - - // It's ok if the pool doesn't exist, as we may still need to delete the vip - // (although I don't believe the system should ever be in that state). - pool, poolErr := pools.Get(lb.network, vip.PoolID).Extract() - if poolErr != nil { - detailedErr, ok := poolErr.(*gophercloud.UnexpectedResponseCodeError) - if !ok || detailedErr.Actual != http.StatusNotFound { - return poolErr - } - } - poolExists := (poolErr == nil) - - // We have to delete the VIP before the pool can be deleted, so we can't - // continue on if this fails. - // TODO(#8352): Only do this if the VIP exists once we can delete pools by - // name rather than by ID. - err := vips.Delete(lb.network, vip.ID).ExtractErr() + vip, err := getVipByName(lb.network, name) if err != nil && err != ErrNotFound { return err } - // Ignore errors for everything following here - - if poolExists { - for _, monId := range pool.MonitorIDs { - pools.DisassociateMonitor(lb.network, pool.ID, monId) - monitors.Delete(monId) + // We have to delete the VIP before the pool can be deleted, + // so no point continuing if this fails. + if vip != nil { + err := vips.Delete(lb.network, vip.ID).ExtractErr() + if err != nil && !isNotFound(err) { + return err + } + } + + var pool *pools.Pool + if vip != nil { + pool, err = pools.Get(lb.network, vip.PoolID).Extract() + if err != nil && !isNotFound(err) { + return err + } + } else { + // The VIP is gone, but it is conceivable that a Pool + // still exists that we failed to delete on some + // previous occasion. Make a best effort attempt to + // cleanup any pools with the same name as the VIP. + pool, err = getPoolByName(lb.network, name) + if err != nil && err != ErrNotFound { + return err + } + } + + if pool != nil { + for _, monId := range pool.MonitorIDs { + _, err = pools.DisassociateMonitor(lb.network, pool.ID, monId).Extract() + if err != nil { + return err + } + + err = monitors.Delete(lb.network, monId).ExtractErr() + if err != nil && !isNotFound(err) { + return err + } + } + err = pools.Delete(lb.network, pool.ID).ExtractErr() + if err != nil && !isNotFound(err) { + return err } - pools.Delete(lb.network, pool.ID) } return nil From 75f49b331aa64b441c1de92fbd137bd0eb221468 Mon Sep 17 00:00:00 2001 From: Angus Lees Date: Fri, 5 Jun 2015 16:27:45 +1000 Subject: [PATCH 4/4] Ignore "unspecified" externalIP during LB create Previously we always passed `Address: externalIP.String()` while creating a loadbalancer VIP. This passed "0.0.0.0" when externalIP was unspecified, effectively making it mandatory to specify an externalIP. This change correctly leaves `Address` unspecified when externalIP is unspecified (has a zero value). (Thanks to @justinsb for the report) --- pkg/cloudprovider/openstack/openstack.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/cloudprovider/openstack/openstack.go b/pkg/cloudprovider/openstack/openstack.go index fb6030cc499..38520b28e7e 100644 --- a/pkg/cloudprovider/openstack/openstack.go +++ b/pkg/cloudprovider/openstack/openstack.go @@ -607,15 +607,19 @@ func (lb *LoadBalancer) CreateTCPLoadBalancer(name, region string, externalIP ne } } - vip, err := vips.Create(lb.network, vips.CreateOpts{ + createOpts := vips.CreateOpts{ Name: name, Description: fmt.Sprintf("Kubernetes external service %s", name), - Address: externalIP.String(), Protocol: "TCP", ProtocolPort: ports[0].Port, //TODO: need to handle multi-port PoolID: pool.ID, Persistence: persistence, - }).Extract() + } + if !externalIP.IsUnspecified() { + createOpts.Address = externalIP.String() + } + + vip, err := vips.Create(lb.network, createOpts).Extract() if err != nil { if mon != nil { monitors.Delete(lb.network, mon.ID)