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 machine-config-server port #1180

Merged
merged 1 commit into from Feb 12, 2019

Conversation

Projects
None yet
8 participants
@kikisdeliveryservice
Copy link
Member

kikisdeliveryservice commented Feb 1, 2019

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

@kikisdeliveryservice

This comment has been minimized.

Copy link
Member Author

kikisdeliveryservice commented Feb 1, 2019

@kikisdeliveryservice kikisdeliveryservice changed the title change machine-config-server port WIP: change machine-config-server port Feb 1, 2019

@kikisdeliveryservice kikisdeliveryservice force-pushed the kikisdeliveryservice:mcs-change-port branch from f151dbc to b977e42 Feb 1, 2019

@openshift-ci-robot openshift-ci-robot added size/M and removed size/XS labels Feb 1, 2019

@kikisdeliveryservice kikisdeliveryservice force-pushed the kikisdeliveryservice:mcs-change-port branch from b977e42 to efd0c46 Feb 1, 2019

@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Feb 4, 2019

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

This comment has been minimized.

Copy link
Contributor

cgwalters commented Feb 4, 2019

Code changes seem good to me offhand.

@wking

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

kikisdeliveryservice commented Feb 4, 2019

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

@wking

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

cgwalters commented Feb 4, 2019

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.

@kikisdeliveryservice kikisdeliveryservice force-pushed the kikisdeliveryservice:mcs-change-port branch from efd0c46 to 5e558f2 Feb 5, 2019

@kikisdeliveryservice kikisdeliveryservice force-pushed the kikisdeliveryservice:mcs-change-port branch 2 times, most recently from 33a7ffa to 5ac7ef3 Feb 5, 2019

@kikisdeliveryservice kikisdeliveryservice force-pushed the kikisdeliveryservice:mcs-change-port branch from 5ac7ef3 to 30d91f2 Feb 5, 2019

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Feb 5, 2019

@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.

@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Feb 9, 2019

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

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 11, 2019

Needs rebase.

@kikisdeliveryservice

This comment has been minimized.

Copy link
Member Author

kikisdeliveryservice commented Feb 12, 2019

@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

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Feb 12, 2019

@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

This comment has been minimized.

Copy link
Contributor

staebler commented Feb 12, 2019

@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

This comment has been minimized.

Copy link
Member Author

kikisdeliveryservice commented Feb 12, 2019

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

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

Closes machine-config-operator issue: #166

@kikisdeliveryservice kikisdeliveryservice force-pushed the kikisdeliveryservice:mcs-change-port branch from 30d91f2 to ab09bc2 Feb 12, 2019

@kikisdeliveryservice kikisdeliveryservice changed the title WIP: change machine-config-server port change machine-config-server port Feb 12, 2019

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 12, 2019

@kikisdeliveryservice is this ready for a final review?

@kikisdeliveryservice

This comment has been minimized.

Copy link
Member Author

kikisdeliveryservice commented Feb 12, 2019

@ashcrow yep. removed the WIP :)

@ashcrow
Copy link
Member

ashcrow left a comment

This looks good to me!

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 12, 2019

@openshift-ci-robot openshift-ci-robot requested review from crawford and wking Feb 12, 2019

@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Feb 12, 2019

/lgtm

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Feb 12, 2019

[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-merge-robot openshift-merge-robot merged commit d99dea5 into openshift:master Feb 12, 2019

12 checks passed

ci/prow/e2e-aws Job succeeded.
Details
ci/prow/gofmt Job succeeded.
Details
ci/prow/golint Job succeeded.
Details
ci/prow/govet Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/rhel-images Job succeeded.
Details
ci/prow/shellcheck Job succeeded.
Details
ci/prow/tf-fmt Job succeeded.
Details
ci/prow/tf-lint Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
ci/prow/yaml-lint Job succeeded.
Details
tide In merge pool.
Details

flaper87 added a commit to flaper87/installer that referenced this pull request Feb 13, 2019

openstack: Change MCO port
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

openstack: Change MCO port
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

openstack: Change MCO port
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 added a commit to trown/installer that referenced this pull request Feb 13, 2019

openstack: Change MCO port
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 added a commit to trown/installer that referenced this pull request Feb 13, 2019

openstack: Change MCO port
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment