Skip to content

Optionally add cluster VIPs to the allowed addresses #52

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
Merged
openshift-merge-robot merged 2 commits into from
Jul 31, 2019
Merged

Optionally add cluster VIPs to the allowed addresses #52

openshift-merge-robot merged 2 commits into from
Jul 31, 2019

Conversation

@tomassedovic
Copy link

@tomassedovic tomassedovic commented Jul 26, 2019 •
edited
Loading

What this PR does / why we need it:

The OpenStack provider is now switching away from the extra LB/DNS VM to using VIPs for the internal API, DNS and ingress access:

openshift/installer#1959
openshift/machine-config-operator#740

To make these VIPs accessible to the machines in the cluster, they need to be added to the OpenStack ports' allowed_address_pairs.

The piece here sets them for the machines managed by CAPO.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Adds the OpenStack API/DNS/Ingress VIPs to the workers' ports' `allowed_address_pairs` if available.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 26, 2019
@tomassedovic tomassedovic mentioned this pull request Jul 26, 2019
Merged
@tomassedovic
Copy link
Author

tomassedovic commented Jul 29, 2019

/retest

@tomassedovic
Copy link
Author

tomassedovic commented Jul 29, 2019

/retest

Cloud timeouts, hopefully transient.

@tomassedovic
Copy link
Author

tomassedovic commented Jul 29, 2019

/retest

3 similar comments
@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

/retest

@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

/retest

@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

/retest

@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

I'm going to test this locally with installer#master (which is what the e2e-openstack job runs).

I've tested this with the "remove service VM" patches and it worked great. The changes here shouldn't break anything on the master, but the string of CI failures is not exactly reassuring.

@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

/retest

@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

Okay, no, nevermind, this is a real issue. The machine-api-controllers pod keeps crashlooping.

@celebdor @tomassedovic
Put OpenStack PlatformStatus VIPs in allowed pairs
667e9ff 
This reads the OpenStack's `cluster` Inflastructure object, gathers the
internal API, DNS and Ingress VIPs and adds them to the
`allowed_address_pair` property to the Machine's port.

This ensures any VIPs attached to the Machine are accessible to the rest
of the cluster.

Signed-off-by: Antoni Segura Puimedon <[email protected]>
@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

Looks like the DNS our CI uses is broken or something. I'm not seeing these issues locally.

@tomassedovic
Copy link
Author

tomassedovic commented Jul 30, 2019

I got Install complete! when running this locally. Looks like the e2e-openstack job is indeed broken at the moment.

@tomassedovic
Copy link
Author

tomassedovic commented Jul 31, 2019

/retest

@mandre
Copy link
Member

mandre commented Jul 31, 2019

/lgtm

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

openshift-ci-robot commented Jul 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandre, tomassedovic

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

The pull request process is described here

Details 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 99ead52 into openshift:master Jul 31, 2019
@tomassedovic tomassedovic mentioned this pull request Jan 2, 2020
Closed
@EmilienM EmilienM mentioned this pull request Jan 31, 2023
Closed
racheljpg pushed a commit to racheljpg/cluster-api-provider-openstack that referenced this pull request Dec 20, 2023
@dims @flaper87
golint has moved! (openshift#52)
35aa273 
Change-Id: I498589845e36300b05dc4c155cce6660cfa297d2
pierreprinetti pushed a commit to shiftstack/cluster-api-provider-openstack that referenced this pull request Apr 22, 2024
@dims @pierreprinetti
golint has moved! (openshift#52)
83b5596 
Change-Id: I498589845e36300b05dc4c155cce6660cfa297d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iamemilio iamemilio Awaiting requested review from iamemilio

@trown trown Awaiting requested review from trown

Assignees

@mandre mandre

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tomassedovic @mandre @openshift-ci-robot @openshift-merge-robot @celebdor