Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update github.com/thanos-io/objstore #6061

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Conversation

andyasp
Copy link
Contributor

@andyasp andyasp commented Sep 18, 2023

What this PR does

Updates objstore, which required code changes to bring forward. The motivation for the update is to bring in a GCS listing optimization.

A choice I made was to not expose the storage_connection_string option objstore added for Azure. The target use case for it seems to be testing with Azurite, so my gut feeling was that it wasn't currently necessary and it could always be added later.

Which issue(s) this PR fixes or relates to

Fixes: N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@andyasp andyasp marked this pull request as ready for review September 18, 2023 22:06
@andyasp andyasp requested review from grafanabot and a team as code owners September 18, 2023 22:06
@andyasp andyasp marked this pull request as draft September 18, 2023 22:13
@andyasp
Copy link
Contributor Author

andyasp commented Sep 19, 2023

Test failure looks legitimate and there's a data race in the logs too. I'll take a closer look tomorrow.

@pracucci
Copy link
Collaborator

A choice I made was to not expose the storage_connection_string option objstore added for Azure. The target use case for it seems to be testing with Azurite, so my gut feeling was that it wasn't currently necessary and it could always be added later.

LGTM. As you already said, we can always add it later if needed.

@andyasp
Copy link
Contributor Author

andyasp commented Sep 19, 2023

Edit: I had an incorrect theory before and modified this post.

This looks like an issue with objstore (debatably mino-go) upstream.

Specifically, in putObjectMultipartStreamNoLength (this is being used since objstore introduced a io.nopCloser which can't be used to guess the size of the upload, which seems like its own issue) minio surprisingly modifies the UserMetadata map in the PutObjectOptions that is passed in:

	if !opts.SendContentMd5 {
		if opts.UserMetadata == nil {
			opts.UserMetadata = make(map[string]string, 1)
		}
		opts.UserMetadata["X-Amz-Checksum-Algorithm"] = "CRC32C"
	}

	// Initiate a new multipart upload.
	uploadID, err := c.newUploadID(ctx, bucketName, objectName, opts)
	if err != nil {
		return UploadInfo{}, err
	}
	delete(opts.UserMetadata, "X-Amz-Checksum-Algorithm")

What map does objstore pass in? The putUserMetadata stored in the Bucket struct:

// Bucket implements the store.Bucket interface against s3-compatible APIs.
type Bucket struct {
	logger          log.Logger
	name            string
	client          *minio.Client
	defaultSSE      encrypt.ServerSide
	putUserMetadata map[string]string
	storageClass    string
	partSize        uint64
	listObjectsV1   bool
}
	if _, err := b.client.PutObject(
		ctx,
		b.name,
		name,
		r,
		size,
		minio.PutObjectOptions{
			PartSize:             partSize,
			ServerSideEncryption: sse,
			UserMetadata:         b.putUserMetadata,
			StorageClass:         b.storageClass,
			// 4 is what minio-go have as the default. To be certain we do micro benchmark before any changes we
			// ensure we pin this number to four.
			// TODO(bwplotka): Consider adjusting this number to GOMAXPROCS or to expose this in config if it becomes bottleneck.
			NumThreads: 4,
		},
	); err != nil {
		return errors.Wrap(err, "upload s3 object")
	}

@andyasp
Copy link
Contributor Author

andyasp commented Sep 20, 2023

Opened thanos-io/objstore#77 and thanos-io/objstore#78

@andyasp andyasp marked this pull request as ready for review September 21, 2023 14:27
@andyasp
Copy link
Contributor Author

andyasp commented Sep 21, 2023

The above PRs were merged and I revendored. The tests are now passing.

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@56quarters 56quarters merged commit 9f671b5 into main Sep 21, 2023
28 checks passed
@56quarters 56quarters deleted the aasp/update-objstore branch September 21, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants