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

Change default server.http-listen-port to 8080 #871

Merged
merged 10 commits into from
Jan 26, 2022

Conversation

dimitarvdimitrov
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov commented Jan 24, 2022

Signed-off-by: Dimitar Dimitrov [email protected]

What this PR does:

Sets the default -server.http-listen-port to 8080.

Which issue(s) this PR fixes:

Fixes https://github.com/grafana/mimir-squad/issues/481

Checklist

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

@dimitarvdimitrov dimitarvdimitrov changed the title WIP: Change default server.http-listen-port to 8080 Change default server.http-listen-port to 8080 Jan 24, 2022
@dimitarvdimitrov dimitarvdimitrov marked this pull request as ready for review January 24, 2022 19:08
@dimitarvdimitrov
Copy link
Contributor Author

This is now ready for review.

@Logiraptor
Copy link
Contributor

cc @jdbaldry @osg-grafana since this will be a significant breaking change for the migration guide

@pstibrany
Copy link
Member

pstibrany commented Jan 25, 2022

We should change the cmd/mimir/Dockerfile too.

@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/http_listen_port_default_8080 branch from d9c552a to 778506f Compare January 25, 2022 10:42
containerPort.newNamed(name='http-metrics', containerPort=80),
containerPort.newNamed(name='http-metrics', containerPort=8080),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure what will happen with these changes after merge. Will they get immediately pulled into deployment tools and applied? If so, that will break all current deployments. Can someone confirm this won't happen?

Copy link
Member

Choose a reason for hiding this comment

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

It won't happen without someone's approval. However we still need to support port 80 until we switch all our deployed images to use port 8080 (and not just Mimir's image but GEM's image too). It may be best to have a config option for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after the weekly call we found out that what the image exposes isn't tied to what the container can listen on. So this shouldn't be a concern anymore, correct @pstibrany?

To my understanding we should still create a jsonnet "revert" PR to set the server port to 80. So once this PR (871) is merged and deployed, the change in the default port wouldn't affect the running services. I'll get started on that PR.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see -server.http-listen-port being overridden in our jsonnet, so restarted components will actually start using new default port 8080. I think we want to update all (!) component definitions in jsonnet to keep using port 80 via this command line flag. This should also be reflected in named ports definitions. (I'd suggest to make port 80 configurable via field in _config).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a similar config in 117b90c

@dimitarvdimitrov
Copy link
Contributor Author

thanks for spotting this @pstibrany, I found a few other mentions of port 80 and pushed an update in 778506f

I'm unsure about some of the changes though. Left a comment in operations/mimir/common.libsonnet

@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/http_listen_port_default_8080 branch from 37691cb to 117b90c Compare January 26, 2022 11:28
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Thanks for addressing my feedback! Before merging I would be glad if @pstibrany could take another look, since he already commented on the PR.

@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/http_listen_port_default_8080 branch from cee276d to 8a96513 Compare January 26, 2022 13:22
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/http_listen_port_default_8080 branch from 8a96513 to fa71733 Compare January 26, 2022 14:26
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Good job!

@pracucci pracucci merged commit 469934e into main Jan 26, 2022
@pracucci pracucci deleted the dimitar/http_listen_port_default_8080 branch January 26, 2022 14:45
dimitarvdimitrov added a commit that referenced this pull request Jan 27, 2022
This complements the change by #871 in backward_compatibility.go

Signed-off-by: Dimitar Dimitrov <[email protected]>
pracucci pushed a commit that referenced this pull request Jan 27, 2022
This complements the change by #871 in backward_compatibility.go

Signed-off-by: Dimitar Dimitrov <[email protected]>
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.

4 participants