Added CI testing against S3 bucket. (#1648)

* Added CI testing against S3 bucket.

* Again, only for master/release builds and PRs from person with write-access to Thanos.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

* Fixed multipart upload for S3 for smaller objects.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

.circleci/config.yml

@@ -26,22 +26,21 @@ jobs:
           command: |
             if ! [ -z ${GCP_PROJECT} ]; then
               echo $GOOGLE_APPLICATION_CREDENTIALS_CONTENT > $GOOGLE_APPLICATION_CREDENTIALS
-              echo "Awesome! GCS integration tests are enabled."
+              echo "Awesome! GCS and S3 AWS integration tests are enabled."
             fi
       - run: make deps
       - run: make lint
       - run: make check-docs
       - run: make format
       - run:
           name: "Run all tests"
-          # TODO(bwplotka): Setup some S3 tests for CI.
           # taskset sets CPU affinity to 2 (current CPU limit).
           command: |
             if [ -z ${GCP_PROJECT} ]; then
               taskset 2 make test-local
               exit
             fi
-            taskset 2 make test-only-gcs
+            taskset 2 make test-ci
 
   # Cross build is needed for publish_release but needs to be done outside of docker.
   cross_build:

Makefile

@@ -206,23 +206,23 @@ test: check-git install-deps
 	@echo ">> running all tests. Do export THANOS_SKIP_GCS_TESTS='true' or/and THANOS_SKIP_S3_AWS_TESTS='true' or/and THANOS_SKIP_AZURE_TESTS='true' and/or THANOS_SKIP_SWIFT_TESTS='true' and/or THANOS_SKIP_TENCENT_COS_TESTS='true' if you want to skip e2e tests against real store buckets"
 	@go test $(shell go list ./... | grep -v /vendor/);
 
-.PHONY: test-only-gcs
-test-only-gcs: export THANOS_SKIP_S3_AWS_TESTS = true
-test-only-gcs: export THANOS_SKIP_AZURE_TESTS = true
-test-only-gcs: export THANOS_SKIP_SWIFT_TESTS = true
-test-only-gcs: export THANOS_SKIP_TENCENT_COS_TESTS = true
-test-only-gcs:
-	@echo ">> Skipping S3 tests"
+.PHONY: test-ci
+test-ci: export THANOS_SKIP_AZURE_TESTS = true
+test-ci: export THANOS_SKIP_SWIFT_TESTS = true
+test-ci: export THANOS_SKIP_TENCENT_COS_TESTS = true
+test-ci:
 	@echo ">> Skipping AZURE tests"
 	@echo ">> Skipping SWIFT tests"
 	@echo ">> Skipping TENCENT tests"
 	$(MAKE) test
 
 .PHONY: test-local
 test-local: export THANOS_SKIP_GCS_TESTS = true
+test-local: export THANOS_SKIP_S3_AWS_TESTS = true
 test-local:
 	@echo ">> Skipping GCE tests"
-	$(MAKE) test-only-gcs
+	@echo ">> Skipping S3 tests"
+	$(MAKE) test-ci
 
 # install-deps installs dependencies for e2e tetss.
 # It installs supported versions of Prometheus and alertmanager to test against in e2e.

cmd/thanos/bucket.go

@@ -178,6 +178,7 @@ func registerBucketLs(m map[string]setupFunc, root *kingpin.CmdClause, name stri
 
 		var (
 			format     = *output
+			objects    = 0
 			printBlock func(id ulid.ULID) error
 		)
 
@@ -234,13 +235,18 @@ func registerBucketLs(m map[string]setupFunc, root *kingpin.CmdClause, name stri
 			}
 		}
 
-		return bkt.Iter(ctx, "", func(name string) error {
+		if err := bkt.Iter(ctx, "", func(name string) error {
 			id, ok := block.IsBlockDir(name)
 			if !ok {
 				return nil
 			}
+			objects++
 			return printBlock(id)
-		})
+		}); err != nil {
+			return errors.Wrap(err, "iter")
+		}
+		level.Info(logger).Log("msg", "ls done", "objects", objects)
+		return nil
 	}
 }
 

