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.

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

[RFC] Flux-CDEvent Provider #4828

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

adamkenihan
Copy link
Contributor

This RFC proposes to add a provider that supports sending CDEvents relevant to the Flux events that trigger the alert. The initial focus of this RFC will be on implementing Helm Events.

Signed-off-by: adamkenihan <[email protected]>
Signed-off-by: adamkenihan <[email protected]>
rfcs/NNNN-cdevents-provider/README.md Outdated Show resolved Hide resolved
rfcs/NNNN-cdevents-provider/README.md Outdated Show resolved Hide resolved

## Proposal

Add CDEvents to the list of available `Providers` in Flux Notification controller. The user should be able to create mappings within the configuration of the `Provider` that dictates the CDEvent payload to send depending on the Flux event that triggered the alert. These CDEvent payloads should have meaningful data from the source event. There will be an initial focus on HelmRelease and related events as the format within Flux for those events is much more consistent.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be easier to comment on the individual lines if the paragraph is wrapped, maybe at 80 or 120 characters, whatever you prefer.

These CDEvent payloads should have meaningful data from the source event.

"event source" may fit well here, instead of "source event", indicating that the event source, the controllers, are responsible for meaningful data.

There will be an initial focus on HelmRelease and related events as the format within Flux for those events is much more consistent.

I don't think this needs to be mentioned in proposal. The corresponding github issues can have such implementation details.

Comment on lines 98 to 100
mapping:
ReconciliationFinished: dev.cdevents.pipelinerun.finished
GitOperationFailed: dev.cdevents.incident.reported
Copy link
Contributor

@darkowlzz darkowlzz Jun 13, 2024

Choose a reason for hiding this comment

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

I would like to discuss this further about adding a new mapping spec field for CDEvents specifically.
My initial understanding before this proposal was that the working of flux components are well defined which could be mapped to CDEvent event types in a deterministic way. Specific events from source-controller can be a kind of Source Code Control Event, events from kustomize-controller and helm-controller can be a kind of Continuous Deployment Event, etc.

I think the flux events can be categorized accordingly and statically configured to specific CDEvents event types. Any new event introduced in flux should also be categorized as a specific CDEvent. In notification-controller, the event handler can parse the events and forward them to message broker as per the fixed event categories.

I may be unaware of certain use cases of CDEvents that require such custom mapping, as I haven't used it myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi sunny, apologies for the slow response, we have decided to in fact switch to an approach that focuses more on a mapping that is defined in the way you described rather than the custom mapping we had considered before. I will make the changes to the RFC proposal to reflect this.

rfcs/NNNN-cdevents-provider/README.md Outdated Show resolved Hide resolved
Comment on lines 151 to 152
|ReconciliationSucceeded | Environment.Modified |
|ReconciliationFailed | Incident.Detected |
Copy link
Contributor

Choose a reason for hiding this comment

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

In the latest HelmRelease v2 API, I can't find these being used. We had these in the old API.

Comment on lines 127 to 151
| Helm Event | CDEvent |
| ------------------------- | ------------------------------- |
| InstallSucceeded | Environment.Modified |
|| Build.Finished (outcome not implemented yet) |
|| PipeLineRun.Finished (with outcome success) |
| InstallFailed | Incident.Detected |
|| Build.Finished (outcome not implemented yet) |
|| PipeLineRun.Finished (with outcome failed) |
|UpgradeSucceeded | TaskRun.Finished (with outcome success) |
|| Build.Finished (outcome not implemented yet) |
|| PipeLineRun.Finished (with outcome success) |
|UpgradeFailed | TaskRun.Finished (with outcome failed) |
|| Build.Finished (outcome not implemented yet) |
|| PipeLineRun.Finished (with outcome failed) |
|TestSucceeded | TestCaseRun.finished (outcome pass) |
|TestFailed | TestCaseRun.finished (outcome fail) |
|RollbackSucceeded | TaskRun.Finished (with outcome success) |
|| Build.Finished (outcome not implemented yet) |
|| PipeLineRun.Finished (with outcome success) |
|RollbackFailed | TaskRun.Finished (with outcome failed) |
|| Build.Finished (outcome not implemented yet) |
|| PipeLineRun.Finished (with outcome failed) |
|UninstallSucceeded | Environment.Modified |
|UninstallFailed | Incident.Detected |
|ReconciliationSucceeded | Environment.Modified |
Copy link
Member

Choose a reason for hiding this comment

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

This mapping feels wrong to me, we should map the HelmRelease to CDEvent service for the deployed, upgraded, rolledback, removed predicates.

adamkenihan and others added 2 commits July 18, 2024 12:00
Co-authored-by: Sunny <[email protected]>
Signed-off-by: adamkenihan <[email protected]>
Signed-off-by: adamkenihan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants