From 74c2048ea9ad0982123623d5e0a184f42490762b Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 18 Jan 2019 23:06:45 +0530 Subject: [PATCH] Add proper contexts with timeouts for etcd operations (#7097) This fixes an issue of perceived hang when incorrect unreachable URLs are specified in MINIO_ETCD_ENDPOINTS variable. Fixes #7096 --- cmd/common-main.go | 1 + cmd/config-common.go | 34 +++++++++++++++++++++++++++------- cmd/config.go | 28 +++++++++++----------------- cmd/iam.go | 6 +++--- pkg/quick/encoding.go | 20 ++++++++++++++------ 5 files changed, 56 insertions(+), 33 deletions(-) diff --git a/cmd/common-main.go b/cmd/common-main.go index ca4de0d39..f18785206 100644 --- a/cmd/common-main.go +++ b/cmd/common-main.go @@ -242,6 +242,7 @@ func handleCommonEnvVars() { return &cert, terr } } + globalEtcdClient, err = etcd.New(etcd.Config{ Endpoints: etcdEndpoints, DialTimeout: defaultDialTimeout, diff --git a/cmd/config-common.go b/cmd/config-common.go index f4cb9045a..397d9b116 100644 --- a/cmd/config-common.go +++ b/cmd/config-common.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "errors" + "fmt" etcd "github.com/coreos/etcd/clientv3" "github.com/minio/minio/cmd/logger" @@ -60,8 +61,15 @@ func deleteConfig(ctx context.Context, objAPI ObjectLayer, configFile string) er } func saveConfigEtcd(ctx context.Context, client *etcd.Client, configFile string, data []byte) error { - _, err := client.Put(ctx, configFile, string(data)) - return err + timeoutCtx, cancel := context.WithTimeout(ctx, defaultContextTimeout) + defer cancel() + _, err := client.Put(timeoutCtx, configFile, string(data)) + if err == context.DeadlineExceeded { + return fmt.Errorf("etcd setup is unreachable, please check your endpoints %s", client.Endpoints()) + } else if err != nil { + return fmt.Errorf("unexpected error %s returned by etcd setup, please check your endpoints %s", err, client.Endpoints()) + } + return nil } func saveConfig(ctx context.Context, objAPI ObjectLayer, configFile string, data []byte) error { @@ -75,9 +83,14 @@ func saveConfig(ctx context.Context, objAPI ObjectLayer, configFile string, data } func readConfigEtcd(ctx context.Context, client *etcd.Client, configFile string) ([]byte, error) { - resp, err := client.Get(ctx, configFile) + timeoutCtx, cancel := context.WithTimeout(ctx, defaultContextTimeout) + defer cancel() + resp, err := client.Get(timeoutCtx, configFile) if err != nil { - return nil, err + if err == context.DeadlineExceeded { + return nil, fmt.Errorf("etcd setup is unreachable, please check your endpoints %s", client.Endpoints()) + } + return nil, fmt.Errorf("unexpected error %s returned by etcd setup, please check your endpoints %s", err, client.Endpoints()) } if resp.Count == 0 { return nil, errConfigNotFound @@ -93,7 +106,9 @@ func readConfigEtcd(ctx context.Context, client *etcd.Client, configFile string) // watchConfig - watches for changes on `configFile` on etcd and loads them. func watchConfig(objAPI ObjectLayer, configFile string, loadCfgFn func(ObjectLayer) error) { if globalEtcdClient != nil { - for watchResp := range globalEtcdClient.Watch(context.Background(), configFile) { + ctx, cancel := context.WithTimeout(context.Background(), defaultContextTimeout) + defer cancel() + for watchResp := range globalEtcdClient.Watch(ctx, configFile) { for _, event := range watchResp.Events { if event.IsModify() || event.IsCreate() { loadCfgFn(objAPI) @@ -104,9 +119,14 @@ func watchConfig(objAPI ObjectLayer, configFile string, loadCfgFn func(ObjectLay } func checkConfigEtcd(ctx context.Context, client *etcd.Client, configFile string) error { - resp, err := globalEtcdClient.Get(ctx, configFile) + timeoutCtx, cancel := context.WithTimeout(ctx, defaultContextTimeout) + defer cancel() + resp, err := client.Get(timeoutCtx, configFile) if err != nil { - return err + if err == context.DeadlineExceeded { + return fmt.Errorf("etcd setup is unreachable, please check your endpoints %s", client.Endpoints()) + } + return fmt.Errorf("unexpected error %s returned by etcd setup, please check your endpoints %s", err, client.Endpoints()) } if resp.Count == 0 { return errConfigNotFound diff --git a/cmd/config.go b/cmd/config.go index 8a7d1d7e4..8799c263f 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -24,7 +24,6 @@ import ( "path" "runtime" "strings" - "time" "github.com/minio/minio/cmd/logger" "github.com/minio/minio/pkg/quick" @@ -52,10 +51,7 @@ func saveServerConfig(ctx context.Context, objAPI ObjectLayer, config *serverCon configFile := path.Join(minioConfigPrefix, minioConfigFile) if globalEtcdClient != nil { - timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Minute) - _, err = globalEtcdClient.Put(timeoutCtx, configFile, string(data)) - defer cancel() - return err + return saveConfigEtcd(ctx, globalEtcdClient, configFile, data) } // Create a backup of the current config @@ -163,18 +159,16 @@ func initConfig(objAPI ObjectLayer) error { } if globalEtcdClient != nil { - ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) - resp, err := globalEtcdClient.Get(ctx, getConfigFile()) - cancel() - if err == nil && resp.Count > 0 { - if err = migrateConfig(); err != nil { - return err - } - - // Migrates etcd ${HOME}/.minio/config.json to '/config/config.json' - if err := migrateConfigToMinioSys(objAPI); err != nil { - return err - } + if err := checkConfigEtcd(context.Background(), globalEtcdClient, getConfigFile()); err != nil { + return err + } + // Migrates all configs at old location. + if err := migrateConfig(); err != nil { + return err + } + // Migrates etcd ${HOME}/.minio/config.json to '/config/config.json' + if err := migrateConfigToMinioSys(objAPI); err != nil { + return err } } else { if isFile(getConfigFile()) { diff --git a/cmd/iam.go b/cmd/iam.go index 3bd6ea872..9ff27032f 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -464,13 +464,13 @@ func (sys *IAMSys) IsAllowed(args iampolicy.Args) bool { return args.IsOwner } -var defaultContextTimeout = 5 * time.Minute +var defaultContextTimeout = 30 * time.Second // Similar to reloadUsers but updates users, policies maps from etcd server, func reloadEtcdUsers(prefix string, usersMap map[string]auth.Credentials, policyMap map[string]string) error { ctx, cancel := context.WithTimeout(context.Background(), defaultContextTimeout) - r, err := globalEtcdClient.Get(ctx, prefix, etcd.WithPrefix(), etcd.WithKeysOnly()) defer cancel() + r, err := globalEtcdClient.Get(ctx, prefix, etcd.WithPrefix(), etcd.WithKeysOnly()) if err != nil { return err } @@ -536,8 +536,8 @@ func reloadEtcdUsers(prefix string, usersMap map[string]auth.Credentials, policy func reloadEtcdPolicies(prefix string, cannedPolicyMap map[string]iampolicy.Policy) error { ctx, cancel := context.WithTimeout(context.Background(), defaultContextTimeout) - r, err := globalEtcdClient.Get(ctx, prefix, etcd.WithPrefix(), etcd.WithKeysOnly()) defer cancel() + r, err := globalEtcdClient.Get(ctx, prefix, etcd.WithPrefix(), etcd.WithKeysOnly()) if err != nil { return err } diff --git a/pkg/quick/encoding.go b/pkg/quick/encoding.go index cdaa46b3d..562abbdd0 100644 --- a/pkg/quick/encoding.go +++ b/pkg/quick/encoding.go @@ -138,18 +138,26 @@ func saveFileConfigEtcd(filename string, clnt *etcd.Client, v interface{}) error dataBytes = []byte(strings.Replace(string(dataBytes), "\n", "\r\n", -1)) } - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) - _, err = clnt.Put(ctx, filename, string(dataBytes)) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - return err + _, err = clnt.Put(ctx, filename, string(dataBytes)) + if err == context.DeadlineExceeded { + return fmt.Errorf("etcd setup is unreachable, please check your endpoints %s", clnt.Endpoints()) + } else if err != nil { + return fmt.Errorf("unexpected error %s returned by etcd setup, please check your endpoints %s", err, clnt.Endpoints()) + } + return nil } func loadFileConfigEtcd(filename string, clnt *etcd.Client, v interface{}) error { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) - resp, err := clnt.Get(ctx, filename) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() + resp, err := clnt.Get(ctx, filename) if err != nil { - return err + if err == context.DeadlineExceeded { + return fmt.Errorf("etcd setup is unreachable, please check your endpoints %s", clnt.Endpoints()) + } + return fmt.Errorf("unexpected error %s returned by etcd setup, please check your endpoints %s", err, clnt.Endpoints()) } if resp.Count == 0 { return dns.ErrNoEntriesFound