diff --git a/ctl/backup.go b/ctl/backup.go index ff5a93307..4a0e1bade 100644 --- a/ctl/backup.go +++ b/ctl/backup.go @@ -42,6 +42,9 @@ type BackupCommand struct { // nolint: maligned // Amount of time after first failed request to continue retrying. RetryPeriod time.Duration `json:"retry-period"` + // Response Header Timeout for HTTP Requests + HeaderTimeout time.Duration `json:"header-timeout"` + // Host:port on which to listen for pprof. Pprof string `json:"pprof"` @@ -59,10 +62,11 @@ type BackupCommand struct { // nolint: maligned // NewBackupCommand returns a new instance of BackupCommand. func NewBackupCommand(stdin io.Reader, stdout, stderr io.Writer) *BackupCommand { return &BackupCommand{ - CmdIO: pilosa.NewCmdIO(stdin, stdout, stderr), - Concurrency: 1, - RetryPeriod: time.Minute, - Pprof: "localhost:0", + CmdIO: pilosa.NewCmdIO(stdin, stdout, stderr), + Concurrency: 1, + RetryPeriod: time.Minute, + HeaderTimeout: time.Second * 3, + Pprof: "localhost:0", } } @@ -89,7 +93,7 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) { } // Create a client to the server. - client, err := commandClient(cmd, fb_http.WithClientRetryPeriod(cmd.RetryPeriod)) + client, err := commandClient(cmd, fb_http.WithClientRetryPeriod(cmd.RetryPeriod), fb_http.ClientResponseHeaderTimeoutOption(cmd.HeaderTimeout)) if err != nil { return fmt.Errorf("creating client: %w", err) } @@ -285,7 +289,9 @@ func (cmd *BackupCommand) backupShardNode(ctx context.Context, indexName string, logger := cmd.Logger() logger.Printf("backing up shard: index=%q id=%d", indexName, shard) - client := fb_http.NewInternalClientFromURI(&node.URI, fb_http.GetHTTPClient(cmd.tlsConfig), fb_http.WithClientRetryPeriod(cmd.RetryPeriod)) + client := fb_http.NewInternalClientFromURI(&node.URI, + fb_http.GetHTTPClient(cmd.tlsConfig, fb_http.ClientResponseHeaderTimeoutOption(cmd.HeaderTimeout)), + fb_http.WithClientRetryPeriod(cmd.RetryPeriod)) rc, err := client.ShardReader(ctx, indexName, shard) if err != nil { return fmt.Errorf("fetching shard reader: %w", err) diff --git a/ctl/common.go b/ctl/common.go index 028a1250a..ecb20d8e9 100644 --- a/ctl/common.go +++ b/ctl/common.go @@ -2,11 +2,8 @@ package ctl import ( - "net" "time" - gohttp "net/http" - "github.com/molecula/featurebase/v3/http" "github.com/molecula/featurebase/v3/logger" "github.com/molecula/featurebase/v3/server" @@ -30,24 +27,49 @@ func SetTLSConfig(flags *pflag.FlagSet, prefix string, certificatePath *string, flags.BoolVarP(enableClientVerification, prefix+"tls.enable-client-verification", "", false, "Enable TLS certificate client verification for incoming connections") } -// default dial timeout is 30s for some reason which makes testing -// failures/retries really awkward. I don't think we need it that -// high, so I set it to 1s here... let's see what happens. -func clientOptions(client *gohttp.Client, dialer *net.Dialer) *gohttp.Client { - dialer.Timeout = time.Second * 1 - return client -} +// AnyClientOption can be either http.InternalClientOption or +// http.ClientOption. The internal options are specific to the +// featurebase client, whereas the client options are applied to the +// Go HTTP client that gets used under the hood. +type AnyClientOption interface{} // commandClient returns a pilosa.InternalHTTPClient for the command -func commandClient(cmd CommandWithTLSSupport, opts ...http.InternalClientOption) (*http.InternalClient, error) { +func commandClient(cmd CommandWithTLSSupport, opts ...AnyClientOption) (*http.InternalClient, error) { + internalopts, clientopts, err := separateOptions(opts...) + if err != nil { + return nil, errors.Wrap(err, "separating client options") + } + + // we default dial timeout to 3s in commandClient, but prepend it + // to the option list so other options can override it. + clientopts = append([]http.ClientOption{http.ClientDialTimeoutOption(time.Second * 3)}, clientopts...) tls := cmd.TLSConfiguration() tlsConfig, err := server.GetTLSConfig(&tls, cmd.Logger()) if err != nil { return nil, errors.Wrap(err, "getting tls config") } - client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig, clientOptions), opts...) + client, err := http.NewInternalClient(cmd.TLSHost(), http.GetHTTPClient(tlsConfig, clientopts...), internalopts...) if err != nil { return nil, errors.Wrap(err, "getting internal client") } return client, err } + +// separateOptions splits the list of AnyClientOption into the two +// possible types. +func separateOptions(opts ...AnyClientOption) ([]http.InternalClientOption, []http.ClientOption, error) { + internalopts := []http.InternalClientOption{} + clientopts := []http.ClientOption{} + for _, opt := range opts { + if iopt, ok := opt.(http.InternalClientOption); ok { + internalopts = append(internalopts, iopt) + continue + } + if copt, ok := opt.(http.ClientOption); ok { + clientopts = append(clientopts, copt) + continue + } + return nil, nil, errors.Errorf("opt: %+v of type %[1]T must be an InternalClientOption or a ClientOption", opt) + } + return internalopts, clientopts, nil +} diff --git a/http/handler.go b/http/handler.go index 173988485..700b4dc7a 100644 --- a/http/handler.go +++ b/http/handler.go @@ -2864,9 +2864,23 @@ func (s queryValidationSpec) validate(query url.Values) error { type ClientOption func(client *http.Client, dialer *net.Dialer) *http.Client +func ClientResponseHeaderTimeoutOption(dur time.Duration) ClientOption { + return func(client *http.Client, dialer *net.Dialer) *http.Client { + client.Transport.(*http.Transport).ResponseHeaderTimeout = dur + return client + } +} + +func ClientDialTimeoutOption(dur time.Duration) ClientOption { + return func(client *http.Client, dialer *net.Dialer) *http.Client { + dialer.Timeout = dur + return client + } +} + func GetHTTPClient(t *tls.Config, opts ...ClientOption) *http.Client { dialer := &net.Dialer{ - Timeout: 5 * time.Second, + Timeout: 30 * time.Second, KeepAlive: 15 * time.Second, DualStack: true, } @@ -2878,7 +2892,6 @@ func GetHTTPClient(t *tls.Config, opts ...ClientOption) *http.Client { IdleConnTimeout: 20 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, - ResponseHeaderTimeout: 4 * time.Second, } if t != nil { transport.TLSClientConfig = t diff --git a/internal/clustertests/cluster_test.go b/internal/clustertests/cluster_test.go index ad31172e2..1d97d85f2 100644 --- a/internal/clustertests/cluster_test.go +++ b/internal/clustertests/cluster_test.go @@ -236,6 +236,16 @@ func TestClusterStuff(t *testing.T) { t.Fatalf("restore failed: %v", err) } + fmt.Println("pausing all featurebasen") + if err = sendCmd("docker", "pause", container(t, "pilosa1")); err != nil { + t.Fatalf("sending pause command: %v", err) + } + if err = sendCmd("docker", "pause", container(t, "pilosa2")); err != nil { + t.Fatalf("sending pause command: %v", err) + } + if err = sendCmd("docker", "pause", container(t, "pilosa3")); err != nil { + t.Fatalf("sending pause command: %v", err) + } // now do backup with all nodes down and too short a timeout // so it fails. Has be to be all 3 because the cluster has // replicas=3 and the backup command will retry on replicas. @@ -250,21 +260,10 @@ func TestClusterStuff(t *testing.T) { t.Fatalf("sending second backup command: %v", err) } } - time.Sleep(time.Millisecond * 10) // want the backup to get started, then fail - fmt.Println("pausing all featurebasen") - if err = sendCmd("docker", "pause", container(t, "pilosa1")); err != nil { - t.Fatalf("sending pause command: %v", err) - } - if err = sendCmd("docker", "pause", container(t, "pilosa2")); err != nil { - t.Fatalf("sending pause command: %v", err) - } - if err = sendCmd("docker", "pause", container(t, "pilosa3")); err != nil { - t.Fatalf("sending pause command: %v", err) - } - fmt.Println("sleeping long") - time.Sleep(time.Second * 10) - fmt.Println("restarting") + t.Logf("sleeping 8s") + time.Sleep(time.Second * 8) + t.Logf("restarting FB nodes") if err = sendCmd("docker", "unpause", container(t, "pilosa1")); err != nil { t.Fatalf("sending unpause command: %v", err)