Plugin/RELOAD - Tune usage of var global, add limit to options (#1457)

* tune usage of var global, add limit to options

* update readme for minimal values

* useless change to quick-off codecov

* fix msgs for min values and tune the flag for end of reload usage, with a 'maybe' option

* adding UT for min values, adding MD5 of corefile on the log

plugin/reload/README.md

@@ -35,6 +35,7 @@ reload [INTERVAL] [JITTER]
 * The plugin will check for changes every **INTERVAL**, subject to +/- the **JITTER** duration
 * **INTERVAL** and **JITTER** are Golang (durations)[https://golang.org/pkg/time/#ParseDuration]
 * Default **INTERVAL** is 30s, default **JITTER** is 15s
+* Minimal value for **INTERVAL** is 2s, and for **JITTER** is 1s
 * If **JITTER** is more than half of **INTERVAL**, it will be set to half of **INTERVAL**
 
 ## Examples

plugin/reload/reload.go

@@ -9,12 +9,15 @@ import (
 )
 
 // reload periodically checks if the Corefile has changed, and reloads if so
+const (
+	unused    = 0
+	maybeUsed = 1
+	used      = 2
+)
 
 type reload struct {
-	instance *caddy.Instance
 	interval time.Duration
-	sum      [md5.Size]byte
-	stopped  bool
+	usage    int
 	quit     chan bool
 }
 
@@ -26,13 +29,14 @@ func hook(event caddy.EventName, info interface{}) error {
 	// if reload is removed from the Corefile, then the hook
 	// is still registered but setup is never called again
 	// so we need a flag to tell us not to reload
-	if r.stopped {
+	if r.usage == unused {
 		return nil
 	}
 
 	// this should be an instance. ok to panic if not
-	r.instance = info.(*caddy.Instance)
-	r.sum = md5.Sum(r.instance.Caddyfile().Body())
+	instance := info.(*caddy.Instance)
+	md5sum := md5.Sum(instance.Caddyfile().Body())
+	log.Printf("[INFO] Running configuration MD5 = %x\n", md5sum)
 
 	go func() {
 		tick := time.NewTicker(r.interval)
@@ -40,19 +44,26 @@ func hook(event caddy.EventName, info interface{}) error {
 		for {
 			select {
 			case <-tick.C:
-				corefile, err := caddy.LoadCaddyfile(r.instance.Caddyfile().ServerType())
+				corefile, err := caddy.LoadCaddyfile(instance.Caddyfile().ServerType())
 				if err != nil {
 					continue
 				}
 				s := md5.Sum(corefile.Body())
-				if s != r.sum {
-					_, err := r.instance.Restart(corefile)
+				if s != md5sum {
+					// Let not try to restart with the same file, even though it is wrong.
+					md5sum = s
+					// now lets consider that plugin will not be reload, unless appear in next config file
+					// change status iof usage will be reset in setup if the plugin appears in config file
+					r.usage = maybeUsed
+					_, err := instance.Restart(corefile)
 					if err != nil {
 						log.Printf("[ERROR] Corefile changed but reload failed: %s\n", err)
 						continue
 					}
-					// we are done, this hook gets called again with new instance
-					r.stopped = true
+					// we are done, if the plugin was not set used, then it is not.
+					if r.usage == maybeUsed {
+						r.usage = unused
+					}
 					return
 				}
 			case <-r.quit:

plugin/reload/setup.go

@@ -1,6 +1,7 @@
 package reload
 
 import (
+	"fmt"
 	"math/rand"
 	"sync"
 	"time"
@@ -17,7 +18,11 @@ func init() {
 	})
 }
 
-var r *reload
+// the info reload is global to all application, whatever number of reloads.
+// it is used to transmit data between Setup and start of the hook called 'onInstanceStartup'
+// channel for QUIT is never changed in purpose.
+// WARNING: this data may be unsync after an invalid attempt of reload Corefile.
+var r = reload{interval: defaultInterval, usage: unused, quit: make(chan bool)}
 var once sync.Once
 
 func setup(c *caddy.Controller) error {
@@ -32,19 +37,25 @@ func setup(c *caddy.Controller) error {
 	if len(args) > 0 {
 		d, err := time.ParseDuration(args[0])
 		if err != nil {
-			return err
+			return plugin.Error("reload", err)
 		}
 		i = d
 	}
+	if i < minInterval {
+		return plugin.Error("reload", fmt.Errorf("interval value must be greater or equal to %v", minInterval))
+	}
 
 	j := defaultJitter
 	if len(args) > 1 {
 		d, err := time.ParseDuration(args[1])
 		if err != nil {
-			return err
+			return plugin.Error("reload", err)
 		}
 		j = d
 	}
+	if j < minJitter {
+		return plugin.Error("reload", fmt.Errorf("jitter value must be greater or equal to %v", minJitter))
+	}
 
 	if j > i/2 {
 		j = i / 2
@@ -53,20 +64,25 @@ func setup(c *caddy.Controller) error {
 	jitter := time.Duration(rand.Int63n(j.Nanoseconds()) - (j.Nanoseconds() / 2))
 	i = i + jitter
 
-	r = &reload{interval: i, quit: make(chan bool)}
+	// prepare info for next onInstanceStartup event
+	r.interval = i
+	r.usage = used
+
 	once.Do(func() {
 		caddy.RegisterEventHook("reload", hook)
 	})
 
+	// re-register on finalShutDown as the instance most-likely will be changed
 	c.OnFinalShutdown(func() error {
 		r.quit <- true
 		return nil
 	})
-
 	return nil
 }
 
 const (
+	minJitter       = 1 * time.Second
+	minInterval     = 2 * time.Second
 	defaultInterval = 30 * time.Second
 	defaultJitter   = 15 * time.Second
 )

plugin/reload/setup_test.go

@@ -36,4 +36,16 @@ func TestSetupReload(t *testing.T) {
 	if err := setup(c); err == nil {
 		t.Fatalf("Expected errors, but got: %v", err)
 	}
+	c = caddy.NewTestController("dns", `reload 1s`)
+	if err := setup(c); err == nil {
+		t.Fatalf("Expected errors, but got: %v", err)
+	}
+	c = caddy.NewTestController("dns", `reload 0s`)
+	if err := setup(c); err == nil {
+		t.Fatalf("Expected errors, but got: %v", err)
+	}
+	c = caddy.NewTestController("dns", `reload 3s 0.5s`)
+	if err := setup(c); err == nil {
+		t.Fatalf("Expected errors, but got: %v", err)
+	}
 }