plugin/{file,auto}: drop fsnotify (#1090)

* plugin/{file,auto}: drop fsnotify

Reload every minute. This is more deterministic then fsnotify. Also
other thing cropped up: sharing zone files between zone; there is only
1 fsnotify event and we need to fan out the reload to all zone files.
This is a large rewrite (which could still be done), for now, poll the
zone file on disk.

Give serial no change a special error type so we can check for this.
Improve the logging for reloading:

2017/09/19 07:34:39 [INFO] Successfully reloaded zone "miek.nl." in "db.miek.nl" with serial 128263060
2017/09/19 07:34:45 [INFO] Successfully reloaded zone "miek.nl." in "db.miek.nl" with serial 128263059
2017/09/19 07:34:51 [INFO] Successfully reloaded zone "miek.nl." in "db.miek.nl" with serial 128263060

Fixes #1013

* typo

plugin/auto/README.md

@@ -28,8 +28,8 @@ are used.
   name `db.example.com`, the extracted origin will be `example.com`. **TIMEOUT** specifies how often
   CoreDNS should scan the directory, the default is every 60 seconds. This value is in seconds.
   The minimum value is 1 second.
-* `no_reload` by default CoreDNS will reload a zone from disk whenever it detects a change to the
+* `no_reload` by default CoreDNS will try to reload a zone every minute and reloads if the
-  file. This option disables that behavior.
+  SOA's serial has changed. This option disables that behavior.
 * `upstream` defines upstream resolvers to be used resolve external names found (think CNAMEs)
   pointing to external names. **ADDRESS** can be an IP address, and IP:port or a string pointing to
   a file that is structured as /etc/resolv.conf.

plugin/file/README.md

@@ -20,9 +20,6 @@ file DBFILE [ZONES...]
 
 If you want to round robin A and AAAA responses look at the *loadbalance* plugin.
 
-TSIG key configuration is TODO; directive format for transfer will probably be extended with
-TSIG key information, something like `transfer out [ADDRESS...] key [NAME[:ALG]] [BASE64]`
-
 ~~~
 file DBFILE [ZONES... ] {
     transfer to ADDRESS...
@@ -35,8 +32,8 @@ file DBFILE [ZONES... ] {
   the direction. **ADDRESS** must be denoted in CIDR notation (127.0.0.1/32 etc.) or just as plain
   addresses. The special wildcard `*` means: the entire internet (only valid for 'transfer to').
   When an address is specified a notify message will be send whenever the zone is reloaded.
-* `no_reload` by default CoreDNS will reload a zone from disk whenever it detects a change to the
+* `no_reload` by default CoreDNS will try to reload a zone every minute and reloads if the
-  file. This option disables that behavior.
+  SOA's serial has changed. This option disables that behavior.
 * `upstream` defines upstream resolvers to be used resolve external names found (think CNAMEs)
   pointing to external names. This is only really useful when CoreDNS is configured as a proxy, for
   normal authoritative serving you don't need *or* want to use this. **ADDRESS** can be an IP

plugin/file/file.go

@@ -105,6 +105,17 @@ func (f File) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (i
 // Name implements the Handler interface.
 func (f File) Name() string { return "file" }
 
+type serialErr struct {
+	err    string
+	zone   string
+	origin string
+	serial int64
+}
+
+func (s *serialErr) Error() string {
+	return fmt.Sprintf("%s for origin %s in file %s, with serial %d", s.err, s.zone, s.serial)
+}
+
 // Parse parses the zone in filename and returns a new Zone or an error.
 // If serial >= 0 it will reload the zone, if the SOA hasn't changed
 // it returns an error indicating nothing was read.
@@ -119,8 +130,8 @@ func Parse(f io.Reader, origin, fileName string, serial int64) (*Zone, error) {
 
 		if !seenSOA && serial >= 0 {
 			if s, ok := x.RR.(*dns.SOA); ok {
-				if s.Serial == uint32(serial) { // same zone
+				if s.Serial == uint32(serial) { // same serial
-					return nil, fmt.Errorf("no change in serial: %d", serial)
+					return nil, &serialErr{err: "no change in SOA serial", origin: origin, zone: fileName, serial: serial}
 				}
 				seenSOA = true
 			}

plugin/file/reload.go

@@ -3,42 +3,38 @@ package file
 import (
 	"log"
 	"os"
-	"path"
+	"time"
-
-	"github.com/fsnotify/fsnotify"
 )
 
+// TickTime is the default time we use to reload zone. Exported to be tweaked in tests.
+var TickTime = 1 * time.Minute
+
 // Reload reloads a zone when it is changed on disk. If z.NoRoload is true, no reloading will be done.
 func (z *Zone) Reload() error {
 	if z.NoReload {
 		return nil
 	}
-	watcher, err := fsnotify.NewWatcher()
+
-	if err != nil {
+	tick := time.NewTicker(TickTime)
-		return err
-	}
-	err = watcher.Add(path.Dir(z.file))
-	if err != nil {
-		return err
-	}
 
 	go func() {
-		// TODO(miek): needs to be killed on reload.
+
 		for {
 			select {
-			case event := <-watcher.Events:
-				if path.Clean(event.Name) == z.file {
 
+			case <-tick.C:
 				reader, err := os.Open(z.file)
 				if err != nil {
-						log.Printf("[ERROR] Failed to open `%s' for `%s': %v", z.file, z.origin, err)
+					log.Printf("[ERROR] Failed to open zone %q in %q: %v", z.origin, z.file, err)
 					continue
 				}
 
 				serial := z.SOASerialIfDefined()
 				zone, err := Parse(reader, z.origin, z.file, serial)
 				if err != nil {
-						log.Printf("[WARNING] Parsing zone `%s': %v", z.origin, err)
+					if _, ok := err.(*serialErr); !ok {
+						log.Printf("[ERROR] Parsing zone %q: %v", z.origin, err)
+					}
 					continue
 				}
 
@@ -48,11 +44,11 @@ func (z *Zone) Reload() error {
 				z.Tree = zone.Tree
 				z.reloadMu.Unlock()
 
-					log.Printf("[INFO] Successfully reloaded zone `%s'", z.origin)
+				log.Printf("[INFO] Successfully reloaded zone %q in %q with serial %d", z.origin, z.file, z.Apex.SOA.Serial)
 				z.Notify()
-				}
+
 			case <-z.ReloadShutdown:
-				watcher.Close()
+				tick.Stop()
 				return
 			}
 		}

plugin/file/reload_test.go

@@ -31,7 +31,9 @@ func TestZoneReload(t *testing.T) {
 		t.Fatalf("failed to parse zone: %s", err)
 	}
 
+	TickTime = 500 * time.Millisecond
 	z.Reload()
+	time.Sleep(time.Second)
 
 	r := new(dns.Msg)
 	r.SetQuestion("miek.nl", dns.TypeSOA)

test/file_reload_test.go

@@ -2,10 +2,10 @@ package test
 
 import (
 	"io/ioutil"
-	"log"
 	"testing"
 	"time"
 
+	"github.com/coredns/coredns/plugin/file"
 	"github.com/coredns/coredns/plugin/proxy"
 	"github.com/coredns/coredns/plugin/test"
 	"github.com/coredns/coredns/request"
@@ -14,8 +14,7 @@ import (
 )
 
 func TestZoneReload(t *testing.T) {
-	t.Parallel()
+	file.TickTime = 1 * time.Second
-	log.SetOutput(ioutil.Discard)
 
 	name, rm, err := TempFile(".", exampleOrg)
 	if err != nil {
@@ -52,7 +51,7 @@ example.net:0 {
 	// Remove RR from the Apex
 	ioutil.WriteFile(name, []byte(exampleOrgUpdated), 0644)
 
-	time.Sleep(1 * time.Second) // fsnotify
+	time.Sleep(2 * time.Second) // reload time
 
 	resp, err = p.Lookup(state, "example.org.", dns.TypeA)
 	if err != nil {