luxolus
/
open-nomad
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

Code review feedback

This commit is contained in:
Chelsea Holland Komlo 2018-03-09 12:28:01 -05:00
parent 34dc58421c
commit 53a5bc2bb3
4 changed files with 156 additions and 147 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

76
client/client.go
View File

@ -259,7 +259,8 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic
}
fingerprintManager := NewFingerprintManager(c.GetConfig, c.config.Node,
c.shutdownCh, c.updateNodeFromFingerprint, c.updateNodeFromHealthCheck, c.logger)
c.shutdownCh, c.updateNodeFromFingerprint, c.updateNodeFromHealthCheck,
c.updateNodeFromDriver, c.logger)
// Fingerprint the node and scan for drivers
if err := fingerprintManager.Run(); err != nil {
@ -1006,23 +1007,66 @@ func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintRespons
c.config.Node.Resources.Merge(response.Resources)
}
// XXX --------------
// All drivers only expose DriverInfo
// This becomes a separate setter. So we have:
// * updateNodeFromFingerprinters
// * updateNodeFromDriver(fingerprint, health *DriverInfo)
/// TODO is anything updating the Node.Attributes based on updating
// driverinfos
if nodeHasChanged {
c.updateNode()
}
for name, newVal := range response.Drivers {
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 {
c.config.Node.Drivers[name].MergeFingerprintInfo(newVal)
return c.config.Node
}
// updateNodeFromDriver receives either a fingerprint of the driver or its
// health and merges this into a single DriverInfo object
func (c *Client) updateNodeFromDriver(name string, fingerprint, health *structs.DriverInfo) *structs.Node {
c.configLock.Lock()
defer c.configLock.Unlock()
var hasChanged bool
if fingerprint != nil {
if c.config.Node.Drivers[name] == nil {
hasChanged = true
c.config.Node.Drivers[name] = fingerprint
} else {
if c.config.Node.Drivers[name].Detected != fingerprint.Detected {
hasChanged = true
c.config.Node.Drivers[name].Detected = fingerprint.Detected
}
for attrName, newVal := range fingerprint.Attributes {
oldVal := c.config.Node.Drivers[name].Attributes[attrName]
if oldVal == newVal {
continue
}
hasChanged = true
if newVal == "" {
delete(c.config.Node.Attributes, attrName)
} else {
c.config.Node.Attributes[attrName] = newVal
}
}
}
}
if nodeHasChanged {
if health != nil {
if c.config.Node.Drivers[name] == nil {
hasChanged = true
c.config.Node.Drivers[name] = health
} else {
oldVal := c.config.Node.Drivers[name]
if !health.HealthCheckEquals(oldVal) {
hasChanged = true
c.config.Node.Drivers[name].MergeHealthCheck(health)
} else {
// make sure we accurately reflect the last time a health check has been
// performed for the driver.
oldVal.UpdateTime = health.UpdateTime
}
}
}
if hasChanged {
c.config.Node.Drivers[name].UpdateTime = time.Now()
c.updateNode()
}
@ -1060,6 +1104,7 @@ func watcher(driver Driver) {
// updateNodeFromHealthCheck receives a health check response and updates the
// node accordingly
// TODO is this even needed?
func (c *Client) updateNodeFromHealthCheck(response *cstructs.HealthCheckResponse) *structs.Node {
c.configLock.Lock()
defer c.configLock.Unlock()
@ -1083,7 +1128,6 @@ func (c *Client) updateNodeFromHealthCheck(response *cstructs.HealthCheckRespons
if oldVal == nil {
c.config.Node.Drivers[name] = newVal
} else {
c.config.Node.Drivers[name].MergeHealthCheck(newVal)
}
}

173
client/fingerprint_manager.go
View File

@ -28,7 +28,9 @@ type FingerprintManager struct {
// updateHealthCheck is a callback to the client to update the state of the
// node for resources that require a health check
updateHealthCheck func(*cstructs.HealthCheckResponse) *structs.Node
logger *log.Logger
updateNodeFromDriver func(string, *structs.DriverInfo, *structs.DriverInfo) *structs.Node
logger *log.Logger
}
// NewFingerprintManager is a constructor that creates and returns an instance
@ -38,11 +40,13 @@ func NewFingerprintManager(getConfig func() *config.Config,
shutdownCh chan struct{},
updateNodeAttributes func(*cstructs.FingerprintResponse) *structs.Node,
updateHealthCheck func(*cstructs.HealthCheckResponse) *structs.Node,
updateNodeFromDriver func(string, *structs.DriverInfo, *structs.DriverInfo) *structs.Node,
logger *log.Logger) *FingerprintManager {
return &FingerprintManager{
getConfig: getConfig,
updateNodeAttributes: updateNodeAttributes,
updateHealthCheck: updateHealthCheck,
updateNodeFromDriver: updateNodeFromDriver,
node: node,
shutdownCh: shutdownCh,
logger: logger,
@ -53,9 +57,14 @@ func NewFingerprintManager(getConfig func() *config.Config,
func (fm *FingerprintManager) runFingerprint(f fingerprint.Fingerprint, period time.Duration, name string) {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting %s every %v", name, period)
timer := time.NewTimer(period)
defer timer.Stop()
for {
select {
case <-time.After(period):
case <-timer.C:
timer.Reset(period)
_, err := fm.fingerprint(name, f)
if err != nil {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: periodic fingerprinting for %v failed: %+v", name, err)
@ -68,26 +77,6 @@ func (fm *FingerprintManager) runFingerprint(f fingerprint.Fingerprint, period t
}
}
// XXX Do we need this for now
// runHealthCheck runs each health check individually on an ongoing basis
func (fm *FingerprintManager) runHealthCheck(hc fingerprint.HealthCheck, period time.Duration, name string) {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: healthchecking %s every %v", name, period)
for {
select {
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)
continue
}
case <-fm.shutdownCh:
return
}
}
}
// setupDrivers is used to fingerprint the node to see if these drivers are
// supported
func (fm *FingerprintManager) setupDrivers(drivers []string) error {
@ -100,62 +89,81 @@ func (fm *FingerprintManager) setupDrivers(drivers []string) error {
return err
}
if hc, ok := d.(fingerprint.HealthCheck); ok {
fm.healthCheck(name, hc)
}
detected, err := fm.fingerprintDriver(name, d)
if err != nil {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting for %v failed: %+v", name, err)
return err
}
if hc, ok := d.(fingerprint.HealthCheck); ok {
fm.runHealthCheck(name, hc)
} else {
// for drivers which are not of type health check, update them to have their
// health status match the status of whether they are detected or not.
healthInfo := &structs.DriverInfo{
Healthy: detected,
UpdateTime: time.Now(),
}
if node := fm.updateNodeFromDriver(name, nil, healthInfo); node != nil {
fm.nodeLock.Lock()
fm.node = node
fm.nodeLock.Unlock()
}
}
go fm.watchDriver(d, name)
// log the fingerprinters which have been applied
if detected {
availDrivers = append(availDrivers, name)
}
// We should only run the health check task if the driver is detected
// Note that if the driver is detected later in a periodic health check,
// this won't automateically trigger the periodic health check.
if hc, ok := d.(fingerprint.HealthCheck); ok && detected {
p, period := d.Periodic()
if p {
go fm.runFingerprint(d, period, name)
}
req := &cstructs.HealthCheckIntervalRequest{}
resp := &cstructs.HealthCheckIntervalResponse{}
hc.GetHealthCheckInterval(req, resp)
if resp.Eligible {
go fm.runHealthCheck(hc, resp.Period, name)
}
} else {
p, period := d.Periodic()
if p {
go fm.runFingerprintDriver(d, period, name)
}
}
}
fm.logger.Printf("[DEBUG] client.fingerprint_manager: detected drivers %v", availDrivers)
return nil
}
func (fm *FingerprintManager) runFingerprintDriver(f fingerprint.Fingerprint, period time.Duration, name string) {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting driver %s every %v", name, period)
// watchDrivers facilitates the different periods between fingerprint and
// health checking a driver
func (fm *FingerprintManager) watchDriver(d driver.Driver, name string) {
_, fingerprintPeriod := d.Periodic()
fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting driver %s every %v", name, fingerprintPeriod)
var healthCheckPeriod time.Duration
hc, isHealthCheck := d.(fingerprint.HealthCheck)
if isHealthCheck {
req := &cstructs.HealthCheckIntervalRequest{}
var resp cstructs.HealthCheckIntervalResponse
hc.GetHealthCheckInterval(req, &resp)
if resp.Eligible {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: health checking driver %s every %v", name, healthCheckPeriod)
healthCheckPeriod = resp.Period
}
}
t1 := time.NewTimer(fingerprintPeriod)
defer t1.Stop()
t2 := time.NewTimer(healthCheckPeriod)
defer t2.Stop()
for {
select {
case <-time.After(period):
_, err := fm.fingerprintDriver(name, f)
if err != nil {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: periodic fingerprinting for driver %v failed: %+v", name, err)
continue
}
case <-fm.shutdownCh:
return
case <-t1.C:
t1.Reset(fingerprintPeriod)
_, err := fm.fingerprintDriver(name, d)
if err != nil {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: periodic fingerprinting for driver %v failed: %+v", name, err)
}
case <-t2.C:
if isHealthCheck {
t2.Reset(healthCheckPeriod)
err := fm.runHealthCheck(name, hc)
if err != nil {
fm.logger.Printf("[DEBUG] client.fingerprint_manager: health checking for %v failed: %v", name, err)
}
}
}
}
}
@ -176,6 +184,12 @@ func (fm *FingerprintManager) fingerprintDriver(name string, f fingerprint.Finge
return false, err
}
if node := fm.updateNodeAttributes(&response); node != nil {
fm.nodeLock.Lock()
fm.node = node
fm.nodeLock.Unlock()
}
// COMPAT: Remove in 0.9: As of Nomad 0.8 there is a temporary measure to
// update all driver attributes to its corresponding driver info object,
// as eventually all drivers will need to
@ -188,43 +202,17 @@ func (fm *FingerprintManager) fingerprintDriver(name string, f fingerprint.Finge
strings.Replace(copy, "driver.", "", 1)
strippedAttributes[k] = v
}
di := &structs.DriverInfo{
Attributes: strippedAttributes,
Detected: response.Detected,
}
response.AddDriver(name, di)
if node := fm.updateNodeAttributes(&response); node != nil {
if node := fm.updateNodeFromDriver(name, di, nil); node != nil {
fm.nodeLock.Lock()
fm.node = node
fm.nodeLock.Unlock()
}
// COMPAT: Remove in 0.9: As of Nomad 0.8, there is a driver info for every
// driver. As a compatibility mechanism, we proactively set this driver info
// for drivers which do not yet support health checks.
if _, ok := f.(fingerprint.HealthCheck); !ok {
resp := &cstructs.HealthCheckResponse{
Drivers: map[string]*structs.DriverInfo{
name: &structs.DriverInfo{
// This achieves backwards compatibility- before, we only relied on the
// status of the fingerprint method to determine whether a driver was
// enabled. For drivers without health checks yet enabled, we should always
// set this to true, and then once we enable health checks, this can
// dynamically update this value.
Healthy: true,
UpdateTime: time.Now(),
},
},
}
if node := fm.updateHealthCheck(resp); node != nil {
fm.nodeLock.Lock()
fm.node = node
fm.nodeLock.Unlock()
}
}
return response.Detected, nil
}
@ -253,12 +241,12 @@ func (fm *FingerprintManager) fingerprint(name string, f fingerprint.Fingerprint
}
// healthcheck checks the health of the specified resource.
func (fm *FingerprintManager) healthCheck(name string, hc fingerprint.HealthCheck) error {
func (fm *FingerprintManager) runHealthCheck(name string, hc fingerprint.HealthCheck) error {
request := &cstructs.HealthCheckRequest{}
var response cstructs.HealthCheckResponse
err := hc.HealthCheck(request, &response)
if node := fm.updateHealthCheck(&response); node != nil {
if node := fm.updateNodeFromDriver(name, nil, response.Drivers[name]); node != nil {
fm.nodeLock.Lock()
fm.node = node
fm.nodeLock.Unlock()
@ -294,17 +282,6 @@ func (fm *FingerprintManager) setupFingerprinters(fingerprints []string) error {
if p {
go fm.runFingerprint(f, period, name)
}
// XXX This code path doesn't exist right now
if hc, ok := f.(fingerprint.HealthCheck); ok {
req := &cstructs.HealthCheckIntervalRequest{}
var resp cstructs.HealthCheckIntervalResponse
if err := hc.GetHealthCheckInterval(req, &resp); err != nil {
if resp.Eligible {
go fm.runHealthCheck(hc, resp.Period, name)
}
}
}
}
fm.logger.Printf("[DEBUG] client.fingerprint_manager: detected fingerprints %v", appliedFingerprints)

41
client/fingerprint_manager_test.go
View File

@ -6,7 +6,6 @@ import (
"github.com/hashicorp/nomad/client/config"
"github.com/hashicorp/nomad/client/driver"
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/nomad/testutil"
"github.com/stretchr/testify/require"
@ -37,6 +36,7 @@ func TestFingerprintManager_Run_MockDriver(t *testing.T) {
make(chan struct{}),
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -70,6 +70,7 @@ func TestFingerprintManager_Run_ResourcesFingerprint(t *testing.T) {
make(chan struct{}),
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -105,6 +106,7 @@ func TestFingerprintManager_Fingerprint_Run(t *testing.T) {
make(chan struct{}),
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -149,6 +151,7 @@ func TestFingerprintManager_Fingerprint_Periodic(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -194,24 +197,13 @@ func TestFingerprintManager_HealthCheck_Driver(t *testing.T) {
node := &structs.Node{
Attributes: make(map[string]string, 0),
Links: make(map[string]string, 0),
Resources: &structs.Resources{},
Drivers: make(map[string]*structs.DriverInfo, 0),
}
updateNode := func(r *cstructs.FingerprintResponse) *structs.Node {
if r.Attributes != nil {
for k, v := range r.Attributes {
node.Attributes[k] = v
}
}
return node
}
updateHealthCheck := func(resp *cstructs.HealthCheckResponse) *structs.Node {
if resp.Drivers != nil {
for k, v := range resp.Drivers {
node.Drivers[k] = v
}
}
return node
}
testConfig := config.Config{Node: node}
testClient := &Client{config: &testConfig}
conf := config.DefaultConfig()
conf.Options = map[string]string{
"driver.raw_exec.enable": "1",
@ -231,8 +223,9 @@ func TestFingerprintManager_HealthCheck_Driver(t *testing.T) {
getConfig,
node,
shutdownCh,
updateNode,
updateHealthCheck,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -327,6 +320,7 @@ func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -384,7 +378,7 @@ func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) {
if mockDriverInfo == nil {
return false, fmt.Errorf("mock driver info should be set on the client")
}
if !mockDriverInfo.Detected {
if mockDriverInfo.Detected {
return false, fmt.Errorf("mock driver should be detected")
}
if mockDriverInfo.Healthy {
@ -427,6 +421,7 @@ func TestFimgerprintManager_Run_InWhitelist(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -466,6 +461,7 @@ func TestFingerprintManager_Run_InBlacklist(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -506,6 +502,7 @@ func TestFingerprintManager_Run_Combination(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -550,6 +547,7 @@ func TestFingerprintManager_Run_WhitelistDrivers(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -590,6 +588,7 @@ func TestFingerprintManager_Run_AllDriversBlacklisted(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -634,6 +633,7 @@ func TestFingerprintManager_Run_DriversWhiteListBlacklistCombination(t *testing.
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)
@ -677,6 +677,7 @@ func TestFingerprintManager_Run_DriversInBlacklist(t *testing.T) {
shutdownCh,
testClient.updateNodeFromFingerprint,
testClient.updateNodeFromHealthCheck,
testClient.updateNodeFromDriver,
testLogger(),
)

13
client/structs/structs.go
View File

@ -361,19 +361,6 @@ type FingerprintResponse struct {
// Detected is a boolean indicating whether the fingerprinter detected
// if the resource was available
Detected bool
// Drivers is a map of driver names to driver info. This allows the
// fingerprint method of each driver to set whether the driver is enabled or
// not, as well as its attributes
Drivers map[string]*structs.DriverInfo
}
func (f *FingerprintResponse) AddDriver(name string, value *structs.DriverInfo) {
if f.Drivers == nil {
f.Drivers = make(map[string]*structs.DriverInfo, 0)
}
f.Drivers[name] = value
}
// AddAttribute adds the name and value for a node attribute to the fingerprint