.*: v0.19.0-rc.1 - Huge usage of memory to upload blocks to S3 compared to 0.18.0

[ahurtaud]

Thanos, Prometheus and Golang version used:
v0.19.0-rc.1

Object Storage Provider:
S3-capable API. (Scality)

What happened:
While validating 0.19.0-rc.1, we faced many OOMKilled thanos sidecars while uploading blocks.
Attaching 2 pprof of Allocs. v0.19 and v0.18
profile-0.19.pdf
minio-go
Client putObjectMultipartStreamFromReadAt 2052.85MB (46.65%)

profile-0.18.pdf

We are seeing same issue with the compactor, and thanos store I believe (but these have more memory limits)

What you expected to happen:
Everything was working fine with 0.18, I saw minio-go lib got updated, maybe a bug there?

How to reproduce it (as minimally and precisely as possible):
Hard to tell, dont know if this affect every S3 object storage. But memory consumption during upload (on big prometheus instance)

[yeya24]

The latest release adds hash calculation for each file when uploading the TSDB block. I am not sure if it will cause the memory issue.
@ahurtaud Maybe you can try to set hash-func flag to empty string and see the memory usage? https://github.com/thanos-io/thanos/blob/main/cmd/thanos/config.go#L144

If OOMs still happen, then it might be some problems with the minio sdk.

[GiedriusS]

What options do you have on Thanos Sidecar? 🤔 What about Store & Compact? Could you upload profiles from them as well? The minio-go function indeed seems to be the difference here.

[GiedriusS]

It's probably related to minio/mc#3376 & minio/minio-go#1357

[ahurtaud]

I am looking at it today.
@yeya24 : hash-func default value is the empty string :/
@GiedriusS : nothing fancy I think.

    Args:
      sidecar
      --log.level=info
      --tsdb.path=/var/prometheus
      --prometheus.url=http://localhost:9090
      --reloader.config-file=/etc/prometheus/config/prometheus.yml.tmpl
      --reloader.config-envsubst-file=/etc/prometheus/shared/prometheus.yml
      --objstore.config-file=/etc/prometheus/s3config/bucket.yml
      --reloader.rule-dir=/etc/prometheus/rules

I've checked also the issue you linked. It looks like minio is not planning a fix for now. I believe it is fine to release thanos like that and see if it impacts Amazon S3 as well.

Will keep you posted about Store and Compact.

[ahurtaud]

Ok so, it is definitely a memory Increase during uploads. The store component is not affected. Only the shipper is impacted. (so I believe sidecar, compact, receive and ruler)

I'll attached the profile for the compactor during upload, however it is less visible as it is using I guess lots of page cache or so for the downsampling and so all my memory is "used". We can still see minio-go putObjectMultipartStreamFromReadAt high.
profile-compact-019.pdf
This memory is part of RSS (subject to kubernetes OOM kill).

Big grafana dashboard

So I believe the minio-go issue linked above by @GiedriusS is the root cause.

I think, we will increase the memory limits for the components which are doing uploads.
Do you think it would be worth looking at it on minio-go project?

[GiedriusS]

I'm not sure if there is a way to avoid this behaviour from the caller:

- For size smaller than 5MiB PutObject automatically does a single atomic Put operation.
- For size larger than 5MiB PutObject automatically does a resumable multipart Put operation.
- For size input as -1 PutObject does a multipart Put operation until input stream reaches EOF.
  Maximum object size that can be uploaded through this operation will be 5TiB.

@harshavardhana sorry for pinging you here on an unrelated project but maybe it would be possible to control this new behaviour introduced in minio/minio-go#1357 with a flag or something? In our case, we are sure that the files we are trying to upload are immutable due to how TSDB works so that would help out a lot. 🤗

Another option is to revert the minio-go version upgrade ;/

[harshavardhana]

I'm not sure if there is a way to avoid this behaviour from the caller:

- For size smaller than 5MiB PutObject automatically does a single atomic Put operation.
- For size larger than 5MiB PutObject automatically does a resumable multipart Put operation.
- For size input as -1 PutObject does a multipart Put operation until input stream reaches EOF.
  Maximum object size that can be uploaded through this operation will be 5TiB.

@harshavardhana sorry for pinging you here on an unrelated project but maybe it would be possible to control this new behaviour introduced in minio/minio-go#1357 with a flag or something? In our case, we are sure that the files we are trying to upload are immutable due to how TSDB works so that would help out a lot.

@GiedriusS you can set the PartSize for each request to reduce the memory usage overall

NumThreads: 2, // upload two parts in parallel 
PartSize: 10 * humanize.MiByte, // use 10MiB per part size instead of default 128MiB

[harshavardhana]

I'm not sure if there is a way to avoid this behaviour from the caller:

