From bcfc69a0e0c76f8db3d4a3654766a3d592f6c0c3 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 1/9] frontend: better identify the build submitter


Major motivation
https://pagure.io/pagure/issue/4569

The problem is that the 'agent' field isn't set to the user who
actually triggered the fedora-messaging event, but to artificial
string "pagure" (a non-existing user e.g. in pagure.io).

Therefore we have to look at better data, which is in the
pullrequest.user.name field.  This field is not in the "push" event
so we keep using the "agent" there (in this case it is set correctly).

---

diff --git a/frontend/coprs_frontend/pagure_events.py b/frontend/coprs_frontend/pagure_events.py
index ba919a8..91cfc44 100755
--- a/frontend/coprs_frontend/pagure_events.py
+++ b/frontend/coprs_frontend/pagure_events.py
@@ -137,14 +137,17 @@ def event_info_from_pr_comment(data, base_url):
         'branch_to': data['msg']['pullrequest']['branch'],
         'start_commit': data['msg']['pullrequest']['commit_start'],
         'end_commit': data['msg']['pullrequest']['commit_stop'],
-        'agent': data['msg']['agent'],
+        'user': data['msg']['pullrequest']['user']['name'],
     })
 
 
 def event_info_from_pr(data, base_url):
     """
     Message handler for new pull-request opened in pagure.
-    Topic: ``*.pagure.pull-request.new``
+    Topics:
+    - ``*.pagure.pull-request.new``
+    - ``*.pagure.pull-request.updated``
+    - ``*.pagure.pull-request.rebased``
     """
     return munch.Munch({
         'object_id': data['msg']['pullrequest']['id'],
@@ -159,7 +162,7 @@ def event_info_from_pr(data, base_url):
         'branch_to': data['msg']['pullrequest']['branch'],
         'start_commit': data['msg']['pullrequest']['commit_start'],
         'end_commit': data['msg']['pullrequest']['commit_stop'],
-        'agent': data['msg']['agent'],
+        'user': data['msg']['pullrequest']['user']['name'],
     })
 
 
@@ -181,7 +184,11 @@ def event_info_from_push(data, base_url):
         'branch_to': data['msg']['branch'],
         'start_commit': data['msg']['start_commit'],
         'end_commit': data['msg']['end_commit'],
-        'agent': data['msg']['agent'],
+        # There's no better user identification of the committer.  It can be
+        # normal user, or some bot.  We use this value for sandboxing so the
+        # value doesn't play a security role too much -- pushed stuff should be
+        # safe to build no matter what.
+        'user': data['msg']['agent'],
     })
 
 
@@ -288,7 +295,7 @@ class build_on_fedmsg_loop():
                         event_info.object_type,
                         event_info.object_id,
                         scm_object_url,
-                        "{}user/{}".format(base_url, event_info.agent),
+                        "{}user/{}".format(base_url, event_info.user),
                     )
                     if build:
                         log.info('\t -> {}'.format(build.to_dict()))
diff --git a/frontend/coprs_frontend/tests/test_pagure_events.py b/frontend/coprs_frontend/tests/test_pagure_events.py
index 3e47039..9fc1af1 100644
--- a/frontend/coprs_frontend/tests/test_pagure_events.py
+++ b/frontend/coprs_frontend/tests/test_pagure_events.py
@@ -29,7 +29,13 @@ class TestPagureEvents(CoprsTestCase):
                     "url_path": "test/copr/copr",
                 },
                 'status': 'Open',
-                "comments": []
+                "comments": [],
+                'user': {
+                    "fullname": "John Doe",
+                    "url_path": "user/jdoe",
+                    "full_url": "https://src.fedoraproject.org/user/jdoe",
+                    "name": "jdoe"
+                },
             }
         }
     }
@@ -131,6 +137,7 @@ class TestPagureEvents(CoprsTestCase):
     def test_positive_event_info_from_pr(self):
         event_info = event_info_from_pr(self.data, self.base_url)
         assert event_info.base_clone_url == "https://pagure.io/test/copr/copr"
+        assert event_info.user == "jdoe"
 
     def test_positive_event_info_from_push(self):
         self.data['msg'] = {
@@ -142,6 +149,7 @@ class TestPagureEvents(CoprsTestCase):
         self.data['msg']['repo'] = {"fullname": "test", "url_path": "test"}
         event_info = event_info_from_push(self.data, self.base_url)
         assert event_info.base_clone_url == "https://pagure.io/test"
+        assert event_info.user == "test"
 
     @mock.patch('pagure_events.helpers.raw_commit_changes')
     @mock.patch('pagure_events.get_repeatedly')

From 79a861db0d10a0c21e90980a59816907a0ebec8a Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 2/9] frontend: minimize the /backend/pending-tasks/ json


The only dict fields that the current build dispatcher needs for the
fair build queue planning are those that we left behind the
'short == True' condition.  For more info about what is needed and what
not see backend/copr_backend/rpm_builds.py:BuildQueueTask

This should slightly optimize the slow pending-tasks route that was the
bottleneck during last queue peak >= 20k jobs.

---

diff --git a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
index 7c11d6c..2f89b13 100644
--- a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
+++ b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
@@ -84,7 +84,25 @@ def dist_git_upload_completed():
     return flask.jsonify({"updated": True})
 
 
-def get_build_record(task, short=False):
+def get_build_record(task, for_backend=False):
+    """
+    Transform an ORM BuildChroot instance into a Python dictionary that is later
+    converted to a JSON string and sent (as task build instructions) to Copr
+    Backend or Copr Builder machine.
+
+    The Backend needs only a limited amount of information to correctly schedule
+    the task processing (what to build, when, how, where...), whilst Builder
+    needs the full information to properly perform the build.
+
+    The build queue may be rather large (tens of thousands tasks) in some peak
+    situations, so we try to really limit the amount of data processed and sent
+    to Backend (array).  OTOH, Builder's single-row queries are rather cheap and
+    thus we don't have to pay attention to such optimizations.
+
+    :param for_backend: True if the data are consumed by Backend (smaller
+        dictionary output), False if the data are consumed by Builder (full task
+        info).
+    """
     if not task:
         return None
 
@@ -94,11 +112,18 @@ def get_build_record(task, short=False):
             "task_id": task.task_id,
             "build_id": task.build.id,
             "project_owner": task.build.copr.owner_name,
+            "sandbox": task.build.sandbox,
+            "background": bool(task.build.is_background),
+            "chroot": task.mock_chroot.name,
+        }
+
+        if for_backend:
+            return build_record
+
+        build_record.update({
             "project_name": task.build.copr_name,
             "project_dirname": task.build.copr_dirname,
             "submitter": task.build.submitter[0],
-            "sandbox": task.build.sandbox,
-            "chroot": task.mock_chroot.name,
             "repos": task.build.repos,
             "memory_reqs": task.build.memory_reqs,
             "timeout": task.build.timeout,
@@ -110,14 +135,7 @@ def get_build_record(task, short=False):
             "uses_devel_repo": task.build.copr.devel_mode,
             "isolation": task.build.isolation,
             "fedora_review": task.build.copr.fedora_review,
-        }
-
-
-        if task.build.is_background:
-            build_record['background'] = True
-
-        if short:
-            return build_record
+        })
 
         copr_chroot = CoprChrootsLogic.get_by_name_safe(task.build.copr, task.mock_chroot.name)
         modules = copr_chroot.module_setup_commands
@@ -144,7 +162,13 @@ def get_build_record(task, short=False):
     return build_record
 
 
-def get_srpm_build_record(task):
+def get_srpm_build_record(task, for_backend=False):
+    """
+    Transform an ORM Build instance (how to build SRPM) into a Python dictionary
+    that is later converted to a JSON string and sent (as task build
+    instructions) to Copr Backend or Copr Builder machine.  For more info see
+    get_build_record() documentation.
+    """
     if not task:
         return None
 
@@ -158,15 +182,21 @@ def get_srpm_build_record(task):
             "task_id": task.task_id,
             "build_id": task.id,
             "project_owner": task.copr.owner_name,
-            "project_name": task.copr_name,
-            "project_dirname": task.copr_dirname,
-            "submitter": task.submitter[0],
             "sandbox": task.sandbox,
-            "source_type": task.source_type,
-            "source_json": task.source_json,
             "chroot": chroot,
         }
 
+        if for_backend:
+            return build_record
+
+        build_record.update({
+            "source_type": task.source_type,
+            "source_json": task.source_json,
+            "submitter": task.submitter[0],
+            "project_name": task.copr_name,
+            "project_dirname": task.copr_dirname,
+        })
+
     except Exception as err:
         app.logger.exception(err)
         return None
@@ -249,12 +279,11 @@ def pending_jobs():
     """
     Return the job queue.
     """
-    srpm_tasks = [build for build in
-                  BuildsLogic.get_pending_srpm_build_tasks(for_backend=True)
-                  if not build.blocked]
     build_records = (
-        [get_srpm_build_record(task) for task in srpm_tasks] +
-        [get_build_record(task, short=True)
+        [get_srpm_build_record(task, for_backend=True)
+         for task in BuildsLogic.get_pending_srpm_build_tasks(for_backend=True)
+         if not task.blocked] +
+        [get_build_record(task, for_backend=True)
          for task in BuildsLogic.get_pending_build_tasks(for_backend=True)]
     )
     log.info('Selected build records: {}'.format(build_records))

From cdc7628d94ffbc49c566f1f433d272275a42aedb Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 3/9] frontend: provide the forgotten background flag


