-
Notifications
You must be signed in to change notification settings - Fork 45
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
Alert history #3191
Alert history #3191
Conversation
Hello alexandre-allard-scality,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
80bd91d
to
3873910
Compare
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
3873910
to
1a333a3
Compare
History mismatchMerge commit #f6cf8807ddc2744994944db8283b45725c082587 on the integration branch It is likely due to a rebase of the branch Please use the |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
1a333a3
to
10f8ed3
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
c6320f9
to
d196d3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly peripheral comments, some things should change in this PR though:
- add the logger Go code to the linter
- adjust the K8s manifests to use standard labels
- adjust the test to check alert format (using the Watchdog "example")
As for now, there is no retention based on labels, streams, tenant or | ||
whatever (on-going discussion `GH Loki #162`_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to follow this closely, and maybe contribute. Having tenant-based retention could be really nice for separating different kinds of log streams (not only alerts, but also e.g. Salt events, audit logs, ...).
Name container | ||
Format regex | ||
Regex ^(?<time>[^ ]+) (?<stream>stdout|stderr) (?<logtag>[^ ]+) (?<message>.+)$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this parser should always be used for containerd
-formatted logs, no? And as such, likely this commit should be backported to the first version where Loki was introduced (2.6, I guess?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, we should probably backport it.
Will do it once this PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR opened here #3276
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
54a973f
to
4af620b
Compare
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
c11fa8b
to
b660dcb
Compare
Move all `.uml` to a `diagrams` directory, so the directory only contains architecture document which is cleaner.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
1 similar comment
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some relatively minor comments, feel free to put them in a debt ticket.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
1dd396f
to
864d947
Compare
This is a simple HTTP server, listening on a port (default to 19094), waiting for HTTP post request from alertmanager. It then logs the content of these requests to stdout. Refs: #3180
This container image is used for alert history. Refs: #3180
Remove the date from the logs once parsed by fluent-bit as we do not need it anymore and it allows to improve the logs readability. Plus, it is also need by the alert history feature, this way we only end up with a JSON formatted alert in logs which makes it easier to parse/use by other components. Refs: #3180
We now send all the alerts to the alert-logger receiver. Refs: #3180
We also need to enable CORS to allow Cross Origin requests coming from the web UI. Refs: #3180
This scenario checks that we can retrieve the cluster alerts from the Loki API. Refs: #3180
We were passing a CmdAction object to target actions, so doit was not doing anything. We now pass the execute method of the object.
We need a more recent version of plantuml to be able to build alert-history diagram and this version is not shipped in ubuntu:18.04
There is an issue with mktexpk not being able to create a directory, so wo create these fonts prior to launch the build. Also fix an issue by always changing ownership of doc build artifacts to avoid having root owned files when doc build fails.
864d947
to
b7d65df
Compare
/approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget about https://github.com/scality/metalk8s/pull/3191/files#r601234682 😉
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye alexandre-allard-scality. |
Component: images, build, salt
Context:
Summary:
Acceptance criteria:
Closes: #3180