#1252 frontend: catch OSError when srpm_storage is full
Merged 4 years ago by praiskup. Opened 5 years ago by schlupov.
copr/ schlupov/copr upload_srcrpm_failure  into  master

@@ -51,6 +51,12 @@ 

      _code = 500

  

  

+ class InsufficientStorage(CoprHttpException):

+     """When there is not enough space left on the server for the src rpm."""

+     _default = "Not enough space left"

+     _code = 500

+ 

+ 

  class MalformedArgumentException(ValueError):

      pass

  

@@ -27,6 +27,7 @@ 

      ConflictingRequest,

      DuplicateException,

      InsufficientRightsException,

+     InsufficientStorage,

      MalformedArgumentException,

      UnrepeatableBuildException,

  )
@@ -504,11 +505,17 @@ 

          :param f_uploader(file_path): function which stores data at the given `file_path`

          :return:

          """

-         tmp = tempfile.mkdtemp(dir=app.config["STORAGE_DIR"])

-         tmp_name = os.path.basename(tmp)

-         filename = secure_filename(orig_filename)

-         file_path = os.path.join(tmp, filename)

-         f_uploader(file_path)

+         tmp = None

+         try:

+             tmp = tempfile.mkdtemp(dir=app.config["STORAGE_DIR"])

+             tmp_name = os.path.basename(tmp)

+             filename = secure_filename(orig_filename)

+             file_path = os.path.join(tmp, filename)

+             f_uploader(file_path)

+         except OSError as error:

+             if tmp:

+                 shutil.rmtree(tmp)

+             raise InsufficientStorage("Can not create storage directory for uploaded file: {}".format(str(error)))

  

          # make the pkg public

          pkg_url = "{baseurl}/tmp/{tmp_dir}/{filename}".format(

@@ -6,7 +6,13 @@ 

  from functools import wraps

  from werkzeug.datastructures import ImmutableMultiDict

  from coprs import app

- from coprs.exceptions import CoprHttpException, ObjectNotFound, AccessRestricted, ActionInProgressException

+ from coprs.exceptions import (

+     AccessRestricted,

+     ActionInProgressException,

+     CoprHttpException,

+     InsufficientStorage,

+     ObjectNotFound,

+ )

  from coprs.logic.complex_logic import ComplexLogic

  

  
@@ -36,6 +42,8 @@ 

          return self.handle_xxx(error)

  

      def handle_500(self, error):

+         if isinstance(error, InsufficientStorage):

+             return self.handle_xxx(error)

          if isinstance(error, ActionInProgressException):

              return self.handle_xxx(error)

          return self.respond("Request wasn't successful, there is probably a bug in the API code.", 500)

@@ -8,8 +8,12 @@ 

  from coprs import models

  

  from copr_common.enums import StatusEnum

- from coprs.exceptions import ActionInProgressException, InsufficientRightsException, \

-                              MalformedArgumentException, BadRequest

+ from coprs.exceptions import (ActionInProgressException,

+                               InsufficientRightsException,

+                               MalformedArgumentException,

+                               BadRequest,

+                               InsufficientStorage)

+ 

  from coprs.logic.actions_logic import ActionsLogic

  from coprs.logic.builds_logic import BuildsLogic

  
@@ -376,3 +380,16 @@ 

  

          assert "has no active chroots" in str(error.value)

          assert len(self.c1.active_copr_chroots) == 0

+ 

+     @pytest.mark.usefixtures("f_users", "f_coprs", "f_mock_chroots", "f_db")

+     def test_create_new_from_upload_no_space_left(self):

+         def f_uploader(file_path):

+             raise OSError("[Errno 28] No space left on device")

+ 

+         with pytest.raises(InsufficientStorage) as error:

+             BuildsLogic.create_new_from_upload(self.u1, self.c1, f_uploader,

+                                                "fake.src.rpm",

+                                                chroot_names=["fedora-18-x86_64"],

+                                                copr_dirname=None)

+         assert "Can not create storage directory for uploaded file" in str(error.value)

+         assert "[Errno 28] No space left on device" in str(error.value)

We should inform the user that there is no space left in /var/lib/copr/data/srpm_storage instead of message "Request wasn't successful, there is probably a bug in the API code."

rebased onto cd8f4bfedf32ae68fe63fc40faabd0c5fbe14232

5 years ago

error.args[1] seems weird, better would be error.strerror.

The ActionInProgressException class is somewhat broken. The second argument is later used in
.format(action="Can't create storage directory for this task").
... but since we nowhere have {action} used, ... it's pretty useless.

Can you use something like:

msg="Can not create storage directory for uploaded file: {}".format(error.strerror)
raise ActionInProgressException(msg, "unused")

?

Ok, even the use ActionInProgressException isn't correct; that should be
used for interrupting actions, not builds.

Metadata Update from @praiskup:
- Pull-request tagged with: needs-work

4 years ago

rebased onto a277dc8c614f1efe3f4153e5f98302384fffecd0

4 years ago

rebased onto 4f28fdf52b3bcc6d43f4fdc56c7b3feebf4e9cb4

4 years ago

Metadata Update from @praiskup:
- Pull-request untagged with: needs-work

4 years ago

Per mtg discussion, @schlupov is planning to take a look at this PR once more.

Metadata Update from @praiskup:
- Pull-request tagged with: needs-work

4 years ago

rebased onto 3e0fb0c19c6132ff45a09cf1e64dffd27d940c84

4 years ago

rebased onto 820f06dd2e73c3b5ec75bd32c714833d3374af69

4 years ago

Metadata Update from @schlupov:
- Pull-request untagged with: needs-work

4 years ago

rebased onto 385eeea6e079d90ca6deba2113390910cb1ba036

4 years ago

Reading once more ... it is pretty unlikely to get an error here (directory doesn't eat too much storage). Are we actually handling the f_uploader(file_path) correctly?

Nothing against the PR or @praiskup suggestions. I wanted to address just one thing that we talked about at the meeting.

If we wanted to make sure, that we don't store incomplete SRPMs and that they really have the size that they are supposed to have, I think we would have to update copr.v3.requests.FileRequest and put the expected file size into headers. Then the actual file size uploaded to frontend can be read with len(form.pkgs.data.read()) and then we can easily compare the two.

From app.config["STORAGE_DIR"] we know the name of the directory to be created. Maybe we could delete the directory while handling the InsufficientStorage exception, without checking what size is in the header. The file is still likely to be incomplete.

[Mon Aug 03 09:41:10.372141 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170] --- Logging error ---
[Mon Aug 03 09:41:10.377067 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170] Traceback (most recent call last):
[Mon Aug 03 09:41:10.377154 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/lib/python3.7/site-packages/flask/app.py", line 2292, in wsgi_app
[Mon Aug 03 09:41:10.377159 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     response = self.full_dispatch_request()
[Mon Aug 03 09:41:10.377166 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/lib/python3.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
[Mon Aug 03 09:41:10.377169 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     rv = self.handle_user_exception(e)
[Mon Aug 03 09:41:10.377175 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/share/copr/coprs_frontend/coprs/rest_api/__init__.py", line 44, in error_router
[Mon Aug 03 09:41:10.377179 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     return original_handler(e)
[Mon Aug 03 09:41:10.377184 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/lib/python3.7/site-packages/flask/app.py", line 1718, in handle_user_exception
[Mon Aug 03 09:41:10.377188 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     reraise(exc_type, exc_value, tb)
[Mon Aug 03 09:41:10.377193 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/lib/python3.7/site-packages/flask/_compat.py", line 35, in reraise
[Mon Aug 03 09:41:10.377196 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     raise value
[Mon Aug 03 09:41:10.377202 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/lib/python3.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
[Mon Aug 03 09:41:10.377205 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     rv = self.dispatch_request()
[Mon Aug 03 09:41:10.377210 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/lib/python3.7/site-packages/flask/app.py", line 1799, in dispatch_request
[Mon Aug 03 09:41:10.377242 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     return self.view_functions[rule.endpoint](**req.view_args)
[Mon Aug 03 09:41:10.377251 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/share/copr/coprs_frontend/coprs/views/misc.py", line 277, in decorated_function
[Mon Aug 03 09:41:10.377255 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     return f(*args, **kwargs)
[Mon Aug 03 09:41:10.377261 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/__init__.py", line 101, in file_upload_wrapper
[Mon Aug 03 09:41:10.377264 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     return f(*args, **kwargs)
[Mon Aug 03 09:41:10.377269 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 163, in create_from_upload
[Mon Aug 03 09:41:10.377273 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]     return process_creating_new_build(copr, form, create_new_build)
[Mon Aug 03 09:41:10.377278 2020] [wsgi:error] [pid 59569:tid 140225965299456] [remote 213.175.37.10:19170]   File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 267, in process_creating_new_build

While the uploaded file /var/lib/copr/data/srpm_storage/tmpn75oagmq/test.src.rpm has only 224M (expected is 500M). IOW, I'd view this as normal ENOSPC situation, and the exception we catch doesn't change anything I think.

Maybe we could delete the directory while handling the InsufficientStorage exception

Definitely, we seem to keep that forever.

I am proposing this patch. If you are able to produce a different exception that I am catching in the test, please paste it here, so I can write another test case.

From eb6512c552731e28fa6972e6b9eb05533ef979e1 Mon Sep 17 00:00:00 2001
From: Jakub Kadlcik <frostyx@email.cz>
Date: Fri, 7 Aug 2020 12:52:55 +0200
Subject: [PATCH] frontend: add test for running out of space when uploading a
 SRPM

This test is addressing an exception from
/var/log/copr-frontend/frontend.log

    File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 161, in create_new_build
        copr_dirname=form.project_dirname.data,
    File "/usr/share/copr/coprs_frontend/coprs/logic/builds_logic.py", line 516, in create_new_from_upload
        f_uploader(file_path)
    File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 157, in <lambda>
        f_uploader=lambda path: form.pkgs.data.save(path),
    File "/usr/lib/python3.7/site-packages/werkzeug/datastructures.py", line 2731, in save
        dst.close()
    OSError: [Errno 28] No space left on device
---
 .../coprs/logic/builds_logic.py               | 12 ++++++-----
 .../tests/test_logic/test_builds_logic.py     | 21 +++++++++++++++++--
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/frontend/coprs_frontend/coprs/logic/builds_logic.py b/frontend/coprs_frontend/coprs/logic/builds_logic.py
index c96000ef..67f6ebf8 100644
--- a/frontend/coprs_frontend/coprs/logic/builds_logic.py
+++ b/frontend/coprs_frontend/coprs/logic/builds_logic.py
@@ -507,12 +507,14 @@ class BuildsLogic(object):
         """
         try:
             tmp = tempfile.mkdtemp(dir=app.config["STORAGE_DIR"])
