controllers: tweak events and logging

- Ensure all logged messages start with a lowercase.
- Make some pushed (and logged) events of type `EventTypeTrace` to
  prevent them from being sinked to the external event recorder, to
  prevent spam.
- Only log if artifact is up-to-date with upstream (instead of pushing
  an event).

Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit is contained in:
Hidde Beydals 2022-01-26 20:24:01 +01:00 committed by Sunny
parent 015e3fec93
commit d4db5a57dd
8 changed files with 56 additions and 48 deletions

View File

@ -28,6 +28,7 @@ import (
"time"
gcpstorage "cloud.google.com/go/storage"
"github.com/fluxcd/pkg/runtime/events"
"github.com/fluxcd/source-controller/pkg/gcp"
"github.com/minio/minio-go/v7"
"github.com/minio/minio-go/v7/pkg/credentials"
@ -134,7 +135,7 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
// Return early if the object is suspended
if obj.Spec.Suspend {
log.Info("Reconciliation is suspended for this object")
log.Info("reconciliation is suspended for this object")
return ctrl.Result{}, nil
}
@ -221,7 +222,7 @@ func (r *BucketReconciler) summarizeAndPatch(ctx context.Context, obj *sourcev1.
// error.
func (r *BucketReconciler) reconcile(ctx context.Context, obj *sourcev1.Bucket, reconcilers []bucketReconcilerFunc) (sreconcile.Result, error) {
if obj.Generation != obj.Status.ObservedGeneration {
conditions.MarkReconciling(obj, "NewGeneration", "Reconciling new generation %d", obj.Generation)
conditions.MarkReconciling(obj, "NewGeneration", "reconciling new generation %d", obj.Generation)
}
var artifact sourcev1.Artifact
@ -275,7 +276,7 @@ func (r *BucketReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.B
// Record that we do not have an artifact
if obj.GetArtifact() == nil {
conditions.MarkReconciling(obj, "NoArtifact", "No artifact for resource in storage")
conditions.MarkReconciling(obj, "NoArtifact", "no artifact for resource in storage")
return sreconcile.ResultSuccess, nil
}
@ -463,8 +464,8 @@ func (r *BucketReconciler) reconcileMinioSource(ctx context.Context, obj *source
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.BucketOperationFailedReason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
r.eventLogf(ctx, obj, corev1.EventTypeNormal, sourcev1.BucketOperationSucceedReason,
"downloaded %d files from bucket '%s' revision '%s'", len(index), obj.Spec.BucketName, revision)
r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.BucketOperationSucceedReason,
"downloaded %d files with revision '%s' from '%s'", len(index), revision, obj.Spec.BucketName)
}
conditions.Delete(obj, sourcev1.FetchFailedCondition)
@ -618,8 +619,8 @@ func (r *BucketReconciler) reconcileGCPSource(ctx context.Context, obj *sourcev1
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, sourcev1.BucketOperationFailedReason, e.Err.Error())
return sreconcile.ResultEmpty, e
}
r.eventLogf(ctx, obj, corev1.EventTypeNormal, sourcev1.BucketOperationSucceedReason,
"downloaded %d files from bucket '%s' revision '%s'", len(index), obj.Spec.BucketName, revision)
r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.BucketOperationSucceedReason,
"downloaded %d files from bucket '%s'", len(index), obj.Spec.BucketName)
}
conditions.Delete(obj, sourcev1.FetchFailedCondition)
@ -647,7 +648,7 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.
// The artifact is up-to-date
if obj.GetArtifact().HasRevision(artifact.Revision) {
r.eventLogf(ctx, obj, corev1.EventTypeNormal, meta.SucceededReason, "already up to date, current revision '%s'", artifact.Revision)
ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision)
return sreconcile.ResultSuccess, nil
}
@ -713,7 +714,6 @@ func (r *BucketReconciler) reconcileArtifact(ctx context.Context, obj *sourcev1.
// reconcileDelete handles the deletion of an object. It first garbage collects all artifacts for the object from the
// artifact storage, if successful, the finalizer is removed from the object.
// func (r *BucketReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.Bucket) (ctrl.Result, error) {
func (r *BucketReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.Bucket) (sreconcile.Result, error) {
// Garbage collect the resource's artifacts
if err := r.garbageCollect(ctx, obj); err != nil {
@ -741,7 +741,7 @@ func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Buc
}
obj.Status.Artifact = nil
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
"garbage collected artifacts for deleted resource")
return nil
}
@ -753,7 +753,7 @@ func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Buc
}
}
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", "garbage collected old artifacts")
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts")
}
return nil
}

