-
Notifications
You must be signed in to change notification settings - Fork 817
Vendor update #1510
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
Vendor update #1510
Conversation
I have opened weaveworks/common#161 for some conflicts. |
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
The |
Do you think the flakyness is caused by the vendor update? |
I plan to investigate that sometime soon. If it was flaky before we would have seen it in other places too, so it might be the vendor update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need convincing that alertmanager will still work after this, and instructions on how to upgrade.
go.mod
Outdated
github.com/prometheus/common v0.3.0 | ||
github.com/prometheus/prometheus v0.0.0-20190417125241-3cc5f9d88062 | ||
github.com/prometheus/tsdb v0.7.2-0.20190506134726-2ae028114c89 | ||
github.com/prometheus/alertmanager v0.17.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. All of the gossip/mesh protocols have changed in this upgrade. At a minimum I would want to see what is necessary to upgrade the alertmanager after this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prometheus uses v0.17.0 of alertmanager, hence this gets updated automatically if we want to update prometheus. Also the changes that I did to pkg/alertmanager
was not final (was randomly making changes to fit the new API). It still needs testing and verifying.
[This PR is still in a Draft stage as I have to verify the changes to alertmanager package and see that it works fine] |
Plugging this here prometheus/prometheus#5813. I am going to decouple alertmanager from prometheus and vendor it here instead of bumping the alertmanager version. |
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
Now I have vendored against This keeps the alertmanager version of cortex unchanged. |
Signed-off-by: Ganesh Vernekar <[email protected]>
Signed-off-by: Ganesh Vernekar <[email protected]>
In the same way, alertmanager 0.13.0 was not totally compatible with client_golang 1.0.0. And the latest Prometheus would pull it in. I created another branch of alertmanager called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks sane to me. We talked at the community meeting and decided that using the branches was ok for now to not block Prometheus upgrades.
@@ -117,6 +117,7 @@ func buildNotifierConfig(rulerConfig *Config) (*config.Config, error) { | |||
} | |||
} | |||
amConfig := &config.AlertmanagerConfig{ | |||
APIVersion: config.AlertmanagerAPIVersionV2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this appears to break alerts completely, since the v2 endpoint is not wired up - #1604
It includes this query optimization.