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

Move alertmanager-proxy port to 9095 #411

Merged
merged 2 commits into from Jul 25, 2019

Conversation

paulfantom
Copy link
Contributor

Needed to merge openshift/prometheus-operator#35 as we need to move alertmanager-proxy to not be on port 9094.

/cc @brancz @lilic @s-urbaniak

We need to free port 9094 as we are moving alertmanager mesh to this
port from 6783
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 25, 2019
@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: paulfantom, s-urbaniak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [paulfantom,s-urbaniak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c83d746 into openshift:master Jul 25, 2019
@paulfantom paulfantom deleted the am_port branch July 25, 2019 11:22
@brancz
Copy link
Contributor

brancz commented Jul 25, 2019

Would it be possible to instead of changing the port to just bind against the pod ip? Alertmanager binds against localhost iirc. I feel that would be a less breaking and less unexpected setup.

@s-urbaniak
Copy link
Contributor

ping @paulfantom can you comment on #411 (comment)?

@brancz
Copy link
Contributor

brancz commented Aug 9, 2019

For context, we've had bugs opened for similar changes, and this one is rather extreme, as if someone integrated with the alertmanager endpoints (which we actively encourage), then we've just broken their alerting pipeline.

@paulfantom
Copy link
Contributor Author

We can roll it back and even provide backwards compatibility by forcing alertmanager to advertise gossip not on port 9094 but on 6783 by using --cluster.listen-address alertmanager parameter. However this would need some changes in prometheus-operator, so maybe you are right and let's just bind against the pod ip.

I didn't want to have two different things on port with the same number as this might be confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants