Skip to content
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

Jump to bottom

Bug 1829824: Remove dead member from LB pool. #612

Merged
merged 1 commit into from May 7, 2020
Merged

Bug 1829824: Remove dead member from LB pool. #612

Show file tree
Hide file tree
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/platform/openstack/kuryr_bootstrap.go
View file
Open in desktop
Expand Up @@ -457,9 +457,11 @@ func BootstrapKuryr(conf *operv1.NetworkSpec, kubeClient client.Client) (*bootst
log.Print("Creating OpenShift API loadbalancer pool members")
r, _ := regexp.Compile(fmt.Sprintf("^%s-(master-port-[0-9]+|bootstrap-port)$", clusterID))
portList, err := listOpenStackPortsMatchingPattern(client, tag, r)
addresses := make([]string, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for make() in that case, just var addresses []string to create a nil slice should be fine, append will be fine with it.

for _, port := range portList {
if len(port.FixedIPs) > 0 {
portIp := port.FixedIPs[0].IPAddress
addresses = append(addresses, portIp)
log.Printf("Found port %s with IP %s", port.ID, portIp)

// We want bootstrap to stop being used as soon as possible, as it will serve
Expand All @@ -481,6 +483,11 @@ func BootstrapKuryr(conf *operv1.NetworkSpec, kubeClient client.Client) (*bootst
}
}

err = purgeOpenStackLbPoolMember(client, poolId, addresses)
if err != nil {
return nil, errors.Wrap(err, "Failed on purging invalid LB members from LB pool")
}

log.Print("Ensuring certificates")
ca, key, err := ensureCA(kubeClient)
if err != nil {
Expand Down
32 changes: 32 additions & 0 deletions pkg/platform/openstack/loadbalancer.go
View file
Open in desktop
Expand Up @@ -411,3 +411,35 @@ func listOpenStackOctaviaProviders(client *gophercloud.ServiceClient) ([]provide
return providersList, nil
}
}

// Iterate on pool members and check their address against provided list
// addresses of current master/bootstrap nodes. Remove all surplus members,
// which address doesn't exists on that list.
func purgeOpenStackLbPoolMember(client *gophercloud.ServiceClient, poolId string, addresses []string) error {
page, err := pools.ListMembers(client, poolId, nil).AllPages()
if err != nil {
return errors.Wrap(err, "failed to get LB member list")
}

members, err := pools.ExtractMembers(page)
if err != nil {
return errors.Wrap(err, "failed to extract LB members list")
}

for _, member := range members {
Copy link
Contributor

Choose a reason for hiding this comment

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

as we have already added the "new members", does it make sense to compare here members and addresses length first, and if the same assume members are already up to date?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems tempting, although it won't prevent us from making less Octavia calls, right? And this will ensure, that members addresses are the same as for nodes addresses (as it should be) :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I support @gryf here, we could miss kind of a failover of master VM. Regardless of if it's possible or not, it's almost no cost to a do full comparison. It's just golang makes it look long and clumsy. ;)

found := false
for _, address := range addresses {
if address == member.Address {
found = true
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using found flag you could just use loops labels: mark loop in 429 as members_loop and make this a continue members_loop.

}
}
if !found {
err = pools.DeleteMember(client, poolId, member.ID).ExtractErr()
if err != nil {
return errors.Wrap(err, "failed to delete LB member")
}
}
}
return nil
}