From 005872deb5bfdc2b668d7e12a8a702d25ba80469 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Mon, 22 Apr 2024 09:17:29 +0000 Subject: [PATCH 01/32] adding parent struct --- main_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/main_test.go b/main_test.go index 0d87ec221e..f7e0f74523 100644 --- a/main_test.go +++ b/main_test.go @@ -170,7 +170,9 @@ func (t *MainTest) TestStringifyShouldReturnAllFlagsPassedInMountConfigAsMarshal ListConfig: config.ListConfig{ EnableEmptyManagedFolders: false, }, - EnableHNS: true, + BucketFlow: config.BucketFlow{ + EnableHNS: true, + }, } actual, err := util.Stringify(mountConfig) From afbb4688f732e1a51128218ee76bdcd9f5793a1a Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Mon, 22 Apr 2024 07:38:49 +0000 Subject: [PATCH 02/32] adding new control client --- go.mod | 1 + go.sum | 2 ++ internal/storage/storage_handle.go | 57 +++++++++++++++++++++++++++--- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index e869e91843..681f10aab1 100644 --- a/go.mod +++ b/go.mod @@ -38,6 +38,7 @@ require ( cloud.google.com/go v0.112.1 // indirect cloud.google.com/go/compute v1.24.0 // indirect cloud.google.com/go/iam v1.1.7 // indirect + cloud.google.com/go/longrunning v0.5.5 // indirect cloud.google.com/go/monitoring v1.18.0 // indirect cloud.google.com/go/pubsub v1.36.1 // indirect cloud.google.com/go/trace v1.10.5 // indirect diff --git a/go.sum b/go.sum index f72878f69e..36a8d9a6bb 100644 --- a/go.sum +++ b/go.sum @@ -51,6 +51,8 @@ cloud.google.com/go/iam v1.1.7 h1:z4VHOhwKLF/+UYXAJDFwGtNF0b6gjsW1Pk9Ml0U/IoM= cloud.google.com/go/iam v1.1.7/go.mod h1:J4PMPg8TtyurAUvSmPj8FF3EDgY1SPRZxcUGrn7WXGA= cloud.google.com/go/kms v1.15.7 h1:7caV9K3yIxvlQPAcaFffhlT7d1qpxjB1wHBtjWa13SM= cloud.google.com/go/kms v1.15.7/go.mod h1:ub54lbsa6tDkUwnu4W7Yt1aAIFLnspgh0kPGToDukeI= +cloud.google.com/go/longrunning v0.5.5 h1:GOE6pZFdSrTb4KAiKnXsJBtlE6mEyaW44oKyMILWnOg= +cloud.google.com/go/longrunning v0.5.5/go.mod h1:WV2LAxD8/rg5Z1cNW6FJ/ZpX4E4VnDnoTk0yawPBB7s= cloud.google.com/go/monitoring v1.18.0 h1:NfkDLQDG2UR3WYZVQE8kwSbUIEyIqJUPl+aOQdFH1T4= cloud.google.com/go/monitoring v1.18.0/go.mod h1:c92vVBCeq/OB4Ioyo+NbN2U7tlg5ZH41PZcdvfc+Lcg= cloud.google.com/go/pubsub v1.0.1/go.mod h1:R0Gpsv3s54REJCy4fxDixWD93lHJMoZTyQ2kNxGRt3I= diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 4f09242195..d348e90e32 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -20,6 +20,7 @@ import ( "os" "cloud.google.com/go/storage" + control "cloud.google.com/go/storage/control/apiv2" "github.com/googleapis/gax-go/v2" "github.com/googlecloudplatform/gcsfuse/v2/internal/logger" mountpkg "github.com/googlecloudplatform/gcsfuse/v2/internal/mount" @@ -44,13 +45,48 @@ type StorageHandle interface { } type storageClient struct { - client *storage.Client + client *storage.Client + storageControlClient *control.StorageControlClient +} + +// The default protocol for the Go Storage control client's folders API is gRPC. +// gcsfuse will initially mirror this behavior due to the client's lack of HTTP support. +func createGRPCStorageControlClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *control.StorageControlClient, err error) { + var clientOpts []option.ClientOption + + if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { + logger.Fatal("error setting direct path env var: %v", err) + } + + // Add Custom endpoint option. + if clientConfig.CustomEndpoint != nil { + clientOpts = append(clientOpts, option.WithEndpoint(clientConfig.CustomEndpoint.String())) + clientOpts = append(clientOpts, option.WithoutAuthentication()) + clientOpts = append(clientOpts, option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials()))) + } else { + tokenSrc, err := storageutil.CreateTokenSource(clientConfig) + if err != nil { + err = fmt.Errorf("while fetching tokenSource: %w", err) + return + } + clientOpts = append(clientOpts, option.WithTokenSource(tokenSrc)) + } + + clientOpts = append(clientOpts, option.WithGRPCConnectionPool(clientConfig.GrpcConnPoolSize)) + clientOpts = append(clientOpts, option.WithUserAgent(clientConfig.UserAgent)) + + // Unset the environment variable, since it's used only while creation of grpc client. + if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { + logger.Fatal("error while unsetting direct path env var: %v", err) + } + + return } // Followed https://pkg.go.dev/cloud.google.com/go/storage#hdr-Experimental_gRPC_API to create the gRPC client. -func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, err error) { +func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, controlClient *control.StorageControlClient, err error) { if clientConfig.ClientProtocol != mountpkg.GRPC { - return nil, fmt.Errorf("client-protocol requested is not GRPC: %s", clientConfig.ClientProtocol) + return nil, nil, fmt.Errorf("client-protocol requested is not GRPC: %s", clientConfig.ClientProtocol) } if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { @@ -88,6 +124,16 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora clientOpts = append(clientOpts, option.WithUserAgent(clientConfig.UserAgent)) sc, err = storage.NewGRPCClient(ctx, clientOpts...) + if err != nil { + err = fmt.Errorf("NewGRPCClient: %w", err) + } + + if true { + controlClient, err = control.NewStorageControlClient(ctx, clientOpts...) + if err != nil { + err = fmt.Errorf("NewStorageControlClient: %w", err) + } + } // Unset the environment variable, since it's used only while creation of grpc client. if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { @@ -137,8 +183,9 @@ func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.Stora // http and gRPC client. func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClientConfig) (sh StorageHandle, err error) { var sc *storage.Client + var controlClient *control.StorageControlClient if clientConfig.ClientProtocol == mountpkg.GRPC { - sc, err = createGRPCClientHandle(ctx, &clientConfig) + sc, controlClient, err = createGRPCClientHandle(ctx, &clientConfig) } else if clientConfig.ClientProtocol == mountpkg.HTTP1 || clientConfig.ClientProtocol == mountpkg.HTTP2 { sc, err = createHTTPClientHandle(ctx, &clientConfig) } else { @@ -165,7 +212,7 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien storage.WithPolicy(storage.RetryAlways), storage.WithErrorFunc(storageutil.ShouldRetry)) - sh = &storageClient{client: sc} + sh = &storageClient{client: sc, storageControlClient: controlClient} return } From 7dfaee031cb091d73bbb76a35d867ff97033d4c5 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Mon, 22 Apr 2024 09:53:34 +0000 Subject: [PATCH 03/32] creating new storage control client on flag value true --- internal/storage/storage_handle.go | 52 +++++++------------------- internal/storage/storageutil/client.go | 3 ++ main.go | 1 + 3 files changed, 17 insertions(+), 39 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index d348e90e32..7d3f098c3f 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -49,40 +49,6 @@ type storageClient struct { storageControlClient *control.StorageControlClient } -// The default protocol for the Go Storage control client's folders API is gRPC. -// gcsfuse will initially mirror this behavior due to the client's lack of HTTP support. -func createGRPCStorageControlClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *control.StorageControlClient, err error) { - var clientOpts []option.ClientOption - - if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { - logger.Fatal("error setting direct path env var: %v", err) - } - - // Add Custom endpoint option. - if clientConfig.CustomEndpoint != nil { - clientOpts = append(clientOpts, option.WithEndpoint(clientConfig.CustomEndpoint.String())) - clientOpts = append(clientOpts, option.WithoutAuthentication()) - clientOpts = append(clientOpts, option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials()))) - } else { - tokenSrc, err := storageutil.CreateTokenSource(clientConfig) - if err != nil { - err = fmt.Errorf("while fetching tokenSource: %w", err) - return - } - clientOpts = append(clientOpts, option.WithTokenSource(tokenSrc)) - } - - clientOpts = append(clientOpts, option.WithGRPCConnectionPool(clientConfig.GrpcConnPoolSize)) - clientOpts = append(clientOpts, option.WithUserAgent(clientConfig.UserAgent)) - - // Unset the environment variable, since it's used only while creation of grpc client. - if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { - logger.Fatal("error while unsetting direct path env var: %v", err) - } - - return -} - // Followed https://pkg.go.dev/cloud.google.com/go/storage#hdr-Experimental_gRPC_API to create the gRPC client. func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, controlClient *control.StorageControlClient, err error) { if clientConfig.ClientProtocol != mountpkg.GRPC { @@ -128,7 +94,7 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora err = fmt.Errorf("NewGRPCClient: %w", err) } - if true { + if clientConfig.EnableHNS { controlClient, err = control.NewStorageControlClient(ctx, clientOpts...) if err != nil { err = fmt.Errorf("NewStorageControlClient: %w", err) @@ -143,7 +109,7 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora return } -func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, err error) { +func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, controlClient *control.StorageControlClient, err error) { var clientOpts []option.ClientOption // Add WithHttpClient option. @@ -157,7 +123,7 @@ func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.Stora clientOpts = append(clientOpts, option.WithHTTPClient(httpClient)) } else { - return nil, fmt.Errorf("client-protocol requested is not HTTP1 or HTTP2: %s", clientConfig.ClientProtocol) + return nil, nil, fmt.Errorf("client-protocol requested is not HTTP1 or HTTP2: %s", clientConfig.ClientProtocol) } if clientConfig.AnonymousAccess { @@ -174,7 +140,13 @@ func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.Stora clientOpts = append(clientOpts, option.WithEndpoint(clientConfig.CustomEndpoint.String())) } - return storage.NewClient(ctx, clientOpts...) + sc, err = storage.NewClient(ctx, clientOpts...) + + if clientConfig.EnableHNS { + logger.Info("GCSFuse does not support storage control client flow for HTTP client.") + } + + return sc, nil, err } // NewStorageHandle returns the handle of http or grpc Go storage client based on the @@ -183,11 +155,13 @@ func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.Stora // http and gRPC client. func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClientConfig) (sh StorageHandle, err error) { var sc *storage.Client + // The default protocol for the Go Storage control client's folders API is gRPC. + // gcsfuse will initially mirror this behavior due to the client's lack of HTTP support. var controlClient *control.StorageControlClient if clientConfig.ClientProtocol == mountpkg.GRPC { sc, controlClient, err = createGRPCClientHandle(ctx, &clientConfig) } else if clientConfig.ClientProtocol == mountpkg.HTTP1 || clientConfig.ClientProtocol == mountpkg.HTTP2 { - sc, err = createHTTPClientHandle(ctx, &clientConfig) + sc, controlClient, err = createHTTPClientHandle(ctx, &clientConfig) } else { err = fmt.Errorf("invalid client-protocol requested: %s", clientConfig.ClientProtocol) } diff --git a/internal/storage/storageutil/client.go b/internal/storage/storageutil/client.go index 6d5f4596e9..f7f5388748 100644 --- a/internal/storage/storageutil/client.go +++ b/internal/storage/storageutil/client.go @@ -52,6 +52,9 @@ type StorageClientConfig struct { /** Grpc client parameters. */ GrpcConnPoolSize int + + // Enabling new API flow for HNS bucket. + EnableHNS bool } func CreateHttpClient(storageClientConfig *StorageClientConfig) (httpClient *http.Client, err error) { diff --git a/main.go b/main.go index 81bbcc9dfe..058ba3a42b 100644 --- a/main.go +++ b/main.go @@ -115,6 +115,7 @@ func createStorageHandle(flags *flagStorage, mountConfig *config.MountConfig, us ReuseTokenFromUrl: flags.ReuseTokenFromUrl, ExperimentalEnableJsonRead: flags.ExperimentalEnableJsonRead, GrpcConnPoolSize: mountConfig.GrpcClientConfig.ConnPoolSize, + EnableHNS: mountConfig.BucketFlow.EnableHNS, } logger.Infof("UserAgent = %s\n", storageClientConfig.UserAgent) storageHandle, err = storage.NewStorageHandle(context.Background(), storageClientConfig) From 8418e6866b4591597780c71bb024d5e5c7237418 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 05:39:19 +0000 Subject: [PATCH 04/32] adding unit tests --- internal/storage/storage_handle.go | 2 +- internal/storage/storageutil/client.go | 3 ++- main.go | 2 +- main_test.go | 4 +--- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 7d3f098c3f..62df9b0ec8 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -143,7 +143,7 @@ func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.Stora sc, err = storage.NewClient(ctx, clientOpts...) if clientConfig.EnableHNS { - logger.Info("GCSFuse does not support storage control client flow for HTTP client.") + err = fmt.Errorf("GCSFuse does not support storage control client flow for HTTP client.") } return sc, nil, err diff --git a/internal/storage/storageutil/client.go b/internal/storage/storageutil/client.go index f7f5388748..eea0a1b2bd 100644 --- a/internal/storage/storageutil/client.go +++ b/internal/storage/storageutil/client.go @@ -23,6 +23,7 @@ import ( "time" "github.com/googlecloudplatform/gcsfuse/v2/internal/auth" + "github.com/googlecloudplatform/gcsfuse/v2/internal/config" mountpkg "github.com/googlecloudplatform/gcsfuse/v2/internal/mount" "golang.org/x/net/context" "golang.org/x/oauth2" @@ -54,7 +55,7 @@ type StorageClientConfig struct { GrpcConnPoolSize int // Enabling new API flow for HNS bucket. - EnableHNS bool + EnableHNS config.EnableHNS } func CreateHttpClient(storageClientConfig *StorageClientConfig) (httpClient *http.Client, err error) { diff --git a/main.go b/main.go index 058ba3a42b..00dced71fc 100644 --- a/main.go +++ b/main.go @@ -115,7 +115,7 @@ func createStorageHandle(flags *flagStorage, mountConfig *config.MountConfig, us ReuseTokenFromUrl: flags.ReuseTokenFromUrl, ExperimentalEnableJsonRead: flags.ExperimentalEnableJsonRead, GrpcConnPoolSize: mountConfig.GrpcClientConfig.ConnPoolSize, - EnableHNS: mountConfig.BucketFlow.EnableHNS, + EnableHNS: mountConfig.EnableHNS, } logger.Infof("UserAgent = %s\n", storageClientConfig.UserAgent) storageHandle, err = storage.NewStorageHandle(context.Background(), storageClientConfig) diff --git a/main_test.go b/main_test.go index f7e0f74523..0d87ec221e 100644 --- a/main_test.go +++ b/main_test.go @@ -170,9 +170,7 @@ func (t *MainTest) TestStringifyShouldReturnAllFlagsPassedInMountConfigAsMarshal ListConfig: config.ListConfig{ EnableEmptyManagedFolders: false, }, - BucketFlow: config.BucketFlow{ - EnableHNS: true, - }, + EnableHNS: true, } actual, err := util.Stringify(mountConfig) From cc1e6933fe3479ff4ca96daa2df3477596400704 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 07:44:01 +0000 Subject: [PATCH 05/32] unit tests using new assertion --- internal/storage/storageutil/run_test.go | 1 + 1 file changed, 1 insertion(+) create mode 100644 internal/storage/storageutil/run_test.go diff --git a/internal/storage/storageutil/run_test.go b/internal/storage/storageutil/run_test.go new file mode 100644 index 0000000000..81831f6c42 --- /dev/null +++ b/internal/storage/storageutil/run_test.go @@ -0,0 +1 @@ +package storageutil From 6cc9f6373ce563ea78ecc0f605df9329901d1700 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 07:47:14 +0000 Subject: [PATCH 06/32] remove unnecerry changes --- internal/storage/storageutil/run_test.go | 1 - 1 file changed, 1 deletion(-) delete mode 100644 internal/storage/storageutil/run_test.go diff --git a/internal/storage/storageutil/run_test.go b/internal/storage/storageutil/run_test.go deleted file mode 100644 index 81831f6c42..0000000000 --- a/internal/storage/storageutil/run_test.go +++ /dev/null @@ -1 +0,0 @@ -package storageutil From 0ef039a564252bb7af807c412eb3fce5179c39f8 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 07:52:27 +0000 Subject: [PATCH 07/32] triggering e2e tests From f7258c94dfa005a59aa0a3e4e9ff16768729c60f Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 10:45:14 +0000 Subject: [PATCH 08/32] review changes --- internal/storage/storage_handle.go | 79 ++++++++++++++++++------------ 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 62df9b0ec8..2661f25a32 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -49,18 +49,7 @@ type storageClient struct { storageControlClient *control.StorageControlClient } -// Followed https://pkg.go.dev/cloud.google.com/go/storage#hdr-Experimental_gRPC_API to create the gRPC client. -func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, controlClient *control.StorageControlClient, err error) { - if clientConfig.ClientProtocol != mountpkg.GRPC { - return nil, nil, fmt.Errorf("client-protocol requested is not GRPC: %s", clientConfig.ClientProtocol) - } - - if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { - logger.Fatal("error setting direct path env var: %v", err) - } - - var clientOpts []option.ClientOption - +func createClientOptionForGRPCClient(clientConfig *storageutil.StorageClientConfig) (clientOpts []option.ClientOption, err error) { // Add Custom endpoint option. if clientConfig.CustomEndpoint != nil { if clientConfig.AnonymousAccess { @@ -89,16 +78,46 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora clientOpts = append(clientOpts, option.WithGRPCConnectionPool(clientConfig.GrpcConnPoolSize)) clientOpts = append(clientOpts, option.WithUserAgent(clientConfig.UserAgent)) - sc, err = storage.NewGRPCClient(ctx, clientOpts...) + return +} + +func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *control.StorageControlClient, err error) { + if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { + logger.Fatal("error setting direct path env var: %v", err) + } + + clientOpts, err := createClientOptionForGRPCClient(clientConfig) if err != nil { - err = fmt.Errorf("NewGRPCClient: %w", err) + err = fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) + } + sc, err = control.NewStorageControlClient(ctx, clientOpts...) + if err != nil { + err = fmt.Errorf("NewStorageControlClient: %w", err) } - if clientConfig.EnableHNS { - controlClient, err = control.NewStorageControlClient(ctx, clientOpts...) - if err != nil { - err = fmt.Errorf("NewStorageControlClient: %w", err) - } + // Unset the environment variable, since it's used only while creation of grpc client. + if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { + logger.Fatal("error while unsetting direct path env var: %v", err) + } + + return sc, err +} + +// Followed https://pkg.go.dev/cloud.google.com/go/storage#hdr-Experimental_gRPC_API to create the gRPC client. +func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, err error) { + if clientConfig.ClientProtocol != mountpkg.GRPC { + return nil, fmt.Errorf("client-protocol requested is not GRPC: %s", clientConfig.ClientProtocol) + } + + if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { + logger.Fatal("error setting direct path env var: %v", err) + } + + clientOpts, err := createClientOptionForGRPCClient(clientConfig) + + sc, err = storage.NewGRPCClient(ctx, clientOpts...) + if err != nil { + err = fmt.Errorf("NewGRPCClient: %w", err) } // Unset the environment variable, since it's used only while creation of grpc client. @@ -109,7 +128,7 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora return } -func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, controlClient *control.StorageControlClient, err error) { +func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, err error) { var clientOpts []option.ClientOption // Add WithHttpClient option. @@ -123,7 +142,7 @@ func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.Stora clientOpts = append(clientOpts, option.WithHTTPClient(httpClient)) } else { - return nil, nil, fmt.Errorf("client-protocol requested is not HTTP1 or HTTP2: %s", clientConfig.ClientProtocol) + return nil, fmt.Errorf("client-protocol requested is not HTTP1 or HTTP2: %s", clientConfig.ClientProtocol) } if clientConfig.AnonymousAccess { @@ -140,13 +159,7 @@ func createHTTPClientHandle(ctx context.Context, clientConfig *storageutil.Stora clientOpts = append(clientOpts, option.WithEndpoint(clientConfig.CustomEndpoint.String())) } - sc, err = storage.NewClient(ctx, clientOpts...) - - if clientConfig.EnableHNS { - err = fmt.Errorf("GCSFuse does not support storage control client flow for HTTP client.") - } - - return sc, nil, err + return storage.NewClient(ctx, clientOpts...) } // NewStorageHandle returns the handle of http or grpc Go storage client based on the @@ -159,9 +172,9 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien // gcsfuse will initially mirror this behavior due to the client's lack of HTTP support. var controlClient *control.StorageControlClient if clientConfig.ClientProtocol == mountpkg.GRPC { - sc, controlClient, err = createGRPCClientHandle(ctx, &clientConfig) + sc, err = createGRPCClientHandle(ctx, &clientConfig) } else if clientConfig.ClientProtocol == mountpkg.HTTP1 || clientConfig.ClientProtocol == mountpkg.HTTP2 { - sc, controlClient, err = createHTTPClientHandle(ctx, &clientConfig) + sc, err = createHTTPClientHandle(ctx, &clientConfig) } else { err = fmt.Errorf("invalid client-protocol requested: %s", clientConfig.ClientProtocol) } @@ -171,6 +184,12 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien return } + if clientConfig.EnableHNS { + controlClient, err = createGRPCControlClientHandle(ctx, &clientConfig) + if err != nil { + err = fmt.Errorf("StorageControl Client: %w", err) + } + } // ShouldRetry function checks if an operation should be retried based on the // response of operation (error.Code). // RetryAlways causes all operations to be checked for retries using From 6e7ebfa7c898f161f5a12bd337bb9ebb1361cb60 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 10:52:13 +0000 Subject: [PATCH 09/32] lint fix --- internal/storage/storage_handle.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 2661f25a32..2b5e76e4da 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -86,7 +86,8 @@ func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageuti logger.Fatal("error setting direct path env var: %v", err) } - clientOpts, err := createClientOptionForGRPCClient(clientConfig) + var clientOpts []option.ClientOption + clientOpts, err = createClientOptionForGRPCClient(clientConfig) if err != nil { err = fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } @@ -113,7 +114,11 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora logger.Fatal("error setting direct path env var: %v", err) } - clientOpts, err := createClientOptionForGRPCClient(clientConfig) + var clientOpts []option.ClientOption + clientOpts, err = createClientOptionForGRPCClient(clientConfig) + if err != nil { + err = fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) + } sc, err = storage.NewGRPCClient(ctx, clientOpts...) if err != nil { From e2875d11675f0f126ae04493e37bae1291affa70 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 11:20:45 +0000 Subject: [PATCH 10/32] lint fix --- internal/storage/storage_handle.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 2b5e76e4da..25daa1cbf5 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -90,6 +90,7 @@ func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageuti clientOpts, err = createClientOptionForGRPCClient(clientConfig) if err != nil { err = fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) + return } sc, err = control.NewStorageControlClient(ctx, clientOpts...) if err != nil { @@ -118,6 +119,7 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora clientOpts, err = createClientOptionForGRPCClient(clientConfig) if err != nil { err = fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) + return } sc, err = storage.NewGRPCClient(ctx, clientOpts...) From 502ebe8787eb29c35018d98369f25577d8bcc19b Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 26 Apr 2024 11:22:13 +0000 Subject: [PATCH 11/32] lint fix --- internal/storage/storage_handle.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 25daa1cbf5..989d41ed4c 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -89,8 +89,7 @@ func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageuti var clientOpts []option.ClientOption clientOpts, err = createClientOptionForGRPCClient(clientConfig) if err != nil { - err = fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) - return + return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } sc, err = control.NewStorageControlClient(ctx, clientOpts...) if err != nil { @@ -118,8 +117,7 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora var clientOpts []option.ClientOption clientOpts, err = createClientOptionForGRPCClient(clientConfig) if err != nil { - err = fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) - return + return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } sc, err = storage.NewGRPCClient(ctx, clientOpts...) From 72545aed5515486ab24a755ab17e0c6b6239c57f Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 30 Apr 2024 08:40:10 +0000 Subject: [PATCH 12/32] removing unnecessary changes for go.mod --- go.mod | 3 --- go.sum | 3 +-- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 681f10aab1..3da44b3add 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,6 @@ require ( github.com/jacobsa/syncutil v0.0.0-20180201203307-228ac8e5a6c3 github.com/jacobsa/timeutil v0.0.0-20170205232429-577e5acbbcf6 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 - github.com/stretchr/testify v1.9.0 github.com/urfave/cli v1.22.14 go.opencensus.io v0.24.0 golang.org/x/net v0.22.0 @@ -48,7 +47,6 @@ require ( github.com/cncf/udpa/go v0.0.0-20220112060539-c52dc94e7fbe // indirect github.com/cncf/xds/go v0.0.0-20231128003011-0fa0005c9caa // indirect github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect - github.com/davecgh/go-spew v1.1.1 // indirect github.com/envoyproxy/go-control-plane v0.12.0 // indirect github.com/envoyproxy/protoc-gen-validate v1.0.4 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect @@ -64,7 +62,6 @@ require ( github.com/grpc-ecosystem/grpc-gateway/v2 v2.15.2 // indirect github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/pkg/xattr v0.4.9 // indirect - github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/prometheus v0.35.0 // indirect github.com/rogpeppe/go-internal v1.12.0 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect diff --git a/go.sum b/go.sum index 36a8d9a6bb..6d938306fd 100644 --- a/go.sum +++ b/go.sum @@ -1078,9 +1078,8 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= +github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= -github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= -github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/syndtr/gocapability v0.0.0-20170704070218-db04d3cc01c8/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww= github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww= From d809b0dad050a01374242dc73207d2d2805e8160 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 30 Apr 2024 09:59:58 +0000 Subject: [PATCH 13/32] review comment --- internal/storage/storage_handle.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 989d41ed4c..cd2028eb6e 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -189,12 +189,14 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien return } + // TODO: Verify that clientConfig controls the client protocol even when HTTP is available. if clientConfig.EnableHNS { controlClient, err = createGRPCControlClientHandle(ctx, &clientConfig) if err != nil { - err = fmt.Errorf("StorageControl Client: %w", err) + err = fmt.Errorf("Could not create StorageControl Client: %w", err) } } + // ShouldRetry function checks if an operation should be retried based on the // response of operation (error.Code). // RetryAlways causes all operations to be checked for retries using From d8c3f47f4d43471ba48fc753efd79a0bbb2eb2a1 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 30 Apr 2024 10:27:02 +0000 Subject: [PATCH 14/32] review comment --- internal/storage/storage_handle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index cd2028eb6e..d776d4f0e9 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -189,7 +189,7 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien return } - // TODO: Verify that clientConfig controls the client protocol even when HTTP is available. + // TODO: We will implement an additional check for the HTTP control client protocol once the Go SDK supports HTTP. if clientConfig.EnableHNS { controlClient, err = createGRPCControlClientHandle(ctx, &clientConfig) if err != nil { From 74c2778243120e4f0a7c0b8dadd81e8b77c86502 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Wed, 1 May 2024 07:37:55 +0000 Subject: [PATCH 15/32] rebasing --- internal/storage/storage_handle_test.go | 20 ++++++++++++++++++++ internal/storage/storageutil/test_util.go | 1 + 2 files changed, 21 insertions(+) diff --git a/internal/storage/storage_handle_test.go b/internal/storage/storage_handle_test.go index b71c761668..a0c4d7e767 100644 --- a/internal/storage/storage_handle_test.go +++ b/internal/storage/storage_handle_test.go @@ -276,3 +276,23 @@ func (testSuite *StorageHandleTest) TestNewStorageHandleWithGRPCClientWithCustom assert.Contains(testSuite.T(), err.Error(), "GRPC client doesn't support auth for custom-endpoint. Please set anonymous-access: true via config-file.") assert.Nil(testSuite.T(), handleCreated) } + +func TestCreateGRPCControlClientHandle(t *testing.T) { + sc := storageutil.GetDefaultStorageClientConfig() + sc.ClientProtocol = mountpkg.GRPC + + controlClient, err := createGRPCControlClientHandle(context.Background(), &sc) + + AssertEq(nil, err) + AssertNe(nil, controlClient) +} + +func TestCreateStorageHandleWithEnableHNSTrue(t *testing.T) { + sc := storageutil.GetDefaultStorageClientConfig() + sc.EnableHNS = true + + sh, err := NewStorageHandle(context.Background(), sc) + + AssertEq(nil, err) + AssertNe(nil, sh) +} diff --git a/internal/storage/storageutil/test_util.go b/internal/storage/storageutil/test_util.go index ada982ec93..660a65419c 100644 --- a/internal/storage/storageutil/test_util.go +++ b/internal/storage/storageutil/test_util.go @@ -41,5 +41,6 @@ func GetDefaultStorageClientConfig() (clientConfig StorageClientConfig) { ReuseTokenFromUrl: true, ExperimentalEnableJsonRead: false, AnonymousAccess: true, + EnableHNS: false, } } From 23832dab2fe4a9f58e47ba165d0d9c43c7a7e6ac Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 05:55:46 +0000 Subject: [PATCH 16/32] local changes --- go.mod | 3 ++ internal/storage/storage_handle.go | 50 +++++++++++++++++++++++++ internal/storage/storage_handle_test.go | 12 +++--- 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 3da44b3add..64fe2fa00d 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/jacobsa/syncutil v0.0.0-20180201203307-228ac8e5a6c3 github.com/jacobsa/timeutil v0.0.0-20170205232429-577e5acbbcf6 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 + github.com/stretchr/testify v1.8.4 github.com/urfave/cli v1.22.14 go.opencensus.io v0.24.0 golang.org/x/net v0.22.0 @@ -47,6 +48,7 @@ require ( github.com/cncf/udpa/go v0.0.0-20220112060539-c52dc94e7fbe // indirect github.com/cncf/xds/go v0.0.0-20231128003011-0fa0005c9caa // indirect github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect github.com/envoyproxy/go-control-plane v0.12.0 // indirect github.com/envoyproxy/protoc-gen-validate v1.0.4 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect @@ -62,6 +64,7 @@ require ( github.com/grpc-ecosystem/grpc-gateway/v2 v2.15.2 // indirect github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/pkg/xattr v0.4.9 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/prometheus v0.35.0 // indirect github.com/rogpeppe/go-internal v1.12.0 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index d776d4f0e9..c331db5652 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -18,6 +18,7 @@ import ( "fmt" "net/http" "os" + "time" "cloud.google.com/go/storage" control "cloud.google.com/go/storage/control/apiv2" @@ -28,6 +29,7 @@ import ( "golang.org/x/net/context" option "google.golang.org/api/option" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials/insecure" // Side effect to run grpc client with direct-path on gcp machine. @@ -81,6 +83,41 @@ func createClientOptionForGRPCClient(clientConfig *storageutil.StorageClientConf return } +func StorageControlClientRetriesForCreateAndDeleteFolder(clientConfig *storageutil.StorageClientConfig) *control.StorageControlCallOptions { + return &control.StorageControlCallOptions{ + CreateFolder: []gax.CallOption{ + gax.WithRetry(func() gax.Retryer { + return gax.Retryer( + storage.WithBackoff( + gax.Backoff{ + Max: clientConfig.MaxRetrySleep, + Multiplier: clientConfig.RetryMultiplier, + }), + storage.WithErrorFunc(storageutil.ShouldRetry), + storage.WithPolicy(storage.RetryAlways)), + } + }, + DeleteFolder: []gax.CallOption{ + gax.WithTimeout(60000 * time.Millisecond), + }, + GetFolder: []gax.CallOption{ + gax.WithRetry(func() gax.Retryer { + return gax.OnCodes([]codes.Code{ + codes.ResourceExhausted, + codes.Unavailable, + codes.DeadlineExceeded, + codes.Internal, + codes.Unknown, + }, gax.Backoff{ + Initial: 1000 * time.Millisecond, + Max: 60000 * time.Millisecond, + Multiplier: 2.00, + }) + }), + }, + } +} + func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *control.StorageControlClient, err error) { if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { logger.Fatal("error setting direct path env var: %v", err) @@ -91,11 +128,23 @@ func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageuti if err != nil { return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } + + retryer := gax.OnCodes([]codes.Code{ + codes.Unavailable, // Retry on server unavailable errors + codes.DeadlineExceeded, // Retry on deadline exceeded errors + }, gax.Backoff{ + Initial: 100 * time.Millisecond, + Max: 1000 * time.Millisecond, + Multiplier: 1.3, + }) + sc, err = control.NewStorageControlClient(ctx, clientOpts...) if err != nil { err = fmt.Errorf("NewStorageControlClient: %w", err) } + sc = control.StorageControlClient{CallOptions: control.StorageControlCallOptions{CreateFolder:}} + // Unset the environment variable, since it's used only while creation of grpc client. if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { logger.Fatal("error while unsetting direct path env var: %v", err) @@ -195,6 +244,7 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien if err != nil { err = fmt.Errorf("Could not create StorageControl Client: %w", err) } + } // ShouldRetry function checks if an operation should be retried based on the diff --git a/internal/storage/storage_handle_test.go b/internal/storage/storage_handle_test.go index a0c4d7e767..dd86120796 100644 --- a/internal/storage/storage_handle_test.go +++ b/internal/storage/storage_handle_test.go @@ -277,22 +277,22 @@ func (testSuite *StorageHandleTest) TestNewStorageHandleWithGRPCClientWithCustom assert.Nil(testSuite.T(), handleCreated) } -func TestCreateGRPCControlClientHandle(t *testing.T) { +func (testSuite *StorageHandleTest) TestCreateGRPCControlClientHandle() { sc := storageutil.GetDefaultStorageClientConfig() sc.ClientProtocol = mountpkg.GRPC controlClient, err := createGRPCControlClientHandle(context.Background(), &sc) - AssertEq(nil, err) - AssertNe(nil, controlClient) + assert.Nil(testSuite.T(),err) + assert.NotNil(testSuite.T(),controlClient) } -func TestCreateStorageHandleWithEnableHNSTrue(t *testing.T) { +func (testSuite *StorageHandleTest) TestCreateStorageHandleWithEnableHNSTrue() { sc := storageutil.GetDefaultStorageClientConfig() sc.EnableHNS = true sh, err := NewStorageHandle(context.Background(), sc) - AssertEq(nil, err) - AssertNe(nil, sh) + assert.Nil(testSuite.T(), err) + assert.NotNil(testSuite.T(),sh) } From d262ca98cb9e8828a7db11e8d1be4f78d1cd4ca9 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 06:34:55 +0000 Subject: [PATCH 17/32] adding retry option for control client --- internal/storage/storage_handle.go | 60 +++++++------------------ internal/storage/storage_handle_test.go | 20 +++++++++ 2 files changed, 37 insertions(+), 43 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index c331db5652..7d32921dba 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -18,7 +18,6 @@ import ( "fmt" "net/http" "os" - "time" "cloud.google.com/go/storage" control "cloud.google.com/go/storage/control/apiv2" @@ -29,7 +28,6 @@ import ( "golang.org/x/net/context" option "google.golang.org/api/option" "google.golang.org/grpc" - "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials/insecure" // Side effect to run grpc client with direct-path on gcp machine. @@ -83,38 +81,22 @@ func createClientOptionForGRPCClient(clientConfig *storageutil.StorageClientConf return } -func StorageControlClientRetriesForCreateAndDeleteFolder(clientConfig *storageutil.StorageClientConfig) *control.StorageControlCallOptions { +func storageControlClientRetryOptions(clientConfig *storageutil.StorageClientConfig) []gax.CallOption { + return []gax.CallOption{ gax.WithRetry(func() gax.Retryer { + return gax.OnErrorFunc(gax.Backoff{ + Max: clientConfig.MaxRetrySleep, + Multiplier: clientConfig.RetryMultiplier, + }, storageutil.ShouldRetry) + })} +} + +func storageControlClientRetries(clientConfig *storageutil.StorageClientConfig) *control.StorageControlCallOptions { return &control.StorageControlCallOptions{ - CreateFolder: []gax.CallOption{ - gax.WithRetry(func() gax.Retryer { - return gax.Retryer( - storage.WithBackoff( - gax.Backoff{ - Max: clientConfig.MaxRetrySleep, - Multiplier: clientConfig.RetryMultiplier, - }), - storage.WithErrorFunc(storageutil.ShouldRetry), - storage.WithPolicy(storage.RetryAlways)), - } - }, - DeleteFolder: []gax.CallOption{ - gax.WithTimeout(60000 * time.Millisecond), - }, - GetFolder: []gax.CallOption{ - gax.WithRetry(func() gax.Retryer { - return gax.OnCodes([]codes.Code{ - codes.ResourceExhausted, - codes.Unavailable, - codes.DeadlineExceeded, - codes.Internal, - codes.Unknown, - }, gax.Backoff{ - Initial: 1000 * time.Millisecond, - Max: 60000 * time.Millisecond, - Multiplier: 2.00, - }) - }), - }, + CreateFolder: storageControlClientRetryOptions(clientConfig), + DeleteFolder: storageControlClientRetryOptions(clientConfig), + GetFolder: storageControlClientRetryOptions(clientConfig), + RenameFolder: storageControlClientRetryOptions(clientConfig), + GetStorageLayout: storageControlClientRetryOptions(clientConfig), } } @@ -129,21 +111,13 @@ func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageuti return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } - retryer := gax.OnCodes([]codes.Code{ - codes.Unavailable, // Retry on server unavailable errors - codes.DeadlineExceeded, // Retry on deadline exceeded errors - }, gax.Backoff{ - Initial: 100 * time.Millisecond, - Max: 1000 * time.Millisecond, - Multiplier: 1.3, - }) - sc, err = control.NewStorageControlClient(ctx, clientOpts...) if err != nil { err = fmt.Errorf("NewStorageControlClient: %w", err) } - sc = control.StorageControlClient{CallOptions: control.StorageControlCallOptions{CreateFolder:}} + // Set retries for control client. + sc = &control.StorageControlClient{CallOptions: storageControlClientRetries(clientConfig)} // Unset the environment variable, since it's used only while creation of grpc client. if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { diff --git a/internal/storage/storage_handle_test.go b/internal/storage/storage_handle_test.go index dd86120796..df2d5f4d06 100644 --- a/internal/storage/storage_handle_test.go +++ b/internal/storage/storage_handle_test.go @@ -296,3 +296,23 @@ func (testSuite *StorageHandleTest) TestCreateStorageHandleWithEnableHNSTrue() { assert.Nil(testSuite.T(), err) assert.NotNil(testSuite.T(),sh) } + +func (testSuite *StorageHandleTest) TestStorageControlClientRetryOptions() { + sc := storageutil.GetDefaultStorageClientConfig() + + gaxOpts := storageControlClientRetryOptions(&sc) + + assert.NotNil(testSuite.T(), gaxOpts) +} + +func (testSuite *StorageHandleTest) TestStorageControlClientRetries() { + sc := storageutil.GetDefaultStorageClientConfig() + + storageControlOpts := storageControlClientRetries(&sc) + + assert.NotNil(testSuite.T(), storageControlOpts.CreateFolder) + assert.NotNil(testSuite.T(), storageControlOpts.DeleteFolder) + assert.NotNil(testSuite.T(), storageControlOpts.RenameFolder) + assert.NotNil(testSuite.T(), storageControlOpts.GetFolder) + assert.NotNil(testSuite.T(), storageControlOpts.GetStorageLayout) +} \ No newline at end of file From 9d2e9561c6e606d69c4fe0e2ea5ee001c0532318 Mon Sep 17 00:00:00 2001 From: Tulsi Shah <46474643+Tulsishah@users.noreply.github.com> Date: Fri, 3 May 2024 12:14:06 +0530 Subject: [PATCH 18/32] Update go.mod --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 64fe2fa00d..681f10aab1 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,7 @@ require ( github.com/jacobsa/syncutil v0.0.0-20180201203307-228ac8e5a6c3 github.com/jacobsa/timeutil v0.0.0-20170205232429-577e5acbbcf6 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 - github.com/stretchr/testify v1.8.4 + github.com/stretchr/testify v1.9.0 github.com/urfave/cli v1.22.14 go.opencensus.io v0.24.0 golang.org/x/net v0.22.0 From 2c3426530eaf579554ed0ae2df10fa234ae1cfdb Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 06:46:01 +0000 Subject: [PATCH 19/32] linux tests --- go.sum | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/go.sum b/go.sum index 6d938306fd..36a8d9a6bb 100644 --- a/go.sum +++ b/go.sum @@ -1078,8 +1078,9 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= +github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/syndtr/gocapability v0.0.0-20170704070218-db04d3cc01c8/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww= github.com/syndtr/gocapability v0.0.0-20180916011248-d98352740cb2/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww= From 1d03f418e02a6abc0af266a16746656b01626972 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 09:19:30 +0000 Subject: [PATCH 20/32] formating in control client file --- internal/storage/storage_handle.go | 54 ++------------- internal/storage/storage_handle_test.go | 30 -------- .../storage/storageutil/control_client.go | 64 +++++++++++++++++ .../storageutil/control_client_test.go | 69 +++++++++++++++++++ main2.go | 18 +++++ 5 files changed, 157 insertions(+), 78 deletions(-) create mode 100644 internal/storage/storageutil/control_client.go create mode 100644 internal/storage/storageutil/control_client_test.go create mode 100644 main2.go diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 7d32921dba..5bb3ab28cf 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -81,52 +81,6 @@ func createClientOptionForGRPCClient(clientConfig *storageutil.StorageClientConf return } -func storageControlClientRetryOptions(clientConfig *storageutil.StorageClientConfig) []gax.CallOption { - return []gax.CallOption{ gax.WithRetry(func() gax.Retryer { - return gax.OnErrorFunc(gax.Backoff{ - Max: clientConfig.MaxRetrySleep, - Multiplier: clientConfig.RetryMultiplier, - }, storageutil.ShouldRetry) - })} -} - -func storageControlClientRetries(clientConfig *storageutil.StorageClientConfig) *control.StorageControlCallOptions { - return &control.StorageControlCallOptions{ - CreateFolder: storageControlClientRetryOptions(clientConfig), - DeleteFolder: storageControlClientRetryOptions(clientConfig), - GetFolder: storageControlClientRetryOptions(clientConfig), - RenameFolder: storageControlClientRetryOptions(clientConfig), - GetStorageLayout: storageControlClientRetryOptions(clientConfig), - } -} - -func createGRPCControlClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *control.StorageControlClient, err error) { - if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { - logger.Fatal("error setting direct path env var: %v", err) - } - - var clientOpts []option.ClientOption - clientOpts, err = createClientOptionForGRPCClient(clientConfig) - if err != nil { - return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) - } - - sc, err = control.NewStorageControlClient(ctx, clientOpts...) - if err != nil { - err = fmt.Errorf("NewStorageControlClient: %w", err) - } - - // Set retries for control client. - sc = &control.StorageControlClient{CallOptions: storageControlClientRetries(clientConfig)} - - // Unset the environment variable, since it's used only while creation of grpc client. - if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { - logger.Fatal("error while unsetting direct path env var: %v", err) - } - - return sc, err -} - // Followed https://pkg.go.dev/cloud.google.com/go/storage#hdr-Experimental_gRPC_API to create the gRPC client. func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.StorageClientConfig) (sc *storage.Client, err error) { if clientConfig.ClientProtocol != mountpkg.GRPC { @@ -214,11 +168,15 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien // TODO: We will implement an additional check for the HTTP control client protocol once the Go SDK supports HTTP. if clientConfig.EnableHNS { - controlClient, err = createGRPCControlClientHandle(ctx, &clientConfig) + clientOpts, err := createClientOptionForGRPCClient(&clientConfig) if err != nil { - err = fmt.Errorf("Could not create StorageControl Client: %w", err) + return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } + controlClient, err = storageutil.CreateGRPCControlClientHandle(ctx, clientOpts, &clientConfig) + if err != nil { + err = fmt.Errorf("Could not create StorageControl Client: %w", err) + } } // ShouldRetry function checks if an operation should be retried based on the diff --git a/internal/storage/storage_handle_test.go b/internal/storage/storage_handle_test.go index df2d5f4d06..cb58771864 100644 --- a/internal/storage/storage_handle_test.go +++ b/internal/storage/storage_handle_test.go @@ -277,16 +277,6 @@ func (testSuite *StorageHandleTest) TestNewStorageHandleWithGRPCClientWithCustom assert.Nil(testSuite.T(), handleCreated) } -func (testSuite *StorageHandleTest) TestCreateGRPCControlClientHandle() { - sc := storageutil.GetDefaultStorageClientConfig() - sc.ClientProtocol = mountpkg.GRPC - - controlClient, err := createGRPCControlClientHandle(context.Background(), &sc) - - assert.Nil(testSuite.T(),err) - assert.NotNil(testSuite.T(),controlClient) -} - func (testSuite *StorageHandleTest) TestCreateStorageHandleWithEnableHNSTrue() { sc := storageutil.GetDefaultStorageClientConfig() sc.EnableHNS = true @@ -296,23 +286,3 @@ func (testSuite *StorageHandleTest) TestCreateStorageHandleWithEnableHNSTrue() { assert.Nil(testSuite.T(), err) assert.NotNil(testSuite.T(),sh) } - -func (testSuite *StorageHandleTest) TestStorageControlClientRetryOptions() { - sc := storageutil.GetDefaultStorageClientConfig() - - gaxOpts := storageControlClientRetryOptions(&sc) - - assert.NotNil(testSuite.T(), gaxOpts) -} - -func (testSuite *StorageHandleTest) TestStorageControlClientRetries() { - sc := storageutil.GetDefaultStorageClientConfig() - - storageControlOpts := storageControlClientRetries(&sc) - - assert.NotNil(testSuite.T(), storageControlOpts.CreateFolder) - assert.NotNil(testSuite.T(), storageControlOpts.DeleteFolder) - assert.NotNil(testSuite.T(), storageControlOpts.RenameFolder) - assert.NotNil(testSuite.T(), storageControlOpts.GetFolder) - assert.NotNil(testSuite.T(), storageControlOpts.GetStorageLayout) -} \ No newline at end of file diff --git a/internal/storage/storageutil/control_client.go b/internal/storage/storageutil/control_client.go new file mode 100644 index 0000000000..43cb1df1f9 --- /dev/null +++ b/internal/storage/storageutil/control_client.go @@ -0,0 +1,64 @@ +// Copyright 2024 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package storageutil + +import ( + "context" + "fmt" + "os" + + control "cloud.google.com/go/storage/control/apiv2" + "github.com/googleapis/gax-go/v2" + "github.com/googlecloudplatform/gcsfuse/v2/internal/logger" + "google.golang.org/api/option" +) + +func storageControlClientRetryOptions(clientConfig *StorageClientConfig) []gax.CallOption { + return []gax.CallOption{gax.WithRetry(func() gax.Retryer { + return gax.OnErrorFunc(gax.Backoff{ + Max: clientConfig.MaxRetrySleep, + Multiplier: clientConfig.RetryMultiplier, + }, ShouldRetry) + })} +} + +func storageControlClientRetries(sc *control.StorageControlClient, clientConfig *StorageClientConfig) { + sc.CallOptions.CreateFolder = storageControlClientRetryOptions(clientConfig) + sc.CallOptions.DeleteFolder = storageControlClientRetryOptions(clientConfig) + sc.CallOptions.RenameFolder = storageControlClientRetryOptions(clientConfig) + sc.CallOptions.GetFolder = storageControlClientRetryOptions(clientConfig) + sc.CallOptions.GetStorageLayout = storageControlClientRetryOptions(clientConfig) +} + +func CreateGRPCControlClientHandle(ctx context.Context, clientOpts []option.ClientOption, clientConfig *StorageClientConfig) (sc *control.StorageControlClient, err error) { + if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { + logger.Fatal("error setting direct path env var: %v", err) + } + + sc, err = control.NewStorageControlClient(ctx, clientOpts...) + if err != nil { + err = fmt.Errorf("NewStorageControlClient: %w", err) + } + + // Set retries for control client. + storageControlClientRetries(sc, clientConfig) + + // Unset the environment variable, since it's used only while creation of grpc client. + if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { + logger.Fatal("error while unsetting direct path env var: %v", err) + } + + return sc, err +} diff --git a/internal/storage/storageutil/control_client_test.go b/internal/storage/storageutil/control_client_test.go new file mode 100644 index 0000000000..d060bd4db4 --- /dev/null +++ b/internal/storage/storageutil/control_client_test.go @@ -0,0 +1,69 @@ +// Copyright 2024 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package storageutil + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +type ControlClientTest struct { + suite.Suite +} + +func TestControlClientTestSuite(t *testing.T) { + suite.Run(t, new(ControlClientTest)) +} + +func (testSuite *ControlClientTest) SetupTest() { +} + +func (testSuite *ControlClientTest) TearDownTest() { +} + +func (testSuite *ControlClientTest) TestStorageControlClientRetryOptions() { + clientConfig := GetDefaultStorageClientConfig() + + gaxOpts := storageControlClientRetryOptions(&clientConfig) + + assert.NotNil(testSuite.T(), gaxOpts) +} + +func (testSuite *ControlClientTest) TestStorageControlClient() { + clientConfig := GetDefaultStorageClientConfig() + + controlClient, err := CreateGRPCControlClientHandle(context.Background(), nil, &clientConfig) + + assert.Nil(testSuite.T(), err) + assert.NotNil(testSuite.T(), controlClient) +} + +func (testSuite *ControlClientTest) TestStorageControlClientSetRetry() { + clientConfig := GetDefaultStorageClientConfig() + controlClient, err := CreateGRPCControlClientHandle(context.Background(), nil, &clientConfig) + assert.Nil(testSuite.T(), err) + assert.NotNil(testSuite.T(), controlClient) + + storageControlClientRetries(controlClient, &clientConfig) + + assert.NotNil(testSuite.T(), controlClient.CallOptions.CreateFolder) + assert.NotNil(testSuite.T(), controlClient.CallOptions.DeleteFolder) + assert.NotNil(testSuite.T(), controlClient.CallOptions.RenameFolder) + assert.NotNil(testSuite.T(), controlClient.CallOptions.GetFolder) + assert.NotNil(testSuite.T(), controlClient.CallOptions.GetStorageLayout) +} diff --git a/main2.go b/main2.go new file mode 100644 index 0000000000..41701c4551 --- /dev/null +++ b/main2.go @@ -0,0 +1,18 @@ +package main + +import ( + control "cloud.google.com/go/storage/control/apiv2" +) + +func storageControlClientRetries(sc *control.StorageControlClient) { + return +} + +func CreateGRPCControlClientHandle() (sc *control.StorageControlClient, err error) { + + // Set retries for control client. + storageControlClientRetries(sc) + + + return sc, err +} From 79da07ea0f25f7500e5c43c6472919d67b65d883 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 09:19:39 +0000 Subject: [PATCH 21/32] formating in control client file --- main2.go | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 main2.go diff --git a/main2.go b/main2.go deleted file mode 100644 index 41701c4551..0000000000 --- a/main2.go +++ /dev/null @@ -1,18 +0,0 @@ -package main - -import ( - control "cloud.google.com/go/storage/control/apiv2" -) - -func storageControlClientRetries(sc *control.StorageControlClient) { - return -} - -func CreateGRPCControlClientHandle() (sc *control.StorageControlClient, err error) { - - // Set retries for control client. - storageControlClientRetries(sc) - - - return sc, err -} From 5fe80d23e427779791ec30f761b7158f8bb64232 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 09:27:18 +0000 Subject: [PATCH 22/32] adding unit test --- internal/storage/storage_handle_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/storage/storage_handle_test.go b/internal/storage/storage_handle_test.go index cb58771864..c621d6df65 100644 --- a/internal/storage/storage_handle_test.go +++ b/internal/storage/storage_handle_test.go @@ -286,3 +286,12 @@ func (testSuite *StorageHandleTest) TestCreateStorageHandleWithEnableHNSTrue() { assert.Nil(testSuite.T(), err) assert.NotNil(testSuite.T(),sh) } + +func (testSuite *StorageHandleTest) TestCreateClientOptionForGRPCClient() { + sc := storageutil.GetDefaultStorageClientConfig() + + clientOption, err := createClientOptionForGRPCClient(&sc) + + assert.Nil(testSuite.T(), err) + assert.NotNil(testSuite.T(),clientOption) +} From e13ad6270e7fadfb5f11b330cb1706743724b0bf Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 09:40:15 +0000 Subject: [PATCH 23/32] lint fix --- internal/storage/storage_handle.go | 1 - internal/storage/storage_handle_test.go | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 5bb3ab28cf..3fed2b78cc 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -172,7 +172,6 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien if err != nil { return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } - controlClient, err = storageutil.CreateGRPCControlClientHandle(ctx, clientOpts, &clientConfig) if err != nil { err = fmt.Errorf("Could not create StorageControl Client: %w", err) diff --git a/internal/storage/storage_handle_test.go b/internal/storage/storage_handle_test.go index c621d6df65..1e92ff3153 100644 --- a/internal/storage/storage_handle_test.go +++ b/internal/storage/storage_handle_test.go @@ -284,7 +284,7 @@ func (testSuite *StorageHandleTest) TestCreateStorageHandleWithEnableHNSTrue() { sh, err := NewStorageHandle(context.Background(), sc) assert.Nil(testSuite.T(), err) - assert.NotNil(testSuite.T(),sh) + assert.NotNil(testSuite.T(), sh) } func (testSuite *StorageHandleTest) TestCreateClientOptionForGRPCClient() { @@ -293,5 +293,5 @@ func (testSuite *StorageHandleTest) TestCreateClientOptionForGRPCClient() { clientOption, err := createClientOptionForGRPCClient(&sc) assert.Nil(testSuite.T(), err) - assert.NotNil(testSuite.T(),clientOption) + assert.NotNil(testSuite.T(), clientOption) } From 8baf6013ecec0b54f205202064948f758ef22dfd Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 09:41:59 +0000 Subject: [PATCH 24/32] lint fix --- internal/storage/storage_handle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 3fed2b78cc..47a9a94be8 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -174,7 +174,7 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien } controlClient, err = storageutil.CreateGRPCControlClientHandle(ctx, clientOpts, &clientConfig) if err != nil { - err = fmt.Errorf("Could not create StorageControl Client: %w", err) + return nil, fmt.Errorf("Could not create StorageControl Client: %w", err) } } From f6117de39bd1678784e21a1202de2cfb33ed20f6 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 09:57:47 +0000 Subject: [PATCH 25/32] linux tests --- .../storage/storageutil/control_client_test.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/internal/storage/storageutil/control_client_test.go b/internal/storage/storageutil/control_client_test.go index d060bd4db4..80d1ae4cd0 100644 --- a/internal/storage/storageutil/control_client_test.go +++ b/internal/storage/storageutil/control_client_test.go @@ -52,18 +52,3 @@ func (testSuite *ControlClientTest) TestStorageControlClient() { assert.Nil(testSuite.T(), err) assert.NotNil(testSuite.T(), controlClient) } - -func (testSuite *ControlClientTest) TestStorageControlClientSetRetry() { - clientConfig := GetDefaultStorageClientConfig() - controlClient, err := CreateGRPCControlClientHandle(context.Background(), nil, &clientConfig) - assert.Nil(testSuite.T(), err) - assert.NotNil(testSuite.T(), controlClient) - - storageControlClientRetries(controlClient, &clientConfig) - - assert.NotNil(testSuite.T(), controlClient.CallOptions.CreateFolder) - assert.NotNil(testSuite.T(), controlClient.CallOptions.DeleteFolder) - assert.NotNil(testSuite.T(), controlClient.CallOptions.RenameFolder) - assert.NotNil(testSuite.T(), controlClient.CallOptions.GetFolder) - assert.NotNil(testSuite.T(), controlClient.CallOptions.GetStorageLayout) -} From 71271da6203ccae62f231852de924062b598d686 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 10:06:37 +0000 Subject: [PATCH 26/32] linux tests --- internal/storage/storage_handle.go | 2 +- internal/storage/storageutil/control_client.go | 10 +++++----- internal/storage/storageutil/control_client_test.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 47a9a94be8..fe0836ddbe 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -172,7 +172,7 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien if err != nil { return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) } - controlClient, err = storageutil.CreateGRPCControlClientHandle(ctx, clientOpts, &clientConfig) + controlClient, err = storageutil.CreateGRPCControlClient(ctx, clientOpts, &clientConfig) if err != nil { return nil, fmt.Errorf("Could not create StorageControl Client: %w", err) } diff --git a/internal/storage/storageutil/control_client.go b/internal/storage/storageutil/control_client.go index 43cb1df1f9..9b485dfc17 100644 --- a/internal/storage/storageutil/control_client.go +++ b/internal/storage/storageutil/control_client.go @@ -42,23 +42,23 @@ func storageControlClientRetries(sc *control.StorageControlClient, clientConfig sc.CallOptions.GetStorageLayout = storageControlClientRetryOptions(clientConfig) } -func CreateGRPCControlClientHandle(ctx context.Context, clientOpts []option.ClientOption, clientConfig *StorageClientConfig) (sc *control.StorageControlClient, err error) { +func CreateGRPCControlClient(ctx context.Context, clientOpts []option.ClientOption, clientConfig *StorageClientConfig) (controlClient *control.StorageControlClient, err error) { if err := os.Setenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS", "true"); err != nil { logger.Fatal("error setting direct path env var: %v", err) } - sc, err = control.NewStorageControlClient(ctx, clientOpts...) + controlClient, err = control.NewStorageControlClient(ctx, clientOpts...) if err != nil { - err = fmt.Errorf("NewStorageControlClient: %w", err) + return nil, fmt.Errorf("NewStorageControlClient: %w", err) } // Set retries for control client. - storageControlClientRetries(sc, clientConfig) + storageControlClientRetries(controlClient, clientConfig) // Unset the environment variable, since it's used only while creation of grpc client. if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { logger.Fatal("error while unsetting direct path env var: %v", err) } - return sc, err + return controlClient, err } diff --git a/internal/storage/storageutil/control_client_test.go b/internal/storage/storageutil/control_client_test.go index 80d1ae4cd0..ceaab443a8 100644 --- a/internal/storage/storageutil/control_client_test.go +++ b/internal/storage/storageutil/control_client_test.go @@ -47,7 +47,7 @@ func (testSuite *ControlClientTest) TestStorageControlClientRetryOptions() { func (testSuite *ControlClientTest) TestStorageControlClient() { clientConfig := GetDefaultStorageClientConfig() - controlClient, err := CreateGRPCControlClientHandle(context.Background(), nil, &clientConfig) + controlClient, err := CreateGRPCControlClient(context.Background(), nil, &clientConfig) assert.Nil(testSuite.T(), err) assert.NotNil(testSuite.T(), controlClient) From cdc825d93661308b1cf1d459c364c815cf905921 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Fri, 3 May 2024 10:20:18 +0000 Subject: [PATCH 27/32] linux tests --- internal/storage/storageutil/control_client_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/storage/storageutil/control_client_test.go b/internal/storage/storageutil/control_client_test.go index ceaab443a8..999748519f 100644 --- a/internal/storage/storageutil/control_client_test.go +++ b/internal/storage/storageutil/control_client_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" + "google.golang.org/api/option" ) type ControlClientTest struct { @@ -45,9 +46,11 @@ func (testSuite *ControlClientTest) TestStorageControlClientRetryOptions() { } func (testSuite *ControlClientTest) TestStorageControlClient() { + var clientOpts []option.ClientOption + clientOpts = append(clientOpts, option.WithoutAuthentication()) clientConfig := GetDefaultStorageClientConfig() - controlClient, err := CreateGRPCControlClient(context.Background(), nil, &clientConfig) + controlClient, err := CreateGRPCControlClient(context.Background(), clientOpts, &clientConfig) assert.Nil(testSuite.T(), err) assert.NotNil(testSuite.T(), controlClient) From 06b4e197d533e194c801cc6ee1ca7d94cc5fd7f6 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Mon, 6 May 2024 04:44:58 +0000 Subject: [PATCH 28/32] adding comment for clientOpts --- internal/storage/storage_handle.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index fe0836ddbe..e86777f7ed 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -49,6 +49,7 @@ type storageClient struct { storageControlClient *control.StorageControlClient } +// Return clientOpts for both gRPC client and control client. func createClientOptionForGRPCClient(clientConfig *storageutil.StorageClientConfig) (clientOpts []option.ClientOption, err error) { // Add Custom endpoint option. if clientConfig.CustomEndpoint != nil { From 53e7e5854b899d59384bd9f8e4390768e24f5397 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 7 May 2024 10:32:13 +0000 Subject: [PATCH 29/32] review comment --- internal/storage/storageutil/control_client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/storage/storageutil/control_client.go b/internal/storage/storageutil/control_client.go index 9b485dfc17..cffe05a2a5 100644 --- a/internal/storage/storageutil/control_client.go +++ b/internal/storage/storageutil/control_client.go @@ -34,7 +34,7 @@ func storageControlClientRetryOptions(clientConfig *StorageClientConfig) []gax.C })} } -func storageControlClientRetries(sc *control.StorageControlClient, clientConfig *StorageClientConfig) { +func setRetryConfigForFolderAPIs(sc *control.StorageControlClient, clientConfig *StorageClientConfig) { sc.CallOptions.CreateFolder = storageControlClientRetryOptions(clientConfig) sc.CallOptions.DeleteFolder = storageControlClientRetryOptions(clientConfig) sc.CallOptions.RenameFolder = storageControlClientRetryOptions(clientConfig) @@ -53,7 +53,7 @@ func CreateGRPCControlClient(ctx context.Context, clientOpts []option.ClientOpti } // Set retries for control client. - storageControlClientRetries(controlClient, clientConfig) + setRetryConfigForFolderAPIs(controlClient, clientConfig) // Unset the environment variable, since it's used only while creation of grpc client. if err := os.Unsetenv("GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS"); err != nil { From 55f5b67b96c0f73c33ba42bf5974f320f88e77e2 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Wed, 8 May 2024 10:19:02 +0000 Subject: [PATCH 30/32] review comment --- internal/storage/storage_handle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index e86777f7ed..008d426812 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -171,7 +171,7 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien if clientConfig.EnableHNS { clientOpts, err := createClientOptionForGRPCClient(&clientConfig) if err != nil { - return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) + return nil, fmt.Errorf("error in getting clientOpts for gRPC client: %w", err) } controlClient, err = storageutil.CreateGRPCControlClient(ctx, clientOpts, &clientConfig) if err != nil { From 21ebe16a1e6ab606303203084c2e9047b2e2a91d Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Wed, 8 May 2024 10:21:55 +0000 Subject: [PATCH 31/32] review comment --- internal/storage/storage_handle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 008d426812..5706cd3dcc 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -175,7 +175,7 @@ func NewStorageHandle(ctx context.Context, clientConfig storageutil.StorageClien } controlClient, err = storageutil.CreateGRPCControlClient(ctx, clientOpts, &clientConfig) if err != nil { - return nil, fmt.Errorf("Could not create StorageControl Client: %w", err) + return nil, fmt.Errorf("could not create StorageControl Client: %w", err) } } From 2ce923282060bf75678421864f3df46bae77515e Mon Sep 17 00:00:00 2001 From: Tulsi Shah <46474643+Tulsishah@users.noreply.github.com> Date: Wed, 8 May 2024 16:01:05 +0530 Subject: [PATCH 32/32] Update storage_handle.go --- internal/storage/storage_handle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/storage_handle.go b/internal/storage/storage_handle.go index 5706cd3dcc..fd2f21ca24 100644 --- a/internal/storage/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -95,7 +95,7 @@ func createGRPCClientHandle(ctx context.Context, clientConfig *storageutil.Stora var clientOpts []option.ClientOption clientOpts, err = createClientOptionForGRPCClient(clientConfig) if err != nil { - return nil, fmt.Errorf("Error in getting clientOpts for gRPC client: %w", err) + return nil, fmt.Errorf("error in getting clientOpts for gRPC client: %w", err) } sc, err = storage.NewGRPCClient(ctx, clientOpts...)