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

Update Prometheus to cd73b3d33e064bbd846fc7a26dc8c313d46af382 #2596

Merged
merged 6 commits into from
May 18, 2020
Merged

Update Prometheus to cd73b3d33e064bbd846fc7a26dc8c313d46af382 #2596

merged 6 commits into from
May 18, 2020

Conversation

codesome
Copy link
Contributor

@codesome codesome commented May 13, 2020 •
edited
Loading

As we are blocked on some breaking changes to be addressed before we can upgrade Prometheus to the latest master, this PR attempts to upgrade Prometheus upto this commit prometheus/prometheus@cd73b3d which hopefully does not have any breaking changes and helps the TSDB block based ingesters in Cortex :)

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

Changes

Verification

@codesome
Copy link
Contributor Author

codesome commented May 13, 2020 •
edited
Loading

I would have expected go.sum file to have changed after mod tidy and mod vendor, but that is not the case. What is more worrying is that, go.mod and go.sum in upstream point to different commit. Upstream go.mod says b90be6f32a33 and go.sum says 1e64d757f711, and the CI picks up 1e64d757f711.

@codesome
Copy link
Contributor Author

Marco pointed me out to the override in go.mod, makes sense now, removing it and trying

@pracucci
Copy link
Contributor

There's an overrided in go.mod replace which you should change as well.

@codesome codesome force-pushed the update-prometheus branch 2 times, most recently from 18c50eb to fa70438 Compare May 14, 2020 07:08
@codesome
Fix all the breaking things
2bbe4a8 
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome codesome force-pushed the update-prometheus branch 3 times, most recently from 370cb9c to d01df33 Compare May 14, 2020 10:10
@codesome
Fix tests
caf78bd 
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome
Merge remote-tracking branch 'upstream/master' into update-prometheus
6e7a9c6 
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
bwplotka
bwplotka reviewed May 14, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks 💪

Some suggestions, otherwise lgtm.

Also @codesome we usually put in CHANGELOG such update as it usually adds a lot. Do you think there was something significant we pulled with this.

Also we need to pull more often (:

cmd/thanos/rule.go Outdated Show resolved Hide resolved
cmd/thanos/tools.go Show resolved Hide resolved
cmd/thanos/tools.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
pkg/compact/downsample/downsample.go Outdated Show resolved Hide resolved
pkg/receive/multitsdb.go Show resolved Hide resolved
pkg/rule/api/v1.go Show resolved Hide resolved
pkg/rule/rule.go Outdated Show resolved Hide resolved
pkg/rule/rule.go
@@ -132,10 +143,10 @@ func (r RuleGroup) MarshalYAML() (interface{}, error) {
}

rs := struct {
RuleGroup rulefmt.RuleGroup `yaml:",inline"`
PartialResponseStrategy *string `yaml:"partial_response_strategy,omitempty"`
RuleGroup PromRuleGroup `yaml:",inline"`
Copy link
Member

Choose a reason for hiding this comment

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

BTW this will kill us @s-urbaniak XD

@bwplotka
Copy link
Member

As we are blocked on some breaking changes to be addressed before we can upgrade Prometheus to the latest master, this PR attempts to upgrade Prometheus upto this commit prometheus/prometheus@cd73b3d which hopefully does not have any breaking changes and helps the TSDB block based ingesters in Cortex :)

Why not up to latest?

I guess then @krasi-georgiev want to take over?

@codesome
Copy link
Contributor Author

Why not up to latest?

That has more breaking changes which will take time to fix here, and idea was to get some improvements from Prometheus soon. And yes, Krasi will take over after this one.

@codesome
Copy link
Contributor Author

Also, I have some ugly hacks in place right now to fix some breaking changes. Once I fix the e2e tests, I plan to review stuff again and do a cleanup.
One such breaking change was replacing Rule with RuleNode in a RuleGroup upstream. The RuleNode upstream uses a yaml Node instead of a string like in Rule, which is painful when using it here, so I added a PromRuleGroup which does not use RuleNode.

@codesome
Copy link
Contributor Author

Do you think there was something significant we pulled with this.

I will take a look at the changelogs and add the significate changes :)

@bwplotka
Copy link
Member

This makes totally sense @codesome , let's do step by step. Thanks for helping ❤️

@codesome codesome force-pushed the update-prometheus branch 3 times, most recently from 832e018 to 32392d7 Compare May 15, 2020 10:25
bwplotka
bwplotka approved these changes May 15, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Awsome, LGTM! We have some CI failures, otherwise good. Thanks 💪

@bwplotka
Copy link
Member

Looks like essentially we have something wrong with flags still. e2e test:

=== CONT  TestReceive/hashring
Starting receive-1
receive-1: Error parsing commandline arguments: time: unknown unit d in duration 15d
receive-1: usage: thanos receive [<flags>]
receive-1: Accept Prometheus remote write API requests and write to local tsdb`

@codesome
Copy link
Contributor Author

I guess its the modelDuration method after all. I will fall back to using them :)

@codesome
Fixes
4dc9706 
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome
Copy link
Contributor Author

Tests are passing and changelog entries added!

kakkoyun
kakkoyun approved these changes May 18, 2020
View reviewed changes
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

CHANGELOG.md Outdated Show resolved Hide resolved
@codesome
Add CHANGELOG entry
7f45a48 
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@bwplotka
Copy link
Member

@codesome looks like some conflicts this morning. Let's fix & merge, and then I will do 0.13 release (: 💪

Thanks!

squat
squat reviewed May 18, 2020
View reviewed changes
cmd/thanos/tools.go Outdated Show resolved Hide resolved
@codesome
Merge remote-tracking branch 'upstream/master' into update-prometheus
b0f7eec 
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@bwplotka
Copy link
Member

LGTM, Let's merge on green (:

@bwplotka bwplotka merged commit 748740d into thanos-io:master May 18, 2020
@bwplotka
Copy link
Member

bwplotka commented May 18, 2020 •
edited
Loading

Thanks!

@codesome @pracucci @pstibrany it unblocks Cortex I guess.

@kakkoyun kakkoyun mentioned this pull request May 27, 2020
Closed
1 task
@pracucci pracucci mentioned this pull request Jun 10, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squat squat squat left review comments

@kakkoyun kakkoyun kakkoyun approved these changes

@bwplotka bwplotka bwplotka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@codesome @pracucci @bwplotka @kakkoyun @squat