Skip to content

Conversation

@cybertron
Copy link
Member

- What I did
This moves the baremetal infra bootstrap static pod configurations to MCO, which is necessary so the installer can look up the images from the release and pass them in. Previously the pod definitions were in the installer itself, which meant they were written before bootkube.sh was run and could not use the standard method to retrieve the images. This also moves all of the baremetal infra templates into the same repo and converts them to use the MCO templating just like the master versions.

- How to verify it
Remove the existing baremetal systemd services from the installer and deploy with this series of commits. Coredns and keepalived should be deployed on the bootstrap node as static pods.

- Description for the changelog
Baremetal infra static pod definitions are now deployed during the machine-config-operator bootstrap phase.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 23, 2019
Copy link

@iamemilio iamemilio Jul 24, 2019

Choose a reason for hiding this comment

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

Is there some way for this to take in platform as a variable:
/manifests/{platform}/*

Choose a reason for hiding this comment

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

/manifests/spec.Platform/*

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, although because of the way the manifests are compiled in as bindata I'm not sure it's as simple as globbing a path. For the moment I'm just conditionalizing the list based on platform (in a commit I haven't pushed yet).

Copy link
Contributor

Choose a reason for hiding this comment

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

SHouldn't all these be out of this PR and just in #795 Is this just to make it work and we'll rebase as soon as 795 is merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically yes. I needed some of the changes in 795 in order to do this so I based my changes on it. We'll need to rebase this once 795 merges.

@cybertron cybertron force-pushed the runtimecfg branch 2 times, most recently from d608d42 to f93e929 Compare July 26, 2019 15:49
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 26, 2019
@cybertron
Copy link
Member Author

This is rebased now that #795 has merged. It also includes a couple of fixes for things we identified in testing.

Also note that this is a dependency for openshift/installer#2067

filename: "baremetal/manifests/keepalived.yaml",
}, {
name: "manifests/baremetal/keepalived.conf.tmpl",
filename: "baremetal/static-pod-resources/keepalived/keepalived.conf.tmpl",
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't all these end up under bootstrap/ similar to https://github.com/openshift/machine-config-operator/pull/1002/files#diff-d6f38878757202a810cfa8038e7ae122R161

Have you tested this out manually to verify this is applied at bootstrap?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe @celebdor made this change because having them in bootstrap caused them to be applied twice - once by kubelet and again during cluster-bootstrap. We will be explicitly copying them into place in the installer[0], so the specific location here doesn't matter much.

0: celebdor/installer@134d74c

@runcom
Copy link
Member

runcom commented Jul 26, 2019

cc @abhinavdahiya - can you take a look at the overall mechanism here? I feel like this is sane to do but maybe you have more insight from the installer and this can be achieved from the installer as well (key thing here is that we want the images from the payload to be available to render these manifests and previously we only had systemd units in the installer bootstrap data).

@cybertron
Copy link
Member Author

The other reason we wanted to move these here was so we had a consistent templating mechanism between bootstrap and master/worker nodes. When these resources lived in the installer we had to retrieve the inputs in more hacky ways. Here we can use the exact same platform status data as the master templates.

Baremetal deployments require a couple of additional services to run
on the bootstrap node. This change adds static pod manifests and the
associated configuration templates to the MCO bootstrap process.

Co-authored-by: Brad P. Crochet <[email protected]>
Co-authored-by: Antoni Segura Puimedon <[email protected]>
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 29, 2019
@cgwalters
Copy link
Member

/approve
I didn't review this in detail, but it looks good to me offhand. I think probably the best strategy is to get this and the installer PR merged and let people do testing from master builds.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2019
@runcom
Copy link
Member

runcom commented Jul 30, 2019

/lgtm

deferring my question to @abhinavdahiya from #1002 (comment)

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

7 participants