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

Jump to bottom

fix: add finalizers to cleanup cluster scoped resources on stack dele… #608

Merged
merged 1 commit into from
Oct 29, 2024
Merged

fix: add finalizers to cleanup cluster scoped resources on stack dele… #608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion pkg/controllers/monitoring/monitoring-stack/alertmanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func newAlertmanagerPDB(ms *stack.MonitoringStack, instanceSelectorKey string, i
}
}

func newAlertManagerClusterRole(ms *stack.MonitoringStack, rbacResourceName string, rbacVerbs []string) *rbacv1.ClusterRole {
func newAlertManagerClusterRole(rbacResourceName string, rbacVerbs []string) *rbacv1.ClusterRole {
return &rbacv1.ClusterRole{
TypeMeta: metav1.TypeMeta{
APIVersion: rbacv1.SchemeGroupVersion.String(),
Expand Down
24 changes: 20 additions & 4 deletions pkg/controllers/monitoring/monitoring-stack/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,23 @@ const (
prometheusSecretsMountPoint = "/etc/prometheus/secrets"
)

var (
rbacVerbs = []string{"get", "list", "watch"}
)

func stackComponentCleanup(ms *stack.MonitoringStack) []reconciler.Reconciler {
prometheusName := ms.Name + "-prometheus"
alertmanagerName := ms.Name + "-alertmanager"
return []reconciler.Reconciler{
reconciler.NewDeleter(newPrometheusClusterRole(prometheusName, rbacVerbs)),
reconciler.NewDeleter(newClusterRoleBinding(ms, prometheusName)),
reconciler.NewDeleter(newRoleBindingForClusterRole(ms, prometheusName)),
reconciler.NewDeleter(newAlertManagerClusterRole(alertmanagerName, rbacVerbs)),
reconciler.NewDeleter(newClusterRoleBinding(ms, alertmanagerName)),
reconciler.NewDeleter(newRoleBindingForClusterRole(ms, alertmanagerName)),
}
}

func stackComponentReconcilers(
ms *stack.MonitoringStack,
instanceSelectorKey string,
Expand All @@ -35,15 +52,14 @@ func stackComponentReconcilers(
) []reconciler.Reconciler {
prometheusName := ms.Name + "-prometheus"
alertmanagerName := ms.Name + "-alertmanager"
rbacVerbs := []string{"get", "list", "watch"}
additionalScrapeConfigsSecretName := ms.Name + "-prometheus-additional-scrape-configs"
hasNsSelector := ms.Spec.NamespaceSelector != nil
deployAlertmanager := !ms.Spec.AlertmanagerConfig.Disabled

return []reconciler.Reconciler{
// Prometheus Deployment
reconciler.NewUpdater(newServiceAccount(prometheusName, ms.Namespace), ms),
reconciler.NewUpdater(newPrometheusClusterRole(ms, prometheusName, rbacVerbs), ms),
reconciler.NewUpdater(newPrometheusClusterRole(prometheusName, rbacVerbs), ms),
reconciler.NewUpdater(newAdditionalScrapeConfigsSecret(ms, additionalScrapeConfigsSecretName), ms),
reconciler.NewUpdater(newPrometheus(ms, prometheusName,
additionalScrapeConfigsSecretName,
Expand All @@ -60,7 +76,7 @@ func stackComponentReconcilers(
reconciler.NewOptionalUpdater(newClusterRoleBinding(ms, prometheusName), ms, hasNsSelector),
reconciler.NewOptionalUpdater(newRoleBindingForClusterRole(ms, prometheusName), ms, !hasNsSelector),

reconciler.NewOptionalUpdater(newAlertManagerClusterRole(ms, alertmanagerName, rbacVerbs), ms, deployAlertmanager),
reconciler.NewOptionalUpdater(newAlertManagerClusterRole(alertmanagerName, rbacVerbs), ms, deployAlertmanager),

// create clusterrolebinding if alertmanager is enabled and namespace selector is also present in MonitoringStack
reconciler.NewOptionalUpdater(newClusterRoleBinding(ms, alertmanagerName), ms, deployAlertmanager && hasNsSelector),
Expand All @@ -72,7 +88,7 @@ func stackComponentReconcilers(
}
}

func newPrometheusClusterRole(ms *stack.MonitoringStack, rbacResourceName string, rbacVerbs []string) *rbacv1.ClusterRole {
func newPrometheusClusterRole(rbacResourceName string, rbacVerbs []string) *rbacv1.ClusterRole {
return &rbacv1.ClusterRole{
TypeMeta: metav1.TypeMeta{
APIVersion: rbacv1.SchemeGroupVersion.String(),
Expand Down
34 changes: 33 additions & 1 deletion pkg/controllers/monitoring/monitoring-stack/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package monitoringstack
import (
"context"
"fmt"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -70,6 +71,8 @@ type Options struct {
Thanos ThanosConfiguration
}

const finalizerName = "monitoring.observability.openshift.io/finalizer"

// RBAC for managing monitoring stacks
//+kubebuilder:rbac:groups=monitoring.rhobs,resources=monitoringstacks,verbs=list;watch;create;update
//+kubebuilder:rbac:groups=monitoring.rhobs,resources=monitoringstacks/status,verbs=get;update
Expand Down Expand Up @@ -144,11 +147,40 @@ func (rm resourceManager) Reconcile(ctx context.Context, req ctrl.Request) (ctrl
return ctrl.Result{}, nil
}

// Check if the plugin is being deleted
if !ms.ObjectMeta.DeletionTimestamp.IsZero() {
logger.V(6).Info("skipping reconcile since object is already schedule for deletion")
logger.V(6).Info("removing cluster scoped resources")

reconcilers := stackComponentCleanup(ms)
Copy link
Contributor

Choose a reason for hiding this comment

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

just a nit over here, if we have the prometheus and the alertamanager names, why not adding that also as a log? It may be useful when debugging. Otherwise lgtm!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just an info log. In line 158 an error is logged in case something goes wrong, that'll include the resource name that creates an error. Is that what you're after?

Not sure we should log 6 resource names on the happy path.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, mistook the 'Info' for 'Err' in my mind, LGTM then, thanks!

for _, reconciler := range reconcilers {
err := reconciler.Reconcile(ctx, rm.k8sClient, rm.scheme)
if err != nil {
logger.Error(err, "failed to cleanup monitoring stack")
}
}

// Remove finalizer if present
if slices.Contains(ms.ObjectMeta.Finalizers, finalizerName) {
ms.ObjectMeta.Finalizers = slices.DeleteFunc(ms.ObjectMeta.Finalizers, func(currentFinalizerName string) bool {
return currentFinalizerName == finalizerName
})
if err := rm.k8sClient.Update(ctx, ms); err != nil {
return ctrl.Result{}, err
}
}

logger.V(6).Info("skipping reconcile since object is already scheduled for deletion")
return ctrl.Result{}, nil
}

// Add finalizer if not present
if !slices.Contains(ms.ObjectMeta.Finalizers, finalizerName) {
ms.ObjectMeta.Finalizers = append(ms.ObjectMeta.Finalizers, finalizerName)
if err := rm.k8sClient.Update(ctx, ms); err != nil {
return ctrl.Result{}, err
}
}

reconcilers := stackComponentReconcilers(ms,
rm.instanceSelectorKey,
rm.instanceSelectorValue,
Expand Down