Until now, all source RPMs were non-background because we forgot to pass
down the background flag.  I.e. it fixes residual unfairness in
scheduler.

---

diff --git a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
index 2f89b13..5f0241f 100644
--- a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
+++ b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
@@ -183,6 +183,7 @@ def get_srpm_build_record(task, for_backend=False):
             "build_id": task.id,
             "project_owner": task.copr.owner_name,
             "sandbox": task.sandbox,
+            "background": bool(task.is_background),
             "chroot": chroot,
         }
 

From 38dde8fe31f92e7b5c83434e5d06915eab617b29 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 4/9] frontend: cache 'Batch.finished' and 'Batch.blocked'


Batches caused a serious slowdown on /backend/pending-jobs/ before.  The
reason was that every single task there was checked whether it is or is
not blocked/finished.  It for BuildRoots means that
'build_chroot.build.batch.builds[].build_chroots[]' was evaluated for
every single row, and ditto for every single row in "source builds".
The worst thing wasn't CPU calculation, but that the info was mostly
lazy-loaded on demand without practical reason.  This is now mitigated.

The trick implemented in this patch is that only **one batch** in each
**tree of batches** has to be expensively checked for the finished
state.  That's given by those facts:
  - Batch is blocked if parent batch is blocked, and ditto for grand
    parents.  So in most cases we can go to descendants and check for
    blocked state there only.
  - We don't have to check for finished batches all the time, but we can
    cache (1 hour in this commit) the finished status.  This is because
    we never add new builds into already finished batch.
IOW, we only have to check the status for the batch in tree that is
currently being processed.

While we are on it, it doesn't even make much sense to re-calculate the
'Build.finished' property all the time.  If batch B depends on batch A,
then asking if 'B.finished' means that we have to calculate 'A.finished'
right-away.  Later, when explicitly asking for 'A.finished', we don't
have to recalculate (among possibly thousands of builds and
build_chroots in batch A) A.finished - we can use the in-memory cache.
That's why 'Batch.finished' uses 'Flask-Caching', and only if not cached
we fallback to 'Batch.finished_slow'.

---

diff --git a/frontend/coprs_frontend/coprs/__init__.py b/frontend/coprs_frontend/coprs/__init__.py
index 28c7e56..75f0c3a 100644
--- a/frontend/coprs_frontend/coprs/__init__.py
+++ b/frontend/coprs_frontend/coprs/__init__.py
@@ -72,6 +72,7 @@ cache = Cache(app, config={
     'CACHE_REDIS_HOST': cache_rcp.host,
     'CACHE_REDIS_PORT': cache_rcp.port,
 })
+app.cache = cache
 
 from coprs.views import admin_ns
 from coprs.views.admin_ns import admin_general
diff --git a/frontend/coprs_frontend/coprs/models.py b/frontend/coprs_frontend/coprs/models.py
index 4bdae9f..dd4b30c 100644
--- a/frontend/coprs_frontend/coprs/models.py
+++ b/frontend/coprs_frontend/coprs/models.py
@@ -1268,7 +1268,10 @@ class Build(db.Model, helpers.Serializer):
 
     @property
     def blocked(self):
-        return bool(self.batch and self.batch.blocked_by and not self.batch.blocked_by.finished)
+        """
+        Detect if the batch we are in is blocked.
+        """
+        return bool(self.batch and self.batch.blocked)
 
     @property
     def persistent(self):
@@ -1980,16 +1983,68 @@ class Batch(db.Model):
     blocked_by_id = db.Column(db.Integer, db.ForeignKey("batch.id"), nullable=True)
     blocked_by = db.relationship("Batch", remote_side=[id])
 
+    _is_finished = None
+
     @property
-    def finished(self):
+    def finished_slow(self):
+        """
+        Check if this batch is finished by iterating through all the contained
+        builds.
+        """
+        cache_timeout = 3600
+        redis_cache_id = "batch_finished_{}".format(self.id)
+
+        if app.cache.get(redis_cache_id):
+            # prolong the cache after the access
+            app.cache.set(redis_cache_id, True, timeout=cache_timeout)
+            return True
+
         if not self.builds:
             # no builds assigned to this batch (yet)
             return False
