Automount: neuro run --volume=ALL #980

atemate · 2019-08-22T17:15:15Z

closes #974

asvetlov · 2019-08-22T19:35:33Z

Please merge #977, rebase against the master, and ask for review again.
Keeping in mind what is related to the PR and what not is a challenge. I would like to not resolve this pussle.

atemate · 2019-08-23T08:59:30Z

Please merge #977, rebase against the master, and ask for review again.

Lately, when I was having multiple consequent PRs one depending on another, I always added the notification "WARNING: depends on #977" to the description of all non-ready-for-review PRs. Sorry for misunderstanding. From now on, I will not open a non-WIP PRs if they are not ready to be compared against master.

codecov · 2019-08-26T12:53:48Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f6bca6d). Click here to learn what that means.
The diff coverage is 96.87%.

@@           Coverage Diff            @@
##             master    #980   +/-   ##
========================================
  Coverage          ?   92.5%           
========================================
  Files             ?      37           
  Lines             ?    4283           
  Branches          ?     647           
========================================
  Hits              ?    3962           
  Misses            ?     213           
  Partials          ?     108

Impacted Files	Coverage Δ
neuromation/cli/job.py	`90.85% <96.87%> (ø)`

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 f6bca6d...a41ef6b. Read the comment docs.

codecov · 2019-08-26T12:53:48Z

Codecov Report

Merging #980 into master will increase coverage by 4.85%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
+ Coverage    87.3%   92.16%   +4.85%     
==========================================
  Files          37       37              
  Lines        4568     4592      +24     
  Branches      681      686       +5     
==========================================
+ Hits         3988     4232     +244     
+ Misses        452      245     -207     
+ Partials      128      115      -13

Impacted Files	Coverage Δ
neuromation/cli/job.py	`89.15% <96.96%> (+7.01%)`	⬆️
neuromation/api/config_factory.py	`90.38% <0%> (+1.28%)`	⬆️
neuromation/api/jobs.py	`94.84% <0%> (+2.12%)`	⬆️
neuromation/cli/formatters/jobs.py	`97.97% <0%> (+2.42%)`	⬆️
neuromation/api/url_utils.py	`92.42% <0%> (+4.54%)`	⬆️
neuromation/cli/formatters/config.py	`100% <0%> (+5.26%)`	⬆️
neuromation/cli/utils.py	`88.74% <0%> (+5.62%)`	⬆️
neuromation/cli/main.py	`69.8% <0%> (+7.42%)`	⬆️
neuromation/cli/asyncio_utils.py	`76.59% <0%> (+14.89%)`	⬆️
... and 4 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 4fc431d...3788049. Read the comment docs.

atemate · 2019-08-26T12:55:02Z

tests/cli/test_job.py

+@pytest.mark.parametrize(
+    "env_var", ["NP_STORAGE_ROOT_MOUNTPOINT", "NP_STORAGE_ROOT_MOUNTPOINT=value"]
+)
+def test_build_env_reserved_env_var_conflict_passed_as_parameter(env_var: str) -> None:


apart from these two tests, build_env is not covered

should we properly cover it then?

sure. but not in this PR. Issue: #995

neuromation/cli/job.py

dalazx · 2019-08-27T10:06:34Z

neuromation/cli/job.py

@@ -877,6 +873,55 @@ def format_fail(job: str, reason: Exception) -> str:
    return job


+async def _build_volumes(
+    root: Root, input_volumes: Sequence[str], env_dict: Dict[str, str]


are input_volumes unique?

no, it's direct user input.

should we handle edge cases such as same volumes etc? (prob out of scope)

looking at the code, I can say that this case is handled properly: the result collection volumes is of type Set (but in order to reduce number of loop iterations, I changed input array to the type of Set as well).

However, this is a good case indeed, neuro run --volume=ALL --volume=<custom>, so since --volume=ALL already mounts all possible volumes, I changed the code to forbid this option together with any other option -- thank you for pointing this out!

neuromation/cli/job.py

dalazx · 2019-08-27T10:09:28Z

neuromation/cli/job.py

+                    )
+                for perm in available:
+                    root_dir = (
+                        perm.uri.host if perm.uri.host != root.username else "home"


not sure the home link is what we want really. I'd rather have a separate link (an additional mount) instead of renaming the owners home dir. also, we could easily introduce an environment var to address this (e.g. $HOME).

In this PR, we made this decision to mount user home directory to /var/storage/home not to /var/storage/username in order to be compliant with --volume=HOME in order not to confuse users. I don't think we need to have both mounts.

In addition, in a scheme storage://username/path/to/dir, we do not use username as a location of user's home directory, technically, in NFS it can be anywhere (for example, not in /username but /home/username), so I think it's good practice to mount user dir to /var/storage/home.

I think this is wrong as this is not corresponding to what neuro ls storage:/ returns.

this is question of taste, let's ask our users (cc @mariyadavydova).

Currently:

--volume=HOME: storage://alice/ -> /var/storage/home/

We need to implement --volume=ALL. Suppose, alice is using this command. There are 2 options:

replace user's name with home in order to be consistent with --volume=HOME (and not cause surprises for our users):

storage://alice/dir1 -> /var/storage/home/dir1

storage://bob/dir2 -> /var/storage/bob/dir2

as @dalazx proposes, do not change anything and thus simplify algorithm for --volume=ALL:

storage://alice/dir1 -> /var/storage/alice/dir1

storage://bob/dir2 -> /var/storage/bob/dir2

@ayushkovskiy This is a very good idea! We can introduce something like this:

> neuro storage protect storage:project/data > neuro run ... --volume storage:project/data:/var/storage/data:rw ... Warning: You are trying to mount storage:project/data with read-write access, while this directory is marked as protected. Access us downgraded to read-only. To get read-write access for this directory please remove the protection mode with neuro storage protect --remove storage:project/data

looks too complicated in my opinion.

I think it should be an option for "serious teams, concerned about data security". I mean, in most cases you can simply watch out your commands and use proper access modifiers. But if you are really concerned, you may additionally use such an option as a safety net. This may be especially relevant for teams, sharing the same data.

I agree we should print a warning for all rw volumes

neuromation/cli/job.py

atemate requested review from asvetlov, dalazx and serhiy-storchaka August 22, 2019 17:15

astaff added CLIents platform labels Aug 22, 2019

atemate mentioned this pull request Aug 22, 2019

Add e2e test on neuro run --volume=ALL with shared resources neuro-inc/platform-e2e#22

Closed

atemate force-pushed the ay/volume-all branch from 6387ac8 to a41ef6b Compare August 26, 2019 12:53

atemate commented Aug 26, 2019

View reviewed changes

dalazx reviewed Aug 27, 2019

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

dalazx reviewed Aug 27, 2019

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

dalazx reviewed Aug 27, 2019

View reviewed changes

asvetlov reviewed Aug 27, 2019

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

dalazx reviewed Aug 27, 2019

View reviewed changes

atemate mentioned this pull request Aug 27, 2019

Cover build_env() #995

Closed

atemate commented Aug 27, 2019

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

This was referenced Aug 28, 2019

Volume automount conflict: --volume=ALL and --pass-config #1002

Closed

Print mounted volumes on neuro status #1003

Closed

Deprecate neuro run --volume=HOME #1009

Closed

review: final version

fa0f49d

atemate force-pushed the ay/volume-all branch from 58e1992 to fa0f49d Compare August 28, 2019 16:24

Artem Yushkovskiy added 4 commits August 28, 2019 19:28

add changelog + --pass-config

1476a4d

fix

98131d3

remove test, cleanup

9fd0796

remove todo

c1723fe

asvetlov reviewed Aug 30, 2019

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

asvetlov approved these changes Aug 30, 2019

View reviewed changes

Artem Yushkovskiy added 2 commits August 30, 2019 16:05

review

3ce44b1

Merge branch 'master' into ay/volume-all

1992719

atemate mentioned this pull request Aug 30, 2019

Print warning for mounting volumes in rw mode while --volume=ALL #1018

Closed

asvetlov reviewed Aug 30, 2019

View reviewed changes

neuromation/cli/job.py Outdated Show resolved Hide resolved

Artem Yushkovskiy added 2 commits August 30, 2019 16:26

clenaup

99c5474

Merge branch 'master' into ay/volume-all

3788049

atemate merged commit 0518c51 into master Sep 2, 2019

atemate deleted the ay/volume-all branch September 2, 2019 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automount: neuro run --volume=ALL #980

Automount: neuro run --volume=ALL #980

atemate commented Aug 22, 2019 •

edited

Loading

asvetlov commented Aug 22, 2019

atemate commented Aug 23, 2019

codecov bot commented Aug 26, 2019

codecov bot commented Aug 26, 2019 •

edited

Loading

atemate Aug 26, 2019

dalazx Aug 27, 2019

atemate Aug 27, 2019

dalazx Aug 27, 2019

atemate Aug 27, 2019

dalazx Aug 27, 2019

atemate Aug 28, 2019

dalazx Aug 27, 2019

atemate Aug 27, 2019

atemate Aug 27, 2019

dalazx Aug 27, 2019

atemate Aug 28, 2019 •

edited

Loading

mariyadavydova Aug 29, 2019 •

edited

Loading

dalazx Aug 29, 2019

mariyadavydova Aug 29, 2019 •

edited

Loading

atemate Aug 29, 2019

atemate Aug 30, 2019

Automount: neuro run --volume=ALL #980

Automount: neuro run --volume=ALL #980

Conversation

atemate commented Aug 22, 2019 • edited Loading

asvetlov commented Aug 22, 2019

atemate commented Aug 23, 2019

codecov bot commented Aug 26, 2019

Codecov Report

codecov bot commented Aug 26, 2019 • edited Loading

Codecov Report

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

atemate Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

mariyadavydova Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariyadavydova Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atemate commented Aug 22, 2019 •

edited

Loading

codecov bot commented Aug 26, 2019 •

edited

Loading

atemate Aug 28, 2019 •

edited

Loading

mariyadavydova Aug 29, 2019 •

edited

Loading

mariyadavydova Aug 29, 2019 •

edited

Loading