Condense health checkers into one health checker for all backends. #264
Conversation
k8s-ci-robot
added
cncf-cla: yes
cmd/glbc/main.go
Outdated
| @@ -90,15 +90,15 @@ func main() { | |||
|
|
|||
| cloud := app.NewGCEClient() | |||
| defaultBackendServicePort := app.DefaultBackendServicePort(kubeClient) | |||
| clusterManager, err := controller.NewClusterManager(cloud, namer, flags.F.HealthCheckPath) | |||
| clusterManager, err := controller.NewClusterManager(cloud, namer, *defaultBackendServicePort, flags.F.HealthCheckPath) | |||
There was a problem hiding this comment.
I suggest we get rid of the app.DefaultBackendServicePort call. We don't really care that the service exists until an ingress sync. Using the client to fetch it now seems racey for new clusters.
Maybe create a utility function that converts strings of "namespace/name" to types.NamespacedName and use that to convert the flag value. Then pass that value to the LBC?
There was a problem hiding this comment.
Good point, can I do this in a follow up PR? It's slightly more involved than just removing that call so I want to spend more time thinking about it.
pkg/healthchecks/healthchecks.go
Outdated
| @@ -32,6 +33,9 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| // DefaultBackendHealthCheckPath defines the path that the system default | |||
| // backend should serve 200's on. | |||
| DefaultBackendHealthCheckPath = "/healthz" | |||
There was a problem hiding this comment.
Something tells me that we should make this a flag that defaults to '/healthz'. Since the default backend is modifiable, this should probably be too.
rramkumar1
force-pushed
the
fix-health-checks
branch
from
f203295
to
8ae3af9
Compare
rramkumar1
force-pushed
the
fix-health-checks
branch
from
8ae3af9
to
04b548e
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This PR should wait on #262
TODOs before merge:
/assign @nicksardo