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

Change the router reload supression, and add some more controls #16564

Closed
Closed
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
4 changes: 4 additions & 0 deletions contrib/completions/bash/openshift
Original file line number Diff line number Diff line change
Expand Up @@ -24282,6 +24282,10 @@ _openshift_infra_router()
local_nonpersistent_flags+=("--project-labels=")
flags+=("--reload=")
local_nonpersistent_flags+=("--reload=")
flags+=("--reload-event-wait=")
local_nonpersistent_flags+=("--reload-event-wait=")
flags+=("--reload-gap=")
local_nonpersistent_flags+=("--reload-gap=")
flags+=("--request-timeout=")
local_nonpersistent_flags+=("--request-timeout=")
flags+=("--resync-interval=")
Expand Down
4 changes: 4 additions & 0 deletions contrib/completions/zsh/openshift
Original file line number Diff line number Diff line change
Expand Up @@ -24431,6 +24431,10 @@ _openshift_infra_router()
local_nonpersistent_flags+=("--project-labels=")
flags+=("--reload=")
local_nonpersistent_flags+=("--reload=")
flags+=("--reload-event-wait=")
local_nonpersistent_flags+=("--reload-event-wait=")
flags+=("--reload-gap=")
local_nonpersistent_flags+=("--reload-gap=")
flags+=("--request-timeout=")
local_nonpersistent_flags+=("--request-timeout=")
flags+=("--resync-interval=")
Expand Down
34 changes: 23 additions & 11 deletions pkg/cmd/infra/router/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ import (
"github.com/openshift/origin/pkg/version"
)

// defaultReloadInterval is how often to do reloads in seconds.
const defaultReloadInterval = 5

