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

Error when worker version doesn't match server #307 #327

Merged
merged 6 commits into from
Jan 8, 2019

Conversation

fpiedrah
Copy link
Contributor

@fpiedrah fpiedrah commented Dec 12, 2018

Fixes #307

  • Add turbina.__version__ to the TurbinaTask class as an attribute
  • Validate versions in TurbiniaTask.run_wrapper() before TurbiniaTask.run() is ever ran

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@fpiedrah
Copy link
Contributor Author

@aarontp Hi! I've been looking at the code and I've got new questions

  1. I've added TurbiniaTask.turbina_version = turbina.__version__ this will give us the task version inside the class. I need to validate the matching server information in TurbiniaTask.run_wrapper() how would I get the server version from this task?
  2. This validation should run before TurbiniaTask.run() once this mismatch is found the TurbiniaTask.run_wrapper() should yield an error?
  3. Do I need to add version to the TurbiniaServer class?

Sorry for the amount of questions, but I rather understand the whole thing so I can be useful by contributing to other issues too. : )

@aarontp
Copy link
Member

aarontp commented Dec 13, 2018

Hello: No worries, I think it's better to ask questions than get too far and have to change a bunch of code too, :)

  1. The TurbiniaTask is actually created server side, so the version that is an attribute of that object will actually be the version that the server is running. Then the Worker can compare the task.version directly against turbinia.version (which should be the version that the Worker started with).

  2. Yes, it will result in an error, but the way the error will be sent back to the server is by returning the error in the TurbiniaTaskResult object (should be self.result from the Task code). You can add it as an error, and also set a relevant status message (this status is what will bubble up to the user in a turbiniactl status command).

  3. I don't think you'll need to add it there, but let me know if you have any other questions related to this.

@fpiedrah
Copy link
Contributor Author

@aarontp I keep being confuse with some things

  1. TurbiniaTask is the server side portion of the whole thing, but the remote execution happens with the Turbinia TaskManager does this mean that when I compare versions I should do that using the TurbiniaTaskResult object returned from running TurbiniaTask.setup() meaning that TurbiniaTaskResult should have a field with the version? I'm confuse because I don get from where I could get the remote objects version.

  2. Now this is clear now, with the returned TurbiniaTaskResult, that will be the self.result, I'll have to add a meaningful TurbiniaTaskResult.status and TurbiniaTaskResult.error for the version mismatch.

Thanks for the help!

Copy link
Member

@aarontp aarontp left a comment

Choose a reason for hiding this comment

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

Hello:

I've made a couple comments on the PR, and here are some responses to your earlier questions:

  1. So the TurbiniaTask is created on the server, but it actually gets scheduled via a task scheduler to execute on a different worker. Since it gets created server-side, when you initially set the turbinia version in TurbiniaTask.__init__(), that will be the server version. Then, when the worker starts, it will have it's own turbinia.version variable that will be the Worker version, and that can be compared like you're doing in the run_wrapper(). The TurbiniaTaskResult() won't need to have the version as an attribute.
  2. Yep, sounds good. I left comments on the code before I saw your comments, so I left a similar comment there.

TL;DR: I think you have the comparison correct, we just need to update it to return the results with the error (as you mention).

turbinia/workers/__init__.py Outdated Show resolved Hide resolved
turbinia/workers/__init__.py Outdated Show resolved Hide resolved
turbinia/workers/__init__.py Outdated Show resolved Hide resolved
@fpiedrah
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Member

@aarontp aarontp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aarontp aarontp merged commit 9a31566 into google:master Jan 8, 2019
ericzinnikas pushed a commit to ericzinnikas/turbiniafb that referenced this pull request Jan 10, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants