Skip to content

Commit

Permalink
controllers: switch to k8s contextual logger
Browse files Browse the repository at this point in the history
Remove own logger from controllers' reconcilers and switch to k8s
contextual logger instead [1].

Provide own LogConstructor to make it possible to change it's
configuration (levels and output) in the future.

Use contextual logger for SecretGeneratorReconciler and
CertConfigmapGeneratorReconciler setups as well.

Shorter name of the reconcilers' loggers, that
"operator"."controllers" sounds pretty much overkill for the only
operator.

DSCI unit test requires adoption as well.

[1] https://www.kubernetes.dev/blog/2022/05/25/contextual-logging/

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
  • Loading branch information
ykaliuta committed Oct 2, 2024
1 parent 0eade72 commit a897dac
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"context"
"reflect"

"github.com/go-logr/logr"
operatorv1 "github.com/openshift/api/operator/v1"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand All @@ -16,10 +15,12 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/logger"
annotation "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/trustedcabundle"
)
Expand All @@ -28,15 +29,14 @@ import (
type CertConfigmapGeneratorReconciler struct {
Client client.Client
Scheme *runtime.Scheme
Log logr.Logger
}

// SetupWithManager sets up the controller with the Manager.
func (r *CertConfigmapGeneratorReconciler) SetupWithManager(mgr ctrl.Manager) error {
log := r.Log
log.Info("Adding controller for Configmap Generation.")
func (r *CertConfigmapGeneratorReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
logf.FromContext(ctx).Info("Adding controller for Configmap Generation.")
return ctrl.NewControllerManagedBy(mgr).
Named("cert-configmap-generator-controller").
WithLogConstructor(logger.NewLogConstructor("CertConfigmapGenerator")).
Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.watchTrustedCABundleConfigMapResource), builder.WithPredicates(ConfigMapChangedPredicate)).
Watches(&corev1.Namespace{}, handler.EnqueueRequestsFromMapFunc(r.watchNamespaceResource), builder.WithPredicates(NamespaceCreatedPredicate)).
Complete(r)
Expand All @@ -45,7 +45,7 @@ func (r *CertConfigmapGeneratorReconciler) SetupWithManager(mgr ctrl.Manager) er
// Reconcile will generate new configmap, odh-trusted-ca-bundle, that includes cluster-wide trusted-ca bundle and custom
// ca bundle in every new namespace created.
func (r *CertConfigmapGeneratorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
log := r.Log
log := logf.FromContext(ctx)
// Request includes namespace that is newly created or where odh-trusted-ca-bundle configmap is updated.
log.Info("Reconciling CertConfigMapGenerator.", " Request.Namespace", req.NamespacedName)
// Get namespace instance
Expand Down Expand Up @@ -109,8 +109,8 @@ func (r *CertConfigmapGeneratorReconciler) watchNamespaceResource(_ context.Cont
return nil
}

func (r *CertConfigmapGeneratorReconciler) watchTrustedCABundleConfigMapResource(_ context.Context, a client.Object) []reconcile.Request {
log := r.Log
func (r *CertConfigmapGeneratorReconciler) watchTrustedCABundleConfigMapResource(ctx context.Context, a client.Object) []reconcile.Request {
log := logf.FromContext(ctx)
if a.GetName() == trustedcabundle.CAConfigMapName {
log.Info("Cert configmap has been updated, start reconcile")
return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: a.GetName(), Namespace: a.GetNamespace()}}}
Expand Down
23 changes: 12 additions & 11 deletions controllers/datasciencecluster/datasciencecluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

Expand All @@ -56,6 +57,7 @@ import (
"github.com/opendatahub-io/opendatahub-operator/v2/components/modelregistry"
"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/logger"
ctrlogger "github.com/opendatahub-io/opendatahub-operator/v2/pkg/logger"
annotations "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
Expand All @@ -66,7 +68,6 @@ import (
type DataScienceClusterReconciler struct {
client.Client
Scheme *runtime.Scheme
Log logr.Logger
// Recorder to generate events
Recorder record.EventRecorder
DataScienceCluster *DataScienceClusterConfig
Expand All @@ -84,7 +85,7 @@ const (
// Reconcile is part of the main kubernetes reconciliation loop which aims to
// move the current state of the cluster closer to the desired state.
func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { //nolint:maintidx,gocyclo
log := r.Log
log := logf.FromContext(ctx)
log.Info("Reconciling DataScienceCluster resources", "Request.Name", req.Name)

// Get information on version and platform
Expand Down Expand Up @@ -172,7 +173,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
saved.Status.Phase = status.PhaseError
})
if err != nil {
r.reportError(err, instance, "failed to update DataScienceCluster condition")
r.reportError(ctx, err, instance, "failed to update DataScienceCluster condition")

return ctrl.Result{}, err
}
Expand Down Expand Up @@ -236,7 +237,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
saved.Status.Release = currentOperatorRelease
})
if err != nil {
_ = r.reportError(err, instance, fmt.Sprintf("failed to add conditions to status of DataScienceCluster resource name %s", req.Name))
_ = r.reportError(ctx, err, instance, fmt.Sprintf("failed to add conditions to status of DataScienceCluster resource name %s", req.Name))

return ctrl.Result{}, err
}
Expand Down Expand Up @@ -292,7 +293,7 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R
func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context, instance *dscv1.DataScienceCluster,
platform cluster.Platform, component components.ComponentInterface,
) (*dscv1.DataScienceCluster, error) {
log := r.Log
log := logf.FromContext(ctx)
componentName := component.GetComponentName()

enabled := component.GetManagementState() == operatorv1.Managed
Expand All @@ -309,7 +310,7 @@ func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context
status.SetComponentCondition(&saved.Status.Conditions, componentName, status.ReconcileInit, message, corev1.ConditionUnknown)
})
if err != nil {
_ = r.reportError(err, instance, "failed to update DataScienceCluster conditions before first time reconciling "+componentName)
_ = r.reportError(ctx, err, instance, "failed to update DataScienceCluster conditions before first time reconciling "+componentName)
// try to continue with reconciliation, as further updates can fix the status
}
}
Expand All @@ -321,7 +322,7 @@ func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context