var routerLong = templates.LongDesc(`
Start a router

Expand Down Expand Up @@ -68,6 +65,8 @@ type TemplateRouter struct {
TemplateFile string
ReloadScript string
ReloadInterval time.Duration
ReloadGap time.Duration
ReloadEventWait time.Duration
DefaultCertificate string
DefaultCertificatePath string
DefaultCertificateDir string
Expand All @@ -81,14 +80,15 @@ type TemplateRouter struct {
MetricsType string
}

// reloadInterval returns how often to run the router reloads. The interval
// value is based on an environment variable or the default.
func reloadInterval() time.Duration {
interval := util.Env("RELOAD_INTERVAL", fmt.Sprintf("%vs", defaultReloadInterval))
value, err := time.ParseDuration(interval)
// getDurationEnv() gets retrieves the value of the named environment
// variable and checks that is a valid duration. If it is not, or it
// is not set, it returns the given default.
func getDurationEnv(envName string, defaultDuration time.Duration) time.Duration {
valueString := util.Env(envName, defaultDuration.String())
value, err := time.ParseDuration(valueString)
if err != nil {
glog.Warningf("Invalid RELOAD_INTERVAL %q, using default value %v ...", interval, defaultReloadInterval)
value = time.Duration(defaultReloadInterval * time.Second)
glog.Warningf("Invalid %s %q, using default value %s ...", envName, valueString, defaultDuration.String())
value = defaultDuration
}
return value
}
Expand All @@ -103,7 +103,9 @@ func (o *TemplateRouter) Bind(flag *pflag.FlagSet) {
flag.StringVar(&o.DefaultDestinationCAPath, "default-destination-ca-path", util.Env("DEFAULT_DESTINATION_CA_PATH", "/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt"), "A path to a PEM file containing the default CA bundle to use with re-encrypt routes. This CA should sign for certificates in the Kubernetes DNS space (service.namespace.svc).")
flag.StringVar(&o.TemplateFile, "template", util.Env("TEMPLATE_FILE", ""), "The path to the template file to use")
flag.StringVar(&o.ReloadScript, "reload", util.Env("RELOAD_SCRIPT", ""), "The path to the reload script to use")
flag.DurationVar(&o.ReloadInterval, "interval", reloadInterval(), "Controls how often router reloads are invoked. Mutiple router reload requests are coalesced for the duration of this interval since the last reload time.")
flag.DurationVar(&o.ReloadInterval, "interval", getDurationEnv("RELOAD_INTERVAL", 5*time.Second), "Controls how often router reloads are invoked. Mutiple router reload requests are coalesced for the duration of this interval since the time the last reload started.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Typos

flag.DurationVar(&o.ReloadGap, "reload-gap", getDurationEnv("RELOAD_GAP", 100*time.Millisecond), "Controls how often router reloads are invoked. Mutiple router reload requests are coalesced for the duration of this interval since the time the last reload ended.")
Copy link
Contributor

Choose a reason for hiding this comment

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

How often are we really going to tune these? Adding lots of extra tunables comes with a cost - do these really need to be changeable or can they just be proportional to the overall duration?

Every tunable we add is more choices that someone can get wrong. Also, I couldn't understand how the flags are used from the descriptions, which is a problem.

flag.DurationVar(&o.ReloadEventWait, "reload-event-wait", getDurationEnv("RELOAD_EVENT_WAIT", 10*time.Millisecond), "Controls how often router reloads are invoked. Mutiple router reload events are gathered for the given duration before a reload is triggered.")
flag.BoolVar(&o.ExtendedValidation, "extended-validation", util.Env("EXTENDED_VALIDATION", "true") == "true", "If set, then an additional extended validation step is performed on all routes admitted in by this router. Defaults to true and enables the extended validation checks.")
flag.BoolVar(&o.BindPortsAfterSync, "bind-ports-after-sync", util.Env("ROUTER_BIND_PORTS_AFTER_SYNC", "") == "true", "Bind ports only after route state has been synchronized")
flag.StringVar(&o.MaxConnections, "max-connections", util.Env("ROUTER_MAX_CONNECTIONS", ""), "Specifies the maximum number of concurrent connections.")
Expand Down Expand Up @@ -205,6 +207,14 @@ func (o *TemplateRouterOptions) Complete() error {
return fmt.Errorf("invalid reload interval: %v - must be a positive duration", nsecs)
}

if o.ReloadGap < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in validate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the other checks in here be in validate too? Or is there some pattern that I don't grok fully.

return fmt.Errorf("invalid reload gap: %s - must not be negative", o.ReloadGap.String())
}

if o.ReloadEventWait < 0 {
return fmt.Errorf("invalid reload event wait: %s - must not be negative", o.ReloadEventWait.String())
}

if len(routerCanonicalHostname) > 0 {
if errs := validation.IsDNS1123Subdomain(routerCanonicalHostname); len(errs) != 0 {
return fmt.Errorf("invalid canonical hostname: %s", routerCanonicalHostname)
Expand Down Expand Up @@ -324,6 +334,8 @@ func (o *TemplateRouterOptions) Run() error {
TemplatePath: o.TemplateFile,
ReloadScriptPath: o.ReloadScript,
ReloadInterval: o.ReloadInterval,
ReloadGap: o.ReloadGap,
ReloadEventWait: o.ReloadEventWait,
DefaultCertificate: o.DefaultCertificate,
DefaultCertificatePath: o.DefaultCertificatePath,
DefaultCertificateDir: o.DefaultCertificateDir,
Expand Down
14 changes: 8 additions & 6 deletions pkg/router/template/fake.go
Original file line number Diff line number Diff line change
@@ -1,25 +1,27 @@
package templaterouter

import "time"

// NewFakeTemplateRouter provides an empty template router with a simple certificate manager
// backed by a fake cert writer for testing
func NewFakeTemplateRouter() *templateRouter {
fakeCertManager, _ := newSimpleCertificateManager(newFakeCertificateManagerConfig(), &fakeCertWriter{})
return &templateRouter{
state: map[string]ServiceAliasConfig{},
serviceUnits: make(map[string]ServiceUnit),
certManager: fakeCertManager,
rateLimitedCommitFunction: nil,
rateLimitedCommitStopChannel: make(chan struct{}),
state: map[string]ServiceAliasConfig{},
serviceUnits: make(map[string]ServiceUnit),
certManager: fakeCertManager,
}
}

// FakeReloadHandler implements the minimal changes needed to make the locking behavior work
// This MUST match the behavior with the stateChanged of commitAndReload
// This MUST match the behavior with the object updates of commitAndReload() in router.go
func (r *templateRouter) FakeReloadHandler() {
r.lock.Lock()
defer r.lock.Unlock()

r.stateChanged = false
r.lastReloadStart = time.Now()
r.lastReloadEnd = time.Now()

return
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/router/template/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ type TemplatePluginConfig struct {
TemplatePath string
ReloadScriptPath string
ReloadInterval time.Duration
ReloadGap time.Duration
ReloadEventWait time.Duration
ReloadCallbacks []func()
DefaultCertificate string
DefaultCertificatePath string
Expand Down Expand Up @@ -123,6 +125,8 @@ func NewTemplatePlugin(cfg TemplatePluginConfig, lookupSvc ServiceLookup) (*Temp
templates: templates,
reloadScriptPath: cfg.ReloadScriptPath,
reloadInterval: cfg.ReloadInterval,
reloadGap: cfg.ReloadGap,
reloadEventWait: cfg.ReloadEventWait,
reloadCallbacks: cfg.ReloadCallbacks,
defaultCertificate: cfg.DefaultCertificate,
defaultCertificatePath: cfg.DefaultCertificatePath,
Expand Down
Loading