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

baremetal: Add coredns, mdns-publisher, haproxy, keepalived static pods #795

Merged
merged 2 commits into from
Jul 26, 2019
Merged

baremetal: Add coredns, mdns-publisher, haproxy, keepalived static pods #795

merged 2 commits into from
Jul 26, 2019

Conversation

bcrochet
Copy link
Member

@bcrochet bcrochet commented May 28, 2019 •
edited by kikisdeliveryservice
Loading

Adds pods to master and worker nodes as appropriate for baremetal deployment.

Requires: #943

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 28, 2019
@bcrochet bcrochet mentioned this pull request May 28, 2019
Closed
cgwalters
cgwalters reviewed May 28, 2019
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

First, this duplicates/overlaps #740 right?

The .gitkeep files shouldn't be necessary anymore.

templates/master/00-master/baremetal/files/baremetal-chrony.yaml Outdated Show resolved Hide resolved
templates/master/00-master/baremetal/files/baremetal-clusterinfo.yaml Outdated Show resolved Hide resolved
templates/master/00-master/baremetal/files/baremetal-fletcher8.yaml Outdated Show resolved Hide resolved
templates/master/00-master/baremetal/files/baremetal-get-vip-subnet-cidr.yaml Outdated Show resolved Hide resolved
@bcrochet
Copy link
Member Author

First, this duplicates/overlaps #740 right?

The .gitkeep files shouldn't be necessary anymore.

There is some little overlap, but does not directly duplicate. If you have a suggestion for de-duping between just two platforms, that would be helpful.

I'll remove the .gitkeep files.

@stbenjam stbenjam mentioned this pull request May 30, 2019
Closed
@bcrochet bcrochet force-pushed the baremetal-mdns branch 3 times, most recently from e1fc4a0 to d1947b8 Compare May 30, 2019 11:57
@bcrochet bcrochet mentioned this pull request Jun 4, 2019
Closed
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2019
@bcrochet bcrochet force-pushed the baremetal-mdns branch 2 times, most recently from 79a77c0 to 71395f2 Compare June 17, 2019 22:05
@hardys hardys mentioned this pull request Jun 18, 2019
Closed
@hardys hardys mentioned this pull request Jun 20, 2019
Merged
@cgwalters
Copy link
Member

Sorry about the delay on this...there's a lot going on and we may need periodic pings if PRs are important/ready.

I feel like this would probably be best as a separate git repository, with the contents built into one or more container images. The relevant code in the MCO would boil down to injecting the static pod entries, much like is done for etcd.

Among other things, that would avoid the awkward shell-script-in-yaml-in-yaml and more generally allow your team to easily iterate on the content without blocking on us here.

@cgwalters cgwalters mentioned this pull request Jun 21, 2019
Closed
@stbenjam stbenjam mentioned this pull request Jun 24, 2019
Closed
@bcrochet
Copy link
Member Author

Sorry about the delay on this...there's a lot going on and we may need periodic pings if PRs are important/ready.

I feel like this would probably be best as a separate git repository, with the contents built into one or more container images. The relevant code in the MCO would boil down to injecting the static pod entries, much like is done for etcd.

Among other things, that would avoid the awkward shell-script-in-yaml-in-yaml and more generally allow your team to easily iterate on the content without blocking on us here.

We are working on the amount of shell-scripty things. Is there an existing component with such a repo that I can look at?

@cgwalters
Copy link
Member

Since the runway is relatively short here for 4.2, we discussed landing this roughly as is and iterating from there.

One argument to having this be a separate operator/controller was that the MCO's role here should be more around "infrastructure" rather than platform-specific code, but on the other hand in practice actually the MCO codebase has a lot of convenient parts for this (templating MachineConfig files based on data from the cluster config), plus its already extant support for platform conditionals for MachineConfig, etc.

Another aspect is that these templates also need to run at install time for the masters, and introducing a new operator/controller rendering MachineConfigs would require changes to the installer's bootkube.sh (not complex changes though).

@bcrochet bcrochet force-pushed the baremetal-mdns branch 3 times, most recently from e4c2117 to 25393f3 Compare July 9, 2019 20:25
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2019
@bcrochet bcrochet force-pushed the baremetal-mdns branch 4 times, most recently from 71e06a3 to 48de99f Compare July 23, 2019 16:35
@bcrochet bcrochet mentioned this pull request Jul 23, 2019
Merged
@celebdor celebdor mentioned this pull request Jul 24, 2019
Merged
@bcrochet bcrochet force-pushed the baremetal-mdns branch 2 times, most recently from 2161280 to 2755d5b Compare July 25, 2019 13:38
@bcrochet
Copy link
Member Author

/retest

@bcrochet bcrochet force-pushed the baremetal-mdns branch 2 times, most recently from 34c3c06 to 341a752 Compare July 25, 2019 17:51
@bcrochet
Add coredns, mdns-publisher, haproxy, keepalived, baremetal-runtimecf…
6a03862 
…g images

This patch adds the images so they can be referenced from templates.
@bcrochet bcrochet force-pushed the baremetal-mdns branch 2 times, most recently from bf809d6 to 07c4311 Compare July 26, 2019 00:28
@bcrochet
Copy link
Member Author

@runcom Review please.

@runcom
Copy link
Member

runcom commented Jul 26, 2019

/hold cancel
/approve

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2019
@bcrochet @celebdor
baremetal: Add coredns, mdns-publisher, haproxy, keepalived static pods
ae52421 
Adds pods to master and worker nodes as appropriate

Updates haproxy container to use openshift/router-haproxy image instead of
docker.io/library/haproxy

Also adds liveness tests for the coredns,mdns-publisher,
haproxy and keepalived static pods, changes worker node
/etc/resolv.conf to point to node's IP instead of 127.0.0.1 and
fix the bug generating haproxy cfg file

Due to the fact that both use the same image, there was a bit of
confusion here. We want keepalived to track OCP Router 1936 and we want
API LB Haproxy pod to have health checked at 50936, which is where we
configure haproxy to expose health at.

Depends-On: openshift#943
Depends-On: openshift#984
@runcom
Copy link
Member

runcom commented Jul 26, 2019

an exception has been granted for this so
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bcrochet, celebdor, cgwalters, runcom

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 efc0e00 into openshift:master Jul 26, 2019
@bcrochet bcrochet mentioned this pull request Jul 26, 2019
Merged
cybertron added a commit to cybertron/installer that referenced this pull request Jul 26, 2019
@cybertron
Pass baremetal infra images to MCO bootstrap
6158aeb 
In order for the baremetal infra services to use the images from the
release, we need to look up the correct image during bootstrap and
pass that in to the MCO.

The new image parameters were added to MCO in
openshift/machine-config-operator#795

Part of the changes for openshift#2067
@abhinavdahiya abhinavdahiya mentioned this pull request Jul 29, 2019
Merged
@tomassedovic tomassedovic mentioned this pull request Aug 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgolangh rgolangh rgolangh left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@runcom runcom runcom left review comments

@cgwalters cgwalters cgwalters left review comments

@markmc markmc markmc left review comments

@celebdor celebdor celebdor approved these changes

@jlebon jlebon Awaiting requested review from jlebon

Assignees

@runcom runcom

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

Successfully merging this pull request may close these issues.
9 participants
@bcrochet @cgwalters @runcom @markmc @openshift-ci-robot @celebdor @smarterclayton @rgolangh @openshift-merge-robot