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

Alerts integration improvements and oom killer #9

Merged
merged 6 commits into from
Jun 29, 2021

Conversation

aantn
Copy link
Collaborator

@aantn aantn commented Jun 28, 2021

No description provided.

@aantn aantn requested a review from arikalon1 June 28, 2021 12:17
playbooks/oom_killer.py Outdated Show resolved Hide resolved
for pod in results.items:
oom_statuses = filter(is_oom_status, pod.status.containerStatuses)
for status in oom_statuses:
dt = datetime.fromisoformat(status.lastState.terminated.finishedAt.replace("Z", "+00:00"))
Copy link
Contributor

@arikalon1 arikalon1 Jun 28, 2021

Choose a reason for hiding this comment

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

isn't there a way to use the k8s datetime str?
I think Z means utc.
If there will be an api server running on a different tz, I think there will be a bug here
this maybe:
dt = datetime.datetime.strptime(status.lastState.terminated.finishedAt, "%Y-%m-%dT%H:%M:%S%z")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is a little tricky and it's similar to the Hikaru issue we saw with datetimes.

All the datetimes I get from the APIServer have Z at the end without a specific timezone set. I've done some brief research and I'm pretty sure that APIServers typically run in UTC and changing that is extremely uncommon / impossible.

Are you sure that the code you suggested behaves differently then what I committed? Wont both snippets throw an exception if the string that K8s returns has a timezone present?

I'm ok just ignoring this for now as I'm not sure the issue is actually possible. Wdyt?

attribute_name: str
prometheus_label: str

MAPPINGS = [ResourceMapping(RobustaPod, "pod", "pod"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the order matters?
I thought it did matter, now I'm not sure
I think some alerts may contain more than a single attribute (like pod and job)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they can have multiple attributes, but the order doesn't matter as I will populate all of them.

@arikalon1
Copy link
Contributor

added some comments, left for you to merge

@aantn aantn merged commit 97496cc into master Jun 29, 2021
@aantn aantn deleted the alerts_integration_improvements_and_oom_killer branch June 29, 2021 16:31
RoiGlinik pushed a commit that referenced this pull request Oct 19, 2022
* updated forwarder and runner serviceaccount permission for clusterrolebindings

* Job Action Added

* Webex Sink Integration (#3)

* Added webex_sink

* Bug fix with fileblocks, code refactoring, documentation add and minor bug fixes

* Delete job_restart_on_oomkilled.py

* Updated Webex_Sink (#6)

* Added webex_sink

* Bug fix with fileblocks, code refactoring, documentation add and minor bug fixes

* Added Enums and replaced emoji code with emoji icons

* Removed PDF functionality and separated JSON logic from adaptive card function. (#7)

* Added webex_sink

* Bug fix with fileblocks, code refactoring, documentation add and minor bug fixes

* Added Enums and replaced emoji code with emoji icons

* Removed PDF functionality and seperated JSON creation logic from the adaptivecard function logic

* Formatted using Black

* Changes in _send_files and _separate_blocks function (#8)

* Added webex_sink

* Bug fix with fileblocks, code refactoring, documentation add and minor bug fixes

* Added Enums and replaced emoji code with emoji icons

* Removed PDF functionality and seperated JSON creation logic from the adaptivecard function logic

* Formatted using Black

* Changes in _send_files and _separate_blocks function

* Changes in _send_files and _separate_blocks function (#9)

* Added webex_sink

* Bug fix with fileblocks, code refactoring, documentation add and minor bug fixes

* Added Enums and replaced emoji code with emoji icons

* Removed PDF functionality and seperated JSON creation logic from the adaptivecard function logic

* Formatted using Black

* Changes in _send_files and _separate_blocks function

Co-authored-by: wahajXgrid <wahaj.ali@xgrid.co>
Co-authored-by: wahajXgrid <107921443+wahajXgrid@users.noreply.github.com>
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.

2 participants