-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
OCPBUGS-6508: Update Control Plane replica validation for Single Node OpenShift #9048
base: master
Are you sure you want to change the base?
Conversation
@sadasu: This pull request references CORS-3456 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.18.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
@sadasu: This pull request references Jira Issue OCPBUGS-6508, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. In 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 openshift-eng/jira-lifecycle-plugin repository. |
/test ? |
@sadasu: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In 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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test e2e-agent-sno-ipv6 |
/retest |
|
||
// supportedSingleNodePlatform indicates if the IPI Installer can be used to install SNO on | ||
// a platform. | ||
func supportedSingleNodePlatform(isSingleNodeOpenShifti bool, p *types.Platform) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zaneb , @cybertron Are the supported platforms accurate here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if SNO is supported on platform External
. If so, does it need a valid bootstrapInPlace
config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is supported on platform:external
6befc4c
to
d5f71a6
Compare
// Single node OpenShift installations supported without `bootstrapInPlace` | ||
return true | ||
} | ||
if isSingleNodeOpenShift && (p.BareMetal != nil || p.None != nil) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baremetal is not supported for bootstrap-in-place; only none.
Installer can be used to provision Single Node OpenShift(SNO) only for AWS/GCP/Azure. SNO is not supported on all on-prem platforms yet. Even on the platforms that support SNO installs, the install method currently supported are assisted/agent/ZTP.
/test e2e-agent-sno-ipv6 |
/retest-required |
@sadasu: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "number of control plane replicas must be positive")) | ||
case *pool.Replicas == 1 && !supportedSingleNodePlatform(isSingleNodeOpenShift, platform): | ||
allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "Single Node OpenShift(SNO) not supported by this install method for this platform")) | ||
case *pool.Replicas != 3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs (from what I could find), the default is 1 replica but it does not specify a maximum number or a constraint of 3.
Are we sure that 3 is the constraint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
This has nothing to do with what is valid in the MachineSet API (which is not actually used for control plane Machines) and everything to do with what topologies of OpenShift are supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danmanor FYI - obviously we will expand the range here for 4/5 node control planes when you are ready to support that (if it has to be in this PR, then hopefully in a separate commit so that we could backport the !=3 check if necessary).
Sandhya, please co-ordinate with Dan on the timing and keep @rwsu in the loop on the agent side.
if pool.Replicas != nil { | ||
switch { | ||
case *pool.Replicas == 0: | ||
allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "number of control plane replicas must be positive")) | ||
case *pool.Replicas == 1 && !supportedSingleNodePlatform(isSingleNodeOpenShift, platform): | ||
allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "Single Node OpenShift(SNO) not supported by this install method for this platform")) | ||
case *pool.Replicas != 3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these validation functions can get pretty unreadable. I recommend extracting this into a separate function, say ValidateControlPlaneReplicaCount
and use the same pattern as ValidateMachinePool
so this function looks like:
allErrs = append(allErrs, ValidateControlPlaneReplicaCount(platform, pool, fldPath)...)
allErrs = append(allErrs, ValidateMachinePool(platform, pool, fldPath)...)
// supportedSingleNodePlatform indicates if the IPI Installer can be used to install SNO on | ||
// a platform. | ||
func supportedSingleNodePlatform(bootstrapInPlace bool, p *types.Platform) bool { | ||
if p.AWS != nil || p.GCP != nil || p.Azure != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to me that we can add powervs (but not ibmcloud) to this list.
https://issues.redhat.com/browse/OCPBUGS-6508?focusedId=25675347&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-25675347
Installer can be used to provision Single Node OpenShift(SNO) only for AWS/GCP/Azure.
SNO is not supported on all on-prem platforms yet. Even on the platforms that support SNO installs, the install method currently supported are assisted/agent/ZTP.