Add Job filters #247

aarontp · 2018-09-07T00:34:14Z

Adds Job whitelist/blacklists. This is used when either starting the Turbinia Server, or per-request from the client.

aarontp · 2018-09-07T00:51:14Z

I'm not sure I agree with codefactor's "complexity" rating here. The method seems fairly straightforward to me. I'm open to suggestions for how to handle this.

Onager · 2018-09-10T12:01:17Z

turbinia/client.py

@@ -350,13 +350,15 @@ class TurbiniaServer(object):

  Attributes:
    task_manager (TaskManager): An object to manage turbinia tasks.
+    jobs_blacklist (list): Jobs we will exclude from running


List of what? Strings? Class names? Instances? Please add this to the docstring.

Onager · 2018-09-10T12:02:15Z

turbinia/jobs/__init__.py

+  and all Jobs will be returned if both are specified.
+
+  Args:
+    jobs_blacklist (list): Jobs that will be excluded from running


As above, please indicate the type of things in these lists

Onager · 2018-09-10T12:04:12Z

turbinia/jobs/__init__.py

+      TomcatAnalysisJob()]
+
+  if jobs_whitelist and jobs_blacklist:
+    log.info(


Just .info? I would have thought Warn, at least. I also would prefer raising here, rather than trying to continue - what do you think?

+1 on raising and let the user know.

Yeah, I think raising is a better idea as well. Done.

turbinia/jobs/__init__.py

berggren · 2018-09-21T13:39:15Z

turbinia/jobs/__init__.py

+      TomcatAnalysisJob()]
+
+  if jobs_whitelist and jobs_blacklist:
+    log.info(


+1 on raising and let the user know.

turbinia/task_manager.py

aarontp

Ok, I addressed the comments and merged this in to work with the new Job manager. PTAL. Thanks!

aarontp · 2018-11-08T00:56:00Z

turbinia/client.py

@@ -350,13 +350,15 @@ class TurbiniaServer(object):

  Attributes:
    task_manager (TaskManager): An object to manage turbinia tasks.
+    jobs_blacklist (list): Jobs we will exclude from running


turbinia/jobs/__init__.py

aarontp · 2018-11-08T05:02:26Z

turbinia/jobs/__init__.py

+  and all Jobs will be returned if both are specified.
+
+  Args:
+    jobs_blacklist (list): Jobs that will be excluded from running


aarontp · 2018-11-08T05:02:49Z

turbinia/jobs/__init__.py

+      TomcatAnalysisJob()]
+
+  if jobs_whitelist and jobs_blacklist:
+    log.info(


Yeah, I think raising is a better idea as well. Done.

turbinia/task_manager.py

aarontp · 2018-11-14T19:20:29Z

Sorry, I'll fix the conflicts first.

aarontp · 2018-11-15T23:00:00Z

Ok, PTAL. Thanks!

Onager

Looks good, just one thing I think should be different.

Onager · 2018-11-21T16:24:00Z

turbinia/jobs/manager.py


 class JobsManager(object):
  """The jobs manager."""

  _job_classes = {}

+  @classmethod
+  def FilterJobNames(
+      cls, jobs, jobs_blacklist=None, jobs_whitelist=None, objects=False):


I'm not a big fan of the "objects" arg here, and it doesn't seem to be used so I just removing it.

I recommend two different methods if you want this functionality in the future.

Yeah, I felt it was a bit skeevy too. It was actually used, but I refactored it into a separate function, and I think this is better.

PTAL

Onager · 2018-12-07T11:55:00Z

turbinia/client.py

  """

-  def __init__(self):
+  def __init__(self, jobs_blacklist=None, jobs_whitelist=None):
    """Initialize Turbinia Server."""


"Initializes" + add args section

Thanks, yeah, it was documented just above in the class attributes, but I realized that they are not actually class attributes, so I moved it here.

Onager · 2018-12-07T11:57:46Z

turbinia/jobs/manager.py

+    jobs_whitelist and jobs_blacklist must not be specified at the same time.
+
+    Args:
+      jobs (list[str]): The names of the jobs to filter.


rename to job_names for clarity

Onager · 2018-12-07T11:58:15Z

turbinia/jobs/manager.py

+    jobs_whitelist and jobs_blacklist must not be specified at the same time.
+
+    Args:
+      jobs (list[TurbiniaJob]): The names of the jobs to filter.


Fix the docstring here - this is the jobs themselves not their names.

Onager · 2018-12-07T12:03:59Z

turbinia/task_manager.py

+    """Does setup of Task manager and its dependencies.
+
+    Args:
+      jobs_blacklist (list): Jobs that will be excluded from running


(Optional[list[str]), not (list)

Done. Can you point me to a specification for this? Thanks.

https://github.com/log2timeline/l2tdocs/blob/master/process/Style-guide.md#docstrings

Onager · 2018-12-07T12:04:41Z

turbinia/task_manager.py

    self._backend_setup(*args, **kwargs)
    # TODO(aarontp): Consider instantiating a job per evidence object
    job_names = jobs_manager.JobsManager.GetJobNames()
+    if jobs_blacklist or jobs_whitelist:


Add a check here for specifying both? Since that will cause an exception later anyway.

See my comment below.

Onager · 2018-12-07T12:05:12Z

turbinia/task_manager.py

@@ -125,11 +137,22 @@ def add_evidence(self, evidence_):
    log.info('Adding new evidence: {0:s}'.format(str(evidence_)))
    self.evidence.append(evidence_)
    job_count = 0
+    jobs_whitelist = evidence_.config.get('jobs_whitelist', [])
+    jobs_blacklist = evidence_.config.get('jobs_blacklist', [])
+    if jobs_blacklist or jobs_whitelist:


See above comment about raising here instead of later

I actually originally had a check that raised an exception here, but @berggren suggested removing it. Unless you feel strongly about this, I think I'll leave it as is since it will still raise an exception anyway. In practice this condition will be caught on start anyway (with another check that occurs during arg parse time).

aarontp

PTAL

aarontp · 2018-12-07T17:16:03Z

turbinia/client.py

  """

-  def __init__(self):
+  def __init__(self, jobs_blacklist=None, jobs_whitelist=None):
    """Initialize Turbinia Server."""


Thanks, yeah, it was documented just above in the class attributes, but I realized that they are not actually class attributes, so I moved it here.

aarontp · 2018-12-07T17:19:49Z

turbinia/jobs/manager.py

+    jobs_whitelist and jobs_blacklist must not be specified at the same time.
+
+    Args:
+      jobs (list[str]): The names of the jobs to filter.


aarontp · 2018-12-07T17:20:22Z

turbinia/jobs/manager.py

+    jobs_whitelist and jobs_blacklist must not be specified at the same time.
+
+    Args:
+      jobs (list[TurbiniaJob]): The names of the jobs to filter.


aarontp · 2018-12-07T17:28:34Z

turbinia/task_manager.py

+    """Does setup of Task manager and its dependencies.
+
+    Args:
+      jobs_blacklist (list): Jobs that will be excluded from running


Done. Can you point me to a specification for this? Thanks.

aarontp · 2018-12-07T19:17:17Z

turbinia/task_manager.py

@@ -125,11 +137,22 @@ def add_evidence(self, evidence_):
    log.info('Adding new evidence: {0:s}'.format(str(evidence_)))
    self.evidence.append(evidence_)
    job_count = 0
+    jobs_whitelist = evidence_.config.get('jobs_whitelist', [])
+    jobs_blacklist = evidence_.config.get('jobs_blacklist', [])
+    if jobs_blacklist or jobs_whitelist:


I actually originally had a check that raised an exception here, but @berggren suggested removing it. Unless you feel strongly about this, I think I'll leave it as is since it will still raise an exception anyway. In practice this condition will be caught on start anyway (with another check that occurs during arg parse time).

aarontp · 2018-12-07T19:17:46Z

turbinia/task_manager.py

    self._backend_setup(*args, **kwargs)
    # TODO(aarontp): Consider instantiating a job per evidence object
    job_names = jobs_manager.JobsManager.GetJobNames()
+    if jobs_blacklist or jobs_whitelist:


See my comment below.

All review feedback should already be addressed

aarontp · 2018-12-10T18:18:53Z

@berggren I ended up having to "dismiss" your review, though AFAIK all of the review feedback has been addressed. There was one comment I hadn't resolved, but it won't let me resolve it or comment on it now (I believe because it refers to a commit that no longer exists due to pushing a rebased-branch). If anyone knows of a better way to resolve reviews without dismissing them, please let me know.

* Pin specific Celery/Kombu/Redis versions (google#305) * in TurbiniaTaskResult, input_evidence doesn't have to be a list (google#309) * input_evidence doesn't have to be a list * Fix exception * current pypi version of google-cloud-storage (1.13.0) requires google-cloud-core 0.28.1 (before the rename of google.cloud.iam (core) to google.api_core.iam (google#315) * Use a link to a "parent Evidence" instead of subclassing (google#296) * parent evidence * undo some simplifications for the sake of a simpler CL * Add some serialization * update docstring * Initialize attribute * set parent evidence if Evidence type is context dependant * don't pass parent_evidence at instantiation * undo linter stuff * comments * fix aim lib breaking tests * typo * Print version on start 3 (google#320) * Add files via upload * Delete turbiniactl.py * Delete turbiniactl.py * Add files via upload * Delete turbiniactl.py * Add files via upload * Update turbiniactl.py * Caps * Quick update to evidence docstrings (google#317) ... to disambiguate between _preprocess() and preprocess(). * Add Job filters (google#247) * Add job filters * fix docstrings. * update docstring * Get jobs filters working with new job manager * Refactor out FilterJobObjects into new method * Update YAPF * remove missed confict markers * Docstrings and renaming * Migrate job graph generator to use new manager (google#321) * Update Evidence local_path when it's saved (google#319) * Pin google-cloud-storage to 1.13.0 (google#326) Fixes google#325 Looks like google-cloud-storage was updated in: googleapis/google-cloud-python#6741 Which just got released as 1.13.1: https://pypi.org/project/google-cloud-storage/#history * Set image export to process all partitions (google#324) * Add --partitions all to image_export invocations * Fix typo * Explicitly set saved_paths to list (google#323) * Move version print after log level setup (google#322) * Move version print after log level setup * Remove extra whitespace * update the pystyle link (google#333) * Undefined name: Define 'unicode' in Python 3 (google#337) * Undefined name: Define 'unicode' in Python 3 __unicode()__ was removed in Python 3 because all __str__ are Unicode. [flake8](http://flake8.pycqa.org) testing of https://github.com/google/turbinia on Python 3.7.1 $ __flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics__ ``` ./tools/turbinia_job_graph.py:47:40: F821 undefined name 'unicode' parser.add_argument('filename', type=unicode, help='where to save the file') ^ 1 F821 undefined name 'unicode' 1 ``` * Placate PyLint * Added PSQ timeout to 1 week (google#336) * Error when worker version doesn't match server google#307 (google#327) * Added turbina_version to TurbinaTask * First approach * Changed to no rise error and return instead * Restored the run from run_wrapper * Changed format of strings * Changed words fixed line too long * bump version

aarontp requested review from berggren and Onager September 7, 2018 00:51

Onager suggested changes Sep 10, 2018

View reviewed changes

Onager mentioned this pull request Sep 10, 2018

Added job manager class #248

Closed

berggren previously requested changes Sep 21, 2018

View reviewed changes

aarontp force-pushed the job-filter branch from 8da90b3 to cbb50b7 Compare November 8, 2018 02:04

aarontp commented Nov 8, 2018

View reviewed changes

aarontp requested review from berggren and Onager November 14, 2018 19:18

aarontp assigned Onager Nov 14, 2018

Onager suggested changes Nov 21, 2018

View reviewed changes

aarontp added 7 commits December 6, 2018 15:54

Add job filters

5c924fa

fix docstrings.

59e38f1

update docstring

125ce0a

Get jobs filters working with new job manager

af2e661

Refactor out FilterJobObjects into new method

ef01d9e

Update YAPF

32369c5

remove missed confict markers

50134af

aarontp force-pushed the job-filter branch from 60f3c2e to 50134af Compare December 7, 2018 00:36

aarontp requested a review from Onager December 7, 2018 00:44

Onager suggested changes Dec 7, 2018

View reviewed changes

Docstrings and renaming

f853af7

aarontp commented Dec 7, 2018

View reviewed changes

aarontp requested a review from Onager December 7, 2018 19:24

Onager approved these changes Dec 10, 2018

View reviewed changes

aarontp merged commit 4a1afaf into master Dec 10, 2018

aarontp deleted the job-filter branch December 10, 2018 18:19

Add Job filters #247

Add Job filters #247

Conversation

aarontp commented Sep 7, 2018

aarontp commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarontp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarontp commented Nov 14, 2018

aarontp commented Nov 15, 2018

Onager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarontp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarontp commented Dec 10, 2018