Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make default value of enable-hns flag true #2377

Merged
merged 12 commits into from
Aug 23, 2024
2 changes: 1 addition & 1 deletion cfg/config.go
Tulsishah marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ func BindFlags(v *viper.Viper, flagSet *pflag.FlagSet) error {
return err
}

flagSet.BoolP("enable-hns", "", false, "Enables support for HNS buckets")
flagSet.BoolP("enable-hns", "", true, "Enables support for HNS buckets")

if err := v.BindPFlag("enable-hns", flagSet.Lookup("enable-hns")); err != nil {
return err
Expand Down
5 changes: 0 additions & 5 deletions cfg/rationalize.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ func decodeURL(u string) (string, error) {

// Rationalize updates the config fields based on the values of other fields.
func Rationalize(c *Config) error {
// The EnableEmptyManagedFolders flag must be set to true to enforce folder prefixes for Hierarchical buckets.
if c.EnableHns {
c.List.EnableEmptyManagedFolders = true
Tulsishah marked this conversation as resolved.
Show resolved Hide resolved
}

var err error
if c.GcsConnection.CustomEndpoint, err = decodeURL(c.GcsConnection.CustomEndpoint); err != nil {
return err
Expand Down
48 changes: 0 additions & 48 deletions cfg/rationalize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,54 +20,6 @@ import (
"github.com/stretchr/testify/assert"
)

func TestRationalizeEnableEmptyManagedFolders(t *testing.T) {
testcases := []struct {
name string
enableHns bool
enableEmptyManagedFolders bool
expectedEnableEmptyManagedFolders bool
}{
{
name: "both enable-hns and enable-empty-managed-folders set to true",
enableHns: true,
enableEmptyManagedFolders: true,
expectedEnableEmptyManagedFolders: true,
},
{
name: "enable-hns set to true and enable-empty-managed-folders set to false",
enableHns: true,
enableEmptyManagedFolders: false,
expectedEnableEmptyManagedFolders: true,
},
{
name: "enable-hns set to false and enable-empty-managed-folders set to true",
enableHns: false,
enableEmptyManagedFolders: true,
expectedEnableEmptyManagedFolders: true,
},
{
name: "both enable-hns and enable-empty-managed-folders set to false",
enableHns: false,
enableEmptyManagedFolders: false,
expectedEnableEmptyManagedFolders: false,
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
c := Config{
EnableHns: tc.enableHns,
List: ListConfig{EnableEmptyManagedFolders: tc.enableEmptyManagedFolders},
}

err := Rationalize(&c)

if assert.NoError(t, err) {
assert.Equal(t, tc.expectedEnableEmptyManagedFolders, c.List.EnableEmptyManagedFolders)
}
})
}
}

func TestRationalizeCustomEndpointSuccessful(t *testing.T) {
testCases := []struct {
name string
Expand Down
59 changes: 0 additions & 59 deletions cmd/config_rationalization_test.go

This file was deleted.

4 changes: 2 additions & 2 deletions cmd/config_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -550,14 +550,14 @@ func TestValidateConfigFile_EnableHNSConfigSuccessful(t *testing.T) {
name: "empty_config_file",
configFile: "testdata/empty_file.yaml",
expectedConfig: &cfg.Config{
EnableHns: false,
EnableHns: true,
},
},
{
name: "valid_config_file",
configFile: "testdata/valid_config.yaml",
expectedConfig: &cfg.Config{
EnableHns: true,
EnableHns: false,
},
},
}
Expand Down
52 changes: 0 additions & 52 deletions cmd/legacy_param_mapper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -644,58 +644,6 @@ func TestLogSeverityRationalization(t *testing.T) {
}
}

func TestEnableEmptyManagedFoldersRationalization(t *testing.T) {
testcases := []struct {
name string
enableHns bool
enableEmptyManagedFolders bool
expectedEnableEmptyManagedFolders bool
}{
{
name: "both enable-hns and enable-empty-managed-folders set to true",
enableHns: true,
enableEmptyManagedFolders: true,
expectedEnableEmptyManagedFolders: true,
},
{
name: "enable-hns set to true and enable-empty-managed-folders set to false",
enableHns: true,
enableEmptyManagedFolders: false,
expectedEnableEmptyManagedFolders: true,
},
{
name: "enable-hns set to false and enable-empty-managed-folders set to true",
enableHns: false,
enableEmptyManagedFolders: true,
expectedEnableEmptyManagedFolders: true,
},
{
name: "both enable-hns and enable-empty-managed-folders set to false",
enableHns: false,
enableEmptyManagedFolders: false,
expectedEnableEmptyManagedFolders: false,
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
flags := &flagStorage{
ClientProtocol: mountpkg.ClientProtocol("http1"),
SequentialReadSizeMb: 200,
ExperimentalMetadataPrefetchOnMount: "disabled",
}
c := config.NewMountConfig()
c.EnableHNS = tc.enableHns
c.ListConfig.EnableEmptyManagedFolders = tc.enableEmptyManagedFolders

resolvedConfig, err := PopulateNewConfigFromLegacyFlagsAndConfig(&mockCLIContext{}, flags, c)

if assert.NoError(t, err) {
assert.Equal(t, tc.expectedEnableEmptyManagedFolders, resolvedConfig.List.EnableEmptyManagedFolders)
}
})
}
}

func TestPopulateConfigFromLegacyFlags_MountOption(t *testing.T) {
flags := &flagStorage{
MountOptions: []string{"rw,nodev", "user=jacobsa,noauto"},
Expand Down
6 changes: 3 additions & 3 deletions cmd/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -735,13 +735,13 @@ func TestArgsParsing_EnableHNSFlags(t *testing.T) {
}{
{
name: "normal",
args: []string{"gcsfuse", "--enable-hns", "abc", "pqr"},
expectedEnableHNS: true,
args: []string{"gcsfuse", "--enable-hns=false", "abc", "pqr"},
expectedEnableHNS: false,
},
{
name: "default",
args: []string{"gcsfuse", "abc", "pqr"},
expectedEnableHNS: false,
expectedEnableHNS: true,
},
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/testdata/valid_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,4 @@ file-system:
temp-dir: ~/temp
list:
enable-empty-managed-folders: true
enable-hns: true
enable-hns: false
2 changes: 1 addition & 1 deletion internal/config/mount_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const (
DefaultEnableEmptyManagedFoldersListing = false
DefaultGrpcConnPoolSize = 1
DefaultAnonymousAccess = false
DefaultEnableHNS = false
DefaultEnableHNS = true
DefaultIgnoreInterrupts = true
DefaultPrometheusPort = 0

Expand Down
2 changes: 1 addition & 1 deletion internal/config/yaml_parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func validateDefaultConfig(t *testing.T, mountConfig *MountConfig) {
assert.Equal(t, int64(4*1024*1024), mountConfig.FileCacheConfig.WriteBufferSize)
assert.Equal(t, 1, mountConfig.GCSConnection.GRPCConnPoolSize)
assert.False(t, mountConfig.GCSAuth.AnonymousAccess)
assert.False(t, bool(mountConfig.EnableHNS))
assert.True(t, bool(mountConfig.EnableHNS))
assert.True(t, mountConfig.FileSystemConfig.IgnoreInterrupts)
assert.False(t, mountConfig.FileSystemConfig.DisableParallelDirops)
assert.Equal(t, DefaultKernelListCacheTtlSeconds, mountConfig.KernelListCacheTtlSeconds)
Expand Down
37 changes: 20 additions & 17 deletions internal/fs/inode/dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,9 @@ type dirInode struct {
// Constant data
/////////////////////////

id fuseops.InodeID
implicitDirs bool
enableManagedFoldersListing bool
id fuseops.InodeID
implicitDirs bool
includeFoldersAsPrefixes bool

enableNonexistentTypeCache bool

Expand Down Expand Up @@ -247,7 +247,7 @@ func NewDirInode(
name Name,
attrs fuseops.InodeAttributes,
implicitDirs bool,
enableManagedFoldersListing bool,
includeFoldersAsPrefixes bool,
enableNonexistentTypeCache bool,
typeCacheTTL time.Duration,
bucket *gcsx.SyncerBucket,
Expand All @@ -262,18 +262,18 @@ func NewDirInode(
}

typed := &dirInode{
bucket: bucket,
mtimeClock: mtimeClock,
cacheClock: cacheClock,
id: id,
implicitDirs: implicitDirs,
enableManagedFoldersListing: enableManagedFoldersListing,
enableNonexistentTypeCache: enableNonexistentTypeCache,
name: name,
attrs: attrs,
cache: metadata.NewTypeCache(typeCacheMaxSizeMB, typeCacheTTL),
isHNSEnabled: isHNSEnabled,
unlinked: false,
bucket: bucket,
mtimeClock: mtimeClock,
cacheClock: cacheClock,
id: id,
implicitDirs: implicitDirs,
includeFoldersAsPrefixes: includeFoldersAsPrefixes,
enableNonexistentTypeCache: enableNonexistentTypeCache,
name: name,
attrs: attrs,
cache: metadata.NewTypeCache(typeCacheMaxSizeMB, typeCacheTTL),
isHNSEnabled: isHNSEnabled,
unlinked: false,
}

typed.lc.Init(id)
Expand Down Expand Up @@ -654,6 +654,9 @@ func (d *dirInode) ReadDescendants(ctx context.Context, limit int) (map[Name]*Co
func (d *dirInode) readObjects(
ctx context.Context,
tok string) (cores map[Name]*Core, newTok string, err error) {
if d.isBucketHierarchical() {
d.includeFoldersAsPrefixes = true
}
// Ask the bucket to list some objects.
req := &gcs.ListObjectsRequest{
Delimiter: "/",
Expand All @@ -664,7 +667,7 @@ func (d *dirInode) readObjects(
// Setting Projection param to noAcl since fetching owner and acls are not
// required.
ProjectionVal: gcs.NoAcl,
IncludeFoldersAsPrefixes: d.enableManagedFoldersListing,
IncludeFoldersAsPrefixes: d.includeFoldersAsPrefixes,
}

listing, err := d.bucket.ListObjects(ctx, req)
Expand Down
2 changes: 1 addition & 1 deletion internal/fs/inode/dir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func (p DirentSlice) Less(i, j int) bool { return p[i].Name < p[j].Name }
func (p DirentSlice) Swap(i, j int) { p[i], p[j] = p[j], p[i] }

// NOTE: A limitation in the fake bucket's API prevents the direct creation of managed folders.
// This poses a challenge for writing unit tests for enableManagedFoldersListing.
// This poses a challenge for writing unit tests for includeFoldersAsPrefixes.

func (t *DirTest) resetInode(implicitDirs, enableNonexistentTypeCache, enableManagedFoldersListing bool) {
t.resetInodeWithTypeCacheConfigs(implicitDirs, enableNonexistentTypeCache, enableManagedFoldersListing, config.DefaultTypeCacheMaxSizeMB, typeCacheTTL)
Expand Down
4 changes: 2 additions & 2 deletions internal/fs/inode/explicit_dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func NewExplicitDirInode(
m *gcs.MinObject,
attrs fuseops.InodeAttributes,
implicitDirs bool,
enableManagedFoldersListing bool,
includeFoldersAsPrefixes bool,
enableNonexistentTypeCache bool,
typeCacheTTL time.Duration,
bucket *gcsx.SyncerBucket,
Expand All @@ -51,7 +51,7 @@ func NewExplicitDirInode(
name,
attrs,
implicitDirs,
enableManagedFoldersListing,
includeFoldersAsPrefixes,
enableNonexistentTypeCache,
typeCacheTTL,
bucket,
Expand Down
Loading