Fixed bug where some user provided values that can be set to 0 were s… (#714)

* Fixed bug where some user provided values that can be set to 0 were set to default value

* Addressed comments

* Fixed unit test for file cache empty config

* Allow for cli params to be set to 0

* Fixed defaults and merged isSet

* Added test

Co-authored-by: Gauri Prasad <gapra@microsoft.com>
This commit is contained in:
Gauri Prasad 2022-03-07 10:23:52 -08:00 коммит произвёл GitHub
Родитель 827506595a
Коммит dd401750a3
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
11 изменённых файлов: 164 добавлений и 53 удалений

Просмотреть файл

@ -105,6 +105,7 @@ func (opt *mountOptions) validate(skipEmptyMount bool) error {
}
}
// A user provided value of 0 doesnt make sense for MaxLogFileSize or LogFileCount.
if opt.Logging.MaxLogFileSize == 0 {
opt.Logging.MaxLogFileSize = common.DefaultMaxLogFileSize
}
@ -221,6 +222,18 @@ var mountCmd = &cobra.Command{
os.Exit(1)
}
if !config.IsSet("config-file") {
options.ConfigFile = "config.yaml"
}
if !config.IsSet("logging.file-path") {
options.Logging.LogFilePath = common.DefaultLogFilePath
}
if !config.IsSet("logging.log-level") {
options.Logging.LogLevel = "LOG_WARNING"
}
err = options.validate(false)
if err != nil {
fmt.Printf("Mount: error invalid options [%v]", err)

Просмотреть файл

@ -216,22 +216,21 @@ func UnmarshalKey(key string, obj interface{}) error {
if err != nil {
return fmt.Errorf("config error: unmarshalling [%v]", err)
}
userOptions.envTree.MergeWithKey(key, obj, func(val interface{}) (interface{}, interface{}, bool) {
userOptions.envTree.MergeWithKey(key, obj, func(val interface{}) (interface{}, bool) {
envVar := val.(string)
res, ok := os.LookupEnv(envVar)
if ok {
return res, "", true
} else if res == "" {
return "", "", false
return res, true
} else {
return "", false
}
return "", "", false
})
userOptions.flagTree.MergeWithKey(key, obj, func(val interface{}) (interface{}, interface{}, bool) {
userOptions.flagTree.MergeWithKey(key, obj, func(val interface{}) (interface{}, bool) {
flag := val.(*pflag.Flag)
if flag.Changed {
return flag.Value.String(), flag.DefValue, true
return flag.Value.String(), true
} else {
return "", flag.DefValue, true
return "", false
}
})
return nil
@ -244,22 +243,21 @@ func Unmarshal(obj interface{}) error {
if err != nil {
return fmt.Errorf("config error: unmarshalling [%v]", err)
}
userOptions.envTree.Merge(obj, func(val interface{}) (interface{}, interface{}, bool) {
userOptions.envTree.Merge(obj, func(val interface{}) (interface{}, bool) {
envVar := val.(string)
res, ok := os.LookupEnv(envVar)
if ok {
return res, "", true
} else if res == "" {
return "", "", false
return res, true
} else {
return "", false
}
return "", "", false
})
userOptions.flagTree.Merge(obj, func(val interface{}) (interface{}, interface{}, bool) {
userOptions.flagTree.Merge(obj, func(val interface{}) (interface{}, bool) {
flag := val.(*pflag.Flag)
if flag.Changed {
return flag.Value.String(), flag.DefValue, true
return flag.Value.String(), true
} else {
return "", flag.DefValue, true
return "", false
}
})
@ -275,7 +273,18 @@ func SetBool(key string, val bool) {
}
func IsSet(key string) bool {
return viper.IsSet(key)
if viper.IsSet(key) {
return true
}
pieces := strings.Split(key, ".")
node := userOptions.flagTree.head
for _, s := range pieces {
node = node.children[s]
if node == nil {
return false
}
}
return node.value.(*pflag.Flag).Changed
}
//AttachToFlagSet is used to attach the flags in config to the cmd flags

Просмотреть файл

@ -220,7 +220,7 @@ func parseValue(val string, toType reflect.Kind) interface{} {
//MergeWithKey is used to merge the contained tree with the object (obj) that is passed in as parameter.
//getValue parameter is a function that accepts the value stored in a TreeNode and performs any business logic and returns the value that has to be placed in the obj parameter
//it must also return true|false based on which the value wil be set in the obj parameter.
func (tree *Tree) MergeWithKey(key string, obj interface{}, getValue func(val interface{}) (res interface{}, defVal interface{}, ok bool)) {
func (tree *Tree) MergeWithKey(key string, obj interface{}, getValue func(val interface{}) (res interface{}, ok bool)) {
subTree := tree.GetSubTree(key)
if subTree == nil {
return
@ -241,30 +241,25 @@ func (tree *Tree) MergeWithKey(key string, obj interface{}, getValue func(val in
} else if elem.Field(i).Type().Kind() == reflect.Ptr {
subKey := key + "." + idx
tree.MergeWithKey(subKey, elem.Field(i).Elem().Addr().Interface(), getValue)
} else {
val, def, ok := getValue(subTree.children[idx].value)
val, ok := getValue(subTree.children[idx].value)
if ok {
if reflect.ValueOf(val).IsZero() && elem.Field(i).IsZero() {
assignToField(elem.Field(i), def)
} else if !reflect.ValueOf(val).IsZero() {
assignToField(elem.Field(i), val)
}
assignToField(elem.Field(i), val)
}
}
}
}
} else if isPrimitiveType(elem.Type().Kind()) {
val, def, ok := getValue(subTree.value)
val, ok := getValue(subTree.value)
if ok {
assignToField(elem, val)
} else if elem.IsZero() {
assignToField(elem, def)
}
}
}
//Merge performs the same function as MergeWithKey but at the root level
func (tree *Tree) Merge(obj interface{}, getValue func(val interface{}) (res interface{}, defVal interface{}, ok bool)) {
func (tree *Tree) Merge(obj interface{}, getValue func(val interface{}) (res interface{}, ok bool)) {
subTree := tree.head
if subTree == nil {
return
@ -286,23 +281,17 @@ func (tree *Tree) Merge(obj interface{}, getValue func(val interface{}) (res int
subKey := idx
tree.MergeWithKey(subKey, elem.Field(i).Elem().Addr().Interface(), getValue)
} else {
val, def, ok := getValue(subTree.children[idx].value)
val, ok := getValue(subTree.children[idx].value)
if ok {
if reflect.ValueOf(val).IsZero() && elem.Field(i).IsZero() {
assignToField(elem.Field(i), def)
} else {
assignToField(elem.Field(i), val)
}
assignToField(elem.Field(i), val)
}
}
}
}
} else if isPrimitiveType(elem.Type().Kind()) {
val, def, ok := getValue(subTree.value)
val, ok := getValue(subTree.value)
if ok {
assignToField(elem, val)
} else if elem.IsZero() {
assignToField(elem, def)
}
}
}

Просмотреть файл

@ -123,7 +123,7 @@ func (ac *AttrCache) Configure() error {
return fmt.Errorf("config error in %s [%s]", ac.Name(), err.Error())
}
if conf.Timeout != 0 {
if config.IsSet(compName + ".timeout-sec") {
ac.cacheTimeout = conf.Timeout
} else {
ac.cacheTimeout = defaultAttrCacheTimeout
@ -521,7 +521,7 @@ func NewAttrCacheComponent() internal.Component {
// On init register this component to pipeline and supply your constructor
func init() {
internal.AddComponent(compName, NewAttrCacheComponent)
attrCacheTimeout := config.AddInt32Flag("attr-cache-timeout", int32(defaultAttrCacheTimeout), "attribute cache timeout")
attrCacheTimeout := config.AddUint32Flag("attr-cache-timeout", defaultAttrCacheTimeout, "attribute cache timeout")
config.BindPFlag(compName+".timeout-sec", attrCacheTimeout)
noSymlinks := config.AddBoolFlag("no-symlinks", false, "whether or not symlinks should be supported")
config.BindPFlag(compName+".no-symlinks", noSymlinks)

Просмотреть файл

@ -222,6 +222,18 @@ func (suite *attrCacheTestSuite) TestConfig() {
suite.assert.Equal(suite.attrCache.noSymlinks, true)
}
func (suite *attrCacheTestSuite) TestConfigZero() {
defer suite.cleanupTest()
suite.cleanupTest() // clean up the default attr cache generated
config := "attr_cache:\n timeout-sec: 0\n no-cache-on-list: true\n no-symlinks: true"
suite.setupTestHelper(config) // setup a new attr cache with a custom config (clean up will occur after the test as usual)
suite.assert.Equal(suite.attrCache.Name(), "attr_cache")
suite.assert.EqualValues(suite.attrCache.cacheTimeout, 0)
suite.assert.Equal(suite.attrCache.cacheOnList, false)
suite.assert.Equal(suite.attrCache.noSymlinks, true)
}
// Tests Create Directory
func (suite *attrCacheTestSuite) TestCreateDir() {
defer suite.cleanupTest()

Просмотреть файл

@ -336,7 +336,8 @@ func ParseAndValidateConfig(az *AzStorage, opt AzStorageOptions) error {
return errors.New("invalid auth mode")
}
// Retry plicy configuration
// Retry policy configuration
// A user provided value of 0 doesnt make sense for MaxRetries, MaxTimeout, BackoffTime, or MaxRetryDelay.
az.stConfig.maxRetries = 3
az.stConfig.maxTimeout = 3600
az.stConfig.backoffTime = 1
@ -369,6 +370,7 @@ func ParseAndReadDynamicConfig(az *AzStorage, opt AzStorageOptions, reload bool)
log.Trace("ParseAndReadDynamicConfig : Reparsing config")
// If block size and max concurrency is configured use those
// A user provided value of 0 doesnt make sense for BlockSize, or MaxConcurrency.
if opt.BlockSize != 0 {
az.stConfig.blockSize = opt.BlockSize * 1024 * 1024
}

Просмотреть файл

@ -96,10 +96,11 @@ type FileCacheOptions struct {
}
const (
compName = "file_cache"
defaultMaxEviction = 5000
defaultMaxThreshold = 80
defaultMinThreshold = 60
compName = "file_cache"
defaultMaxEviction = 5000
defaultMaxThreshold = 80
defaultMinThreshold = 60
defaultFileCacheTimeout = 120
)
// Verification to check satisfaction criteria with Component Interface
@ -179,7 +180,11 @@ func (c *FileCache) Configure() error {
}
c.createEmptyFile = conf.CreateEmptyFile
c.cacheTimeout = float64(conf.Timeout)
if config.IsSet(compName + ".timeout-sec") {
c.cacheTimeout = float64(conf.Timeout)
} else {
c.cacheTimeout = float64(defaultFileCacheTimeout)
}
c.allowNonEmpty = conf.AllowNonEmpty
c.cleanupOnStart = conf.CleanupOnStart
c.policyTrace = conf.EnablePolicyTrace
@ -264,6 +269,7 @@ func (c *FileCache) OnConfigChange() {
}
func (c *FileCache) GetPolicyConfig(conf FileCacheOptions) cachePolicyConfig {
// A user provided value of 0 doesnt make sense for MaxEviction, HighThreshold or LowThreshold.
if conf.MaxEviction == 0 {
conf.MaxEviction = defaultMaxEviction
}
@ -1294,6 +1300,8 @@ func NewFileCacheComponent() internal.Component {
// On init register this component to pipeline and supply your constructor
func init() {
internal.AddComponent(compName, NewFileCacheComponent)
fileCacheTimeout := config.AddUint32Flag("file-cache-timeout", defaultFileCacheTimeout, "file cache timeout")
config.BindPFlag(compName+".timeout-sec", fileCacheTimeout)
tmpPathFlag := config.AddStringFlag("tmp-path", "", "Configures the tmp location for the cache. Configure the fastest disk (SSD or ramdisk) for best performance.")
config.BindPFlag(compName+".path", tmpPathFlag)
config.RegisterFlagCompletionFunc("tmp-path", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {

Просмотреть файл

@ -152,7 +152,7 @@ func (suite *fileCacheTestSuite) TestEmpty() {
suite.assert.Equal(suite.fileCache.createEmptyFile, false)
suite.assert.Equal(suite.fileCache.allowNonEmpty, false)
suite.assert.EqualValues(suite.fileCache.cacheTimeout, 0)
suite.assert.EqualValues(suite.fileCache.cacheTimeout, 120)
suite.assert.Equal(suite.fileCache.cleanupOnStart, false)
}
@ -188,6 +188,37 @@ func (suite *fileCacheTestSuite) TestConfig() {
suite.assert.Equal(suite.fileCache.cleanupOnStart, cleanupOnStart)
}
func (suite *fileCacheTestSuite) TestConfigZero() {
defer suite.cleanupTest()
suite.cleanupTest() // teardown the default file cache generated
policy := "lfu"
maxSizeMb := 1024
cacheTimeout := 0
maxDeletion := 10
highThreshold := 90
lowThreshold := 10
createEmptyFile := true
allowNonEmptyTemp := true
cleanupOnStart := true
config := fmt.Sprintf("file_cache:\n path: %s\n policy: %s\n max-size-mb: %d\n timeout-sec: %d\n max-eviction: %d\n high-threshold: %d\n low-threshold: %d\n create-empty-file: %t\n allow-non-empty-temp: %t\n cleanup-on-start: %t",
suite.cache_path, policy, maxSizeMb, cacheTimeout, maxDeletion, highThreshold, lowThreshold, createEmptyFile, allowNonEmptyTemp, cleanupOnStart)
suite.setupTestHelper(config) // setup a new file cache with a custom config (teardown will occur after the test as usual)
suite.assert.Equal(suite.fileCache.Name(), "file_cache")
suite.assert.Equal(suite.fileCache.tmpPath, suite.cache_path)
suite.assert.Equal(suite.fileCache.policy.Name(), policy)
suite.assert.EqualValues(suite.fileCache.policy.(*lfuPolicy).maxSizeMB, maxSizeMb)
suite.assert.EqualValues(suite.fileCache.policy.(*lfuPolicy).maxEviction, maxDeletion)
suite.assert.EqualValues(suite.fileCache.policy.(*lfuPolicy).highThreshold, highThreshold)
suite.assert.EqualValues(suite.fileCache.policy.(*lfuPolicy).lowThreshold, lowThreshold)
suite.assert.Equal(suite.fileCache.createEmptyFile, createEmptyFile)
suite.assert.Equal(suite.fileCache.allowNonEmpty, allowNonEmptyTemp)
suite.assert.EqualValues(suite.fileCache.cacheTimeout, cacheTimeout)
suite.assert.Equal(suite.fileCache.cleanupOnStart, cleanupOnStart)
}
// Tests CreateDir
func (suite *fileCacheTestSuite) TestCreateDir() {
defer suite.cleanupTest()

Просмотреть файл

@ -161,19 +161,19 @@ func (lf *Libfuse) Validate(opt *LibfuseOptions) error {
}
}
if config.IsSet(compName + ".entry-expiration") {
if config.IsSet(compName + ".entry-expiration-sec") {
lf.entryExpiration = opt.EntryExpiration
} else {
lf.entryExpiration = defaultEntryExpiration
}
if config.IsSet(compName + ".attribute-expiration") {
if config.IsSet(compName + ".attribute-expiration-sec") {
lf.attributeExpiration = opt.AttributeExpiration
} else {
lf.attributeExpiration = defaultAttrExpiration
}
if config.IsSet(compName + ".negativeTimeout") {
if config.IsSet(compName + ".negative-entry-expiration-sec") {
lf.negativeTimeout = opt.NegativeEntryExpiration
} else {
lf.negativeTimeout = defaultNegativeEntryExpiration

Просмотреть файл

@ -34,6 +34,8 @@
package libfuse
import (
"blobfuse2/common"
"io/fs"
"testing"
"github.com/stretchr/testify/suite"
@ -43,6 +45,51 @@ import (
func (suite *libfuseTestSuite) TestDefault() {
defer suite.cleanupTest()
suite.assert.Equal(suite.libfuse.Name(), "libfuse")
suite.assert.Empty(suite.libfuse.mountPath)
suite.assert.False(suite.libfuse.readOnly)
suite.assert.False(suite.libfuse.traceEnable)
suite.assert.False(suite.libfuse.allowOther)
suite.assert.Equal(suite.libfuse.dirPermission, uint(common.DefaultDirectoryPermissionBits))
suite.assert.Equal(suite.libfuse.filePermission, uint(common.DefaultFilePermissionBits))
suite.assert.Equal(suite.libfuse.entryExpiration, uint32(120))
suite.assert.Equal(suite.libfuse.attributeExpiration, uint32(120))
suite.assert.Equal(suite.libfuse.negativeTimeout, uint32(120))
}
func (suite *libfuseTestSuite) TestConfig() {
defer suite.cleanupTest()
suite.cleanupTest() // clean up the default libfuse generated
config := "allow-other: true\nread-only: true\nlibfuse:\n default-permission: 0777\n attribute-expiration-sec: 60\n entry-expiration-sec: 60\n negative-entry-expiration-sec: 60\n fuse-trace: true\n"
suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
suite.assert.Equal(suite.libfuse.Name(), "libfuse")
suite.assert.Empty(suite.libfuse.mountPath)
suite.assert.True(suite.libfuse.readOnly)
suite.assert.True(suite.libfuse.traceEnable)
suite.assert.True(suite.libfuse.allowOther)
suite.assert.Equal(suite.libfuse.dirPermission, uint(fs.FileMode(0777)))
suite.assert.Equal(suite.libfuse.filePermission, uint(fs.FileMode(0777)))
suite.assert.Equal(suite.libfuse.entryExpiration, uint32(60))
suite.assert.Equal(suite.libfuse.attributeExpiration, uint32(60))
suite.assert.Equal(suite.libfuse.negativeTimeout, uint32(60))
}
func (suite *libfuseTestSuite) TestConfigZero() {
defer suite.cleanupTest()
suite.cleanupTest() // clean up the default libfuse generated
config := "allow-other: true\nread-only: true\nlibfuse:\n default-permission: 0777\n attribute-expiration-sec: 0\n entry-expiration-sec: 0\n negative-entry-expiration-sec: 0\n fuse-trace: true\n"
suite.setupTestHelper(config) // setup a new libfuse with a custom config (clean up will occur after the test as usual)
suite.assert.Equal(suite.libfuse.Name(), "libfuse")
suite.assert.Empty(suite.libfuse.mountPath)
suite.assert.True(suite.libfuse.readOnly)
suite.assert.True(suite.libfuse.traceEnable)
suite.assert.True(suite.libfuse.allowOther)
suite.assert.Equal(suite.libfuse.dirPermission, uint(fs.FileMode(0777)))
suite.assert.Equal(suite.libfuse.filePermission, uint(fs.FileMode(0777)))
suite.assert.Equal(suite.libfuse.entryExpiration, uint32(0))
suite.assert.Equal(suite.libfuse.attributeExpiration, uint32(0))
suite.assert.Equal(suite.libfuse.negativeTimeout, uint32(0))
}
// getattr

Просмотреть файл

@ -26,9 +26,9 @@ components:
# Libfuse configuration
libfuse:
default-permission: 0777|0666|0644|0444 <default permissions to be presented for block blobs>
attribute-expiration-sec: <time kernel can cache inode attributes (in sec)>
entry-expiration-sec: <time kernel can cache directory listing attributes (in sec)>
negative-entry-expiration-sec: <time kernel can cache attributes of non existant paths (in sec)>
attribute-expiration-sec: <time kernel can cache inode attributes (in sec). Default - 120 sec>
entry-expiration-sec: <time kernel can cache directory listing attributes (in sec). Default - 120 sec>
negative-entry-expiration-sec: <time kernel can cache attributes of non existant paths (in sec). Default - 120 sec>
fuse-trace: true|false <enable libfuse api trace logs for debugging>
extension: <physical path to extension library>
@ -52,7 +52,7 @@ file_cache:
# Optional
policy: lru|lfu <eviction policy to be engaged for cache eviction. lru = least recently used file to be delted, lfu = least frequently used file to be deleted. Default - lru>
timeout-sec: <default cache eviction timeout (in sec). Default - 0 sec>
timeout-sec: <default cache eviction timeout (in sec). Default - 120 sec>
max-eviction: <number of files that can be evicted at once. Default - 5000>
max-size-mb: <maximum cache size allowed. Default - 0 (unlimited)>
high-threshold: <% disk space consumed which triggers eviction. This parameter overrides 'timeout-sec' parameter and cached files will be removed even if they have not expired. Default - 80>