From 44487cc7f1493836b7c8bcffb34eadea19d45751 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sun, 10 Jul 2016 13:55:06 -0700 Subject: [PATCH 1/7] Build the Cgroup fingerprinter on only Linux. Change the logic from `!linux` to an empty build tag so that *if* another platform picks up Cgroups support they can add themselves to the necessary build tags for this fingerprinter and be on their way. Because this technology isn't inherently Linux-specific and isn't mutually exclusive of other resource isolation containers, resist the urge to rename the Cgroup fingerprinter to something generic like the ResourceContainerFingerprinter. --- client/fingerprint/cgroup.go | 2 ++ client/fingerprint/cgroup_default.go | 2 +- client/fingerprint/cgroup_test.go | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/client/fingerprint/cgroup.go b/client/fingerprint/cgroup.go index c0ec7c674..1ec8d8793 100644 --- a/client/fingerprint/cgroup.go +++ b/client/fingerprint/cgroup.go @@ -1,3 +1,5 @@ +// +build linux + package fingerprint import ( diff --git a/client/fingerprint/cgroup_default.go b/client/fingerprint/cgroup_default.go index 43c55fd85..51e3bf58c 100644 --- a/client/fingerprint/cgroup_default.go +++ b/client/fingerprint/cgroup_default.go @@ -1,4 +1,4 @@ -// +build !linux +// +build package fingerprint diff --git a/client/fingerprint/cgroup_test.go b/client/fingerprint/cgroup_test.go index be5d251e0..f12379405 100644 --- a/client/fingerprint/cgroup_test.go +++ b/client/fingerprint/cgroup_test.go @@ -1,3 +1,5 @@ +// +build linux + package fingerprint import ( From 572925a5fa6a9695a033770535a6c96fea5fe407 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sun, 10 Jul 2016 14:53:13 -0700 Subject: [PATCH 2/7] Fix test for non-Linux platforms. The following tests now check a whitelist for whether or not their driver is present or not, or if the OS is supported or not. * `TestAllocDir_MountSharedAlloc` * `TestClient_Drivers_InWhitelist` (`exec` driver) * `TestClient_Drivers` (`exec` driver) * `TestJavaDriver_Fingerprint` (`java` driver) --- client/allocdir/alloc_dir_test.go | 11 ++++++++++- client/client_test.go | 21 ++++++++++++++++++--- client/driver/java_test.go | 13 ++++++++++++- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index 90f25af2c..c39497ec9 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/client/allocdir/alloc_dir_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "reflect" + "runtime" "testing" "github.com/hashicorp/nomad/client/testutil" @@ -12,6 +13,10 @@ import ( ) var ( + osMountSharedDirSupport = map[string]bool{ + "linux": true, + } + t1 = &structs.Task{ Name: "web", Driver: "exec", @@ -193,7 +198,11 @@ func TestAllocDir_MountSharedAlloc(t *testing.T) { for _, task := range tasks { // Mount and then check that the file exists in the task directory. if err := d.MountSharedDir(task.Name); err != nil { - t.Fatalf("MountSharedDir(%v) failed: %v", task.Name, err) + if v, ok := osMountSharedDirSupport[runtime.GOOS]; v && ok { + t.Fatalf("MountSharedDir(%v) failed: %v", task.Name, err) + } else { + t.Skipf("MountShareDir(%v) failed, no OS support") + } } taskDir, ok := d.TaskDirs[task.Name] diff --git a/client/client_test.go b/client/client_test.go index e4b1d461e..60af15bad 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -7,6 +7,7 @@ import ( "net" "os" "path/filepath" + "runtime" "sync/atomic" "testing" "time" @@ -22,7 +23,13 @@ import ( ctestutil "github.com/hashicorp/nomad/client/testutil" ) -var nextPort uint32 = 16000 +var ( + nextPort uint32 = 16000 + + osExecDriverSupport = map[string]bool{ + "linux": true, + } +) func getPort() int { return int(atomic.AddUint32(&nextPort, 1)) @@ -225,7 +232,11 @@ func TestClient_Drivers(t *testing.T) { node := c.Node() if node.Attributes["driver.exec"] == "" { - t.Fatalf("missing exec driver") + if v, ok := osExecDriverSupport[runtime.GOOS]; v && ok { + t.Fatalf("missing exec driver") + } else { + t.Skipf("missing exec driver, no OS support") + } } } @@ -242,7 +253,11 @@ func TestClient_Drivers_InWhitelist(t *testing.T) { node := c.Node() if node.Attributes["driver.exec"] == "" { - t.Fatalf("missing exec driver") + if v, ok := osExecDriverSupport[runtime.GOOS]; v && ok { + t.Fatalf("missing exec driver") + } else { + t.Skipf("missing exec driver, no OS support") + } } } diff --git a/client/driver/java_test.go b/client/driver/java_test.go index e33b47597..5daad2580 100644 --- a/client/driver/java_test.go +++ b/client/driver/java_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "time" @@ -15,6 +16,12 @@ import ( ctestutils "github.com/hashicorp/nomad/client/testutil" ) +var ( + osJavaDriverSupport = map[string]bool{ + "linux": true, + } +) + // javaLocated checks whether java is installed so we can run java stuff. func javaLocated() bool { _, err := exec.Command("java", "-version").CombinedOutput() @@ -40,7 +47,11 @@ func TestJavaDriver_Fingerprint(t *testing.T) { t.Fatalf("Fingerprinter should detect Java when it is installed") } if node.Attributes["driver.java"] != "1" { - t.Fatalf("missing driver") + if v, ok := osJavaDriverSupport[runtime.GOOS]; v && ok { + t.Fatalf("missing java driver") + } else { + t.Skipf("missing java driver, no OS support") + } } for _, key := range []string{"driver.java.version", "driver.java.runtime", "driver.java.vm"} { if node.Attributes[key] == "" { From 226df7dea9b8082d9f21497b580cf9ca59d51b1f Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sun, 10 Jul 2016 20:16:06 -0700 Subject: [PATCH 3/7] Skip the network fingerprinter test when offline. Conditionalize the network fingerprinter test so that it works when a user is offline. Similarly, when the network fingerprint test fails in the future pass a HINT to the user to set an env var to allow the test to be skipped in the future. --- client/fingerprint/network_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/client/fingerprint/network_test.go b/client/fingerprint/network_test.go index 6742c5ca5..2d8fa4ea3 100644 --- a/client/fingerprint/network_test.go +++ b/client/fingerprint/network_test.go @@ -3,12 +3,17 @@ package fingerprint import ( "fmt" "net" + "os" "testing" "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/nomad/structs" ) +// Set skipOnlineTestEnvVar to a non-empty value to skip network tests. Useful +// when working offline (e.g. an airplane). +const skipOnlineTestsEnvVar = "TEST_NOMAD_SKIP_ONLINE_NET" + var ( lo = net.Interface{ Index: 2, @@ -138,6 +143,10 @@ func (n *NetworkInterfaceDetectorMultipleInterfaces) Addrs(intf *net.Interface) } func TestNetworkFingerprint_basic(t *testing.T) { + if v := os.Getenv(skipOnlineTestsEnvVar); v != "" { + t.Skipf("Environment variable %+q not empty, skipping test", skipOnlineTestsEnvVar) + } + f := &NetworkFingerprint{logger: testLogger(), interfaceDetector: &DefaultNetworkInterfaceDetector{}} node := &structs.Node{ Attributes: make(map[string]string), @@ -149,7 +158,7 @@ func TestNetworkFingerprint_basic(t *testing.T) { t.Fatalf("err: %v", err) } if !ok { - t.Fatalf("should apply") + t.Fatalf("should apply (HINT: working offline? Set env %q=y", skipOnlineTestsEnvVar) } assertNodeAttributeContains(t, node, "unique.network.ip-address") From c26bdebc28289f7717ab38b1480698b4acb583dc Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sun, 10 Jul 2016 20:18:57 -0700 Subject: [PATCH 4/7] Improve readability: use of a switch vs two if's --- client/fingerprint/network.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/client/fingerprint/network.go b/client/fingerprint/network.go index f2fec805e..9d4e8683a 100644 --- a/client/fingerprint/network.go +++ b/client/fingerprint/network.go @@ -55,12 +55,11 @@ func (f *NetworkFingerprint) Fingerprint(cfg *config.Config, node *structs.Node) var ip string intf, err := f.findInterface(cfg.NetworkInterface) - if err != nil { + switch { + case err != nil: return false, fmt.Errorf("Error while detecting network interface during fingerprinting: %v", err) - } - - // No interface could be found - if intf == nil { + case intf == nil: + // No interface could be found return false, nil } From bb8b47d47803027e9aba44acc791f57e8d1c65a1 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 11 Jul 2016 12:16:56 -0700 Subject: [PATCH 5/7] Remove cgroup fingerprinter from non-linux systems. If someone wants to extend or reuse Cgroup detenction in the future they can move `cgroup_linux.go` to `cgroup.go` and add the relevant build tags. Requested by: @dadgar --- client/fingerprint/cgroup_default.go | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 client/fingerprint/cgroup_default.go diff --git a/client/fingerprint/cgroup_default.go b/client/fingerprint/cgroup_default.go deleted file mode 100644 index 51e3bf58c..000000000 --- a/client/fingerprint/cgroup_default.go +++ /dev/null @@ -1,18 +0,0 @@ -// +build - -package fingerprint - -import ( - client "github.com/hashicorp/nomad/client/config" - "github.com/hashicorp/nomad/nomad/structs" -) - -// FindCgroupMountpointDir returns an empty path on non-Linux systems -func FindCgroupMountpointDir() (string, error) { - return "", nil -} - -// Fingerprint tries to find a valid cgroup moint point -func (f *CGroupFingerprint) Fingerprint(cfg *client.Config, node *structs.Node) (bool, error) { - return false, nil -} From 444998cc37bf38c2288b3ad8ccc9e1d2dc6abde8 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 11 Jul 2016 12:19:17 -0700 Subject: [PATCH 6/7] Darwin currently has allocdir support. Pointed out by: @dadgar --- client/allocdir/alloc_dir_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index c39497ec9..6f459fff4 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/client/allocdir/alloc_dir_test.go @@ -14,7 +14,8 @@ import ( var ( osMountSharedDirSupport = map[string]bool{ - "linux": true, + "darwin": true, + "linux": true, } t1 = &structs.Task{ From dd7c28165bb346bf5ee907a0592f14f6cdf77804 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Mon, 11 Jul 2016 12:23:46 -0700 Subject: [PATCH 7/7] Alpha-sort the build platforms --- client/driver/exec_default.go | 2 +- client/fingerprint/fingerprint_default.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/client/driver/exec_default.go b/client/driver/exec_default.go index 975cbc595..176938726 100644 --- a/client/driver/exec_default.go +++ b/client/driver/exec_default.go @@ -1,4 +1,4 @@ -//+build windows darwin dragonfly freebsd netbsd openbsd solaris +//+build darwin dragonfly freebsd netbsd openbsd solaris windows package driver diff --git a/client/fingerprint/fingerprint_default.go b/client/fingerprint/fingerprint_default.go index 2825408d9..e2ae1ec6f 100644 --- a/client/fingerprint/fingerprint_default.go +++ b/client/fingerprint/fingerprint_default.go @@ -1,4 +1,4 @@ -// +build windows darwin dragonfly freebsd netbsd openbsd solaris +// +build darwin dragonfly freebsd netbsd openbsd solaris windows package fingerprint