Skip to content

Commit

Permalink
Refactor logging for reconcilers (#447)
Browse files Browse the repository at this point in the history 
Log from context on all reconcilers
  • Loading branch information
@efiacor
efiacor authored Nov 28, 2023
1 parent 6068346 commit 9030f68
Show file tree
Hide file tree
Showing 11 changed files with 127 additions and 152 deletions.
9 changes: 3 additions & 6 deletions controllers/pkg/reconcilers/approval/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
"k8s.io/client-go/tools/record"

porchv1alpha1 "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
"github.com/go-logr/logr"
porchclient "github.com/nephio-project/nephio/controllers/pkg/porch/client"
porchconds "github.com/nephio-project/nephio/controllers/pkg/porch/condition"
porchutil "github.com/nephio-project/nephio/controllers/pkg/porch/util"
Expand Down Expand Up @@ -84,20 +83,18 @@ type reconciler struct {
porchClient client.Client
porchRESTClient rest.Interface
recorder record.EventRecorder

l logr.Logger
}

func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
r.l = log.FromContext(ctx).WithValues("req", req)
r.l.Info("reconcile approval")
log := log.FromContext(ctx).WithValues("req", req)
log.Info("reconcile approval")

pr := &porchv1alpha1.PackageRevision{}
if err := r.Get(ctx, req.NamespacedName, pr); err != nil {
// There's no need to requeue if we no longer exist. Otherwise we'll be
// requeued implicitly because we return an error.
if resource.IgnoreNotFound(err) != nil {
r.l.Error(err, "cannot get resource")
log.Error(err, "cannot get resource")
return ctrl.Result{}, errors.Wrap(resource.IgnoreNotFound(err), "cannot get resource")
}
return ctrl.Result{}, nil
Expand Down
39 changes: 18 additions & 21 deletions controllers/pkg/reconcilers/bootstrap-packages/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (

porchv1alpha1 "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
porchconfigv1alpha1 "github.com/GoogleContainerTools/kpt/porch/api/porchconfig/v1alpha1"
"github.com/go-logr/logr"
"github.com/nephio-project/nephio/controllers/pkg/cluster"
ctrlconfig "github.com/nephio-project/nephio/controllers/pkg/reconcilers/config"
reconcilerinterface "github.com/nephio-project/nephio/controllers/pkg/reconcilers/reconciler-interface"
Expand Down Expand Up @@ -89,19 +88,17 @@ func (r *reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, c a
type reconciler struct {
client.Client
porchClient client.Client

l logr.Logger
}

func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
r.l = log.FromContext(ctx)
log := log.FromContext(ctx)
cr := &porchv1alpha1.PackageRevision{}
if err := r.Get(ctx, req.NamespacedName, cr); err != nil {
// There's no need to requeue if we no longer exist. Otherwise we'll be
// requeued implicitly because we return an error.
if resource.IgnoreNotFound(err) != nil {
msg := "cannot get resource"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(resource.IgnoreNotFound(err), msg)
}
return ctrl.Result{}, nil
Expand All @@ -112,15 +109,15 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
stagingPR, err := r.IsStagingPackageRevision(ctx, cr)
if err != nil {
msg := "cannot list repositories"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(err, msg)
}
if stagingPR && porchv1alpha1.LifecycleIsPublished(cr.Spec.Lifecycle) {
r.l.Info("reconcile package revision")
log.Info("reconcile package revision")
resources, namespacePresent, err := r.getResources(ctx, req)
if err != nil {
msg := "cannot get resources"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(err, msg)
}
// we expect the clusterName to be applied to all resources in the
Expand All @@ -129,7 +126,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
if len(resources) > 0 {
clusterName, ok := resources[0].GetAnnotations()[clusterNameKey]
if !ok {
r.l.Info("clusterName not found",
log.Info("clusterName not found",
"resource", fmt.Sprintf("%s.%s.%s", resources[0].GetAPIVersion(), resources[0].GetKind(), resources[0].GetName()),
"annotations", resources[0].GetAnnotations())
return ctrl.Result{}, nil
Expand All @@ -138,7 +135,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
secrets := &corev1.SecretList{}
if err := r.List(ctx, secrets); err != nil {
msg := "cannot list secrets"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(err, msg)
}
found := false
Expand All @@ -151,34 +148,34 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
clusterClient, ready, err := clusterClient.GetClusterClient(ctx)
if err != nil {
msg := "cannot get clusterClient"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{RequeueAfter: 30 * time.Second}, errors.Wrap(err, msg)
}
if !ready {
r.l.Info("cluster not ready")
log.Info("cluster not ready")
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
if !namespacePresent {
ns := &corev1.Namespace{}
if err = clusterClient.Get(ctx, types.NamespacedName{Name: configsyncNamespace}, ns); err != nil {
if resource.IgnoreNotFound(err) != nil {
msg := fmt.Sprintf("cannot get namespace: %s", configsyncNamespace)
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{RequeueAfter: 30 * time.Second}, errors.Wrap(err, msg)
}
msg := fmt.Sprintf("namespace: %s, does not exist, retry...", configsyncNamespace)
r.l.Info(msg)
log.Info(msg)
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
}
// install resources
for _, resource := range resources {
resource := resource // required to prevent gosec warning: G601 (CWE-118): Implicit memory aliasing in for loop
r.l.Info("install manifest", "resource",
log.Info("install manifest", "resource",
fmt.Sprintf("%s.%s.%s", resource.GetAPIVersion(), resource.GetKind(), resource.GetName()))
if err := clusterClient.Apply(ctx, &resource); err != nil {
msg := fmt.Sprintf("cannot apply resource to cluster: resourceName: %s", resource.GetName())
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(err, msg)
}
}
Expand All @@ -187,7 +184,7 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
}
if !found {
// the cluster client was not found, we retry
r.l.Info("cluster client not found, retry...")
log.Info("cluster client not found, retry...")
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
}
Expand Down Expand Up @@ -219,11 +216,11 @@ func (r *reconciler) IsStagingPackageRevision(ctx context.Context, cr *porchv1al
func (r *reconciler) getResources(ctx context.Context, req ctrl.Request) ([]unstructured.Unstructured, bool, error) {
prr := &porchv1alpha1.PackageRevisionResources{}
if err := r.porchClient.Get(ctx, req.NamespacedName, prr); err != nil {
r.l.Error(err, "cannot get package revision resourcelist", "key", req.NamespacedName)
log.FromContext(ctx).Error(err, "cannot get package revision resourcelist", "key", req.NamespacedName)
return nil, false, err
}

return r.getResourcesPRR(prr.Spec.Resources)
return r.getResourcesPRR(ctx, prr.Spec.Resources)
}

func includeFile(path string, match []string) bool {
Expand All @@ -236,7 +233,7 @@ func includeFile(path string, match []string) bool {
return false
}

func (r *reconciler) getResourcesPRR(resources map[string]string) ([]unstructured.Unstructured, bool, error) {
func (r *reconciler) getResourcesPRR(ctx context.Context, resources map[string]string) ([]unstructured.Unstructured, bool, error) {
inputs := []kio.Reader{}
for path, data := range resources {
if includeFile(path, []string{"*.yaml", "*.yml", "Kptfile"}) {
Expand Down Expand Up @@ -267,7 +264,7 @@ func (r *reconciler) getResourcesPRR(resources map[string]string) ([]unstructure
}
u := unstructured.Unstructured{}
if err := yaml.Unmarshal([]byte(n.MustString()), &u); err != nil {
r.l.Error(err, "cannot unmarshal data", "data", n.MustString())
log.FromContext(ctx).Error(err, "cannot unmarshal data", "data", n.MustString())
// we don't fail
continue
}
Expand Down
3 changes: 2 additions & 1 deletion controllers/pkg/reconcilers/bootstrap-packages/reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package bootstrappackages

import (
"context"
"fmt"
"testing"

Expand Down Expand Up @@ -75,7 +76,7 @@ spec:
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
r := reconciler{}
us, _, err := r.getResourcesPRR(tc.resources)
us, _, err := r.getResourcesPRR(context.Background(), tc.resources)

if tc.expectedErr {
assert.Error(t, err)
Expand Down
25 changes: 11 additions & 14 deletions controllers/pkg/reconcilers/bootstrap-secret/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"strings"
"time"

"github.com/go-logr/logr"
"github.com/nephio-project/nephio/controllers/pkg/cluster"
reconcilerinterface "github.com/nephio-project/nephio/controllers/pkg/reconcilers/reconciler-interface"
"github.com/nephio-project/nephio/controllers/pkg/resource"
Expand Down Expand Up @@ -66,19 +65,17 @@ func (r *reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, c a

type reconciler struct {
client.Client

l logr.Logger
}

func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
r.l = log.FromContext(ctx)
log := log.FromContext(ctx)

cr := &corev1.Secret{}
if err := r.Get(ctx, req.NamespacedName, cr); err != nil {
// if the resource no longer exists the reconcile loop is done
if resource.IgnoreNotFound(err) != nil {
msg := "cannot get resource"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(resource.IgnoreNotFound(err), msg)
}
return reconcile.Result{}, nil
Expand All @@ -96,13 +93,13 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
if cr.GetAnnotations()[nephioAppKey] == configsyncApp &&
cr.GetAnnotations()[clusterNameKey] != "" &&
cr.GetAnnotations()[clusterNameKey] != "mgmt" {
r.l.Info("reconcile secret")
log.Info("reconcile secret")
clusterName := cr.GetAnnotations()[clusterNameKey]

secrets := &corev1.SecretList{}
if err := r.List(ctx, secrets); err != nil {
msg := "cannot list secrets"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(err, msg)
}
found := false
Expand All @@ -115,23 +112,23 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
clusterClient, ready, err := clusterClient.GetClusterClient(ctx)
if err != nil {
msg := "cannot get clusterClient"
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{RequeueAfter: 30 * time.Second}, errors.Wrap(err, msg)
}
if !ready {
r.l.Info("cluster not ready")
log.Info("cluster not ready")
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
// check if namespace exists, if not retry
ns := &corev1.Namespace{}
if err = clusterClient.Get(ctx, types.NamespacedName{Name: configsyncNamespace}, ns); err != nil {
if resource.IgnoreNotFound(err) != nil {
msg := fmt.Sprintf("cannot get namespace: %s", configsyncNamespace)
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{RequeueAfter: 30 * time.Second}, errors.Wrap(err, msg)
}
msg := fmt.Sprintf("namespace: %s, does not exist, retry...", configsyncNamespace)
r.l.Info(msg)
log.Info(msg)
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}

Expand All @@ -145,18 +142,18 @@ func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
})
newcr.ResourceVersion = ""
newcr.UID = ""
r.l.Info("secret info", "secret", newcr.Annotations)
log.Info("secret info", "secret", newcr.Annotations)
if err := clusterClient.Apply(ctx, newcr); err != nil {
msg := fmt.Sprintf("cannot apply secret to cluster %s", clusterName)
r.l.Error(err, msg)
log.Error(err, msg)
return ctrl.Result{}, errors.Wrap(err, msg)
}
}
}
}
if !found {
// the cluster client was not found, we retry
r.l.Info("cluster client not found, retry...")
log.Info("cluster client not found, retry...")
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
}
}
Expand Down
Loading

0 comments on commit 9030f68

Please sign in to comment.