Some work on isolate#992
Conversation
Mostly, this is to give isolate directories in a format that it expects; in particular, in should be relative and out should be absolute. Isolate was silently modifying them in the same way we are doing here explicitly.
Codecov Report
@@ Coverage Diff @@
## master #992 +/- ##
==========================================
+ Coverage 45.97% 45.99% +0.02%
==========================================
Files 231 231
Lines 17744 17747 +3
==========================================
+ Hits 8158 8163 +5
+ Misses 9586 9584 -2
Continue to review full report at Codecov.
|
gollux
left a comment
gollux
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @stefano-maggiolo and @lerks)
cms/grading/Sandbox.py, line 911 at r6 (raw file):
# self.path will be bind-mounted inside the sandbox as inner_temp_dir. # We use a subdirectory of /tmp so that the sandbox will create a
Really?
cms/grading/languages/rust.py, line 58 at r6 (raw file):
return [ # rustc requires a /tmp directory, so we create it if necessary. ["/bin/mkdir", "-p", "/tmp"],
Maybe it makes sense that isolate creates /tmp by default?
stefano-maggiolo
left a comment
stefano-maggiolo
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @stefano-maggiolo, @codacy-bot, and @lerks)
cms/grading/Sandbox.py, line 911 at r6 (raw file):
Previously, gollux (Martin Mareš) wrote…
Really?
Ahah I was trying a few things and thought this might be a shortcut :)
Turns out that then /tmp is not writable but the sandbox user, so it's still no good.
cms/grading/languages/rust.py, line 58 at r6 (raw file):
Previously, gollux (Martin Mareš) wrote…
Maybe it makes sense that isolate creates /tmp by default?
Maybe. Btw, this mkdir also fails because there is no permission to create /tmp/.
The constrictions we have are:
- we need /tmp to exists;
- we want the sandbox user to be able to write stuff in /tmp;
- we want to bind-mount an external directory that might be a subdir of /tmp with the same name inside the sandbox.
For 3, I think the proper solution is to specify an in-sandbox name that we control - I was just trying to postpone doing this work tbh...
For 1,2, I see several solutions CMS-side:
- do the same thing we do for /tmp (before this PR), so: create an outer dir as 0700, create a subdir as 0777 and bind-mount this subdir as /tmp in the sandbox;
- just bind-mount /tmp, once 3 is solved;
- ask the compilers to use a different tmp dir.
In addition there is yours which IIUC is to have isolate create a 0777 /tmp by default.
I guess I'll suck it up and do the proper thing for 3. For 1 and 2 I think the isolate fix is reasonable and would make it easier for us. WDYT?
|
I'm going to retire this PR, and post an updated one with a better fix soon. In short, the new PR ensures that all map have a constant mount point in the sandbox that we control and doesn't clash. We mount our inner_temp_dir for the sandbox to /tmp, so that compilers that need it have /tmp (this is the existing behaviour, is not great, but waiting for @gollux to see if we can unload the creation of /tmp to isolate). |
3 participants
Mostly, a quick fix for the build.
This change is