Client driver health checks for Docker #3856

chelseakomlo · 2018-02-08T20:27:27Z

No description provided.

dadgar · 2018-02-15T18:11:17Z

client/client.go

+	return c.config.Node
+}
+
+func (c *Client) updateNodeFromHealthCheck(response *cstructs.HealthCheckResponse) *structs.Node {


What synchronizes this with the servers

I'm planning to add this in a separate PR that will address synchronization and scheduling.

dadgar · 2018-02-15T18:12:14Z

nomad/structs/structs.go

@@ -1139,6 +1148,9 @@ type Node struct {
 	// Raft Indexes
 	CreateIndex uint64
 	ModifyIndex uint64
+


The Raft indexes are kept at the bottom of the structs so put this above them. I would put it above attributes.

dadgar · 2018-02-15T18:13:00Z

nomad/structs/structs.go

@@ -1062,6 +1062,15 @@ func ValidNodeStatus(status string) bool {
 	}
 }

+// DriverInfo is the current state of a single driver. This is updated
+// regularly as driver health changes on the node.
+type DriverInfo struct {


Will have to make a duplicate of this in the api/node.go and update the node

dadgar · 2018-02-15T18:14:21Z

client/fingerprint/fingerprint.go

+	// it should be run periodically. The return value is a boolean indicating
+	// whether it should be done periodically, and the time interval at which
+	// this check should happen.
+	CheckHealthPeriodic() (bool, time.Duration)


This should be request response as well.

I am not a fan of either name.

Check -> HealthCheck
CheckHealthPeriodic -> GetHealthCheckInterval

dadgar · 2018-02-15T18:16:13Z

client/structs/structs.go

+type HealthCheckRequest struct{}
+
+type HealthCheckResponse struct {
+


Remove blank line

dadgar · 2018-02-15T18:22:45Z

client/fingerprint_manager.go

+
+	fm.nodeLock.Lock()
+	if node := fm.updateHealthCheck(&response); node != nil {
+		fm.node = node


The lock only needs to be held in this if

dadgar · 2018-02-15T18:27:11Z

client/driver/docker.go

+		HealthDescription: "Docker driver is available but unresponsive",
+		UpdateTime:        time.Now(),
+	}
+


Use d.dockerClients()

dadgar · 2018-02-15T18:28:30Z

client/driver/docker.go

+		return err
+	}
+
+	d.logger.Printf("[DEBUG] driver.docker: docker driver is available and is responsive to `docker ps`")


dadgar · 2018-02-15T18:30:48Z

client/fingerprint_manager.go

+	if err := hc.Check(request, &response); err != nil {
+		return err
+	}
+


If the value of the health check hasn't changed avoid calling updateHealthCheck. Otherwise every minute we will be causing a the client to push changes to the server and cause unnecessary Raft transactions.

I was thinking about handling that in updateHealthCheck itself, as #3873 moves this logic to updateNodeFromFingeprint

dadgar · 2018-02-15T18:32:57Z

client/client.go

+	for name, val := range response.Drivers {
+		c.config.Node.Drivers[name] = val
+	}
+


The driver health seems like a nice metric to emit. Healthy and unhealthy with driver as a label.

Good idea. This seemed like the best option but let me know if you had something else in mind. https://github.com/armon/go-metrics/blob/2e4f2be0fe4f6b7096471aa85f2c342bff3b8f4f/metrics.go#L105

dadgar · 2018-03-05T21:20:36Z

nomad/structs/node.go

+		return false
+	}
+
+	if strings.Compare(di.HealthDescription, other.HealthDescription) != 0 {


Just use ==

dadgar · 2018-03-05T21:21:30Z

scheduler/feasible.go

-					option.ID, driverStr, value)
-			return false
-		}
+		// TODO this is a compatibility mechanism- as of Nomad 0.8, nodes have a


Use this syntax for compatibility code:

// COMPAT: Remove in 0.X: As of Nomad 0.8....

Lets us search for compatibility code

dadgar · 2018-03-05T21:23:40Z

client/client.go