- For size smaller than 5MiB PutObject automatically does a single atomic Put operation.
- For size larger than 5MiB PutObject automatically does a resumable multipart Put operation.
- For size input as -1 PutObject does a multipart Put operation until input stream reaches EOF.
  Maximum object size that can be uploaded through this operation will be 5TiB.

@harshavardhana sorry for pinging you here on an unrelated project but maybe it would be possible to control this new behaviour introduced in minio/minio-go#1357 with a flag or something? In our case, we are sure that the files we are trying to upload are immutable due to how TSDB works so that would help out a lot.

@GiedriusS you can set the PartSize for each request to reduce the memory usage overall

NumThreads: 2, // upload two parts in parallel 
PartSize: 10 * humanize.MiByte, // use 10MiB per part size instead of default 128MiB

#3935 something like this will reduce the memory usage by orders of magnitude.

[bwplotka]

Thanks all for reporting and drilling down. I think we narrowed down to the root cause: minio/minio-go#1357

I think it's quite odd that minio client by default for multipart uses 0.5GB of memory (4 workers by default). We would maybe like to use HTTP request for such large part, not necessary that much memory for it 🤔 Wonder if we can improve this on minio size first.

Also to me it looks like we need some even local benchmark for UploadFile with minio client 🤗

[bwplotka]

Let's check, I think I might have some ideas to help, commenting here: minio/mc#3376 (comment)

BTW thanks @harshavardhana for jumping and helping! 🤗

[bwplotka]

Looking on it, last thing until v0.19.0-rc.2 Will try to make some benchmarks

[bwplotka]

Performed some benchmarks:

	// Uploading 100MB with:
	// PartSize: 1024 * 1024 * 100 = BenchmarkUpload-12    	      84	 416025119 ns/op	421129826 B/op	     946 allocs/op  (400MB allocated for single op)
	// ParSize:  1024 * 1024 * 25  = BenchmarkUpload-12    	     100	 273648888 ns/op	106639830 B/op	    1576 allocs/op (100MB overallocated).
	// PartSize: 1024 * 1024 * 8   = BenchmarkUpload-12    	      97	 254052232 ns/op	35392395 B/op	    3078 allocs/op (Still 33MB allocated [8x4])

As expected @harshavardhana, wonder how it was before this change against race? 🤔

EDIT:

I reverted the race fix locally on my branch and the result is:

// PartSize: 1024 * 1024 * 100 = BenchmarkUpload-12    	      88	 282906636 ns/op	 1671249 B/op	     907 allocs/op
// PartSize: 1024 * 1024 * 8 =BenchmarkUpload-12    	     100	 300660587 ns/op	 2195440 B/op	    3044 allocs/op

The difference is significant.

Code:

// Copyright (c) The Thanos Authors.
// Licensed under the Apache License 2.0.

package s3_test

import (
	"bytes"
	"context"
	"strings"
	"testing"

	"github.com/cortexproject/cortex/integration/e2e"
	e2edb "github.com/cortexproject/cortex/integration/e2e/db"
	"github.com/go-kit/kit/log"
	"github.com/thanos-io/thanos/pkg/objstore/s3"
	"github.com/thanos-io/thanos/test/e2e/e2ethanos"

	"github.com/thanos-io/thanos/pkg/testutil"
)

// Regression benchmark for https://github.com/thanos-io/thanos/issues/3917.
func BenchmarkUpload(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()

	s, err := e2e.NewScenario("e2e_bench_mino_client")
	testutil.Ok(b, err)
	b.Cleanup(e2ethanos.CleanScenario(b, s))

	const bucket = "test"
	m := e2edb.NewMinio(8080, bucket)
	testutil.Ok(b, s.StartAndWaitReady(m))

	bkt, err := s3.NewBucketWithConfig(log.NewNopLogger(), s3.Config{
		Bucket:    bucket,
		AccessKey: e2edb.MinioAccessKey,
		SecretKey: e2edb.MinioSecretKey,
		Endpoint:  m.HTTPEndpoint(),
		Insecure:  true,
	}, "test-feed")
	testutil.Ok(b, err)

	buf := bytes.Buffer{}
	buf.Grow(1028 * 1028 * 100) // 100MB.
	word := "abcdefghij"
	for i := 0; i < buf.Cap()/len(word); i++ {
		_, _ = buf.WriteString(word)
	}
	str := buf.String()

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		testutil.Ok(b, bkt.Upload(ctx, "test", strings.NewReader(str)))
	}
}

[bwplotka]

I am considering using fork for release, and fix this later on master. Any objections?

[bwplotka]

Discussing minio/mc#3376 for long term approach with super helpful @harshavardhana

[ahurtaud]

That is fine for me, also we should rename and re-label the issue properly (no 0.19 tag then, and affecting only the components doing S3 uploads: sidecar, compact, receive, ruler?).
Feel free to rename to something more fitting the issue.