objstore/s3: make IdleConnTimeout in http.Transport configurable by GiedriusS · Pull Request #567 · thanos-io/thanos

GiedriusS · 2018-10-10T18:46:53Z

Let's make the http.Transport used inside objstore/s3 configurable. It helps in some cases where a HAProxy in front of the S3 storage sometimes has smaller time-out values and then spurious messages are printed to the logs which are actually harmless. It seems that it also helps with invalid HTTP queries that Go's stdlib sometimes sends to check if a connection is still alive.

Changes

Added new fields to the Config struct and added the default values from the code before. No impact to the user at all if they have nothing specified in those config files.

The documentation should be updated as well but I am not sure what kind of format should I follow for the docs so I left it up to you :)

Verification

Added unit tests for checking the new parsing function which has default values.

minio/minio-go#1037 check this link for more information.

bwplotka

One suggestion -> remember you are defining here an "API" that we aim to be stable, so we need to be careful here

bwplotka · 2018-10-11T12:43:11Z

pkg/objstore/s3/s3.go

+	SignatureV2           bool   `yaml:"signature-version2"`
+	SSEEncryption         bool   `yaml:"encrypt-sse"`
+	SecretKey             string `yaml:"secret-key"`
+	IdleConnTimeout       int    `yaml:"idle-conn-timeout`


Why not using model.Time here? It nicely marshals from 5s or 5m . I think it would better than guessing what is the unit of those (:

Don't you mean model.Duration? It seems like model.Time can only unmarshal strings of format "a.b" and then it converts it into seconds whereas model.Duration has a whole parser for different units like you said: https://github.com/prometheus/common/blob/master/model/time.go#L182

I agree with you there, though. Some software only recognizes and accepts seconds however we could be smarter here and re-use some of the Prometheus code to implement it properly :)

I meant Duration, sorry (:

jturolla · 2018-10-18T19:37:41Z

What problem are you trying to solve? Do you have issues with the defaults?

We have not encountered issues with this, maybe if you're experiencing timeouts you should move to a bucket near your location?

GiedriusS · 2018-10-18T20:35:38Z

Yes, I guess that they probably only make sense when using Google's or Amazon's S3 storage. In our setup, we have a HAProxy in front of a Ceph-based S3 storage. This is related to keepalive connection timeouts mismatches between the HAProxy and the http.Transport in s3.go in Thanos. I outlined the issue here: minio/minio-go#1037. I could provide you the exact timeout values but you get the idea: the mismatch causes that harmless message to be printed from time to time, and the implementation of http.Transport also prints a message that a keep-alive connection had been closed unexpectedly, when that 400 error happens, in stdout.
The bucket is certainly close to the location where Thanos is running, the latencies are around <1ms. :) That's not the problem.

GiedriusS · 2018-10-18T20:59:17Z

Sorry for the mess with the commits and rebasing, should be all OK now :)

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

bwplotka

OK two things comes to my mind when I reviewed this PR.

A) Since config is part of our interface, adding additional (even optional) fields makes our interface wider,
which increases complexity.

Are you adding those just in case of you need to tweak particular option? If only particular one, can we just add that one?

Why we cannot have ONE defaults that works for all cases? Isn't that possible?

B) What if someone want to tweak let's say IdleConnTimeout but for GCS provider? We would end up doing same code there as well? Maybe doing this in common place makes sense?

Sorry for delay in review!

bwplotka · 2018-10-19T15:53:57Z

pkg/objstore/s3/s3.go

 // NewBucket returns a new Bucket using the provided s3 config values.
 func NewBucket(logger log.Logger, conf []byte, reg prometheus.Registerer, component string) (*Bucket, error) {
-	var config Config
+	defIdleConnTimeout, _ := model.ParseDuration("90s")


Defaults could be done as package var and have model.MustParseDuration
1a. Why even parsing? 90 * time.Second does not work here?

This strictly relays on yaml Marshaling and how it deals if fields is not filled. We would feel more safe if we had a unit test for this?

Let's maybe pack this into function? so we can nicely test it

The idea here was to use model.ParseDuration so that we would less associate with the actual underlying type of model.Duration but I guess I will change it to just use 90 * time.Second if that is OK with you. I agree that it definitely makes things clearer. As for other points, I agree. I thought it was the default and only acceptable behaviour of unmarshal function but we can move it into a function - it would be much nicer.

GiedriusS · 2018-10-21T18:08:19Z

In our particular case only IdleConnTimeout is enough but what about the other options? Perhaps someone may want to change the other ones as well so I added these to this PR just in case. I don't quite agree with adding one single option for tweaking the idle connections timeout because e.g. other storage providers may not use at all the HTTP protocol underneath their calls.
I think that having one single defaults like this isn't really possible because someone might have e.g. different timeout settings in their haproxy instances and it might be impossible to change on them on the server itself if, for example, it is used intensively by other applications.
I'll make changes and update the PR. Thank you for your review.

bwplotka · 2018-10-23T15:47:39Z

Perhaps someone may want to change the other ones as well so I added these to this PR just in case.

Yes, but it's not a problem to add this later if needed. Those options are quite low level and requires deep knowledge on HTTP client library trivia. I would vote for adding what you need and also pack it in some http-config nested structure, so it will look like:

type: S3
config:
  http_config:
    idle_conn_timeout: <val>

What do you think?

bwplotka · 2018-10-23T15:49:17Z

pkg/objstore/s3/s3.go

+	SignatureV2           bool           `yaml:"signature-version2"`
+	SSEEncryption         bool           `yaml:"encrypt-sse"`
+	SecretKey             string         `yaml:"secret-key"`
+	IdleConnTimeout       model.Duration `yaml:"idle-conn-timeout"`


Can we do yaml names with underscore? I don't know how we slipped the dash convention, but I would stay with the style Prometheus has: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#%3Cscrape_config%3E

GiedriusS · 2018-10-30T12:37:40Z

@bwplotka could you PTAL again? I moved the HTTP config to a different struct and the parsing to a separate function, added a simple unit test for it. I still think there is value in being able to configure other values as well. Being able to configure idle connection timeouts works for us here but someone might run into other issues if they have other settings in their reverse proxy.

GiedriusS · 2018-11-08T20:48:01Z

@bwplotka ping.

bwplotka

Some suggestions, but overall LGTM

It's overall YAGNI rule - to not add something that no one wants right now, just because it will be maybe useful (: but not a blocker here.

bwplotka · 2018-11-09T18:23:59Z

pkg/objstore/s3/s3.go

+	HTTPConfig    HTTPConfig `yaml:"http_config"`
+}
+
+// HTTPConfig stores the http.Transport configuration for the s3 minio client


