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

OCPBUGS-6508: Update Control Plane replica validation for Single Node OpenShift #9048

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+25 −4
Open

OCPBUGS-6508: Update Control Plane replica validation for Single Node OpenShift #9048

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions pkg/types/validation/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ func ValidateInstallConfig(c *types.InstallConfig, usingAgentMethod bool) field.
}
allErrs = append(allErrs, validatePlatform(&c.Platform, usingAgentMethod, field.NewPath("platform"), c.Networking, c)...)
if c.ControlPlane != nil {
allErrs = append(allErrs, validateControlPlane(&c.Platform, c.ControlPlane, field.NewPath("controlPlane"))...)
allErrs = append(allErrs, validateControlPlane(&c.Platform, c.ControlPlane, c.IsSingleNodeOpenShift(), field.NewPath("controlPlane"))...)
} else {
allErrs = append(allErrs, field.Required(field.NewPath("controlPlane"), "controlPlane is required"))
}
Expand Down Expand Up @@ -596,13 +596,20 @@ func validateNetworkingClusterNetworkMTU(c *types.InstallConfig, fldPath *field.
return allErrs
}

func validateControlPlane(platform *types.Platform, pool *types.MachinePool, fldPath *field.Path) field.ErrorList {
func validateControlPlane(platform *types.Platform, pool *types.MachinePool, isSingleNodeOpenShift bool, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
if pool.Name != types.MachinePoolControlPlaneRoleName {
allErrs = append(allErrs, field.NotSupported(fldPath.Child("name"), pool.Name, []string{types.MachinePoolControlPlaneRoleName}))
}
if pool.Replicas != nil && *pool.Replicas == 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "number of control plane replicas must be positive"))
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:
Copy link
Contributor

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.

https://docs.openshift.com/container-platform/4.16/rest_api/machine_apis/machineset-machine-openshift-io-v1beta1.html

Are we sure that 3 is the constraint here?

Copy link
Member

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.

Copy link
Member

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.

Comment on lines +604 to +610
Copy link
Contributor

@patrickdillon patrickdillon Sep 27, 2024 •
edited
Loading

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)...)

allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "number of control plane replicas must equal 3"))
}
}
allErrs = append(allErrs, ValidateMachinePool(platform, pool, fldPath)...)
return allErrs
Expand Down Expand Up @@ -1345,3 +1352,17 @@ func validateReleaseArchitecture(controlPlanePool *types.MachinePool, computePoo

return allErrs
}

// 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 {
Copy link
Member

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

// Single node OpenShift installations supported without `bootstrapInPlace`
return true
}
if bootstrapInPlace && p.None != nil {
// Single node OpenShift installations supported with `bootstrapInPlace`
return true
}
return false
}