Change the router reload supression, and add some more controls

The main purpose is to change the locking so that a reload doesn't
hold a lock of the router object for the duration of the reload.

The router controls are:
 - $RELOAD_INTERVAL - Max Reload Frequency (start-to-start of a reload), default 5s
 - $RELOAD_GAP - Gaps between reloads (end-to-start of a reload), default 100ms
 - $RELOAD_EVENT_WAIT - Bunch events by waiting a little before reload (wait N ms between
   the time the event comes in and when the reload starts), default 10ms

The commit routine is now changed to have a top and bottom half.  The
top half checks to see if a reload is scheduled, and if it is, it
returns immediately.  If there's no reload scheduled then it calls the
bottom half and returns.

The bottom half is in charge of determining if it can immediately
reload or if it has to wait.  If it must wait, then it works out the
earliest time it can reload and schedules a callback to itself for
that time.

If it determines it can reload, then it runs the reload code
immediately.  When the reload is complete, it calls itself again to
make sure there was no other pending reload that had come in while the
reload was running.

pkg/cmd/infra/router/template.go

@@ -37,6 +37,12 @@ import (
 // defaultReloadInterval is how often to do reloads in seconds.
 const defaultReloadInterval = 5
 
+// defaultReloadGap is the minimum gap between a reload end and start in milliseconds.
+const defaultReloadGap = 100
+
+// defaultReloadEventWait is how long to wait for more events before triggering a reload.  In milliseconds
+const defaultReloadEventWait = 10
+
 var routerLong = templates.LongDesc(`
 	Start a router
 
@@ -68,6 +74,8 @@ type TemplateRouter struct {
 	TemplateFile             string
 	ReloadScript             string
 	ReloadInterval           time.Duration
+	ReloadGap                time.Duration
+	ReloadEventWait          time.Duration
 	DefaultCertificate       string
 	DefaultCertificatePath   string
 	DefaultCertificateDir    string
@@ -87,12 +95,36 @@ func reloadInterval() time.Duration {
 	interval := util.Env("RELOAD_INTERVAL", fmt.Sprintf("%vs", defaultReloadInterval))
 	value, err := time.ParseDuration(interval)
 	if err != nil {
-		glog.Warningf("Invalid RELOAD_INTERVAL %q, using default value %v ...", interval, defaultReloadInterval)
+		glog.Warningf("Invalid RELOAD_INTERVAL %q, using default value %vs ...", interval, defaultReloadInterval)
 		value = time.Duration(defaultReloadInterval * time.Second)
 	}
 	return value
 }
 
+// reloadGap returns how often to wait between a reload end and the start of the next reload.
+// The gap value is based on an environment variable or the default.
+func reloadGap() time.Duration {
+	gap := util.Env("RELOAD_GAP", fmt.Sprintf("%vms", defaultReloadGap))
+	value, err := time.ParseDuration(gap)
+	if err != nil {
+		glog.Warningf("Invalid RELOAD_GAP %q, using default value %vms ...", gap, defaultReloadGap)
+		value = time.Duration(defaultReloadGap * time.Millisecond)
+	}
+	return value
+}
+
+// reloadEventWait returns how often to wait after getting the first event after a reload before triggering the next reload.
+// The event wait value is based on an environment variable or the default.
+func reloadEventWait() time.Duration {
+	eventWait := util.Env("RELOAD_EVENT_WAIT", fmt.Sprintf("%vms", defaultReloadEventWait))
+	value, err := time.ParseDuration(eventWait)
+	if err != nil {
+		glog.Warningf("Invalid RELOAD_EVENT_WAIT %q, using default value %vms ...", eventWait, defaultReloadEventWait)
+		value = time.Duration(defaultReloadEventWait * time.Millisecond)
+	}
+	return value
+}
+
 func (o *TemplateRouter) Bind(flag *pflag.FlagSet) {
 	flag.StringVar(&o.RouterName, "name", util.Env("ROUTER_SERVICE_NAME", "public"), "The name the router will identify itself with in the route status")
 	flag.StringVar(&o.RouterCanonicalHostname, "router-canonical-hostname", util.Env("ROUTER_CANONICAL_HOSTNAME", ""), "CanonicalHostname is the external host name for the router that can be used as a CNAME for the host requested for this route. This value is optional and may not be set in all cases.")
@@ -103,7 +135,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", reloadInterval(), "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.")
+	flag.DurationVar(&o.ReloadGap, "reload-gap", reloadGap(), "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.")
+	flag.DurationVar(&o.ReloadEventWait, "reload-event-wait", reloadEventWait(), "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.")
@@ -205,6 +239,14 @@ func (o *TemplateRouterOptions) Complete() error {
 		return fmt.Errorf("invalid reload interval: %v - must be a positive duration", nsecs)
 	}
 
+	if o.ReloadGap < 0 {
+		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)
@@ -324,6 +366,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,

pkg/router/template/fake.go

@@ -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
 }

pkg/router/template/plugin.go

@@ -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
@@ -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,