[NET-6588] Remove abandoned virtual nodes from Consul Catalog

[curtbushko]

Changes proposed in this PR:

• When deregistering a service in Consul we would leave the virtual node sitting around
• This PR checks if a node has an empty service list and if so deletes

How I've tested this PR:

• manually testing in kind to make sure nodes are cleaned up
• unit tests. The enpoints controller tests have a lot of coverage for service interactions.

How I expect reviewers to test this PR:

👀

Checklist:

• Tests added
• CHANGELOG entry added

[DanStough]

I wonder if this is the wrong function because it filters on the k8sServiceName, but to clean the node we need to know if ANY service is associated with the node.

[curtbushko]

We are getting all the services associated with that node with:

If I am reading the filter correctly it is:

give me the node with:
name = "foo-virtual"
meta {
"k8s-service" = "static-server"
"k8s-namespace" = "default"
"managed-by" = "endpoints-controller"
}

filter := fmt.Sprintf(Meta[%q] == %q and Meta[%q] == %q and Meta[%q] == %q, metaKeyKubeServiceName, k8sServiceName, constants.MetaKeyKubeNS, k8sServiceNamespace, metaKeyManagedBy, constants.ManagedByValue)

serviceList, _, err = apiClient.Catalog().NodeServiceList(nodeName, &api.QueryOptions{Filter: filter})

I think we do it with a filter like this instead of node because Consul would be slow to return without the filter.

[komapa]

Let me know if you want me to run this in a real cluster which can reproduce this problem many times per day :)

[curtbushko]

Let me know if you want me to run this in a real cluster which can reproduce this problem many times per day :)

@komapa Thank you!

You can grab the image at: docker.io/curtbushko/consul-k8s-control-plane-dev:abandoned-nodes

(I tagged you late on Friday but I found a bug that I had to fix. It should be better now)

[thisisnotashwin]

I really like this. I have minor concerns about the performance of the endpoints controller given we have steadily increased what happens during the reconciles, but i think that requires a different, more thorough introspection into what we do in the endpoints controller. This is great work right here though.

[curtbushko]

I really like this. I have minor concerns about the performance of the endpoints controller given we have steadily increased what happens during the reconciles, but i think that requires a different, more thorough introspection into what we do in the endpoints controller. This is great work right here though.

@thisisnotashwin Zalimeni has a PR in draft to help with the endpoints controller performance. I do think that my PR might help with performance as we would have fewer abandoned nodes to scan through on each reconcile.

[thisisnotashwin]

I really like this. I have minor concerns about the performance of the endpoints controller given we have steadily increased what happens during the reconciles, but i think that requires a different, more thorough introspection into what we do in the endpoints controller. This is great work right here though.

@thisisnotashwin Zalimeni has a PR in draft to help with the endpoints controller performance. I do think that my PR might help with performance as we would have fewer abandoned nodes to scan through on each reconcile.

yayyy!! that definitely helps. i think this multipronged approach is the way forward!