Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add annotation if addon uses InstallStrategy #236

Merged
merged 2 commits into from
Feb 2, 2024
Merged

add annotation if addon uses InstallStrategy #236

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7736396
add annotation if addon uses InstallStrategy
haoqing0110 Jan 31, 2024
74b6399
add cma controller
haoqing0110 Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/addonfactory/addonfactory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func (f *AgentAddonFactory) WithGetValuesFuncs(getValuesFuncs ...GetValuesFunc)
// WithInstallStrategy defines the installation strategy of the manifests prescribed by Manifests(..).
// Deprecated: add annotation "addon.open-cluster-management.io/lifecycle: addon-manager" to ClusterManagementAddon
// and define install strategy in ClusterManagementAddon spec.installStrategy instead.
// The migration plan refer to https://github.com/open-cluster-management-io/ocm/issues/355.
func (f *AgentAddonFactory) WithInstallStrategy(strategy *agent.InstallStrategy) *AgentAddonFactory {
if strategy.InstallNamespace == "" {
strategy.InstallNamespace = AddonDefaultInstallNamespace
Expand Down
47 changes: 39 additions & 8 deletions pkg/addonmanager/controllers/addoninstall/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,40 @@ import (
addonlisterv1alpha1 "open-cluster-management.io/api/client/addon/listers/addon/v1alpha1"
clusterinformers "open-cluster-management.io/api/client/cluster/informers/externalversions/cluster/v1"
clusterlister "open-cluster-management.io/api/client/cluster/listers/cluster/v1"
"open-cluster-management.io/sdk-go/pkg/patcher"

"open-cluster-management.io/addon-framework/pkg/agent"
"open-cluster-management.io/addon-framework/pkg/basecontroller/factory"
)

// managedClusterController reconciles instances of ManagedCluster on the hub.
type addonInstallController struct {
addonClient addonv1alpha1client.Interface
managedClusterLister clusterlister.ManagedClusterLister
managedClusterAddonLister addonlisterv1alpha1.ManagedClusterAddOnLister
agentAddons map[string]agent.AgentAddon
addonClient addonv1alpha1client.Interface
managedClusterLister clusterlister.ManagedClusterLister
managedClusterAddonLister addonlisterv1alpha1.ManagedClusterAddOnLister
clusterManagementAddonLister addonlisterv1alpha1.ClusterManagementAddOnLister
agentAddons map[string]agent.AgentAddon
addonPatcher patcher.Patcher[*addonapiv1alpha1.ClusterManagementAddOn,
addonapiv1alpha1.ClusterManagementAddOnSpec,
addonapiv1alpha1.ClusterManagementAddOnStatus]
}

func NewAddonInstallController(
addonClient addonv1alpha1client.Interface,
clusterInformers clusterinformers.ManagedClusterInformer,
addonInformers addoninformerv1alpha1.ManagedClusterAddOnInformer,
clusterManagementAddonInformers addoninformerv1alpha1.ClusterManagementAddOnInformer,
agentAddons map[string]agent.AgentAddon,
) factory.Controller {
c := &addonInstallController{
addonClient: addonClient,
managedClusterLister: clusterInformers.Lister(),
managedClusterAddonLister: addonInformers.Lister(),
agentAddons: agentAddons,
addonClient: addonClient,
managedClusterLister: clusterInformers.Lister(),
managedClusterAddonLister: addonInformers.Lister(),
clusterManagementAddonLister: clusterManagementAddonInformers.Lister(),
agentAddons: agentAddons,
addonPatcher: patcher.NewPatcher[*addonapiv1alpha1.ClusterManagementAddOn,
addonapiv1alpha1.ClusterManagementAddOnSpec,
addonapiv1alpha1.ClusterManagementAddOnStatus](addonClient.AddonV1alpha1().ClusterManagementAddOns()),
}

return factory.New().WithFilteredEventsInformersQueueKeysFunc(
Expand Down Expand Up @@ -98,6 +108,27 @@ func (c *addonInstallController) sync(ctx context.Context, syncCtx factory.SyncC
continue
}

cma, err := c.clusterManagementAddonLister.Get(addonName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be called per mca/cluster event. It might bring a lot of conflict. How about we put this in a controller that reconcile clustermanagementaddon only?

Copy link
Member Author

@haoqing0110 haoqing0110 Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be a better choice. The current code will have conflicts when the first time adding the annotation, then won't trigger patch annotation action.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have another existing controller that is reconciling the clustermanageraddon

if err != nil {
if !errors.IsNotFound(err) {
return err
}
} else {
// If the addon defines install strategy via WithInstallStrategy(), force add annotation "addon.open-cluster-management.io/lifecycle: self" to cma.
// The annotation with value "self" will be removed when remove WithInstallStrategy() in addon-framework.
// The migration plan refer to https://github.com/open-cluster-management-io/ocm/issues/355.
cmaCopy := cma.DeepCopy()
if cmaCopy.Annotations == nil {
cmaCopy.Annotations = map[string]string{}
}
cmaCopy.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey] = addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue

_, err = c.addonPatcher.PatchLabelAnnotations(ctx, cmaCopy, cmaCopy.ObjectMeta, cma.ObjectMeta)
if err != nil {
return err
}
}

managedClusterFilter := addon.GetAgentAddonOptions().InstallStrategy.GetManagedClusterFilter()
if managedClusterFilter == nil {
continue
Expand Down
106 changes: 101 additions & 5 deletions pkg/addonmanager/controllers/addoninstall/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package addoninstall

import (
"context"
"encoding/json"
"testing"
"time"

Expand All @@ -16,6 +17,7 @@ import (
fakecluster "open-cluster-management.io/api/client/cluster/clientset/versioned/fake"
clusterv1informers "open-cluster-management.io/api/client/cluster/informers/externalversions"
clusterv1 "open-cluster-management.io/api/cluster/v1"
"open-cluster-management.io/sdk-go/pkg/patcher"
)

type testAgent struct {
Expand Down Expand Up @@ -47,9 +49,16 @@ func newManagedClusterWithAnnotation(name, key, value string) *clusterv1.Managed
return cluster
}

func newClusterManagementAddonWithAnnotation(name string, annotations map[string]string) *addonapiv1alpha1.ClusterManagementAddOn {
cma := addontesting.NewClusterManagementAddon(name, "", "").Build()
cma.Annotations = annotations
return cma
}

func TestReconcile(t *testing.T) {
cases := []struct {
name string
cma []runtime.Object
addon []runtime.Object
testaddons map[string]agent.AgentAddon
cluster []runtime.Object
Expand Down Expand Up @@ -193,12 +202,90 @@ func TestReconcile(t *testing.T) {
})},
},
},
{
name: "add annotation when uses install strategy",
cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{
"test": "test",
})},
addon: []runtime.Object{},
cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")},
validateAddonActions: func(t *testing.T, actions []clienttesting.Action) {
addontesting.AssertActions(t, actions, "patch", "create")
patch := actions[0].(clienttesting.PatchActionImpl).Patch
cma := &addonapiv1alpha1.ClusterManagementAddOn{}
err := json.Unmarshal(patch, cma)
if err != nil {
t.Fatal(err)
}

if len(cma.Annotations) != 1 || cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey] != addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue {
t.Errorf("cma annotation is not correct, expected self but got %s", cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey])
}
},
testaddons: map[string]agent.AgentAddon{
"test": &testAgent{name: "test", strategy: agent.InstallAllStrategy("test")},
},
},
{
name: "override annotation when uses install strategy",
cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{
"test": "test",
addonapiv1alpha1.AddonLifecycleAnnotationKey: addonapiv1alpha1.AddonLifecycleAddonManagerAnnotationValue,
})},
addon: []runtime.Object{},
cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")},
validateAddonActions: func(t *testing.T, actions []clienttesting.Action) {
addontesting.AssertActions(t, actions, "patch", "create")
patch := actions[0].(clienttesting.PatchActionImpl).Patch
cma := &addonapiv1alpha1.ClusterManagementAddOn{}
err := json.Unmarshal(patch, cma)
if err != nil {
t.Fatal(err)
}

if len(cma.Annotations) != 1 || cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey] != addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue {
t.Errorf("cma annotation is not correct, expected self but got %s", cma.Annotations[addonapiv1alpha1.AddonLifecycleAnnotationKey])
}
},
testaddons: map[string]agent.AgentAddon{
"test": &testAgent{name: "test", strategy: agent.InstallAllStrategy("test")},
},
},
{
name: "no patch annotation if managed by self",
cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{
"test": "test",
addonapiv1alpha1.AddonLifecycleAnnotationKey: addonapiv1alpha1.AddonLifecycleSelfManageAnnotationValue,
})},
addon: []runtime.Object{},
cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")},
validateAddonActions: func(t *testing.T, actions []clienttesting.Action) {
addontesting.AssertActions(t, actions, "create")
},
testaddons: map[string]agent.AgentAddon{
"test": &testAgent{name: "test", strategy: agent.InstallAllStrategy("test")},
},
},
{
name: "no patch annotation if no install strategy",
cma: []runtime.Object{newClusterManagementAddonWithAnnotation("test", map[string]string{
"test": "test",
addonapiv1alpha1.AddonLifecycleAnnotationKey: addonapiv1alpha1.AddonLifecycleAddonManagerAnnotationValue,
})},
addon: []runtime.Object{},
cluster: []runtime.Object{addontesting.NewManagedCluster("cluster1")},
validateAddonActions: addontesting.AssertNoActions,
testaddons: map[string]agent.AgentAddon{
"test": &testAgent{name: "test"},
},
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
obj := append(c.addon, c.cma...)
fakeClusterClient := fakecluster.NewSimpleClientset(c.cluster...)
fakeAddonClient := fakeaddon.NewSimpleClientset(c.addon...)
fakeAddonClient := fakeaddon.NewSimpleClientset(obj...)

addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute)
clusterInformers := clusterv1informers.NewSharedInformerFactory(fakeClusterClient, 10*time.Minute)
Expand All @@ -213,12 +300,21 @@ func TestReconcile(t *testing.T) {
t.Fatal(err)
}
}
for _, obj := range c.cma {
if err := addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer().GetStore().Add(obj); err != nil {
t.Fatal(err)
}
}

