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

Jump to bottom

Reintroduce missing events for helmChart reconciliation failures #907

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Reintroduce missing events for helmChart reconciliation failures #907

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
3 changes: 3 additions & 0 deletions internal/controller/helmrelease_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
conditions.MarkStalled(obj, aclv1.AccessDeniedReason, err.Error())
conditions.MarkFalse(obj, meta.ReadyCondition, aclv1.AccessDeniedReason, err.Error())
conditions.Delete(obj, meta.ReconcilingCondition)
r.Eventf(obj, eventv1.EventSeverityError, aclv1.AccessDeniedReason, err.Error())
Copy link
Contributor

@darkowlzz darkowlzz Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The event type seems incorrect for our event recorder. Our event recorder implements the kubernetes EventRecorder interface which only supports Normal and Warning events. In our implementation, we also accept Trace type and convert them to Normal type, refer https://github.com/fluxcd/pkg/blob/0cf0546cb4ded7301cf3f8476c2d868c8d27a187/runtime/events/recorder.go#L225 .
eventv1.EventSeverityError seems to be for notification-controller event severity.

Because of how our event recorder is written, it doesn't result in any error/failure but the argument value seems incorrect and this is also done similarly in line 240 above. But everywhere else in this repository, proper event type is passed to event recorder.

Copy link
Member Author

Choose a reason for hiding this comment

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

the way this was fixed in helm-controller was with this function

func (r *HelmReleaseReconciler) event(_ context.Context, hr v2.HelmRelease, revision, severity, msg string) {
...
  eventType := corev1.EventTypeNormal
  if severity == eventv1.EventSeverityError {
    eventType = corev1.EventTypeWarning
  }
  r.EventRecorder.AnnotatedEventf(&hr, eventMeta, eventType, severity, msg)
}

and then in fluxcd/pkg/runtime/events/recorder.go we have this function:

func eventTypeToSeverity(eventType string) string {
	switch eventType {
	case corev1.EventTypeWarning:
		return eventv1.EventSeverityError
	case eventv1.EventTypeTrace:
		return eventv1.EventSeverityTrace
	default:
		return eventv1.EventSeverityInfo
	}
}

This code makes sure that you always pass warning or normal to the recorder that can then re-switch to the right severity for NC.

For Trace events you can pass them as-is to the recorder that expects it, and do not send them to NC:

	// Do not send trace events to notification controller,
	// traces are persisted as Kubernetes events only as normal events.
	if severity == eventv1.EventSeverityTrace {
		r.EventRecorder.AnnotatedEventf(object, annotations, corev1.EventTypeNormal, reason, messageFmt, args...)
		return
	}

I think we can make sure that the recorder infer the right severity and final type based on the passed event type directly in fluxcd/pkg/runtime/events/recorder.go. I'll give it a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the severity is for NC events to filter the notifications based on severity. The event type that the event recorder accepts is the upstream core types, Normal and Warning. But since we needed a way to only send kubernetes events and skip NC events, we introduced Trace type as well. So the event recorder events are different from NC event types with different purposes.


// Recovering from this is not possible without a restart of the
// controller or a change of spec, both triggering a new
Expand Down Expand Up @@ -293,6 +294,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
values, err := chartutil.ChartValuesFromReferences(ctx, r.Client, obj.Namespace, obj.GetValues(), obj.Spec.ValuesFrom...)
if err != nil {
conditions.MarkFalse(obj, meta.ReadyCondition, "ValuesError", err.Error())
r.Eventf(obj, eventv1.EventSeverityError, "ValuesError", err.Error())
return ctrl.Result{}, err
}
// Remove any stale corresponding Ready=False condition with Unknown.
Expand All @@ -311,6 +313,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
}

conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, fmt.Sprintf("Could not load chart: %s", err.Error()))
r.Eventf(obj, eventv1.EventSeverityError, v2.ArtifactFailedReason, err.Error())
return ctrl.Result{}, err
}
// Remove any stale corresponding Ready=False condition with Unknown.
Expand Down
2 changes: 2 additions & 0 deletions internal/controller/helmrelease_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
WithStatusSubresource(&v2.HelmRelease{}).
WithObjects(obj).
Build(),
EventRecorder: record.NewFakeRecorder(32),
}

res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
Expand Down Expand Up @@ -383,6 +384,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
WithStatusSubresource(&v2.HelmRelease{}).
WithObjects(chart, obj).
Build(),
EventRecorder: record.NewFakeRecorder(32),
}

_, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
Expand Down
Loading