Skip to content

Bug 1916692: OpenStack: Delete leftover LBs when destroying cluster #4563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Feb 3, 2021
Merged

Bug 1916692: OpenStack: Delete leftover LBs when destroying cluster #4563

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions pkg/destroy/openstack/openstack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,57 @@ func getRouterByPort(client *gophercloud.ServiceClient, allPorts []ports.Port) (
return "", nil
}

func deleteLeftoverLoadBalancers(opts *clientconfig.ClientOpts, logger logrus.FieldLogger, networkID string) {
conn, err := clientconfig.NewServiceClient("load-balancer", opts)
if err != nil {
// Ignore the error if Octavia is not available for the cloud
var gerr *gophercloud.ErrEndpointNotFound

@bogdando bogdando Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you use it as a pointer type, and below in the similar clause, as a refular type. This doesn't look consistent?

if errors.As(err, &gerr) {
logger.Debug("Skip load balancer deletion because Octavia endpoint is not found")
return
}
logger.Error(err)
return
}

listOpts := loadbalancers.ListOpts{
VipNetworkID: networkID,
}
allPages, err := loadbalancers.List(conn, listOpts).AllPages()
if err != nil {

@Fedosin Fedosin Jan 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to handle ErrDefault403 errors separately to prevent situations when a user doesn't have permissions to list load balancers

@mandre mandre Jan 19, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is a pattern we're using all over the place in this file, and if we wanted to treat 403 differently I think we should do it in a separate change.
How do you suggest we should handle a ErrDefault403 ?

@Fedosin Fedosin Jan 21, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if err != nil {
    var gerr gophercloud.ErrDefault403
    if !errors.As(err, &gerr) {
        logger.Debugf("It's forbidden to list load balancers")
        return true, nil
    }
    logger.Error(err)
    return false, nil
}

logger.Error(err)
return
}

allLoadBalancers, err := loadbalancers.ExtractLoadBalancers(allPages)
if err != nil {
logger.Error(err)
return
}
deleteOpts := loadbalancers.DeleteOpts{
Cascade: true,
}
for _, loadbalancer := range allLoadBalancers {
if !strings.HasPrefix(loadbalancer.Description, "Kubernetes external service") {

@luis5tb luis5tb Jan 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps this should rely on Loadbalancer tags rather than text on the description

@mandre mandre Jan 18, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't. The in-tree cloud provider doesn't tag the LB resources.

logger.Debugf("Not deleting LoadBalancer %q with description %q", loadbalancer.ID, loadbalancer.Description)
continue
}
logger.Debugf("Deleting LoadBalancer %q", loadbalancer.ID)
err = loadbalancers.Delete(conn, loadbalancer.ID, deleteOpts).ExtractErr()
if err != nil {
// Ignore the error if the load balancer cannot be found
var gerr gophercloud.ErrDefault404

@bogdando bogdando Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please compare to the line #660

@Fedosin Fedosin Jan 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is correct... This is how errors in gophercloud are organized

if !errors.As(err, &gerr) {
// This can fail when the load balancer is still in use so return/retry
logger.Debugf("Deleting load balancer %q failed: %v", loadbalancer.ID, err)
return
}
logger.Debugf("Cannot find load balancer %q. It's probably already been deleted.", loadbalancer.ID)
}
}
return
}

func isClusterSubnet(subnets []subnets.Subnet, subnetID string) bool {
for _, subnet := range subnets {
if subnet.ID == subnetID {
Expand Down Expand Up @@ -731,6 +782,8 @@ func deleteNetworks(opts *clientconfig.ClientOpts, filter Filter, logger logrus.
if !errors.As(err, &gerr) {
// This can fail when network is still in use
logger.Debugf("Deleting Network %q failed: %v", network.ID, err)
// Try to delete eventual leftover load balancers
deleteLeftoverLoadBalancers(opts, logger, network.ID)

@Fedosin Fedosin Jan 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteLeftoverLoadBalancers returns bool and error. are you going to handle it?

@mandre mandre Jan 19, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't planning to. I don't want to change the return value of deleteNetworks() based on the return of deleteLeftoverLoadBalancers(), since this would only be triggered when deleteNetworks() has failed already.

IMO, the destroy module needs a good refactoring that I don't want to commit to right now.

@Fedosin Fedosin Jan 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean you created a new function deleteLeftoverLoadBalancers and it returns two values that you ignore later. I think we need to either change the signature of the function, or handle the returned values properly.

@mandre mandre Feb 3, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I changed the signature. PTAL.

return false, nil
}
logger.Debugf("Cannot find network %q. It's probably already been deleted.", network.ID)
Expand Down