From da81dd0db9ad5f74c48166503edd56f461f36ee7 Mon Sep 17 00:00:00 2001 From: Zhiwei Yin Date: Fri, 27 Mar 2026 16:37:24 +0800 Subject: [PATCH] Fix cma Progressing status addon counting error (#1454) Signed-off-by: Zhiwei Yin --- .../cma_progressing_reconciler_test.go | 613 +++++++++++++++++- .../controllers/addonconfiguration/graph.go | 55 +- .../addonconfiguration/graph_test.go | 378 +++++++++++ 3 files changed, 1029 insertions(+), 17 deletions(-) diff --git a/pkg/addon/controllers/addonconfiguration/cma_progressing_reconciler_test.go b/pkg/addon/controllers/addonconfiguration/cma_progressing_reconciler_test.go index 84c955d3b..0944decc4 100644 --- a/pkg/addon/controllers/addonconfiguration/cma_progressing_reconciler_test.go +++ b/pkg/addon/controllers/addonconfiguration/cma_progressing_reconciler_test.go @@ -586,14 +586,617 @@ func TestMgmtAddonProgressingReconcile(t *testing.T) { if len(cma.Status.DefaultConfigReferences) != 0 { t.Errorf("DefaultConfigReferences object is not correct: %v", cma.Status.DefaultConfigReferences) } - if cma.Status.InstallProgressions[0].ConfigReferences[0].LastKnownGoodConfig != nil { - t.Errorf("InstallProgressions LastKnownGoodConfig is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences[0]) + if cma.Status.InstallProgressions[0].ConfigReferences[0].LastKnownGoodConfig == nil { + t.Errorf("InstallProgressions LastKnownGoodConfig should be set: %v", cma.Status.InstallProgressions[0].ConfigReferences[0]) } + if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonCompleted { + t.Errorf("InstallProgressions condition is not correct: %v", cma.Status.InstallProgressions[0].Conditions) + } + if cma.Status.InstallProgressions[0].Conditions[0].Message != "1/1 completed with no errors, 0 failed 0 timeout." { + t.Errorf("InstallProgressions condition is not correct: %v", cma.Status.InstallProgressions[0].Conditions) + } + }, + }, + { + name: "mca override cma configs with 2 clusters, one with mca configs succeeded", + managedClusteraddon: []runtime.Object{ + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster1") + addon.Spec.Configs = []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + }, + } + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hashmca", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hashmca", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonCompleted, + }, + } + return addon + }(), + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster2") + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonCompleted, + }, + } + return addon + }(), + }, + clusterManagementAddon: []runtime.Object{addontesting.NewClusterManagementAddon("test", "", ""). + WithPlacementStrategy(addonv1alpha1.PlacementStrategy{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + RolloutStrategy: clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All}, + }).WithInstallProgression(addonv1alpha1.InstallProgression{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + ConfigReferences: []addonv1alpha1.InstallConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash", + }, + }, + }, + }).Build()}, + placements: []runtime.Object{ + &clusterv1beta1.Placement{ObjectMeta: metav1.ObjectMeta{Name: "placement1", Namespace: "test"}}, + }, + placementDecisions: []runtime.Object{ + &clusterv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement1", + Namespace: "test", + Labels: map[string]string{ + clusterv1beta1.PlacementLabel: "placement1", + clusterv1beta1.DecisionGroupIndexLabel: "0", + }, + }, + Status: clusterv1beta1.PlacementDecisionStatus{ + Decisions: []clusterv1beta1.ClusterDecision{{ClusterName: "cluster1"}, {ClusterName: "cluster2"}}, + }, + }, + }, + validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { + addontesting.AssertActions(t, actions, "patch") + actual := actions[0].(clienttesting.PatchActionImpl).Patch + cma := &addonv1alpha1.ClusterManagementAddOn{} + err := json.Unmarshal(actual, cma) + if err != nil { + t.Fatal(err) + } + + if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonCompleted { + t.Errorf("InstallProgressions condition reason is not correct, expected Completed, got: %v", + cma.Status.InstallProgressions[0].Conditions[0].Reason) + } + if cma.Status.InstallProgressions[0].Conditions[0].Message != "2/2 completed with no errors, 0 failed 0 timeout." { + t.Errorf("InstallProgressions condition message is not correct: %v", + cma.Status.InstallProgressions[0].Conditions[0].Message) + } + }, + }, + { + name: "mca override cma configs with mca failed", + managedClusteraddon: []runtime.Object{ + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster1") + addon.Spec.Configs = []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + }, + } + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hashmca", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hasholdmca", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonFailed, + }, + } + return addon + }(), + }, + clusterManagementAddon: []runtime.Object{addontesting.NewClusterManagementAddon("test", "", ""). + WithPlacementStrategy(addonv1alpha1.PlacementStrategy{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + RolloutStrategy: clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All}, + }).WithInstallProgression(addonv1alpha1.InstallProgression{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + ConfigReferences: []addonv1alpha1.InstallConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash", + }, + }, + }, + }).Build()}, + placements: []runtime.Object{ + &clusterv1beta1.Placement{ObjectMeta: metav1.ObjectMeta{Name: "placement1", Namespace: "test"}}, + }, + placementDecisions: []runtime.Object{ + &clusterv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement1", + Namespace: "test", + Labels: map[string]string{ + clusterv1beta1.PlacementLabel: "placement1", + clusterv1beta1.DecisionGroupIndexLabel: "0", + }, + }, + Status: clusterv1beta1.PlacementDecisionStatus{ + Decisions: []clusterv1beta1.ClusterDecision{{ClusterName: "cluster1"}}, + }, + }, + }, + validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { + addontesting.AssertActions(t, actions, "patch") + actual := actions[0].(clienttesting.PatchActionImpl).Patch + cma := &addonv1alpha1.ClusterManagementAddOn{} + err := json.Unmarshal(actual, cma) + if err != nil { + t.Fatal(err) + } + if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonProgressing { - t.Errorf("InstallProgressions condition is not correct: %v", cma.Status.InstallProgressions[0].Conditions) + t.Errorf("InstallProgressions condition reason is not correct, expected Progressing, got: %v", + cma.Status.InstallProgressions[0].Conditions[0].Reason) } - if cma.Status.InstallProgressions[0].Conditions[0].Message != "0/1 progressing..., 0 failed 0 timeout." { - t.Errorf("InstallProgressions condition is not correct: %v", cma.Status.InstallProgressions[0].Conditions) + if cma.Status.InstallProgressions[0].Conditions[0].Message != "1/1 progressing..., 0 failed 0 timeout." { + t.Errorf("InstallProgressions condition message is not correct: %v", + cma.Status.InstallProgressions[0].Conditions[0].Message) + } + }, + }, + { + name: "mca overrides 1 of 2 cma configs, inherited config matches, both addons succeeded", + managedClusteraddon: []runtime.Object{ + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster1") + addon.Spec.Configs = []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + }, + } + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hashmca", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hashmca", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonCompleted, + }, + } + return addon + }(), + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster2") + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonCompleted, + }, + } + return addon + }(), + }, + clusterManagementAddon: []runtime.Object{addontesting.NewClusterManagementAddon("test", "", ""). + WithPlacementStrategy(addonv1alpha1.PlacementStrategy{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + RolloutStrategy: clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All}, + }).WithInstallProgression(addonv1alpha1.InstallProgression{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + ConfigReferences: []addonv1alpha1.InstallConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + }, + }, + }).Build()}, + placements: []runtime.Object{ + &clusterv1beta1.Placement{ObjectMeta: metav1.ObjectMeta{Name: "placement1", Namespace: "test"}}, + }, + placementDecisions: []runtime.Object{ + &clusterv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement1", + Namespace: "test", + Labels: map[string]string{ + clusterv1beta1.PlacementLabel: "placement1", + clusterv1beta1.DecisionGroupIndexLabel: "0", + }, + }, + Status: clusterv1beta1.PlacementDecisionStatus{ + Decisions: []clusterv1beta1.ClusterDecision{{ClusterName: "cluster1"}, {ClusterName: "cluster2"}}, + }, + }, + }, + validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { + addontesting.AssertActions(t, actions, "patch") + actual := actions[0].(clienttesting.PatchActionImpl).Patch + cma := &addonv1alpha1.ClusterManagementAddOn{} + err := json.Unmarshal(actual, cma) + if err != nil { + t.Fatal(err) + } + + if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonCompleted { + t.Errorf("InstallProgressions condition reason is not correct, expected Completed, got: %v", + cma.Status.InstallProgressions[0].Conditions[0].Reason) + } + if cma.Status.InstallProgressions[0].Conditions[0].Message != "2/2 completed with no errors, 0 failed 0 timeout." { + t.Errorf("InstallProgressions condition message is not correct: %v", + cma.Status.InstallProgressions[0].Conditions[0].Message) + } + }, + }, + { + name: "mca overrides 1 of 2 cma configs, inherited config not match, addon progressing", + managedClusteraddon: []runtime.Object{ + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster1") + addon.Spec.Configs = []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + }, + } + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hashmca", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "testmca"}, + SpecHash: "hashmca", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar-old"}, + SpecHash: "hashbar-old", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar-old"}, + SpecHash: "hashbar-old", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonCompleted, + }, + } + return addon + }(), + }, + clusterManagementAddon: []runtime.Object{addontesting.NewClusterManagementAddon("test", "", ""). + WithPlacementStrategy(addonv1alpha1.PlacementStrategy{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + RolloutStrategy: clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All}, + }).WithInstallProgression(addonv1alpha1.InstallProgression{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + ConfigReferences: []addonv1alpha1.InstallConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar-new"}, + SpecHash: "hashbar-new", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar-old"}, + SpecHash: "hashbar-old", + }, + }, + }, + }).Build()}, + placements: []runtime.Object{ + &clusterv1beta1.Placement{ObjectMeta: metav1.ObjectMeta{Name: "placement1", Namespace: "test"}}, + }, + placementDecisions: []runtime.Object{ + &clusterv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement1", + Namespace: "test", + Labels: map[string]string{ + clusterv1beta1.PlacementLabel: "placement1", + clusterv1beta1.DecisionGroupIndexLabel: "0", + }, + }, + Status: clusterv1beta1.PlacementDecisionStatus{ + Decisions: []clusterv1beta1.ClusterDecision{{ClusterName: "cluster1"}}, + }, + }, + }, + validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { + addontesting.AssertActions(t, actions, "patch") + actual := actions[0].(clienttesting.PatchActionImpl).Patch + cma := &addonv1alpha1.ClusterManagementAddOn{} + err := json.Unmarshal(actual, cma) + if err != nil { + t.Fatal(err) + } + + // addon overrides Foo but inherited Bar doesn't match (bar-old vs bar-new), + // so addon is not counted as succeeded, it needs to be rolled out. + if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonProgressing { + t.Errorf("InstallProgressions condition reason is not correct, expected Progressing, got: %v", + cma.Status.InstallProgressions[0].Conditions[0].Reason) + } + if cma.Status.InstallProgressions[0].Conditions[0].Message != "1/1 progressing..., 0 failed 0 timeout." { + t.Errorf("InstallProgressions condition message is not correct: %v", + cma.Status.InstallProgressions[0].Conditions[0].Message) + } + }, + }, + { + name: "2 cma configs, no override by addon, both addons succeeded", + managedClusteraddon: []runtime.Object{ + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster1") + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonCompleted, + }, + } + return addon + }(), + func() *addonv1alpha1.ManagedClusterAddOn { + addon := addontesting.NewAddon("test", "cluster2") + addon.Status.ConfigReferences = []addonv1alpha1.ConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + }, + } + addon.Status.Conditions = []metav1.Condition{ + { + Type: addonv1alpha1.ManagedClusterAddOnConditionProgressing, + Reason: addonv1alpha1.ProgressingReasonCompleted, + }, + } + return addon + }(), + }, + clusterManagementAddon: []runtime.Object{addontesting.NewClusterManagementAddon("test", "", ""). + WithPlacementStrategy(addonv1alpha1.PlacementStrategy{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + RolloutStrategy: clusterv1alpha1.RolloutStrategy{Type: clusterv1alpha1.All}, + }).WithInstallProgression(addonv1alpha1.InstallProgression{ + PlacementRef: addonv1alpha1.PlacementRef{Name: "placement1", Namespace: "test"}, + ConfigReferences: []addonv1alpha1.InstallConfigReference{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Foo"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "test1"}, + SpecHash: "hash1", + }, + }, + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "core", Resource: "Bar"}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + LastAppliedConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "bar1"}, + SpecHash: "hashbar1", + }, + }, + }, + }).Build()}, + placements: []runtime.Object{ + &clusterv1beta1.Placement{ObjectMeta: metav1.ObjectMeta{Name: "placement1", Namespace: "test"}}, + }, + placementDecisions: []runtime.Object{ + &clusterv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement1", + Namespace: "test", + Labels: map[string]string{ + clusterv1beta1.PlacementLabel: "placement1", + clusterv1beta1.DecisionGroupIndexLabel: "0", + }, + }, + Status: clusterv1beta1.PlacementDecisionStatus{ + Decisions: []clusterv1beta1.ClusterDecision{{ClusterName: "cluster1"}, {ClusterName: "cluster2"}}, + }, + }, + }, + validateAddonActions: func(t *testing.T, actions []clienttesting.Action) { + addontesting.AssertActions(t, actions, "patch") + actual := actions[0].(clienttesting.PatchActionImpl).Patch + cma := &addonv1alpha1.ClusterManagementAddOn{} + err := json.Unmarshal(actual, cma) + if err != nil { + t.Fatal(err) + } + + if cma.Status.InstallProgressions[0].Conditions[0].Reason != addonv1alpha1.ProgressingReasonCompleted { + t.Errorf("InstallProgressions condition reason is not correct, expected Completed, got: %v", + cma.Status.InstallProgressions[0].Conditions[0].Reason) + } + if cma.Status.InstallProgressions[0].Conditions[0].Message != "2/2 completed with no errors, 0 failed 0 timeout." { + t.Errorf("InstallProgressions condition message is not correct: %v", + cma.Status.InstallProgressions[0].Conditions[0].Message) } }, }, diff --git a/pkg/addon/controllers/addonconfiguration/graph.go b/pkg/addon/controllers/addonconfiguration/graph.go index c11dd4b54..55f538795 100644 --- a/pkg/addon/controllers/addonconfiguration/graph.go +++ b/pkg/addon/controllers/addonconfiguration/graph.go @@ -493,9 +493,15 @@ func (n *installStrategyNode) getAddonsSucceeded() []*addonNode { func (n *installStrategyNode) countAddonUpgradeSucceed() int { count := 0 for _, addon := range n.children { - if desiredConfigsEqual(addon.desiredConfigs, n.desiredConfigs) && - addon.status.Status == clustersdkv1alpha1.Succeeded && - !rolloutStatusHasCluster(n.rolloutResult.ClustersToRollout, addon.mca.Namespace) { + if addon.status.Status != clustersdkv1alpha1.Succeeded { + continue + } + if rolloutStatusHasCluster(n.rolloutResult.ClustersToRollout, addon.mca.Namespace) { + continue + } + // Check if the addon's inherited desired configs (not overridden by addon.spec.configs) + // match the install strategy's desired configs. + if desiredConfigsEqual(addon.desiredConfigs, n.desiredConfigs, addon.mca.Spec.Configs) { count += 1 } } @@ -506,9 +512,15 @@ func (n *installStrategyNode) countAddonUpgradeSucceed() int { func (n *installStrategyNode) countAddonUpgradeFailed() int { count := 0 for _, addon := range n.children { - if desiredConfigsEqual(addon.desiredConfigs, n.desiredConfigs) && - addon.status.Status == clustersdkv1alpha1.Failed && - !rolloutStatusHasCluster(n.rolloutResult.ClustersToRollout, addon.mca.Namespace) { + if addon.status.Status != clustersdkv1alpha1.Failed { + continue + } + if rolloutStatusHasCluster(n.rolloutResult.ClustersToRollout, addon.mca.Namespace) { + continue + } + // Check if the addon's inherited desired configs (not overridden by addon.spec.configs) + // match the install strategy's desired configs. + if desiredConfigsEqual(addon.desiredConfigs, n.desiredConfigs, addon.mca.Spec.Configs) { count += 1 } } @@ -540,17 +552,36 @@ func getClusterRolloutStatus(clusterName string, addonNode *addonNode) (clusters return *addonNode.status, nil } -func desiredConfigsEqual(a, b addonConfigMap) bool { - if len(a) != len(b) { +// desiredConfigsEqual checks if two addonConfigMaps are equal, skipping GVKs +// that are overridden by overrideConfigs. +func desiredConfigsEqual(addonDesired, strategyDesired addonConfigMap, overrideConfigs []addonv1alpha1.AddOnConfig) bool { + overrideGRs := addonConfigMap{} + for _, config := range overrideConfigs { + overrideGRs[config.ConfigGroupResource] = append(overrideGRs[config.ConfigGroupResource], + addonv1alpha1.ConfigReference{ConfigGroupResource: config.ConfigGroupResource}) + } + + overrideDesired := strategyDesired.copy() + for overrideGR, overrideConfigReference := range overrideGRs { + overrideDesired[overrideGR] = overrideConfigReference + } + + if len(addonDesired) != len(overrideDesired) { return false } - for configgrA := range a { - if len(a[configgrA]) != len(b[configgrA]) { + for gr, overrideConfigs := range overrideDesired { + addonConfigs, ok := addonDesired[gr] + if !ok || len(overrideConfigs) != len(addonConfigs) { return false } - for i := range a[configgrA] { - if a[configgrA][i] != b[configgrA][i] { + + if _, ok := overrideGRs[gr]; ok { + continue + } + + for i := range overrideConfigs { + if !equality.Semantic.DeepEqual(overrideConfigs[i], addonConfigs[i]) { return false } } diff --git a/pkg/addon/controllers/addonconfiguration/graph_test.go b/pkg/addon/controllers/addonconfiguration/graph_test.go index 2b2175e7a..e7135967c 100644 --- a/pkg/addon/controllers/addonconfiguration/graph_test.go +++ b/pkg/addon/controllers/addonconfiguration/graph_test.go @@ -869,3 +869,381 @@ func newDefaultConfigReference(group, resource, name, hash string) addonv1alpha1 }, } } + +func newConfigReference(group, resource, name, hash string) addonv1alpha1.ConfigReference { + return addonv1alpha1.ConfigReference{ + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: group, Resource: resource}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: name}, + DesiredConfig: &addonv1alpha1.ConfigSpecHash{ + ConfigReferent: addonv1alpha1.ConfigReferent{Name: name}, + SpecHash: hash, + }, + } +} + +func TestDesiredConfigsEqual(t *testing.T) { + cases := []struct { + name string + addonDesired addonConfigMap + strategyDesired addonConfigMap + addonConfigs []addonv1alpha1.AddOnConfig + expected bool + }{ + { + name: "all strategy configs overridden by addon configs", + strategyDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "strategy-cfg", "hash1"), + }, + }, + addonConfigs: []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "addon", Resource: "AddonDeploymentConfig"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg"}, + }, + }, + addonDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg", "hash-addon"), + }, + }, + expected: true, + }, + { + name: "no addon configs, addonDesired matches strategy", + strategyDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg1", "hash1"), + }, + }, + addonConfigs: []addonv1alpha1.AddOnConfig{}, + addonDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg1", "hash1"), + }, + }, + expected: true, + }, + { + name: "addon overrides one GVK, inherited GVK matches", + strategyDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "strategy-cfg", "hash1"), + }, + {Group: "addon", Resource: "AddonTemplate"}: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + addonConfigs: []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "addon", Resource: "AddonDeploymentConfig"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg"}, + }, + }, + addonDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg", "hash-addon"), + }, + {Group: "addon", Resource: "AddonTemplate"}: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + expected: true, + }, + { + name: "addon overrides one GVK, inherited GVK does not match", + strategyDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "strategy-cfg", "hash1"), + }, + {Group: "addon", Resource: "AddonTemplate"}: { + newConfigReference("addon", "AddonTemplate", "tpl-new", "hash-tpl-new"), + }, + }, + addonConfigs: []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "addon", Resource: "AddonDeploymentConfig"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg"}, + }, + }, + addonDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg", "hash-addon"), + }, + {Group: "addon", Resource: "AddonTemplate"}: { + newConfigReference("addon", "AddonTemplate", "tpl-old", "hash-tpl-old"), + }, + }, + expected: false, + }, + { + name: "no addon configs, addonDesired does not match strategy", + strategyDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg-new", "hash-new"), + }, + }, + addonConfigs: []addonv1alpha1.AddOnConfig{}, + addonDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg-old", "hash-old"), + }, + }, + expected: false, + }, + { + name: "inherited GVK missing from addonDesired", + strategyDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "strategy-cfg", "hash1"), + }, + {Group: "addon", Resource: "AddonTemplate"}: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + addonConfigs: []addonv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonv1alpha1.ConfigGroupResource{Group: "addon", Resource: "AddonDeploymentConfig"}, + ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg"}, + }, + }, + addonDesired: addonConfigMap{ + {Group: "addon", Resource: "AddonDeploymentConfig"}: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg", "hash-addon"), + }, + }, + expected: false, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + actual := desiredConfigsEqual(c.addonDesired, c.strategyDesired, c.addonConfigs) + if actual != c.expected { + t.Errorf("expected %v, got %v", c.expected, actual) + } + }) + } +} + +func TestCountAddonUpgradeSucceedAndFailed(t *testing.T) { + deploymentConfigGR := addonv1alpha1.ConfigGroupResource{Group: "addon", Resource: "AddonDeploymentConfig"} + templateGR := addonv1alpha1.ConfigGroupResource{Group: "addon", Resource: "AddonTemplate"} + + cases := []struct { + name string + node *installStrategyNode + expectedSucceed int + expectedFailed int + }{ + { + name: "1 config overridden by addon, succeed addon counted", + node: &installStrategyNode{ + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "strategy-cfg", "hash1"), + }, + }, + children: map[string]*addonNode{ + "cluster1": { + mca: newManagedClusterAddon("test", "cluster1", + []addonv1alpha1.AddOnConfig{ + {ConfigGroupResource: deploymentConfigGR, ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg"}}, + }, + nil, nil, + ), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg", "hash-addon"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster1", + Status: clustersdkv1alpha1.Succeeded, + }, + }, + "cluster2": { + mca: newManagedClusterAddon("test", "cluster2", + []addonv1alpha1.AddOnConfig{ + {ConfigGroupResource: deploymentConfigGR, ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg2"}}, + }, + nil, nil, + ), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg2", "hash-addon2"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster2", + Status: clustersdkv1alpha1.Failed, + }, + }, + }, + rolloutResult: clustersdkv1alpha1.RolloutResult{}, + }, + expectedSucceed: 1, + expectedFailed: 1, + }, + { + name: "2 configs, one overridden by addon, inherited matches", + node: &installStrategyNode{ + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "strategy-cfg", "hash1"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + children: map[string]*addonNode{ + // cluster1: overrides AddonDeploymentConfig, inherits AddonTemplate (matches) -> succeed counted + "cluster1": { + mca: newManagedClusterAddon("test", "cluster1", + []addonv1alpha1.AddOnConfig{ + {ConfigGroupResource: deploymentConfigGR, ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg"}}, + }, + nil, nil, + ), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg", "hash-addon"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster1", + Status: clustersdkv1alpha1.Succeeded, + }, + }, + // cluster2: overrides AddonDeploymentConfig, inherited AddonTemplate mismatches -> failed NOT counted + "cluster2": { + mca: newManagedClusterAddon("test", "cluster2", + []addonv1alpha1.AddOnConfig{ + {ConfigGroupResource: deploymentConfigGR, ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg2"}}, + }, + nil, nil, + ), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg2", "hash-addon2"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl-old", "hash-tpl-old"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster2", + Status: clustersdkv1alpha1.Failed, + }, + }, + // cluster3: overrides AddonDeploymentConfig, inherits AddonTemplate (matches) -> failed counted + "cluster3": { + mca: newManagedClusterAddon("test", "cluster3", + []addonv1alpha1.AddOnConfig{ + {ConfigGroupResource: deploymentConfigGR, ConfigReferent: addonv1alpha1.ConfigReferent{Name: "addon-cfg3"}}, + }, + nil, nil, + ), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "addon-cfg3", "hash-addon3"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster3", + Status: clustersdkv1alpha1.Failed, + }, + }, + }, + rolloutResult: clustersdkv1alpha1.RolloutResult{}, + }, + expectedSucceed: 1, + expectedFailed: 1, + }, + { + name: "2 configs, no override by addon", + node: &installStrategyNode{ + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg1", "hash1"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + children: map[string]*addonNode{ + // cluster1: no override, desiredConfigs matches strategy -> succeed counted + "cluster1": { + mca: newManagedClusterAddon("test", "cluster1", nil, nil, nil), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg1", "hash1"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster1", + Status: clustersdkv1alpha1.Succeeded, + }, + }, + // cluster2: no override, desiredConfigs mismatches strategy -> failed NOT counted + "cluster2": { + mca: newManagedClusterAddon("test", "cluster2", nil, nil, nil), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg-old", "hash-old"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster2", + Status: clustersdkv1alpha1.Failed, + }, + }, + // cluster3: no override, desiredConfigs matches strategy -> failed counted + "cluster3": { + mca: newManagedClusterAddon("test", "cluster3", nil, nil, nil), + desiredConfigs: addonConfigMap{ + deploymentConfigGR: { + newConfigReference("addon", "AddonDeploymentConfig", "cfg1", "hash1"), + }, + templateGR: { + newConfigReference("addon", "AddonTemplate", "tpl1", "hash-tpl"), + }, + }, + status: &clustersdkv1alpha1.ClusterRolloutStatus{ + ClusterName: "cluster3", + Status: clustersdkv1alpha1.Failed, + }, + }, + }, + rolloutResult: clustersdkv1alpha1.RolloutResult{}, + }, + expectedSucceed: 1, + expectedFailed: 1, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + actualSucceed := c.node.countAddonUpgradeSucceed() + if actualSucceed != c.expectedSucceed { + t.Errorf("countAddonUpgradeSucceed: expected %d, got %d", c.expectedSucceed, actualSucceed) + } + actualFailed := c.node.countAddonUpgradeFailed() + if actualFailed != c.expectedFailed { + t.Errorf("countAddonUpgradeFailed: expected %d, got %d", c.expectedFailed, actualFailed) + } + }) + } +}