storage: Swift backend using thanos.io/objstore #11672

btaani · 2024-01-12T16:21:58Z

What this PR does / why we need it:

Update thanos-io/objstore to use the latest version
Implement a new swift chunk client that relies on Thanos
Add TLS support for swift bucket and chuck clients

Which issue(s) this PR fixes:
Fixes LOG-4819, LOG-5063

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

pkg/storage/chunk/client/openstack/swift_object_client.go

slim-bean · 2024-01-22T15:47:43Z

pkg/storage/bucket/swift/config.go

+	Timeout               time.Duration `yaml:"timeout"`
+	IdleConnTimeout       time.Duration `yaml:"idle_conn_timeout"`
+	ResponseHeaderTimeout time.Duration `yaml:"response_header_timeout"`


These values are define here but I don't see anywhere they are actually being used to configure the underlying transport?

Thanks, you're right I had assumed that this was the struct being used inside the chunks package. I've moved it there already.

Following @periklis 's advice to reuse the bucket_http config inside the swift bucket config, I've noticed that the swift config used is coming from thanos-io/objstore, which doesn't have an HTTP field built into it (in contrary to s3's thanos/objstore config struct which does have the HTTP field).
This limits the addition of the HTTPConfig.Transport field to the swift config.

So I need your advice here since I lack experience in this codebase, do you suggest that we wait until it's added to thanos-io/objstore's swift config? Or should we proceed with this PR only with the object client, while leaving the bucket client until either a restructure of the config is done, or till the correct fields are added to the thanos/objstore config?
Edit: after discussing with @periklis, we agreed to 1) add the required HTTP config in thanos-io/objstore and 2) use the thanos-io/objstore config in both the bucket and object client, for the sake of unifying all storage clients.

slim-bean · 2024-01-22T15:49:37Z

pkg/storage/bucket/swift/config.go

