From 35ab26658c4f9408d7e32e14d1cf5122dcec73a0 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Fri, 11 Jan 2019 11:50:46 -0600 Subject: [PATCH 1/3] Make docker driver logging less redundant --- drivers/docker/driver.go | 4 ++++ drivers/docker/fingerprint.go | 30 +++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index d20de68ad..068032481 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -88,6 +88,10 @@ type Driver struct { // gpuRuntime indicates nvidia-docker runtime availability gpuRuntime bool + + // hasFingerprinted is used to store whether we have fingerprinted before + hasFingerprinted bool + fingerprintLock sync.Mutex } // NewDockerDriver returns a docker implementation of a driver plugin diff --git a/drivers/docker/fingerprint.go b/drivers/docker/fingerprint.go index acdad87fc..6bb14c518 100644 --- a/drivers/docker/fingerprint.go +++ b/drivers/docker/fingerprint.go @@ -16,6 +16,20 @@ func (d *Driver) Fingerprint(ctx context.Context) (<-chan *drivers.Fingerprint, return ch, nil } +// fingerprinted returns whether the driver has fingerprinted before +func (d *Driver) fingerprinted() bool { + d.fingerprintLock.Lock() + defer d.fingerprintLock.Unlock() + return d.hasFingerprinted +} + +// setFingerprinted marks the driver as having fingerprinted once before +func (d *Driver) setFingerprinted() { + d.fingerprintLock.Lock() + d.hasFingerprinted = true + d.fingerprintLock.Unlock() +} + func (d *Driver) handleFingerprint(ctx context.Context, ch chan *drivers.Fingerprint) { defer close(ch) ticker := time.NewTimer(0) @@ -33,6 +47,10 @@ func (d *Driver) handleFingerprint(ctx context.Context, ch chan *drivers.Fingerp } func (d *Driver) buildFingerprint() *drivers.Fingerprint { + defer func() { + d.setFingerprinted() + }() + fp := &drivers.Fingerprint{ Attributes: map[string]*pstructs.Attribute{}, Health: drivers.HealthStateHealthy, @@ -40,7 +58,9 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { } client, _, err := d.dockerClients() if err != nil { - d.logger.Info("failed to initialize client", "error", err) + if !d.fingerprinted() { + d.logger.Info("failed to initialize client", "error", err) + } return &drivers.Fingerprint{ Health: drivers.HealthStateUndetected, HealthDescription: "Failed to initialize docker client", @@ -49,7 +69,9 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { env, err := client.Version() if err != nil { - d.logger.Debug("could not connect to docker daemon", "endpoint", client.Endpoint(), "error", err) + if !d.fingerprinted() { + d.logger.Debug("could not connect to docker daemon", "endpoint", client.Endpoint(), "error", err) + } return &drivers.Fingerprint{ Health: drivers.HealthStateUnhealthy, HealthDescription: "Failed to connect to docker daemon", @@ -84,7 +106,9 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { } else { // Docker 17.09.0-ce dropped the Gateway IP from the bridge network // See https://github.com/moby/moby/issues/32648 - d.logger.Debug("bridge_ip could not be discovered") + if !d.fingerprinted() { + d.logger.Debug("bridge_ip could not be discovered") + } } break } From 80c00fc26886bdf32595b07037578baea5b485dc Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Mon, 14 Jan 2019 15:33:42 -0600 Subject: [PATCH 2/3] Refactor logging in drivers to use a tri-state boolean Changes logging warnings/errors only if the state changes from healthy to unhealthy --- drivers/docker/driver.go | 7 ++++--- drivers/docker/fingerprint.go | 29 ++++++++++++++-------------- drivers/exec/driver.go | 30 ++++++++++++++++++++++++++++- drivers/rkt/driver.go | 36 ++++++++++++++++++----------------- 4 files changed, 67 insertions(+), 35 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 068032481..0de93aba7 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -89,9 +89,10 @@ type Driver struct { // gpuRuntime indicates nvidia-docker runtime availability gpuRuntime bool - // hasFingerprinted is used to store whether we have fingerprinted before - hasFingerprinted bool - fingerprintLock sync.Mutex + // A tri-state boolean to know if the fingerprinting has happened and + // whether it has been successful + fingerprintSuccess *bool + fingerprintLock sync.Mutex } // NewDockerDriver returns a docker implementation of a driver plugin diff --git a/drivers/docker/fingerprint.go b/drivers/docker/fingerprint.go index 6bb14c518..50d64374c 100644 --- a/drivers/docker/fingerprint.go +++ b/drivers/docker/fingerprint.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/plugins/drivers" pstructs "github.com/hashicorp/nomad/plugins/shared/structs" ) @@ -16,17 +17,17 @@ func (d *Driver) Fingerprint(ctx context.Context) (<-chan *drivers.Fingerprint, return ch, nil } -// fingerprinted returns whether the driver has fingerprinted before -func (d *Driver) fingerprinted() bool { +// setFingerprintSuccess marks the driver as having fingerprinted successfully +func (d *Driver) setFingerprintSuccess() { d.fingerprintLock.Lock() - defer d.fingerprintLock.Unlock() - return d.hasFingerprinted + d.fingerprintSuccess = helper.BoolToPtr(true) + d.fingerprintLock.Unlock() } -// setFingerprinted marks the driver as having fingerprinted once before -func (d *Driver) setFingerprinted() { +// setFingerprintFailure marks the driver as having failed fingerprinting +func (d *Driver) setFingerprintFailure() { d.fingerprintLock.Lock() - d.hasFingerprinted = true + d.fingerprintSuccess = helper.BoolToPtr(false) d.fingerprintLock.Unlock() } @@ -47,10 +48,6 @@ func (d *Driver) handleFingerprint(ctx context.Context, ch chan *drivers.Fingerp } func (d *Driver) buildFingerprint() *drivers.Fingerprint { - defer func() { - d.setFingerprinted() - }() - fp := &drivers.Fingerprint{ Attributes: map[string]*pstructs.Attribute{}, Health: drivers.HealthStateHealthy, @@ -58,9 +55,10 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { } client, _, err := d.dockerClients() if err != nil { - if !d.fingerprinted() { + if d.fingerprintSuccess == nil || *d.fingerprintSuccess { d.logger.Info("failed to initialize client", "error", err) } + d.setFingerprintFailure() return &drivers.Fingerprint{ Health: drivers.HealthStateUndetected, HealthDescription: "Failed to initialize docker client", @@ -69,9 +67,10 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { env, err := client.Version() if err != nil { - if !d.fingerprinted() { + if d.fingerprintSuccess == nil || *d.fingerprintSuccess { d.logger.Debug("could not connect to docker daemon", "endpoint", client.Endpoint(), "error", err) } + d.setFingerprintFailure() return &drivers.Fingerprint{ Health: drivers.HealthStateUnhealthy, HealthDescription: "Failed to connect to docker daemon", @@ -106,7 +105,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { } else { // Docker 17.09.0-ce dropped the Gateway IP from the bridge network // See https://github.com/moby/moby/issues/32648 - if !d.fingerprinted() { + if d.fingerprintSuccess == nil || *d.fingerprintSuccess { d.logger.Debug("bridge_ip could not be discovered") } } @@ -132,5 +131,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { strings.Join(runtimeNames, ",")) } + d.setFingerprintSuccess() + return fp } diff --git a/drivers/exec/driver.go b/drivers/exec/driver.go index 3a085345b..8aab879ed 100644 --- a/drivers/exec/driver.go +++ b/drivers/exec/driver.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/nomad/client/fingerprint" "github.com/hashicorp/nomad/drivers/shared/eventer" "github.com/hashicorp/nomad/drivers/shared/executor" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/plugins/base" "github.com/hashicorp/nomad/plugins/drivers" "github.com/hashicorp/nomad/plugins/drivers/utils" @@ -20,6 +21,7 @@ import ( "github.com/hashicorp/nomad/plugins/shared/hclspec" "github.com/hashicorp/nomad/plugins/shared/loader" pstructs "github.com/hashicorp/nomad/plugins/shared/structs" + "sync" ) const ( @@ -95,6 +97,11 @@ type Driver struct { // logger will log to the Nomad agent logger hclog.Logger + + // A tri-state boolean to know if the fingerprinting has happened and + // whether it has been successful + fingerprintSuccess *bool + fingerprintLock sync.Mutex } // TaskConfig is the driver configuration of a task within a job @@ -126,6 +133,20 @@ func NewExecDriver(logger hclog.Logger) drivers.DriverPlugin { } } +// setFingerprintSuccess marks the driver as having fingerprinted successfully +func (d *Driver) setFingerprintSuccess() { + d.fingerprintLock.Lock() + d.fingerprintSuccess = helper.BoolToPtr(true) + d.fingerprintLock.Unlock() +} + +// setFingerprintFailure marks the driver as having failed fingerprinting +func (d *Driver) setFingerprintFailure() { + d.fingerprintLock.Lock() + d.fingerprintSuccess = helper.BoolToPtr(false) + d.fingerprintLock.Unlock() +} + func (d *Driver) PluginInfo() (*base.PluginInfoResponse, error) { return pluginInfo, nil } @@ -177,6 +198,7 @@ func (d *Driver) handleFingerprint(ctx context.Context, ch chan<- *drivers.Finge func (d *Driver) buildFingerprint() *drivers.Fingerprint { if runtime.GOOS != "linux" { + d.setFingerprintFailure() return &drivers.Fingerprint{ Health: drivers.HealthStateUndetected, HealthDescription: "exec driver unsupported on client OS", @@ -192,6 +214,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { if !utils.IsUnixRoot() { fp.Health = drivers.HealthStateUndetected fp.HealthDescription = drivers.DriverRequiresRootMessage + d.setFingerprintFailure() return fp } @@ -199,17 +222,22 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { if err != nil { fp.Health = drivers.HealthStateUnhealthy fp.HealthDescription = drivers.NoCgroupMountMessage - d.logger.Warn(fp.HealthDescription, "error", err) + if d.fingerprintSuccess == nil || *d.fingerprintSuccess { + d.logger.Warn(fp.HealthDescription, "error", err) + } + d.setFingerprintFailure() return fp } if mount == "" { fp.Health = drivers.HealthStateUnhealthy fp.HealthDescription = drivers.CgroupMountEmpty + d.setFingerprintFailure() return fp } fp.Attributes["driver.exec"] = pstructs.NewBoolAttribute(true) + d.setFingerprintSuccess() return fp } diff --git a/drivers/rkt/driver.go b/drivers/rkt/driver.go index 067e78ef6..2035327d0 100644 --- a/drivers/rkt/driver.go +++ b/drivers/rkt/driver.go @@ -28,6 +28,7 @@ import ( "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/drivers/shared/eventer" "github.com/hashicorp/nomad/drivers/shared/executor" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/plugins/base" "github.com/hashicorp/nomad/plugins/drivers" "github.com/hashicorp/nomad/plugins/shared" @@ -193,9 +194,10 @@ type Driver struct { // logger will log to the Nomad agent logger hclog.Logger - // hasFingerprinted is used to store whether we have fingerprinted before - hasFingerprinted bool - fingerprintLock sync.Mutex + // A tri-state boolean to know if the fingerprinting has happened and + // whether it has been successful + fingerprintSuccess *bool + fingerprintLock sync.Mutex } func NewRktDriver(logger hclog.Logger) drivers.DriverPlugin { @@ -264,25 +266,21 @@ func (d *Driver) handleFingerprint(ctx context.Context, ch chan *drivers.Fingerp } } -// setFingerprinted marks the driver as having fingerprinted once before -func (d *Driver) setFingerprinted() { +// setFingerprintSuccess marks the driver as having fingerprinted successfully +func (d *Driver) setFingerprintSuccess() { d.fingerprintLock.Lock() - d.hasFingerprinted = true + d.fingerprintSuccess = helper.BoolToPtr(true) d.fingerprintLock.Unlock() } -// fingerprinted returns whether the driver has fingerprinted before -func (d *Driver) fingerprinted() bool { +// setFingerprintFailure marks the driver as having failed fingerprinting +func (d *Driver) setFingerprintFailure() { d.fingerprintLock.Lock() - defer d.fingerprintLock.Unlock() - return d.hasFingerprinted + d.fingerprintSuccess = helper.BoolToPtr(false) + d.fingerprintLock.Unlock() } func (d *Driver) buildFingerprint() *drivers.Fingerprint { - defer func() { - d.setFingerprinted() - }() - fingerprint := &drivers.Fingerprint{ Attributes: map[string]*pstructs.Attribute{}, Health: drivers.HealthStateHealthy, @@ -291,9 +289,10 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { // Only enable if we are root if syscall.Geteuid() != 0 { - if !d.fingerprinted() { + if d.fingerprintSuccess == nil || *d.fingerprintSuccess { d.logger.Debug("must run as root user, disabling") } + d.setFingerprintFailure() fingerprint.Health = drivers.HealthStateUndetected fingerprint.HealthDescription = drivers.DriverRequiresRootMessage return fingerprint @@ -303,6 +302,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { if err != nil { fingerprint.Health = drivers.HealthStateUndetected fingerprint.HealthDescription = fmt.Sprintf("Failed to execute %s version: %v", rktCmd, err) + d.setFingerprintFailure() return fingerprint } out := strings.TrimSpace(string(outBytes)) @@ -312,6 +312,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { if len(rktMatches) != 2 || len(appcMatches) != 2 { fingerprint.Health = drivers.HealthStateUndetected fingerprint.HealthDescription = "Unable to parse rkt version string" + d.setFingerprintFailure() return fingerprint } @@ -321,10 +322,11 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { // Do not allow ancient rkt versions fingerprint.Health = drivers.HealthStateUndetected fingerprint.HealthDescription = fmt.Sprintf("Unsuported rkt version %s", currentVersion) - if !d.fingerprinted() { + if d.fingerprintSuccess == nil || *d.fingerprintSuccess { d.logger.Warn("unsupported rkt version please upgrade to >= "+minVersion.String(), "rkt_version", currentVersion) } + d.setFingerprintFailure() return fingerprint } @@ -334,7 +336,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { if d.config.VolumesEnabled { fingerprint.Attributes["driver.rkt.volumes.enabled"] = pstructs.NewBoolAttribute(true) } - + d.setFingerprintSuccess() return fingerprint } From 3e16b523619a75ee8b8a90eeb3f2a57ed4fe9c28 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Wed, 16 Jan 2019 11:04:11 -0600 Subject: [PATCH 3/3] clean up read access --- drivers/docker/driver.go | 2 +- drivers/docker/fingerprint.go | 14 +++++++++++--- drivers/exec/driver.go | 10 +++++++++- drivers/rkt/driver.go | 12 ++++++++++-- 4 files changed, 31 insertions(+), 7 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 0de93aba7..3467338c5 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -92,7 +92,7 @@ type Driver struct { // A tri-state boolean to know if the fingerprinting has happened and // whether it has been successful fingerprintSuccess *bool - fingerprintLock sync.Mutex + fingerprintLock sync.RWMutex } // NewDockerDriver returns a docker implementation of a driver plugin diff --git a/drivers/docker/fingerprint.go b/drivers/docker/fingerprint.go index 50d64374c..a84bafd90 100644 --- a/drivers/docker/fingerprint.go +++ b/drivers/docker/fingerprint.go @@ -31,6 +31,14 @@ func (d *Driver) setFingerprintFailure() { d.fingerprintLock.Unlock() } +// fingerprintSuccessful returns true if the driver has +// never fingerprinted or has successfully fingerprinted +func (d *Driver) fingerprintSuccessful() bool { + d.fingerprintLock.Lock() + defer d.fingerprintLock.Unlock() + return d.fingerprintSuccess == nil || *d.fingerprintSuccess +} + func (d *Driver) handleFingerprint(ctx context.Context, ch chan *drivers.Fingerprint) { defer close(ch) ticker := time.NewTimer(0) @@ -55,7 +63,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { } client, _, err := d.dockerClients() if err != nil { - if d.fingerprintSuccess == nil || *d.fingerprintSuccess { + if d.fingerprintSuccessful() { d.logger.Info("failed to initialize client", "error", err) } d.setFingerprintFailure() @@ -67,7 +75,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { env, err := client.Version() if err != nil { - if d.fingerprintSuccess == nil || *d.fingerprintSuccess { + if d.fingerprintSuccessful() { d.logger.Debug("could not connect to docker daemon", "endpoint", client.Endpoint(), "error", err) } d.setFingerprintFailure() @@ -105,7 +113,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { } else { // Docker 17.09.0-ce dropped the Gateway IP from the bridge network // See https://github.com/moby/moby/issues/32648 - if d.fingerprintSuccess == nil || *d.fingerprintSuccess { + if d.fingerprintSuccessful() { d.logger.Debug("bridge_ip could not be discovered") } } diff --git a/drivers/exec/driver.go b/drivers/exec/driver.go index 8aab879ed..d8250d873 100644 --- a/drivers/exec/driver.go +++ b/drivers/exec/driver.go @@ -147,6 +147,14 @@ func (d *Driver) setFingerprintFailure() { d.fingerprintLock.Unlock() } +// fingerprintSuccessful returns true if the driver has +// never fingerprinted or has successfully fingerprinted +func (d *Driver) fingerprintSuccessful() bool { + d.fingerprintLock.Lock() + defer d.fingerprintLock.Unlock() + return d.fingerprintSuccess == nil || *d.fingerprintSuccess +} + func (d *Driver) PluginInfo() (*base.PluginInfoResponse, error) { return pluginInfo, nil } @@ -222,7 +230,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { if err != nil { fp.Health = drivers.HealthStateUnhealthy fp.HealthDescription = drivers.NoCgroupMountMessage - if d.fingerprintSuccess == nil || *d.fingerprintSuccess { + if d.fingerprintSuccessful() { d.logger.Warn(fp.HealthDescription, "error", err) } d.setFingerprintFailure() diff --git a/drivers/rkt/driver.go b/drivers/rkt/driver.go index 2035327d0..54f0510c1 100644 --- a/drivers/rkt/driver.go +++ b/drivers/rkt/driver.go @@ -280,6 +280,14 @@ func (d *Driver) setFingerprintFailure() { d.fingerprintLock.Unlock() } +// fingerprintSuccessful returns true if the driver has +// never fingerprinted or has successfully fingerprinted +func (d *Driver) fingerprintSuccessful() bool { + d.fingerprintLock.Lock() + defer d.fingerprintLock.Unlock() + return d.fingerprintSuccess == nil || *d.fingerprintSuccess +} + func (d *Driver) buildFingerprint() *drivers.Fingerprint { fingerprint := &drivers.Fingerprint{ Attributes: map[string]*pstructs.Attribute{}, @@ -289,7 +297,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { // Only enable if we are root if syscall.Geteuid() != 0 { - if d.fingerprintSuccess == nil || *d.fingerprintSuccess { + if d.fingerprintSuccessful() { d.logger.Debug("must run as root user, disabling") } d.setFingerprintFailure() @@ -322,7 +330,7 @@ func (d *Driver) buildFingerprint() *drivers.Fingerprint { // Do not allow ancient rkt versions fingerprint.Health = drivers.HealthStateUndetected fingerprint.HealthDescription = fmt.Sprintf("Unsuported rkt version %s", currentVersion) - if d.fingerprintSuccess == nil || *d.fingerprintSuccess { + if d.fingerprintSuccessful() { d.logger.Warn("unsupported rkt version please upgrade to >= "+minVersion.String(), "rkt_version", currentVersion) }