modify code

Signed-off-by: haoqing0110 <qhao@redhat.com>
open-cluster-management-io · Aug 9, 2024 · 1c0879c · 1c0879c
1 parent 4c7ce51
commit 1c0879c
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 34 deletions.
diff --git a/pkg/addon/controllers/addonconfiguration/addon_configuration_reconciler.go b/pkg/addon/controllers/addonconfiguration/addon_configuration_reconciler.go
@@ -47,7 +47,9 @@ func (d *managedClusterAddonConfigurationReconciler) mergeAddonConfig(
 	mcaCopy := mca.DeepCopy()
 
 	mergedConfigs := make(addonConfigMap)
-	// remove configs that are not desired
+	// First go through the configReferences listed in mca status,
+	// if the existing config (gvk + namespace + name) is also in the desiredConfigMap, append it to mergedConfigs,
+	// this will save the LastAppliedConfig and LastObservedGeneration from mca status.
 	for _, configRef := range mcaCopy.Status.ConfigReferences {
 		gr := configRef.ConfigGroupResource
 		if _, ok := mergedConfigs[gr]; !ok {
@@ -58,7 +60,10 @@ func (d *managedClusterAddonConfigurationReconciler) mergeAddonConfig(
 		}
 	}
 
-	// append or update configs
+	// Then go through the desiredConfigMap, for each configReference,
+	// if the desired config (gvk + namespace + name) is aleady in the mergedConfigs,
+	// update the ConfigReferent and DesiredConfig (including desired spechash) to mergedConfigs.
+	// else just append it to the mergedConfigs.
 	for gr, configReferences := range desiredConfigMap {
 		for _, configRef := range configReferences {
 			if _, ok := mergedConfigs[gr]; !ok {

diff --git a/pkg/addon/controllers/addonconfiguration/graph.go b/pkg/addon/controllers/addonconfiguration/graph.go
@@ -103,10 +103,16 @@ func (n *addonNode) setRolloutStatus() {
 }
 
 func (m addonConfigMap) copy() addonConfigMap {
-	output := make(addonConfigMap)
+	output := make(addonConfigMap, len(m))
 	for k, v := range m {
-		output[k] = make([]addonv1alpha1.ConfigReference, len(v))
-		copy(output[k], v)
+		// copy the key ConfigGroupResource
+		newk := k.DeepCopy()
+		// copy the slice of ConfigReference
+		newv := make([]addonv1alpha1.ConfigReference, len(v))
+		for i, cr := range v {
+			newv[i] = *cr.DeepCopy()
+		}
+		output[*newk] = newv
 	}
 	return output
 }
@@ -254,7 +260,7 @@ func (g *configurationGraph) addPlacementNode(
 	// overrides configuration by install strategy
 	if len(installConfigReference) > 0 {
 		node.desiredConfigs = node.desiredConfigs.copy()
-		overrideConfigMapByInstallConfigRef(installConfigReference, node.desiredConfigs)
+		overrideConfigMapByInstallConfigRef(node.desiredConfigs, installConfigReference)
 	}
 
 	// remove addon in defaults and other placements.
@@ -329,7 +335,7 @@ func (n *installStrategyNode) addNode(addon *addonv1alpha1.ManagedClusterAddOn)
 	if len(addon.Spec.Configs) > 0 {
 		n.children[addon.Namespace].desiredConfigs = n.children[addon.Namespace].desiredConfigs.copy()
 		// TODO we should also filter out the configs which are not supported configs.
-		overrideConfigMapByAddOnConfigs(addon.Spec.Configs, n.children[addon.Namespace].desiredConfigs)
+		overrideConfigMapByAddOnConfigs(n.children[addon.Namespace].desiredConfigs, addon.Spec.Configs)
 
 		// copy the spechash from mca status
 		for _, configRef := range addon.Status.ConfigReferences {
@@ -496,21 +502,26 @@ func rolloutStatusHasCluster(clusterRolloutStatus []clustersdkv1alpha1.ClusterRo
 	return false
 }
 
+// Override the desired addonConfigMap by a slice of InstallConfigReference (from cma status),
 func overrideConfigMapByInstallConfigRef(
-	installConfigRefs []addonv1alpha1.InstallConfigReference,
 	desiredConfigs addonConfigMap,
+	installConfigRefs []addonv1alpha1.InstallConfigReference,
 ) {
-	gvkOverwritten := make(map[addonv1alpha1.ConfigGroupResource]struct{})
+	gvkOverwritten := sets.New[addonv1alpha1.ConfigGroupResource]()
+	// Go through the cma installConfigReferences,
+	// for a group of configs with same gvk, cma installConfigReferences override the desiredConfigs.
 	for _, configRef := range installConfigRefs {
 		if configRef.DesiredConfig == nil {
 			continue
 		}
 		gr := configRef.ConfigGroupResource
 		referent := configRef.DesiredConfig.ConfigReferent
-		if _, exist := gvkOverwritten[gr]; !exist {
+		if !gvkOverwritten.Has(gr) {
 			desiredConfigs[gr] = []addonv1alpha1.ConfigReference{}
-			gvkOverwritten[gr] = struct{}{}
+			gvkOverwritten.Insert(gr)
 		}
+		// If a config not exist in the desiredConfigs, append it.
+		// This is to avoid adding duplicate configs (name + namespace).
 		if _, exist := desiredConfigs.containsConfig(gr, referent); !exist {
 			desiredConfigs[gr] = append(desiredConfigs[gr], addonv1alpha1.ConfigReference{
 				ConfigGroupResource: gr,
@@ -521,18 +532,23 @@ func overrideConfigMapByInstallConfigRef(
 	}
 }
 
+// Override the desired addonConfigMap by a slice of InstallConfigReference (from mca spec),
 func overrideConfigMapByAddOnConfigs(
-	addOnConfigs []addonv1alpha1.AddOnConfig,
 	desiredConfigs addonConfigMap,
+	addOnConfigs []addonv1alpha1.AddOnConfig,
 ) {
-	gvkOverwritten := make(map[addonv1alpha1.ConfigGroupResource]struct{})
+	gvkOverwritten := sets.New[addonv1alpha1.ConfigGroupResource]()
+	// Go through the mca addOnConfigs,
+	// for a group of configs with same gvk, mca addOnConfigs override the desiredConfigs.
 	for _, config := range addOnConfigs {
 		gr := config.ConfigGroupResource
 		referent := config.ConfigReferent
-		if _, exist := gvkOverwritten[gr]; !exist {
+		if !gvkOverwritten.Has(gr) {
 			desiredConfigs[gr] = []addonv1alpha1.ConfigReference{}
-			gvkOverwritten[gr] = struct{}{}
+			gvkOverwritten.Insert(gr)
 		}
+		// If a config not exist in the desiredConfigs, append it.
+		// This is to avoid adding duplicate configs (name + namespace).
 		if _, exist := desiredConfigs.containsConfig(gr, referent); !exist {
 			desiredConfigs[gr] = append(desiredConfigs[gr], addonv1alpha1.ConfigReference{
 				ConfigGroupResource: gr,

diff --git a/pkg/addon/controllers/cmainstallprogression/controller.go b/pkg/addon/controllers/cmainstallprogression/controller.go
@@ -7,6 +7,7 @@ import (
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 
 	addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
@@ -132,15 +133,15 @@ func setInstallProgression(supportedConfigs []addonv1alpha1.ConfigMeta, placemen
 		}
 
 		// set config references as default configuration
-		installConfigReferencesMap := map[addonv1alpha1.ConfigGroupResource][]addonv1alpha1.ConfigReferent{}
+		installConfigReferencesMap := map[addonv1alpha1.ConfigGroupResource]sets.Set[addonv1alpha1.ConfigReferent]{}
 		for _, config := range supportedConfigs {
 			if config.DefaultConfig != nil {
-				installConfigReferencesMap[config.ConfigGroupResource] = []addonv1alpha1.ConfigReferent{*config.DefaultConfig}
+				installConfigReferencesMap[config.ConfigGroupResource] = sets.New[addonv1alpha1.ConfigReferent](*config.DefaultConfig)
 			}
 		}
 
 		// override the default configuration for each placement
-		overrideConfigMapByAddOnConfigs(placementStrategy.Configs, installConfigReferencesMap)
+		overrideConfigMapByAddOnConfigs(installConfigReferencesMap, placementStrategy.Configs)
 
 		// sort gvk
 		gvks := []addonv1alpha1.ConfigGroupResource{}
@@ -158,7 +159,7 @@ func setInstallProgression(supportedConfigs []addonv1alpha1.ConfigMeta, placemen
 		installConfigReferences := []addonv1alpha1.InstallConfigReference{}
 		for _, gvk := range gvks {
 			if configRefs, ok := installConfigReferencesMap[gvk]; ok {
-				for _, configRef := range configRefs {
+				for configRef := range configRefs {
 					installConfigReferences = append(installConfigReferences,
 						addonv1alpha1.InstallConfigReference{
 							ConfigGroupResource: gvk,
@@ -218,27 +219,24 @@ func mergeInstallProgression(newobj, oldobj *addonv1alpha1.InstallProgression) {
 	newobj.Conditions = oldobj.Conditions
 }
 
+// Override the desired configs by a slice of AddOnConfig (from cma install strategy),
 func overrideConfigMapByAddOnConfigs(
+	desiredConfigs map[addonv1alpha1.ConfigGroupResource]sets.Set[addonv1alpha1.ConfigReferent],
 	addOnConfigs []addonv1alpha1.AddOnConfig,
-	desiredConfigs map[addonv1alpha1.ConfigGroupResource][]addonv1alpha1.ConfigReferent,
 ) {
-	gvkOverwritten := make(map[addonv1alpha1.ConfigGroupResource]struct{})
+	gvkOverwritten := sets.New[addonv1alpha1.ConfigGroupResource]()
+	// 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 _, exists := gvkOverwritten[gr]; !exists {
-			desiredConfigs[gr] = []addonv1alpha1.ConfigReferent{}
-			gvkOverwritten[gr] = struct{}{}
+		if !gvkOverwritten.Has(gr) {
+			desiredConfigs[gr] = sets.New[addonv1alpha1.ConfigReferent]()
+			gvkOverwritten.Insert(gr)
 		}
-		// check and avoid adding duplicate configs
-		var exist bool
-		for _, configReferent := range desiredConfigs[gr] {
-			if configReferent == config.ConfigReferent {
-				exist = true
-				break
-			}
-		}
-		if !exist {
-			desiredConfigs[gr] = append(desiredConfigs[gr], config.ConfigReferent)
+		// If a config 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)
 		}
 	}
 }