docs/storage.md

@@ -55,11 +55,11 @@ Current object storage client implementations:
 
 | Provider             | Maturity | Auto-tested on CI | Maintainers |
 |----------------------|-------------------|-----------|---------------|
-| [Google Cloud Storage](#gcs) | Stable  (production usage)             | yes       | @bwplotka   |
-| [AWS/S3](#s3) | Stable  (production usage)               | yes        | @bwplotka          |
-| [Azure Storage Account](#azure) | Stable  (production usage) | yes       | @vglafirov   |
-| [OpenStack Swift](#openstack-swift)      | Beta  (working PoCs, testing usage)               | no        | @sudhi-vm   |
-| [Tencent COS](#tencent-cos)          | Beta  (testing usage)                   | no        | @jojohappy          |
+| [Google Cloud Storage](./storage.md#gcs) | Stable  (production usage)             | yes       | @bwplotka   |
+| [AWS/S3](./storage.md#s3) | Stable  (production usage)               | yes        | @bwplotka          |
+| [Azure Storage Account](./storage.md#azure) | Stable  (production usage) | yes       | @vglafirov   |
+| [OpenStack Swift](./storage.md#openstack-swift)      | Beta  (working PoCs, testing usage)               | no        | @sudhi-vm   |
+| [Tencent COS](./storage.md#tencent-cos)          | Beta  (testing usage)                   | no        | @jojohappy          |
 
 NOTE: Currently Thanos requires strong consistency (write-read) for object store implementation.
 
@@ -85,12 +85,12 @@ config:
   secret_key: ""
   put_user_metadata: {}
   http_config:
-    idle_conn_timeout: 0s
-    response_header_timeout: 0s
+    idle_conn_timeout: 90s
+    response_header_timeout: 2m
     insecure_skip_verify: false
   trace:
     enable: false
-  part_size: 0
+  part_size: 134217728
 ```
 
 At a minimum, you will need to provide a value for the `bucket`, `endpoint`, `access_key`, and `secret_key` keys. The rest of the keys are optional.

pkg/objstore/objtesting/acceptance_e2e_test.go

@@ -98,7 +98,8 @@ func TestObjStore_AcceptanceTest_e2e(t *testing.T) {
 
 		testutil.Ok(t, bkt.Delete(ctx, "id1/obj_2.some"))
 		// Delete is expected to fail on non existing object.
-		testutil.NotOk(t, bkt.Delete(ctx, "id1/obj_2.some"))
+		// NOTE: Don't rely on this. S3 is not complying with this as GCS is.
+		// testutil.NotOk(t, bkt.Delete(ctx, "id1/obj_2.some"))
 
 		// Can we iter over items from id1/ dir and see obj2 being deleted?
 		seen = []string{}

pkg/objstore/objtesting/foreach.go

@@ -37,7 +37,7 @@ func ForeachStore(t *testing.T, testFn func(t testing.TB, bkt objstore.Bucket))
 		testutil.Ok(t, err)
 
 		ok := t.Run("gcs", func(t *testing.T) {
-			// TODO(bplotka): Add leaktest when https://github.com/GoogleCloudPlatform/google-cloud-go/issues/1025 is resolved.
+			// TODO(bwplotka): Add leaktest when https://github.com/GoogleCloudPlatform/google-cloud-go/issues/1025 is resolved.
 			testFn(t, bkt)
 		})
 		closeFn()
@@ -48,11 +48,10 @@ func ForeachStore(t *testing.T, testFn func(t testing.TB, bkt objstore.Bucket))
 		t.Log("THANOS_SKIP_GCS_TESTS envvar present. Skipping test against GCS.")
 	}
 
-	// Optional S3 AWS.
-	// TODO(bwplotka): Prepare environment & CI to run it automatically.
+	// Optional S3.
 	if _, ok := os.LookupEnv("THANOS_SKIP_S3_AWS_TESTS"); !ok {
 		// TODO(bwplotka): Allow taking location from envvar.
-		bkt, closeFn, err := s3.NewTestBucket(t, "eu-west-1")
+		bkt, closeFn, err := s3.NewTestBucket(t, "us-west-2")
 		testutil.Ok(t, err)
 
 		ok := t.Run("aws s3", func(t *testing.T) {

pkg/objstore/s3/s3.go

@@ -32,9 +32,16 @@ import (
 // DirDelim is the delimiter used to model a directory structure in an object store bucket.
 const DirDelim = "/"
 
-// Minimum file size after which an HTTP multipart request should be used to upload objects to storage.
-// Set to 128 MiB as in the minio client.
-const defaultMinPartSize = 1024 * 1024 * 128
+var DefaultConfig = Config{
+	PutUserMetadata: map[string]string{},
+	HTTPConfig: HTTPConfig{
+		IdleConnTimeout:       model.Duration(90 * time.Second),
+		ResponseHeaderTimeout: model.Duration(2 * time.Minute),
+	},
+	// Minimum file size after which an HTTP multipart request should be used to upload objects to storage.
+	// Set to 128 MiB as in the minio client.
+	PartSize: 1024 * 1024 * 128,
+}
 
 // Config stores the configuration for s3 bucket.
 type Config struct {
@@ -49,7 +56,8 @@ type Config struct {
 	PutUserMetadata map[string]string `yaml:"put_user_metadata"`
 	HTTPConfig      HTTPConfig        `yaml:"http_config"`
 	TraceConfig     TraceConfig       `yaml:"trace"`
-	PartSize        uint64            `yaml:"part_size"`
+	// PartSize used for multipart upload. Only used if uploaded object size is known and larger than configured PartSize.
+	PartSize uint64 `yaml:"part_size"`
 }
 
 type TraceConfig struct {
@@ -75,23 +83,11 @@ type Bucket struct {
 
 // 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),
-		ResponseHeaderTimeout: model.Duration(2 * time.Minute),
-	}
-	config := Config{HTTPConfig: defaultHTTPConfig}
+	config := DefaultConfig
 	if err := yaml.Unmarshal(conf, &config); err != nil {
 		return Config{}, err
 	}
 
-	if config.PutUserMetadata == nil {
-		config.PutUserMetadata = make(map[string]string)
-	}
-
-	if config.PartSize == 0 {
-		config.PartSize = defaultMinPartSize
-	}
-
 	return config, nil
 }
 
@@ -314,16 +310,21 @@ func (b *Bucket) guessFileSize(name string, r io.Reader) int64 {
 // Upload the contents of the reader as an object into the bucket.
 func (b *Bucket) Upload(ctx context.Context, name string, r io.Reader) error {
 	// TODO(https://github.com/thanos-io/thanos/issues/678): Remove guessing length when minio provider will support multipart upload without this.
-	fileSize := b.guessFileSize(name, r)
+	size := b.guessFileSize(name, r)
 
+	// partSize cannot be larger than object size.
+	partSize := b.partSize
+	if size < int64(partSize) {
+		partSize = 0
+	}
 	if _, err := b.client.PutObjectWithContext(
 		ctx,
 		b.name,
 		name,
 		r,
-		fileSize,
+		size,
 		minio.PutObjectOptions{
-			PartSize:             b.partSize,
+			PartSize:             partSize,
 			ServerSideEncryption: b.sse,
 			UserMetadata:         b.putUserMetadata,
 		},

scripts/cfggen/main.go

@@ -30,7 +30,7 @@ var (
 	bucketConfigs = map[client.ObjProvider]interface{}{
 		client.AZURE: azure.Config{},
 		client.GCS:   gcs.Config{},
-		client.S3:    s3.Config{},
+		client.S3:    s3.DefaultConfig,
 		client.SWIFT: swift.SwiftConfig{},
 		client.COS:   cos.Config{},
 	}