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

switch to structured logging #258

Merged
merged 1 commit into from
Jan 4, 2024
+203 −143
Merged

switch to structured logging #258

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
83 changes: 50 additions & 33 deletions controllers/cinder_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,6 @@ func (r *CinderReconciler) GetKClient() kubernetes.Interface {
return r.Kclient
}

// GetLogger -
func (r *CinderReconciler) GetLogger() logr.Logger {
return r.Log
}

// GetScheme -
func (r *CinderReconciler) GetScheme() *runtime.Scheme {
return r.Scheme
Expand All @@ -84,10 +79,14 @@ func (r *CinderReconciler) GetScheme() *runtime.Scheme {
type CinderReconciler struct {
client.Client
Kclient kubernetes.Interface
Log logr.Logger
Scheme *runtime.Scheme
}

// GetLogger returns a logger object with a logging prefix of "controller.name" and additional controller context fields
func (r *CinderReconciler) GetLogger(ctx context.Context) logr.Logger {
return log.FromContext(ctx).WithName("Controllers").WithName("Cinder")
}

// +kubebuilder:rbac:groups=cinder.openstack.org,resources=cinders,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=cinder.openstack.org,resources=cinders/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=cinder.openstack.org,resources=cinders/finalizers,verbs=update
Expand Down Expand Up @@ -122,7 +121,7 @@ type CinderReconciler struct {

// Reconcile -
func (r *CinderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, _err error) {
_ = log.FromContext(ctx)
Log := r.GetLogger(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I still wish it was possible to pass the logger function down the call stack in order to minimize the number of places that have to call this function. But maybe I just need to get used to a new (to me) convention.


// Fetch the Cinder instance
instance := &cinderv1beta1.Cinder{}
Expand All @@ -143,7 +142,7 @@ func (r *CinderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
r.Client,
r.Kclient,
r.Scheme,
r.Log,
Log,
Copy link
Contributor

Choose a reason for hiding this comment

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

@gibizer I recall you saying you'd like to reduce (eliminate?) our use of helpers. With this PR, the Log element in the helper is no longer used by the controller, but we have to supply one in order create the helper. It looks like we have an opportunity to remove the Log from the helper (since nobody will use it), but I don't know how to coordinate doing that across all the repos that use the current helper. Any thoughts on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we could have had a PR to remove the parameter in lib-common, and then bump its pinning here in this same PR so we could remove the passing of the parameter in one go.

With the current approach we need to:

  • Merge this PR
  • Remove the code from lib-common that requires the logger argument
  • Update the pinning in all the operators at the same time we remove the passing of the argument.

Our current approach seems more painful, but for the shake of getting things done and keep moving we can merge it.

)
if err != nil {
return ctrl.Result{}, err
Expand Down Expand Up @@ -218,7 +217,7 @@ func (r *CinderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
}

// SetupWithManager sets up the controller with the Manager.
func (r *CinderReconciler) SetupWithManager(mgr ctrl.Manager) error {
func (r *CinderReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
// transportURLSecretFn - Watch for changes made to the secret associated with the RabbitMQ
// TransportURL created and used by Cinder CRs. Watch functions return a list of namespace-scoped
// CRs that then get fed to the reconciler. Hence, in this case, we need to know the name of the
Expand All @@ -233,13 +232,15 @@ func (r *CinderReconciler) SetupWithManager(mgr ctrl.Manager) error {
transportURLSecretFn := func(o client.Object) []reconcile.Request {
result := []reconcile.Request{}

Log := r.GetLogger(ctx)

// get all Cinder CRs
cinders := &cinderv1beta1.CinderList{}
listOpts := []client.ListOption{
client.InNamespace(o.GetNamespace()),
}
if err := r.Client.List(context.Background(), cinders, listOpts...); err != nil {
r.Log.Error(err, "Unable to retrieve Cinder CRs %v")
if err := r.Client.List(ctx, cinders, listOpts...); err != nil {
Log.Error(err, "Unable to retrieve Cinder CRs %v")
return nil
}

Expand All @@ -252,7 +253,7 @@ func (r *CinderReconciler) SetupWithManager(mgr ctrl.Manager) error {
Namespace: o.GetNamespace(),
Name: cr.Name,
}
r.Log.Info(fmt.Sprintf("TransportURL Secret %s belongs to TransportURL belonging to Cinder CR %s", o.GetName(), cr.Name))
Log.Info(fmt.Sprintf("TransportURL Secret %s belongs to TransportURL belonging to Cinder CR %s", o.GetName(), cr.Name))
result = append(result, reconcile.Request{NamespacedName: name})
}
}
Expand All @@ -265,6 +266,8 @@ func (r *CinderReconciler) SetupWithManager(mgr ctrl.Manager) error {
}

memcachedFn := func(o client.Object) []reconcile.Request {
Log := r.GetLogger(ctx)

result := []reconcile.Request{}

// get all Cinder CRs
Expand All @@ -273,7 +276,7 @@ func (r *CinderReconciler) SetupWithManager(mgr ctrl.Manager) error {
client.InNamespace(o.GetNamespace()),
}
if err := r.Client.List(context.Background(), cinders, listOpts...); err != nil {
r.Log.Error(err, "Unable to retrieve Cinder CRs %w")
Log.Error(err, "Unable to retrieve Cinder CRs %w")
return nil
}

Expand All @@ -283,7 +286,7 @@ func (r *CinderReconciler) SetupWithManager(mgr ctrl.Manager) error {
Namespace: o.GetNamespace(),
Name: cr.Name,
}
r.Log.Info(fmt.Sprintf("Memcached %s is used by Cinder CR %s", o.GetName(), cr.Name))
Log.Info(fmt.Sprintf("Memcached %s is used by Cinder CR %s", o.GetName(), cr.Name))
result = append(result, reconcile.Request{NamespacedName: name})
}
}
Expand Down Expand Up @@ -316,7 +319,9 @@ func (r *CinderReconciler) SetupWithManager(mgr ctrl.Manager) error {
}

func (r *CinderReconciler) reconcileDelete(ctx context.Context, instance *cinderv1beta1.Cinder, helper *helper.Helper) (ctrl.Result, error) {
r.Log.Info(fmt.Sprintf("Reconciling Service '%s' delete", instance.Name))
Log := r.GetLogger(ctx)

Log.Info(fmt.Sprintf("Reconciling Service '%s' delete", instance.Name))

// remove db finalizer first
db, err := mariadbv1.GetDatabaseByName(ctx, helper, instance.Name)
Expand All @@ -335,7 +340,7 @@ func (r *CinderReconciler) reconcileDelete(ctx context.Context, instance *cinder

// Service is deleted so remove the finalizer.
controllerutil.RemoveFinalizer(instance, helper.GetFinalizer())
r.Log.Info(fmt.Sprintf("Reconciled Service '%s' delete successfully", instance.Name))
Log.Info(fmt.Sprintf("Reconciled Service '%s' delete successfully", instance.Name))

return ctrl.Result{}, nil
}
Expand All @@ -347,7 +352,9 @@ func (r *CinderReconciler) reconcileInit(
serviceLabels map[string]string,
serviceAnnotations map[string]string,
) (ctrl.Result, error) {
r.Log.Info(fmt.Sprintf("Reconciling Service '%s' init", instance.Name))
Log := r.GetLogger(ctx)

Log.Info(fmt.Sprintf("Reconciling Service '%s' init", instance.Name))

//
// create service DB instance
Expand Down Expand Up @@ -442,7 +449,7 @@ func (r *CinderReconciler) reconcileInit(
}
if dbSyncjob.HasChanged() {
instance.Status.Hash[cinderv1beta1.DbSyncHash] = dbSyncjob.GetHash()
r.Log.Info(fmt.Sprintf("Service '%s' - Job %s hash added - %s", instance.Name, jobDef.Name, instance.Status.Hash[cinderv1beta1.DbSyncHash]))
Log.Info(fmt.Sprintf("Service '%s' - Job %s hash added - %s", instance.Name, jobDef.Name, instance.Status.Hash[cinderv1beta1.DbSyncHash]))
}
instance.Status.Conditions.MarkTrue(condition.DBSyncReadyCondition, condition.DBSyncReadyMessage)

Expand All @@ -451,12 +458,14 @@ func (r *CinderReconciler) reconcileInit(

// run Cinder db sync - end

r.Log.Info(fmt.Sprintf("Reconciled Service '%s' init successfully", instance.Name))
Log.Info(fmt.Sprintf("Reconciled Service '%s' init successfully", instance.Name))
return ctrl.Result{}, nil
}

func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinderv1beta1.Cinder, helper *helper.Helper) (ctrl.Result, error) {
r.Log.Info(fmt.Sprintf("Reconciling Service '%s'", instance.Name))
Log := r.GetLogger(ctx)

Log.Info(fmt.Sprintf("Reconciling Service '%s'", instance.Name))

// Service account, role, binding
rbacRules := []rbacv1.PolicyRule{
Expand Down Expand Up @@ -502,13 +511,13 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder
}

if op != controllerutil.OperationResultNone {
r.Log.Info(fmt.Sprintf("TransportURL %s successfully reconciled - operation: %s", transportURL.Name, string(op)))
Log.Info(fmt.Sprintf("TransportURL %s successfully reconciled - operation: %s", transportURL.Name, string(op)))
}

instance.Status.TransportURLSecret = transportURL.Status.SecretName

if instance.Status.TransportURLSecret == "" {
r.Log.Info(fmt.Sprintf("Waiting for TransportURL %s secret to be created", transportURL.Name))
Log.Info(fmt.Sprintf("Waiting for TransportURL %s secret to be created", transportURL.Name))
instance.Status.Conditions.Set(condition.FalseCondition(
condition.RabbitMqTransportURLReadyCondition,
condition.RequestedReason,
Expand Down Expand Up @@ -691,7 +700,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder
return ctrl.Result{}, err
}
if op != controllerutil.OperationResultNone {
r.Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
}

// Mirror CinderAPI status' APIEndpoints and ReadyCount to this parent CR
Expand All @@ -717,7 +726,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder
return ctrl.Result{}, err
}
if op != controllerutil.OperationResultNone {
r.Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
}

// Mirror CinderScheduler status' ReadyCount to this parent CR
Expand Down Expand Up @@ -746,7 +755,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder
return ctrl.Result{}, err
}
if op != controllerutil.OperationResultNone {
r.Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
}

// Mirror CinderBackup status' ReadyCount to this parent CR
Expand Down Expand Up @@ -787,7 +796,7 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder
return ctrl.Result{}, err
}
if op != controllerutil.OperationResultNone {
r.Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
Log.Info(fmt.Sprintf("Deployment %s successfully reconciled - operation: %s", instance.Name, string(op)))
}

// Mirror CinderVolume status' ReadyCount to this parent CR
Expand Down Expand Up @@ -843,27 +852,31 @@ func (r *CinderReconciler) reconcileNormal(ctx context.Context, instance *cinder
instance.Status.Conditions.MarkTrue(condition.CronJobReadyCondition, condition.CronJobReadyMessage)
// create CronJob - end

r.Log.Info(fmt.Sprintf("Reconciled Service '%s' successfully", instance.Name))
Log.Info(fmt.Sprintf("Reconciled Service '%s' successfully", instance.Name))
return ctrl.Result{}, nil
}

func (r *CinderReconciler) reconcileUpdate(ctx context.Context, instance *cinderv1beta1.Cinder, helper *helper.Helper) (ctrl.Result, error) {
r.Log.Info(fmt.Sprintf("Reconciling Service '%s' update", instance.Name))
Log := r.GetLogger(ctx)

Log.Info(fmt.Sprintf("Reconciling Service '%s' update", instance.Name))

// TODO: should have minor update tasks if required
// - delete dbsync hash from status to rerun it?

r.Log.Info(fmt.Sprintf("Reconciled Service '%s' update successfully", instance.Name))
Log.Info(fmt.Sprintf("Reconciled Service '%s' update successfully", instance.Name))
return ctrl.Result{}, nil
}

func (r *CinderReconciler) reconcileUpgrade(ctx context.Context, instance *cinderv1beta1.Cinder, helper *helper.Helper) (ctrl.Result, error) {
r.Log.Info(fmt.Sprintf("Reconciling Service '%s' upgrade", instance.Name))
Log := r.GetLogger(ctx)

Log.Info(fmt.Sprintf("Reconciling Service '%s' upgrade", instance.Name))

// TODO: should have major version upgrade tasks
// -delete dbsync hash from status to rerun it?

r.Log.Info(fmt.Sprintf("Reconciled Service '%s' upgrade successfully", instance.Name))
Log.Info(fmt.Sprintf("Reconciled Service '%s' upgrade successfully", instance.Name))
return ctrl.Result{}, nil
}

Expand Down Expand Up @@ -954,6 +967,8 @@ func (r *CinderReconciler) createHashOfInputHashes(
instance *cinderv1beta1.Cinder,
envVars map[string]env.Setter,
) (string, bool, error) {
Log := r.GetLogger(ctx)

var hashMap map[string]string
changed := false
mergedMapVars := env.MergeEnvs([]corev1.EnvVar{}, envVars)
Expand All @@ -963,7 +978,7 @@ func (r *CinderReconciler) createHashOfInputHashes(
}
if hashMap, changed = util.SetHash(instance.Status.Hash, common.InputHashName, hash); changed {
instance.Status.Hash = hashMap
r.Log.Info(fmt.Sprintf("Input maps hash %s - %s", common.InputHashName, hash))
Log.Info(fmt.Sprintf("Input maps hash %s - %s", common.InputHashName, hash))
}
return hash, changed, nil
}
Expand Down Expand Up @@ -1184,13 +1199,15 @@ func (r *CinderReconciler) volumeDeploymentCreateOrUpdate(ctx context.Context, i
// longer appears in the spec. These will be volumes named something like
// "cinder-volume-X" where "X" is not in the CinderVolumes spec.
func (r *CinderReconciler) volumeCleanupDeployments(ctx context.Context, instance *cinderv1beta1.Cinder) error {
Log := r.GetLogger(ctx)

// Generate a list of volume CRs
volumes := &cinderv1beta1.CinderVolumeList{}
listOpts := []client.ListOption{
client.InNamespace(instance.Namespace),
}
if err := r.Client.List(ctx, volumes, listOpts...); err != nil {
r.Log.Error(err, "Unable to retrieve volume CRs %v")
Log.Error(err, "Unable to retrieve volume CRs %v")
return nil
}

Expand Down
Loading