Skip to content

allow writing into Data Volume in job submission #9

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.

Sign up for GitHub

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

Open
mtaghiza wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

allow writing into Data Volume in job submission #9

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
346b9ca
allow writing into Data Volume in job submission
mtaghiza Mar 24, 2020
59348ba
add name to data volume definition in job submitting methods
mtaghiza Apr 3, 2020
804b3ce
fixed typo
mtaghiza Apr 3, 2020
b869120
fixed documentation, open jobs, and allowed actions checks
mtaghiza Apr 21, 2020
9d0f19a
added getJobQueues function, and improved way of setting/defining res…
mtaghiza May 12, 2020
127fcac
small fix
mtaghiza May 13, 2020
4781501
allow including all data volumes and user volumes by specifying 'all'…
mtaghiza Mar 9, 2021
6db2371
raise exception with None values for user/data volumes in docker job …
mtaghiza Mar 9, 2021
b2ad78c
include 'all' option in missing remaining methods
mtaghiza Mar 9, 2021
ace43f8
fifed 'all' assignement
mtaghiza Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions py2/SciServer/Jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,15 +410,27 @@ def submitNotebookJob(notebookPath, dockerComputeDomain=None, dockerImageName=No
datVols = [];
if dataVolumes is None:
Copy link
Contributor

@glemson glemson Mar 9, 2021

Choose a reason for hiding this comment

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

I know this is not part of this PR, but I believe that when no dataVolumes are explicitly provided, whether as [] or as None, we should NOT mount any data volumes.

Copy link
Contributor

@glemson glemson Mar 9, 2021

Choose a reason for hiding this comment

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

however, in any case I would NOT make them writable, even though the DV may be writable by default. I would be happy if users are explicit in what thye want us to do, and if by accident (not providing explicit list) they mount all data volumes, at least they should not be writable.

Copy link

@jonashaase jonashaase Mar 9, 2021

Choose a reason for hiding this comment

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

Currently behavior is to mount everything if no volumes are specified though. I think that does make for a much more convenient experience for the user instead having to specify a long list of dictionaries of user and data volumes.

Copy link
Contributor Author

@mtaghiza mtaghiza Mar 9, 2021

Choose a reason for hiding this comment

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

@jonashaase : could you confirm you agree with the changes I just made?

for vol in dockerComputeDomain.get('volumes'):
datVols.append({'name': vol.get('name')});
if 'write' in vol.get('allowedActions'):
datVols.append({'name': vol.get('name'), 'needsWriteAccess': True});
else:
datVols.append({'name': vol.get('name'), 'needsWriteAccess': False});

else:
for dVol in dataVolumes:
Copy link
Contributor

@glemson glemson Mar 9, 2021

Choose a reason for hiding this comment

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

it is still possible for the input argument dataVolumes to be None. In that case this code would throw an error I think.
maybe protect against that by setting dataVolumes to [] if the input was None?
Same for userVolumes.

found = False;
for vol in dockerComputeDomain.get('volumes'):
if vol.get('name') == dVol.get('name'):
found = True;
datVols.append({'name': vol.get('name')});
if (dVol.get('needsWriteAccess')):
if dVol.get('needsWriteAccess') == True and 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});
else:
if 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});

if not found:
raise Exception("Data volume '" + dVol.get('name') + "' not found within Compute domain")
Expand Down Expand Up @@ -507,9 +519,9 @@ def submitShellCommandJob(shellCommand, dockerComputeDomain = None, dockerImageN
for uVol in userVolumes:
found = False;
for vol in dockerComputeDomain.get('userVolumes'):
if vol.get('name') == uVol.get('name'):
if vol.get('name') == uVol.get('name') and vol.get('rootVolumeName') == uVol.get('rootVolumeName') and vol.get('owner') == uVol.get('owner'):
found = True;
if (uVol.has_key('needsWriteAccess')):
if (uVol.get('needsWriteAccess')):
if uVol.get('needsWriteAccess') == True and 'write' in vol.get('allowedActions'):
uVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
Expand All @@ -526,15 +538,27 @@ def submitShellCommandJob(shellCommand, dockerComputeDomain = None, dockerImageN
datVols = [];
if dataVolumes is None:
for vol in dockerComputeDomain.get('volumes'):
datVols.append({'name': vol.get('name')});
if 'write' in vol.get('allowedActions'):
datVols.append({'name': vol.get('name'), 'needsWriteAccess': True});
else:
datVols.append({'name': vol.get('name'), 'needsWriteAccess': False});

else:
for dVol in dataVolumes:
found = False;
for vol in dockerComputeDomain.get('volumes'):
if vol.get('name') == dVol.get('name'):
found = True;
datVols.append({'name': vol.get('name')});
if (dVol.get('needsWriteAccess')):
if dVol.get('needsWriteAccess') == True and 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});
else:
if 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});

if not found:
raise Exception("Data volume '" + dVol.get('name') + "' not found within Compute domain")
Expand Down
34 changes: 29 additions & 5 deletions py3/SciServer/Jobs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,15 +410,27 @@ def submitNotebookJob(notebookPath, dockerComputeDomain=None, dockerImageName=No
datVols = [];
if dataVolumes is None:
for vol in dockerComputeDomain.get('volumes'):
datVols.append({'name': vol.get('name')});
if 'write' in vol.get('allowedActions'):
datVols.append({'name': vol.get('name'), 'needsWriteAccess': True});
else:
datVols.append({'name': vol.get('name'), 'needsWriteAccess': False});

else:
for dVol in dataVolumes:
found = False;
for vol in dockerComputeDomain.get('volumes'):
if vol.get('name') == dVol.get('name'):
found = True;
datVols.append({'name': vol.get('name')});
if (dVol.get('needsWriteAccess')):
if dVol.get('needsWriteAccess') == True and 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});
else:
if 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});

if not found:
raise Exception("Data volume '" + dVol.get('name') + "' not found within Compute domain")
Expand Down Expand Up @@ -507,7 +519,7 @@ def submitShellCommandJob(shellCommand, dockerComputeDomain = None, dockerImageN
for uVol in userVolumes:
found = False;
for vol in dockerComputeDomain.get('userVolumes'):
if vol.get('name') == uVol.get('name'):
if vol.get('name') == uVol.get('name') and vol.get('rootVolumeName') == uVol.get('rootVolumeName') and vol.get('owner') == uVol.get('owner'):
found = True;
if (uVol.get('needsWriteAccess')):
if uVol.get('needsWriteAccess') == True and 'write' in vol.get('allowedActions'):
Expand All @@ -526,15 +538,27 @@ def submitShellCommandJob(shellCommand, dockerComputeDomain = None, dockerImageN
datVols = [];
if dataVolumes is None:
for vol in dockerComputeDomain.get('volumes'):
datVols.append({'name': vol.get('name')});
if 'write' in vol.get('allowedActions'):
datVols.append({'name': vol.get('name'), 'needsWriteAccess': True});
else:
datVols.append({'name': vol.get('name'), 'needsWriteAccess': False});

else:
for dVol in dataVolumes:
found = False;
for vol in dockerComputeDomain.get('volumes'):
if vol.get('name') == dVol.get('name'):
found = True;
datVols.append({'name': vol.get('name')});
if (dVol.get('needsWriteAccess')):
if dVol.get('needsWriteAccess') == True and 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});
else:
if 'write' in vol.get('allowedActions'):
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': True});
else:
datVols.append({'userVolumeId': vol.get('id'), 'needsWriteAccess': False});

if not found:
raise Exception("Data volume '" + dVol.get('name') + "' not found within Compute domain")
Expand Down