controller := addonInstallController{
addonClient: fakeAddonClient,
managedClusterLister: clusterInformers.Cluster().V1().ManagedClusters().Lister(),
managedClusterAddonLister: addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Lister(),
agentAddons: c.testaddons,
addonClient: fakeAddonClient,
managedClusterLister: clusterInformers.Cluster().V1().ManagedClusters().Lister(),
managedClusterAddonLister: addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Lister(),
clusterManagementAddonLister: addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(),
agentAddons: c.testaddons,
addonPatcher: patcher.NewPatcher[*addonapiv1alpha1.ClusterManagementAddOn,
addonapiv1alpha1.ClusterManagementAddOnSpec,
addonapiv1alpha1.ClusterManagementAddOnStatus](fakeAddonClient.AddonV1alpha1().ClusterManagementAddOns()),
}

for _, obj := range c.cluster {
Expand Down
1 change: 1 addition & 0 deletions pkg/addonmanager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ func (a *addonManager) StartWithInformers(ctx context.Context,
addonClient,
clusterInformers.Cluster().V1().ManagedClusters(),
addonInformers.Addon().V1alpha1().ManagedClusterAddOns(),
addonInformers.Addon().V1alpha1().ClusterManagementAddOns(),
a.addonAgents,
)

Expand Down
1 change: 1 addition & 0 deletions pkg/agent/inteface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ type AgentAddonOptions struct {
// Addon will not be installed automatically until a ManagedClusterAddon is applied to the cluster's
// namespace if InstallStrategy is nil.
// Deprecated: use installStrategy config in ClusterManagementAddOn API instead
// The migration plan refer to https://github.com/open-cluster-management-io/ocm/issues/355.
// +optional
InstallStrategy *InstallStrategy

Expand Down
Loading