-
Notifications
You must be signed in to change notification settings - Fork 400
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
Warn about duplicated events received from GitHub via Admin Monitor #388
base: master
Are you sure you want to change the base?
Warn about duplicated events received from GitHub via Admin Monitor #388
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.
LGTM. Made some suggestions.
src/main/java/org/jenkinsci/plugins/github/extension/GHSubscriberEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DuplicateEventsSubscriber.java
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DuplicateEventsSubscriber.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DuplicateEventsSubscriber.java
Show resolved
Hide resolved
* At present, as the {@link #TTL_MILLIS} is set to 10 minutes, we consider half of it for cleanup. | ||
* This recurrence period is chosen to balance removing stale entries from accumulating in memory vs. | ||
* additional load on Jenkins due to a new periodic job execution. |
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.
Caffeine could be considered if you are looking for stronger assertions about the number of entries.
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.
Never knew about caffeine (checked now, it's good, thank you). But not sure if it is okay to bring in a new dependency into the plugin though.
The map here (Map<String, Long>
) approximately taking 44 bytes per entry (String: storing GUID 36 bytes, Long: 8 bytes); with 1MB memory usage ~23k entries can be stored; this is seems like a sufficient for 10 minutes worth events from GH (for one controller) (?).
So thinking maybe bringing in caffeine would be adding another dependency in here.
…DuplicateEventsSubscriber.java Co-authored-by: Vincent Latombe <[email protected]>
…admin monitor (checking if this is the best, if yes then will write a test for it)
I am fixing how the last duplicate event is presented to the user from, Edit: I made the payload open in a new browser tab since browsers like Firefox and Chrome format JSON nicely. With ~200 lines, it's easy to view without issues. The admin can still download it if needed or simply review it in the browser and proceed. |
src/main/java/org/jenkinsci/plugins/github/admin/GitHubDuplicateEventsMonitor.java
Fixed
Show resolved
Hide resolved
I think this PR is now ready 😄 I feel like covered all the features, and automated tests and manual tests. |
Problem
If duplicated GitHub events get delivered to Jenkins controller, Jenkins need to act on events both times (they are not automatically de-duplicated). These events may be a branch scan, build trigger etc.
further discussion of problem:
A branch scan event running twice may not be a problem, however build triggering twice could be a problem. Even the builds doesn't get in triggered twice due to
quietPeriod: 5s
(unless the quietPeriod was set to 0, or duplicates come after quietPeriod: 5s, which case two builds do get triggered).Note: usually GH by itself won't automatically redeliver an event if failed, GH simply marks the event as failed (an event is marked failed if GH didn't receive a 200 OK response within 10s; reference)
The reason for such duplicated events is,
Although it is typically rare for this duplication issue to happen, if it happens, it could require quite some time to figure out what is going on. And in some case it may simply go unnoticed.
Logs showing build can trigger twice in case of `quietPeriod: 0`
Note: in the below logs, when the two events came
Started by GitHub push by gbhat618
twice (ref); however rest of logs showed build steps were executed only once; I verified by this by having a single executor.Proposed solution
An admin monitor to show duplicate events are being received - admin monitor goes away by itself after 24 hours if no more duplicates.
further discussion of solution:
Duplication of event is identified by a header `X-GitHub-Delivery` ([reference](https://docs.github.com/en/webhooks/webhook-events-and-payloads#delivery-headers), also linked in javadoc), which is a unique identifier for an event in GitHub.An admin monitor is shown to warn about duplicated events being received. If the duplicated events are to occur, they typically occur in a few seconds window itself, we track an event for duplicate checking for about
10 minutes
- so it should be enough to catch the issue.For the cleanup of cached event entries, currently implemented via a PeriodWork to avoid any slowness in the event processing code path.
The admin can find the last received duplicate payload via the
Click Here
button shown (see the screenshots in Testing section) -- it opens the payload JSON in the new browser tab next to current tab. (browsers like firefox and chrome are by default acting as pretty json viewer - due to content-typeapplication/json
)I have considered alternative ways of presenting the event payload as well,
Click Here
: most of the admin can take a decision directly from seeing the payload in the browser (downloading to laptop - is another problem for the admin to cleanup the file later)Testing done
Automated tests are submitted
Manual testing
quietPeriod: 0
).FINE
logs.Screenshots
Submitter checklist