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

change machine-config-server port #1180

Merged
merged 1 commit into from
Feb 12, 2019
Merged

change machine-config-server port #1180

merged 1 commit into from
Feb 12, 2019

Conversation

kikisdeliveryservice
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice commented Feb 1, 2019 •
edited
Loading

Change machine-config-server port from 49500 -> 22623
to avoid conflict with local port and node port ranges.

Closes machine-config-operator issue: 166
Requires: openshift/machine-config-operator#368

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 1, 2019
@kikisdeliveryservice kikisdeliveryservice mentioned this pull request Feb 1, 2019
Merged
@kikisdeliveryservice
Copy link
Contributor Author

cc: @abhinavdahiya

@kikisdeliveryservice kikisdeliveryservice changed the title change machine-config-server port WIP: change machine-config-server port Feb 1, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 1, 2019
@cgwalters
Copy link
Member

One thing I hadn't considered too is that if we want to change this port in the future, it will also need changes in firewalling (AWS: security groups). Do we have those under cluster management today?

If we're going to pull the trigger on this port we should probably be really sure that 22623 is what we want and can stick with.

@cgwalters
Copy link
Member

Code changes seem good to me offhand.

@wking
Copy link
Member

wking commented Feb 4, 2019

The OpenStack folks seem to have a few 49500 references in their HAProxy watcher:

$ git grep 49500 origin/pr/1180 -- data
origin/pr/1180:data/data/openstack/lb/main.tf:    source = "data:,listen%20${var.cluster_name}-api-80%0D%0A%20%20%20%20bind%200.0.0.0%3A80%0D%0A%20%20%20%20mode%20tcp%0D%0A%20%20%20%20stats%20enable%0D%0A%20%20%20%20stats%20uri%20%2Fhaproxy%3Fstatus%0D%0A%20%20%20%20balance%20roundrobin%0D%0A%20%20%20%20server%20${var.cluster_name}-bootstrap%20${var.cluster_name}-bootstrap.${var.cluster_domain}%3A80%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-0%20${var.cluster_name}-master-0.${var.cluster_domain}%3A80%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-1%20${var.cluster_name}-master-1.${var.cluster_domain}%3A80%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-2%20${var.cluster_name}-master-2.${var.cluster_domain}%3A80%20check%0D%0A%0D%0Alisten%20${var.cluster_name}-api-6443%0D%0A%20%20%20%20bind%200.0.0.0%3A6443%0D%0A%20%20%20%20mode%20tcp%0D%0A%20%20%20%20stats%20enable%0D%0A%20%20%20%20stats%20uri%20%2Fhaproxy%3Fstatus%0D%0A%20%20%20%20balance%20roundrobin%0D%0A%20%20%20%20server%20${var.cluster_name}-bootstrap%20${var.cluster_name}-bootstrap.${var.cluster_domain}%3A6443%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-0%20${var.cluster_name}-master-0.${var.cluster_domain}%3A6443%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-1%20${var.cluster_name}-master-1.${var.cluster_domain}%3A6443%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-2%20${var.cluster_name}-master-2.${var.cluster_domain}%3A6443%20check%0D%0A%0D%0Alisten%20${var.cluster_name}-api-443%0D%0A%20%20%20%20bind%200.0.0.0%3A443%0D%0A%20%20%20%20mode%20tcp%0D%0A%20%20%20%20stats%20enable%0D%0A%20%20%20%20stats%20uri%20%2Fhaproxy%3Fstatus%0D%0A%20%20%20%20balance%20roundrobin%0D%0A%20%20%20%20server%20${var.cluster_name}-bootstrap%20${var.cluster_name}-bootstrap.${var.cluster_domain}%3A443%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-0%20${var.cluster_name}-master-0.${var.cluster_domain}%3A443%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-1%20${var.cluster_name}-master-1.${var.cluster_domain}%3A443%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-2%20${var.cluster_name}-master-2.${var.cluster_domain}%3A443%20check%0D%0A%0D%0Alisten%20${var.cluster_name}-api-49500%0D%0A%20%20%20%20bind%200.0.0.0%3A49500%0D%0A%20%20%20%20mode%20tcp%0D%0A%20%20%20%20stats%20enable%0D%0A%20%20%20%20stats%20uri%20%2Fhaproxy%3Fstatus%0D%0A%20%20%20%20balance%20roundrobin%0D%0A%20%20%20%20server%20${var.cluster_name}-bootstrap%20${var.cluster_name}-bootstrap.${var.cluster_domain}%3A49500%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-0%20${var.cluster_name}-master-0.${var.cluster_domain}%3A49500%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-1%20${var.cluster_name}-master-1.${var.cluster_domain}%3A49500%20check%0D%0A%20%20%20%20server%20${var.cluster_name}-master-2%20${var.cluster_name}-master-2.${var.cluster_domain}%3A49500%20check"

