this switch case can be definitely compacted :)

mmm... can we? I was hoping so initially, but I'm not sure how we can access the fields of each individual type except through the switch (or equivalent explicit type conversions). It's possible that my understanding of go is wrong, but from what I understand event in each of these separate switch branches has a different type. Unless we were doing C-style unsafe data overlay stuff here, I'm not sure how we can otherwise get to the fields even if they all have the same names, types and order declaration. Please do let me know if I'm wrong.

oh ok event is an empty interface, so yes never mind, if you collapse the 2 case the type will remain the same

Actually looking at it better this can be compacted as:

switch event := ev.(type) {
	case networkdb.CreateEvent:
	case networkdb.DeleteEvent:
	case networkdb.UpdateEvent:
		nid = event.NetworkID
		eid = event.Key
		value = event.Value
	default:
		logrus.Errorf("Unexpected update service table event = %#v", event)
		return
	}

both the 3 events are actually defined as:

// CreateEvent generates a table entry create event to the watchers
type CreateEvent event

// UpdateEvent generates a table entry update event to the watchers
type UpdateEvent event

// DeleteEvent generates a table entry delete event to the watchers
type DeleteEvent event

so they are all sharing the same base type:

type event struct {
	Table     string
	NetworkID string
	Key       string
	Value     []byte
}

make the collapse of the 3 cases possible

What you wrote above is legal, but doesn't do what you think it does. :) CreateEvent and DeleteEvent cases are empty branches. So they will not populate nid, eid, and value. Furthermore, you can't fix that by adding fallthrough to those branches because fallthrough is specifically forbidden in type switches. However, since they are all type event under the hood, I can compact it with a second type assertion. Will try this out and repost.

Actually, I can't do that because networkdb.event is not an exported type. :(

sorry I wrote it C++ style, and yes because of event is not exported won't work if you don't change the scope. Ok I'm giving up

is there a reason to dup the map and not enhance the backends to have a flag enable?

That approach would certainly work as well. It would make some things cleaner and others a bit more awkward. I'll give it a quick shot and see how it looks.

nit: Failed to cleanup

Should we use ep.ID() and n.ID() in the warnings too (rather than ep.id/n.id)?

That would have been more consistent I guess. But (unless I am misunderstanding something fundamental) they had better produce the same result or something is seriously wrong. :)

Not specific to this PR but had a concern since we are touching this area overall: Will it be safer to remove the service binding (to stop resolving the names) first and then call disableServiceInNetworkDB/agent.networkDB.DeleteEntry

That's a fair question. Since the usual order is "lookup name, issue connection request, get redirected by NAT, get load balanced, connect to server", it would make sense to disable the name lookup first and then the load balancing. Or from a code standpoint, since we do (1) set up load balancing and then (2) set up name resolution, it makes sense to tear them down in the opposite order. Patch was already merged, but we could create another PR to address this. I'll confess that I haven't looked in detail at why it removal matched instead of reversed the order of "undo"s. Would need to verify there are no issues there.

On a bit further reflection, though, this doesn't get hit as much as I first thought. Clients don't generally connect to the service backends through their backend service names. They connect through the front-end load balancer. The DNS resolution for this doesn't go away until the last backend goes away. So at that point the connection is supposed to fail. The only difference will be whether the client receives a "host unreachable" or a "name resolution failure". And even then, only if it happens to occur within the fractions of a second between removing the front-end IP address and the DNS entry.

Agree with the fact that any negative effect of this will be extremely short lived. The comment was more in the context of proper ordering of cleanup ops. We can address this in a potential cleanup patch later.

disableServiceInNetworkDB - API handles many error conditions. But we are not propagating to the caller like deleteServiceInfoFromCluster. Dont we need to propagate any error messages to the caller ?

Fair question. In this case I think "no". The main reason I put that logic in a separate function was for code style and to keep both the indent level and the length of deleteServiceInfoFromCluster() from growing too much. However, some of the errors that said helper function can encounter are warning-level and some are error-level. Returning an error back to the caller instead of doing the logging directly would require parsing the error type and issuing logs which would negate the readability benefits of using the helper function in the first place.

If the result of any of these errors would have altered the control flow of deleteServiceInfoFromCluster(), then returning the error would have been absolutely the right thing to do. However, there is really no way to recover from said errors as evidenced by the handling in the if fullremove { branch above the disableServiceInNetworkDB() call. (which was the state of the code before this patch). If that branch encounters an error creating a cluster event it warns and moves on.

Also, for the record, the error-level logs in disableServiceInNetworkDB() are basically assertions. The only reason they should ever trigger is that there is some critical error in our automatically-generated protocol buffer code.

ok. LGTM