+		oldVal := c.config.Node.Drivers[name]
+		if oldVal == nil && newVal != nil {
+			c.config.Node.Drivers[name] = newVal
+		} else if newVal != nil && newVal.Detected != oldVal.Detected {


What happens if the attributes change? This will skip merging

dadgar · 2018-03-05T21:25:26Z

client/driver/docker.go

@@ -552,6 +551,48 @@ func (d *DockerDriver) Fingerprint(req *cstructs.FingerprintRequest, resp *cstru
 	return nil
 }

+// HealthChck implements the interface for the HealthCheck interface. This


dadgar · 2018-03-05T21:25:52Z

client/driver/docker.go

+
+	client, _, err := d.dockerClients()
+	if err != nil {
+		d.logger.Printf("[WARN] driver.docker: docker driver is available but is unresponsive to `docker ps`")


Always log the error

Also if you see this in logs how will you know which line caused the log? Add distinguishing information in the log lines so they are useful for diagnosing. An example:

d.logger.Printf("[WARN] driver.docker: failed to retrieve Docker client docker health check: %v", err)

dadgar · 2018-03-05T21:32:03Z

client/fingerprint_manager.go

+		case <-time.After(period):
+			err := fm.healthCheck(name, hc)
+			if err != nil {
+				fm.logger.Printf("[DEBUG] client.fingerprint_manager: health checking for %v failed: %+v", name, err)


Use %v for errors

dadgar · 2018-03-05T21:33:57Z

client/fingerprint_manager.go

-	logger     *log.Logger
+	updateNodeAttributes func(*cstructs.FingerprintResponse) *structs.Node
+
+	// UpdateHealthCheck is a callback to the client to update the state of the


dadgar · 2018-03-05T22:02:53Z

client/fingerprint_manager.go

+// away from annotating a node's attributes to demonstrate support for a
+// particular driver. Takes the FingerprintResponse and converts it to the
+// proper DriverInfo update and then sets the prefix attributes as well
+func (fm *FingerprintManager) fingerprintDriver(name string, f fingerprint.Fingerprint) (bool, error) {


This is only being called the first time. Periodic, health checking drivers should always call this. Where "this" is the merging logic into the driverinfo

dadgar · 2018-03-05T22:06:52Z

client/fingerprint_manager.go

-			go fm.run(d, period, name)
+			go fm.runFingerprint(d, period, name)
+		}
+


I think what this should do is not use runFingerprint or runHealthCheck but instead gather the fact that the Driver is health checking or periodic and pass all that to a new method that runs the drivers fingerprint/healthcheck periodically and then take the returned info and properly construct the DriverInfo object (merged health check and attributes)

This is a bit tricky as the fingerprint/healthcheck methods run on different intervals, so if we merge them in lockstep then we only update the node as often has the slowest interval. Thinking through ways to do this more cleanly.

dadgar · 2018-03-05T22:09:15Z

client/fingerprint_manager.go

+	// in a backwards compatible way, where node attributes for drivers will
+	// eventually be phased out.
+	di := &structs.DriverInfo{
+		Attributes: response.Attributes,


Stripping:

DriverInfo.Attributes: map[string]string{ "version": "1.12.3", } Node.Attributes: { "driver.docker.version": "1.12.3" }

preetapan · 2018-03-06T17:31:42Z

client/client.go

+	// update the node with the latest driver health information
+	for name, newVal := range response.Drivers {
+		if newVal == nil {
+			continue


What about when newVal is nil but oldVal is not here?

This is just a safety precaution, every driver should have a corresponding driver info value for a node.

preetapan · 2018-03-07T16:46:24Z

scheduler/feasible_test.go

+		}
+		checker := NewDriverChecker(ctx, drivers)
+		if act := checker.Feasible(c.Node); act != c.Result {
+			t.Fatalf("case(%d) failed: got %v; want %v", i, act, c.Result)


rmaybe use require rather than t.Fatalf here?

preetapan · 2018-03-07T16:50:17Z

scheduler/feasible.go

+				return false
+			}
+
+			if !enabled {


simplify by changing this to return enabled

preetapan · 2018-03-07T16:53:34Z

client/structs/structs.go

+// If the Drivers field has not yet been initialized, it does so here.
+func (h *HealthCheckResponse) AddDriverInfo(name string, driverInfo *structs.DriverInfo) {
+	// initialize Drivers if it has not been already
+	if h.Drivers == nil {


Why not make a constructor for HealthCheckResponse and create an empty map there? Doing a lazy init is a bit more efficient because it avoids creating the map till you actually need it, but it trades off some readability and I am not sure the small efficiency gains are worth it.

preetapan

MAde some comments

dadgar · 2018-03-15T23:34:56Z

client/client.go

+
+	if fingerprint != nil {
+		if c.config.Node.Drivers[name] == nil {
+			// if the driver info has not yet been set, do that here


Capitalize your comments. This is Go style: https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

dadgar · 2018-03-15T23:36:33Z

client/client.go

+				events := []*structs.NodeEvent{
+					{
+						Subsystem: "Driver",
+						Message:   fmt.Sprintf("Driver status changed: %s", health.HealthDescription),


"Driver status changed:" can probably be removed.

dadgar · 2018-03-15T23:38:45Z

client/driver/docker.go

+
+	_, err = client.ListContainers(docker.ListContainersOptions{All: false})
+	if err != nil {
+		d.logger.Printf("[WARN] driver.docker: failed to list Docker containers in the process of a docker health check: %v", err)


Capitalize Docker

dadgar · 2018-03-15T23:42:22Z

scheduler/feasible.go

-					option.ID, driverStr, value)
-			return false
-		}
+		// COMPAT: Remove in 0.X: As of Nomad 0.8, nodes have a DriverInfo that


0.X -> 0.10

dadgar · 2018-03-15T23:43:06Z

client/fingerprint_manager.go

-	logger     *log.Logger
+	updateNodeAttributes func(*cstructs.FingerprintResponse) *structs.Node
+
+	updateNodeFromDriver func(string, *structs.DriverInfo, *structs.DriverInfo) *structs.Node


dadgar · 2018-03-16T00:16:49Z

client/fingerprint_manager.go

+		case <-t2.C:
+			if isHealthCheck {
+				t2.Reset(healthCheckPeriod)
+				err := fm.runHealthCheck(name, hc)


if err := fm.runHealthCheck(name, hc); err != nil {

dadgar · 2018-03-16T00:18:25Z

client/fingerprint_manager.go

@@ -118,6 +234,21 @@ func (fm *FingerprintManager) fingerprint(name string, f fingerprint.Fingerprint
 	return response.Detected, nil
 }

+// healthcheck checks the health of the specified resource.
+func (fm *FingerprintManager) runHealthCheck(name string, hc fingerprint.HealthCheck) error {


rename to runDriverHealthCheck and fix comment

dadgar · 2018-03-16T00:20:25Z

client/fingerprint_manager.go

@@ -2,6 +2,7 @@ package client

 import (
 	"log"
+	"strings"


Can you try to arrange the methods such that drivers methods are colocated and fingerprint methods are too. I think it would be simpler to follow if it went:

Struct

NewFingerprintManager

Run.

SetupFingerprinter

SetupDriver
6-X: Fingerprinter stuff
7-X: Driver stuff

dadgar · 2018-03-16T00:28:49Z

client/fingerprint_manager.go

+				fm.logger.Printf("[DEBUG] client.fingerprint_manager: periodic fingerprinting for driver %v failed: %+v", name, err)
+			}
+		case <-t2.C:
+			if isHealthCheck {


I feel like we shouldn't run the health check if detected isn't true. Otherwise you will log errors continuously, right?

I think you also skip the initial health check here and just rely on this loop: https://github.com/hashicorp/nomad/pull/3856/files#diff-8cf4b6aaa496508270af1cd14fdc1527R92

Agree with this, but the second point would entail that all drivers that allow health checking will also be periodic. If that is always true, then we can do as you said and skip the initial check and only update on a periodic interval.

dadgar · 2018-03-16T00:29:39Z

client/driver/docker.go

+
+	client, _, err := d.dockerClients()
+	if err != nil {
+		d.logger.Printf("[WARN] driver.docker: failed to retrieve Docker client in the process of a docker health check: %v", err)


This is part of the reason I feel like we skip the health check unless it is detected, otherwise you will mark it as unhealthy and log even if docker just isn't there.

fix up feedback from code review add driver info for all drivers to node

… check Slight updates for go style

…r checks

bump log level

github-actions · 2023-03-09T02:18:48Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

chelseakomlo changed the title ~~Client add driverhealth checks~~ Client add driver health checks Feb 8, 2018

chelseakomlo changed the title ~~Client add driver health checks~~ [WIP] Client add driver health checks Feb 9, 2018

chelseakomlo force-pushed the f-client-add-health-checks branch 2 times, most recently from ee0f21f to 9e06dd9 Compare February 13, 2018 18:02

chelseakomlo changed the title ~~[WIP] Client add driver health checks~~ Client driver health checks for Docker Feb 13, 2018

dadgar requested changes Feb 15, 2018

View reviewed changes

chelseakomlo force-pushed the f-client-add-health-checks branch 4 times, most recently from 2831093 to 29c49cf Compare February 23, 2018 19:15

chelseakomlo mentioned this pull request Feb 26, 2018

[Blocked] Use driver health check status to determine scheduling #3905

Closed

chelseakomlo force-pushed the f-client-add-health-checks branch 2 times, most recently from b8695fd to 4f1ba6b Compare March 1, 2018 21:05

dadgar requested changes Mar 5, 2018

View reviewed changes

preetapan reviewed Mar 6, 2018

View reviewed changes

preetapan reviewed Mar 7, 2018

View reviewed changes

preetapan approved these changes Mar 7, 2018

View reviewed changes

chelseakomlo force-pushed the f-client-add-health-checks branch 5 times, most recently from bd262da to 244389c Compare March 15, 2018 22:20

dadgar requested changes Mar 16, 2018

View reviewed changes

chelseakomlo force-pushed the f-client-add-health-checks branch 4 times, most recently from 7ee98af to 2adea66 Compare March 21, 2018 17:14

chelseakomlo force-pushed the f-client-add-health-checks branch from 2adea66 to e17343d Compare March 21, 2018 17:32

chelseakomlo and others added 24 commits March 21, 2018 15:15

add concept of health checks to fingerprinters and nodes

1570972

fix up feedback from code review add driver info for all drivers to node

allow nomad to schedule based on the status of a client driver health…

a340bad

… check Slight updates for go style

fix scheduler driver name; create node structs file

9ec5a93

fix up gofmt

7c34605

go style; update comments

9dfb5c6

fix up scheduler mocks

70bebd1

updating comments; locking concurrent node access

fd25db9

fix up racy tests

865b7e0

fix up codereview feedback

240fee4

refresh driver information for non-health checking drivers periodically

521fbd4

improve tests

8a0ed4e

notes from walk through

3ad03d9

Code review feedback

8aefd29

fix up health check logic comparison; add node events to client drive…

115d54c

…r checks

simplify logic

8597da0

bump log level

remove unused function

7b2ed01

improve comments; update watchDriver

06a306e

function rename and re-arrange functions in fingerprint_manager

cba0a4d

fix issue when updating node events

9c14330

Only run health check if driver is detected

ffe9292

Docker driver doesn't return errors but injects into the DriverInfo

b59bea9

set driver to unhealthy once if it cannot be detected in periodic check

127b2c6

always set initial health status for every driver

eb3a53e

fix up scheduling test

89ffc96

chelseakomlo force-pushed the f-client-add-health-checks branch from e17343d to 89ffc96 Compare March 21, 2018 19:57

chelseakomlo merged commit 48c5f0e into master Mar 21, 2018

chelseakomlo deleted the f-client-add-health-checks branch March 21, 2018 22:05

github-actions bot locked as resolved and limited conversation to collaborators Mar 9, 2023

		type HealthCheckRequest struct{}

		type HealthCheckResponse struct {

Client driver health checks for Docker #3856

Client driver health checks for Docker #3856

Conversation

chelseakomlo commented Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetapan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 9, 2023

chelseakomlo commented Feb 8, 2018 •

edited

Loading