-        return all([b.finished for b in self.builds])
+
+        # Some Batches are rather large;  use the all+map pair here, not a list
+        # comprehension, to escape the loop as soon as possible on the first
+        # miss (comprehension would go through all builds unnecessarily)
+        if all(map(lambda x: x.finished, self.builds)):
+            # nothing can switch finished batch to non-finished state, cache it
+            app.cache.set(redis_cache_id, True, cache_timeout)
+            return True
+        return False
+
+    @property
+    def finished(self):
+        """
+        Same as self.finished_slow, but doesn't require re-calculation for all
+        the builds, buildchroots, states, etc.  Or checking Redis caches.
+        """
+        if self._is_finished is None:
+            self._is_finished = self.finished_slow
+        return self._is_finished
+
+    @property
+    def blocked(self):
+        """
+        Batch is blocked when the parent batch is not yet finished.
+        """
+        if not self.blocked_by_id:
+            return False
+
+        # a) we are blocked if parent is blocked, or ...
+        if self.blocked_by.blocked:
+            # Optimization, checking for "blocked" is often cheaper than
+            # checking for "finished" because batches tend to contain too many
+            # builds (and transitively build_chroots).  IOW, since in each batch
+            # tree is only one batch being processed - it doesn't make sense to
+            # re-calculate 'finished' state for all of them.
+            return True
+
+        # b) ... when parent is not yet finished
+        return not self.blocked_by.finished
 
     @property
     def state(self):
-        if self.blocked_by and not self.blocked_by.finished:
+        if self.blocked:
             return "blocked"
         return "finished" if self.finished else "processing"
 

From e543402a158be367080898606146b9ca2ab77993 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 5/9] frontend: bettter preload /pending-jobs/ queries


First, we don't have to do the 'joinedload' action for normal single-row
queries (like /get-srpm-build-task/<51816>) where we (a) can rely on
lazy-loads and we (b) need a full info about the task (all fields).
So let's stop doing so (when for_backend=False, aka "for builder").

Then, backend needs only a very limited info about the task, like the
task sandbox, project name, etc. so it can correctly calculate the task
priority.  It doesn't make sense to load all the fields into ORM:
https://docs.sqlalchemy.org/en/13/faq/performance.html .
Therefore, when `for_backend=True`, we only lazy load those fields that
are really necessary.

While I'm on it, I also made the 'joinedload' more careful - so really
no other queries (except for batches, see previous patch) is needed.

---

diff --git a/frontend/coprs_frontend/coprs/logic/builds_logic.py b/frontend/coprs_frontend/coprs/logic/builds_logic.py
index d9658bb..699f851 100644
--- a/frontend/coprs_frontend/coprs/logic/builds_logic.py
+++ b/frontend/coprs_frontend/coprs/logic/builds_logic.py
@@ -8,7 +8,7 @@ import requests
 
 from sqlalchemy.sql import text
 from sqlalchemy.sql.expression import not_
-from sqlalchemy.orm import joinedload, selectinload
+from sqlalchemy.orm import joinedload, selectinload, load_only
 from sqlalchemy import func, desc, or_, and_
 from sqlalchemy.sql import false,true
 from werkzeug.utils import secure_filename
@@ -298,10 +298,23 @@ class BuildsLogic(object):
     def get_pending_srpm_build_tasks(cls, background=None, for_backend=False):
         query = (
             models.Build.query
+            .join(models.Copr)
             .filter(models.Build.canceled == false())
             .filter(models.Build.source_status.in_(cls._todo_states(for_backend)))
             .order_by(models.Build.is_background.asc(), models.Build.id.asc())
         )
+        if for_backend:
+            query = query.options(
+                load_only("is_background", "source_type", "source_json",
+                          "submitted_by"),
+                # from copr project info we only need the project name
+                joinedload('copr').load_only("user_id", "group_id", "name")
+                .joinedload('user', 'group'),
+                # who submitted the build?
+                joinedload('user').load_only("username"),
+                # is this blocked?
+                joinedload('batch'),
+            )
         if background is not None:
             query = query.filter(models.Build.is_background == (true() if background else false()))
         return query
@@ -321,11 +334,22 @@ class BuildsLogic(object):
             # configuration which can be changed in the middle of the
             # BuildChroot processing.
             .join(models.Package, models.Package.id == models.Build.package_id)
-            .options(joinedload('build').joinedload('copr_dir'),
-                     joinedload('build').joinedload('package'))
             .filter(models.Build.canceled == false())
             .filter(models.BuildChroot.status.in_(cls._todo_states(for_backend)))
             .order_by(models.Build.is_background.asc(), models.Build.id.asc()))
+
+        if for_backend:
+            query = query.options(
+                joinedload('build').load_only("is_background", "submitted_by",
+                                              "source_json", "source_type")
+                # from copr project info we only need the project name
+                .joinedload('copr').load_only("user_id", "group_id", "name")
+                .joinedload('user', 'group'),
+                joinedload('mock_chroot'),
+                # submitter
+                joinedload('build').load_only('id').joinedload('user').load_only("username")
+            )
+
         if background is not None:
             query = query.filter(models.Build.is_background == (true() if background else false()))
         return query
diff --git a/frontend/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py b/frontend/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py
index a38c9e2..193ec0d 100644
--- a/frontend/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py
+++ b/frontend/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py
@@ -3,9 +3,12 @@ import json
 from unittest import mock, skip
 import pytest
 
