daemon: Logging drivers refactoring

- noplog driver pkg for '--log-driver=none' (null object pattern)
- centralized factory for log drivers (instead of case/switch)
- logging drivers registers themselves to factory upon import
  (easy plug/unplug of drivers in daemon/logdrivers.go)
- daemon now doesn't start with an invalid log driver
- Name() method of loggers is actually now their cli names (made it useful)
- generalized Read() logic, made it unsupported except json-file (preserves
  existing behavior)

Spotted some duplication code around processing of legacy json-file
format, didn't touch that and refactored in both places.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>

api/client/logs.go

@@ -37,8 +37,8 @@ func (cli *DockerCli) CmdLogs(args ...string) error {
 		return err
 	}
 
-	if c.HostConfig.LogConfig.Type != "json-file" {
-		return fmt.Errorf("\"logs\" command is supported only for \"json-file\" logging driver")
+	if logType := c.HostConfig.LogConfig.Type; logType != "json-file" {
+		return fmt.Errorf("\"logs\" command is supported only for \"json-file\" logging driver (got: %s)", logType)
 	}
 
 	v := url.Values{}

daemon/container.go

@@ -22,9 +22,7 @@ import (
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/daemon/execdriver"
 	"github.com/docker/docker/daemon/logger"
-	"github.com/docker/docker/daemon/logger/journald"
 	"github.com/docker/docker/daemon/logger/jsonfilelog"
-	"github.com/docker/docker/daemon/logger/syslog"
 	"github.com/docker/docker/daemon/network"
 	"github.com/docker/docker/daemon/networkdriver/bridge"
 	"github.com/docker/docker/image"
@@ -947,18 +945,6 @@ func (container *Container) Unmount() error {
 	return container.daemon.Unmount(container)
 }
 
-func (container *Container) logPath(name string) (string, error) {
-	return container.GetRootResourcePath(fmt.Sprintf("%s-%s.log", container.ID, name))
-}
-
-func (container *Container) ReadLog(name string) (io.Reader, error) {
-	pth, err := container.logPath(name)
-	if err != nil {
-		return nil, err
-	}
-	return os.Open(pth)
-}
-
 func (container *Container) hostConfigPath() (string, error) {
 	return container.GetRootResourcePath("hostconfig.json")
 }
@@ -1445,41 +1431,45 @@ func (container *Container) setupWorkingDirectory() error {
 	return nil
 }
 
-func (container *Container) startLogging() error {
+func (container *Container) getLogConfig() runconfig.LogConfig {
 	cfg := container.hostConfig.LogConfig
-	if cfg.Type == "" {
-		cfg = container.daemon.defaultLogConfig
+	if cfg.Type != "" { // container has log driver configured
+		return cfg
 	}
-	var l logger.Logger
-	switch cfg.Type {
-	case "json-file":
-		pth, err := container.logPath("json")
-		if err != nil {
-			return err
-		}
-		container.LogPath = pth
+	// Use daemon's default log config for containers
+	return container.daemon.defaultLogConfig
+}
 
-		dl, err := jsonfilelog.New(pth)
+func (container *Container) getLogger() (logger.Logger, error) {
+	cfg := container.getLogConfig()
+	c, err := logger.GetLogDriver(cfg.Type)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to get logging factory: %v", err)
+	}
+	ctx := logger.Context{
+		ContainerID:   container.ID,
+		ContainerName: container.Name,
+	}
+
+	// Set logging file for "json-logger"
+	if cfg.Type == jsonfilelog.Name {
+		ctx.LogPath, err = container.GetRootResourcePath(fmt.Sprintf("%s-json.log", container.ID))
 		if err != nil {
-			return err
+			return nil, err
 		}
-		l = dl
-	case "syslog":
-		dl, err := syslog.New(container.ID[:12])
-		if err != nil {
-			return err
-		}
-		l = dl
-	case "journald":
-		dl, err := journald.New(container.ID, container.Name)
-		if err != nil {
-			return err
-		}
-		l = dl
-	case "none":
-		return nil
-	default:
-		return fmt.Errorf("Unknown logging driver: %s", cfg.Type)
+	}
+	return c(ctx)
+}
+
+func (container *Container) startLogging() error {
+	cfg := container.getLogConfig()
+	if cfg.Type == "none" {
+		return nil // do not start logging routines
+	}
+
+	l, err := container.getLogger()
+	if err != nil {
+		return fmt.Errorf("Failed to initialize logging driver: %v", err)
 	}
 
 	copier, err := logger.NewCopier(container.ID, map[string]io.Reader{"stdout": container.StdoutPipe(), "stderr": container.StderrPipe()}, l)
@@ -1490,6 +1480,11 @@ func (container *Container) startLogging() error {
 	copier.Run()
 	container.logDriver = l
 
+	// set LogPath field only for json-file logdriver
+	if jl, ok := l.(*jsonfilelog.JSONFileLogger); ok {
+		container.LogPath = jl.LogPath()
+	}
+
 	return nil
 }
 
@@ -1663,28 +1658,13 @@ func (c *Container) Attach(stdin io.ReadCloser, stdout io.Writer, stderr io.Writ
 
 func (c *Container) AttachWithLogs(stdin io.ReadCloser, stdout, stderr io.Writer, logs, stream bool) error {
 	if logs {
-		cLog, err := c.ReadLog("json")
-		if err != nil && os.IsNotExist(err) {
-			// Legacy logs
-			logrus.Debugf("Old logs format")
-			if stdout != nil {
-				cLog, err := c.ReadLog("stdout")
-				if err != nil {
-					logrus.Errorf("Error reading logs (stdout): %s", err)
-				} else if _, err := io.Copy(stdout, cLog); err != nil {
-					logrus.Errorf("Error streaming logs (stdout): %s", err)
-				}
-			}
-			if stderr != nil {
-				cLog, err := c.ReadLog("stderr")
-				if err != nil {
-					logrus.Errorf("Error reading logs (stderr): %s", err)
-				} else if _, err := io.Copy(stderr, cLog); err != nil {
-					logrus.Errorf("Error streaming logs (stderr): %s", err)
-				}
-			}
-		} else if err != nil {
-			logrus.Errorf("Error reading logs (json): %s", err)
+		logDriver, err := c.getLogger()
+		cLog, err := logDriver.GetReader()
+
+		if err != nil {
+			logrus.Errorf("Error reading logs: %s", err)
+		} else if c.LogDriverType() != jsonfilelog.Name {
+			logrus.Errorf("Reading logs not implemented for driver %s", c.LogDriverType())
 		} else {
 			dec := json.NewDecoder(cLog)
 			for {

daemon/daemon.go

@@ -24,6 +24,7 @@ import (
 	"github.com/docker/docker/daemon/execdriver/execdrivers"
 	"github.com/docker/docker/daemon/graphdriver"
 	_ "github.com/docker/docker/daemon/graphdriver/vfs"
+	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/daemon/network"
 	"github.com/docker/docker/daemon/networkdriver/bridge"
 	"github.com/docker/docker/graph"
@@ -798,6 +799,14 @@ func NewDaemon(config *Config, registryService *registry.Service) (daemon *Daemo
 		}
 	}()
 
+	// Verify logging driver type
+	if config.LogConfig.Type != "none" {
+		if _, err := logger.GetLogDriver(config.LogConfig.Type); err != nil {
+			return nil, fmt.Errorf("error finding the logging driver: %v", err)
+		}
+	}
+	logrus.Debugf("Using default logging driver %s", config.LogConfig.Type)
+
 	if config.EnableSelinuxSupport {
 		if selinuxEnabled() {
 			// As Docker on btrfs and SELinux are incompatible at present, error on both being enabled

daemon/logdrivers.go

@@ -0,0 +1,9 @@
+package daemon
+
+// Importing packages here only to make sure their init gets called and
+// therefore they register themselves to the logdriver factory.
+import (
+	_ "github.com/docker/docker/daemon/logger/journald"
+	_ "github.com/docker/docker/daemon/logger/jsonfilelog"
+	_ "github.com/docker/docker/daemon/logger/syslog"
+)

daemon/logger/copier_test.go

@@ -3,6 +3,7 @@ package logger
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"io"
 	"testing"
 	"time"
@@ -12,16 +13,14 @@ type TestLoggerJSON struct {
 	*json.Encoder
 }
 
-func (l *TestLoggerJSON) Log(m *Message) error {
-	return l.Encode(m)
-}
+func (l *TestLoggerJSON) Log(m *Message) error { return l.Encode(m) }
 
-func (l *TestLoggerJSON) Close() error {
-	return nil
-}
+func (l *TestLoggerJSON) Close() error { return nil }
 
-func (l *TestLoggerJSON) Name() string {
-	return "json"
+func (l *TestLoggerJSON) Name() string { return "json" }
+
+func (l *TestLoggerJSON) GetReader() (io.Reader, error) {
+	return nil, errors.New("not used in the test")
 }
 
 type TestLoggerText struct {
@@ -33,12 +32,12 @@ func (l *TestLoggerText) Log(m *Message) error {
 	return err
 }
 
-func (l *TestLoggerText) Close() error {
-	return nil
-}
+func (l *TestLoggerText) Close() error { return nil }
 
-func (l *TestLoggerText) Name() string {
-	return "text"
+func (l *TestLoggerText) Name() string { return "text" }
+
+func (l *TestLoggerText) GetReader() (io.Reader, error) {
+	return nil, errors.New("not used in the test")
 }
 
 func TestCopier(t *testing.T) {

daemon/logger/factory.go

@@ -0,0 +1,56 @@
+package logger
+
+import (
+	"fmt"
+	"sync"
+)
+
+// Creator is a method that builds a logging driver instance with given context
+type Creator func(Context) (Logger, error)
+
+// Context provides enough information for a logging driver to do its function
+type Context struct {
+	ContainerID   string
+	ContainerName string
+	LogPath       string
+}
+
+type logdriverFactory struct {
+	registry map[string]Creator
+	m        sync.Mutex
+}
+
+func (lf *logdriverFactory) register(name string, c Creator) error {
+	lf.m.Lock()
+	defer lf.m.Unlock()
+
+	if _, ok := lf.registry[name]; ok {
+		return fmt.Errorf("logger: log driver named '%s' is already registered", name)
+	}
+	lf.registry[name] = c
+	return nil
+}
+
+func (lf *logdriverFactory) get(name string) (Creator, error) {
+	lf.m.Lock()
+	defer lf.m.Unlock()
+
+	c, ok := lf.registry[name]
+	if !ok {
+		return c, fmt.Errorf("logger: no log driver named '%s' is registered", name)
+	}
+	return c, nil
+}
+
+var factory = &logdriverFactory{registry: make(map[string]Creator)} // global factory instance
+
+// RegisterLogDriver registers the given logging driver builder with given logging
+// driver name.
+func RegisterLogDriver(name string, c Creator) error {
+	return factory.register(name, c)
+}
+
+// GetLogDriver provides the logging driver builder for a logging driver name.
+func GetLogDriver(name string) (Creator, error) {
+	return factory.get(name)
+}

daemon/logger/journald/journald.go

@@ -2,27 +2,38 @@ package journald
 
 import (
 	"fmt"
+	"io"
 
+	"github.com/Sirupsen/logrus"
 	"github.com/coreos/go-systemd/journal"
 	"github.com/docker/docker/daemon/logger"
 )
 
+const name = "journald"
+
 type Journald struct {
 	Jmap map[string]string
 }
 
-func New(id string, name string) (logger.Logger, error) {
+func init() {
+	if err := logger.RegisterLogDriver(name, New); err != nil {
+		logrus.Fatal(err)
+	}
+}
+
+func New(ctx logger.Context) (logger.Logger, error) {
 	if !journal.Enabled() {
 		return nil, fmt.Errorf("journald is not enabled on this host")
 	}
 	// Strip a leading slash so that people can search for
 	// CONTAINER_NAME=foo rather than CONTAINER_NAME=/foo.
+	name := ctx.ContainerName
 	if name[0] == '/' {
 		name = name[1:]
 	}
 	jmap := map[string]string{
-		"CONTAINER_ID":      id[:12],
-		"CONTAINER_ID_FULL": id,
+		"CONTAINER_ID":      ctx.ContainerID[:12],
+		"CONTAINER_ID_FULL": ctx.ContainerID,
 		"CONTAINER_NAME":    name}
 	return &Journald{Jmap: jmap}, nil
 }
@@ -39,5 +50,9 @@ func (s *Journald) Close() error {
 }
 
 func (s *Journald) Name() string {
-	return "Journald"
+	return name
+}
+
+func (s *Journald) GetReader() (io.Reader, error) {
+	return nil, logger.ReadLogsNotSupported
 }

daemon/logger/jsonfilelog/jsonfilelog.go

@@ -2,31 +2,46 @@ package jsonfilelog
 
 import (
 	"bytes"
+	"io"
 	"os"
 	"sync"
 
+	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/daemon/logger"
 	"github.com/docker/docker/pkg/jsonlog"
 	"github.com/docker/docker/pkg/timeutils"
 )
 
+const (
+	Name = "json-file"
+)
+
 // JSONFileLogger is Logger implementation for default docker logging:
 // JSON objects to file
 type JSONFileLogger struct {
 	buf *bytes.Buffer
 	f   *os.File   // store for closing
 	mu  sync.Mutex // protects buffer
+
+	ctx logger.Context
+}
+
+func init() {
+	if err := logger.RegisterLogDriver(Name, New); err != nil {
+		logrus.Fatal(err)
+	}
 }
 
 // New creates new JSONFileLogger which writes to filename
-func New(filename string) (logger.Logger, error) {
-	log, err := os.OpenFile(filename, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0600)
+func New(ctx logger.Context) (logger.Logger, error) {
+	log, err := os.OpenFile(ctx.LogPath, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0600)
 	if err != nil {
 		return nil, err
 	}
 	return &JSONFileLogger{
 		f:   log,
 		buf: bytes.NewBuffer(nil),
+		ctx: ctx,
 	}, nil
 }
 
@@ -34,6 +49,7 @@ func New(filename string) (logger.Logger, error) {
 func (l *JSONFileLogger) Log(msg *logger.Message) error {
 	l.mu.Lock()
 	defer l.mu.Unlock()
+
 	timestamp, err := timeutils.FastMarshalJSON(msg.Timestamp)
 	if err != nil {
 		return err
@@ -52,6 +68,14 @@ func (l *JSONFileLogger) Log(msg *logger.Message) error {
 	return nil
 }
 
+func (l *JSONFileLogger) GetReader() (io.Reader, error) {
+	return os.Open(l.ctx.LogPath)
+}
+
+func (l *JSONFileLogger) LogPath() string {
+	return l.ctx.LogPath
+}
+
 // Close closes underlying file
 func (l *JSONFileLogger) Close() error {
 	return l.f.Close()
@@ -59,5 +83,5 @@ func (l *JSONFileLogger) Close() error {
 
 // Name returns name of this logger
 func (l *JSONFileLogger) Name() string {
-	return "JSONFile"
+	return Name
 }

daemon/logger/jsonfilelog/jsonfilelog_test.go

@@ -12,18 +12,22 @@ import (
 )
 
 func TestJSONFileLogger(t *testing.T) {
+	cid := "a7317399f3f857173c6179d44823594f8294678dea9999662e5c625b5a1c7657"
 	tmp, err := ioutil.TempDir("", "docker-logger-")
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer os.RemoveAll(tmp)
 	filename := filepath.Join(tmp, "container.log")
-	l, err := New(filename)
+	l, err := New(logger.Context{
+		ContainerID: cid,
+		LogPath:     filename,
+	})
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer l.Close()
-	cid := "a7317399f3f857173c6179d44823594f8294678dea9999662e5c625b5a1c7657"
+
 	if err := l.Log(&logger.Message{ContainerID: cid, Line: []byte("line1"), Source: "src1"}); err != nil {
 		t.Fatal(err)
 	}
@@ -48,18 +52,22 @@ func TestJSONFileLogger(t *testing.T) {
 }
 
 func BenchmarkJSONFileLogger(b *testing.B) {
+	cid := "a7317399f3f857173c6179d44823594f8294678dea9999662e5c625b5a1c7657"
 	tmp, err := ioutil.TempDir("", "docker-logger-")
 	if err != nil {
 		b.Fatal(err)
 	}
 	defer os.RemoveAll(tmp)
 	filename := filepath.Join(tmp, "container.log")
-	l, err := New(filename)
+	l, err := New(logger.Context{
+		ContainerID: cid,
+		LogPath:     filename,
+	})
 	if err != nil {
 		b.Fatal(err)
 	}
 	defer l.Close()
-	cid := "a7317399f3f857173c6179d44823594f8294678dea9999662e5c625b5a1c7657"
+
 	testLine := "Line that thinks that it is log line from docker\n"
 	msg := &logger.Message{ContainerID: cid, Line: []byte(testLine), Source: "stderr", Timestamp: time.Now().UTC()}
 	jsonlog, err := (&jsonlog.JSONLog{Log: string(msg.Line) + "\n", Stream: msg.Source, Created: msg.Timestamp}).MarshalJSON()

daemon/logger/logger.go

@@ -1,6 +1,12 @@
 package logger
 
-import "time"
+import (
+	"errors"
+	"io"
+	"time"
+)
+
+var ReadLogsNotSupported = errors.New("configured logging reader does not support reading")
 
 // Message is datastructure that represents record from some container
 type Message struct {
@@ -15,4 +21,5 @@ type Logger interface {
 	Log(*Message) error
 	Name() string
 	Close() error
+	GetReader() (io.Reader, error)
 }

daemon/logger/syslog/syslog.go

@@ -2,22 +2,34 @@ package syslog
 
 import (
 	"fmt"
+	"io"
 	"log/syslog"
 	"os"
 	"path"
 
+	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/daemon/logger"
 )
 
+const name = "syslog"
+
 type Syslog struct {
 	writer *syslog.Writer
 }
 
-func New(tag string) (logger.Logger, error) {
+func init() {
+	if err := logger.RegisterLogDriver(name, New); err != nil {
+		logrus.Fatal(err)
+	}
+}
+
+func New(ctx logger.Context) (logger.Logger, error) {
+	tag := ctx.ContainerID[:12]
 	log, err := syslog.New(syslog.LOG_DAEMON, fmt.Sprintf("%s/%s", path.Base(os.Args[0]), tag))
 	if err != nil {
 		return nil, err
 	}
+
 	return &Syslog{
 		writer: log,
 	}, nil
@@ -35,5 +47,9 @@ func (s *Syslog) Close() error {
 }
 
 func (s *Syslog) Name() string {
-	return "Syslog"
+	return name
+}
+
+func (s *Syslog) GetReader() (io.Reader, error) {
+	return nil, logger.ReadLogsNotSupported
 }

daemon/logs.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/Sirupsen/logrus"
+	"github.com/docker/docker/daemon/logger/jsonfilelog"
 	"github.com/docker/docker/pkg/jsonlog"
 	"github.com/docker/docker/pkg/stdcopy"
 	"github.com/docker/docker/pkg/tailfile"
@@ -56,32 +57,15 @@ func (daemon *Daemon) ContainerLogs(name string, config *ContainerLogsConfig) er
 		errStream = outStream
 	}
 
-	if container.LogDriverType() != "json-file" {
+	if container.LogDriverType() != jsonfilelog.Name {
 		return fmt.Errorf("\"logs\" endpoint is supported only for \"json-file\" logging driver")
 	}
-	cLog, err := container.ReadLog("json")
-	if err != nil && os.IsNotExist(err) {
-		// Legacy logs
-		logrus.Debugf("Old logs format")
-		if config.UseStdout {
-			cLog, err := container.ReadLog("stdout")
-			if err != nil {
-				logrus.Errorf("Error reading logs (stdout): %s", err)
-			} else if _, err := io.Copy(outStream, cLog); err != nil {
-				logrus.Errorf("Error streaming logs (stdout): %s", err)
-			}
-		}
-		if config.UseStderr {
-			cLog, err := container.ReadLog("stderr")
-			if err != nil {
-				logrus.Errorf("Error reading logs (stderr): %s", err)
-			} else if _, err := io.Copy(errStream, cLog); err != nil {
-				logrus.Errorf("Error streaming logs (stderr): %s", err)
-			}
-		}
-	} else if err != nil {
-		logrus.Errorf("Error reading logs (json): %s", err)
+	logDriver, err := container.getLogger()
+	cLog, err := logDriver.GetReader()
+	if err != nil {
+		logrus.Errorf("Error reading logs: %s", err)
 	} else {
+		// json-file driver
 		if config.Tail != "all" {
 			var err error
 			lines, err = strconv.Atoi(config.Tail)