github/open-cluster-management
1
0
Fork 0
mirror of https://github.com/open-cluster-management-io/ocm.git synced 2026-08-23 22:26:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 fix: preserve config reference ordering in cmainstallprogression controller (#1610)

Browse Source 
* 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 <tflower@redhat.com>

* 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 <tflower@redhat.com>

---------

Signed-off-by: Tesshu Flower <tflower@redhat.com>
This commit is contained in:
Tesshu Flower
2026-07-03 01:04:13 +00:00
committed by GitHub
parent 929c2975fa
commit 8a96e8e0aa
3 changed files with 210 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/addon/controllers/cmainstallprogression/controller.go
+27 -16
View File
@@ -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
}
pkg/addon/controllers/cmainstallprogression/controller_test.go
+18 -2
View File
@@ -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)
test/integration/addon/addon_configs_test.go
+165
View File
@@ -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,
},
},
)
})
})