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

Attempt at fixing file system not mounted IGV crash #3129

Merged
merged 11 commits into from
Feb 7, 2022
Merged

Conversation

northwestwitch
Copy link
Member

Tentative fix #3128, only the crash part.

I couldn't reproduce locally, so any idea is welcome! My idea is to pass an empty path to the function that servers the file, so it will return "file not found" instead. Basically it will except here:

return abort(404, "File not found")

Testing on cg-vm1 server (Clinical Genomics Stockholm)

Prepare for testing

  1. Make sure the PR is pushed and available on Docker Hub
  2. ssh your.email@cg-vm1.scilifelab.se
  3. sudo -iu hiseq.clinical
  4. ssh localhost
  5. (optional) Find out which scout branch is currently deployed on cg-vm1: podman ps
  6. Stop the service with current deployed branch: systemctl --user stop scout.target
  7. Start the scout service with the branch to test: systemctl --user start scout@<this_branch>
  8. Make sure the branch is deployed: systemctl --user status scout.target

How to test:

  1. how to test it, possibly with real cases/data

Expected outcome:
The functionality should be working
Take a screenshot and attach or copy/paste the output.

Review:

  • code approved by
  • tests executed by

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #3129 (fc45414) into main (b7b895c) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3129   +/-   ##
=======================================
  Coverage   83.84%   83.84%           
=======================================
  Files         287      287           
  Lines       16486    16486           
=======================================
  Hits        13823    13823           
  Misses       2663     2663           
Impacted Files Coverage Δ
scout/server/blueprints/alignviewers/views.py 74.07% <0.00%> (ø)

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 b7b895c...fc45414. Read the comment docs.

@dnil
Copy link
Collaborator

dnil commented Feb 7, 2022

Good start! I took the liberty of adding a try-except, to remove a crash on missing non-ranged files as well.
We should really think a little about if we don't want to add a layer of security so users can't just snatch each others files, but that can be a separate issue. As long as it doesn't happen by mistake, it is not immediately a bug. We have a reasonable amount of trust in the users.

@northwestwitch
Copy link
Member Author

if we don't want to add a layer of security so users can't just snatch each others files,

I'll think about something. Shouldn't be that hard, no?

@northwestwitch
Copy link
Member Author

if we don't want to add a layer of security so users can't just snatch each others files,

To do it properly, this needs a lot fo refactoring, so perhaps let's have it in another PR?

@northwestwitch northwestwitch requested a review from dnil February 7, 2022 13:14
Copy link
Collaborator

@dnil dnil left a comment

Choose a reason for hiding this comment

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

Right!

@dnil dnil merged commit 38c0d6f into main Feb 7, 2022
@northwestwitch northwestwitch deleted the avoid_file_system_crash branch February 7, 2022 13:56
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.

Fix crashes and tighten down file access for file endpoint.
3 participants