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

Bot debugging #975

Merged
12 commits merged into from Jun 13, 2017
Merged

Bot debugging #975

12 commits merged into from Jun 13, 2017

Conversation

e3rd
Copy link
Member

@e3rd e3rd commented May 15, 2017

(This is a continuation of PR #973 , because I completely messed up a git history...)

BotDebugger is called via intelmqctl. It starts a live running bot instance,
leverages logging to DEBUG level and permits even a non-skilled programmer
who may find themselves puzzled with Python nuances and server deployment twists
to see what's happening in the bot and where's the error.

Depending on the subcommand received, the class either

  • starts the bot as is (default)
  • processes single message, either injected or from default pipeline (process subcommand)
  • reads the message from input pipeline or send a message to output pipeline (message subcommand)

Further help was added to argparse help of intelmqctl:
Possible commands:

intelmqctl run bot-id (bot.start())
intelmqctl run bot-id message get (read the next message)
intelmqctl run bot-id message pop (read the next message and pop from queue)
intelmqctl run bot-id message send '{a:b}' (create message from string and send to output queue)
intelmqctl run bot-id process (process single message)
intelmqctl run bot-id process --msg '{a:b}' (process single message from string)
intelmqctl run bot-id process --dryrun (process single message from pipeline or --msg, but never really acknowledge nor send it to output pipeline)
intelmqctl run bot-id console (type) (starts ipdb console with self defined as the bot)

There were commands I always wanted to have. I missed them when creating/quickly debugging the bots. If you find them useful, too, I'd be very glad to publish it in the main repository. I am open to any discussion concerning the new commands.

Wagner's response:

The features are really great. I had something like this in mind too but never had the time to actually do it.
I am missing a detailed explanation including examples for the users in docs/intelmqctl.md
Please fix the code style issues: https://travis-ci.org/certtools/intelmq/jobs/231607021#L2790

Dear @wagner-certat , please respond to my 3 little discussions:

Then I have to finish this:

@ghost ghost self-assigned this May 16, 2017
@ghost ghost self-requested a review May 16, 2017 08:41
@ghost ghost added this to the v1.1 Feature release milestone May 16, 2017
@ghost
Copy link

ghost commented May 16, 2017

(This is a continuation of PR #973 , because I completely messed up a git history...)

You can always force push to your branch.

@e3rd
Copy link
Member Author

e3rd commented May 16, 2017

(You are right, I gave up too early yesterday. :( :D )

I am working on the changes. Meanwhile, I've added another parameter, console, that would start an ipdb console in the bot. I find it very useful too. One doesn't have to write import ipdb; ipdb.set_trace() in the bot every time they want just to check sth out. (For example, how to trick the bot to never pop from internal to source queue – I had to replace the pipe.brpoplpush function by pipe.lindex.)

self.bot_stop(bot_id)
else:
paused = False
self.logger.warning("Main instance of the bot is running in the background. You may want to launch: intelmqctl stop {}".format(bot_id))
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed IntelMQProcessManager.bot_run method so that it's not problem anymore to run multiple instance of bot.
If we run a bot that is already running, only warning "Main instance of the bot is running in the background. You may want to launch: intelmqctl stop {}" is printed.
However, if there is a reason for the background instance is rather paused and relaunched after debugging is finished, I commented all the code so that I can restore it easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, now the background process will get stopped and relaunched afterwards.

@codecov-io
Copy link

codecov-io commented May 18, 2017

Codecov Report

Merging #975 into master will decrease coverage by 0.76%.
The diff coverage is 18.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #975      +/-   ##
==========================================
- Coverage   77.98%   77.22%   -0.77%     
==========================================
  Files         221      222       +1     
  Lines        9047     9197     +150     
==========================================
+ Hits         7055     7102      +47     
- Misses       1992     2095     +103
Impacted Files Coverage Δ
intelmq/bin/intelmqctl.py 12.82% <10.71%> (+0.01%) ⬆️
intelmq/lib/bot_debugger.py 20.33% <20.33%> (ø)
intelmq/bots/outputs/elasticsearch/output.py 89.09% <0%> (-0.91%) ⬇️
intelmq/lib/bot.py 68.65% <0%> (-0.69%) ⬇️
intelmq/tests/lib/test_splitreports.py 98.48% <0%> (-0.03%) ⬇️
intelmq/bots/parsers/abusech/parser_ransomware.py 100% <0%> (ø) ⬆️
intelmq/bots/parsers/fraunhofer/parser_dga.py 100% <0%> (ø) ⬆️
...q/tests/bots/parsers/fraunhofer/test_parser_dga.py 100% <0%> (ø) ⬆️
intelmq/tests/bots/experts/idea/test_expert.py 96.77% <0%> (ø) ⬆️
intelmq/tests/bots/parsers/bambenek/test_parser.py 100% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b03b85d...32a291e. Read the comment docs.

@e3rd
Copy link
Member Author

e3rd commented May 22, 2017

Everything's done. Please tell me if there is anything I can do more :)
KR,
Edvard

@ghost
Copy link

ghost commented May 22, 2017

I'm currently traveling, I will review the code as soon as I have time again. You can feel certain that I am very eager to get this integrated!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

How to quit the console without raising an Exception? Using quit(), Ctrl+D and self.stop() all lead to:

(Pdb) quit()
Traceback (most recent call last):
  File "/usr/local/bin/intelmqctl", line 9, in <module>
    load_entry_point('intelmq==1.0.0.dev7', 'console_scripts', 'intelmqctl')()
  File "/home/sebastian/dev/intelmq/intelmq/bin/intelmqctl.py", line 902, in main
    return x.run()
  File "/home/sebastian/dev/intelmq/intelmq/bin/intelmqctl.py", line 548, in run
    results = args.func(**args_dict)
  File "/home/sebastian/dev/intelmq/intelmq/bin/intelmqctl.py", line 558, in bot_run
    return self.bot_process_manager.bot_run(**kwargs)
  File "/home/sebastian/dev/intelmq/intelmq/bin/intelmqctl.py", line 138, in bot_run
    console_type, dryrun, message_action_kind, msg)
  File "/home/sebastian/dev/intelmq/intelmq/lib/bot_debugger.py", line 51, in __init__
    self._console(console_type)
  File "/home/sebastian/dev/intelmq/intelmq/lib/bot_debugger.py", line 80, in _console
    module.set_trace()
  File "/usr/lib64/python3.4/bdb.py", line 52, in trace_dispatch
    return self.dispatch_return(frame, arg)
  File "/usr/lib64/python3.4/bdb.py", line 96, in dispatch_return
    if self.quitting: raise BdbQuit
bdb.BdbQuit

And it seems the stop-method is not called in these cases, which is necessary for a proper shutdown of the bot.

@@ -168,8 +276,8 @@ intelmqctl: Starting abusech-domain-parser...
intelmqctl: abusech-domain-parser is running.
intelmqctl: Starting abusech-feodo-domains-collector...
intelmqctl: abusech-feodo-domains-collector is running.
intelmqctl: Starting deduplicator-expert...
intelmqctl: deduplicator-expert is running.
intelmqctl: Starting file-output...
Copy link

Choose a reason for hiding this comment

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

Why did you rename all these bot names? They do not make sense, as there is already a file-output in this example.

with open(filename, 'w') as fp:
fp.write(str(os.getpid()))
if pid and self.__status_process(pid):
self.logger.warning("Main instance of the bot is running in the background. You may want to launch: intelmqctl stop {}"
Copy link

Choose a reason for hiding this comment

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

If there one bot is running twice in parallel, this will lead to duplicated messages as they are using the same (internal) source queue. Please restore the previous behavior.

Also see #710 (comment) and certat#92

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok then, now the background process will get stopped and relaunched afterwards.

# Never pops from source to internal queue, thx to disabling brpoplpush operation.
# However, we have to wait manually till there is the message in the queue.
pl = self.instance._Bot__source_pipeline
pl.pipe.brpoplpush = lambda source_q, inter_q, i: pl.pipe.lindex(source_q, -1)
Copy link

Choose a reason for hiding this comment

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

This does not read messages from the internal queue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, corrected.

e3rd added 2 commits June 6, 2017 19:46
For parser bot, the messages are quite often too long. We can now just specify the filepath of a JSON than the JSON itself.
e3rd added a commit to CZ-NIC/intelmq that referenced this pull request Jun 6, 2017
certtools#975 - debugger can accept files as messages
certtools#993 - alienvault-otx conforming ParserBot
@e3rd
Copy link
Member Author

e3rd commented Jun 8, 2017

How to quit the console without raising an Exception?

I've added a hint that "c" command will make you quit the console. However, if we break the process by Ctrl+D, the stopped bot won't get realunched. If that's a big problem, we may disable the possibility of to do bot_run while the bot is running in the background.

@navtej
Copy link
Contributor

navtej commented Jun 10, 2017

This will be good to have.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

We are almost there :)

fp.write(str(os.getpid()))
if pid and self.__status_process(pid):
self.logger.warning("Main instance of the bot is running in the background and will be stopped; "
"when finished, we try to relaunch it again."
Copy link

Choose a reason for hiding this comment

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

Missing space between the two sentences

Copy link
Member Author

@e3rd e3rd Jun 13, 2017

Choose a reason for hiding this comment

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

Where please? "and will be stopped; SPACE when finished"
I must be blind :D , I don't see where it's missing...

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I found it...

Copy link

@ghost ghost Jun 13, 2017

Choose a reason for hiding this comment

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

Either in this line at the end or at the beginning of the next one.

# log_bot_message('starting', bot_id)
# filename = self.PIDFILE.format(bot_id)
# with open(filename, 'w') as fp:
# fp.write(str(os.getpid()))
Copy link

Choose a reason for hiding this comment

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

Without writing the PID the bot can still be started in parallel leading to weird behavior and duplicate messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it! Thanks for reviewing me.

@ghost ghost merged commit 32a291e into certtools:master Jun 13, 2017
@ghost ghost removed this from the v1.1 Feature release milestone Jul 5, 2017
@ghost ghost modified the milestones: v1.0 Stable Release, v1.1 Feature release Jul 5, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants