Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Differentiate liveness and readiness probes for router pods #19009

Merged
merged 2 commits into from
May 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions pkg/cmd/infra/router/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ func (o *TemplateRouterOptions) Validate() error {
// Run launches a template router using the provided options. It never exits.
func (o *TemplateRouterOptions) Run() error {
glog.Infof("Starting template router (%s)", version.Get())
var ptrTemplatePlugin *templateplugin.TemplatePlugin

var reloadCallbacks []func()

Expand Down Expand Up @@ -310,9 +311,15 @@ func (o *TemplateRouterOptions) Run() error {
if err != nil {
return fmt.Errorf("ROUTER_METRICS_READY_HTTP_URL must be a valid URL or empty: %v", err)
}
check := metrics.HTTPBackendAvailable(u)
checkBackend := metrics.HTTPBackendAvailable(u)
if isTrue(util.Env("ROUTER_USE_PROXY_PROTOCOL", "")) {
check = metrics.ProxyProtocolHTTPBackendAvailable(u)
checkBackend = metrics.ProxyProtocolHTTPBackendAvailable(u)
}
checkSync := metrics.HasSynced(&ptrTemplatePlugin)
checkController := metrics.ControllerLive()
liveChecks := []healthz.HealthzChecker{checkController}
if !(isTrue(util.Env("ROUTER_BIND_PORTS_BEFORE_SYNC", ""))) {
liveChecks = append(liveChecks, checkBackend)
}

kubeconfig := o.Config.KubeConfig()
Expand Down Expand Up @@ -353,7 +360,8 @@ func (o *TemplateRouterOptions) Run() error {
Resource: "routers",
Name: o.RouterName,
},
Checks: []healthz.HealthzChecker{check},
LiveChecks: liveChecks,
ReadyChecks: []healthz.HealthzChecker{checkBackend, checkSync},
}
if certFile := util.Env("ROUTER_METRICS_TLS_CERT_FILE", ""); len(certFile) > 0 {
certificate, err := tls.LoadX509KeyPair(certFile, util.Env("ROUTER_METRICS_TLS_KEY_FILE", ""))
Expand Down Expand Up @@ -411,6 +419,7 @@ func (o *TemplateRouterOptions) Run() error {
if err != nil {
return err
}
ptrTemplatePlugin = templatePlugin

factory := o.RouterSelection.NewFactory(routeclient, projectclient.Project().Projects(), kc)
factory.RouteModifierFn = o.RouteUpdate
Expand Down
19 changes: 9 additions & 10 deletions pkg/oc/admin/router/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,8 @@ const (
// Default port numbers to expose and bind/listen on.
defaultPorts = "80:80,443:443"

// Default stats and healthz port.
defaultStatsPort = 1936
defaultHealthzPort = defaultStatsPort
// Default stats port.
defaultStatsPort = 1936
)

// NewCmdRouter implements the OpenShift CLI router command.
Expand Down Expand Up @@ -436,21 +435,21 @@ func generateSecretsConfig(cfg *RouterConfig, namespace string, defaultCert []by
return secrets, volumes, mounts, nil
}

func generateProbeConfigForRouter(cfg *RouterConfig, ports []kapi.ContainerPort) *kapi.Probe {
func generateProbeConfigForRouter(path string, cfg *RouterConfig, ports []kapi.ContainerPort) *kapi.Probe {
var probe *kapi.Probe

if cfg.Type == "haproxy-router" {
probe = &kapi.Probe{}
healthzPort := defaultHealthzPort
probePort := defaultStatsPort
if cfg.StatsPort > 0 {
healthzPort = cfg.StatsPort
probePort = cfg.StatsPort
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajatchopra @pravisankar does this make it clearer? removing the reference to the healthzPort in favour of a more generic term

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

}

probe.Handler.HTTPGet = &kapi.HTTPGetAction{
Path: "/healthz",
Path: path,
Port: intstr.IntOrString{
Type: intstr.Int,
IntVal: int32(healthzPort),
IntVal: int32(probePort),
},
}

Expand All @@ -466,15 +465,15 @@ func generateProbeConfigForRouter(cfg *RouterConfig, ports []kapi.ContainerPort)
}

func generateLivenessProbeConfig(cfg *RouterConfig, ports []kapi.ContainerPort) *kapi.Probe {
probe := generateProbeConfigForRouter(cfg, ports)
probe := generateProbeConfigForRouter("/healthz", cfg, ports)
if probe != nil {
probe.InitialDelaySeconds = 10
}
return probe
}

func generateReadinessProbeConfig(cfg *RouterConfig, ports []kapi.ContainerPort) *kapi.Probe {
probe := generateProbeConfigForRouter(cfg, ports)
probe := generateProbeConfigForRouter("healthz/ready", cfg, ports)
if probe != nil {
probe.InitialDelaySeconds = 10
}
Expand Down
23 changes: 23 additions & 0 deletions pkg/router/metrics/health.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/golang/glog"

templateplugin "github.com/openshift/origin/pkg/router/template"
"k8s.io/apiserver/pkg/server/healthz"
"k8s.io/kubernetes/pkg/probe"
probehttp "k8s.io/kubernetes/pkg/probe/http"
Expand All @@ -35,6 +36,28 @@ func HTTPBackendAvailable(u *url.URL) healthz.HealthzChecker {
})
}

// HasSynced returns a healthz check that verifies the router has been synced at least
// once.
func HasSynced(router **templateplugin.TemplatePlugin) healthz.HealthzChecker {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort of construct is a violation of our style guides. You should be passing an interface here that exposes the correct check method. Double pointers should never be used. Nothing in metrics should be aware of template plugin at all.

return healthz.NamedCheck("has-synced", func(r *http.Request) error {
if router != nil {
if (*router).Router.SyncedAtLeastOnce() == true {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construct is not appropriate. It should always be if booleancondition {

return nil
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you return early, elide the else, as per our style guide.

return fmt.Errorf("Router not synced")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors should always be lower case, as per the style guide.

}
}
return nil
})
}

func ControllerLive() healthz.HealthzChecker {
return healthz.NamedCheck("controller", func(r *http.Request) error {
return nil
})

}

// ProxyProtocolHTTPBackendAvailable returns a healthz check that verifies a backend supporting
// the HAProxy PROXY protocol responds to a GET to the provided URL with 2xx or 3xx response.
func ProxyProtocolHTTPBackendAvailable(u *url.URL) healthz.HealthzChecker {
Expand Down
6 changes: 4 additions & 2 deletions pkg/router/metrics/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,14 @@ type Listener struct {
Authorizer authorizer.Authorizer
Record authorizer.AttributesRecord

Checks []healthz.HealthzChecker
LiveChecks []healthz.HealthzChecker
ReadyChecks []healthz.HealthzChecker
}

func (l Listener) handler() http.Handler {
mux := http.NewServeMux()
healthz.InstallHandler(mux, l.Checks...)
healthz.InstallHandler(mux, l.LiveChecks...)
healthz.InstallPathHandler(mux, "/healthz/ready", l.ReadyChecks...)

if l.Authenticator != nil {
protected := http.NewServeMux()
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.