I have no idea how they maintain that (CC @flaper87 ;), but a raw sed would probably be good enough for this PR.

One thing I hadn't considered too is that if we want to change this port in the future, it will also need changes in firewalling (AWS: security groups). Do we have those under cluster management today?

I'm not aware of any cluster management for security groups. You're wondering about how this would work if someone wanted to upgrade their cluster to an OpenShift release that used a different port?

@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Feb 4, 2019 •
edited
Loading

@wking I wasn't sure how that was generated either, so I left it in hopes someone would tell me 😄

@wking
Copy link
Member

wking commented Feb 4, 2019

@wking I wasn't sure how that was generated either, so I left it in hopes someone would tell me 😄

Doesn't seem to be generated (at least, #1185 bumped it and the new version contains strings that do not show up elsewhere in that diff). I expect you can just:

$ sed -i s/49500/22623/g data/data/openstack/lb/main.tf

@cgwalters
Copy link
Member

You're wondering about how this would work if someone wanted to upgrade their cluster to an OpenShift release that used a different port?

Right.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2019
@kikisdeliveryservice kikisdeliveryservice force-pushed the mcs-change-port branch 2 times, most recently from 33a7ffa to 5ac7ef3 Compare February 5, 2019 01:57
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 5, 2019 •
edited
Loading

@kikisdeliveryservice: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-openstack 30d91f2 link /test e2e-openstack
ci/prow/e2e-libvirt 30d91f2 link /test e2e-libvirt
ci/prow/launch-aws 30d91f2 link /test launch-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2019
@abhinavdahiya
Copy link
Contributor

@kikisdeliveryservice
openshift/machine-config-operator#368 was merged
Also this needs a rebase..

@ashcrow
Copy link
Member

ashcrow commented Feb 11, 2019

Needs rebase.

@kikisdeliveryservice
Copy link
Contributor Author

@abhinavdahiya question, the file that keeps needing a rebase (3 time so far) is data/data/openstack/lb/main.tf and the bit that changes looks kind of like something auto-generated? I'm now wondering more if I shouldn't update that one to replace the 49500->22523?

If you can confirm changing that is ok, I'll rebase and repush.

@abhinavdahiya
Copy link
Contributor

@abhinavdahiya question, the file that keeps needing a rebase (3 time so far) is data/data/openstack/lb/main.tf and the bit that changes looks kind of like something auto-generated? I'm now wondering more if I shouldn't update that one to replace the 49500->22523?

If you can confirm changing that is ok, I'll rebase and repush.

@flaper87 / @russellb can you help @kikisdeliveryservice here regarding data/data/openstack/lb/main.tf

@staebler
Copy link
Contributor

@abhinavdahiya question, the file that keeps needing a rebase (3 time so far) is data/data/openstack/lb/main.tf and the bit that changes looks kind of like something auto-generated? I'm now wondering more if I shouldn't update that one to replace the 49500->22523?

If you can confirm changing that is ok, I'll rebase and repush.

