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.

Sign up for GitHub

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

Jump to bottom

Build with Go 1.20 #6126

Merged
merged 8 commits into from
Mar 6, 2023
Merged

Build with Go 1.20 #6126

merged 8 commits into from
Mar 6, 2023

Conversation

sylr
Copy link
Contributor

@sylr sylr commented Feb 15, 2023 •
edited
Loading

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

@sylr sylr changed the title Use Go 1.20 Build with Go 1.20 Feb 15, 2023
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 15, 2023
@sylr sylr force-pushed the go1.20 branch 2 times, most recently from 19a80cd to 9b83c0b Compare February 15, 2023 10:09
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 15, 2023
@sylr sylr force-pushed the go1.20 branch 3 times, most recently from 9868d37 to 0054ece Compare February 15, 2023 13:00
@sylr sylr marked this pull request as ready for review February 15, 2023 16:38
@sylr sylr force-pushed the go1.20 branch 3 times, most recently from 5c24a0a to 40fb611 Compare February 23, 2023 07:24
@sylr
Copy link
Contributor Author

sylr commented Feb 23, 2023

@bwplotka @GiedriusS I tried to debug the failing test locally and it turns out that the chunk size varies at each run.

As I have no understanding of prometheus storage backend I'd like to hand over this to someone competent in the matter.

@sylr
Build with Go 1.20
b44cd0b 
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@sylr
Upgrade codeql workflow
3eb840b 
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@sylr
Upgrade go4.org/unsafe/assume-no-moving-gc
a4c218b 
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@sylr
Update Changelog
0c21db0 
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@sylr
Upgrade golangci-lint
6be060f 
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@sylr
Sync github workflows
a65fdc0 
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@sylr
Fix indentation in documentation code snippet
92ba8dc 
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
@saswatamcode
Copy link
Member

Hmm I'm not sure why TestUpload consistently fails here. It passes locally. Will take a deeper look

@saswatamcode
Copy link
Member

saswatamcode commented Mar 5, 2023 •
edited
Loading

Ok I was able to reproduce this after upgrading to Go 1.20.1 locally. The reason TestUpload has become nondeterministic seems to be due to the way we create blocks for testing. We use rand.Float64() for the samples:

thanos/pkg/testutil/e2eutil/prometheus.go

Line 509 in 5e3ecf6

_, err = app.Append(0, lset, t, rand.Float64())
which was deterministic and always produced the same random values in the same order

But as of Go 1.20 "math/rand" package automatically seeds the global random number generator which causes our block chunk file size to vary: https://tip.golang.org/doc/go1.20#minor_library_changes

I think we just need to seed rand here 🙂

Edit: Trying this out with commit: 15217bc
Looks like it worked! 🎉

@saswatamcode
Make createBlock chunk size deterministic
15217bc 
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@sylr
Copy link
Contributor Author

sylr commented Mar 5, 2023

@saswatamcode thank you!

saswatamcode
saswatamcode approved these changes Mar 5, 2023
View reviewed changes
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

LGTM!

GiedriusS
GiedriusS approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

🥇

@GiedriusS GiedriusS merged commit 8da34a1 into thanos-io:main Mar 6, 2023
jan--f pushed a commit to jan--f/thanos that referenced this pull request Apr 18, 2023
@sylr @jan--f
Build with Go 1.20 (thanos-io#6126)
af10708 
* Build with Go 1.20

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>

* Upgrade codeql workflow

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>

* Upgrade go4.org/unsafe/assume-no-moving-gc

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>

* Update Changelog

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>

* Upgrade golangci-lint

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>

* Sync github workflows

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>

* Fix indentation in documentation code snippet

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>

* Make createBlock chunk size deterministic

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

---------

Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Co-authored-by: Saswata Mukherjee <saswataminsta@yahoo.com>
(cherry picked from commit 8da34a1)
openshift-merge-robot added a commit to openshift/thanos that referenced this pull request Apr 19, 2023
@openshift-merge-robot
Merge pull request #106 from jan--f/backport-go1.20-builds
e5ca784 
OCPBUGS-11928: Build with Go 1.20 (thanos-io#6126)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift sonatype-lift left review comments

@GiedriusS GiedriusS GiedriusS approved these changes

@saswatamcode saswatamcode saswatamcode approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sylr @saswatamcode @GiedriusS