Repository: kubernetes/kubernetes (language: go)

Issue:
Crash on kube manager's service-lb-controller after v1.31.0
### What happened?

If kube manager is started with no `cloud-provider`, it looks like the service-lb-controller is instantiated but not started. However, part of its initialization happens on the `Run` method **and** its handlers are added to the informer on instantiation. So the handlers still run and it crashes when they do because there is stuff not initialized, like in this case, the event recorder:
```2024-10-14T11:27:20.573127782Z stderr F 	goroutine 735 [running]:
2024-10-14T11:27:20.573132561Z stderr F 	k8s.io/apimachinery/pkg/util/runtime.logPanic({0x38432a0, 0x5545b00}, {0x2d99000, 0x5480790})
2024-10-14T11:27:20.573137951Z stderr F 		k8s.io/apimachinery/pkg/util/runtime/runtime.go:107 +0xbc
2024-10-14T11:27:20.573154772Z stderr F 	k8s.io/apimachinery/pkg/util/runtime.handleCrash({0x38432a0, 0x5545b00}, {0x2d99000, 0x5480790}, {0x5545b00, 0x0, 0x43d945?})
2024-10-14T11:27:20.573159501Z stderr F 		k8s.io/apimachinery/pkg/util/runtime/runtime.go:82 +0x5e
2024-10-14T11:27:20.573187343Z stderr F 	k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc001e5ec40?})
2024-10-14T11:27:20.573192182Z stderr F 		k8s.io/apimachinery/pkg/util/runtime/runtime.go:59 +0x108
2024-10-14T11:27:20.57319655Z stderr F 	panic({0x2d99000?, 0x5480790?})
2024-10-14T11:27:20.573200548Z stderr F 		runtime/panic.go:770 +0x132
2024-10-14T11:27:20.573204836Z stderr F 	k8s.io/cloud-provider/controllers/service.(*Controller).needsUpdate(0xc000978000, 0xc00324c288, 0xc000cf3688)
2024-10-14T11:27:20.573210016Z stderr F 		k8s.io/cloud-provider/controllers/service/controller.go:606 +0xcbb
2024-10-14T11:27:20.573214644Z stderr F 	k8s.io/cloud-provider/controllers/service.New.func2({0x326c8e0?, 0xc00324c288?}, {0x326c8e0, 0xc000cf3688?})
2024-10-14T11:27:20.573219133Z stderr F 		k8s.io/cloud-provider/controllers/service/controller.go:144 +0x74
2024-10-14T11:27:20.573224062Z stderr F 	k8s.io/client-go/tools/cache.ResourceEventHandlerFuncs.OnUpdate(...)
2024-10-14T11:27:20.573228831Z stderr F 		k8s.io/client-go/tools/cache/controller.go:253
2024-10-14T11:27:20.573233339Z stderr F 	k8s.io/client-go/tools/cache.(*processorListener).run.func1()
2024-10-14T11:27:20.573237758Z stderr F 		k8s.io/client-go/tools/cache/shared_informer.go:976 +0xea
2024-10-14T11:27:20.573241875Z stderr F 	k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x30?)
2024-10-14T11:27:20.57326562Z stderr F 		k8s.io/apimachinery/pkg/util/wait/backoff.go:226 +0x33
2024-10-14T11:27:20.573270419Z stderr F 	k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc00311cf70, {0x380e700, 0xc001e3a8a0}, 0x1, 0xc001e3e780)
2024-10-14T11:27:20.573274767Z stderr F 		k8s.io/apimachinery/pkg/util/wait/backoff.go:227 +0xaf
2024-10-14T11:27:20.573279556Z stderr F 	k8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc001e5a770, 0x3b9aca00, 0x0, 0x1, 0xc001e3e780)
2024-10-14T11:27:20.573284085Z stderr F 		k8s.io/apimachinery/pkg/util/wait/backoff.go:204 +0x7f
2024-10-14T11:27:20.573288433Z stderr F 	k8s.io/apimachinery/pkg/util/wait.Until(...)
2024-10-14T11:27:20.573292821Z stderr F 		k8s.io/apimachinery/pkg/util/wait/backoff.go:161
2024-10-14T11:27:20.573297059Z stderr F 	k8s.io/client-go/tools/cache.(*processorListener).run(0xc000892d80)
2024-10-14T11:27:20.573302339Z stderr F 		k8s.io/client-go/tools/cache/shared_informer.go:972 +0x69
2024-10-14T11:27:20.573306677Z stderr F 	k8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1()
2024-10-14T11:27:20.573310975Z stderr F 		k8s.io/apimachinery/pkg/util/wait/wait.go:72 +0x52
2024-10-14T11:27:20.573315874Z stderr F 	created by k8s.io/apimachinery/pkg/util/wait.(*Group).Start in goroutine 690
2024-10-14T11:27:20.573320913Z stderr F 		k8s.io/apimachinery/pkg/util/wait/wait.go:70 +0x73
```

Might have been introduced with 
https://github.com/kubernetes/kubernetes/commit/50c12437604b0cd5a73514389409fc2fde8b91bd


### What did you expect to happen?

No crash.

### How can we reproduce it (as minimally and precisely as possible)?

```
❯ kind create cluster --name ovn
Creating cluster "ovn" ...
 ✓ Ensuring node image (kindest/node:v1.31.0) 🖼 
 ✓ Preparing nodes 📦  
 ✓ Writing configuration 📜 
 ✓ Starting control-plane 🕹️ 
 ✓ Installing CNI 🔌 
 ✓ Installing StorageClass 💾 
Set kubectl context to "kind-ovn"
You can now use your cluster with:

kubectl cluster-info --context kind-ovn

Not sure what to do next? 😅  Check out https://kind.sigs.k8s.io/docs/user/quick-start/

❯ kind export kubeconfig --name ovn
Set kubectl context to "kind-ovn"

❯ kubectl get pods -A                                                                           
NAMESPACE            NAME                                        READY   STATUS    RESTARTS   AGE
kube-system          coredns-6f6b679f8f-2xrm5                    1/1     Running   0          2m57s
kube-system          coredns-6f6b679f8f-zpkp2                    1/1     Running   0          2m57s
kube-system          etcd-ovn-control-plane                      1/1     Running   0          3m6s
kube-system          kindnet-22kmz                               1/1     Running   0          2m58s
kube-system          kube-apiserver-ovn-control-plane            1/1     Running   0          3m5s
kube-system          kube-controller-manager-ovn-control-plane   1/1     Running   0          3m5s
kube-system          kube-proxy-8z75j                            1/1     Running   0          2m58s
kube-system          kube-scheduler-ovn-control-plane            1/1     Running   0          3m5s
local-path-storage   local-path-provisioner-57c5987fd4-sgvtl     1/1     Running   0          2m57s

❯ cat service.yaml                                                  
apiVersion: v1
kind: Service
metadata:
  name: example-service
spec:
  selector:
    app: example
  ports:
    - port: 8765
      targetPort: 9376
  type: LoadBalancer

❯ kubectl apply -f service.yaml    
service/example-service created

❯ kubectl get pods -A           
NAMESPACE            NAME                                        READY   STATUS    RESTARTS   AGE
kube-system          coredns-6f6b679f8f-2xrm5                    1/1     Running   0          5m15s
kube-system          coredns-6f6b679f8f-zpkp2                    1/1     Running   0          5m15s
kube-system          etcd-ovn-control-plane                      1/1     Running   0          5m24s
kube-system          kindnet-22kmz                               1/1     Running   0          5m16s
kube-system          kube-apiserver-ovn-control-plane            1/1     Running   0          5m23s
kube-system          kube-controller-manager-ovn-control-plane   1/1     Running   0          5m23s
kube-system          kube-proxy-8z75j                            1/1     Running   0          5m16s
kube-system          kube-scheduler-ovn-control-plane            1/1     Running   0          5m23s
local-path-storage   local-path-provisioner-57c5987fd4-sgvtl     1/1     Running   0          5m15s

❯ kubectl patch service example-service -p '{"spec":{"externalTrafficPolicy":"Local"}}'
service/example-service patched

❯ kubectl get pods -A                                                                  
NAMESPACE            NAME                                        READY   STATUS    RESTARTS     AGE
...
kube-system          kube-controller-manager-ovn-control-plane   0/1     Running   1 (2s ago)   6m25s
...
```