View File

@ -201,7 +201,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
"!/reconcile-storage/invalid.txt",
},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "No artifact for resource in storage"),
*conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"),
},
},
{

View File

@ -41,9 +41,9 @@ import (
"github.com/fluxcd/pkg/apis/meta"
"github.com/fluxcd/pkg/runtime/conditions"
helper "github.com/fluxcd/pkg/runtime/controller"
"github.com/fluxcd/pkg/runtime/events"
"github.com/fluxcd/pkg/runtime/patch"
"github.com/fluxcd/pkg/runtime/predicates"
"github.com/fluxcd/source-controller/pkg/sourceignore"
sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
serror "github.com/fluxcd/source-controller/internal/error"
@ -51,6 +51,7 @@ import (
"github.com/fluxcd/source-controller/internal/util"
"github.com/fluxcd/source-controller/pkg/git"
"github.com/fluxcd/source-controller/pkg/git/strategy"
"github.com/fluxcd/source-controller/pkg/sourceignore"
)
// Status conditions owned by the GitRepository reconciler.
@ -139,7 +140,7 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques
// Return early if the object is suspended
if obj.Spec.Suspend {
log.Info("Reconciliation is suspended for this object")
log.Info("reconciliation is suspended for this object")
return ctrl.Result{}, nil
}
@ -308,7 +309,7 @@ func (r *GitRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sou
// If both the checkout and signature verification are successful, the given artifact pointer is set to a new artifact
// with the available metadata.
func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
obj *sourcev1.GitRepository, artifact *sourcev1.Artifact, includes *artifactSet, dir string) (sreconcile.Result, error) {
obj *sourcev1.GitRepository, artifact *sourcev1.Artifact, _ *artifactSet, dir string) (sreconcile.Result, error) {
// Configure authentication strategy to access the source
var authOpts *git.AuthOptions
var err error
@ -378,8 +379,8 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context,
// Coin flip on transient or persistent error, return error and hope for the best
return sreconcile.ResultEmpty, e
}
r.eventLogf(ctx, obj, corev1.EventTypeNormal, sourcev1.GitOperationSucceedReason,
"cloned repository '%s' and checked out revision '%s'", obj.Spec.URL, commit.String())
r.eventLogf(ctx, obj, events.EventTypeTrace, sourcev1.GitOperationSucceedReason,
"cloned '%s' and checked out revision '%s'", obj.Spec.URL, commit.String())
conditions.Delete(obj, sourcev1.FetchFailedCondition)
// Verify commit signature
@ -420,7 +421,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so
// The artifact is up-to-date
if obj.GetArtifact().HasRevision(artifact.Revision) && !includes.Diff(obj.Status.IncludedArtifacts) {
r.eventLogf(ctx, obj, corev1.EventTypeNormal, meta.SucceededReason, "already up to date, current revision '%s'", artifact.Revision)
ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision)
return sreconcile.ResultSuccess, nil
}
@ -492,7 +493,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so
url, err := r.Storage.Symlink(*artifact, "latest.tar.gz")
if err != nil {
r.eventLogf(ctx, obj, corev1.EventTypeWarning, sourcev1.StorageOperationFailedReason,
"Failed to update status URL symlink: %s", err)
"failed to update status URL symlink: %s", err)
}
if url != "" {
obj.Status.URL = url
@ -506,7 +507,7 @@ func (r *GitRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *so
// If an include is unavailable, it marks the object with v1beta1.IncludeUnavailableCondition and returns early.
// If the copy operations are successful, it deletes the v1beta1.IncludeUnavailableCondition from the object.
// If the artifactSet differs from the current set, it marks the object with v1beta1.ArtifactOutdatedCondition.
func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, obj *sourcev1.GitRepository, artifact *sourcev1.Artifact, includes *artifactSet, dir string) (sreconcile.Result, error) {
func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, obj *sourcev1.GitRepository, _ *sourcev1.Artifact, includes *artifactSet, dir string) (sreconcile.Result, error) {
artifacts := make(artifactSet, len(obj.Spec.Include))
for i, incl := range obj.Spec.Include {
// Do this first as it is much cheaper than copy operations
@ -544,7 +545,7 @@ func (r *GitRepositoryReconciler) reconcileInclude(ctx context.Context, obj *sou
// Copy artifact (sub)contents to configured directory
if err := r.Storage.CopyToPath(dep.GetArtifact(), incl.GetFromPath(), toPath); err != nil {
e := &serror.Event{
Err: fmt.Errorf("Failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err),
Err: fmt.Errorf("failed to copy '%s' include from %s to %s: %w", incl.GitRepositoryRef.Name, incl.GetFromPath(), incl.GetToPath(), err),
Reason: "CopyFailure",
}
conditions.MarkTrue(obj, sourcev1.IncludeUnavailableCondition, "CopyFailure", e.Err.Error())
@ -623,7 +624,7 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj
conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, meta.SucceededReason,
"verified signature of commit '%s'", commit.Hash.String())
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "VerifiedCommit",
r.eventLogf(ctx, obj, events.EventTypeTrace, "VerifiedCommit",
"verified signature of commit '%s'", commit.Hash.String())
return sreconcile.ResultSuccess, nil
}
@ -641,7 +642,7 @@ func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourc
}
obj.Status.Artifact = nil
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
"garbage collected artifacts for deleted resource")
return nil
}
@ -652,7 +653,7 @@ func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourc
}
}
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
"garbage collected old artifacts")
}
return nil

