From 8a96e8e0aa4133c07e0281d7df40b3a07b07f69e Mon Sep 17 00:00:00 2001 From: Tesshu Flower Date: Thu, 2 Jul 2026 21:04:13 -0400 Subject: [PATCH] :bug: fix: preserve config reference ordering in cmainstallprogression controller (#1610) * fix: preserve config reference ordering in cmainstallprogression controller Replace sets.Set[ConfigReferent] with []ConfigReferent in setInstallProgression and overrideConfigMapByAddOnConfigs. Iterating a set (Go map) produces nondeterministic ordering when a placement has multiple configs of the same GVK, causing spurious status patches on every reconcile and unstable config ordering downstream to MCA status. The fix preserves the insertion order from the CMA spec's placement configs, matching the pattern already used by overrideConfigMapByAddOnConfigs in pkg/addon/controllers/addonconfiguration/graph.go. Deduplication is maintained via a containsConfigReferent helper (linear scan), consistent with the addonconfiguration controller's containsConfig method. Fixes: https://github.com/open-cluster-management-io/ocm/issues/1609 Signed-off-by: Tesshu Flower * test: add integration test for multi-same-GVK config reference ordering Adds an integration test that verifies the full pipeline when a CMA placement has two configs of the same GVK. Asserts that both CMA status InstallProgressions and MCA status ConfigReferences reflect the configs in the order specified in the CMA placement spec (config-first before config-second), not in a nondeterministic order. This exercises the full controller chain: - cmainstallprogression controller: CMA spec -> CMA status - addonconfiguration controller: CMA status -> MCA status Signed-off-by: Tesshu Flower --------- Signed-off-by: Tesshu Flower --- .../cmainstallprogression/controller.go | 43 +++-- .../cmainstallprogression/controller_test.go | 20 ++- test/integration/addon/addon_configs_test.go | 165 ++++++++++++++++++ 3 files changed, 210 insertions(+), 18 deletions(-) diff --git a/pkg/addon/controllers/cmainstallprogression/controller.go b/pkg/addon/controllers/cmainstallprogression/controller.go index 0b73f2587..b408a0d8d 100644 --- a/pkg/addon/controllers/cmainstallprogression/controller.go +++ b/pkg/addon/controllers/cmainstallprogression/controller.go @@ -5,7 +5,6 @@ import ( "sort" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" addonv1beta1 "open-cluster-management.io/api/addon/v1beta1" @@ -130,17 +129,15 @@ func setInstallProgression(supportedConfigs []addonv1beta1.AddOnConfig, placemen } // set config references as default configuration - installConfigReferencesMap := map[addonv1beta1.ConfigGroupResource]sets.Set[addonv1beta1.ConfigReferent]{} + installConfigReferencesMap := map[addonv1beta1.ConfigGroupResource][]addonv1beta1.ConfigReferent{} for _, config := range supportedConfigs { if config.ConfigReferent.Name == "" || config.ConfigReferent.Name == addonv1beta1.ReservedNoDefaultConfigName { continue } - refs, ok := installConfigReferencesMap[config.ConfigGroupResource] - if !ok { - refs = sets.New[addonv1beta1.ConfigReferent]() - installConfigReferencesMap[config.ConfigGroupResource] = refs + if !containsConfigReferent(installConfigReferencesMap[config.ConfigGroupResource], config.ConfigReferent) { + installConfigReferencesMap[config.ConfigGroupResource] = append( + installConfigReferencesMap[config.ConfigGroupResource], config.ConfigReferent) } - refs.Insert(config.ConfigReferent) } // override the default configuration for each placement @@ -162,7 +159,7 @@ func setInstallProgression(supportedConfigs []addonv1beta1.AddOnConfig, placemen installConfigReferences := []addonv1beta1.InstallConfigReference{} for _, gvk := range gvks { if configRefs, ok := installConfigReferencesMap[gvk]; ok { - for configRef := range configRefs { + for _, configRef := range configRefs { installConfigReferences = append(installConfigReferences, addonv1beta1.InstallConfigReference{ ConfigGroupResource: gvk, @@ -223,23 +220,37 @@ func mergeInstallProgression(newobj, oldobj *addonv1beta1.InstallProgression) { } // Override the desired configs by a slice of AddOnConfig (from cma install strategy), +// preserving the order in which configs appear in addOnConfigs and deduplicating by +// ConfigReferent (namespace + name) within each GVK. func overrideConfigMapByAddOnConfigs( - desiredConfigs map[addonv1beta1.ConfigGroupResource]sets.Set[addonv1beta1.ConfigReferent], + desiredConfigs map[addonv1beta1.ConfigGroupResource][]addonv1beta1.ConfigReferent, addOnConfigs []addonv1beta1.AddOnConfig, ) { - gvkOverwritten := sets.New[addonv1beta1.ConfigGroupResource]() + gvkOverwritten := map[addonv1beta1.ConfigGroupResource]bool{} // Go through the cma install strategy configs, // for a group of configs with same gvk, install strategy configs override the desiredConfigs. for _, config := range addOnConfigs { gr := config.ConfigGroupResource - if !gvkOverwritten.Has(gr) { - desiredConfigs[gr] = sets.New[addonv1beta1.ConfigReferent]() - gvkOverwritten.Insert(gr) + if !gvkOverwritten[gr] { + desiredConfigs[gr] = []addonv1beta1.ConfigReferent{} + gvkOverwritten[gr] = true } - // If a config not exist in the desiredConfigs, append it. + // If a config does not exist in the desiredConfigs, append it. // This is to avoid adding duplicate configs (name + namespace). - if !desiredConfigs[gr].Has(config.ConfigReferent) { - desiredConfigs[gr].Insert(config.ConfigReferent) + if !containsConfigReferent(desiredConfigs[gr], config.ConfigReferent) { + desiredConfigs[gr] = append(desiredConfigs[gr], config.ConfigReferent) } } } + +// containsConfigReferent returns true if the given ConfigReferent exists in the slice. +// ConfigReferent is a struct of two strings (Namespace, Name), so == performs a +// field-by-field value comparison equivalent to r.Namespace == ref.Namespace && r.Name == ref.Name. +func containsConfigReferent(refs []addonv1beta1.ConfigReferent, ref addonv1beta1.ConfigReferent) bool { + for _, r := range refs { + if r == ref { + return true + } + } + return false +} diff --git a/pkg/addon/controllers/cmainstallprogression/controller_test.go b/pkg/addon/controllers/cmainstallprogression/controller_test.go index 1558128f9..8e0a55721 100644 --- a/pkg/addon/controllers/cmainstallprogression/controller_test.go +++ b/pkg/addon/controllers/cmainstallprogression/controller_test.go @@ -243,7 +243,16 @@ func TestReconcile(t *testing.T) { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) } if len(cma.Status.InstallProgressions[2].ConfigReferences) != 2 { - t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + t.Fatalf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + } + // Verify ordering matches the spec order: test/test first, then test1/test + if cma.Status.InstallProgressions[2].ConfigReferences[0].DesiredConfig.Namespace != "test" || + cma.Status.InstallProgressions[2].ConfigReferences[0].DesiredConfig.Name != "test" { + t.Errorf("InstallProgressions ConfigReferences[0] ordering is not correct: %v", cma.Status.InstallProgressions[2].ConfigReferences[0].DesiredConfig) + } + if cma.Status.InstallProgressions[2].ConfigReferences[1].DesiredConfig.Namespace != "test1" || + cma.Status.InstallProgressions[2].ConfigReferences[1].DesiredConfig.Name != "test" { + t.Errorf("InstallProgressions ConfigReferences[1] ordering is not correct: %v", cma.Status.InstallProgressions[2].ConfigReferences[1].DesiredConfig) } if len(cma.Status.InstallProgressions[3].ConfigReferences) != 1 { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) @@ -362,7 +371,14 @@ func TestReconcile(t *testing.T) { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig.Name) } if len(cma.Status.InstallProgressions[1].ConfigReferences) != 2 { - t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + t.Fatalf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + } + // Verify ordering matches the spec order: test1 first, then test2 + if cma.Status.InstallProgressions[1].ConfigReferences[0].DesiredConfig.Name != "test1" { + t.Errorf("InstallProgressions ConfigReferences[0] ordering is not correct: %v", cma.Status.InstallProgressions[1].ConfigReferences[0].DesiredConfig) + } + if cma.Status.InstallProgressions[1].ConfigReferences[1].DesiredConfig.Name != "test2" { + t.Errorf("InstallProgressions ConfigReferences[1] ordering is not correct: %v", cma.Status.InstallProgressions[1].ConfigReferences[1].DesiredConfig) } if len(cma.Status.InstallProgressions[2].ConfigReferences) != 3 { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) diff --git a/test/integration/addon/addon_configs_test.go b/test/integration/addon/addon_configs_test.go index 5fb9ddccc..3bccbcf14 100644 --- a/test/integration/addon/addon_configs_test.go +++ b/test/integration/addon/addon_configs_test.go @@ -440,4 +440,169 @@ var _ = ginkgo.Describe("AddConfigs Beta", func() { }, }) }) + + ginkgo.It("Should preserve config reference ordering for multiple same-GVK configs in a placement", func() { + // Create two AddonDeploymentConfigs of the same GVK in the placement namespace. + // The ordering in the CMA placement spec should be preserved in both the CMA status + // InstallProgressions and the MCA status ConfigReferences. + config1 := &addonapiv1beta1.AddOnDeploymentConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-first", + Namespace: configDefaultNamespace, + }, + Spec: addOnTest1ConfigSpecBeta, + } + _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(configDefaultNamespace).Create( + context.Background(), config1, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + config2 := &addonapiv1beta1.AddOnDeploymentConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-second", + Namespace: configDefaultNamespace, + }, + Spec: addOnTest2ConfigSpecBeta, + } + _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(configDefaultNamespace).Create( + context.Background(), config2, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Patch CMA to use a placement strategy with 2 configs of the same GVK. + // The ordering config-first → config-second must be preserved. + cma, err := hubAddonClient.AddonV1beta1().ClusterManagementAddOns().Get( + context.Background(), testAddOnConfigsImpl.name, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + cma.Spec.InstallStrategy = addonapiv1beta1.InstallStrategy{ + Type: addonapiv1beta1.AddonInstallStrategyPlacements, + Placements: []addonapiv1beta1.PlacementStrategy{ + { + PlacementRef: addonapiv1beta1.PlacementRef{ + Name: "test-placement", + Namespace: configDefaultNamespace, + }, + Configs: []addonapiv1beta1.AddOnConfig{ + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-first", + }, + }, + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-second", + }, + }, + }, + RolloutStrategy: clusterv1alpha1.RolloutStrategy{ + Type: clusterv1alpha1.All, + }, + }, + }, + } + patchClusterManagementAddOnBeta(context.Background(), cma) + + // Create placement and placement decision so the cluster is selected. + placement := &clusterv1beta1.Placement{ + ObjectMeta: metav1.ObjectMeta{Name: "test-placement", Namespace: configDefaultNamespace}, + } + _, err = hubClusterClient.ClusterV1beta1().Placements(configDefaultNamespace).Create( + context.Background(), placement, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + decision := &clusterv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-placement", + Namespace: configDefaultNamespace, + Labels: map[string]string{ + clusterv1beta1.PlacementLabel: "test-placement", + clusterv1beta1.DecisionGroupIndexLabel: "0", + }, + }, + } + decision, err = hubClusterClient.ClusterV1beta1().PlacementDecisions(configDefaultNamespace).Create( + context.Background(), decision, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + decision.Status.Decisions = []clusterv1beta1.ClusterDecision{ + {ClusterName: managedClusterName}, + } + _, err = hubClusterClient.ClusterV1beta1().PlacementDecisions(configDefaultNamespace).UpdateStatus( + context.Background(), decision, metav1.UpdateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // CMA status: InstallProgressions should have both configs in spec order (config-first → config-second). + assertClusterManagementAddOnInstallProgressionBeta(testAddOnConfigsImpl.name, addonapiv1beta1.InstallProgression{ + PlacementRef: addonapiv1beta1.PlacementRef{Name: "test-placement", Namespace: configDefaultNamespace}, + ConfigReferences: []addonapiv1beta1.InstallConfigReference{ + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-first", + }, + SpecHash: addOnTest1ConfigSpecHash, + }, + }, + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-second", + }, + SpecHash: addOnTest2ConfigSpecHash, + }, + }, + }, + }) + + // MCA status: ConfigReferences should reflect the same order (config-first → config-second). + assertManagedClusterAddOnConfigReferencesBeta(testAddOnConfigsImpl.name, managedClusterName, + addonapiv1beta1.ConfigReference{ + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + LastObservedGeneration: 1, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-first", + }, + SpecHash: addOnTest1ConfigSpecHash, + }, + }, + addonapiv1beta1.ConfigReference{ + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + LastObservedGeneration: 1, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-second", + }, + SpecHash: addOnTest2ConfigSpecHash, + }, + }, + ) + }) })