Skip to content

Conversation

@andreyv
Copy link
Member

@andreyv andreyv commented Aug 17, 2019

This ensures proper error handling. In some places manual error reporting is also replaced with Python chained exceptions.

Manual error handling is left in loaders because multiple other places also return None instead of raising an exception. It can be refactored too if needed.


This change is Reviewable

@andreyv
Copy link
Member Author

andreyv commented Aug 17, 2019

One of the newly added checks fails:

ERROR [Worker,3 4 Worker::execute_job_group] Worker failed: Command '['isolate', '--cg', '--box-id=40', '--dir=/tmp=/tmp/cms-compile-50t1z18h/home:rw', '--run', '--', '/bin/chmod', '777', '-R', '/tmp']' returned non-zero exit status 1..

@andreyv
Copy link
Member Author

andreyv commented Aug 17, 2019

The output from isolate is something like

/bin/chmod: changing permissions of '/tmp': Operation not permitted
/bin/chmod: changing permissions of '/tmp/checker': Operation not permitted
/bin/chmod: changing permissions of '/tmp/user_output.txt': Operation not permitted
/bin/chmod: changing permissions of '/tmp/input.txt': Operation not permitted
/bin/chmod: changing permissions of '/tmp/correct_output.txt': Operation not permitted
Exited with error status 1

Looking at the home directory, the mentioned files are owned by the CMS user instead of the sandbox user. The chmod command runs as the sandbox user.

Looks like this behavior is intended. A comment can be added that the exit code should be ignored.

@codecov
Copy link

codecov bot commented Aug 17, 2019

Codecov Report

Merging #1128 (18959eb) into master (31599b6) will decrease coverage by 1.54%.
The diff coverage is 12.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1128      +/-   ##
==========================================
- Coverage   63.52%   61.98%   -1.55%     
==========================================
  Files         232      230       -2     
  Lines       17028    16611     -417     
==========================================
- Hits        10817    10296     -521     
- Misses       6211     6315     +104     
Flag Coverage Δ
functionaltests 45.71% <12.00%> (-1.70%) ⬇️
unittests 43.25% <0.00%> (-0.88%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cms/service/PrintingService.py 0.00% <0.00%> (ø)
cmscontrib/loaders/tps.py 8.50% <0.00%> (-0.74%) ⬇️
cmstaskenv/cmsMake.py 0.00% <0.00%> (ø)
cms/grading/Sandbox.py 67.74% <75.00%> (-1.08%) ⬇️
cms/grading/languages/python3_cpython.py 42.85% <0.00%> (-57.15%) ⬇️
cmscommon/commands.py 71.42% <0.00%> (-28.58%) ⬇️
cmscommon/datetime.py 49.05% <0.00%> (-20.65%) ⬇️
cms/server/admin/handlers/contestannouncement.py 36.00% <0.00%> (-10.67%) ⬇️
cms/service/ProxyService.py 57.97% <0.00%> (-8.17%) ⬇️
cms/server/admin/handlers/contestquestion.py 26.22% <0.00%> (-7.60%) ⬇️
... and 98 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 31599b6...c2427e1. Read the comment docs.

This ensures proper error handling. In some places manual error
reporting is also replaced with Python chained exceptions.

Manual error handling is left in loaders because multiple other
places also return None instead of raising an exception. It could
be refactored separately if needed.
@andreyv andreyv merged commit 4535c2f into cms-dev:master Feb 10, 2021
@andreyv andreyv deleted the subprocess-call branch February 10, 2021 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant