-
Notifications
You must be signed in to change notification settings - Fork 107
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
Wrap sendAlert() in a try/except block. #10467
Wrap sendAlert() in a try/except block. #10467
Conversation
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.
Thanks for providing this fix, Todor. It looks good to me.
However, I'm only approving it once we also have the relevant configuration changes linked in this PR :-D (can you also please update the initial description? There is one field with the placeholder content).
Hi @amaltaro I am getting there (to the PR with the proper configuration changes) really soon ... The problem is I cannot manually generate an alert with the |
Jenkins results:
|
Hi @amaltaro, |
Jenkins results:
|
@todor-ivanov these changes look good to me. I have two questions though: |
01ffb2a
to
8720f89
Compare
Jenkins results:
|
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.
Thanks Todor, these changes look good to me.
I added the MSOutput configuration merge requests to the PR initial description.
Are you done testing these changes? Please also update it in the PR description.
Thanks @amaltaro ! Yes I am done with all the tests needed. Sorry for not reflecting this in the PR description. |
Thanks Todor. |
Fixes #10464
Status
Ready
Description
While using the old mechanism for sending alerts from
MSTransferor
we were not trying to handle exceptions at place, when it comes to sending alerts. But this was breaking the creation of the Rucio rules for a given data block. Or more precisely it was calling Rucio successfully and creating all the necessary rules, and only when it was about some large blocks(which were exceeding the so configured threshold), it was trying to send alerts which from times to times may break, due to a plethora of reasons [1]. In result the function callself.makeTransferRucio()
was never able to succeed and the call for the given DID was retried every MSTransferor polling cycle with one and the same RSE list, while the workflow was sitting inassigned
. In general Rucio is able to protect us from situation like this because it does not recreate already existing rules. A side effect of such behavior, though, is that in case any of the RSEs which are to be used for these rule submissions gets out of quota (i.e. for a reason not related to our broken workflow)MSTransferor
drops it from the list of RSEs for this DID and in result the new rule to be created is not considered as duplicate and then Rucio creates a brand new rule, which leads to the over replication.The current PR aims to enclose the the call to the alertManager in order to avoid situation as the one explained above.
[1]
WMCore/src/python/WMCore/MicroService/MSTransferor/MSTransferor.py
Line 727 in 5da7f3c
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/83
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/84
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/85
and configuration for MSOutput
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/86
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/87
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/88
External dependencies / deployment changes
In order to fix the actual issue we also need to provide the proper PR in the gitlab K8 configuration repository for adding the correct
AlertManagerUrl
to theMSTransfferor
configuration