Adding reconfigure on delete

[alexlovelltroy]

My first PR attempting a one-line change to address automatic reconfiguration after deleting a Service LoadBalancer

Fixes #413

maybe...

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[dave-tock]

Sadly, this is not going to do it. The "reprocess all other services for changes" logic happens in the caller of these functions, in internal/k8s/. That package contains all the k8s goop to talk to the cluster and fire these callbacks. In particular, when SetConfig returns, it returns true if the config load was successful, and the framework in internal/k8s interprets that to mean "okay I need to fire SetBalancer for all known services in case they changed".

We need to effectively do something similar for the SetBalancer calls from internal/k8s, i.e. give it a way to say "this was successful (which is the current true/false return), and also please trigger a reprocess of all other services"

Several ways to do that. One would be to make DeleteBalancer its own call in the internal/k8s interface, so that it just Knows to reprocess all services on successful delete. Another would be to make SetBalancer return two bools, (success, processAll). Success is true if the update was processed correctly (false triggers a backoff+retry), processAll is this new signal that does a similar behavior to SetConfig.

I'm okay with either of these approaches, with a slight preference for making DeleteBalancer a first-class method in the internal/k8s interface. WDYT?

[alexlovelltroy]

Ahhh. I wondered about the interaction between this and internal/k8s/k8s.go.

When I looked at it though, it wasn't clear to me where to add a DeleteBalancer call in there. If I understand the flow, when a Service is deleted from kubernetes k8s/k8s.go will receive a notification as cfg.ServiceChanged. Is that right?

[danderson]

Yeah, the way internal/k8s works is that it subscribes to information about a bunch of objects (all Service and Endpoints objects in the cluster). When you delete a service, the sync logic receives an event for that service name, looks it up, and the client says "no such object". At which point we call ServiceChanged with a nil object, and the code in controller+speaker interpret that as "oh the service doesn't exist any more."

look for the sync() function in internal/k8s/k8s.go, that's the meat of it. In the svcKey case, there's an if !exists branch. That's where you'd want to invoke this new cfg.ServiceDeleted callback, rather than cfg.ServiceChanged as it does today.

[danderson]

Implemented this myself. Deleting an LB now reprocesses all known services, to try and reassign the freed IP.