-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor client fingerprinters to return a diff of node attributes #3781
Changes from 5 commits
5e8151d
a76a404
c21ac46
f5fc20a
ae889b4
e8aaa93
a9447ad
ba2ebbc
e5ccc55
15cb768
3202200
8049aa0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |
"github.com/hashicorp/nomad/client/driver" | ||
"github.com/hashicorp/nomad/client/fingerprint" | ||
"github.com/hashicorp/nomad/client/stats" | ||
cstructs "github.com/hashicorp/nomad/client/structs" | ||
"github.com/hashicorp/nomad/client/vaultclient" | ||
"github.com/hashicorp/nomad/command/agent/consul" | ||
"github.com/hashicorp/nomad/helper" | ||
|
@@ -931,33 +932,38 @@ func (c *Client) fingerprint() error { | |
|
||
c.logger.Printf("[DEBUG] client: built-in fingerprints: %v", fingerprint.BuiltinFingerprints()) | ||
|
||
var applied []string | ||
var skipped []string | ||
var appliedFingerprints []string | ||
var skippedFingerprints []string | ||
for _, name := range fingerprint.BuiltinFingerprints() { | ||
// Skip modules that are not in the whitelist if it is enabled. | ||
if _, ok := whitelist[name]; whitelistEnabled && !ok { | ||
skipped = append(skipped, name) | ||
skippedFingerprints = append(skippedFingerprints, name) | ||
continue | ||
} | ||
// Skip modules that are in the blacklist | ||
if _, ok := blacklist[name]; ok { | ||
skipped = append(skipped, name) | ||
skippedFingerprints = append(skippedFingerprints, name) | ||
continue | ||
} | ||
|
||
f, err := fingerprint.NewFingerprint(name, c.logger) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
c.configLock.Lock() | ||
applies, err := f.Fingerprint(c.config, c.config.Node) | ||
c.configLock.Unlock() | ||
// Apply the finerprint to our list so that we can log it later | ||
appliedFingerprints = append(appliedFingerprints, name) | ||
|
||
request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node} | ||
var response cstructs.FingerprintResponse | ||
err = f.Fingerprint(request, &response) | ||
if err != nil { | ||
return err | ||
} | ||
if applies { | ||
applied = append(applied, name) | ||
} | ||
|
||
// add the diff found from each fingerprinter | ||
c.updateNodeFromFingerprint(&response) | ||
|
||
p, period := f.Periodic() | ||
if p { | ||
// TODO: If more periodic fingerprinters are added, then | ||
|
@@ -966,9 +972,10 @@ func (c *Client) fingerprint() error { | |
go c.fingerprintPeriodic(name, f, period) | ||
} | ||
} | ||
c.logger.Printf("[DEBUG] client: applied fingerprints %v", applied) | ||
if len(skipped) != 0 { | ||
c.logger.Printf("[DEBUG] client: fingerprint modules skipped due to white/blacklist: %v", skipped) | ||
|
||
c.logger.Printf("[DEBUG] client: applied fingerprints %v", appliedFingerprints) | ||
if len(skippedFingerprints) != 0 { | ||
c.logger.Printf("[DEBUG] client: fingerprint modules skipped due to white/blacklist: %v", skippedFingerprints) | ||
} | ||
return nil | ||
} | ||
|
@@ -979,11 +986,16 @@ func (c *Client) fingerprintPeriodic(name string, f fingerprint.Fingerprint, d t | |
for { | ||
select { | ||
case <-time.After(d): | ||
c.configLock.Lock() | ||
if _, err := f.Fingerprint(c.config, c.config.Node); err != nil { | ||
request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node} | ||
var response cstructs.FingerprintResponse | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The lock should wrap creating the request since it is accessing the config. (not just here) |
||
err := f.Fingerprint(request, &response) | ||
|
||
if err != nil { | ||
c.logger.Printf("[DEBUG] client: periodic fingerprinting for %v failed: %v", name, err) | ||
} else { | ||
c.updateNodeFromFingerprint(&response) | ||
} | ||
c.configLock.Unlock() | ||
|
||
case <-c.shutdownCh: | ||
return | ||
} | ||
|
@@ -997,35 +1009,38 @@ func (c *Client) setupDrivers() error { | |
whitelistEnabled := len(whitelist) > 0 | ||
blacklist := c.config.ReadStringListToMap("driver.blacklist") | ||
|
||
var avail []string | ||
var skipped []string | ||
var availDrivers []string | ||
var skippedDrivers []string | ||
driverCtx := driver.NewDriverContext("", "", c.config, c.config.Node, c.logger, nil) | ||
for name := range driver.BuiltinDrivers { | ||
// Skip fingerprinting drivers that are not in the whitelist if it is | ||
// enabled. | ||
if _, ok := whitelist[name]; whitelistEnabled && !ok { | ||
skipped = append(skipped, name) | ||
skippedDrivers = append(skippedDrivers, name) | ||
continue | ||
} | ||
// Skip fingerprinting drivers that are in the blacklist | ||
if _, ok := blacklist[name]; ok { | ||
skipped = append(skipped, name) | ||
skippedDrivers = append(skippedDrivers, name) | ||
continue | ||
} | ||
|
||
// Add this driver to our list of available drivers so that we can log it | ||
// later | ||
availDrivers = append(availDrivers, name) | ||
|
||
d, err := driver.NewDriver(name, driverCtx) | ||
if err != nil { | ||
return err | ||
} | ||
c.configLock.Lock() | ||
applies, err := d.Fingerprint(c.config, c.config.Node) | ||
c.configLock.Unlock() | ||
if err != nil { | ||
|
||
request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node} | ||
var response cstructs.FingerprintResponse | ||
if err := d.Fingerprint(request, &response); err != nil { | ||
return err | ||
} | ||
if applies { | ||
avail = append(avail, name) | ||
} | ||
|
||
c.updateNodeFromFingerprint(&response) | ||
|
||
p, period := d.Periodic() | ||
if p { | ||
|
@@ -1034,15 +1049,40 @@ func (c *Client) setupDrivers() error { | |
|
||
} | ||
|
||
c.logger.Printf("[DEBUG] client: available drivers %v", avail) | ||
|
||
if len(skipped) != 0 { | ||
c.logger.Printf("[DEBUG] client: drivers skipped due to white/blacklist: %v", skipped) | ||
c.logger.Printf("[DEBUG] client: available drivers %v", availDrivers) | ||
if len(skippedDrivers) > 0 { | ||
c.logger.Printf("[DEBUG] client: drivers skipped due to white/blacklist: %v", skippedDrivers) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// updateNodeFromFingerprint updates the node with the result of | ||
// fingerprinting the node from the diff that was created | ||
func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintResponse) { | ||
c.configLock.Lock() | ||
defer c.configLock.Unlock() | ||
for name, val := range response.GetAttributes() { | ||
if val == "" { | ||
delete(c.config.Node.Attributes, name) | ||
} else { | ||
c.config.Node.Attributes[name] = val | ||
} | ||
} | ||
|
||
// update node links and resources from the diff created from | ||
// fingerprinting | ||
for name, val := range response.GetLinks() { | ||
if val == "" { | ||
delete(c.config.Node.Links, name) | ||
} else { | ||
c.config.Node.Links[name] = val | ||
} | ||
} | ||
|
||
c.config.Node.Resources.Merge(response.GetResources()) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should change the node watcher from checking periodically to check via a channel trigger and send it when this function is called: https://github.com/hashicorp/nomad/blob/master/client/client.go#L1588 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be in a follow up PR. |
||
|
||
// retryIntv calculates a retry interval value given the base | ||
func (c *Client) retryIntv(base time.Duration) time.Duration { | ||
if c.config.DevMode { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import ( | |
"github.com/hashicorp/consul/lib/freeport" | ||
memdb "github.com/hashicorp/go-memdb" | ||
"github.com/hashicorp/nomad/client/config" | ||
"github.com/hashicorp/nomad/client/driver" | ||
"github.com/hashicorp/nomad/client/fingerprint" | ||
"github.com/hashicorp/nomad/command/agent/consul" | ||
"github.com/hashicorp/nomad/helper" | ||
|
@@ -252,6 +253,46 @@ func TestClient_HasNodeChanged(t *testing.T) { | |
} | ||
} | ||
|
||
func TestClient_Fingerprint_Periodic(t *testing.T) { | ||
if _, ok := driver.BuiltinDrivers["mock_driver"]; !ok { | ||
t.Skip(`test requires mock_driver; run with "-tags nomad_test"`) | ||
} | ||
t.Parallel() | ||
|
||
c1 := testClient(t, func(c *config.Config) { | ||
c.Options = map[string]string{ | ||
"test.shutdown_periodic_after": "true", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make this a constant on the mock driver. |
||
"test.shutdown_periodic_duration": "3", | ||
} | ||
}) | ||
defer c1.Shutdown() | ||
|
||
node := c1.config.Node | ||
mockDriverName := "driver.mock_driver" | ||
|
||
// Ensure the mock driver is registered on the client | ||
testutil.WaitForResult(func() (bool, error) { | ||
mockDriverStatus := node.Attributes[mockDriverName] | ||
if mockDriverStatus == "" { | ||
return false, fmt.Errorf("mock driver attribute should be set on the client") | ||
} | ||
return true, nil | ||
}, func(err error) { | ||
t.Fatalf("err: %v", err) | ||
}) | ||
|
||
// Ensure that the client fingerprinter eventually removes this attribute | ||
testutil.WaitForResult(func() (bool, error) { | ||
mockDriverStatus := node.Attributes[mockDriverName] | ||
if mockDriverStatus != "" { | ||
return false, fmt.Errorf("mock driver attribute should not be set on the client") | ||
} | ||
return true, nil | ||
}, func(err error) { | ||
t.Fatalf("err: %v", err) | ||
}) | ||
} | ||
|
||
func TestClient_Fingerprint_InWhitelist(t *testing.T) { | ||
t.Parallel() | ||
c := testClient(t, func(c *config.Config) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,17 +171,24 @@ func TestDockerDriver_Fingerprint(t *testing.T) { | |
node := &structs.Node{ | ||
Attributes: make(map[string]string), | ||
} | ||
apply, err := d.Fingerprint(&config.Config{}, node) | ||
|
||
request := &cstructs.FingerprintRequest{Config: &config.Config{}, Node: node} | ||
var response cstructs.FingerprintResponse | ||
err := d.Fingerprint(request, &response) | ||
if err != nil { | ||
t.Fatalf("err: %v", err) | ||
} | ||
if apply != testutil.DockerIsConnected(t) { | ||
|
||
attributes := response.GetAttributes() | ||
if testutil.DockerIsConnected(t) && attributes["driver.docker"] == "" { | ||
t.Fatalf("Fingerprinter should detect when docker is available") | ||
} | ||
if node.Attributes["driver.docker"] != "1" { | ||
|
||
if attributes["driver.docker"] != "1" { | ||
t.Log("Docker daemon not available. The remainder of the docker tests will be skipped.") | ||
} | ||
t.Logf("Found docker version %s", node.Attributes["driver.docker.version"]) | ||
|
||
t.Logf("Found docker version %s", attributes["driver.docker.version"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be |
||
} | ||
|
||
// TestDockerDriver_Fingerprint_Bridge asserts that if Docker is running we set | ||
|
@@ -210,18 +217,23 @@ func TestDockerDriver_Fingerprint_Bridge(t *testing.T) { | |
conf := testConfig(t) | ||
conf.Node = mock.Node() | ||
dd := NewDockerDriver(NewDriverContext("", "", conf, conf.Node, testLogger(), nil)) | ||
ok, err := dd.Fingerprint(conf, conf.Node) | ||
|
||
request := &cstructs.FingerprintRequest{Config: conf, Node: conf.Node} | ||
var response cstructs.FingerprintResponse | ||
err = dd.Fingerprint(request, &response) | ||
if err != nil { | ||
t.Fatalf("error fingerprinting docker: %v", err) | ||
} | ||
if !ok { | ||
|
||
attributes := response.GetAttributes() | ||
if attributes["driver.docker"] == "" { | ||
t.Fatalf("expected Docker to be enabled but false was returned") | ||
} | ||
|
||
if found := conf.Node.Attributes["driver.docker.bridge_ip"]; found != expectedAddr { | ||
if found := attributes["driver.docker.bridge_ip"]; found != expectedAddr { | ||
t.Fatalf("expected bridge ip %q but found: %q", expectedAddr, found) | ||
} | ||
t.Logf("docker bridge ip: %q", conf.Node.Attributes["driver.docker.bridge_ip"]) | ||
t.Logf("docker bridge ip: %q", attributes["driver.docker.bridge_ip"]) | ||
} | ||
|
||
func TestDockerDriver_StartOpen_Wait(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe you can get rid of the lock yet. This is a shared data structure that is being mutated/read concurrently. Apples to all places you are doing a fingerprint.