### Anything else we need to know?

_No response_

### Kubernetes version

<details>

```console
$ kubectl version
Client Version: v1.30.2
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.31.0
```

</details>


### Cloud provider

<details>
None
</details>


### OS version

<details>

```console
# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here
```

</details>


### Install tools

<details>

</details>


### Container runtime (CRI) and version (if applicable)

<details>

</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>

</details>



Discussion / hints:
This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the `triage/accepted` label and provide further guidance.

The `triage/accepted` label can be added by org members by writing `/triage accepted` in a comment.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
/sig cloud-provider
/assign
/sig network
> it looks like the service-lb-controller is instantiated but not started.

@carlory in addition to your change, these controllers should not be instantiated at all if there is no cloud provider

I discussed this with @elmiko here https://github.com/kubernetes/kubernetes/pull/127711/files#r1780628883

So your change look correct, but I don't think those controllers should be instantiated at all, and that seems to be easier to backport
/priority important-soon
/kind regression

Fix patch:
diff --git a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go b/staging/src/k8s.io/cloud-provider/controllers/service/controller.go
index 4d3d30c8b1de3..b0360d56d238f 100644
--- a/staging/src/k8s.io/cloud-provider/controllers/service/controller.go
+++ b/staging/src/k8s.io/cloud-provider/controllers/service/controller.go
@@ -139,7 +139,7 @@ func New(
 			UpdateFunc: func(old, cur interface{}) {
 				oldSvc, ok1 := old.(*v1.Service)
 				curSvc, ok2 := cur.(*v1.Service)
-				if ok1 && ok2 && (s.needsUpdate(oldSvc, curSvc) || needsCleanup(curSvc)) {
+				if ok1 && ok2 && (needsUpdate(oldSvc, curSvc) || needsCleanup(curSvc)) {
 					s.enqueueService(cur)
 				}
 			},
@@ -552,19 +552,17 @@ func needsCleanup(service *v1.Service) bool {
 }
 
 // needsUpdate checks if load balancer needs to be updated due to change in attributes.
-func (c *Controller) needsUpdate(oldService *v1.Service, newService *v1.Service) bool {
+func needsUpdate(oldService *v1.Service, newService *v1.Service) bool {
 	if !wantsLoadBalancer(oldService) && !wantsLoadBalancer(newService) {
 		return false
 	}
 	if wantsLoadBalancer(oldService) != wantsLoadBalancer(newService) {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "Type", "%v -> %v",
-			oldService.Spec.Type, newService.Spec.Type)
+		klog.V(2).Infof("Service %s wants load balancer changed from %s to %s", klog.KObj(oldService), oldService.Spec.Type, newService.Spec.Type)
 		return true
 	}
 
 	if wantsLoadBalancer(newService) && !reflect.DeepEqual(oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges) {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "LoadBalancerSourceRanges", "%v -> %v",
-			oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges)
+		klog.V(2).Infof("Service %s LoadBalancerSourceRanges changed from %v to %v", klog.KObj(newService), oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges)
 		return true
 	}
 
@@ -576,19 +574,16 @@ func (c *Controller) needsUpdate(oldService *v1.Service, newService *v1.Service)
 		return true
 	}
 	if !loadBalancerIPsAreEqual(oldService, newService) {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "LoadbalancerIP", "%v -> %v",
-			oldService.Spec.LoadBalancerIP, newService.Spec.LoadBalancerIP)
+		klog.V(2).Infof("Service %s LoadBalancerIP changed from %s to %s", klog.KObj(newService), oldService.Spec.LoadBalancerIP, newService.Spec.LoadBalancerIP)
 		return true
 	}
 	if len(oldService.Spec.ExternalIPs) != len(newService.Spec.ExternalIPs) {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "ExternalIP", "Count: %v -> %v",
-			len(oldService.Spec.ExternalIPs), len(newService.Spec.ExternalIPs))
+		klog.V(2).Infof("Service %s ExternalIPs' count changed from %v to %v", klog.KObj(newService), len(oldService.Spec.ExternalIPs), len(newService.Spec.ExternalIPs))
 		return true
 	}
 	for i := range oldService.Spec.ExternalIPs {
 		if oldService.Spec.ExternalIPs[i] != newService.Spec.ExternalIPs[i] {
-			c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "ExternalIP", "Added: %v",
-				newService.Spec.ExternalIPs[i])
+			klog.V(2).Infof("Service %s ExternalIPs[%d] changed from %v to %v", klog.KObj(newService), i, oldService.Spec.ExternalIPs[i], newService.Spec.ExternalIPs[i])
 			return true
 		}
 	}