View File

@ -49,6 +49,7 @@ import (
"github.com/fluxcd/pkg/apis/meta"
"github.com/fluxcd/pkg/runtime/conditions"
helper "github.com/fluxcd/pkg/runtime/controller"
"github.com/fluxcd/pkg/runtime/events"
"github.com/fluxcd/pkg/runtime/patch"
"github.com/fluxcd/pkg/runtime/predicates"
"github.com/fluxcd/pkg/untar"
@ -357,7 +358,7 @@ func (r *HelmChartReconciler) reconcileSource(ctx context.Context, obj *sourcev1
// a sudden (partial) disappearance of observed state.
// TODO(hidde): include specific name/version information?
if depNum := build.ResolvedDependencies; build.Complete() && depNum > 0 {
r.Eventf(obj, corev1.EventTypeNormal, "ResolvedDependencies", "Resolved %d chart dependencies", depNum)
r.Eventf(obj, events.EventTypeTrace, "ResolvedDependencies", "resolved %d chart dependencies", depNum)
}
// Handle any build error
@ -637,7 +638,7 @@ func (r *HelmChartReconciler) reconcileArtifact(ctx context.Context, obj *source
// Return early if the build path equals the current artifact path
if curArtifact := obj.GetArtifact(); curArtifact != nil && r.Storage.LocalPath(*curArtifact) == b.Path {
r.eventLogf(ctx, obj, corev1.EventTypeNormal, meta.SucceededReason, "already up to date, current revision '%s'", curArtifact.Revision)
ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision)
return sreconcile.ResultSuccess, nil
}
@ -753,7 +754,7 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
}
obj.Status.Artifact = nil
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
"garbage collected artifacts for deleted resource")
return nil
}
@ -765,7 +766,7 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
}
}
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded", "garbage collected old artifacts")
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts")
}
return nil
}

View File

@ -232,7 +232,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
g.Expect(obj.Status.ObservedSourceArtifactRevision).To(Equal(gitArtifact.Revision))
g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "Pulled 'helmchart' chart with version '0.1.0'"),
*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled 'helmchart' chart with version '0.1.0'"),
}))
},
cleanFunc: func(g *WithT, build *chart.Build) {
@ -880,7 +880,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "Pulled 'helmchart' chart with version '0.1.0'"),
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"),
},
},
{
@ -923,7 +923,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
t.Expect(obj.Status.URL).To(BeEmpty())
},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPackageSucceededReason, "Packaged 'helmchart' chart with version '0.1.0'"),
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPackageSucceededReason, "packaged 'helmchart' chart with version '0.1.0'"),
},
},
{
@ -941,7 +941,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "Pulled 'helmchart' chart with version '0.1.0'"),
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"),
},
},
{
@ -958,7 +958,7 @@ func TestHelmChartReconciler_reconcileArtifact(t *testing.T) {
},
want: sreconcile.ResultSuccess,
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "Pulled 'helmchart' chart with version '0.1.0'"),
*conditions.TrueCondition(meta.ReadyCondition, sourcev1.ChartPullSucceededReason, "pulled 'helmchart' chart with version '0.1.0'"),
},
},
}

View File

