Files
open-cluster-management/pkg/addon/controllers/addontemplate/controller_test.go
Jian Zhu b506d16cf8
Some checks failed
Post / coverage (push) Failing after 38s
Post / images (amd64, addon-manager) (push) Failing after 33s
Post / images (amd64, placement) (push) Failing after 41s
Post / images (amd64, registration) (push) Failing after 40s
Post / images (amd64, registration-operator) (push) Failing after 38s
Post / images (amd64, work) (push) Failing after 36s
Post / images (arm64, addon-manager) (push) Failing after 35s
Post / images (arm64, placement) (push) Failing after 39s
Post / images (arm64, registration) (push) Failing after 34s
Post / images (arm64, registration-operator) (push) Failing after 33s
Post / images (arm64, work) (push) Failing after 35s
Post / image manifest (addon-manager) (push) Has been skipped
Post / image manifest (placement) (push) Has been skipped
Post / image manifest (registration) (push) Has been skipped
Post / image manifest (registration-operator) (push) Has been skipped
Post / image manifest (work) (push) Has been skipped
Post / trigger clusteradm e2e (push) Has been skipped
Scorecard supply-chain security / Scorecard analysis (push) Failing after 41s
Close stale issues and PRs / stale (push) Failing after 27s
🐛 Fix ManagedClusterAddons not removed when ClusterManagementAddon is deleted (#1160)
* Fix ManagedClusterAddons not removed when ClusterManagementAddon is deleted

The addon template controller was stopping addon managers immediately when
ClusterManagementAddon was deleted, without waiting for pre-delete jobs
to complete or ManagedClusterAddons to be cleaned up via owner reference
cascading deletion.

This change implements the TODO at line 105 by checking if all
ManagedClusterAddons are deleted before stopping the manager. The controller
now uses field selectors to efficiently query for remaining ManagedClusterAddons
and requeues after 10 seconds if any still exist, allowing time for proper
cleanup.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* add e2e test

Signed-off-by: zhujian <jiazhu@redhat.com>

* return err when stopUnusedManagers failed

Signed-off-by: zhujian <jiazhu@redhat.com>

* Address review comments for addon manager deletion fix

- Use lister instead of API client for better performance
- Add named constant for requeue delay
- Fix test cache synchronization issues
- Improve test coverage from 74.7% to 75.6%

Addresses review feedback from Qiujian16 and CodeRabbit.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Fix e2e test timeout for configmap deletion check

Add explicit 180s timeout for pre-delete job configmap cleanup.
The default 90s timeout was insufficient for the deletion workflow.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Improve error logging in template agent

- Replace utilruntime.HandleError with structured logging in CSR functions
- Add more context to error messages for better debugging
- Use logger.Info for template retrieval errors to provide better visibility

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Use ManagedClusterAddonByName index for efficient lookup

- Replace inefficient list-and-filter with indexed lookup
- Add managedClusterAddonIndexer field to controller struct
- Update comment to accurately describe functionality
- Fix unit tests to properly set up the required index

This addresses the PR review feedback to use the existing index
instead of listing all ManagedClusterAddOns and filtering by name.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Remove unused mcaLister field

Since we now use managedClusterAddonIndexer for efficient lookup,
the mcaLister field is no longer needed. This cleanup reduces
memory usage and simplifies the controller structure.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Replace inefficient list-and-filter with indexed lookup in runController

Use managedClusterAddonIndexer.ByIndex() instead of listing all ManagedClusterAddOns
and filtering by name. This provides O(1) indexed lookup instead of O(n) linear scan.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Fix review comments for addon manager deletion

- Fix closure capture bug in controller test by using captured variables
- Fix typo 'copyiedConfig' to 'copiedConfig' in e2e tests

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Optimize ManagedClusterAddOn event handling in addon template controller

Replace filtered event handling with custom event handlers that only trigger
reconciliation when AddOnTemplate configReferences actually change. This
reduces unnecessary reconciliation cycles by using reflect.DeepEqual to
compare config references between old and new objects.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: zhujian <jiazhu@redhat.com>

* Revert "Optimize ManagedClusterAddOn event handling in addon template controller"

This reverts commit 4649d1b9ac.

Signed-off-by: zhujian <jiazhu@redhat.com>

---------

Signed-off-by: zhujian <jiazhu@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>
2025-09-10 01:30:19 +00:00

420 lines
15 KiB
Go

package addontemplate
import (
"context"
"sync"
"testing"
"time"
"github.com/openshift/library-go/pkg/operator/events/eventstesting"
"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/dynamic/dynamicinformer"
dynamicfake "k8s.io/client-go/dynamic/fake"
fakekube "k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/cache"
"open-cluster-management.io/addon-framework/pkg/addonmanager/addontesting"
"open-cluster-management.io/addon-framework/pkg/utils"
addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1"
fakeaddon "open-cluster-management.io/api/client/addon/clientset/versioned/fake"
addoninformers "open-cluster-management.io/api/client/addon/informers/externalversions"
fakecluster "open-cluster-management.io/api/client/cluster/clientset/versioned/fake"
clusterv1informers "open-cluster-management.io/api/client/cluster/informers/externalversions"
fakework "open-cluster-management.io/api/client/work/clientset/versioned/fake"
workinformers "open-cluster-management.io/api/client/work/informers/externalversions"
addonindex "open-cluster-management.io/ocm/pkg/addon/index"
testingcommon "open-cluster-management.io/ocm/pkg/common/testing"
testinghelpers "open-cluster-management.io/ocm/pkg/registration/helpers/testing"
)
func TestReconcile(t *testing.T) {
cases := []struct {
name string
syncKeys []string
managedClusteraddon []runtime.Object
clusterManagementAddon []runtime.Object
expectedCount int
expectedTimeout bool
}{
{
name: "no clustermanagementaddon",
syncKeys: []string{"test"},
managedClusteraddon: []runtime.Object{},
clusterManagementAddon: []runtime.Object{},
expectedCount: 0,
expectedTimeout: true,
},
{
name: "not template type clustermanagementaddon",
syncKeys: []string{"test"},
managedClusteraddon: []runtime.Object{},
clusterManagementAddon: []runtime.Object{
addontesting.NewClusterManagementAddon("test", "", "").Build()},
expectedCount: 0,
expectedTimeout: true,
},
{
name: "one template type clustermanagementaddon",
syncKeys: []string{"test"},
managedClusteraddon: []runtime.Object{},
clusterManagementAddon: []runtime.Object{
addontesting.NewClusterManagementAddon("test", "", "").WithSupportedConfigs(
addonv1alpha1.ConfigMeta{
ConfigGroupResource: addonv1alpha1.ConfigGroupResource{
Group: utils.AddOnTemplateGVR.Group,
Resource: utils.AddOnTemplateGVR.Resource,
},
DefaultConfig: &addonv1alpha1.ConfigReferent{Name: "test"},
}).Build()},
expectedCount: 1,
expectedTimeout: false,
},
{
name: "two template type clustermanagementaddon",
syncKeys: []string{"test", "test1"},
managedClusteraddon: []runtime.Object{},
clusterManagementAddon: []runtime.Object{
addontesting.NewClusterManagementAddon("test", "", "").WithSupportedConfigs(
addonv1alpha1.ConfigMeta{
ConfigGroupResource: addonv1alpha1.ConfigGroupResource{
Group: utils.AddOnTemplateGVR.Group,
Resource: utils.AddOnTemplateGVR.Resource,
},
DefaultConfig: &addonv1alpha1.ConfigReferent{Name: "test"},
}).Build(),
addontesting.NewClusterManagementAddon("test1", "", "").WithSupportedConfigs(
addonv1alpha1.ConfigMeta{
ConfigGroupResource: addonv1alpha1.ConfigGroupResource{
Group: utils.AddOnTemplateGVR.Group,
Resource: utils.AddOnTemplateGVR.Resource,
},
DefaultConfig: &addonv1alpha1.ConfigReferent{Name: "test"},
}).Build(),
},
expectedCount: 2,
expectedTimeout: false,
},
{
name: "two template type and one not template type clustermanagementaddon",
syncKeys: []string{"test", "test1", "test2"},
managedClusteraddon: []runtime.Object{},
clusterManagementAddon: []runtime.Object{
addontesting.NewClusterManagementAddon("test", "", "").WithSupportedConfigs(
addonv1alpha1.ConfigMeta{
ConfigGroupResource: addonv1alpha1.ConfigGroupResource{
Group: utils.AddOnTemplateGVR.Group,
Resource: utils.AddOnTemplateGVR.Resource,
},
DefaultConfig: &addonv1alpha1.ConfigReferent{Name: "test"},
}).Build(),
addontesting.NewClusterManagementAddon("test1", "", "").WithSupportedConfigs(
addonv1alpha1.ConfigMeta{
ConfigGroupResource: addonv1alpha1.ConfigGroupResource{
Group: utils.AddOnTemplateGVR.Group,
Resource: utils.AddOnTemplateGVR.Resource,
},
DefaultConfig: &addonv1alpha1.ConfigReferent{Name: "test"},
}).Build(),
addontesting.NewClusterManagementAddon("test2", "", "").Build(),
},
expectedCount: 2,
expectedTimeout: true,
},
}
for _, c := range cases {
count := 0
var wg sync.WaitGroup
lock := &sync.Mutex{}
rederCount := func() int {
lock.Lock()
defer lock.Unlock()
return count
}
increaseCount := func() {
lock.Lock()
defer lock.Unlock()
count++
}
for range c.syncKeys {
wg.Add(1)
}
runController := func(ctx context.Context, addonName string) error {
defer wg.Done()
increaseCount()
return nil
}
obj := append(c.clusterManagementAddon, c.managedClusteraddon...) //nolint:gocritic
fakeAddonClient := fakeaddon.NewSimpleClientset(obj...)
addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute)
// Add the index for ManagedClusterAddonByName
err := addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer().AddIndexers(
cache.Indexers{
addonindex.ManagedClusterAddonByName: addonindex.IndexManagedClusterAddonByName,
})
if err != nil {
t.Fatal(err)
}
for _, obj := range c.managedClusteraddon {
if err := addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer().GetStore().Add(obj); err != nil {
t.Fatal(err)
}
}
for _, obj := range c.clusterManagementAddon {
if err := addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Informer().GetStore().Add(obj); err != nil {
t.Fatal(err)
}
}
fakeDynamicClient := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme())
dynamicInformerFactory := dynamicinformer.NewDynamicSharedInformerFactory(fakeDynamicClient, 0)
fakeClusterClient := fakecluster.NewSimpleClientset()
clusterInformers := clusterv1informers.NewSharedInformerFactory(fakeClusterClient, 10*time.Minute)
fakeWorkClient := fakework.NewSimpleClientset()
workInformers := workinformers.NewSharedInformerFactory(fakeWorkClient, 10*time.Minute)
hubKubeClient := fakekube.NewSimpleClientset()
controller := NewAddonTemplateController(
nil,
hubKubeClient,
fakeAddonClient,
fakeWorkClient,
addonInformers,
clusterInformers,
dynamicInformerFactory,
workInformers,
eventstesting.NewTestingEventRecorder(t),
runController,
)
ctx := context.TODO()
for _, syncKey := range c.syncKeys {
syncContext := testingcommon.NewFakeSyncContext(t, syncKey)
err := controller.Sync(ctx, syncContext)
if err != nil {
t.Errorf("expected no error when sync: %v", err)
}
}
ch := make(chan struct{})
go func() {
defer close(ch)
wg.Wait()
}()
select {
case <-ch:
actualCount := rederCount()
if actualCount != c.expectedCount {
t.Errorf("name : %s, expected runControllerFunc to be called %d, but was called %d times",
c.name, c.expectedCount, actualCount)
}
case <-time.After(1 * time.Second):
if !c.expectedTimeout {
t.Errorf("name : %s, expected not timeout, but timeout", c.name)
}
actualCount := rederCount()
if actualCount != c.expectedCount {
t.Errorf("name : %s, expected runControllerFunc to be called %d, but was called %d times",
c.name, c.expectedCount, actualCount)
}
}
}
}
func TestRunController(t *testing.T) {
cases := []struct {
name string
addonName string
expectedErr string
}{
{
name: "addon name empty",
addonName: "",
expectedErr: "addon name should be set",
},
{
name: "fake kubeconfig",
addonName: "test",
expectedErr: `connect: connection refused`,
},
}
for _, c := range cases {
fakeAddonClient := fakeaddon.NewSimpleClientset()
addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute)
fakeDynamicClient := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme())
dynamicInformerFactory := dynamicinformer.NewDynamicSharedInformerFactory(fakeDynamicClient, 0)
fakeClusterClient := fakecluster.NewSimpleClientset()
clusterInformers := clusterv1informers.NewSharedInformerFactory(fakeClusterClient, 10*time.Minute)
fakeWorkClient := fakework.NewSimpleClientset()
workInformers := workinformers.NewSharedInformerFactory(fakeWorkClient, 10*time.Minute)
hubKubeClient := fakekube.NewSimpleClientset()
controller := &addonTemplateController{
kubeConfig: &rest.Config{},
kubeClient: hubKubeClient,
addonClient: fakeAddonClient,
cmaLister: addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(),
managedClusterAddonIndexer: addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer().GetIndexer(),
addonManagers: make(map[string]context.CancelFunc),
addonInformers: addonInformers,
clusterInformers: clusterInformers,
dynamicInformers: dynamicInformerFactory,
workInformers: workInformers,
}
ctx := context.TODO()
err := controller.runController(ctx, c.addonName)
if err == nil {
assert.Empty(t, c.expectedErr)
} else {
assert.Contains(t, err.Error(), c.expectedErr, "name : %s, expected error %v, but got %v", c.name, c.expectedErr, err)
}
}
}
func TestStopUnusedManagers(t *testing.T) {
cases := []struct {
name string
addonName string
managedClusterAddons []runtime.Object
existingManagers map[string]context.CancelFunc
expectedManagerStopped bool
expectedRequeue bool
}{
{
name: "no managed cluster addons, manager should be stopped",
addonName: "test-addon",
managedClusterAddons: []runtime.Object{},
existingManagers: map[string]context.CancelFunc{
"test-addon": func() {},
},
expectedManagerStopped: true,
expectedRequeue: false,
},
{
name: "managed cluster addons exist, manager should not be stopped",
addonName: "test-addon",
managedClusterAddons: []runtime.Object{
testinghelpers.NewManagedClusterAddons("test-addon", "cluster1", nil, nil),
},
existingManagers: map[string]context.CancelFunc{
"test-addon": func() {},
},
expectedManagerStopped: false,
expectedRequeue: true,
},
{
name: "multiple managed cluster addons exist, manager should not be stopped",
addonName: "test-addon",
managedClusterAddons: []runtime.Object{
testinghelpers.NewManagedClusterAddons("test-addon", "cluster1", nil, nil),
testinghelpers.NewManagedClusterAddons("test-addon", "cluster2", nil, nil),
},
existingManagers: map[string]context.CancelFunc{
"test-addon": func() {},
},
expectedManagerStopped: false,
expectedRequeue: true,
},
{
name: "no manager exists, should not error",
addonName: "test-addon",
managedClusterAddons: []runtime.Object{},
existingManagers: map[string]context.CancelFunc{},
expectedManagerStopped: false,
expectedRequeue: false,
},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
fakeAddonClient := fakeaddon.NewSimpleClientset(c.managedClusterAddons...)
addonInformers := addoninformers.NewSharedInformerFactory(fakeAddonClient, 10*time.Minute)
fakeDynamicClient := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme())
dynamicInformerFactory := dynamicinformer.NewDynamicSharedInformerFactory(fakeDynamicClient, 0)
fakeClusterClient := fakecluster.NewSimpleClientset()
clusterInformers := clusterv1informers.NewSharedInformerFactory(fakeClusterClient, 10*time.Minute)
fakeWorkClient := fakework.NewSimpleClientset()
workInformers := workinformers.NewSharedInformerFactory(fakeWorkClient, 10*time.Minute)
hubKubeClient := fakekube.NewSimpleClientset()
managerStopped := false
existingManagers := make(map[string]context.CancelFunc)
for name, stopFunc := range c.existingManagers {
capturedName := name
capturedStopFunc := stopFunc
existingManagers[name] = func() {
if capturedName == c.addonName {
managerStopped = true
}
capturedStopFunc()
}
}
// Add the index for ManagedClusterAddonByName
err := addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer().AddIndexers(
cache.Indexers{
addonindex.ManagedClusterAddonByName: addonindex.IndexManagedClusterAddonByName,
})
if err != nil {
t.Fatal(err)
}
controller := &addonTemplateController{
kubeConfig: &rest.Config{},
kubeClient: hubKubeClient,
addonClient: fakeAddonClient,
workClient: fakeWorkClient,
cmaLister: addonInformers.Addon().V1alpha1().ClusterManagementAddOns().Lister(),
managedClusterAddonIndexer: addonInformers.Addon().V1alpha1().ManagedClusterAddOns().Informer().GetIndexer(),
addonManagers: existingManagers,
addonInformers: addonInformers,
clusterInformers: clusterInformers,
dynamicInformers: dynamicInformerFactory,
workInformers: workInformers,
eventRecorder: eventstesting.NewTestingEventRecorder(t),
}
// Start informers and wait for cache sync
ctx := context.TODO()
addonInformers.Start(ctx.Done())
addonInformers.WaitForCacheSync(ctx.Done())
syncContext := testingcommon.NewFakeSyncContext(t, c.addonName)
err = controller.stopUnusedManagers(ctx, syncContext, c.addonName)
assert.NoError(t, err)
// Check if manager was stopped
if c.expectedManagerStopped {
assert.True(t, managerStopped, "expected manager to be stopped")
_, exists := controller.addonManagers[c.addonName]
assert.False(t, exists, "expected manager to be removed from map")
} else {
assert.False(t, managerStopped, "expected manager not to be stopped")
if len(c.existingManagers) > 0 {
_, exists := controller.addonManagers[c.addonName]
assert.True(t, exists, "expected manager to still exist in map")
}
}
// Check if requeue was called
if c.expectedRequeue {
// We can't easily test the exact requeue behavior with the fake sync context
// but we can verify the manager wasn't stopped when it should be requeued
assert.False(t, managerStopped, "manager should not be stopped when requeue is expected")
}
})
}
}