fix: cleaning up grafana datasource on deleting monitoring stack #107

prashbnair · 2021-12-15T14:18:56Z

this fix will clean up the grafana data source created by the monitoring
stack on deleting the monitoring stack

sthaha · 2021-12-22T06:58:27Z

pkg/controllers/monitoring-stack/controller.go

+	if ms.ObjectMeta.DeletionTimestamp.IsZero() {
+		if !containsString(ms.ObjectMeta.Finalizers, finalizerName) {
+			controllerutil.AddFinalizer(ms, finalizerName)
+			if err := r.k8sClient.Update(ctx, ms); err != nil {
+				if errors.IsConflict(err) {
+					return ctrl.Result{RequeueAfter: 2 * time.Second}, nil
+				}
+				return ctrl.Result{}, err
+			}
+		}


This part is a bit confusing to understand ... Why does cleanupResources add a finalizer?

We can change it but then we will need to add a check that DeletionTimestamp is not zero before we can actually delete the resource. That is what tells us that we are in delete mode.
I feel it better this way as the logic is clear.

sthaha · 2021-12-22T07:00:33Z

pkg/controllers/monitoring-stack/controller.go

@@ -46,11 +46,13 @@ import (

 	"github.com/go-logr/logr"
 	monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+	grafana_operator "github.com/rhobs/monitoring-stack-operator/pkg/controllers/grafana-operator"


it would be nice if package names didn't use _ - how about goctrl just like in operator.go

sthaha · 2021-12-22T07:03:45Z

pkg/controllers/monitoring-stack/controller.go

+
+func (r *reconciler) cleanupResources(ctx context.Context, ms *stack.MonitoringStack) (ctrl.Result, error) {
+	if ms.ObjectMeta.DeletionTimestamp.IsZero() {
+		if !containsString(ms.ObjectMeta.Finalizers, finalizerName) {


this check shouldn't necessary since AddFinalizers already does this check

fpetkovski · 2021-12-22T07:09:42Z

pkg/controllers/monitoring-stack/controller.go

+
+func (r *reconciler) cleanupResources(ctx context.Context, ms *stack.MonitoringStack) (ctrl.Result, error) {
+	if ms.ObjectMeta.DeletionTimestamp.IsZero() {
+		if !containsString(ms.ObjectMeta.Finalizers, finalizerName) {


We can use an existing helper for this: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil#ContainsFinalizer

sthaha · 2021-12-22T07:11:51Z

pkg/controllers/monitoring-stack/controller.go

+				return ctrl.Result{}, err
+			}
+		}
+	} else if containsString(ms.ObjectMeta.Finalizers, finalizerName) {


should we check if the finalizer is set before deleting grafanadatasource?

It would have been set in the if condition, this is just to confirm.

pkg/controllers/monitoring-stack/controller.go

sthaha · 2021-12-22T07:15:34Z

pkg/controllers/monitoring-stack/controller.go


+		if err := r.k8sClient.Delete(ctx, grafanaDS); err != nil {


I think we should ignore errors.IsNotExists() when deleting

Do you want to ignore the error when deleting, I am not sure about that.

I think this error is safe to ignore. Otherwise, we can end up in an endless loop if the child resource does not exist because errors are retried.

test/e2e/monitoring_stack_controller_test.go

sthaha · 2021-12-22T07:19:21Z

test/e2e/monitoring_stack_controller_test.go

+
+	f.K8sClient.Delete(context.Background(), ms)
+
+	if err = wait.Poll(5*time.Second, 20*time.Second, func() (done bool, err error) {


can't we use AssertResourceNeverExists here?

sthaha · 2021-12-22T07:25:01Z

pkg/controllers/monitoring-stack/controller.go

@@ -164,7 +166,41 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu

 		}
 	}
+	return r.cleanupResources(ctx, ms)


wouldn't this run all patchers unnecessarily when a monitoring stack is deleted?
Shouldn't we handle deletion way early on in the reconcile loop .. i.e as soon as get the ms - MonitoringStack ?

If we move it to the top, when the reconcile loop runs for the first time, the stack is not completely initialised.
Shouldn't we be adding the finalizer only after all components have been created?

I think the idea is to run this code at the top when the deletion timestamp is set. So the algorithm would be:

If deletion timestamp is set, delete datasource, remove finalizer and update stack

else If finalizer not set, set finalizer and update stack

else reconcile child resources

sthaha · 2022-01-05T06:35:12Z

pkg/controllers/monitoring-stack/controller.go

+}
+
+func (r *reconciler) setupFinalizer(ctx context.Context, ms *stack.MonitoringStack) (ctrl.Result, error) {
+	if !controllerutil.ContainsFinalizer(ms, finalizerName) {


Suggested change

if !controllerutil.ContainsFinalizer(ms, finalizerName) {

if controllerutil.ContainsFinalizer(ms, finalizerName) {

sthaha · 2022-01-05T06:37:01Z

test/e2e/monitoring_stack_controller_test.go

+
+	f.AssertResourceEventuallyExists(datasourceName, goctrl.Namespace, grafanaDS)
+
+	assert.NilError(t, err, "grafana data source is not created")


is this true?

Suggested change

assert.NilError(t, err, "grafana data source is not created")

sthaha · 2022-01-05T06:37:26Z

test/e2e/monitoring_stack_controller_test.go

+	assert.NilError(t, err, "grafana data source is not created")
+
+	f.K8sClient.Delete(context.Background(), ms)
+


Suggested change

sthaha · 2022-01-05T06:37:54Z

test/e2e/monitoring_stack_controller_test.go

+	grafanaDS := &grafanav1alpha1.GrafanaDataSource{}
+
+	f.AssertResourceEventuallyExists(datasourceName, goctrl.Namespace, grafanaDS)
+


Suggested change

sthaha · 2022-01-05T06:40:17Z

test/e2e/monitoring_stack_controller_test.go

+	assert.NilError(t, err, "failed to create a monitoring stack")
+
+	grafanaDS := &grafanav1alpha1.GrafanaDataSource{}
+


Suggested change

sthaha

Almost good to go.

this fix will clean up the grafana data source created by the monitoring stack on deleting the monitoring stack

sthaha · 2022-01-06T01:18:32Z

pkg/controllers/monitoring-stack/controller.go

+	return ctrl.Result{}, err
+}
+
+func (r *reconciler) setupFinalizer(ctx context.Context, ms *stack.MonitoringStack) (ctrl.Result, error) {


@prashbnair I think it is worth having a test that validates that the finalizer is set on a monitoring stack

Note ... This can be done is a follow up PR as well.

sthaha · 2022-01-06T01:18:41Z

test/e2e/monitoring_stack_controller_test.go

+
+	grafanaDS := &grafanav1alpha1.GrafanaDataSource{}
+	f.AssertResourceEventuallyExists(datasourceName, goctrl.Namespace, grafanaDS)(t)
+


We can add another validation here that the finalizer is set on the ms that was created above

sthaha

@prashbnair I am approving so that we can merge this PR that has been waiting for a longtime. However, I do think we need to validate that the finalizer is set.

prashbnair requested a review from a team December 15, 2021 14:18

prashbnair changed the title ~~fix:cleaning up grafana datasource on deleting monitoring stack~~ fix :cleaning up grafana datasource on deleting monitoring stack Dec 15, 2021

prashbnair changed the title ~~fix :cleaning up grafana datasource on deleting monitoring stack~~ fix: cleaning up grafana datasource on deleting monitoring stack Dec 15, 2021

prashbnair force-pushed the mon-2029 branch 3 times, most recently from 6422166 to b3b9538 Compare December 21, 2021 10:24

sthaha reviewed Dec 22, 2021

View reviewed changes

fpetkovski reviewed Dec 22, 2021

View reviewed changes

sthaha reviewed Dec 22, 2021

View reviewed changes

pkg/controllers/monitoring-stack/controller.go Outdated Show resolved Hide resolved

sthaha reviewed Dec 22, 2021

View reviewed changes

test/e2e/monitoring_stack_controller_test.go Outdated Show resolved Hide resolved

sthaha reviewed Dec 22, 2021

View reviewed changes

prashbnair force-pushed the mon-2029 branch 3 times, most recently from 24496c1 to b433371 Compare December 23, 2021 09:17

prashbnair force-pushed the mon-2029 branch from b433371 to bbda4b5 Compare January 5, 2022 05:47

sthaha reviewed Jan 5, 2022

View reviewed changes

sthaha requested changes Jan 5, 2022

View reviewed changes

fix: cleaning up grafana datasource on deleting monitoring stack

cc27ae2

this fix will clean up the grafana data source created by the monitoring stack on deleting the monitoring stack

prashbnair force-pushed the mon-2029 branch from bbda4b5 to cc27ae2 Compare January 5, 2022 09:21

sthaha reviewed Jan 6, 2022

View reviewed changes

sthaha approved these changes Jan 6, 2022

View reviewed changes

prashbnair merged commit b125c25 into rhobs:main Jan 6, 2022

prashbnair deleted the mon-2029 branch January 6, 2022 04:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cleaning up grafana datasource on deleting monitoring stack #107

fix: cleaning up grafana datasource on deleting monitoring stack #107

prashbnair commented Dec 15, 2021

sthaha Dec 22, 2021

prashbnair Dec 22, 2021

sthaha Dec 22, 2021 •

edited

Loading

prashbnair Dec 22, 2021

sthaha Dec 22, 2021

prashbnair Dec 22, 2021

fpetkovski Dec 22, 2021 •

edited

Loading

prashbnair Dec 22, 2021

sthaha Dec 22, 2021

prashbnair Dec 22, 2021

sthaha Dec 22, 2021

prashbnair Dec 22, 2021

fpetkovski Dec 22, 2021

prashbnair Dec 23, 2021

sthaha Dec 22, 2021

prashbnair Dec 22, 2021

sthaha Dec 22, 2021

prashbnair Dec 22, 2021

fpetkovski Dec 22, 2021

prashbnair Dec 23, 2021

sthaha Jan 5, 2022

sthaha Jan 5, 2022

sthaha Jan 5, 2022

sthaha Jan 5, 2022

sthaha Jan 5, 2022

sthaha left a comment

sthaha Jan 6, 2022

sthaha Jan 6, 2022

sthaha left a comment


		f.K8sClient.Delete(context.Background(), ms)

		if err = wait.Poll(5time.Second, 20time.Second, func() (done bool, err error) {

	if !controllerutil.ContainsFinalizer(ms, finalizerName) {
	if controllerutil.ContainsFinalizer(ms, finalizerName) {


		f.AssertResourceEventuallyExists(datasourceName, goctrl.Namespace, grafanaDS)

		assert.NilError(t, err, "grafana data source is not created")

		grafanaDS := &grafanav1alpha1.GrafanaDataSource{}

		f.AssertResourceEventuallyExists(datasourceName, goctrl.Namespace, grafanaDS)

		assert.NilError(t, err, "failed to create a monitoring stack")

		grafanaDS := &grafanav1alpha1.GrafanaDataSource{}

fix: cleaning up grafana datasource on deleting monitoring stack #107

fix: cleaning up grafana datasource on deleting monitoring stack #107

Conversation

prashbnair commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha Dec 22, 2021 • edited Loading

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

fpetkovski Dec 22, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

sthaha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha left a comment

Choose a reason for hiding this comment

sthaha Dec 22, 2021 •

edited

Loading

fpetkovski Dec 22, 2021 •

edited

Loading