I am fairly certain that it is not auto-generated. Unfortunately, it is a script crammed into a single URL-encoded line that is going to have conflicts whenever anyone makes any change to it.

@kikisdeliveryservice
Copy link
Contributor Author

thanks @staebler , I'll go ahead and rebase then.

@kikisdeliveryservice
change machine-config-server port
ab09bc2 
Change machine-config-server port from 49500 -> 22623
to avoid conflict with local port and node port ranges.

Closes machine-config-operator issue: openshift#166
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2019
@kikisdeliveryservice kikisdeliveryservice changed the title WIP: change machine-config-server port change machine-config-server port Feb 12, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2019
@ashcrow
Copy link
Member

ashcrow commented Feb 12, 2019

@kikisdeliveryservice is this ready for a final review?

@kikisdeliveryservice
Copy link
Contributor Author

@ashcrow yep. removed the WIP :)

ashcrow
ashcrow approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@ashcrow
Copy link
Member

ashcrow commented Feb 12, 2019

/cc @wking @crawford

@abhinavdahiya
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, ashcrow, kikisdeliveryservice

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2019
@openshift-merge-robot openshift-merge-robot merged commit d99dea5 into openshift:master Feb 12, 2019
@kikisdeliveryservice kikisdeliveryservice mentioned this pull request Feb 13, 2019
Closed
flaper87 added a commit to flaper87/installer that referenced this pull request Feb 13, 2019
@flaper87
openstack: Change MCO port
bde5a70 
Commit openshift#1180 changed the MCO port from 4900 to 22623, which broke
OpenStack deployments as the replace didn't catch 2 places where the
port was still hardcoded.

In addition, I've decoded the data URL to make the maintenance of this
script easier.
@flaper87 flaper87 mentioned this pull request Feb 13, 2019
Merged
flaper87 added a commit to flaper87/installer that referenced this pull request Feb 13, 2019
@flaper87
openstack: Change MCO port
5ba1250 
Commit openshift#1180 changed the MCO port from 4900 to 22623, which broke
OpenStack deployments as the replace didn't catch 2 places where the
port was still hardcoded.

In addition, I've decoded the data URL to make the maintenance of this
script easier.
flaper87 added a commit to flaper87/installer that referenced this pull request Feb 13, 2019
@flaper87
openstack: Change MCO port
8a8b59b 
Commit openshift#1180 changed the MCO port from 4900 to 22623, which broke
OpenStack deployments as the replace didn't catch 2 places where the
port was still hardcoded.

In addition, I've decoded the data URL to make the maintenance of this
script easier.
trown pushed a commit to trown/installer that referenced this pull request Feb 13, 2019
@flaper87
openstack: Change MCO port
b328c7f 
Commit openshift#1180 changed the MCO port from 4900 to 22623, which broke
OpenStack deployments as the replace didn't catch 2 places where the
port was still hardcoded.

In addition, I've decoded the data URL to make the maintenance of this
script easier.
@jlebon jlebon mentioned this pull request Feb 13, 2019
Merged
trown pushed a commit to trown/installer that referenced this pull request Feb 13, 2019
@flaper87
openstack: Change MCO port
ff59ecb 
Commit openshift#1180 changed the MCO port from 4900 to 22623, which broke
OpenStack deployments as the replace didn't catch 2 places where the
port was still hardcoded.

In addition, I've decoded the data URL to make the maintenance of this
script easier.
This was referenced Feb 13, 2019
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashcrow ashcrow ashcrow approved these changes

@aaronlevy aaronlevy Awaiting requested review from aaronlevy

@tomassedovic tomassedovic Awaiting requested review from tomassedovic

@crawford crawford Awaiting requested review from crawford

@wking wking Awaiting requested review from wking

Assignees

@abhinavdahiya abhinavdahiya

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kikisdeliveryservice @cgwalters @wking @openshift-ci-robot @abhinavdahiya @ashcrow @staebler @openshift-merge-robot