+from flask_sqlalchemy import get_debug_queries
+
 from copr_common.enums import BackendResultEnum, StatusEnum, DefaultActionPriorityEnum
 from tests.coprs_test_case import CoprsTestCase, new_app_context
 from coprs.logic.builds_logic import BuildsLogic
+from coprs import app
 
 
 class TestGetBuildTask(CoprsTestCase):
@@ -98,6 +101,48 @@ class TestWaitingBuilds(CoprsTestCase):
         assert self.b3.id not in ids
         assert {self.b2.id, self.b4.id}.issubset(ids)
 
+    @pytest.mark.usefixtures("f_users", "f_coprs", "f_mock_chroots", "f_builds", "f_db")
+    def test_build_jobs_performance(self):
+        self.b2.source_status = StatusEnum("pending")
+        self.b2.is_background = True
+        for bch in self.b3_bc:
+            bch.status = StatusEnum("pending")
+        self.db.session.commit()
+
+        with app.app_context():
+            r = self.tc.get("/backend/pending-jobs/")
+            data = json.loads(r.data.decode("utf-8"))
+            dq = get_debug_queries()
+
+        # Only two queries should occur.  If you happen to see higher number
+        # here, please check the get_pending_srpm_build_tasks and
+        # get_pending_build_tasks methods to enhance the preloaded data.
+        assert len(dq) == 2
+
+        # No redundant data should occur in the output.  Only what BE needs.
+        assert data == [{
+            'build_id': 2,
+            'task_id': '2',
+            'background': True,
+            'chroot': None,
+            'project_owner': 'user1',
+            'sandbox':
+            'user1/foocopr--user2',
+        }, {
+            'build_id': 3,
+            'task_id': '3-fedora-17-x86_64',
+            'background': False,
+            'chroot': 'fedora-17-x86_64',
+            'project_owner': 'user2',
+            'sandbox': 'user2/foocopr--user2',
+        }, {
+            'build_id': 3,
+            'task_id': '3-fedora-17-i386',
+            'background': False,
+            'chroot': 'fedora-17-i386',
+            'project_owner': 'user2',
+            'sandbox': 'user2/foocopr--user2',
+        }]
 
 # status = 0 # failure
 # status = 1 # succeeded

From 859c932479c6d682e15f95cea76d4bceab12a892 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 6/9] frontend: don't schedule blocked BuildChroots


Previously we only blocked builds of source RPMs that were blocked by
parent batch.  Not the blocked binary RPM builds.

Fortunately, to consider BuildChroot ready for build (read the
get_pending_build_tasks method), we **usually* have to first flip it's
state from 'waiting' to 'pending' (we do so at the same time when
'build.source_status' is flipped to 'succeeded' state).

There's though at least one exception to this rule -- the situation when
we resubmit a build that was previously done from uploaded SRPM.
Because the uploaded SRPM is not available anymore, we don't rely on
that and we simply rebuild from the imported sources in our proxy
dist-git.

This is rarely exposed use-case in reality, so that's probably the
reason we didn't notice this problem before.  I mean the problem when
build in dependant batch is built before its prerequisites.

---

diff --git a/frontend/coprs_frontend/coprs/logic/builds_logic.py b/frontend/coprs_frontend/coprs/logic/builds_logic.py
index 699f851..3540492 100644
--- a/frontend/coprs_frontend/coprs/logic/builds_logic.py
+++ b/frontend/coprs_frontend/coprs/logic/builds_logic.py
@@ -347,7 +347,9 @@ class BuildsLogic(object):
                 .joinedload('user', 'group'),
                 joinedload('mock_chroot'),
                 # submitter
-                joinedload('build').load_only('id').joinedload('user').load_only("username")
+                joinedload('build').load_only('id').joinedload('user').load_only("username"),
+                # preload also batch info
+                joinedload('build').load_only('id').joinedload('batch'),
             )
 
         if background is not None:
diff --git a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
index 5f0241f..b94a7ba 100644
--- a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
+++ b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
@@ -285,7 +285,8 @@ def pending_jobs():
          for task in BuildsLogic.get_pending_srpm_build_tasks(for_backend=True)
          if not task.blocked] +
         [get_build_record(task, for_backend=True)
-         for task in BuildsLogic.get_pending_build_tasks(for_backend=True)]
+         for task in BuildsLogic.get_pending_build_tasks(for_backend=True)
+         if not task.build.blocked]
     )
     log.info('Selected build records: {}'.format(build_records))
     return flask.jsonify(build_records)

From 7dcf3259c9db805eaf3e83de8832f580f6f26607 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 7/9] frontend: test serveral batch build bugs


See previous three commits, we test that we don't do too many SQL
queries under certain very concrete situation, that blocked BuildChroots
are really blocked, and that "background" flag is also propagated in
the source rpm task info.

---

