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

cancel analysis #1305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cancel analysis #1305

wants to merge 1 commit into from

Conversation

jstucke
Copy link
Collaborator

@jstucke jstucke commented Nov 25, 2024

  • new feature: cancel analyses that are currently in progress (also works with updates)
    • analyses and extractions that are currently underway will not be canceled
    • instead, following analyses and extractions will not be scheduled
    • can be triggered on the "system health" page in the web interface (REST: TODO)

@jstucke jstucke requested a review from dorpvom November 25, 2024 12:57
@jstucke jstucke self-assigned this Nov 25, 2024
@jstucke jstucke force-pushed the cancel-analysis-poc branch from 61dc184 to ff10fb4 Compare November 25, 2024 13:05
@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.93%. Comparing base (bd5bdb0) to head (2679946).

Files with missing lines Patch % Lines
src/intercom/back_end_binding.py 57.14% 3 Missing ⚠️
src/scheduler/analysis/scheduler.py 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
- Coverage   92.21%   91.93%   -0.29%     
==========================================
  Files         377      376       -1     
  Lines       23068    21047    -2021     
==========================================
- Hits        21273    19350    -1923     
+ Misses       1795     1697      -98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jstucke jstucke force-pushed the cancel-analysis-poc branch from ff10fb4 to 5dff8f2 Compare November 25, 2024 15:44
Copy link
Collaborator

@maringuu maringuu left a comment

Choose a reason for hiding this comment

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

How can analysis be resumed?
Would I have to hit the re-analyze button?

Also, I think there needs to be an endpoint in the rest API to cancel an anylsis.
Furthermore, I think it should be queryable whether an analysis was canceled.
If., e.g., analyzing LFwC which contains thousands of firmware images and I canceled some of them, I would want to exclude them from my farther evaluation.

Comment on lines 36 to 38
self.manager = Manager()
# this object tracks only the FW objects and not the status of the individual files
self.currently_analyzed = self.manager.dict()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be private?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes -> fixed

Comment on lines 87 to 97
class FwAnalysisStatus:
files_to_unpack: Set[str]
files_to_analyze: Set[str]
files_to_unpack: set[str]
files_to_analyze: set[str]
total_files_count: int
hid: str
analysis_plugins: Dict[str, int]
analysis_plugins: dict[str, int]
start_time: float = field(default_factory=time)
completed_files: Set[str] = field(default_factory=set)
completed_files: set[str] = field(default_factory=set)
total_files_with_duplicates: int = 1
unpacked_files_count: int = 1
analyzed_files_count: int = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future please, create a separate commit for (unrelated) formatting changes.
Having them in the same commit makes reviewing harder as you have to deliberatly ignore some parts of the code but not all ofc.

@jstucke
Copy link
Collaborator Author

jstucke commented Nov 27, 2024

How can analysis be resumed?

The analysis cannot be resumed. The files were dropped from scheduling at that point (it would not really be feasible to keep them lying around somewhere for when we might need them again (if that ever happens))

Would I have to hit the re-analyze button?

You would have to use "Redo analysis" as there is currently no way to tell which files were fully extracted/analyzed. We could consider adding some kind of flag to the metadata which could also be displayed in the web interface.

Also, I think there needs to be an endpoint in the rest API to cancel an anylsis.

Yes, that is planned (but in another PR)

Furthermore, I think it should be queryable whether an analysis was canceled.

There should be a new entry recently_canceled_analyses in the status data, so this should be possible already (the FW will be automatically removed from this data after the configured time, though (just like the "recently_finished_analyses"))

@jstucke jstucke force-pushed the cancel-analysis-poc branch 2 times, most recently from 1927858 to 91fc6f0 Compare December 3, 2024 08:25
@jstucke jstucke force-pushed the cancel-analysis-poc branch from 91fc6f0 to 2679946 Compare December 3, 2024 14:16
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.

3 participants