@ -40,6 +40,7 @@ import (
"github.com/fluxcd/pkg/apis/meta"
"github.com/fluxcd/pkg/runtime/conditions"
helper "github.com/fluxcd/pkg/runtime/controller"
"github.com/fluxcd/pkg/runtime/events"
"github.com/fluxcd/pkg/runtime/patch"
"github.com/fluxcd/pkg/runtime/predicates"
@ -127,7 +128,7 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque
// Return early if the object is suspended
if obj.Spec.Suspend {
log.Info("Reconciliation is suspended for this object")
log.Info("reconciliation is suspended for this object")
return ctrl.Result{}, nil
}
@ -252,7 +253,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, obj *sourcev1.
//
// All artifacts for the resource except for the current one are garbage collected from the storage.
// If the artifact in the Status object of the resource disappeared from storage, it is removed from the object.
// If the hostname of any of the URLs on the object do not match the current storage server hostname, they are updated.
// If the hostname of the URLs on the object do not match the current storage server hostname, they are updated.
func (r *HelmRepositoryReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmRepository, artifact *sourcev1.Artifact, chartRepo *repository.ChartRepository) (sreconcile.Result, error) {
// Garbage collect previous advertised artifact(s) from storage
_ = r.garbageCollect(ctx, obj)
@ -413,7 +414,7 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s
}()
if obj.GetArtifact().HasRevision(artifact.Revision) {
r.eventLogf(ctx, obj, corev1.EventTypeNormal, meta.SucceededReason, "already up to date, current revision '%s'", artifact.Revision)
ctrl.LoggerFrom(ctx).Info("artifact up-to-date", "revision", artifact.Revision)
return sreconcile.ResultSuccess, nil
}
@ -450,6 +451,11 @@ func (r *HelmRepositoryReconciler) reconcileArtifact(ctx context.Context, obj *s
}
}
r.AnnotatedEventf(obj, map[string]string{
"revision": artifact.Revision,
"checksum": artifact.Checksum,
}, corev1.EventTypeNormal, "NewArtifact", "stored artifact for revision '%s'", artifact.Revision)
// Record it on the object.
obj.Status.Artifact = artifact.DeepCopy()
@ -495,7 +501,7 @@ func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sour
}
obj.Status.Artifact = nil
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
"garbage collected artifacts for deleted resource")
return nil
}
@ -507,7 +513,7 @@ func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sour
}
}
// TODO(hidde): we should only push this event if we actually garbage collected something
r.eventLogf(ctx, obj, corev1.EventTypeNormal, "GarbageCollectionSucceeded",
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
"garbage collected old artifacts")
}
return nil

View File

@ -140,16 +140,16 @@ type Build struct {
// Summary returns a human-readable summary of the Build.
func (b *Build) Summary() string {
if !b.HasMetadata() {
return "No chart build"
return "no chart build"
}
var s strings.Builder
var action = "New"
var action = "new"
if b.Path != "" {
action = "Pulled"
action = "pulled"
if b.Packaged {
action = "Packaged"
action = "packaged"
}
}
s.WriteString(fmt.Sprintf("%s '%s' chart with version '%s'", action, b.Name, b.Version))

View File

@ -143,7 +143,7 @@ func TestChartBuildResult_Summary(t *testing.T) {
Name: "chart",
Version: "1.2.3-rc.1+bd6bf40",
},
want: "New 'chart' chart with version '1.2.3-rc.1+bd6bf40'",
want: "new 'chart' chart with version '1.2.3-rc.1+bd6bf40'",
},
{
name: "Pulled chart",
@ -152,7 +152,7 @@ func TestChartBuildResult_Summary(t *testing.T) {
Version: "1.2.3-rc.1+bd6bf40",
Path: "chart.tgz",
},
want: "Pulled 'chart' chart with version '1.2.3-rc.1+bd6bf40'",
want: "pulled 'chart' chart with version '1.2.3-rc.1+bd6bf40'",
},
{
name: "Packaged chart",
@ -163,7 +163,7 @@ func TestChartBuildResult_Summary(t *testing.T) {
ValuesFiles: []string{"a.yaml", "b.yaml"},
Path: "chart.tgz",
},
want: "Packaged 'chart' chart with version 'arbitrary-version' and merged values files [a.yaml b.yaml]",
want: "packaged 'chart' chart with version 'arbitrary-version' and merged values files [a.yaml b.yaml]",
},
{
name: "With values files",
@ -174,17 +174,17 @@ func TestChartBuildResult_Summary(t *testing.T) {
ValuesFiles: []string{"a.yaml", "b.yaml"},
Path: "chart.tgz",
},
want: "Packaged 'chart' chart with version 'arbitrary-version' and merged values files [a.yaml b.yaml]",
want: "packaged 'chart' chart with version 'arbitrary-version' and merged values files [a.yaml b.yaml]",
},
{
name: "Empty build",
build: &Build{},
want: "No chart build",
want: "no chart build",
},
{
name: "Nil build",
build: nil,
want: "No chart build",
want: "no chart build",
},
}
for _, tt := range tests {