if err != nil {
// reconciliation failed: log errors, raise event and update status accordingly
instance = r.reportError(err, instance, "failed to reconcile "+componentName+" on DataScienceCluster")
instance = r.reportError(ctx, err, instance, "failed to reconcile "+componentName+" on DataScienceCluster")
instance, _ = status.UpdateWithRetry(ctx, r.Client, instance, func(saved *dscv1.DataScienceCluster) {
if enabled {
if strings.Contains(err.Error(), datasciencepipelines.ArgoWorkflowCRD+" CRD already exists") {
Expand Down Expand Up @@ -357,7 +358,7 @@ func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context
}
})
if err != nil {
instance = r.reportError(err, instance, "failed to update DataScienceCluster status after reconciling "+componentName)
instance = r.reportError(ctx, err, instance, "failed to update DataScienceCluster status after reconciling "+componentName)

return instance, err
}
Expand All @@ -374,9 +375,8 @@ func newComponentLogger(logger logr.Logger, componentName string, dscispec *dsci
return ctrlogger.NewNamedLogger(logger, "DSC.Components."+componentName, mode)
}

func (r *DataScienceClusterReconciler) reportError(err error, instance *dscv1.DataScienceCluster, message string) *dscv1.DataScienceCluster {
log := r.Log
log.Error(err, message, "instance.Name", instance.Name)
func (r *DataScienceClusterReconciler) reportError(ctx context.Context, err error, instance *dscv1.DataScienceCluster, message string) *dscv1.DataScienceCluster {
logf.FromContext(ctx).Error(err, message, "instance.Name", instance.Name)
r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DataScienceClusterReconcileError",
"%s for instance %s", message, instance.Name)
return instance
Expand Down Expand Up @@ -477,6 +477,7 @@ var modelMeshGeneralPredicates = predicate.Funcs{
func (r *DataScienceClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
return ctrl.NewControllerManagedBy(mgr).
For(&dscv1.DataScienceCluster{}).
WithLogConstructor(logger.NewLogConstructor("DataScienceCluster")).
Owns(&corev1.Namespace{}).
Owns(&corev1.Secret{}).
Owns(
Expand Down
17 changes: 9 additions & 8 deletions controllers/dscinitialization/dscinitialization_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"path/filepath"
"reflect"

"github.com/go-logr/logr"
operatorv1 "github.com/openshift/api/operator/v1"
routev1 "github.com/openshift/api/route/v1"
appsv1 "k8s.io/api/apps/v1"
Expand All @@ -39,6 +38,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

Expand All @@ -47,6 +47,7 @@ import (
"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/logger"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/trustedcabundle"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/upgrade"
)
Expand All @@ -63,7 +64,6 @@ var managementStateChangeTrustedCA = false
type DSCInitializationReconciler struct {
client.Client
Scheme *runtime.Scheme
Log logr.Logger
Recorder record.EventRecorder
ApplicationsNamespace string
}
Expand All @@ -76,7 +76,7 @@ type DSCInitializationReconciler struct {

// Reconcile contains controller logic specific to DSCInitialization instance updates.
func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { //nolint:funlen,gocyclo,maintidx
log := r.Log
log := logf.FromContext(ctx)
log.Info("Reconciling DSCInitialization.", "DSCInitialization Request.Name", req.Name)

currentOperatorRelease, err := cluster.GetRelease(ctx, r.Client)
Expand Down Expand Up @@ -273,6 +273,7 @@ func (r *DSCInitializationReconciler) SetupWithManager(ctx context.Context, mgr
&dsciv1.DSCInitialization{},
builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}), dsciPredicateStateChangeTrustedCA),
).
WithLogConstructor(logger.NewLogConstructor("DSCInitialization")).
Owns(
&corev1.Namespace{},
builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}))).
Expand Down Expand Up @@ -365,8 +366,8 @@ var dsciPredicateStateChangeTrustedCA = predicate.Funcs{
},
}

func (r *DSCInitializationReconciler) watchMonitoringConfigMapResource(_ context.Context, a client.Object) []reconcile.Request {
log := r.Log
func (r *DSCInitializationReconciler) watchMonitoringConfigMapResource(ctx context.Context, a client.Object) []reconcile.Request {
log := logf.FromContext(ctx)
if a.GetName() == "prometheus" && a.GetNamespace() == "redhat-ods-monitoring" {
log.Info("Found monitoring configmap has updated, start reconcile")

Expand All @@ -375,8 +376,8 @@ func (r *DSCInitializationReconciler) watchMonitoringConfigMapResource(_ context
return nil
}

func (r *DSCInitializationReconciler) watchMonitoringSecretResource(_ context.Context, a client.Object) []reconcile.Request {
log := r.Log
func (r *DSCInitializationReconciler) watchMonitoringSecretResource(ctx context.Context, a client.Object) []reconcile.Request {
log := logf.FromContext(ctx)
operatorNs, err := cluster.GetOperatorNamespace()
if err != nil {
return nil
Expand All @@ -391,7 +392,7 @@ func (r *DSCInitializationReconciler) watchMonitoringSecretResource(_ context.Co
}

func (r *DSCInitializationReconciler) watchDSCResource(ctx context.Context) []reconcile.Request {
log := r.Log
log := logf.FromContext(ctx)
instanceList := &dscv1.DataScienceClusterList{}
if err := r.Client.List(ctx, instanceList); err != nil {
// do not handle if cannot get list
Expand Down
13 changes: 7 additions & 6 deletions controllers/dscinitialization/monitoring.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
logf "sigs.k8s.io/controller-runtime/pkg/log"

dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
Expand All @@ -39,7 +40,7 @@ var (
// only when reconcile on DSCI CR, initial set to true
// if reconcile from monitoring, initial set to false, skip blackbox and rolebinding.
func (r *DSCInitializationReconciler) configureManagedMonitoring(ctx context.Context, dscInit *dsciv1.DSCInitialization, initial string) error {
log := r.Log
log := logf.FromContext(ctx)
if initial == "init" {
// configure Blackbox exporter
if err := configureBlackboxExporter(ctx, dscInit, r); err != nil {
Expand Down Expand Up @@ -89,7 +90,7 @@ func (r *DSCInitializationReconciler) configureManagedMonitoring(ctx context.Con
}

func configureAlertManager(ctx context.Context, dsciInit *dsciv1.DSCInitialization, r *DSCInitializationReconciler) error {
log := r.Log
log := logf.FromContext(ctx)
// Get Deadmansnitch secret
deadmansnitchSecret, err := r.waitForManagedSecret(ctx, "redhat-rhods-deadmanssnitch", dsciInit.Spec.Monitoring.Namespace)
if err != nil {
Expand Down Expand Up @@ -200,7 +201,7 @@ func configureAlertManager(ctx context.Context, dsciInit *dsciv1.DSCInitializati
}

func configurePrometheus(ctx context.Context, dsciInit *dsciv1.DSCInitialization, r *DSCInitializationReconciler) error {
log := r.Log
log := logf.FromContext(ctx)
// Update rolebinding-viewer
err := common.ReplaceStringsInFile(filepath.Join(prometheusManifestsPath, "prometheus-rolebinding-viewer.yaml"),
map[string]string{
Expand Down Expand Up @@ -348,7 +349,7 @@ func configurePrometheus(ctx context.Context, dsciInit *dsciv1.DSCInitialization
}

func configureBlackboxExporter(ctx context.Context, dsciInit *dsciv1.DSCInitialization, r *DSCInitializationReconciler) error {
log := r.Log
log := logf.FromContext(ctx)
consoleRoute := &routev1.Route{}
err := r.Client.Get(ctx, client.ObjectKey{Name: "console", Namespace: "openshift-console"}, consoleRoute)
if err != nil {
Expand Down Expand Up @@ -438,7 +439,7 @@ func createMonitoringProxySecret(ctx context.Context, cli client.Client, name st
}

func (r *DSCInitializationReconciler) configureSegmentIO(ctx context.Context, dsciInit *dsciv1.DSCInitialization) error {
log := r.Log
log := logf.FromContext(ctx)
// create segment.io only when configmap does not exist in the cluster
segmentioConfigMap := &corev1.ConfigMap{}
if err := r.Client.Get(ctx, client.ObjectKey{
Expand Down Expand Up @@ -467,7 +468,7 @@ func (r *DSCInitializationReconciler) configureSegmentIO(ctx context.Context, ds
}

func (r *DSCInitializationReconciler) configureCommonMonitoring(ctx context.Context, dsciInit *dsciv1.DSCInitialization) error {
log := r.Log
log := logf.FromContext(ctx)
if err := r.configureSegmentIO(ctx, dsciInit); err != nil {
return err
}
Expand Down
5 changes: 3 additions & 2 deletions controllers/dscinitialization/servicemesh_setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
logf "sigs.k8s.io/controller-runtime/pkg/log"

dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
Expand All @@ -19,7 +20,7 @@ import (
)

func (r *DSCInitializationReconciler) configureServiceMesh(ctx context.Context, instance *dsciv1.DSCInitialization) error {
log := r.Log
log := logf.FromContext(ctx)
serviceMeshManagementState := operatorv1.Removed
if instance.Spec.ServiceMesh != nil {
serviceMeshManagementState = instance.Spec.ServiceMesh.ManagementState
Expand Down Expand Up @@ -62,7 +63,7 @@ func (r *DSCInitializationReconciler) configureServiceMesh(ctx context.Context,
}

func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, instance *dsciv1.DSCInitialization) error {
log := r.Log
log := logf.FromContext(ctx)
// on condition of Managed, do not handle Removed when set to Removed it trigger DSCI reconcile to clean up
if instance.Spec.ServiceMesh == nil {
return nil
Expand Down
6 changes: 4 additions & 2 deletions controllers/dscinitialization/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1"
dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
dscictrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/dscinitialization"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/logger"
"github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil"

. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -80,7 +81,9 @@ var _ = BeforeSuite(func() {
// can't use suite's context as the manager should survive the function
gCtx, gCancel = context.WithCancel(context.Background())

logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
l := zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))
logf.SetLogger(l)
logger.SetBaseLogger(l)

By("bootstrapping test environment")
rootPath, pathErr := envtestutil.FindProjectRoot()
Expand Down Expand Up @@ -137,7 +140,6 @@ var _ = BeforeSuite(func() {
err = (&dscictrl.DSCInitializationReconciler{
Client: k8sClient,
Scheme: testScheme,
Log: ctrl.Log.WithName("controllers").WithName("DSCInitialization"),
Recorder: mgr.GetEventRecorderFor("dscinitialization-controller"),
}).SetupWithManager(gCtx, mgr)

Expand Down
Loading

0 comments on commit a897dac

Please sign in to comment.