@@ -596,18 +591,15 @@ func (c *Controller) needsUpdate(oldService *v1.Service, newService *v1.Service)
 		return true
 	}
 	if oldService.UID != newService.UID {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "UID", "%v -> %v",
-			oldService.UID, newService.UID)
+		klog.V(2).Infof("Service %s UID changed from %s to %s", klog.KObj(newService), oldService.UID, newService.UID)
 		return true
 	}
 	if oldService.Spec.ExternalTrafficPolicy != newService.Spec.ExternalTrafficPolicy {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "ExternalTrafficPolicy", "%v -> %v",
-			oldService.Spec.ExternalTrafficPolicy, newService.Spec.ExternalTrafficPolicy)
+		klog.V(2).Infof("Service %s ExternalTrafficPolicy changed from %s to %s", klog.KObj(newService), oldService.Spec.ExternalTrafficPolicy, newService.Spec.ExternalTrafficPolicy)
 		return true
 	}
 	if oldService.Spec.HealthCheckNodePort != newService.Spec.HealthCheckNodePort {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "HealthCheckNodePort", "%v -> %v",
-			oldService.Spec.HealthCheckNodePort, newService.Spec.HealthCheckNodePort)
+		klog.V(2).Infof("Service %s HealthCheckNodePort changed from %v to %v", klog.KObj(newService), oldService.Spec.HealthCheckNodePort, newService.Spec.HealthCheckNodePort)
 		return true
 	}
 
@@ -615,8 +607,7 @@ func (c *Controller) needsUpdate(oldService *v1.Service, newService *v1.Service)
 	// but CAN NOT change primary/secondary clusterIP || ipFamily UNLESS they are changing from/to/ON ExternalName
 	// so not care about order, only need check the length.
 	if len(oldService.Spec.IPFamilies) != len(newService.Spec.IPFamilies) {
-		c.eventRecorder.Eventf(newService, v1.EventTypeNormal, "IPFamilies", "Count: %v -> %v",
-			len(oldService.Spec.IPFamilies), len(newService.Spec.IPFamilies))
+		klog.V(2).Infof("Service %s IPFamilies' count changed from %d to %d", klog.KObj(newService), len(oldService.Spec.IPFamilies), len(newService.Spec.IPFamilies))
 		return true
 	}
 


Test patch:
diff --git a/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go b/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go
index 4c710a7a6d588..391ef8be3c366 100644
--- a/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go
+++ b/staging/src/k8s.io/cloud-provider/controllers/service/controller_test.go
@@ -1498,12 +1498,8 @@ func TestNeedsUpdate(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.testName, func(t *testing.T) {
-			_, ctx := ktesting.NewTestContext(t)
-			ctx, cancel := context.WithCancel(ctx)
-			defer cancel()
-			controller, _, _ := newController(ctx)
 			oldSvc, newSvc := tc.updateFn()
-			obtainedResult := controller.needsUpdate(oldSvc, newSvc)
+			obtainedResult := needsUpdate(oldSvc, newSvc)
 			if obtainedResult != tc.expectedNeedsUpdate {
 				t.Errorf("%v needsUpdate() should have returned %v but returned %v", tc.testName, tc.expectedNeedsUpdate, obtainedResult)
 			}