From 9e2da6cd0dc8646ba542fa727eab2c8019b454b9 Mon Sep 17 00:00:00 2001 From: Patrick Dillon Date: Tue, 3 Mar 2026 15:30:03 -0500 Subject: [PATCH] OCPBUGS-64793: use master schedulability for topology calculation When masters are schedulable OLM and ingress operator (and anything else) can schedule infrastructure to the control plane. So the infrastructure topology needs to take master schedulability into account. --- pkg/asset/manifests/infrastructure.go | 6 ++++-- pkg/asset/manifests/infrastructure_test.go | 12 ++++++++++- pkg/asset/manifests/ingress.go | 3 ++- pkg/asset/manifests/ingress_test.go | 11 +++++++++- pkg/asset/manifests/scheduler.go | 25 +++++++++++++++++++--- pkg/asset/manifests/topologies.go | 23 ++++++++++---------- pkg/asset/manifests/topologies_test.go | 23 +++++++++++++++++++- 7 files changed, 82 insertions(+), 21 deletions(-) diff --git a/pkg/asset/manifests/infrastructure.go b/pkg/asset/manifests/infrastructure.go index bc1ac019e31..efa5d87a3bb 100644 --- a/pkg/asset/manifests/infrastructure.go +++ b/pkg/asset/manifests/infrastructure.go @@ -60,6 +60,7 @@ func (*Infrastructure) Dependencies() []asset.Asset { &installconfig.InstallConfig{}, &CloudProviderConfig{}, &AdditionalTrustBundleConfig{}, + &Scheduler{}, } } @@ -72,7 +73,8 @@ func (i *Infrastructure) Generate(ctx context.Context, dependencies asset.Parent installConfig := &installconfig.InstallConfig{} cloudproviderconfig := &CloudProviderConfig{} trustbundleconfig := &AdditionalTrustBundleConfig{} - dependencies.Get(clusterID, installConfig, cloudproviderconfig, trustbundleconfig) + scheduler := &Scheduler{} + dependencies.Get(clusterID, installConfig, cloudproviderconfig, trustbundleconfig, scheduler) config := &configv1.Infrastructure{ TypeMeta: metav1.TypeMeta{ @@ -94,7 +96,7 @@ func (i *Infrastructure) Generate(ctx context.Context, dependencies asset.Parent }, } - controlPlaneTopology, infrastructureTopology := determineTopologies(installConfig.Config) + controlPlaneTopology, infrastructureTopology := determineTopologies(installConfig.Config, scheduler.MastersSchedulable) config.Status.InfrastructureTopology = infrastructureTopology config.Status.ControlPlaneTopology = controlPlaneTopology diff --git a/pkg/asset/manifests/infrastructure_test.go b/pkg/asset/manifests/infrastructure_test.go index f3fcbd69693..71ccc448f32 100644 --- a/pkg/asset/manifests/infrastructure_test.go +++ b/pkg/asset/manifests/infrastructure_test.go @@ -284,6 +284,15 @@ func TestGenerateInfrastructure(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + // Generate Scheduler asset + scheduler := &Scheduler{} + schedulerParents := asset.Parents{} + schedulerParents.Add(installconfig.MakeAsset(tc.installConfig)) + err := scheduler.Generate(context.Background(), schedulerParents) + if !assert.NoError(t, err, "failed to generate scheduler") { + return + } + parents := asset.Parents{} parents.Add( &installconfig.ClusterID{ @@ -293,9 +302,10 @@ func TestGenerateInfrastructure(t *testing.T) { installconfig.MakeAsset(tc.installConfig), &CloudProviderConfig{}, &AdditionalTrustBundleConfig{}, + scheduler, ) infraAsset := &Infrastructure{} - err := infraAsset.Generate(context.Background(), parents) + err = infraAsset.Generate(context.Background(), parents) if !assert.NoError(t, err, "failed to generate asset") { return } diff --git a/pkg/asset/manifests/ingress.go b/pkg/asset/manifests/ingress.go index 1458732dafe..f88ce3e57cb 100644 --- a/pkg/asset/manifests/ingress.go +++ b/pkg/asset/manifests/ingress.go @@ -79,7 +79,8 @@ func (ing *Ingress) Generate(_ context.Context, dependencies asset.Parents) erro } func (ing *Ingress) generateClusterConfig(config *types.InstallConfig) ([]byte, error) { - controlPlaneTopology, _ := determineTopologies(config) + // mastersSchedulable doesn't affect controlPlaneTopology determination, so we pass false + controlPlaneTopology, _ := determineTopologies(config, false) isSingleControlPlaneNode := controlPlaneTopology == configv1.SingleReplicaTopologyMode diff --git a/pkg/asset/manifests/ingress_test.go b/pkg/asset/manifests/ingress_test.go index 1d76c91a1fe..98f6d7d6f65 100644 --- a/pkg/asset/manifests/ingress_test.go +++ b/pkg/asset/manifests/ingress_test.go @@ -46,8 +46,17 @@ func installConfigFromTopologies(t *testing.T, options []icOption, } } + // Compute mastersSchedulable based on worker count (same logic as Scheduler) + computeReplicas := int64(0) + for _, pool := range installConfig.Compute { + if pool.Replicas != nil { + computeReplicas += *pool.Replicas + } + } + mastersSchedulable := computeReplicas == 0 + // Assert that this function actually works - generatedControlPlaneTopology, generatedInfrastructureTopology := determineTopologies(installConfig) + generatedControlPlaneTopology, generatedInfrastructureTopology := determineTopologies(installConfig, mastersSchedulable) assert.Equal(t, generatedControlPlaneTopology, controlPlaneTopology) assert.Equal(t, generatedInfrastructureTopology, infrastructureTopology) diff --git a/pkg/asset/manifests/scheduler.go b/pkg/asset/manifests/scheduler.go index 4222ed3e2ab..e724f1e6395 100644 --- a/pkg/asset/manifests/scheduler.go +++ b/pkg/asset/manifests/scheduler.go @@ -2,6 +2,7 @@ package manifests import ( "context" + "os" "path" "github.com/pkg/errors" @@ -21,7 +22,8 @@ var ( // Scheduler generates the cluster-scheduler-*.yml files. type Scheduler struct { - FileList []*asset.File + FileList []*asset.File + MastersSchedulable bool } var _ asset.WritableAsset = (*Scheduler)(nil) @@ -86,6 +88,7 @@ func (s *Scheduler) Generate(_ context.Context, dependencies asset.Parents) erro Data: configData, }, } + s.MastersSchedulable = config.Spec.MastersSchedulable return nil } @@ -95,7 +98,23 @@ func (s *Scheduler) Files() []*asset.File { return s.FileList } -// Load returns false since this asset is not written to disk by the installer. +// Load loads the Scheduler manifest from disk if it exists. func (s *Scheduler) Load(f asset.FileFetcher) (bool, error) { - return false, nil + file, err := f.FetchByName(SchedulerCfgFilename) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, err + } + + config := &configv1.Scheduler{} + if err := yaml.Unmarshal(file.Data, config); err != nil { + return false, errors.Wrapf(err, "failed to unmarshal %s", SchedulerCfgFilename) + } + + s.FileList = []*asset.File{file} + s.MastersSchedulable = config.Spec.MastersSchedulable + + return true, nil } diff --git a/pkg/asset/manifests/topologies.go b/pkg/asset/manifests/topologies.go index 5d837e83d1d..6ffad62ee44 100644 --- a/pkg/asset/manifests/topologies.go +++ b/pkg/asset/manifests/topologies.go @@ -9,7 +9,7 @@ import ( // determineTopologies determines the Infrastructure CR's // infrastructureTopology and controlPlaneTopology given an install config file -func determineTopologies(installConfig *types.InstallConfig) (controlPlaneTopology configv1.TopologyMode, infrastructureTopology configv1.TopologyMode) { +func determineTopologies(installConfig *types.InstallConfig, mastersSchedulable bool) (controlPlaneTopology configv1.TopologyMode, infrastructureTopology configv1.TopologyMode) { controlPlaneReplicas := ptr.Deref(installConfig.ControlPlane.Replicas, 3) switch controlPlaneReplicas { case 1: @@ -29,17 +29,16 @@ func determineTopologies(installConfig *types.InstallConfig) (controlPlaneTopolo numOfWorkers += ptr.Deref(mp.Replicas, 0) } - switch numOfWorkers { - 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 - } - case 1: + switch { + case numOfWorkers == 0 && controlPlaneTopology == configv1.DualReplicaTopologyMode: + infrastructureTopology = configv1.HighlyAvailableTopologyMode + case numOfWorkers == 0 && controlPlaneTopology == configv1.HighlyAvailableArbiterMode: + infrastructureTopology = configv1.HighlyAvailableTopologyMode + case numOfWorkers == 0: + infrastructureTopology = controlPlaneTopology + case numOfWorkers == 1 && mastersSchedulable: + infrastructureTopology = configv1.HighlyAvailableTopologyMode + case numOfWorkers == 1: infrastructureTopology = configv1.SingleReplicaTopologyMode default: infrastructureTopology = configv1.HighlyAvailableTopologyMode diff --git a/pkg/asset/manifests/topologies_test.go b/pkg/asset/manifests/topologies_test.go index 4c96fdc7533..0a868063c0a 100644 --- a/pkg/asset/manifests/topologies_test.go +++ b/pkg/asset/manifests/topologies_test.go @@ -13,6 +13,7 @@ func Test_DetermineTopologies(t *testing.T) { testCases := []struct { desc string installConfig *types.InstallConfig + mastersSchedulable bool expectedControlPlane configv1.TopologyMode expectedInfra configv1.TopologyMode }{ @@ -24,6 +25,7 @@ func Test_DetermineTopologies(t *testing.T) { }, Compute: []types.MachinePool{}, }, + mastersSchedulable: true, expectedControlPlane: configv1.HighlyAvailableTopologyMode, expectedInfra: configv1.HighlyAvailableTopologyMode, }, @@ -35,6 +37,7 @@ func Test_DetermineTopologies(t *testing.T) { }, Compute: []types.MachinePool{}, }, + mastersSchedulable: true, expectedControlPlane: configv1.SingleReplicaTopologyMode, expectedInfra: configv1.SingleReplicaTopologyMode, }, @@ -50,6 +53,7 @@ func Test_DetermineTopologies(t *testing.T) { }, }, }, + mastersSchedulable: false, expectedControlPlane: configv1.SingleReplicaTopologyMode, expectedInfra: configv1.HighlyAvailableTopologyMode, }, @@ -65,6 +69,7 @@ func Test_DetermineTopologies(t *testing.T) { }, }, }, + mastersSchedulable: false, expectedControlPlane: configv1.SingleReplicaTopologyMode, expectedInfra: configv1.SingleReplicaTopologyMode, }, @@ -76,6 +81,7 @@ func Test_DetermineTopologies(t *testing.T) { }, Compute: []types.MachinePool{}, }, + mastersSchedulable: true, expectedControlPlane: configv1.DualReplicaTopologyMode, expectedInfra: configv1.HighlyAvailableTopologyMode, }, @@ -90,6 +96,7 @@ func Test_DetermineTopologies(t *testing.T) { }, Compute: []types.MachinePool{}, }, + mastersSchedulable: true, expectedControlPlane: configv1.HighlyAvailableArbiterMode, expectedInfra: configv1.HighlyAvailableTopologyMode, }, @@ -104,6 +111,7 @@ func Test_DetermineTopologies(t *testing.T) { }, Compute: []types.MachinePool{}, }, + mastersSchedulable: true, expectedControlPlane: configv1.HighlyAvailableArbiterMode, expectedInfra: configv1.HighlyAvailableTopologyMode, }, @@ -122,13 +130,26 @@ func Test_DetermineTopologies(t *testing.T) { }, }, }, + mastersSchedulable: false, + expectedControlPlane: configv1.HighlyAvailableTopologyMode, + expectedInfra: configv1.HighlyAvailableTopologyMode, + }, + { + desc: "should default infra to HA when control plane is HA, 0 workers, and masters are not schedulable", + installConfig: &types.InstallConfig{ + ControlPlane: &types.MachinePool{ + Replicas: ptr.To[int64](3), + }, + Compute: []types.MachinePool{}, + }, + mastersSchedulable: false, expectedControlPlane: configv1.HighlyAvailableTopologyMode, expectedInfra: configv1.HighlyAvailableTopologyMode, }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - controlPlane, infra := determineTopologies(tc.installConfig) + controlPlane, infra := determineTopologies(tc.installConfig, tc.mastersSchedulable) if controlPlane != tc.expectedControlPlane { t.Fatalf("expected control plane topology to be %s but got %s", tc.expectedControlPlane, controlPlane) }