missing trailing period

bwplotka · 2018-11-09T18:24:21Z

pkg/objstore/s3/s3.go

 	sse    encrypt.ServerSide
 }

+// ParseConfig unmarshals a buffer into a Config with default HTTPConfig values


missing trailing period

bwplotka · 2018-11-09T18:27:15Z

pkg/objstore/s3/s3.go


+// ParseConfig unmarshals a buffer into a Config with default HTTPConfig values
+func ParseConfig(conf []byte) (Config, error) {
+	defaultHTTPConfig := HTTPConfig{IdleConnTimeout: model.Duration(90 * time.Second),


Let's format it nicely. We need to put newline for each field as the number of elements is large.

bwplotka · 2018-11-09T18:34:26Z

pkg/objstore/s3/s3.go

+
+// HTTPConfig stores the http.Transport configuration for the s3 minio client
+type HTTPConfig struct {
+	IdleConnTimeout       model.Duration `yaml:"idle_conn_timeout"`


I would really suggest just doing the thing you need right now which is IdleConnTimeout explicitly for HA Proxy.

I feel we should stick to YAGNI here and not introduce something that is not useful right now: https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

GiedriusS · 2018-11-15T14:00:39Z

Updated the PR according to your comments. I don't see a problem with giving the users complete power in terms of configuration but I understand your concerns and I only left idle_conn_timeout for now since that is what is causing these problems in the first place. Please take a look when you have time.

bwplotka

Thanks!

GiedriusS force-pushed the configurable_client branch from a8f0366 to 712525a Compare October 10, 2018 18:53

bwplotka requested changes Oct 11, 2018

View reviewed changes

GiedriusS closed this Oct 18, 2018

GiedriusS force-pushed the configurable_client branch from dd33933 to 4a9f380 Compare October 18, 2018 20:45

GiedriusS reopened this Oct 18, 2018

GiedriusS force-pushed the configurable_client branch from 896e39b to ee8494a Compare October 18, 2018 20:54

GiedriusS changed the title ~~objstore/s3: make s3 client configurable~~ objstore/s3: make http.Transport configurable Oct 18, 2018

objstore/s3: make http.Transport configurable

4870b61

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

GiedriusS force-pushed the configurable_client branch from ee8494a to 4870b61 Compare October 18, 2018 21:07

bwplotka requested changes Oct 19, 2018

View reviewed changes

bwplotka reviewed Oct 23, 2018

View reviewed changes

Giedrius Statkevičius added 6 commits October 30, 2018 12:47

Merge remote-tracking branch 'origin/master' into configurable_client

700ee96

objstore/s3: directly use time.Second

cf11135

objstore/s3: move HTTP settings into a different struct

16faa2d

objstore/s3: add unit tests for new function ParseConfig

e45293f

objstore/s3: fix tabs vs. spaces in test string

9e62490

objstore/s3: remove last instance of a dash in HTTPConfig

43555cc

bwplotka approved these changes Nov 9, 2018

View reviewed changes

objstore/s3: only leave idle_conn_timeout

0016510

GiedriusS changed the title ~~objstore/s3: make http.Transport configurable~~ objstore/s3: make IdleConnTimeout in http.Transport configurable Nov 15, 2018

bwplotka approved these changes Nov 16, 2018

View reviewed changes

bwplotka merged commit 10657cb into thanos-io:master Nov 16, 2018

midnightconman mentioned this pull request May 3, 2019

objstore: Increase the response header timeout in the S3 provider to 2 minutes and make it configurable #1094

Merged

Conversation

GiedriusS commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jturolla commented Oct 18, 2018

Uh oh!

GiedriusS commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GiedriusS commented Oct 18, 2018

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GiedriusS commented Oct 21, 2018

Uh oh!

bwplotka commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GiedriusS commented Oct 30, 2018

Uh oh!

GiedriusS commented Nov 8, 2018

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GiedriusS commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GiedriusS commented Oct 10, 2018 •

edited

Loading

GiedriusS commented Oct 18, 2018 •

edited

Loading

GiedriusS commented Nov 15, 2018 •

edited

Loading