Address comments and reserve

client/alloc_runner.go

@@ -395,8 +395,7 @@ func (r *AllocRunner) Run() {
 	if r.ctx == nil {
 		path := filepath.Join(r.config.AllocDir, r.alloc.ID)
 		size := r.Alloc().Resources.DiskMB
-		allocDir := allocdir.NewAllocDir(r.alloc.ID, path, size)
-		allocDir.SetSecretDirFn(r.secretDir.CreateFor)
+		allocDir := allocdir.NewAllocDir(r.alloc.ID, path, size, r.secretDir.CreateFor)
 		if err := allocDir.Build(tg.Tasks); err != nil {
 			r.logger.Printf("[ERR] client: failed to build task directories: %v", err)
 			r.setStatus(structs.AllocClientStatusFailed, fmt.Sprintf("failed to build task dirs for '%s'", alloc.TaskGroup))

client/allocdir/alloc_dir.go

@@ -60,7 +60,8 @@ type AllocDir struct {
 	// AllocID is the allocation ID for this directory
 	AllocID string
 
-	// TODO
+	// createSecretDirFn is used to create a secret directory and retrieve the
+	// path to it so that it can be mounted in the task directory.
 	createSecretDirFn CreateSecretDirFn
 
 	// AllocDir is the directory used for storing any state
@@ -121,8 +122,10 @@ type AllocDirFS interface {
 }
 
 // NewAllocDir initializes the AllocDir struct with allocDir as base path for
-// the allocation directory and maxSize as the maximum allowed size in megabytes.
-func NewAllocDir(allocID, allocDir string, maxSize int) *AllocDir {
+// the allocation directory and maxSize as the maximum allowed size in
+// megabytes. The secretDirFn is used to create secret directories and get their
+// path which will then be mounted into the task directory
+func NewAllocDir(allocID, allocDir string, maxSize int, secretDirFn CreateSecretDirFn) *AllocDir {
 	d := &AllocDir{
 		AllocID:                   allocID,
 		AllocDir:                  allocDir,
@@ -131,11 +134,14 @@ func NewAllocDir(allocID, allocDir string, maxSize int) *AllocDir {
 		CheckDiskMaxEnforcePeriod: checkDiskMaxEnforcePeriod,
 		TaskDirs:                  make(map[string]string),
 		MaxSize:                   maxSize,
+		createSecretDirFn:         secretDirFn,
 	}
 	d.SharedDir = filepath.Join(d.AllocDir, SharedAllocName)
 	return d
 }
 
+// SetSecretDirFn is used to set the function used to create secret
+// directories.
 func (d *AllocDir) SetSecretDirFn(fn CreateSecretDirFn) {
 	d.createSecretDirFn = fn
 }
@@ -263,7 +269,11 @@ func (d *AllocDir) Build(tasks []*structs.Task) error {
 			// Mount the secret directory
 			taskSecret := filepath.Join(taskDir, TaskSecrets)
 			if err := d.mount(sdir, taskSecret); err != nil {
-				return fmt.Errorf("failed to mount secret directory: %v", err)
+				return fmt.Errorf("failed to mount secret directory for task %q: %v", t.Name, err)
+			}
+
+			if err := d.dropDirPermissions(taskSecret); err != nil {
+				return err
 			}
 		}
 	}

client/allocdir/alloc_dir_test.go

@@ -52,7 +52,12 @@ func TestAllocDir_BuildAlloc(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(tmp, structs.DefaultResources().DiskMB)
+	secretDirFn := func(allocID, task string) (string, error) {
+		return ioutil.TempDir("", "")
+	}
+
+	allocID := "123"
+	d := NewAllocDir(allocID, tmp, structs.DefaultResources().DiskMB, secretDirFn)
 	defer d.Destroy()
 	tasks := []*structs.Task{t1, t2}
 	if err := d.Build(tasks); err != nil {
@@ -73,6 +78,10 @@ func TestAllocDir_BuildAlloc(t *testing.T) {
 		if _, err := os.Stat(tDir); os.IsNotExist(err) {
 			t.Fatalf("Build(%v) didn't create TaskDir %v", tasks, tDir)
 		}
+
+		if _, err := os.Stat(filepath.Join(tDir, TaskSecrets)); os.IsNotExist(err) {
+			t.Fatalf("Build(%v) didn't create secret dir %v", tasks)
+		}
 	}
 }
 
@@ -83,7 +92,7 @@ func TestAllocDir_LogDir(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(tmp, structs.DefaultResources().DiskMB)
+	d := NewAllocDir("123", tmp, structs.DefaultResources().DiskMB, TestCreateSecretDirFn)
 	defer d.Destroy()
 
 	expected := filepath.Join(d.AllocDir, SharedAllocName, LogDirName)
@@ -99,7 +108,7 @@ func TestAllocDir_EmbedNonExistent(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(tmp, structs.DefaultResources().DiskMB)
+	d := NewAllocDir("123", tmp, structs.DefaultResources().DiskMB, TestCreateSecretDirFn)
 	defer d.Destroy()
 	tasks := []*structs.Task{t1, t2}
 	if err := d.Build(tasks); err != nil {
@@ -121,7 +130,7 @@ func TestAllocDir_EmbedDirs(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(tmp, structs.DefaultResources().DiskMB)
+	d := NewAllocDir("123", tmp, structs.DefaultResources().DiskMB, TestCreateSecretDirFn)
 	defer d.Destroy()
 	tasks := []*structs.Task{t1, t2}
 	if err := d.Build(tasks); err != nil {
@@ -182,7 +191,7 @@ func TestAllocDir_MountSharedAlloc(t *testing.T) {
 	}
 	defer os.RemoveAll(tmp)
 
-	d := NewAllocDir(tmp, structs.DefaultResources().DiskMB)
+	d := NewAllocDir("123", tmp, structs.DefaultResources().DiskMB, TestCreateSecretDirFn)
 	defer d.Destroy()
 	tasks := []*structs.Task{t1, t2}
 	if err := d.Build(tasks); err != nil {

client/allocdir/alloc_dir_testing.go

@@ -0,0 +1,8 @@
+package allocdir
+
+import "io/ioutil"
+
+// TestCreateSecretDirFn is used to create a secret dir suitable for testing
+func TestCreateSecretDirFn(_, _ string) (string, error) {
+	return ioutil.TempDir("", "")
+}

client/allocdir/alloc_dir_windows.go

@@ -1,9 +1,9 @@
 package allocdir
 
 import (
-	"errors"
 	"os"
 	"path/filepath"
+	"syscall"
 )
 
 var (
@@ -18,14 +18,14 @@ func (d *AllocDir) linkOrCopy(src, dst string, perm os.FileMode) error {
 	return fileCopy(src, dst, perm)
 }
 
-// The windows version does nothing currently.
+// Hardlinks the shared directory. As a side-effect the src and dest directory
+// must be on the same filesystem.
 func (d *AllocDir) mount(src, dest string) error {
-	return errors.New("Mount on Windows not supported.")
+	return syscall.Link(src, dest)
 }
 
-// The windows version does nothing currently.
 func (d *AllocDir) unmount(dir string) error {
-	return nil
+	return syscall.Unlink(dir)
 }
 
 // The windows version does nothing currently.

client/client.go

@@ -111,7 +111,7 @@ type Client struct {
 
 	connPool *nomad.ConnPool
 
-	secretDir *secretdir.SecretDir
+	secretDir secretdir.SecretDirectory
 
 	// lastHeartbeatFromQuorum is an atomic int32 acting as a bool.  When
 	// true, the last heartbeat message had a leader.  When false (0),
@@ -192,7 +192,7 @@ func NewClient(cfg *config.Config, consulSyncer *consul.Syncer, logger *log.Logg
 	}
 
 	// Setup the reserved resources
-	c.reservePorts()
+	c.reserveResources()
 
 	// Store the config copy before restoring state but after it has been
 	// initialized.
@@ -603,10 +603,24 @@ func (c *Client) setupNode() error {
 	return nil
 }
 
+// reserveResources is used to reserve resources on the Node that will be
+// registered with the Server.
+func (c *Client) reserveResources() {
+	c.reservePorts()
+
+	// Add the memory consumed by the secret directory
+	c.configLock.Lock()
+	if c.config.Node.Reserved == nil {
+		c.config.Node.Reserved = new(structs.Resources)
+	}
+	c.config.Node.Reserved.MemoryMB += c.secretDir.MemoryUse()
+	c.configLock.Unlock()
+}
+
 // reservePorts is used to reserve ports on the fingerprinted network devices.
 func (c *Client) reservePorts() {
-	c.configLock.RLock()
-	defer c.configLock.RUnlock()
+	c.configLock.Lock()
+	defer c.configLock.Unlock()
 	global := c.config.GloballyReservedPorts
 	if len(global) == 0 {
 		return

client/client_test.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/nomad/client/config"
+	"github.com/hashicorp/nomad/client/secretdir"
 	"github.com/hashicorp/nomad/command/agent/consul"
 	"github.com/hashicorp/nomad/nomad"
 	"github.com/hashicorp/nomad/nomad/mock"
@@ -149,6 +150,22 @@ func TestClient_RPC_Passthrough(t *testing.T) {
 	})
 }
 
+func TestClient_ReserveSecretDir(t *testing.T) {
+	c := testClient(t, nil)
+	defer c.Shutdown()
+
+	tsd := secretdir.NewTestSecretDir(t)
+	c.secretDir = tsd
+	expected := 10
+	tsd.MemoryUsed = expected
+
+	c.reserveResources()
+	res := c.Node().Reserved
+	if res == nil || res.MemoryMB != expected {
+		t.Fatalf("Unexpected reserved memory: %v", res)
+	}
+}
+
 func TestClient_Fingerprint(t *testing.T) {
 	c := testClient(t, nil)
 	defer c.Shutdown()

client/driver/driver_test.go

@@ -2,7 +2,6 @@ package driver
 
 import (
 	"io"
-	"io/ioutil"
 	"log"
 	"math/rand"
 	"os"
@@ -82,11 +81,8 @@ func testDriverContexts(task *structs.Task) (*DriverContext, *ExecContext) {
 	cfg := testConfig()
 	id := structs.GenerateUUID()
 	path := filepath.Join(cfg.AllocDir, id)
-	allocDir := allocdir.NewAllocDir(id, path, task.Resources.DiskMB)
+	allocDir := allocdir.NewAllocDir(id, path, task.Resources.DiskMB, allocdir.TestCreateSecretDirFn)
 	allocDir.Build([]*structs.Task{task})
-	allocDir.SetSecretDirFn(func(a, b string) (string, error) {
-		return ioutil.TempDir("", "")
-	})
 	alloc := mock.Alloc()
 	execCtx := NewExecContext(allocDir, alloc.ID)
 

client/driver/executor/executor_test.go

@@ -38,10 +38,7 @@ func mockAllocDir(t *testing.T) (*structs.Task, *allocdir.AllocDir) {
 	task := alloc.Job.TaskGroups[0].Tasks[0]
 
 	path := filepath.Join(os.TempDir(), alloc.ID)
-	allocDir := allocdir.NewAllocDir(alloc.ID, path, task.Resources.DiskMB)
-	allocDir.SetSecretDirFn(func(a, b string) (string, error) {
-		return ioutil.TempDir("", "")
-	})
+	allocDir := allocdir.NewAllocDir(alloc.ID, path, task.Resources.DiskMB, allocdir.TestCreateSecretDirFn)
 	if err := allocDir.Build([]*structs.Task{task}); err != nil {
 		log.Panicf("allocDir.Build() failed: %v", err)
 	}

client/secretdir/secret_dir.go

@@ -6,9 +6,8 @@ import (
 	"path/filepath"
 )
 
-const ()
-
 type SecretDirectory interface {
+	MemoryUse() int
 	Destroy() error
 	CreateFor(allocID, task string) (path string, err error)
 	Remove(allocID, task string) error
@@ -38,8 +37,7 @@ func (s *SecretDir) init() error {
 		return fmt.Errorf("failed to stat secret dir: %v", err)
 	}
 
-	// TODO this shouldn't be hardcoded
-	if err := s.create(32); err != nil {
+	if err := s.create(); err != nil {
 		return fmt.Errorf("failed to create secret dir: %v", err)
 	}
 

client/secretdir/secret_dir_testing.go

@@ -11,6 +11,9 @@ import (
 type TestSecretDir struct {
 	// Dir is the path to the secret directory
 	Dir string
+
+	// MemoryUsed is returned when the MemoryUse function is called
+	MemoryUsed int
 }
 
 func NewTestSecretDir(t *testing.T) *TestSecretDir {
@@ -46,3 +49,5 @@ func (s *TestSecretDir) Remove(allocID, task string) error {
 	path := s.getPathFor(allocID, task)
 	return os.RemoveAll(path)
 }
+
+func (s *TestSecretDir) MemoryUse() int { return s.MemoryUsed }

client/secretdir/secret_dir_universal.go

@@ -5,7 +5,7 @@ package secretdir
 import "os"
 
 // create creates a normal folder at the secret dir path
-func (s *SecretDir) create(sizeMB int) error {
+func (s *SecretDir) create() error {
 	return os.MkdirAll(s.Dir, 0700)
 }
 
@@ -13,3 +13,8 @@ func (s *SecretDir) create(sizeMB int) error {
 func (s *SecretDir) destroy() error {
 	return os.RemoveAll(s.Dir)
 }
+
+// MemoryUse returns the memory used by the SecretDir
+func (s *SecretDir) MemoryUse() int {
+	return 0
+}

client/secretdir/secret_dir_unix.go

@@ -8,15 +8,21 @@ import (
 	"syscall"
 )
 
+const (
+	// SecretDirTmpfsSize is the size in MB of the in tmpfs backing the secret
+	// directory
+	SecretDirTmpfsSize = 32
+)
+
 // create creates a tmpfs folder at the secret dir path
-func (s *SecretDir) create(sizeMB int64) error {
+func (s *SecretDir) create() error {
 	if err := os.MkdirAll(s.Dir, 0700); err != nil {
 		return err
 	}
 
 	var flags uintptr
 	flags = syscall.MS_NOEXEC
-	options := fmt.Sprintf("size=%dm", sizeMB)
+	options := fmt.Sprintf("size=%dm", SecretDirTmpfsSize)
 	err := syscall.Mount("tmpfs", s.Dir, "tmpfs", flags, options)
 	return os.NewSyscallError("mount", err)
 }
@@ -29,3 +35,8 @@ func (s *SecretDir) destroy() error {
 
 	return os.RemoveAll(s.Dir)
 }
+
+// MemoryUse returns the memory used by the SecretDir
+func (s *SecretDir) MemoryUse() int {
+	return SecretDirTmpfsSize
+}

client/secretdir/secret_dir_unix_test.go

@@ -1 +0,0 @@
-package secretdir

client/task_runner_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/hashicorp/nomad/client/allocdir"
 	"github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/client/driver"
-	"github.com/hashicorp/nomad/client/secretdir"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/testutil"
@@ -57,9 +56,7 @@ func testTaskRunnerFromAlloc(t *testing.T, restarts bool, alloc *structs.Allocat
 	task.Resources.Networks[0].ReservedPorts = []structs.Port{{"", 80}}
 
 	path := filepath.Join(conf.AllocDir, alloc.ID)
-	allocDir := allocdir.NewAllocDir(alloc.ID, path, task.Resources.DiskMB)
-	sdir := secretdir.NewTestSecretDir(t)
-	allocDir.SetSecretDirFn(sdir.CreateFor)
+	allocDir := allocdir.NewAllocDir(alloc.ID, path, task.Resources.DiskMB, allocdir.TestCreateSecretDirFn)
 	allocDir.Build([]*structs.Task{task})
 
 	ctx := driver.NewExecContext(allocDir, alloc.ID)