Reintroduce missing events for helmChart reconciliation failures

[souleb]

fixes 4649

If implemented this PR reintroduces events for some failing actions during the reconciliation process, related to the chart retrieval and loading of chart and values.

We do not reintroduce all events (still available in logs), but where it is desirable to have notifications in place.

[souleb]

Testing with a faulty configmap gives us:

{"level":"info","ts":"2024-03-06T11:09:39.678Z","logger":"event-server","msg":"dispatching event","eventInvolvedObject":{"kind":"HelmRelease","namespace":"backend","name":"redis","uid":"50f43354-d991-4149-bd7f-31074eed9a83","apiVersion":"helm.toolkit.fluxcd.io/v2beta2","resourceVersion":"625589"},"message":"could not resolve ConfigMap chart values reference 'backend/application-values-g9g5b268dh' with key 'values.yaml': key not found"}

Status:
  Conditions:
    Last Transition Time:  2024-03-06T11:10:17Z
    Message:               Fulfilling prerequisites
    Observed Generation:   8
    Reason:                Progressing
    Status:                True
    Type:                  Reconciling
    Last Transition Time:  2024-03-06T11:10:17Z
    Message:               could not resolve ConfigMap chart values reference 'backend/application-values-g9g5b268dh' with key 'values.yaml': key not found
    Observed Generation:   8
    Reason:                ValuesError
    Status:                False
    Type:                  Ready

[stefanprodan]

LGTM

Thanks @souleb 🥇

[darkowlzz]

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.

[souleb]

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.

[darkowlzz]

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.