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

chelseakomlo · 2018-01-22T13:46:33Z

No description provided.

preetapan · 2018-01-25T20:08:11Z

client/fingerprint/fingerprint.go

-	// and returns if the fingerprint was applicable and a potential error.
-	Fingerprint(*config.Config, *structs.Node) (bool, error)
+	// and returns a diff of updated node attributes and a potential error.
+	Fingerprint(*cstructs.FingerprintRequest, *cstructs.FingerprintResponse) error


Why not make this be Fingerprint(*structs.FingerPrintRequest) (structs.FingerprintResponse, error) so it returns the response?

Sure, I think it could be either way. I'm curious what @dadgar thinks as we talked about this signature before.

I suggested this so it will be easier to move to a remote RPC

For when this moves to a plugin? Makes sense

preetapan · 2018-01-25T20:48:26Z

client/client.go

-		if applies {
+
+		// remove attributes we are supposed to skip
+		for _, attr := range skipped {


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.

preetapan · 2018-01-25T21:49:37Z

client/driver/lxc_test.go

-	}
-	if apply {
-		t.Fatalf("should not apply with config")
+	// test with an empty config


consider turning these into subtests with a test case struct to capture inputs and expected output

dadgar · 2018-01-25T23:47:19Z

client/client.go

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


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.

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.

dadgar · 2018-01-25T23:48:18Z

client/client.go

+			delete(response.Attributes, e)
+		}
+
+		for name := range response.Attributes {


The applied list is applied fingerprinters not the attributes.

dadgar · 2018-01-25T23:57:14Z

client/client.go

+func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintResponse) {
+	c.configLock.Lock()
+	defer c.configLock.Unlock()
+	for name, val := range response.Attributes {


How are you handling removing Attributes/Links?

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.

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.

dadgar · 2018-01-25T23:58:51Z

client/client.go

+		c.config.Node.Links[name] = val
+	}
+
+	c.config.Node.Resources.Merge(response.Resources)


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

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

dadgar · 2018-01-26T00:03:51Z

client/client.go

-		c.configLock.Unlock()
+
+		request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node}
+		response := &cstructs.FingerprintResponse{


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.

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.

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.

write test for client periodic fingerprinters

dadgar · 2018-01-29T22:21:40Z

client/structs/structs.go

+}
+
+type FingerprintResponse struct {
+	attributes map[string]string


These should be public.

There are getters and setters for these, to prevent against uninitialized fields. Alternatively, we could have a constructor for FingerprintResponse which would do initialization of values all at once.

dadgar · 2018-01-29T22:23:01Z

client/structs/structs.go

+type FingerprintResponse struct {
+	attributes map[string]string
+	links      map[string]string
+	resources  *structs.Resources


Should we have a Applicable bool in the response so we can maintain our old behavior of displaying whether the fingerprinter is being applied.

Sure, we could do that, that would probably be cleaner than parsing attribute values after the fact.

On second thought- would logging drivers where fingerprintSuccess is true achieve the same goal?

dadgar · 2018-01-29T22:30:03Z

client/client.go

+		// 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}


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.

public fields and remove getter functions

dadgar · 2018-01-31T18:55:34Z

client/client.go

 			return err
 		}
-		if applies {
-			avail = append(avail, name)
+		c.configLock.Unlock()


Need to unlock on error. Should probably just capture the error, unlock and then check if it isn't nil

dadgar · 2018-01-31T18:58:33Z

client/client.go

+	if response.Resources != nil {
+		c.config.Node.Resources.Merge(response.Resources)
+	}
+}


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

This will be in a follow up PR.

dadgar · 2018-01-31T19:00:23Z

client/client_test.go

+
+	c1 := testClient(t, func(c *config.Config) {
+		c.Options = map[string]string{
+			"test.shutdown_periodic_after":    "true",


Can you make this a constant on the mock driver.
"test.mock_driver.shutdown_periodic..."

dadgar · 2018-01-31T19:03:26Z

client/driver/docker_test.go

 	}
-	t.Logf("Found docker version %s", node.Attributes["driver.docker.version"])
+
+	t.Logf("Found docker version %s", attributes["driver.docker.version"])


Shouldn't this be require.NotEmpty(attributes["driver.docker.version"])

dadgar · 2018-01-31T19:04:29Z

client/driver/docker_test.go

+	}
+
+	attributes := response.Attributes
+	if attributes == nil {


Use require

dadgar · 2018-01-31T19:06:09Z

client/driver/exec_linux.go

 	// 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")


Can you make this an INFO level log

dadgar · 2018-01-31T19:06:29Z

client/driver/exec_linux.go

 		if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
 			d.logger.Printf("[DEBUG] driver.exec: cgroups unavailable, disabling")
 		}
 		d.fingerprintSuccess = helper.BoolToPtr(false)
-		delete(node.Attributes, execDriverAttr)
-		return false, nil
+		resp.RemoveAttribute(execDriverAttr)


In both these cases applicable should be true?

dadgar · 2018-01-31T19:07:39Z

client/driver/java.go

 	// Only enable if we are root and cgroups are mounted when running on linux systems.
-	if runtime.GOOS == "linux" && (syscall.Geteuid() != 0 || !cgroupsMounted(node)) {
+	if runtime.GOOS == "linux" && (syscall.Geteuid() != 0 || !cgroupsMounted(req.Node)) {
 		if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
 			d.logger.Printf("[DEBUG] driver.java: root privileges and mounted cgroups required on linux, disabling")


dadgar · 2018-01-31T19:10:02Z

client/driver/mock_driver.go

+	// if the shutdown configuration options are set, start the timer here.
+	// This config option defaults to false
+	if ctx.config != nil && ctx.config.ReadBoolDefault(fingerprint.ShutdownPeriodicAfter, false) {
+		duration, err := ctx.config.ReadInt(fingerprint.ShutdownPeriodicDuration)


How about storing the duration and start time as a field on the mock driver and changing the Fingerprint method to be if time.Now().After(d.shutdownFingerprintTime) {. Avoids the other function and goroutine

dadgar · 2018-02-01T22:38:10Z

client/driver/java.go

@@ -119,6 +119,7 @@ func (d *JavaDriver) Fingerprint(req *cstructs.FingerprintRequest, resp *cstruct
 		}
 		d.fingerprintSuccess = helper.BoolToPtr(false)
 		resp.RemoveAttribute(javaDriverAttr)
+		resp.Detected = true


We should probably move this block to below checking for the java binary.

dadgar · 2018-02-01T22:40:29Z

client/client.go

@@ -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


The lock should wrap creating the request since it is accessing the config. (not just here)

dadgar · 2018-02-01T22:42:20Z

client/driver/mock_driver.go

 	cstructs "github.com/hashicorp/nomad/client/structs"
 	"github.com/hashicorp/nomad/nomad/structs"
 )

+const (
+


Remove blank line

dadgar · 2018-02-01T22:44:53Z

client/structs/structs.go

@@ -184,3 +187,61 @@ func (d *DriverNetwork) Hash() []byte {
 	}
 	return h.Sum(nil)
 }
+
+type FingerprintRequest struct {


Comments on all public structs

changelog

github-actions · 2023-03-13T02:14:40Z

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 force-pushed the f-client-fingerprint-refactor branch 15 times, most recently from 5802f45 to 60b3c3d Compare January 24, 2018 16:32

refactor Fingerprint to request/response construct

5e8151d

chelseakomlo force-pushed the f-client-fingerprint-refactor branch from 60b3c3d to 5e8151d Compare January 24, 2018 16:54

chelseakomlo changed the title ~~WIP: Refactor client fingerprinters to return a diff of node attributes~~ Refactor client fingerprinters to return a diff of node attributes Jan 24, 2018

chelseakomlo requested review from dadgar and removed request for dadgar January 24, 2018 19:28

add test case for available cgroups

a76a404

chelseakomlo mentioned this pull request Jan 25, 2018

WIP: add driver manager to manage client fingerprinters #3796

Closed

preetapan reviewed Jan 25, 2018

View reviewed changes

dadgar requested changes Jan 26, 2018

View reviewed changes

chelseakomlo added 2 commits January 26, 2018 07:04

fixups from code review

c21ac46

create safe getters and setters for fingerprint response

f5fc20a

chelseakomlo force-pushed the f-client-fingerprint-refactor branch from 1139ae0 to a19ac99 Compare January 28, 2018 15:37

chelseakomlo force-pushed the f-client-fingerprint-refactor branch 2 times, most recently from 5faef04 to 6633c64 Compare January 29, 2018 12:47

chelseakomlo mentioned this pull request Jan 29, 2018

Add fingerprint manager to manage fingerprinting node #3807

Merged

remove attributes from periodic fingerprints when state changes

ae889b4

write test for client periodic fingerprinters

chelseakomlo force-pushed the f-client-fingerprint-refactor branch from 6633c64 to ae889b4 Compare January 29, 2018 18:49

dadgar requested changes Jan 29, 2018

View reviewed changes

locks for fingerprint reads/writes

e8aaa93

chelseakomlo force-pushed the f-client-fingerprint-refactor branch from 05a8c33 to db37992 Compare January 31, 2018 18:02

add applicable boolean to fingerprint response

a9447ad

public fields and remove getter functions

chelseakomlo force-pushed the f-client-fingerprint-refactor branch from db37992 to a9447ad Compare January 31, 2018 18:21

dadgar requested changes Jan 31, 2018

View reviewed changes

chelseakomlo added 3 commits January 31, 2018 18:34

code review fixup

ba2ebbc

add detected to more drivers where the driver is found but unusable

e5ccc55

fix up linting

15cb768

dadgar requested changes Feb 1, 2018

View reviewed changes

dadgar approved these changes Feb 1, 2018

View reviewed changes

req/resp should be within config locks; rename for detected fingerprints

3202200

changelog

chelseakomlo force-pushed the f-client-fingerprint-refactor branch from d9769e2 to 3202200 Compare February 2, 2018 00:02

update log message

8049aa0

chelseakomlo merged commit 7b9cf12 into master Feb 2, 2018

chelseakomlo deleted the f-client-fingerprint-refactor branch February 2, 2018 01:13

chelseakomlo mentioned this pull request Feb 14, 2018

Edge trigger node updates #3873

Merged

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

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

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

Conversation

chelseakomlo commented Jan 22, 2018

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

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 13, 2023