Skip to content

OCPBUGS-64793: Infrastructure Topology calculation is wrong #10077

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −12
Open

OCPBUGS-64793: Infrastructure Topology calculation is wrong #10077

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
8 changes: 4 additions & 4 deletions pkg/asset/manifests/ingress_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,15 +135,15 @@ func TestGenerateIngerssDefaultPlacement(t *testing.T) {
name: "aws single node with 0 or 1 day-1 workers",
installConfigBuildOptions: []icOption{icBuild.forAWS()},
controlPlaneTopology: configv1.SingleReplicaTopologyMode,
infrastructureTopology: configv1.SingleReplicaTopologyMode,
infrastructureTopology: configv1.HighlyAvailableTopologyMode,
expectedIngressAWSLBType: configv1.Classic,
expectedIngressPlacement: configv1.DefaultPlacementWorkers,
},
{
name: "aws multi-node with 1 day-1 worker",
installConfigBuildOptions: []icOption{icBuild.forAWS()},
controlPlaneTopology: configv1.HighlyAvailableTopologyMode,
infrastructureTopology: configv1.SingleReplicaTopologyMode,
infrastructureTopology: configv1.HighlyAvailableTopologyMode,
expectedIngressAWSLBType: configv1.Classic,
expectedIngressPlacement: configv1.DefaultPlacementWorkers,
},
Expand Down Expand Up @@ -187,14 +187,14 @@ func TestGenerateIngerssDefaultPlacement(t *testing.T) {
name: "none-platform single node with 0 or 1 day-1 workers",
installConfigBuildOptions: []icOption{icBuild.forNone()},
controlPlaneTopology: configv1.SingleReplicaTopologyMode,
infrastructureTopology: configv1.SingleReplicaTopologyMode,
infrastructureTopology: configv1.HighlyAvailableTopologyMode,
expectedIngressPlacement: configv1.DefaultPlacementControlPlane,
},
{
name: "none-platform multi-node with 1 day-1 worker",
installConfigBuildOptions: []icOption{icBuild.forNone()},
controlPlaneTopology: configv1.HighlyAvailableTopologyMode,
infrastructureTopology: configv1.SingleReplicaTopologyMode,
infrastructureTopology: configv1.HighlyAvailableTopologyMode,
Copy link
Contributor

@sadasu sadasu Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

When there is a single control-plane node and 0 compute nodes, we should end up with controlPlaneTopology==configv1.SingleReplicaTopologyMode and infrastructureTopology==configv1.SingleReplicaTopologyMode. I don't see a test for that.

Copy link
Contributor Author

@barbacbd barbacbd Nov 14, 2025

Choose a reason for hiding this comment

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

https://github.com/openshift/installer/blob/main/pkg/asset/manifests/topologies_test.go#L31 This test should take care of that.

Copy link
Contributor

@sadasu sadasu Nov 14, 2025

Choose a reason for hiding this comment

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

Thanks!

expectedIngressPlacement: configv1.DefaultPlacementWorkers,
},
{
Expand Down
9 changes: 2 additions & 7 deletions pkg/asset/manifests/scheduler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,9 @@ func (s *Scheduler) Generate(_ context.Context, dependencies asset.Parents) erro
computeReplicas += *pool.Replicas
}
}
if computeReplicas == 0 {
if computeReplicas < 2 {
// A schedulable host is required for a successful install to complete.
// If the install config has 0 replicas for compute hosts, it's one of two cases:
// 1. An IPI deployment with no compute hosts. The deployment can not succeed
// without MastersSchedulable = true.
// 2. A UPI deployment. The deployment may add compute hosts, but to ensure the
// the highest probability of a successful deployment, we default to
// schedulable masters.
// Set the control planes to schedulable if the install config has < 2 replicas for compute hosts.
logrus.Warningf("Making control-plane schedulable by setting MastersSchedulable to true for Scheduler cluster settings")
config.Spec.MastersSchedulable = true
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/asset/manifests/topologies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ func determineTopologies(installConfig *types.InstallConfig) (controlPlaneTopolo
for _, mp := range installConfig.Compute {
numOfWorkers += ptr.Deref(mp.Replicas, 0)
}
if numOfWorkers < 2 {
// Control planes are schedulable when there are < 2 workers.
// Adjust the number of schedulable nodes here to reflect.
numOfWorkers += controlPlaneReplicas
}

switch numOfWorkers {
case 0:
Comment on lines +31 to 38
Copy link
Member

@tthvo tthvo Nov 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

By adding this check, the numOfWorkers is never 0 because number of control plane nodes cannot be 0, right? This makes case 0 unreachable.

installer/pkg/asset/manifests/topologies.go

Lines 33 to 41 in 4180662

case 0:
// Two node deployments with 0 workers mean that the control plane nodes are treated as workers
// in that situation we have decided that it is appropriate to set the infrastructureTopology to HA.
// All other configuration for different worker count are respected with the original intention.
if controlPlaneTopology == configv1.DualReplicaTopologyMode || controlPlaneTopology == configv1.HighlyAvailableArbiterMode {
infrastructureTopology = configv1.HighlyAvailableTopologyMode
} else {
infrastructureTopology = controlPlaneTopology
}

IIUC, it is OK. When using deploying 2-node or arbiter cluster, the infrastructure topology should configv1.HighlyAvailableTopologyMode, which is already handled by the default: case.

/cc @eggfoobar @jaypoulz

Copy link
Member

@zaneb zaneb Nov 16, 2025

Choose a reason for hiding this comment

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

I think this change is effectively applying the same rules that we use for 2-node/arbiter case to everything. So it's valid, but the comments and code below that suggest there's still a special case that applies only to 2-node/arbiter clusters is now misleading. Better to delete it I think.

Expand Down
17 changes: 16 additions & 1 deletion pkg/asset/manifests/topologies_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func Test_DetermineTopologies(t *testing.T) {
},
},
expectedControlPlane: configv1.SingleReplicaTopologyMode,
expectedInfra: configv1.SingleReplicaTopologyMode,
expectedInfra: configv1.HighlyAvailableTopologyMode,
},
{
desc: "should default infra to HA and controlPlane to DualReplica for 2 control replicas",
Expand Down Expand Up @@ -125,6 +125,21 @@ func Test_DetermineTopologies(t *testing.T) {
expectedControlPlane: configv1.HighlyAvailableTopologyMode,
expectedInfra: configv1.HighlyAvailableTopologyMode,
},
{
desc: "should set infra to HA controlPlane and compute from controlPlane value",
installConfig: &types.InstallConfig{
ControlPlane: &types.MachinePool{
Replicas: ptr.To[int64](3),
},
Compute: []types.MachinePool{
{
Replicas: ptr.To[int64](1),
},
},
},
expectedControlPlane: configv1.HighlyAvailableTopologyMode,
expectedInfra: configv1.HighlyAvailableTopologyMode,
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
Expand Down