@@ -54,6 +64,11 @@ func (cfg *Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 	f.IntVar(&cfg.MaxRetries, prefix+"swift.max-retries", 3, "Max retries on requests error.")
 	f.DurationVar(&cfg.ConnectTimeout, prefix+"swift.connect-timeout", 10*time.Second, "Time after which a connection attempt is aborted.")
 	f.DurationVar(&cfg.RequestTimeout, prefix+"swift.request-timeout", 5*time.Second, "Time after which an idle request is aborted. The timeout watchdog is reset each time some data is received, so the timeout triggers after X time no data is received on a request.")
+	f.DurationVar(&cfg.HTTPConfig.IdleConnTimeout, prefix+"swift.http.idle-conn-timeout", 90*time.Second, "The maximum amount of time an idle connection will be held open.")


This would be a change from the currently used default value of 0 for unlimited, I think as we are trying to get this change into a patch fix release it would be better to keep the current default value.

Also as mentioned above I don't see where this value actually configures the underlying transport

@slim-bean AFAICS, GCS, Azure and S3 all define as default 90sec. Maybe you refer to some different place?

periklis · 2024-01-22T20:14:26Z

@btaani @slim-bean After careful consideration the present state of this PR does not consider all the places to configure the HTTP transport capabilities as proposed. TL;DR; The PR considers adding parameters only to the chunk/object client but not to the bucket client.

@btaani FYI the Loki code base is handling for historical reasons (part of it from Cortex's legacy) the buckets and object stored into them via separate clients. The main rationale for this separation is that for each use case we have different transport and encryption requirements (i.e. buckets are created once and is more of a fire and forget operation, while objects are accessed regularly). In any case we need to consider adding the HTTP capabilities in two places as of now:

OpenStack Swift bucket client: https://github.com/grafana/loki/blob/main/pkg/storage/bucket/swift/bucket_client.go
OpenStack Swift chunk/object client: https://github.com/grafana/loki/blob/main/pkg/storage/chunk/client/openstack/swift_object_client.go

For the sake of simplicity, you can consider reusing the bucket/http/config.go. For example S3 is using this in bucket/s3/config.go for the bucket client. OTH for the Swift object client you can keep your custom struct as other providers are doing.

JStickler · 2024-02-12T17:13:57Z

docs/sources/configure/_index.md

@@ -5174,6 +5174,16 @@ The `swift_storage_config` block configures the connection to OpenStack Object S
 # is received on a request.
 # CLI flag: -<prefix>.swift.request-timeout
 [request_timeout: <duration> | default = 5s]
+
+# Set to false to skip verifying the certificate chain and hostname.


Did you edit this file directly? Or is this change automatically generated? The configuration reference should be updated in docs/sources/configure/index.template.

I did so directly.. thanks for pointing this out!

add TLS support for swift

0a946a2

pull-request-size bot added the size/M label Jan 12, 2024

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jan 12, 2024

btaani added 2 commits January 12, 2024 17:23

fix typo

88df54d

fix error from unit test

5f821dd

pull-request-size bot added size/L and removed size/M labels Jan 12, 2024

Merge branch 'main' into swift-tls

749f64b

btaani changed the title ~~Add support for custom TLS config in swift storage client~~ Add support for TLS config in swift storage client Jan 16, 2024

Merge branch 'main' into swift-tls

f5bcc9c

btaani marked this pull request as ready for review January 16, 2024 09:49

btaani requested a review from a team as a code owner January 16, 2024 09:49

btaani and others added 2 commits January 18, 2024 14:07

Merge branch 'main' into swift-tls

06ef15b

remove condition on transport type

fb562c4

pull-request-size bot added size/M and removed size/L labels Jan 18, 2024

periklis reviewed Jan 18, 2024

View reviewed changes

pkg/storage/chunk/client/openstack/swift_object_client.go Outdated Show resolved Hide resolved

remove duplicate initialization

b1b203b

slim-bean reviewed Jan 22, 2024

View reviewed changes

btaani and others added 4 commits February 5, 2024 13:55

modify swift config in object client

7b23e51

use bucket_http config in swift bucket client

d5ad3e7

Merge branch 'main' into swift-tls

6b70293

add swift thanos object client implementation

6a266e0

pull-request-size bot added size/L and removed size/M labels Feb 9, 2024

btaani changed the title ~~Add support for TLS config in swift storage client~~ storage: Swift backend using thanos.io/objstore Feb 9, 2024

JStickler reviewed Feb 12, 2024

View reviewed changes

undo docs change

fd98f34

github-actions bot removed the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Feb 13, 2024

change all objectClient implementations

959aa77

pull-request-size bot added size/XL and removed size/L labels Feb 13, 2024

btaani and others added 7 commits February 13, 2024 18:05

add hedged client to swift thanos client

05acfd9

add hedged client to swift thanos client

ce0c7e6

Merge branch 'main' into swift-tls

3659f19

update thanos-io/objstore

7fe24b8

add TLS support

53fd4ee

Merge branch 'main' into swift-tls

b02afa8

Merge branch 'main' into swift-tls

0c6526e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: Swift backend using thanos.io/objstore #11672

storage: Swift backend using thanos.io/objstore #11672

btaani commented Jan 12, 2024 •

edited

slim-bean Jan 22, 2024

btaani Feb 5, 2024 •

edited

slim-bean Jan 22, 2024

periklis Jan 22, 2024

periklis commented Jan 22, 2024

JStickler Feb 12, 2024

btaani Feb 13, 2024

storage: Swift backend using thanos.io/objstore #11672

Are you sure you want to change the base?

storage: Swift backend using thanos.io/objstore #11672

Conversation

btaani commented Jan 12, 2024 • edited

slim-bean Jan 22, 2024

Choose a reason for hiding this comment

btaani Feb 5, 2024 • edited

Choose a reason for hiding this comment

slim-bean Jan 22, 2024

Choose a reason for hiding this comment

periklis Jan 22, 2024

Choose a reason for hiding this comment

periklis commented Jan 22, 2024

JStickler Feb 12, 2024

Choose a reason for hiding this comment

btaani Feb 13, 2024

Choose a reason for hiding this comment

btaani commented Jan 12, 2024 •

edited

btaani Feb 5, 2024 •

edited