+            tmp_name = os.path.basename(tmp)
+            filename = secure_filename(orig_filename)
+            file_path = os.path.join(tmp, filename)
+            f_uploader(file_path)
         except OSError as error:
-            raise InsufficientStorage("Can not create storage directory for uploaded file: {}".format(error.strerror))
-        tmp_name = os.path.basename(tmp)
-        filename = secure_filename(orig_filename)
-        file_path = os.path.join(tmp, filename)
-        f_uploader(file_path)
+            shutil.rmtree(tmp)
+            raise InsufficientStorage("Can not create storage directory for uploaded file: {}"
+                                      .format(str(error)))

         # make the pkg public
         pkg_url = "{baseurl}/tmp/{tmp_dir}/{filename}".format(
diff --git a/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py b/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py
index 2defcd00..cc192a20 100644
--- a/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py
+++ b/frontend/coprs_frontend/tests/test_logic/test_builds_logic.py
@@ -8,8 +8,12 @@ from sqlalchemy.orm.exc import NoResultFound
 from coprs import models

 from copr_common.enums import StatusEnum
-from coprs.exceptions import ActionInProgressException, InsufficientRightsException, \
-                             MalformedArgumentException, BadRequest
+from coprs.exceptions import (ActionInProgressException,
+                              InsufficientRightsException,
+                              MalformedArgumentException,
+                              BadRequest,
+                              InsufficientStorage)
+
 from coprs.logic.actions_logic import ActionsLogic
 from coprs.logic.builds_logic import BuildsLogic

@@ -376,3 +380,16 @@ class TestBuildsLogic(CoprsTestCase):

         assert "has no active chroots" in str(error.value)
         assert len(self.c1.active_copr_chroots) == 0
+
+    @pytest.mark.usefixtures("f_users", "f_coprs", "f_mock_chroots", "f_db")
+    def test_create_new_from_upload_no_space_left(self):
+        def f_uploader(file_path):
+            raise OSError("[Errno 28] No space left on device")
+
+        with pytest.raises(InsufficientStorage) as error:
+            BuildsLogic.create_new_from_upload(self.u1, self.c1, f_uploader,
+                                               "fake.src.rpm",
+                                               chroot_names=["fedora-18-x86_64"],
+                                               copr_dirname=None)
+        assert "Can not create storage directory for uploaded file" in str(error.value)
+        assert "[Errno 28] No space left on device" in str(error.value)
-- 
2.26.2

I know that @praiskup proposed dropping our custom exceptions and simplifying the error handing but I would be for not complicating the PR here and do it in a separate one. Personally, I am interested in refactoring the error handing and then we can better decide if to drop our custom exceptions or not.

That patch makes the exception handling reasonable....

The traceback I postged before was form error_log, while frontend.log was empty. There was some delay because I can see the traceback now:

2020-08-03 09:41:10,334 [ERROR][/usr/lib/python3.7/site-packages/flask/app.py:1761|app:log_exception] Exception on /api_3/build/create/upload [POST]
Traceback (most recent call last):
  File "/usr/lib/python3.7/site-packages/flask/app.py", line 2292, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/lib/python3.7/site-packages/flask/app.py", line 1815, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/share/copr/coprs_frontend/coprs/rest_api/__init__.py", line 44, in error_router
    return original_handler(e)
  File "/usr/lib/python3.7/site-packages/flask/app.py", line 1718, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/lib/python3.7/site-packages/flask/_compat.py", line 35, in reraise
    raise value
  File "/usr/lib/python3.7/site-packages/flask/app.py", line 1813, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/lib/python3.7/site-packages/flask/app.py", line 1799, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args) 
  File "/usr/share/copr/coprs_frontend/coprs/views/misc.py", line 277, in decorated_function
    return f(*args, **kwargs)
  File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/__init__.py", line 101, in file_upload_wrapper
    return f(*args, **kwargs)
  File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 163, in create_from_upload 
    return process_creating_new_build(copr, form, create_new_build)
  File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 267, in process_creating_new_build
    build = create_new_build()
  File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 161, in create_new_build   
    copr_dirname=form.project_dirname.data,
  File "/usr/share/copr/coprs_frontend/coprs/logic/builds_logic.py", line 511, in create_new_from_upload
    f_uploader(file_path)
  File "/usr/share/copr/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py", line 157, in <lambda>
    f_uploader=lambda path: form.pkgs.data.save(path),
  File "/usr/lib/python3.7/site-packages/werkzeug/datastructures.py", line 2728, in save
    copyfileobj(self.stream, dst, buffer_size) 
  File "/usr/lib64/python3.7/shutil.py", line 82, in copyfileobj
    fdst.write(buf)
OSError: [Errno 28] No space left on device

It means with that patch you would catch the exception correctly. I thnink you just need to check tmp is defined before you start removing it.

rebased onto 4c31a3d1820ca0c455a9d121d0c932a9ddaaf8be

4 years ago

I updated the code but it still doesn't work, it creates the directory with the partial src rpm. The command shutil.rmtree(error.filename) is not in the right place, there must be some other place in the code where the directory with the partial src rpm is created.

@schlupov I guess you didn't apply the patch but changed the code manually? That's not the best approach, you forgot to add the unit test.

Next time, you can easily save the patch as a file and then just do git am foo.patch and it will apply the commit.

is not in the right place, there must be some other place in the code where the directory with the partial src rpm is created.

Just guessing now, but isn't it possible that the still uploading has some modified name and it is renamed once the upload finishes? At least this is how it works when I download things ... In that case it would maybe help to remove the tmp directory, not the error.filename. I would try to define tmp = None before the try: block starts so it has a bigger scope and can be accessed in except block. then I would do

except OSError as error:
    if tmp:
        shutil.rmtree(tmp)

Not saying it would fix this particular issue, but I would prefer this over the current implementation anyway.

In that case it would maybe help to remove the tmp directory, not the error.filename. I would try to define tmp = None before the try: block starts so it has a bigger scope and can be accessed in except block.

I've already tried it, it doesn't work, the tmp variable is not set in the except block.

Setting tmp before the try/except block should indeed work.

rebased onto 2da1e97f8a0891273b3c996b67f14a00c859274c

4 years ago

rebased onto 835155b

4 years ago

I tried it on a devel instance and it works. Thanks to @frostyx for help.

Pull-Request has been merged by praiskup

4 years ago