Skip to content

Add support for baremetal platform#846

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
stbenjam:baremetal
Jun 12, 2019
Merged

Add support for baremetal platform#846
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
stbenjam:baremetal

Conversation

@stbenjam
Copy link
Copy Markdown
Member

@stbenjam stbenjam commented Jun 12, 2019

For IPI baremetal, we need to support the platform in MCO. This PR also overrides the kubelet config to remove the NoSchedule taint.

Currently in the baremetal IPI installer fork, we have to bail out of the installer early to take care of some things - one of those things is disabling NoSchedule taints: https://github.com/openshift-metal3/dev-scripts/blob/master/06_create_cluster.sh#L82-L83

We are anxious to get an experimental version of this platform into openshift/installer, but need to resolve this issue first. This adds the barematel platform to MCO, and overrides the kubelet config.

Once #763 is resolved, this can move to the new mechanism, or alternatively once we have workers being deployed (currently work in progress) we won't need the override at all .

See #722 for additional background. GitHub won't let me re-open that PR.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 12, 2019
@stbenjam
Copy link
Copy Markdown
Member Author

/assign @cgwalters

@cgwalters
Copy link
Copy Markdown
Member

Hm I had more been thinking that you were asking if it was OK to carry a patch to do that downstream but I guess I kind of painted myself into a corner and I understand the urgency behind de-forking so
/approve

It's again sort of unfortunate because we just de-forked the OpenStack one but...
/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 12, 2019
For IPI baremetal, we need to support the platform in MCO. This
overrides the kubelet config to remove the NoSchedule taint, until the
baremetal platform in the installer can deploy a worker.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2019
@stbenjam
Copy link
Copy Markdown
Member Author

stbenjam commented Jun 12, 2019

New changes are detected. LGTM label has been removed.

Pushed a change to resolve conflicts due to #842.

@cgwalters I totally understand, I will submit a PR to remove this the second it's possible to. Thanks.

@cgwalters
Copy link
Copy Markdown
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, stbenjam

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 8f4eb2f into openshift:master Jun 12, 2019
@stbenjam stbenjam deleted the baremetal branch June 12, 2019 23:05
@eparis
Copy link
Copy Markdown
Member

eparis commented Jun 14, 2019

@derekwaynecarr I wonder if this is a bad idea, and if it should be reverted.

@stbenjam
Copy link
Copy Markdown
Member Author

FWIW it should be relatively short lived, on the order of weeks I hope. Reverting this now means prolonging the existence of the kni-installer fork...

@russellb
Copy link
Copy Markdown
Contributor

I don’t understand why this was needed to avoid the kni installer fork. We were always making masters schedulable as a day 2 operation, not via the installer.

This isn’t the behavior we want for all IPI baremetal environments. It’s only applicable to some.

@stbenjam
Copy link
Copy Markdown
Member Author

I don’t understand why this was needed to avoid the kni installer fork. We were always making masters schedulable as a day 2 operation, not via the installer. This isn’t the behavior we want for all IPI baremetal environments. It’s only applicable to some.

Sorry, you missed the discussion last week during our status call. Baremetal IPI environment is not installable without removing the NoSchedule taint from the masters. The ingress, monitoring, and image registry pods can't be scheduled, and installation does not complete. You cannot get a running cluster without this.

Once workers get deployed with the installer that would change, but the timeline for that happening isn't clear yet. The goal is to have a PR open to openshift/installer with our code by the end of this sprint, and we need to remove the early exit and all of the hacks in 06_create_cluster.sh.

@russellb
Copy link
Copy Markdown
Contributor

I don’t understand why this was needed to avoid the kni installer fork. We were always making masters schedulable as a day 2 operation, not via the installer. This isn’t the behavior we want for all IPI baremetal environments. It’s only applicable to some.

Sorry, you missed the discussion last week during our status call. Baremetal IPI environment is not installable without removing the NoSchedule taint from the masters. The ingress, monitoring, and image registry pods can't be scheduled, and installation does not complete. You cannot get a running cluster without this.

Once workers get deployed with the installer that would change, but the timeline for that happening isn't clear yet. The goal is to have a PR open to openshift/installer with our code by the end of this sprint, and we need to remove the early exit and all of the hacks in 06_create_cluster.sh.

OK, thanks. That explains why this applies to all usage of the baremetal platform type. Once workers come up without any extra intervention, then this could be removed, which could be before #763 is resolved

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants