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

[WIP] 441/bugfix/slow report generation with plenty of machines #447

Merged

Conversation

ShayNehmad
Copy link
Contributor

@ShayNehmad ShayNehmad commented Oct 2, 2019

Feature / Fixes

Fixes #441.

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Have you successfully tested your changes locally?

Screenshots:

From:
image
To (Middle of progress):
image
To (final):
image

Changes

  • Improved runtime of the get_displayed_node_by_id function via caching
  • Improved runtime of the get_scanned function via algorithmic improvments
  • Other small optimisations in EdgeService and NodeService via caching mostly.

@ShayNehmad ShayNehmad added Bug An error, flaw, misbehavior or failure in the Monkey or Monkey Island. Island labels Oct 2, 2019
@ShayNehmad ShayNehmad self-assigned this Oct 2, 2019
@ShayNehmad ShayNehmad added this to In progress in Monkey Dev Board via automation Oct 2, 2019
@@ -16,7 +16,8 @@ def parse_creds(attempt):
if attempt[key]:
return '%s ; %s : %s' % (username,
cred['type'],
cred['output'])
# TODO Figure out why this is causing an exception with Vakaris
Copy link
Contributor

Choose a reason for hiding this comment

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

to_id = NodeService.get_monkey_by_id(edge["to"])
if to_id is None:
to_label = NodeService.get_node_label(NodeService.get_node_by_id(edge["to"]))
if Monkey.is_monkey(to_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this needs to be changed now, but this is hacky. We should have the Monkey check inside the NodeService

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we create a Node model and think about the data structure - this should be changed (I think Monkey should be an extension of Node, instead of 2 different models). Until then this patch can remain IMO

for node in mongo.db.node.find({'exploited': True}, {'_id': 1})]
mongo.db.monkey.find({}, {'_id': 1}) if
not NodeService.get_monkey_manual_run(NodeService.get_monkey_by_id(monkey['_id']))]

Copy link
Contributor

Choose a reason for hiding this comment

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

This entire section should probably be rewritten (not now?)
it's basically iterating twice over mongo.db.monkey.find({}, {'_id': 1} and then asking different questions.
I'd rather we pull it once (meaning)
nodes_with_monkeys = [NodeService.get_displayed_node_by_id(monkey['_id'], True) for monkey in mongo.db.monkey.find({}, {'_id': 1})]
And then filter.
This requires an ugly rewrite to use an inner function or have the report not be static :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or a different "exploited_nodes_fetcher" which can be stateful. Again - I agree, but this is not for now. Added to long-term planning on the board.

Comment on lines +145 to +158
nodes_with_monkeys = [NodeService.get_displayed_node_by_id(monkey['_id'], True) for monkey in
mongo.db.monkey.find({}, {'_id': 1})]
nodes = nodes_without_monkeys + nodes_with_monkeys
return nodes

@staticmethod
def get_exploited():
exploited = \
exploited_with_monkeys = \
[NodeService.get_displayed_node_by_id(monkey['_id'], True) for monkey in
mongo.db.monkey.find({}, {'_id': 1})
if not NodeService.get_monkey_manual_run(NodeService.get_monkey_by_id(monkey['_id']))] \
+ [NodeService.get_displayed_node_by_id(node['_id'], True)
for node in mongo.db.node.find({'exploited': True}, {'_id': 1})]
mongo.db.monkey.find({}, {'_id': 1}) if
not NodeService.get_monkey_manual_run(NodeService.get_monkey_by_id(monkey['_id']))]

exploited_without_monkeys = [NodeService.get_displayed_node_by_id(node['_id'], True) for node in
mongo.db.node.find({'exploited': True}, {'_id': 1})]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment just on node find.

@@ -699,6 +706,8 @@ def generate_report():
cross_segment_issues = ReportService.get_cross_segment_issues()
monkey_latest_modify_time = Monkey.get_latest_modifytime()

scanned_nodes = ReportService.get_scanned()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why moved outside? Readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Not critical and can be undone

@ShayNehmad ShayNehmad merged commit 3b6714e into develop Oct 3, 2019
Monkey Dev Board automation moved this from In progress to Done Oct 3, 2019
@ShayNehmad ShayNehmad deleted the 441/bugfix/slow-report-generation-with-plenty-of-machines branch October 3, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug An error, flaw, misbehavior or failure in the Monkey or Monkey Island.
Projects
Development

Successfully merging this pull request may close these issues.

Report generation is slow when dealing with large amounts of network nodes
2 participants