diff --git a/frontend/coprs_frontend/tests/request_test_api.py b/frontend/coprs_frontend/tests/request_test_api.py
index a75a657..e430d5c 100644
--- a/frontend/coprs_frontend/tests/request_test_api.py
+++ b/frontend/coprs_frontend/tests/request_test_api.py
@@ -249,7 +249,7 @@ class API3Requests(_RequestsInterface):
         resp = self.post(route, {"package_name": pkgname})
         return resp
 
-    def rebuild_package(self, project, pkgname):
+    def rebuild_package(self, project, pkgname, build_options=None):
         """ Rebuild one package in a given project using API """
         route = "/api_3/package/build"
         rebuild_data = {
@@ -257,6 +257,7 @@ class API3Requests(_RequestsInterface):
             "projectname": project,
             "package_name": pkgname,
         }
+        rebuild_data.update(self._form_data_from_build_options(build_options))
         return self.post(route, rebuild_data)
 
 
diff --git a/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py b/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
index 069c22e..c31e9a4 100644
--- a/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
+++ b/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
@@ -2,9 +2,11 @@
 Tests for working with Batches
 """
 
+import json
 import pytest
+from flask_sqlalchemy import get_debug_queries
 from copr_common.enums import StatusEnum
-from coprs import models
+from coprs import app, models
 from coprs.exceptions import BadRequest
 from coprs.logic.batches_logic import BatchesLogic
 from tests.coprs_test_case import CoprsTestCase
@@ -14,7 +16,10 @@ from tests.coprs_test_case import CoprsTestCase
 class TestBatchesLogic(CoprsTestCase):
     batches = None
 
-    def _prepare_project_with_batches(self):
+    def _prepare_project_with_batches(self, more=None):
+        """
+        [1, 3] <= [2, 4] (<= [5] ... per "more")
+        """
         self.web_ui.new_project("test", ["fedora-rawhide-i386"])
         assert models.Copr.query.count() == 1
         self.api3.submit_url_build("test")
@@ -32,6 +37,33 @@ class TestBatchesLogic(CoprsTestCase):
         for batch in batches:
             assert len(batch.builds) == 2
 
+        if not more:
+            return
+
+        self.web_ui.create_distgit_package("test", "testpkg")
+
+        # submit two more batches for that package ^^
+        for i in range(more):
+            after_id = i + 4
+            self.api3.rebuild_package(
+                "test", "testpkg",
+                # assure that BuildChroot is pre-generated
+                build_options={
+                    'chroots': ["fedora-rawhide-i386"],
+                    "after_build_id": after_id,
+                })
+
+        # Emulate the SRPM upload "resubmit" action - the source_status is
+        # succeeded, and all build chroots are pending.  See BuildsLogic.add()
+        # and the skip_import argument.  Such BuildChroot is immediately ready
+        # to be taken (except that it is blocked by the Batch 2).  It would be
+        # nice to train on a real uploaded build here instead, of course.
+        self.batches = batches = models.Batch.query.all()
+        self.batches[1+more].builds[0].source_status = StatusEnum("succeeded")
+        self.batches[1+more].builds[0].build_chroots[0].status = StatusEnum("pending")
+        assert len(batches) == 2 + more
+        self.db.session.commit()
+
     def _succeed_first_batch(self):
         for build in self.batches[0].builds:
             build.source_status = StatusEnum("succeeded")
@@ -102,3 +134,59 @@ class TestBatchesLogic(CoprsTestCase):
             BatchesLogic.get_batch_or_create(1, user, modify=True)
         assert "Build 1 is not yet in any batch" in str(error)
         assert "'user2' doesn't have the build permissions" in str(error)
+
+    def test_batched_build_queue_sql_performance(self):
+        more_bchs = 5
+        with app.app_context():
+            self._prepare_project_with_batches(more=more_bchs)
+            self._succeed_first_batch()
+
+        with app.app_context():
+            r = self.tc.get("/backend/pending-jobs/")
+            data = json.loads(r.data.decode("utf-8"))
+            dq = get_debug_queries()
+
+        # Be very careful if you have to bump the number here.  Any O(N)
+        # slowdown means huge penalty on /bakcend/pending-jobs/ route.
+        #
+        # 1. Get user1 info (for self.test_client).
+        # 2. Large query for Source builds (get_pending_srpm_build_tasks).
+        # 3. Get info about Batch 1 - *expected* (even though it's in "finished"
+        #    state and is cached in Redis), this is triggered by lazy-loads from
+        #    accessing objects from query 1 => the source builds in the query
+        #    are from other batches (e.g. 3), but to check if Batch 3 is blocked
+        #    (already loaded) we have to check if Batch 2 is blocked (loaded)
+        #    and thus also if Batch 1 is blocked.  Because we ask for
+        #    'batch_1.blocked' it needs to be loaded **now** because it is not
+        #    yet (it is not loaded because all builds there are already
+        #    finished).
+        # 4. Read all builds (lazy) from Batch 2 to get Build statuses.  This is
+        #    the only Batch where we need to iterate through Builds (as there's
+        #    only one tree of batches).
+        # 5. Read BuildChroots (lazy) from ^^ to get statuses.
+        # 6. Large query for BuildChroots (get_pending_build_tasks).
+        # 7.-N. The last batch (ID=2+more_bchs) contains one "ready" BuildChroot
+        #    task, which we know by parent batch (ID=2+more_bchs-1).  So parent
+        #    batch, and its parent batch, etc. needs to be loaded till Batch 2
+        #    which already is loaded.  So e.g. if more_bchs == 5, we have 7 batches
+        #    in total, batch 1/2 and 7 is pre-loaded, but we have to load batch
+        #    6, 5, 4 and 3 now (more_bchs-1 queries).
+        assert len(dq) == 6-1+more_bchs
+
+        # First batch is done, second is processing and third is blocked.  Only
+        # the builds from second batch are present.
+        assert data == [{
+            'background': False,
+            'build_id': 2,
+            'chroot': None,
+            'project_owner': 'user1',
+            'sandbox': 'user1/test--user1',
+            'task_id': '2',
+        }, {
+            'background': False,
+            'build_id': 4,
+            'chroot': None,
+            'project_owner': 'user1',
+            'sandbox': 'user1/test--user1',
+            'task_id': '4',
+        }]

From 61316b3fa91575ddbc7903b92ba362208300dd6d Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 8/9] frontend: cache Batch objects to prevent garbage-collection


.. that would otherwise lead to re-loading the Batch data from database,
which is especially expensive when the batches are rather large.

---

diff --git a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
index b94a7ba..446b1c2 100644
--- a/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
+++ b/frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
@@ -280,14 +280,26 @@ def pending_jobs():
     """
     Return the job queue.
     """
+
+    # This code is really expensive, and takes a long time when there is a large
+    # build queue.  We want to avoid repeated reload of models.Batch data, and
+    # for that we need to have it strongly referenced.
+    cache = set()
+
+    def build_ready(build):
+        """ Is the build blocked? """
+        cache.add(build.batch)
+        return not build.blocked
+
     build_records = (
         [get_srpm_build_record(task, for_backend=True)
          for task in BuildsLogic.get_pending_srpm_build_tasks(for_backend=True)
-         if not task.blocked] +
+         if build_ready(task)] +
         [get_build_record(task, for_backend=True)
          for task in BuildsLogic.get_pending_build_tasks(for_backend=True)
-         if not task.build.blocked]
+         if build_ready(task.build)]
     )
+
     log.info('Selected build records: {}'.format(build_records))
     return flask.jsonify(build_records)
 
diff --git a/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py b/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
index c31e9a4..0d85c33 100644
--- a/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
+++ b/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
@@ -165,13 +165,21 @@ class TestBatchesLogic(CoprsTestCase):
         #    only one tree of batches).
         # 5. Read BuildChroots (lazy) from ^^ to get statuses.
         # 6. Large query for BuildChroots (get_pending_build_tasks).
-        # 7.-N. The last batch (ID=2+more_bchs) contains one "ready" BuildChroot
-        #    task, which we know by parent batch (ID=2+more_bchs-1).  So parent
-        #    batch, and its parent batch, etc. needs to be loaded till Batch 2
-        #    which already is loaded.  So e.g. if more_bchs == 5, we have 7 batches
-        #    in total, batch 1/2 and 7 is pre-loaded, but we have to load batch
-        #    6, 5, 4 and 3 now (more_bchs-1 queries).
-        assert len(dq) == 6-1+more_bchs
+        #
+        # The last batch (ID=2+more_bchs) contains one "ready" BuildChroot task
+        # (the srpm upload emulation, see _prepare_project_with_batches()) which
+        # is only blocked by parent batch.  But because we cache Batch objects
+        # in pending_jobs() method - they are preloaded and we can be sure that
+        # we don't have to re-load the batch data to check if that is finished.
+        expected = 6
+        if expected != len(dq):
+            print()
+            for n, query in enumerate(dq):
+                print("==== query {} ====".format(n))
+                print(query)
+            print()
+
+        assert len(dq) == expected
 
         # First batch is done, second is processing and third is blocked.  Only
         # the builds from second batch are present.

From a89db04e29c34952914d20a79ad03e48a080ec12 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Mar 11 2021 16:22:15 +0000
Subject: [PATCH 9/9] frontend: one large test for Batched builds


Try to fill DB with 24k of builds, all being batched while in each
project (6 projects) there's one batch being processed with 1000
builds (20 build chroots and 980 source builds).

---

diff --git a/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py b/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
index 0d85c33..a53e9d5 100644
--- a/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
+++ b/frontend/coprs_frontend/tests/test_logic/test_batch_logic.py
@@ -3,6 +3,7 @@ Tests for working with Batches
 """
 
 import json
