baremetal: Drop custom kubelet configuration. #993
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
the
size/M
Contributor
Author
|
/hold As discussed in the description, this needs to wait for one of the two mentioned installer PRs to merge first. |
openshift-ci-robot
added
the
do-not-merge/hold
Contributor
openshift-ci-robot
added
the
needs-rebase
Contributor
Author
|
The installer change went in, but the scheduler setting is not working as expected, bug filed here: #1024 |
The purpose of this change was to make masters able to run workloads by default. This is needed to complete a successful deployment of a 3-node bare metal install. This particular approach was only short term, while better interfaces were developed to control this behavior. The scheduler configuration resource now includes a "mastersSchedulable" boolean, enabled here: openshift#937 This installer PR made it the default behavior if no workers were defined at install time: openshift/installer#2004 With these changes in place, the custom kubelet config for the baremetal platform is no longer necessary.
russellb
force-pushed
the
drop-baremetal-hack
branch
from
3adad4e to
bcb4835
Compare
openshift-ci-robot
removed
the
needs-rebase
Contributor
Author
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
Contributor
Author
|
The prerequisites for this merged a while ago, so this should be ready to go in. /cc stbenjam |
Member
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: russellb, stbenjam The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Member
|
/retest |
Contributor
Author
|
/test e2e-gcp-upgrade |
Contributor
|
/skip |
Member
|
I believe we went a different direction and don't need this anymoe. |
Member
|
master still has the baremetal kubelet override: https://github.com/openshift/machine-config-operator/blob/master/templates/master/01-master-kubelet/baremetal/units/kubelet.yaml @russellb We should be able to drop this now, right? |
Contributor
Author
Member
|
/reopen |
Contributor
|
@stbenjam: Reopened this PR. DetailsIn response to this:
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. |
russellb
added a commit
to openshift-kni/machine-config-operator
that referenced
this pull request
Jan 21, 2020
Commit 7ab08f8 did this for the base template in templates/master/01-master-kubelet/baremetal/units/kubelet.yaml. We need this for the baremetal platform specific kubelet config as well. Note that this baremetal kubelet override will go away, but this is needed until the following PR is merged: openshift#993
Contributor
|
@russellb: The following tests failed, say
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. DetailsInstructions 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. |
yazug
pushed a commit
to openshift-kni/machine-config-operator
that referenced
this pull request
Feb 5, 2020
Commit 7ab08f8 did this for the base template in templates/master/01-master-kubelet/baremetal/units/kubelet.yaml. We need this for the baremetal platform specific kubelet config as well. Note that this baremetal kubelet override will go away, but this is needed until the following PR is merged: openshift#993
|
@russellb Hey this looks like it resolves https://bugzilla.redhat.com/show_bug.cgi?id=1828250 - would you mind rebasing when you get a moment and we can re-test/review? |
russellb
added a commit
to russellb/machine-config-operator
that referenced
this pull request
Jun 11, 2020
This came up because of a bug report showing that workloads were scheduled on masters for the baremetal platform regardless of the schedulableMasters scheduler configuration. This is due to the NoSchedule taint not being applied by default on this platform. The baremetal platform specific kubelet caused this behavior. We were going to remove the custom kubelet config once this behavior was configurable (see PR openshift#993), but it was never removed because we ended up needing to make some IPv6 related customizations in this file. We also forgot to re-add the default taint. Meanwhile, some other changes to the kubelet unit were not applied to the baremetal version. I also checked the openstack and vsphere files and found discrepencies there, as well. This is the simplest fix, which is to get these files in sync again. The differences are very minor, so a better follow-up would be to get back to a single kubelet unit, or at least share the duplicated content somehow. I'm leaving that further cleanup as another change, since the most straight forward fix will be the simpler one to backport. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1828250
Contributor
Author
|
This PR is replaced by #1817 |
openshift-cherrypick-robot
pushed a commit
to openshift-cherrypick-robot/machine-config-operator
that referenced
this pull request
Jun 16, 2020
This came up because of a bug report showing that workloads were scheduled on masters for the baremetal platform regardless of the schedulableMasters scheduler configuration. This is due to the NoSchedule taint not being applied by default on this platform. The baremetal platform specific kubelet caused this behavior. We were going to remove the custom kubelet config once this behavior was configurable (see PR openshift#993), but it was never removed because we ended up needing to make some IPv6 related customizations in this file. We also forgot to re-add the default taint. Meanwhile, some other changes to the kubelet unit were not applied to the baremetal version. I also checked the openstack and vsphere files and found discrepencies there, as well. This is the simplest fix, which is to get these files in sync again. The differences are very minor, so a better follow-up would be to get back to a single kubelet unit, or at least share the duplicated content somehow. I'm leaving that further cleanup as another change, since the most straight forward fix will be the simpler one to backport. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1828250
russellb
added a commit
to russellb/machine-config-operator
that referenced
this pull request
Jun 19, 2020
This came up because of a bug report showing that workloads were scheduled on masters for the baremetal platform regardless of the schedulableMasters scheduler configuration. This is due to the NoSchedule taint not being applied by default on this platform. The baremetal platform specific kubelet caused this behavior. We were going to remove the custom kubelet config once this behavior was configurable (see PR openshift#993), but it was never removed because we ended up needing to make some IPv6 related customizations in this file. We also forgot to re-add the default taint. Meanwhile, some other changes to the kubelet unit were not applied to the baremetal version. This is the simplest fix, which is to get these files in sync again. The differences are very minor, so a better follow-up would be to get back to a single kubelet unit, or at least share the duplicated content somehow. I'm leaving that further cleanup as another change, since the most straight forward fix will be the simpler one to backport. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1828250
Contributor
Contributor
Author
vrutkovs
pushed a commit
to vrutkovs/machine-config-operator
that referenced
this pull request
Jun 24, 2020
This came up because of a bug report showing that workloads were scheduled on masters for the baremetal platform regardless of the schedulableMasters scheduler configuration. This is due to the NoSchedule taint not being applied by default on this platform. The baremetal platform specific kubelet caused this behavior. We were going to remove the custom kubelet config once this behavior was configurable (see PR openshift#993), but it was never removed because we ended up needing to make some IPv6 related customizations in this file. We also forgot to re-add the default taint. Meanwhile, some other changes to the kubelet unit were not applied to the baremetal version. I also checked the openstack and vsphere files and found discrepencies there, as well. This is the simplest fix, which is to get these files in sync again. The differences are very minor, so a better follow-up would be to get back to a single kubelet unit, or at least share the duplicated content somehow. I'm leaving that further cleanup as another change, since the most straight forward fix will be the simpler one to backport. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1828250
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of this change was to make masters able to run workloads
by default. This is needed to complete a successful deployment of a
3-node bare metal install. This particular approach was only short
term, while better interfaces were developed to control this behavior.
The scheduler configuration resource now includes a
"mastersSchedulable" boolean, enabled here:
#937
This installer PR made it the default behavior if no workers were
defined at install time:
openshift/installer#2004
With these changes in place, the custom kubelet config for the
baremetal platform is no longer necessary.