feat: adds controller to allow addon to reconcile on changes #46

JoaoBraveCoding · 2024-05-06T17:58:13Z

This PR introduces a controller that will watch for resources used for the addon configuration and it will annotate the corresponding ManifestWorks triggering a reconciliation

coveralls · 2024-05-06T18:01:16Z

Pull Request Test Coverage Report for Build 9209225475

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 50.637%

Totals
Change from base Build 8877284128:	0.0%
Covered Lines:	318
Relevant Lines:	628

💛 - Coveralls

Signed-off-by: Israel Blancas <iblancasa@gmail.com>

This reverts commit 0e9894e.

iblancasa · 2024-05-08T14:36:30Z

It seems I have permission to push to this branch. I added a commit and reverted it. Added my changes in JoaoBraveCoding#11

* Move the watcher cli application to be a go routine Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix lint Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix lint Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Update internal/controllers/watcher/controller.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Update internal/controllers/watcher/controller.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Update main.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Update main.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Add suggestion Signed-off-by: Israel Blancas <iblancasa@gmail.com> --------- Signed-off-by: Israel Blancas <iblancasa@gmail.com> Co-authored-by: Joao Marcal <joao.marcal12@gmail.com>

Signed-off-by: Israel Blancas <iblancasa@gmail.com>

iblancasa · 2024-05-20T11:02:52Z

What is missing to move from draft to Ready for review?

JoaoBraveCoding

@iblancasa some points to improve

JoaoBraveCoding · 2024-05-21T09:26:59Z

internal/controllers/watcher/controller.go

+	client.Client
+	Log           logr.Logger
+	Scheme        *runtime.Scheme
+	addonnManager *addonmanager.AddonManager


Can we make this a reference? It will make the trigger code cleaner.

Suggested change

addonnManager *addonmanager.AddonManager

addonnManager addonmanager.AddonManager

Any reason why we didn't move this to a reference? This way we can do r.addonnManager.Trigger(req.Namespace, req.Name) instead of *(r.addonnManager).Trigger(req.Namespace, req.Name)

JoaoBraveCoding · 2024-05-21T09:28:41Z

internal/controllers/watcher/controller.go

+func (r *WatcherReconciler) enqueueForClusterWideResource() handler.EventHandler {
+	return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request {
+		addonList := &addonapiv1alpha1.ManagedClusterAddOnList{}
+		if err := r.Client.List(ctx, addonList, &client.ListOptions{
+			LabelSelector: labels.SelectorFromSet(labels.Set{
+				"open-cluster-management.io/addon-name": "multicluster-observability-addon",
+			}),
+		}); err != nil {
+			r.Log.Error(err, "Error listing managedclusteraddon resources in event handler")
+			return nil
+		}
+
+		if len(addonList.Items) == 0 {
+			r.Log.V(2).Info("no managedclusteraddon found")
+			return nil
+		}
+
+		clustersetValue, clustersetExists := obj.GetAnnotations()["cluster.open-cluster-management.io/clusterset"]
+		var clustersInClusterSet map[string]struct{}
+		if clustersetExists {
+			clusterList := &clusterv1.ManagedClusterList{}
+			if err := r.Client.List(ctx, clusterList, &client.ListOptions{
+				LabelSelector: labels.SelectorFromSet(labels.Set{
+					"cluster.open-cluster-management.io/clusterset": clustersetValue,
+				}),
+			}); err != nil {
+				r.Log.Error(err, "Error listing managedcluster resources in event handler")
+				return nil
+			}
+			clustersInClusterSet = make(map[string]struct{}, len(clusterList.Items))
+			for _, cluster := range clusterList.Items {
+				clustersInClusterSet[cluster.Name] = struct{}{}
+			}
+		}
+
+		var requests []reconcile.Request
+		for _, addon := range addonList.Items {
+			_, installed := clustersInClusterSet[addon.Namespace]
+			if clustersetExists && !installed {
+				continue
+			}
+
+			requests = append(requests, reconcile.Request{
+				NamespacedName: types.NamespacedName{
+					Namespace: addon.Namespace,
+					Name:      addon.Name,
+				},
+			})
+		}
+		return requests
+	})
+}


In the new design, we can drop this code as it will not be needed

JoaoBraveCoding · 2024-05-21T09:32:26Z

internal/controllers/watcher/controller.go

+	if req.Name == "" {
+		return ctrl.Result{}, noNameErr
+	}
+	if req.Namespace == "" {
+		return ctrl.Result{}, noNamespaceErr
+	}


We can remove these no? Our code shouldn't be triggering reconciliations with these, if it is then there's a bug somewhere

