fix: auto-retry permanently-failed webhook alert subscriptions - #2
Open
nrajani431 wants to merge 5 commits into
Open
fix: auto-retry permanently-failed webhook alert subscriptions#2nrajani431 wants to merge 5 commits into
nrajani431 wants to merge 5 commits into
Conversation
gyanam.sh init writes ALERT_WEBHOOK_BASE_URL and ALERT_ENABLE_WEBHOOK_FALLBACK into .env, but docker-compose never forwarded them to the collector service. The values were therefore never read and the loopback default in AlertsConfig won, so AlertManager's startup validator disabled webhook fallback for every target — silently, since /health only reports on the collector process. Only the collector needs them; api_main.py never constructs an AlertManager. Also document both variables in .env.example, including the fact that the base URL must be routable from the BMCs and not just from the host. Signed-off-by: narajani <Naman.Rajani@amd.com>
A single PERMANENT SubscriptionFailureType put a target into
_permanently_failed forever, so _refresh_subscriptions skipped it on every
later pass and the only escape was deleting and re-adding the target. But
"permanent" is inferred from one BMC response, and the common causes are
temporary: a BMC rebooting mid-firmware-update, an event service disabled
for maintenance, a subscription table that happened to be full. Those
targets stayed silently dead, which defeats fleet-wide alerting.
_permanently_failed becomes a target_id -> next-retry-deadline map, expired
at the top of each refresh pass. The cooldown is configurable via
alerts.permanent_failure_retry_hours (default 6); 0 restores the old
never-auto-retry behaviour using a datetime.max sentinel that never expires.
The /alerts/manager-stats permanently_failed_targets field changes from a
list of ints to a list of {target_id, next_retry_at} objects. Nothing in the
repo consumed the old shape, so this adds the first consumer:
/api/subscription-status now reports these targets as failed_permanent with
their retry deadline instead of a bare "not_subscribed", and the alerts page
renders the countdown the template already had. The sentinel is reported as
"auto-retry disabled" rather than an ~8000-year countdown, and entries are
shape-checked so an older collector's flat int list cannot 500 the page.
Signed-off-by: narajani <Naman.Rajani@amd.com>
The collector's container healthcheck curls /health, which only reports on the collector process (exporter + poller running). BMC outages and failed alert subscriptions deliberately do not affect it — an unreachable BMC is not a reason to restart the container — but that was undocumented, so operators saw a healthy container and concluded alerting was fine. Point them at the places that do track per-target state: /health/detailed's alert_manager section, /alerts/manager-stats, and the alert Prometheus gauges. Signed-off-by: narajani <Naman.Rajani@amd.com>
Signed-off-by: narajani <Naman.Rajani@amd.com>
Signed-off-by: narajani <Naman.Rajani@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1
A webhook alert subscription that failed permanently stayed dead until someone restarted the collector, and the failure was easy to miss because
/healthstill reported the container as healthy. This branch retries those subscriptions on a cooldown, shows when the next attempt is due, and fixes the two configuration problems that made the failure likely in the first place.What changed
Permanent failures now expire.
AlertManager._permanently_failedchanged from a set of target IDs to a map of target ID to next-retry time. Once that deadline passes, the normal refresh path picks the target up again. The cooldown isalerts.permanent_failure_retry_hours, default 6 hours, and0keeps the old never-retry behavior.config.pyrejects negative values.The stats API stayed backward compatible.
permanently_failed_targetsstill returns a flat list of target IDs, so existing consumers and an older collector image during a rolling upgrade keep working. The newpermanent_failure_retriesfield carries{target_id, next_retry_at}pairs, and the route helper falls back to the old shape when the collector does not send the new one.docker-compose.ymlnow forwardsALERT_WEBHOOK_BASE_URLandALERT_ENABLE_WEBHOOK_FALLBACKto the collector. Before this, the collector ignored both and ran on built-in defaults no matter what.envsaid.Webhook fallback is off by default.
.env.example,docker-compose.yml, andgyanam.sh initnow ship an empty URL andALERT_ENABLE_WEBHOOK_FALLBACK=false. The receiver accepts unauthenticated POSTs and the collector does not publish its webhook port, so enabling it by default gave operators a listener they never asked for at a URL most BMCs could not reach. Turning it on is now a deliberate step that requires supplying a BMC-reachable address.docs/DEPLOYMENT.mdexplains why a healthy-looking collector can still have broken alert subscriptions, since that was the confusing part of diagnosing this.docs/class-diagram.mmdand its rendered PDF track the_permanently_failedtype change.One unrelated commit rides along:
.gitignorenow ignoresuv.lock.Compatibility
Existing deployments keep working without config changes, with one behavior change worth calling out. Anyone relying on webhook fallback being on by default has to set
ALERT_WEBHOOK_BASE_URLandALERT_ENABLE_WEBHOOK_FALLBACK=trueexplicitly after this change.Test plan
./scripts/run-tests.shpasses, 277 tests, coverage above the 50% gatecollector/tests/test_alert_manager_retry.pycover cooldown expiry, the disabled-retry sentinel, the old stats shape, and rejection of negative retry hoursdocker compose --env-file .env.example configrendersALERT_ENABLE_WEBHOOK_FALLBACK: 'false'and an emptyALERT_WEBHOOK_BASE_URLruff checkandmypyclean on the touched Python filesshellcheck gyanam.shSigned-off-byline (DCO)