+import time
 import pytest
 from flask_sqlalchemy import get_debug_queries
 from copr_common.enums import StatusEnum
@@ -198,3 +199,121 @@ class TestBatchesLogic(CoprsTestCase):
             'sandbox': 'user1/test--user1',
             'task_id': '4',
         }]
+
+    def _add_one_large_batch(self, projectname, builds=1000, after_build=None):
+        # create the first build in batch
+        bo = {
+            # pre-create two build chroots
+            "chroots": ["fedora-rawhide-i386", "fedora-18-x86_64"],
+        }
+        if after_build:
+            bo["after_build_id"] = after_build
+        res = self.api3.submit_url_build(projectname, build_options=bo)
+        batch_build_id = json.loads(res.data)['items'][0]['id']
+
+        # create the batch by grouping two builds
+        res = self.web_ui.submit_url_build(projectname, build_options={
+            "with_build_id": batch_build_id,
+        })
+
+        self.db.session.commit()
+
+        build = models.Build.query.get(batch_build_id)
+        batch = build.batch
+        mock_chroot = build.build_chroots[0].mock_chroot
+
+        b_objs = []
+        bch_objs = []
+        for counter in range(builds-2):
+            new_b = models.Build()
+            new_b.pkgs = 'https://example.com/some.src.rpm'
+            new_b.submitted_on = time.time()
+            new_b.source_json = '{"url": "https://example.com/some.src.rpm"}'
+            new_b.srpm_url = 'https://example.com/some.src.rpm'
+            new_b.batch_id = batch.id
+            new_b.canceled = 0
+            new_b.copr_dir_id = build.copr_dir_id
+            new_b.copr_id = build.copr_id
+
+            new_bch = models.BuildChroot()
+            new_bch.build_id = counter + build.id + 2  # two build ready
+            new_bch.mock_chroot_id = mock_chroot.id
+
+            if not after_build and not counter % 50:
+                # a few builds in the unblocked batch are done
+                new_bch.status = StatusEnum("pending")
+                new_b.source_status = StatusEnum("succeeded")
+                new_b.package_id = 1
+            else:
+                new_bch.status = StatusEnum("waiting")
+                new_b.source_status = StatusEnum("pending")
+
+            b_objs.append(new_b)
+            bch_objs.append(new_bch)
+
+        self.db.session.bulk_save_objects(b_objs)
+        self.db.session.bulk_save_objects(bch_objs)
+        self.db.session.commit()
+        return batch_build_id
+
+    def test_large_batch_build_queue(self):
+        """
+        Fill in few thousands of builds, some of them with build chroots, some
+        of them with finished source builds - and measure.  Fail if something
+        takes unexpectedly long.
+        """
+        projects = ["aaa", "bbb", "ccc", "ddd", "eee", "fff"]
+
+        t1 = time.time()
+        with app.app_context():
+            batches = 4
+            for projectname in projects:
+                self.web_ui.new_project(projectname,
+                                        ["fedora-rawhide-i386", "fedora-18-x86_64"])
+                self.web_ui.create_distgit_package(projectname, "testpkg")
+
+                after_build = None
+                for _b in range(batches):
+                    after_build = self._add_one_large_batch(
+                        projectname, after_build=after_build)
+
+        t2 = time.time()
+        with app.app_context():
+            r = self.tc.get("/backend/pending-jobs/")
+            data = json.loads(r.data.decode("utf-8"))
+            dq = get_debug_queries()
+
+        t3 = time.time()
+
+        # each project has 1000 unblocked tasks
+        assert len(data) == len(projects) * 1000
+        # most of the tasks are source builds, but some (each 50th is binary
+        # rpm build, aka BuildChroot)
+        assert len([d for d in data if d["chroot"]]) == len(projects)*20
+        for projectname in projects:
+            assert len([d for d in data if projectname+"--" in d["sandbox"]]) == 1000
+
+        fill_time = t2 - t1
+        sql_alchemy_time = t3 - t2
+        query_time = sum([q.duration for q in dq])
+
+        asserts = [
+            sql_alchemy_time < fill_time/2,
+            query_time < fill_time/20,
+            # - for each project two queries (for batch => one build +batch_build)
+            # - two large queries (srpm + rpms)
+            # - one query for self.tc initialization
+            # Note that we only lazily load first Build in each unblocked batch,
+            # because even that first Build in batch is not yet finished -
+            # meaning that the whole batch is not yet finished as well.
+            len(dq) == len(projects)*2 + 2 + 1,
+        ]
+
+        if not all(asserts):
+            print("fill_time: {}".format(fill_time))
+            print("sql_alchemy_time: {}".format(sql_alchemy_time))
+            print("query_time: {}".format(query_time))
+            for n, query in enumerate(dq):
+                print("=== {} ===".format(n))
+                print(query)
+            assert False