When I tested I tested without these and saw no extra reconciliations, any reason why we left them? Not sure how it works with the new enqueue function, but again, if we are reconciling things that we don't want then I would say there is a bug in the enqueue function.

JoaoBraveCoding · 2024-05-21T09:38:14Z

internal/controllers/watcher/controller.go

+	}()
+}
+
+// WatcherReconciler reconciles the ManagedClusterAddon to annotate the ManiestWorks resource


Comment needs to be updated

JoaoBraveCoding · 2024-05-21T09:39:53Z

internal/controllers/watcher/controller.go

+func (r *WatcherReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&addonapiv1alpha1.ManagedClusterAddOn{}, noReconcilePred).
+		Watches(&corev1.ConfigMap{}, r.enqueueForClusterSpecificResource(), builder.OnlyMetadata).


In the new design, we should be able to drop ConfigMaps.

JoaoBraveCoding · 2024-05-21T09:46:20Z

internal/controllers/watcher/controller.go

+		Complete(r)
+}
+
+func (r *WatcherReconciler) enqueueForClusterSpecificResource() handler.EventHandler {


In the new design, this enqueue function needs to be updated so that if secret gets updated:

If the namespace has a MCOA ManagedClusterAddOn then it should trigger an update for that

Otherwise, list all MCOA ManifestWorks and check if the secret is referenced in the resources, if it is, trigger a reconciliation for the MCOA ManagedClusterAddOn in the same namespace of the ManifestWorks (I'm also open to ideas/suggestion here on how we can do this better)

JoaoBraveCoding · 2024-05-21T15:15:13Z

FYI: Tested the current version of this PR with some of my suggestions together with #48 and everything worked out 👌

* If a secret is updated, check if there is any reference to it in any of the ManifestWorks * Change some comments Signed-off-by: Israel Blancas <iblancasa@gmail.com>

iblancasa · 2024-05-23T08:48:27Z

@JoaoBraveCoding I added the changes yesterday. I tested the changes and I think everything is working fine. Is ok if we move to Ready for review?

periklis · 2024-05-23T09:12:13Z

@iblancasa yes please move to ready for review. I am constantly skim over this and would like to add my review. Thanks for all the dedication and work you both put in here

JoaoBraveCoding · 2024-05-23T10:14:06Z

internal/controllers/watcher/controller.go

+	if req.Name == "" {
+		return ctrl.Result{}, noNameErr
+	}
+	if req.Namespace == "" {
+		return ctrl.Result{}, noNamespaceErr
+	}


When I tested I tested without these and saw no extra reconciliations, any reason why we left them? Not sure how it works with the new enqueue function, but again, if we are reconciling things that we don't want then I would say there is a bug in the enqueue function.

JoaoBraveCoding · 2024-05-23T10:15:12Z

internal/controllers/watcher/controller.go

+	client.Client
+	Log           logr.Logger
+	Scheme        *runtime.Scheme
+	addonnManager *addonmanager.AddonManager


Any reason why we didn't move this to a reference? This way we can do r.addonnManager.Trigger(req.Namespace, req.Name) instead of *(r.addonnManager).Trigger(req.Namespace, req.Name)

JoaoBraveCoding · 2024-05-23T10:20:05Z

internal/controllers/watcher/controller.go

+		Watches(&loggingv1.ClusterLogForwarder{}, r.enqueueForClusterSpecificResource(), builder.OnlyMetadata).
+		Watches(&otelv1alpha1.OpenTelemetryCollector{}, r.enqueueForClusterSpecificResource(), builder.OnlyMetadata).


Both these are not necessary, since they will be referenced on the ManagedClusterAddon Status if any changes happen to these resources it will automatically trigger a reconciliation

JoaoBraveCoding · 2024-05-23T10:21:11Z

internal/controllers/watcher/controller.go

+				switch obj.(type) {
+				case *corev1.Secret:
+					return r.getSecretReconcileRequests(ctx, obj, addon)
+				}
+				return nil


Since the watcher (for now) will only watch secrets we don't need to have this and we can call directly return r.getSecretReconcileRequests(ctx, obj, addon)

JoaoBraveCoding · 2024-05-23T10:27:44Z

internal/controllers/watcher/controller.go

+func (r *WatcherReconciler) getSecretReconcileRequests(ctx context.Context, obj client.Object, addon *addonapiv1alpha1.ManagedClusterAddOn) []reconcile.Request {
+	rqs := []reconcile.Request{}
+	mws := &workapiv1.ManifestWorkList{}
+	if err := r.Client.List(ctx, mws); err != nil {


Here we should list only the ManifestWork that belong to MCOA we can do so in the following way:

Suggested change

if err := r.Client.List(ctx, mws); err != nil {

if err := r.Client.List(ctx, mws, &client.ListOptions{

LabelSelector: labels.SelectorFromSet(labels.Set{

"open-cluster-management.io/addon-name": "multicluster-observability-addon",

}),

}); err != nil {

JoaoBraveCoding · 2024-05-23T10:28:25Z

internal/controllers/watcher/controller.go

+						reconcile.Request{
+							NamespacedName: types.NamespacedName{
+								Name:      addon.Name,
+								Namespace: mw.Namespace,


We can move this to a contant since this will aways be the same

Signed-off-by: Israel Blancas <iblancasa@gmail.com>

JoaoBraveCoding added 3 commits May 6, 2024 10:34

feat: addd controller to watch configuration resources

83db164

feat: adds controller to annotate ManifestWork

8e7d7a1

fix: add missing schemas

7dc5253

JoaoBraveCoding added 3 commits May 7, 2024 12:25

feat: adds new event handler for cluster wide resources

2cf94c9

chore: improves triggers on clusterwideresources

0c76944

chore: improve enqueueForClusterWideResource function

261a928

JoaoBraveCoding force-pushed the watcher branch from eafaf41 to 261a928 Compare May 8, 2024 09:45

iblancasa added 2 commits May 8, 2024 16:32

Add OpenTelemetryCollector resource to the reconciliation

0e9894e

Signed-off-by: Israel Blancas <iblancasa@gmail.com>

Revert "Add OpenTelemetryCollector resource to the reconciliation"

48cad8b

This reverts commit 0e9894e.

iblancasa and others added 2 commits May 14, 2024 14:29

Move to use addon-framework Trigger function (#13)

ef5c3ec

Signed-off-by: Israel Blancas <iblancasa@gmail.com>

revert unrelated changes

86de945

JoaoBraveCoding commented May 21, 2024

View reviewed changes

* Remove ConfigMaps from reconciliation.

f30d666

* If a secret is updated, check if there is any reference to it in any of the ManifestWorks * Change some comments Signed-off-by: Israel Blancas <iblancasa@gmail.com>

JoaoBraveCoding marked this pull request as ready for review May 23, 2024 10:11

JoaoBraveCoding requested review from a team as code owners May 23, 2024 10:11

JoaoBraveCoding commented May 23, 2024

View reviewed changes

Add changes requested in code review

05e497d

Signed-off-by: Israel Blancas <iblancasa@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds controller to allow addon to reconcile on changes #46

feat: adds controller to allow addon to reconcile on changes #46

JoaoBraveCoding commented May 6, 2024

coveralls commented May 6, 2024 •

edited

iblancasa commented May 8, 2024

iblancasa commented May 20, 2024

JoaoBraveCoding left a comment

JoaoBraveCoding May 21, 2024

JoaoBraveCoding May 23, 2024

JoaoBraveCoding May 21, 2024

JoaoBraveCoding May 21, 2024

JoaoBraveCoding May 23, 2024

JoaoBraveCoding May 21, 2024

JoaoBraveCoding May 21, 2024

JoaoBraveCoding May 21, 2024

JoaoBraveCoding commented May 21, 2024

iblancasa commented May 23, 2024

periklis commented May 23, 2024

JoaoBraveCoding May 23, 2024

JoaoBraveCoding May 23, 2024

JoaoBraveCoding May 23, 2024

JoaoBraveCoding May 23, 2024

JoaoBraveCoding May 23, 2024

JoaoBraveCoding May 23, 2024

	addonnManager *addonmanager.AddonManager
	addonnManager addonmanager.AddonManager

		Watches(&loggingv1.ClusterLogForwarder{}, r.enqueueForClusterSpecificResource(), builder.OnlyMetadata).
		Watches(&otelv1alpha1.OpenTelemetryCollector{}, r.enqueueForClusterSpecificResource(), builder.OnlyMetadata).

feat: adds controller to allow addon to reconcile on changes #46

Are you sure you want to change the base?

feat: adds controller to allow addon to reconcile on changes #46

Conversation

JoaoBraveCoding commented May 6, 2024

coveralls commented May 6, 2024 • edited

Pull Request Test Coverage Report for Build 9209225475

Details

💛 - Coveralls

iblancasa commented May 8, 2024

iblancasa commented May 20, 2024

JoaoBraveCoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoaoBraveCoding commented May 21, 2024

iblancasa commented May 23, 2024

periklis commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 6, 2024 •

edited