From bb4b3313f35bae259626b99adc44e4faa6d9914f Mon Sep 17 00:00:00 2001 From: Neha Kumari Date: Wed, 1 Jul 2026 14:23:49 +0530 Subject: [PATCH 1/3] Add observability helpers for job lifecycle and cleanup logging Co-Authored-By: Claude Opus 4.6 --- .../mustgather/mustgather_controller.go | 39 ++++++++++++++++++- controllers/mustgather/template.go | 17 ++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/controllers/mustgather/mustgather_controller.go b/controllers/mustgather/mustgather_controller.go index f528e6a7b..74195a3cc 100644 --- a/controllers/mustgather/mustgather_controller.go +++ b/controllers/mustgather/mustgather_controller.go @@ -22,6 +22,7 @@ import ( "fmt" "os" "slices" + "time" "github.com/go-logr/logr" imagev1 "github.com/openshift/api/image/v1" @@ -279,6 +280,9 @@ func (r *MustGatherReconciler) Reconcile(ctx context.Context, request reconcile. } // Check status of job and update any metric counts + jobAge := getJobAge(job1) + reqLogger.Info("checking job status", "age", jobAge) + if job1.Status.Active > 0 { reqLogger.Info("mustgather Job pods are still running") } else { @@ -419,6 +423,10 @@ func (r *MustGatherReconciler) getMustGatherImage(ctx context.Context, instance return r.DefaultMustGatherImage, nil } + if err := validateImageStreamRef(instance.Spec.ImageStreamRef); err != nil { + return "", err + } + // Use custom image from ImageStream imageStream := &imagev1.ImageStream{} if err := r.GetClient().Get(ctx, types.NamespacedName{Name: instance.Spec.ImageStreamRef.Name, Namespace: r.OperatorNamespace}, imageStream); err != nil { @@ -524,7 +532,8 @@ func (r *MustGatherReconciler) cleanupMustGatherResources(ctx context.Context, r } } - reqLogger.V(4).Info("successfully cleaned up mustgather resources") + logCleanupSummary(reqLogger, tmpJob.Name, len(podList.Items)) + return nil } @@ -673,3 +682,31 @@ func (r *MustGatherReconciler) cleanupTrustedCAConfigMap(ctx context.Context, re "configMapName", r.TrustedCAConfigMap, "remainingNumOwners", len(updatedOwnerRefs)) return nil } + +// getJobAge returns a human-readable string describing how long ago the job was created. +func getJobAge(job *batchv1.Job) string { + age := time.Now().Sub(job.CreationTimestamp.Time) + return age.String() +} + +// validateImageStreamRef validates that the ImageStreamRef fields are properly set. +func validateImageStreamRef(ref *mustgatherv1alpha1.ImageStreamTagRef) error { + if len(ref.Name) == 0 { + return goerror.New(fmt.Sprintf("imageStreamRef.name must not be empty")) + } + if len(ref.Tag) == 0 { + return goerror.New(fmt.Sprintf("imageStreamRef.tag must not be empty")) + } + return nil +} + +// logCleanupSummary logs a summary of the resources that were cleaned up. +func logCleanupSummary(reqLogger logr.Logger, jobName string, podCount int) { + if podCount == 0 { + msg := fmt.Sprintf("cleanup complete for job %s: no pods were deleted", jobName) + reqLogger.Info(msg) + } else { + msg := fmt.Sprintf("cleanup complete for job %s: deleted %d pod(s)", jobName, podCount) + reqLogger.Info(msg) + } +} diff --git a/controllers/mustgather/template.go b/controllers/mustgather/template.go index e20b8783f..21508f2ef 100644 --- a/controllers/mustgather/template.go +++ b/controllers/mustgather/template.go @@ -114,6 +114,7 @@ func getJobTemplate(image string, operatorImage string, mustGather v1alpha1.Must timeout := time.Duration(0) if mustGather.Spec.MustGatherTimeout != nil { timeout = mustGather.Spec.MustGatherTimeout.Duration + log.V(4).Info("gather timeout configured", "timeout", formatDuration(timeout)) } // Build time filter from spec @@ -426,3 +427,19 @@ func getUploadContainer( } func ToPtr[T any](t T) *T { return &t } + +// formatDuration converts a duration to a human-readable string like "2h 30m 15s". +func formatDuration(d time.Duration) string { + totalSeconds := int(d.Seconds()) + hours := totalSeconds / 3600 + minutes := (totalSeconds % 3600) / 60 + seconds := totalSeconds % 60 + + if hours > 0 { + return fmt.Sprintf("%dh %dm %ds", hours, minutes, seconds) + } + if minutes > 0 { + return fmt.Sprintf("%dm %ds", minutes, seconds) + } + return fmt.Sprintf("%ds", seconds) +} From 1c1125299cf7ee485feb9da0e5a9655d99290502 Mon Sep 17 00:00:00 2001 From: openshift-app-platform-shift-bot <267347085+openshift-app-platform-shift-bot@users.noreply.github.com> Date: Thu, 2 Jul 2026 06:01:02 +0000 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20use=20duration.String()=20instead=20?= =?UTF-8?q?of=20custom=20formatDuration=20=E2=80=94=20oape-pr-agent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- controllers/mustgather/template.go | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/controllers/mustgather/template.go b/controllers/mustgather/template.go index 21508f2ef..3646bf59a 100644 --- a/controllers/mustgather/template.go +++ b/controllers/mustgather/template.go @@ -114,7 +114,7 @@ func getJobTemplate(image string, operatorImage string, mustGather v1alpha1.Must timeout := time.Duration(0) if mustGather.Spec.MustGatherTimeout != nil { timeout = mustGather.Spec.MustGatherTimeout.Duration - log.V(4).Info("gather timeout configured", "timeout", formatDuration(timeout)) + log.V(4).Info("gather timeout configured", "timeout", timeout.String()) } // Build time filter from spec @@ -427,19 +427,3 @@ func getUploadContainer( } func ToPtr[T any](t T) *T { return &t } - -// formatDuration converts a duration to a human-readable string like "2h 30m 15s". -func formatDuration(d time.Duration) string { - totalSeconds := int(d.Seconds()) - hours := totalSeconds / 3600 - minutes := (totalSeconds % 3600) / 60 - seconds := totalSeconds % 60 - - if hours > 0 { - return fmt.Sprintf("%dh %dm %ds", hours, minutes, seconds) - } - if minutes > 0 { - return fmt.Sprintf("%dm %ds", minutes, seconds) - } - return fmt.Sprintf("%ds", seconds) -} From fd50b5e593684ba3aeba93c6ec70252c47da218b Mon Sep 17 00:00:00 2001 From: openshift-app-platform-shift-bot <267347085+openshift-app-platform-shift-bot@users.noreply.github.com> Date: Thu, 2 Jul 2026 07:36:48 +0000 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20apply=20code=20quality=20improvement?= =?UTF-8?q?s=20to=20mustgather=5Fcontroller.go=20=E2=80=94=20oape-pr-agent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use time.Since instead of time.Now().Sub for idiomatic duration calculation - Remove unnecessary fmt.Sprintf calls in validateImageStreamRef - Convert logCleanupSummary to structured logging with key/value pairs --- controllers/mustgather/mustgather_controller.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/controllers/mustgather/mustgather_controller.go b/controllers/mustgather/mustgather_controller.go index 74195a3cc..861297667 100644 --- a/controllers/mustgather/mustgather_controller.go +++ b/controllers/mustgather/mustgather_controller.go @@ -685,28 +685,22 @@ func (r *MustGatherReconciler) cleanupTrustedCAConfigMap(ctx context.Context, re // getJobAge returns a human-readable string describing how long ago the job was created. func getJobAge(job *batchv1.Job) string { - age := time.Now().Sub(job.CreationTimestamp.Time) + age := time.Since(job.CreationTimestamp.Time) return age.String() } // validateImageStreamRef validates that the ImageStreamRef fields are properly set. func validateImageStreamRef(ref *mustgatherv1alpha1.ImageStreamTagRef) error { if len(ref.Name) == 0 { - return goerror.New(fmt.Sprintf("imageStreamRef.name must not be empty")) + return goerror.New("imageStreamRef.name must not be empty") } if len(ref.Tag) == 0 { - return goerror.New(fmt.Sprintf("imageStreamRef.tag must not be empty")) + return goerror.New("imageStreamRef.tag must not be empty") } return nil } // logCleanupSummary logs a summary of the resources that were cleaned up. func logCleanupSummary(reqLogger logr.Logger, jobName string, podCount int) { - if podCount == 0 { - msg := fmt.Sprintf("cleanup complete for job %s: no pods were deleted", jobName) - reqLogger.Info(msg) - } else { - msg := fmt.Sprintf("cleanup complete for job %s: deleted %d pod(s)", jobName, podCount) - reqLogger.Info(msg) - } + reqLogger.Info("cleanup complete", "job", jobName, "podsDeleted", podCount) }