Skip to content

[release-4.13] OCPBUGS-14432: Check that number of replicas matches hosts #7221

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

Closed
openshift-cherrypick-robot wants to merge 1 commit into from
Closed

[release-4.13] OCPBUGS-14432: Check that number of replicas matches hosts #7221

Show file tree
Hide file tree
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
20 changes: 20 additions & 0 deletions pkg/asset/agent/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ func (a *OptionalInstallConfig) validateInstallConfig(installConfig *types.Insta

warnUnusedConfig(installConfig)

numMasters, numWorkers := GetReplicaCount(installConfig)
logrus.Infof(fmt.Sprintf("Configuration has %d master replicas and %d worker replicas", numMasters, numWorkers))

if err := a.validateSNOConfiguration(installConfig); err != nil {
allErrs = append(allErrs, err...)
}
Expand Down Expand Up @@ -388,3 +391,20 @@ func warnUnusedConfig(installConfig *types.InstallConfig) {
logrus.Warnf(fmt.Sprintf("%s: %s is ignored", fieldPath, installConfig.BootstrapInPlace.InstallationDisk))
}
}

// GetReplicaCount gets the configured master and worker replicas.
func GetReplicaCount(installConfig *types.InstallConfig) (numMasters, numWorkers int64) {
numRequiredMasters := int64(0)
if installConfig.ControlPlane != nil && installConfig.ControlPlane.Replicas != nil {
numRequiredMasters += *installConfig.ControlPlane.Replicas
}

numRequiredWorkers := int64(0)
for _, worker := range installConfig.Compute {
if worker.Replicas != nil {
numRequiredWorkers += *worker.Replicas
}
}

return numRequiredMasters, numRequiredWorkers
}
33 changes: 33 additions & 0 deletions pkg/asset/agent/manifests/nmstateconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/agent"
"github.com/openshift/installer/pkg/asset/agent/agentconfig"
"github.com/openshift/installer/pkg/types"
)

var (
Expand Down Expand Up @@ -82,6 +83,8 @@ func (n *NMStateConfig) Generate(dependencies asset.Parents) error {
if len(agentConfig.Config.Hosts) == 0 {
return nil
}
checkHostCount(installConfig.Config, agentConfig)

for i, host := range agentConfig.Config.Hosts {
if host.NetworkConfig.Raw != nil {
isNetworkConfigAvailable = true
Expand Down Expand Up @@ -336,3 +339,33 @@ func buildMacInterfaceMap(nmStateConfig aiv1beta1.NMStateConfig) models.MacInter
}
return macInterfaceMap
}

func checkHostCount(installConfig *types.InstallConfig, agentConfig *agentconfig.AgentConfig) {
numRequiredMasters, numRequiredWorkers := agent.GetReplicaCount(installConfig)

// Check that the number of replicas matches the configured hosts
numMasters := int64(0)
numWorkers := int64(0)
for _, host := range agentConfig.Config.Hosts {
switch host.Role {
case "master":
numMasters++
case "worker":
numWorkers++
case "":
// If not defined, the roles will be matched to replicas
if numMasters < numRequiredMasters {
numMasters++
} else if numWorkers < numRequiredWorkers {
Copy link
Copy Markdown
Member

@zaneb zaneb Jun 9, 2023

Choose a reason for hiding this comment

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

This needs to be unconditional if we want to catch the case where there are more hosts defined than replicas.

Copy link
Copy Markdown
Contributor

@bfournie bfournie Jun 9, 2023

Choose a reason for hiding this comment

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

Good point. As this PR is auto cherry-pick of #7059 I'll create a new master PR/bug and fix it there, then cherry-pick that.

numWorkers++
}
}
}
Copy link
Copy Markdown
Member

@zaneb zaneb Jun 9, 2023

Choose a reason for hiding this comment

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

This will fail spuriously in a case like:

- role: ""
- role: ""
- role: master
- role: master
- role: master

We need to iterate over the hosts twice, once to count the explicit roles and once for the implicit ones.

Copy link
Copy Markdown
Contributor

@andfasano andfasano Jun 9, 2023

Choose a reason for hiding this comment

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

I think that the original validation was taken from the #5205, where the very same test scenario was addressed by having the hosts sorted by role before applying the validation.

Copy link
Copy Markdown
Contributor

@bfournie bfournie Jun 9, 2023

Choose a reason for hiding this comment

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

Thanks @andfasano, yes it was based on the baremetal validation. As this list will be sorted I think we can use the one loop as it currently has and not need to iterate over the hosts a 2nd time.


if numMasters != numRequiredMasters {
logrus.Warnf("The number of hosts configured as masters (%d) does not match the master replicas (%d)", numMasters, numRequiredMasters)
}
if numWorkers != numRequiredWorkers {
Copy link
Copy Markdown
Member

@zaneb zaneb Jun 9, 2023

Choose a reason for hiding this comment

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

Given that defining hosts is completely optional, I don't think we should have a warning in the case where numWorkers == 0 (and likewise for numMasters == 0 above).
It would be really unusual to define host-specific data for only some hosts in a particular role, so I think in those cases it makes sense to have a warning.

Copy link
Copy Markdown
Contributor

@bfournie bfournie Jun 9, 2023

Choose a reason for hiding this comment

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

I'll add a check for numWorkers/numMasters != 0. Per the previous comment I'll add this a new master PR.

logrus.Warnf("The number of hosts configured as workers (%d) does not match the worker replicas (%d)", numWorkers, numRequiredWorkers)
}
}