Remove globalMaxCacheSize and globalCacheExpiry variables (#3826)

This patch fixes below

* Remove global variables globalMaxCacheSize and globalCacheExpiry.
* Make global variables into constant in objcache package.

cmd/globals.go

@@ -26,7 +26,6 @@ import (
 	"github.com/fatih/color"
 	"github.com/minio/cli"
 	"github.com/minio/mc/pkg/console"
-	"github.com/minio/minio/pkg/objcache"
 )
 
 // minio configuration related constants.
@@ -78,16 +77,9 @@ var (
 	// Set to true if credentials were passed from env, default is false.
 	globalIsEnvCreds = false
 
-	// Maximum cache size. Defaults to disabled.
-	// Caching is enabled only for RAM size > 8GiB.
-	globalMaxCacheSize = uint64(0)
-
 	// Maximum size of internal objects parts
 	globalPutPartSize = int64(64 * 1024 * 1024)
 
-	// Cache expiry.
-	globalCacheExpiry = objcache.DefaultExpiry
-
 	// Minio local server address (in `host:port` format)
 	globalMinioAddr = ""
 	// Minio default port, can be changed through command line.

cmd/server-main.go

@@ -138,16 +138,8 @@ func initServerConfig(c *cli.Context) {
 	// Load user supplied root CAs
 	loadRootCAs()
 
-	// Set maxOpenFiles, This is necessary since default operating
+	// Set system resources to maximum.
-	// system limits of 1024, 2048 are not enough for Minio server.
+	errorIf(setMaxResources(), "Unable to change resource limit")
-	setMaxOpenFiles()
-
-	// Set maxMemory, This is necessary since default operating
-	// system limits might be changed and we need to make sure we
-	// do not crash the server so the set the maxCacheSize appropriately.
-	setMaxMemory()
-
-	// Do not fail if this is not allowed, lower limits are fine as well.
 }
 
 // Validate if input disks are sufficient for initializing XL.

cmd/server-rlimit.go

@@ -16,17 +16,27 @@
 
 package cmd
 
-import (
+import "github.com/minio/minio/pkg/sys"
-	"github.com/minio/minio/pkg/sys"
-)
 
-func setMaxOpenFiles() error {
+func setMaxResources() (err error) {
-	_, maxLimit, err := sys.GetMaxOpenFileLimit()
+	var maxLimit uint64
-	if err != nil {
+
+	// Set open files limit to maximum.
+	if _, maxLimit, err = sys.GetMaxOpenFileLimit(); err != nil {
 		return err
 	}
 
-	return sys.SetMaxOpenFileLimit(maxLimit, maxLimit)
+	if err = sys.SetMaxOpenFileLimit(maxLimit, maxLimit); err != nil {
+		return err
+	}
+
+	// Set max memory limit as current memory limit.
+	if _, maxLimit, err = sys.GetMaxMemoryLimit(); err != nil {
+		return err
+	}
+
+	err = sys.SetMaxMemoryLimit(maxLimit, maxLimit)
+	return err
 }
 
 func getMaxCacheSize(curLimit, totalRAM uint64) (cacheSize uint64) {
@@ -45,29 +55,25 @@ func getMaxCacheSize(curLimit, totalRAM uint64) (cacheSize uint64) {
 	return cacheSize
 }
 
-func setMaxMemory() error {
+// GetMaxCacheSize returns maximum cache size based on current RAM size and memory limit.
+func GetMaxCacheSize() (cacheSize uint64, err error) {
 	// Get max memory limit
-	_, maxLimit, err := sys.GetMaxMemoryLimit()
+	var curLimit uint64
-	if err != nil {
+	if curLimit, _, err = sys.GetMaxMemoryLimit(); err != nil {
-		return err
+		return cacheSize, err
-	}
-
-	// Set max memory limit as current memory limit.
-	if err = sys.SetMaxMemoryLimit(maxLimit, maxLimit); err != nil {
-		return err
 	}
 
 	// Get total RAM.
-	stats, err := sys.GetStats()
+	var stats sys.Stats
-	if err != nil {
+	if stats, err = sys.GetStats(); err != nil {
-		return err
+		return cacheSize, err
 	}
 
 	// In some OS like windows, maxLimit is zero.  Set total RAM as maxLimit.
-	if maxLimit == 0 {
+	if curLimit == 0 {
-		maxLimit = stats.TotalRAM
+		curLimit = stats.TotalRAM
 	}
 
-	globalMaxCacheSize = getMaxCacheSize(maxLimit, stats.TotalRAM)
+	cacheSize = getMaxCacheSize(curLimit, stats.TotalRAM)
-	return nil
+	return cacheSize, err
 }

cmd/test-utils_test.go

@@ -65,8 +65,8 @@ func init() {
 	// Disable printing console messages during tests.
 	color.Output = ioutil.Discard
 
-	// Enable caching.
+	// Set system resources to maximum.
-	setMaxMemory()
+	setMaxResources()
 }
 
 func prepareFS() (ObjectLayer, string, error) {

cmd/xl-v1.go

@@ -127,14 +127,23 @@ func newXLObjects(storageDisks []StorageAPI) (ObjectLayer, error) {
 		listPool:     listPool,
 	}
 
-	// Object cache is enabled when _MINIO_CACHE env is missing.
+	// Get cache size if _MINIO_CACHE environment variable is set.
-	// and cache size is > 0.
+	var maxCacheSize uint64
-	xl.objCacheEnabled = !objCacheDisabled && globalMaxCacheSize > 0
+	if !objCacheDisabled {
+		maxCacheSize, err = GetMaxCacheSize()
+		errorIf(err, "Unable to get maximum cache size")
+
+		// Enable object cache if cache size is more than zero
+		xl.objCacheEnabled = maxCacheSize > 0
+	}
 
 	// Check if object cache is enabled.
 	if xl.objCacheEnabled {
 		// Initialize object cache.
-		objCache := objcache.New(globalMaxCacheSize, globalCacheExpiry)
+		objCache, err := objcache.New(maxCacheSize, objcache.DefaultExpiry)
+		if err != nil {
+			return nil, err
+		}
 		objCache.OnEviction = func(key string) {
 			debug.FreeOSMemory()
 		}

pkg/objcache/README.md

@@ -6,22 +6,24 @@ package objcache
 
     Package objcache implements in memory caching methods.
 
+CONSTANTS
+
+    const (
+    	// NoExpiry represents caches to be permanent and can only be deleted.
+    	NoExpiry = time.Duration(0)
+
+    	// DefaultExpiry represents three days time duration when individual entries will be expired.
+    	DefaultExpiry = time.Duration(3 * 24 * time.Hour)
+    )
+
 VARIABLES
 
-var DefaultExpiry = time.Duration(72 * time.Hour) // 72hrs.
-
-    DefaultExpiry represents default time duration value when individual
-    entries will be expired.
-
 var ErrCacheFull = errors.New("Not enough space in cache")
     ErrCacheFull - cache is full.
 
 var ErrKeyNotFoundInCache = errors.New("Key not found in cache")
     ErrKeyNotFoundInCache - key not found in cache.
 
-var NoExpiry = time.Duration(0)
-    NoExpiry represents caches to be permanent and can only be deleted.
-
 TYPES
 
 type Cache struct {

pkg/objcache/objcache.go

@@ -1,5 +1,5 @@
 /*
- * Minio Cloud Storage, (C) 2016 Minio, Inc.
+ * Minio Cloud Storage, (C) 2016, 2017 Minio, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,18 +27,20 @@ import (
 	"time"
 )
 
-// NoExpiry represents caches to be permanent and can only be deleted.
+const (
-var NoExpiry = time.Duration(0)
+	// NoExpiry represents caches to be permanent and can only be deleted.
+	NoExpiry = time.Duration(0)
 
-// DefaultExpiry represents default time duration value when individual entries will be expired.
+	// DefaultExpiry represents three days time duration when individual entries will be expired.
-var DefaultExpiry = time.Duration(72 * time.Hour) // 72hrs.
+	DefaultExpiry = time.Duration(3 * 24 * time.Hour)
 
-// DefaultBufferRatio represents default ratio used to calculate the
+	// defaultBufferRatio represents default ratio used to calculate the
-// individual cache entry buffer size.
+	// individual cache entry buffer size.
-var DefaultBufferRatio = uint64(10)
+	defaultBufferRatio = uint64(10)
 
-// DefaultGCPercent represents default garbage collection target percentage.
+	// defaultGCPercent represents default garbage collection target percentage.
-var DefaultGCPercent = 20
+	defaultGCPercent = 20
+)
 
 // buffer represents the in memory cache of a single entry.
 // buffer carries value of the data and last accessed time.
@@ -87,9 +89,10 @@ type Cache struct {
 // duration. If the expiry duration is less than one
 // (or NoExpiry), the items in the cache never expire
 // (by default), and must be deleted manually.
-func New(maxSize uint64, expiry time.Duration) *Cache {
+func New(maxSize uint64, expiry time.Duration) (c *Cache, err error) {
 	if maxSize == 0 {
-		panic("objcache: setting maximum cache size to zero is forbidden.")
+		err = errors.New("invalid maximum cache size")
+		return c, err
 	}
 
 	// A garbage collection is triggered when the ratio
@@ -106,20 +109,20 @@ func New(maxSize uint64, expiry time.Duration) *Cache {
 	// when we get to 8M.
 	//
 	// Set this value to 20% if caching is enabled.
-	debug.SetGCPercent(DefaultGCPercent)
+	debug.SetGCPercent(defaultGCPercent)
 
 	// Max cache entry size - indicates the
 	// maximum buffer per key that can be held in
 	// memory. Currently this value is 1/10th
 	// the size of requested cache size.
 	maxCacheEntrySize := func() uint64 {
-		i := maxSize / DefaultBufferRatio
+		i := maxSize / defaultBufferRatio
 		if i == 0 {
 			i = maxSize
 		}
 		return i
 	}()
-	c := &Cache{
+	c = &Cache{
 		onceGC:            sync.Once{},
 		maxSize:           maxSize,
 		maxCacheEntrySize: maxCacheEntrySize,
@@ -134,7 +137,7 @@ func New(maxSize uint64, expiry time.Duration) *Cache {
 		// Start garbage collection routine to expire objects.
 		c.StartGC()
 	}
-	return c
+	return c, nil
 }
 
 // ErrKeyNotFoundInCache - key not found in cache.
@@ -177,7 +180,7 @@ func (c *Cache) Create(key string, size int64) (w io.WriteCloser, err error) {
 	// Change GC percent if the current cache usage
 	// is already 75% of the maximum allowed usage.
 	if c.currentSize > (75 * c.maxSize / 100) {
-		c.onceGC.Do(func() { debug.SetGCPercent(DefaultGCPercent - 10) })
+		c.onceGC.Do(func() { debug.SetGCPercent(defaultGCPercent - 10) })
 	}
 	c.mutex.Unlock()
 

pkg/objcache/objcache_test.go

@@ -43,7 +43,11 @@ func TestObjExpiry(t *testing.T) {
 
 	// Test case 1 validates running of GC.
 	testCase := testCases[0]
-	cache := New(testCase.cacheSize, testCase.expiry)
+	cache, err := New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	cache.OnEviction = func(key string) {}
 	w, err := cache.Create("test", 1)
 	if err != nil {
@@ -126,15 +130,23 @@ func TestObjCache(t *testing.T) {
 
 	// Test 1 validating Open failure.
 	testCase := testCases[0]
-	cache := New(testCase.cacheSize, testCase.expiry)
+	cache, err := New(testCase.cacheSize, testCase.expiry)
-	_, err := cache.Open("test", fakeObjModTime)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
+	_, err = cache.Open("test", fakeObjModTime)
 	if testCase.err != err {
 		t.Errorf("Test case 2 expected to pass, failed instead %s", err)
 	}
 
 	// Test 2 validating Create failure.
 	testCase = testCases[1]
-	cache = New(testCase.cacheSize, testCase.expiry)
+	cache, err = New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	_, err = cache.Create("test", 2)
 	if testCase.err != err {
 		t.Errorf("Test case 2 expected to pass, failed instead %s", err)
@@ -144,7 +156,11 @@ func TestObjCache(t *testing.T) {
 	// Subsequently we Close() without writing any data, to receive
 	// `io.ErrShortBuffer`
 	testCase = testCases[2]
-	cache = New(testCase.cacheSize, testCase.expiry)
+	cache, err = New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	w, err := cache.Create("test", 1)
 	if testCase.err != err {
 		t.Errorf("Test case 3 expected to pass, failed instead %s", err)
@@ -156,7 +172,11 @@ func TestObjCache(t *testing.T) {
 	// Test 4 validates Create and Close succeeds successfully caching
 	// the writes.
 	testCase = testCases[3]
-	cache = New(testCase.cacheSize, testCase.expiry)
+	cache, err = New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	w, err = cache.Create("test", 5)
 	if testCase.err != err {
 		t.Errorf("Test case 4 expected to pass, failed instead %s", err)
@@ -184,7 +204,11 @@ func TestObjCache(t *testing.T) {
 
 	// Test 5 validates Delete succeeds and Open fails with err
 	testCase = testCases[4]
-	cache = New(testCase.cacheSize, testCase.expiry)
+	cache, err = New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	w, err = cache.Create("test", 5)
 	if err != nil {
 		t.Errorf("Test case 5 expected to pass, failed instead %s", err)
@@ -204,7 +228,11 @@ func TestObjCache(t *testing.T) {
 
 	// Test 6 validates OnEviction being called upon Delete is being invoked.
 	testCase = testCases[5]
-	cache = New(testCase.cacheSize, testCase.expiry)
+	cache, err = New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	w, err = cache.Create("test", 5)
 	if err != nil {
 		t.Errorf("Test case 6 expected to pass, failed instead %s", err)
@@ -227,7 +255,11 @@ func TestObjCache(t *testing.T) {
 
 	// Test 7 validates rejecting requests when excess data is being saved.
 	testCase = testCases[6]
-	cache = New(testCase.cacheSize, testCase.expiry)
+	cache, err = New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	w, err = cache.Create("test1", 5)
 	if err != nil {
 		t.Errorf("Test case 7 expected to pass, failed instead %s", err)
@@ -245,7 +277,11 @@ func TestObjCache(t *testing.T) {
 
 	// Test 8 validates rejecting Writes which write excess data.
 	testCase = testCases[7]
-	cache = New(testCase.cacheSize, testCase.expiry)
+	cache, err = New(testCase.cacheSize, testCase.expiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	w, err = cache.Create("test1", 5)
 	if err != nil {
 		t.Errorf("Test case 8 expected to pass, failed instead %s", err)
@@ -267,7 +303,11 @@ func TestObjCache(t *testing.T) {
 
 // TestStateEntryPurge - tests if objCache purges stale entry and returns ErrKeyNotFoundInCache.
 func TestStaleEntryPurge(t *testing.T) {
-	cache := New(1024, NoExpiry)
+	cache, err := New(1024, NoExpiry)
+	if err != nil {
+		t.Fatalf("Unable to create new objcache")
+	}
+
 	w, err := cache.Create("test", 5)
 	if err != nil {
 		t.Errorf("Test case expected to pass, failed instead %s", err)