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

Handle notifications with large number of tickets and use docker compose syntax #77

Merged
merged 3 commits into from
Jun 10, 2022

Conversation

dav3r
Copy link
Member

@dav3r dav3r commented Jun 10, 2022

🗣 Description

This PR resolves a situation where a Mongo query can run out of memory and fail if a notification contains a large number of tickets.

In addition, 5485ed9 updates create_send_notifications.py to use the newer docker compose syntax instead of the older style docker-compose syntax. The docker compose syntax is the preferred (and only correct) syntax after the changes in cisagov/ansible-role-docker#60.

💭 Motivation and context

After our recent switch to CVSS v3 (see cisagov/cyhy-core#69 and #76), our daily notification process failed because one of the organizations had a very large number of tickets to process for their notification. The process failed with this error:

Jun 10 08:15:55 reporter cyhy-notifications: 2022-06-10 08:15:55,705 INFO <ORG_ID> - Starting to create notification PDF
Jun 10 08:15:56 reporter cyhy-notifications: Traceback (most recent call last):
Jun 10 08:15:56 reporter cyhy-notifications:   File "./create_send_notifications.py", line 184, in <module>
Jun 10 08:15:56 reporter cyhy-notifications:     sys.exit(main())
Jun 10 08:15:56 reporter cyhy-notifications:   File "./create_send_notifications.py", line 115, in main
Jun 10 08:15:56 reporter cyhy-notifications:     num_pdfs_created = generate_notification_pdfs(db, cyhy_org_ids, master_report_key)
Jun 10 08:15:56 reporter cyhy-notifications:   File "./create_send_notifications.py", line 72, in generate_notification_pdfs
Jun 10 08:15:56 reporter cyhy-notifications:     was_encrypted, results = generator.generate_notification()
Jun 10 08:15:56 reporter cyhy-notifications:   File "/usr/local/lib/python2.7/dist-packages/cyhy_report/cyhy_notification/generate_notification.py", line 162, in generate_notification
Jun 10 08:15:56 reporter cyhy-notifications:     self.__run_queries()
Jun 10 08:15:56 reporter cyhy-notifications:   File "/usr/local/lib/python2.7/dist-packages/cyhy_report/cyhy_notification/generate_notification.py", line 361, in __run_queries
Jun 10 08:15:56 reporter cyhy-notifications:     self.__results["tickets"] = self.__load_tickets(ticket_ids)
Jun 10 08:15:56 reporter cyhy-notifications:   File "/usr/local/lib/python2.7/dist-packages/cyhy_report/cyhy_notification/generate_notification.py", line 271, in __load_tickets
Jun 10 08:15:56 reporter cyhy-notifications:     ("details.name", 1),
Jun 10 08:15:56 reporter cyhy-notifications:   File "/usr/local/lib/python2.7/dist-packages/mongokit/cursor.py", line 42, in next
Jun 10 08:15:56 reporter cyhy-notifications:     if len(self.__data) or self._refresh():
Jun 10 08:15:56 reporter cyhy-notifications:   File "/usr/local/lib/python2.7/dist-packages/pymongo/cursor.py", line 1095, in _refresh
Jun 10 08:15:56 reporter cyhy-notifications:     self.__codec_options.uuid_representation))
Jun 10 08:15:56 reporter cyhy-notifications:   File "/usr/local/lib/python2.7/dist-packages/pymongo/cursor.py", line 1037, in __send_message
Jun 10 08:15:56 reporter cyhy-notifications:     self.__compile_re)
Jun 10 08:15:56 reporter cyhy-notifications:   File "/usr/local/lib/python2.7/dist-packages/pymongo/helpers.py", line 144, in _unpack_response
Jun 10 08:15:56 reporter cyhy-notifications:     error_object)
Jun 10 08:15:56 reporter cyhy-notifications: pymongo.errors.OperationFailure: database error: Executor error during OP_QUERY find :: caused by :: errmsg: "Sort operation used more than the maximum 33554432 bytes of RAM. Add an index, or specify a smaller limit."

I first attempted to fix this by adding a new index to support this sort, but I was unable to get the sort to use that index. So instead, I worked around this issue by switching to a Mongo aggregate query, which allowed me to use the allowDiskUse option to circumvent the memory limitation.

🧪 Testing

To test these changes, I first verified that I could manually create a notification for the organization that failed with the memory error, and that was successful. I then created a fresh notification for an org that had previously (this morning) successfully generated a notification, and I compared those two docs to ensure that they were identical (they were).

Finally, I executed the entire daily notification process, which ran without any errors.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

dav3r added 2 commits June 10, 2022 11:17
A regular find/sort query can exceed the memory limit of MongoDB if there are a large number of tickets included in a notification.
Now that we are not using a regular find/sort query, our result set no longer contains TicketDocs, hence these changes.
@dav3r dav3r added the bug This issue or pull request addresses broken functionality label Jun 10, 2022
@dav3r dav3r self-assigned this Jun 10, 2022
@dav3r dav3r requested review from felddy, jsf9k and mcdonnnj June 10, 2022 15:41
Copy link
Member

@felddy felddy left a comment

Choose a reason for hiding this comment

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

allowDiskUse=Booosh!
Fine work @dav3r 💯

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Powerful work as always, @dav3r.

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

🕵️‍♂️ :shipit:

The "docker compose" syntax is the preferred (and only correct) syntax after the changes in cisagov/ansible-role-docker#60.
@dav3r dav3r requested review from felddy, mcdonnnj and jsf9k June 10, 2022 18:09
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Approval intensifies

@dav3r dav3r changed the title Handle notifications with large number of tickets Handle notifications with large number of tickets and use docker compose syntax Jun 10, 2022
Copy link
Member

@felddy felddy left a comment

Choose a reason for hiding this comment

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

  • Changes are limited to a single goal - eschew scope creep!
    🤣

💪

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

🐳

@dav3r dav3r merged commit 4176bac into develop Jun 10, 2022
@dav3r dav3r deleted the bugfix/handle-large-notifications branch June 10, 2022 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants