Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor client fingerprinters to return a diff of node attributes #3781

Merged
merged 12 commits into from
Feb 2, 2018
Merged

Refactor client fingerprinters to return a diff of node attributes #3781

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5e8151d
refactor Fingerprint to request/response construct
chelseakomlo Jan 24, 2018
a76a404
add test case for available cgroups
chelseakomlo Jan 25, 2018
c21ac46
fixups from code review
chelseakomlo Jan 26, 2018
f5fc20a
create safe getters and setters for fingerprint response
chelseakomlo Jan 26, 2018
ae889b4
remove attributes from periodic fingerprints when state changes
chelseakomlo Jan 26, 2018
e8aaa93
locks for fingerprint reads/writes
chelseakomlo Jan 30, 2018
a9447ad
add applicable boolean to fingerprint response
chelseakomlo Jan 30, 2018
ba2ebbc
code review fixup
chelseakomlo Jan 31, 2018
e5ccc55
add detected to more drivers where the driver is found but unusable
chelseakomlo Feb 1, 2018
15cb768
fix up linting
chelseakomlo Feb 1, 2018
3202200
req/resp should be within config locks; rename for detected fingerprints
chelseakomlo Feb 1, 2018
8049aa0
update log message
chelseakomlo Feb 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 72 additions & 11 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -949,15 +950,31 @@ func (c *Client) fingerprint() error {
return err
}

c.configLock.Lock()
applies, err := f.Fingerprint(c.config, c.config.Node)
c.configLock.Unlock()
request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node}
Copy link
Contributor

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.

response := &cstructs.FingerprintResponse{
Attributes: make(map[string]string, 0),
Links: make(map[string]string, 0),
Resources: &structs.Resources{},
}

err = f.Fingerprint(request, response)
if err != nil {
return err
}
if applies {

// if an attribute should be skipped, remove it from the list which we will
// later apply to the node
for _, e := range skipped {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't attributes that are being skipped. These are whole fingerprinters so they won't have created any attribute that needs to be deleted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, only visible after expanding to get context that skipped is constructed from fingerprinter names. The diff made it look like it was mutating the output based on the skipped list.

delete(response.Attributes, e)
}

for name := range response.Attributes {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The applied list is applied fingerprinters not the attributes.

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
Expand All @@ -966,6 +983,7 @@ 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)
Expand All @@ -979,11 +997,21 @@ 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}
response := &cstructs.FingerprintResponse{
Attributes: make(map[string]string, 0),
Links: make(map[string]string, 0),
Resources: &structs.Resources{},
}

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
}
Expand Down Expand Up @@ -1017,16 +1045,30 @@ func (c *Client) setupDrivers() error {
if err != nil {
return err
}
c.configLock.Lock()
applies, err := d.Fingerprint(c.config, c.config.Node)
c.configLock.Unlock()

request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node}
response := &cstructs.FingerprintResponse{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer if it looked like:

var resp cstructs.FingerprintResponse
if err := d.Fingerprint(req, &resp); err != nil {
  // Handle
}

You can then have methods on the response to be like:

func (f *FingerprintResponse) AddAttribute(attr, value string) {...}

Where it initializes the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could have a constructor which would initialize all of these values. I will add getter/setter methods that does initialization for each field, let me know if you think it is cleaner to just do initialization all at once in a constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the constructor could work but it is just idiomatic go to call an RPC like I showed, which is the direction we will be moving.

Attributes: make(map[string]string, 0),
Links: make(map[string]string, 0),
Resources: &structs.Resources{},
}

