MCO-2032: Avoid MCPs duplication at install time

manifests/manifests_pools.go

@@ -0,0 +1,56 @@
+package manifests
+
+import (
+	"fmt"
+	"reflect"
+
+	mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
+	"github.com/openshift/machine-config-operator/lib/resourceread"
+)
+
+const (
+	ManifestFileNameMCPMaster  ManifestMachineConfigPool = "master.machineconfigpool.yaml"
+	ManifestFileNameMCPWorker  ManifestMachineConfigPool = "worker.machineconfigpool.yaml"
+	ManifestFileNameMCPArbiter ManifestMachineConfigPool = "arbiter.machineconfigpool.yaml"
+)
+
+// ManifestMachineConfigPool represents the filename of a built-in MachineConfigPool manifest
+// embedded in the operator.
+type ManifestMachineConfigPool string
+
+// GetMachineConfigPool reads and parses the given built-in MachineConfigPool manifest.
+func GetMachineConfigPool(pool ManifestMachineConfigPool) (*mcfgv1.MachineConfigPool, error) {
+	content, err := ReadFile(string(pool))
+	if err != nil {
+		return nil, fmt.Errorf("error reading pool manifest %s: %v", string(pool), err)
+	}
+	return resourceread.ReadMachineConfigPoolV1OrDie(content), nil
+}
+
+// GetMachineConfigPools reads and parses all built-in MachineConfigPool manifests
+// (master, worker, and arbiter)
+func GetMachineConfigPools() ([]*mcfgv1.MachineConfigPool, error) {
+	var pools []*mcfgv1.MachineConfigPool
+	for _, pool := range []ManifestMachineConfigPool{ManifestFileNameMCPMaster, ManifestFileNameMCPWorker, ManifestFileNameMCPArbiter} {
+		poolObj, err := GetMachineConfigPool(pool)
+		if err != nil {
+			return nil, err
+		}
+		pools = append(pools, poolObj)
+	}
+	return pools, nil
+}
+
+// ContainsMachineConfigPool reports whether pool is identical (via reflect.DeepEqual) to any
+// of the pools in generatedPools.
+func ContainsMachineConfigPool(generatedPools []*mcfgv1.MachineConfigPool, pool *mcfgv1.MachineConfigPool) bool {
+	if pool == nil || generatedPools == nil {
+		return false
+	}
+	for _, generated := range generatedPools {
+		if reflect.DeepEqual(generated, pool) {
+			return true
+		}
+	}
+	return false
+}

pkg/controller/bootstrap/bootstrap.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"time"
 
+	"github.com/openshift/machine-config-operator/manifests"
 	"github.com/openshift/machine-config-operator/pkg/imageutils"
 	"github.com/openshift/machine-config-operator/pkg/osimagestream"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -218,6 +219,11 @@ func (b *Bootstrap) Run(destDir string) error {
 		return fmt.Errorf("error creating feature gates handler: %w", err)
 	}
 
+	// Deduplicate pools to avoid generated ones conflict with user overrides
+	if pools, err = filterPools(pools); err != nil {
+		return fmt.Errorf("error filtering pools: %w", err)
+	}
+
 	var osImageStream *mcfgv1alpha1.OSImageStream
 	// Enable OSImageStreams if the FeatureGate is active and the deployment is not OKD
 	if osimagestream.IsFeatureEnabled(fgHandler) {
@@ -403,6 +409,51 @@ func (b *Bootstrap) Run(destDir string) error {
 
 }
 
+// filterPools deduplicates MachineConfigPools by name. When multiple pools share the same
+// name, user-provided pools (those not identical to the operator's built-in manifests) are
+// preferred over automatically generated ones.
+func filterPools(pools []*mcfgv1.MachineConfigPool) ([]*mcfgv1.MachineConfigPool, error) {
+	groupedPools := make(map[string][]*mcfgv1.MachineConfigPool)
+
+	// Group the pools by name
+	for _, pool := range pools {
+		groupedPools[pool.Name] = append(groupedPools[pool.Name], pool)
+	}
+
+	generatedPools, err := manifests.GetMachineConfigPools()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get generated MCPs: %w", err)
+	}
+
+	// Now ensure we have only one pool, no duplicates
+	// Discard generated ones if required
+	var result []*mcfgv1.MachineConfigPool
+	for _, poolObjs := range groupedPools {
+		if len(poolObjs) == 1 {
+			// Nothing to do, it's already unique
+			result = append(result, poolObjs[0])
+			continue
+		}
+
+		// Remove generated ones first
+		var lastPool *mcfgv1.MachineConfigPool
+		for _, poolObj := range poolObjs {
+			if manifests.ContainsMachineConfigPool(generatedPools, poolObj) {
+				// Handle a dev error in case the same pool is
+				// generated more than once. In that case, keep at
+				// least one generated pool
+				if lastPool == nil {
+					lastPool = poolObj
+				}
+				continue
+			}
+			lastPool = poolObj
+		}
+		result = append(result, lastPool)
+	}
+	return result, nil
+}
+
 func (b *Bootstrap) fetchOSImageStream(
 	imageStream *imagev1.ImageStream,
 	cconfig *mcfgv1.ControllerConfig,

pkg/operator/sync.go

@@ -18,6 +18,8 @@ import (
 	"time"
 
 	configclientscheme "github.com/openshift/client-go/config/clientset/versioned/scheme"
+	"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
+	"github.com/openshift/machine-config-operator/manifests"
 	"github.com/openshift/machine-config-operator/pkg/osimagestream"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -35,6 +37,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/klog/v2"
+	"k8s.io/utils/ptr"
 
 	"github.com/openshift/api/annotations"
 	configv1 "github.com/openshift/api/config/v1"
@@ -715,23 +718,41 @@ func getIgnitionHost(infraStatus *configv1.InfrastructureStatus) (string, error)
 }
 
 func (optr *Operator) syncMachineConfigPools(config *renderConfig, _ *configv1.ClusterOperator) error {
-	mcps := []string{
-		"manifests/master.machineconfigpool.yaml",
-		"manifests/worker.machineconfigpool.yaml",
+	generatedPools, err := manifests.GetMachineConfigPools()
+	if err != nil {
+		return fmt.Errorf("failed to get generated MCPs: %w", err)
 	}
+	for _, mcp := range generatedPools {
+		if config.Infra.Status.ControlPlaneTopology != configv1.HighlyAvailableArbiterMode && mcp.Name == ctrlcommon.MachineConfigPoolArbiter {
+			continue
+		}
 
-	if config.Infra.Status.ControlPlaneTopology == configv1.HighlyAvailableArbiterMode {
-		mcps = append(mcps, "manifests/arbiter.machineconfigpool.yaml")
-	}
+		existing, err := optr.client.MachineconfigurationV1().MachineConfigPools().Get(context.TODO(), mcp.GetName(), metav1.GetOptions{})
+		if err != nil && !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to get MachineConfigPool %s: %w", mcp.GetName(), err)
+		}
 
-	for _, mcp := range mcps {
-		mcpBytes, err := renderAsset(config, mcp)
-		if err != nil {
-			return err
+		// If the MCP doesn't exist, create it
+		if apierrors.IsNotFound(err) {
+			klog.Infof("Creating managed MachineConfigPool %s as it does not exist", mcp.GetName())
+			if _, err := optr.client.MachineconfigurationV1().MachineConfigPools().Create(context.TODO(), mcp, metav1.CreateOptions{}); err != nil {
+				return fmt.Errorf("failed to create MachineConfigPool %s: %w", mcp.GetName(), err)
+			}
+			continue
 		}
-		p := mcoResourceRead.ReadMachineConfigPoolV1OrDie(mcpBytes)
-		_, _, err = mcoResourceApply.ApplyMachineConfigPool(optr.client.MachineconfigurationV1(), p)
-		if err != nil {
+
+		// Ensure the metadata we manage and want to enforce is present
+		// We don't care about the spec, the user is free to touch parameters
+		// in the spec, and we don't want to interfere with them. Merge strategies
+		// won't work in the spec as it won't allow a user to reset a field to blank
+		modified := ptr.To(false)
+		resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, mcp.ObjectMeta)
+		if !*modified {
+			continue
+		}
+
+		// The metadata has changed, apply it
+		if _, err := optr.client.MachineconfigurationV1().MachineConfigPools().Update(context.TODO(), existing, metav1.UpdateOptions{}); err != nil {
 			return err
 		}
 	}