Refactor client fingerprinters to return a diff of node attributes #3781
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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,42 @@ func (c *Client) fingerprint() error { | |
|
|
||
| c.logger.Printf("[DEBUG] client: built-in fingerprints: %v", fingerprint.BuiltinFingerprints()) | ||
|
|
||
| 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 { | ||
| skippedFingerprints = append(skippedFingerprints, name) | ||
| continue | ||
| } | ||
| // Skip modules that are in the blacklist | ||
| if _, ok := blacklist[name]; ok { | ||
| skippedFingerprints = append(skippedFingerprints, name) | ||
| continue | ||
| } | ||
|
|
||
| f, err := fingerprint.NewFingerprint(name, c.logger) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node} | ||
| var response cstructs.FingerprintResponse | ||
| c.configLock.Lock() | ||
| err = f.Fingerprint(request, &response) | ||
| c.configLock.Unlock() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // log the fingerprinters which have been applied | ||
| if response.Applicable { | ||
| appliedFingerprints = append(appliedFingerprints, 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 +976,10 @@ func (c *Client) fingerprint() error { | |
| go c.fingerprintPeriodic(name, f, period) | ||
| } | ||
| } | ||
|
|
||
| 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 +990,18 @@ func (c *Client) fingerprintPeriodic(name string, f fingerprint.Fingerprint, d t | |
| for { | ||
| select { | ||
| case <-time.After(d): | ||
| request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node} | ||
| var response cstructs.FingerprintResponse | ||
|
There was a problem hiding this comment. The lock should wrap creating the request since it is accessing the config. (not just here) |
||
| c.configLock.Lock() | ||
| err := f.Fingerprint(request, &response) | ||
| c.configLock.Unlock() | ||
|
|
||
| if err != nil { | ||
| c.logger.Printf("[DEBUG] client: periodic fingerprinting for %v failed: %v", name, err) | ||
| } else { | ||
| c.updateNodeFromFingerprint(&response) | ||
| } | ||
|
|
||
| case <-c.shutdownCh: | ||
| return | ||
| } | ||
|
|
@@ -997,52 +1015,85 @@ func (c *Client) setupDrivers() error { | |
| whitelistEnabled := len(whitelist) > 0 | ||
| blacklist := c.config.ReadStringListToMap("driver.blacklist") | ||
|
|
||
| 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 { | ||
| skippedDrivers = append(skippedDrivers, name) | ||
| continue | ||
| } | ||
| // Skip fingerprinting drivers that are in the blacklist | ||
| if _, ok := blacklist[name]; ok { | ||
| skippedDrivers = append(skippedDrivers, name) | ||
| continue | ||
| } | ||
|
|
||
| d, err := driver.NewDriver(name, driverCtx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node} | ||
| var response cstructs.FingerprintResponse | ||
| c.configLock.Lock() | ||
| if err := d.Fingerprint(request, &response); err != nil { | ||
| return err | ||
| } | ||
| c.configLock.Unlock() | ||
|
There was a problem hiding this comment. Need to unlock on error. Should probably just capture the error, unlock and then check if it isn't nil |
||
|
|
||
| // log the fingerprinters which have been applied | ||
| if response.Applicable { | ||
| availDrivers = append(availDrivers, name) | ||
| } | ||
|
|
||
| c.updateNodeFromFingerprint(&response) | ||
|
|
||
| p, period := d.Periodic() | ||
| if p { | ||
| go c.fingerprintPeriodic(name, d, period) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| 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.Attributes { | ||
|
There was a problem hiding this comment. How are you handling removing Attributes/Links? There was a problem hiding this comment. As in fingerprinter_foo adds attribute = {"foo": "bar", "bam": "baz"} and then the next time it is called only returns {"foo": "bar"} because "bam" should be removed. There was a problem hiding this comment. Good point. One way we can handle this is if the attribute is equal to empty in the fingerprinter's response, we can delete this attribute from the node's attributes. That way we don't need to rely on the fingerprinter modifying the node directly. |
||
| 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.Links { | ||
| if val == "" { | ||
| delete(c.config.Node.Links, name) | ||
| } else { | ||
| c.config.Node.Links[name] = val | ||
| } | ||
| } | ||
|
|
||
| if response.Resources != nil { | ||
| c.config.Node.Resources.Merge(response.Resources) | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. 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. 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 { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.