err = d.Fingerprint(request, response)
if err != nil {
return err
}
if applies {

// remove attributes we are supposed to skip
for _, attr := range skipped {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't care about thiese skipped attributes and are going to delete them from the response as soon as the fingerprint call is done, I would suggest pushing that logic one level down. You could have the FingerprintRequest also take a list of skipped(better name would be ignore) and have the implementors of FingerPrint ignore or remove anything in skipped before it returns.

The current approach works, but it is not well encapsulated - its doing something, then mutating the response before using it.

delete(response.Attributes, attr)
}

for name := range response.Attributes {
avail = append(avail, name)
}

c.updateNodeFromFingerprint(response)

p, period := d.Periodic()
if p {
go c.fingerprintPeriodic(name, d, period)
Expand All @@ -1035,6 +1077,7 @@ func (c *Client) setupDrivers() error {
}

c.logger.Printf("[DEBUG] client: available drivers %v", avail)
c.logger.Printf("[DEBUG] client: skipped attributes %v", skipped)

if len(skipped) != 0 {
c.logger.Printf("[DEBUG] client: drivers skipped due to white/blacklist: %v", skipped)
Expand All @@ -1043,6 +1086,24 @@ func (c *Client) setupDrivers() error {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you handling removing Attributes/Links?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

c.config.Node.Attributes[name] = val
}

// update node links and resources from the diff created from
// fingerprinting
for name, val := range response.Links {
c.config.Node.Links[name] = val
}

c.config.Node.Resources.Merge(response.Resources)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with this strategy for now but this doesn't let a fingerprinter report a non-zero value and then set it to zero. The long term solution is that we pass in a different struct type that has all the fields as a pointer and as long as the fingerprinter sets the pointer we merge the value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add that as a tech task to complete after this refactor.

}
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Expand Down
22 changes: 9 additions & 13 deletions client/driver/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/go-plugin"
"github.com/hashicorp/nomad/client/allocdir"
"github.com/hashicorp/nomad/client/config"
"github.com/hashicorp/nomad/client/driver/env"
"github.com/hashicorp/nomad/client/driver/executor"
dstructs "github.com/hashicorp/nomad/client/driver/structs"
Expand Down Expand Up @@ -476,42 +475,39 @@ func NewDockerDriver(ctx *DriverContext) Driver {
return &DockerDriver{DriverContext: *ctx}
}

func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
// Initialize docker API clients
func (d *DockerDriver) Fingerprint(req *cstructs.FingerprintRequest, resp *cstructs.FingerprintResponse) error {
client, _, err := d.dockerClients()
if err != nil {
if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
d.logger.Printf("[INFO] driver.docker: failed to initialize client: %s", err)
}
delete(node.Attributes, dockerDriverAttr)
d.fingerprintSuccess = helper.BoolToPtr(false)
return false, nil
return nil
}

// This is the first operation taken on the client so we'll try to
// establish a connection to the Docker daemon. If this fails it means
// Docker isn't available so we'll simply disable the docker driver.
env, err := client.Version()
if err != nil {
delete(node.Attributes, dockerDriverAttr)
if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
d.logger.Printf("[DEBUG] driver.docker: could not connect to docker daemon at %s: %s", client.Endpoint(), err)
}
d.fingerprintSuccess = helper.BoolToPtr(false)
return false, nil
return nil
}

node.Attributes[dockerDriverAttr] = "1"
node.Attributes["driver.docker.version"] = env.Get("Version")
resp.Attributes[dockerDriverAttr] = "1"
resp.Attributes["driver.docker.version"] = env.Get("Version")

privileged := d.config.ReadBoolDefault(dockerPrivilegedConfigOption, false)
if privileged {
node.Attributes[dockerPrivilegedConfigOption] = "1"
resp.Attributes[dockerPrivilegedConfigOption] = "1"
}

// Advertise if this node supports Docker volumes
if d.config.ReadBoolDefault(dockerVolumesConfigOption, dockerVolumesConfigDefault) {
node.Attributes["driver."+dockerVolumesConfigOption] = "1"
resp.Attributes["driver."+dockerVolumesConfigOption] = "1"
}

// Detect bridge IP address - #2785
Expand All @@ -529,7 +525,7 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
}

if n.IPAM.Config[0].Gateway != "" {
node.Attributes["driver.docker.bridge_ip"] = n.IPAM.Config[0].Gateway
resp.Attributes["driver.docker.bridge_ip"] = n.IPAM.Config[0].Gateway
} else if d.fingerprintSuccess == nil {
// Docker 17.09.0-ce dropped the Gateway IP from the bridge network
// See https://github.com/moby/moby/issues/32648
Expand All @@ -540,7 +536,7 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
}

d.fingerprintSuccess = helper.BoolToPtr(true)
return true, nil
return nil
}

// Validate is used to validate the driver configuration
Expand Down
35 changes: 27 additions & 8 deletions client/driver/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,17 +171,28 @@ 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}
response := &cstructs.FingerprintResponse{
Attributes: make(map[string]string, 0),
Links: make(map[string]string, 0),
Resources: &structs.Resources{},
}

