Added comments

client/client.go

@@ -445,19 +445,13 @@ func (c *Client) Stats() map[string]map[string]string {
 
 // CollectAllocation garbage collects a single allocation
 func (c *Client) CollectAllocation(allocID string) error {
-	if err := c.garbageCollector.Collect(allocID); err != nil {
-		return err
-	}
-	return nil
+	return c.garbageCollector.Collect(allocID)
 }
 
 // CollectAllAllocs garbage collects all allocations on a node in the terminal
 // state
 func (c *Client) CollectAllAllocs() error {
-	if err := c.garbageCollector.CollectAll(); err != nil {
-		return err
-	}
-	return nil
+	return c.garbageCollector.CollectAll()
 }
 
 // Node returns the locally registered node

client/gc.go

@@ -28,6 +28,8 @@ const (
 	MB = 1024 * 1024
 )
 
+// GCAlloc wraps an allocation runner and an index enabling it to be used within
+// a PQ
 type GCAlloc struct {
 	timeStamp   time.Time
 	allocRunner *AllocRunner
@@ -162,7 +164,7 @@ func (a *AllocGarbageCollector) run() {
 		select {
 		case <-ticker.C:
 			if err := a.keepUsageBelowThreshold(); err != nil {
-				a.logger.Printf("[ERR] client: error GCing allocation: %v", err)
+				a.logger.Printf("[ERR] client: error garbage collecting allocation: %v", err)
 			}
 		case <-a.shutdownCh:
 			ticker.Stop()
@@ -214,7 +216,7 @@ func (a *AllocGarbageCollector) keepUsageBelowThreshold() error {
 }
 
 func (a *AllocGarbageCollector) Stop() {
-	a.shutdownCh <- struct{}{}
+	close(a.shutdownCh)
 }
 
 // Collect garbage collects a single allocation on a node
@@ -281,7 +283,7 @@ func (a *AllocGarbageCollector) MakeRoomFor(allocations []*structs.Allocation) e
 		}
 
 		if allocDirStats != nil {
-			if allocDirStats.Available >= uint64(totalResource.DiskMB*1024*1024) {
+			if allocDirStats.Available >= uint64(totalResource.DiskMB*MB) {
 				break
 			}
 		} else {

client/gc_test.go

@@ -162,6 +162,7 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_EnoughSpace(t *testing.T)
 	statsCollector.inodePercents = []float64{0}
 
 	alloc := mock.Alloc()
+	alloc.Resources.DiskMB = 150
 	if err := gc.MakeRoomFor([]*structs.Allocation{alloc}); err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -197,6 +198,7 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Partial(t *testing.T) {
 	statsCollector.inodePercents = []float64{0, 0, 0}
 
 	alloc := mock.Alloc()
+	alloc.Resources.DiskMB = 150
 	if err := gc.MakeRoomFor([]*structs.Allocation{alloc}); err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -233,6 +235,7 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_All(t *testing.T) {
 	statsCollector.inodePercents = []float64{0, 0, 0}
 
 	alloc := mock.Alloc()
+	alloc.Resources.DiskMB = 150
 	if err := gc.MakeRoomFor([]*structs.Allocation{alloc}); err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -260,6 +263,7 @@ func TestAllocGarbageCollector_MakeRoomForAllocations_GC_Fallback(t *testing.T)
 	}
 
 	alloc := mock.Alloc()
+	alloc.Resources.DiskMB = 150
 	if err := gc.MakeRoomFor([]*structs.Allocation{alloc}); err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -297,6 +301,14 @@ func TestAllocGarbageCollector_UsageBelowThreshold(t *testing.T) {
 	if err := gc.keepUsageBelowThreshold(); err != nil {
 		t.Fatalf("err: %v", err)
 	}
+
+	// We shouldn't GC any of the allocs since the used percent values are below
+	// threshold
+	for i := 0; i < 2; i++ {
+		if gcAlloc := gc.allocRunners.Pop(); gcAlloc == nil {
+			t.Fatalf("err: %v", gcAlloc)
+		}
+	}
 }
 
 func TestAllocGarbageCollector_UsedPercentThreshold(t *testing.T) {
@@ -322,4 +334,14 @@ func TestAllocGarbageCollector_UsedPercentThreshold(t *testing.T) {
 	if err := gc.keepUsageBelowThreshold(); err != nil {
 		t.Fatalf("err: %v", err)
 	}
+
+	// We should be GC-ing only one of the alloc runners since the second time
+	// used percent returns a number below threshold.
+	if gcAlloc := gc.allocRunners.Pop(); gcAlloc == nil {
+		t.Fatalf("err: %v", gcAlloc)
+	}
+
+	if gcAlloc := gc.allocRunners.Pop(); gcAlloc != nil {
+		t.Fatalf("gcAlloc: %v", gcAlloc)
+	}
 }

client/stats/host.go

@@ -54,6 +54,8 @@ type DiskStats struct {
 	InodesUsedPercent float64
 }
 
+// NodeStatsCollector is an interface which is used for the puproses of mocking
+// the HostStatsCollector in the tests
 type NodeStatsCollector interface {
 	Collect() error
 	Stats() *HostStats
@@ -70,7 +72,9 @@ type HostStatsCollector struct {
 	allocDir        string
 }
 
-// NewHostStatsCollector returns a HostStatsCollector
+// NewHostStatsCollector returns a HostStatsCollector. The allocDir is passed in
+// so that we can present the disk related statistics for the mountpoint where
+// the allocation directory lives
 func NewHostStatsCollector(logger *log.Logger, allocDir string) *HostStatsCollector {
 	numCores := runtime.NumCPU()
 	statsCalculator := make(map[string]*HostCpuStatsCalculator)
@@ -138,6 +142,7 @@ func (h *HostStatsCollector) Collect() error {
 	}
 	hs.DiskStats = diskStats
 
+	// Getting the disk stats for the allocation directory
 	usage, err := disk.Usage(h.allocDir)
 	if err != nil {
 		return err

command/agent/alloc_endpoint.go

@@ -90,13 +90,11 @@ func (s *HTTPServer) ClientGCRequest(resp http.ResponseWriter, req *http.Request
 	if s.agent.client == nil {
 		return nil, clientNotRunning
 	}
-	err := s.agent.Client().CollectAllAllocs()
-	return nil, err
+	return nil, s.agent.Client().CollectAllAllocs()
 }
 
 func (s *HTTPServer) allocGC(allocID string, resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	err := s.agent.Client().CollectAllocation(allocID)
-	return nil, err
+	return nil, s.agent.Client().CollectAllocation(allocID)
 }
 
 func (s *HTTPServer) allocSnapshot(allocID string, resp http.ResponseWriter, req *http.Request) (interface{}, error) {