err := d.Fingerprint(request, response)
if err != nil {
t.Fatalf("err: %v", err)
}
if apply != testutil.DockerIsConnected(t) {

if testutil.DockerIsConnected(t) && response.Attributes["driver.docker"] == "" {
t.Fatalf("Fingerprinter should detect when docker is available")
}
if node.Attributes["driver.docker"] != "1" {

if response.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", response.Attributes["driver.docker.version"])
}

// TestDockerDriver_Fingerprint_Bridge asserts that if Docker is running we set
Expand Down Expand Up @@ -210,18 +221,26 @@ 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}
response := &cstructs.FingerprintResponse{
Attributes: make(map[string]string, 0),
Links: make(map[string]string, 0),
Resources: &structs.Resources{},
}

err = dd.Fingerprint(request, response)
if err != nil {
t.Fatalf("error fingerprinting docker: %v", err)
}
if !ok {
if response.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 := response.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", response.Attributes["driver.docker.bridge_ip"])
}

func TestDockerDriver_StartOpen_Wait(t *testing.T) {
Expand Down
7 changes: 3 additions & 4 deletions client/driver/exec_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
package driver

import (
"github.com/hashicorp/nomad/client/config"
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/helper"
"github.com/hashicorp/nomad/nomad/structs"
)

func (d *ExecDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
func (d *ExecDriver) Fingerprint(req *cstructs.FingerprintRequest, resp *cstructs.FingerprintResponse) error {
d.fingerprintSuccess = helper.BoolToPtr(false)
return false, nil
return nil
}
17 changes: 7 additions & 10 deletions client/driver/exec_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package driver

import (
"github.com/hashicorp/nomad/client/config"
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/helper"
"github.com/hashicorp/nomad/nomad/structs"
"golang.org/x/sys/unix"
)

Expand All @@ -13,28 +12,26 @@ const (
execDriverAttr = "driver.exec"
)

func (d *ExecDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
func (d *ExecDriver) Fingerprint(req *cstructs.FingerprintRequest, resp *cstructs.FingerprintResponse) error {
// Only enable if cgroups are available and we are root
if !cgroupsMounted(node) {
if !cgroupsMounted(req.Node) {
if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
d.logger.Printf("[DEBUG] driver.exec: cgroups unavailable, disabling")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this an INFO level log

}
d.fingerprintSuccess = helper.BoolToPtr(false)
delete(node.Attributes, execDriverAttr)
return false, nil
return nil
} else if unix.Geteuid() != 0 {
if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
d.logger.Printf("[DEBUG] driver.exec: must run as root user, disabling")
}
delete(node.Attributes, execDriverAttr)
d.fingerprintSuccess = helper.BoolToPtr(false)
return false, nil
return nil
}

if d.fingerprintSuccess == nil || !*d.fingerprintSuccess {
d.logger.Printf("[DEBUG] driver.exec: exec driver is enabled")
}
node.Attributes[execDriverAttr] = "1"
resp.Attributes[execDriverAttr] = "1"
d.fingerprintSuccess = helper.BoolToPtr(true)
return true, nil
return nil
}
16 changes: 11 additions & 5 deletions client/driver/exec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"github.com/hashicorp/nomad/client/config"
"github.com/hashicorp/nomad/client/driver/env"
cstructs "github.com/hashicorp/nomad/client/structs"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/nomad/testutil"

Expand All @@ -37,14 +38,19 @@ func TestExecDriver_Fingerprint(t *testing.T) {
"unique.cgroup.mountpoint": "/sys/fs/cgroup",
},
}
apply, err := d.Fingerprint(&config.Config{}, node)

request := &cstructs.FingerprintRequest{Config: &config.Config{}, Node: node}
response := &cstructs.FingerprintResponse{
Attributes: make(map[string]string, 0),
Links: make(map[string]string, 0),
Resources: &structs.Resources{},
}

err := d.Fingerprint(request, response)
if err != nil {
t.Fatalf("err: %v", err)
}
if !apply {
t.Fatalf("should apply")
}
if node.Attributes["driver.exec"] == "" {
if response.Attributes["driver.exec"] == "" {
t.Fatalf("missing driver")
}
}
Expand Down
Loading