copr / copr

Clone

#613 Respect module buildorder by setting dependencies among build batches
Merged 5 years ago by frostyx. Opened 5 years ago by frostyx.
copr/ frostyx/copr follow-module-buildorder  into  master

file added
+24 
@@ -0,0 +1,24 @@ 
 

+ """Add blocked_by column for batch
 

+ 

+ Revision ID: 8ae65946df53
 

+ Revises: b64659389c54
 

+ Create Date: 2019-03-27 18:38:03.758974
 

+ 

+ """
 

+ 

+ # revision identifiers, used by Alembic.
 

+ revision = '8ae65946df53'
 

+ down_revision = '9bc8681ed275'
 

+ 

+ from alembic import op
 

+ import sqlalchemy as sa
 

+ 

+ 

+ def upgrade():
 

+     op.add_column('batch', sa.Column('blocked_by_id', sa.Integer(), nullable=True))
 

+     op.create_foreign_key('batch_blocked_by_id_fkey', 'batch', 'batch', ['blocked_by_id'], ['id'])
 

+ 

+ 

+ def downgrade():
 

+     op.drop_constraint('batch_blocked_by_id_fkey', 'batch', type_='foreignkey')
 

+     op.drop_column('batch', 'blocked_by_id')

file modified
+5 
@@ -111,8 +111,10 @@ 
 

          return [batches[number] for number in sorted(batches.keys())]
 

  

      def add_builds(self, rpms, module):
 

+         blocked_by_id = None
 

          for group in self.get_build_batches(rpms):
 

              batch = models.Batch()
 

+             batch.blocked_by_id = blocked_by_id
 

              db.session.add(batch)
 

              for pkgname, rpm in group.items():
 

                  clone_url = self.get_clone_url(pkgname, rpm)
 
@@ -123,6 +125,9 @@ 
 

                  build.module_id = module.id
 

                  db.session.add(build)
 

  

+             # Every batch needs to by blocked by the previous one
 

+             blocked_by_id = batch.id
 

+ 

      def get_clone_url(self, pkgname, rpm):
 

          if rpm.get_repository():
 

              return rpm.get_repository()

file modified
+16 -1 
@@ -1021,7 +1021,16 @@ 
 

          Build is finished only if all its build_chroots are in finished state or
 

          the build was canceled.
 

          """
 

-         return self.canceled or all([chroot.finished for chroot in self.build_chroots])
 

+         if self.canceled:
 

+             return True
 

+         if not self.build_chroots:
 

+             # Not even SRPM is finished
 

+             return False
 

+         return all([chroot.finished for chroot in self.build_chroots])
 

+ 

+     @property
 

+     def blocked(self):
 

+         return bool(self.batch and self.batch.blocked_by and not self.batch.blocked_by.finished)
 

  

      @property
 

      def persistent(self):
 
@@ -1440,6 +1449,12 @@ 
 

  

  class Batch(db.Model):
 

      id = db.Column(db.Integer, primary_key=True)
 

+     blocked_by_id = db.Column(db.Integer, db.ForeignKey("batch.id"), nullable=True)
 

+     blocked_by = db.relationship("Batch", remote_side=[id])
 

+ 

+     @property
 

+     def finished(self):
 

+         return all([b.finished for b in self.builds])
 

  

  

  class Module(db.Model, helpers.Serializer):

file modified
+2 -1 
@@ -183,8 +183,9 @@ 
 

      """
 

      Return the job queue.
 

      """
 

+     srpm_tasks = [build for build in BuildsLogic.get_pending_srpm_build_tasks() if not build.blocked]
 

      build_records = ([get_build_record(task) for task in BuildsLogic.get_pending_build_tasks()] +
 

-                      [get_srpm_build_record(task) for task in BuildsLogic.get_pending_srpm_build_tasks()])
 

+                      [get_srpm_build_record(task) for task in srpm_tasks])
 

      log.info('Selected build records: {}'.format(build_records))
 

      return flask.jsonify(build_records)

file modified
+7 
@@ -443,6 +443,13 @@ 
 

              copr=self.c1, copr_dir=self.c4_dir, name="hello-world",
 

              source_type=0)
 

  

+     @pytest.fixture
 

+     def f_batches(self):
 

+         self.batch1 = models.Batch()
 

+         self.batch2 = models.Batch()
 

+         self.batch3 = models.Batch()
 

+         self.batch4 = models.Batch()
 

+ 

      def request_rest_api_with_auth(self, url,
 

                                     login=None, token=None,
 

                                     content=None, method="GET",

file modified
+13 
@@ -56,6 +56,19 @@ 
 

          assert batches[1] == {"tomcatjss": pkg2, "ldapjdk": pkg3}
 

          assert batches[2] == {"pki-core": pkg4, "dogtag-pki": pkg5}
 

  

+     def test_add_builds_batches(self, f_users, f_coprs, f_mock_chroots, f_builds, f_modules, f_db):
 

+         pkg1 = Modulemd.ComponentRpm(name="foo", rationale="foo package")
 

+         pkg2 = Modulemd.ComponentRpm(name="bar", rationale="bar package", buildorder=10)
 

+         pkg3 = Modulemd.ComponentRpm(name="baz", rationale="baz package", buildorder=10)
 

+ 

+         generator = ModulemdGenerator(name="testmodule", stream="master", version=123, summary="some summary")
 

+         facade = ModuleBuildFacade(self.u1, self.c1, generator.generate(), "testmodule.yaml")
 

+         facade.add_builds({"foo": pkg1, "bar": pkg2, "baz": pkg3}, self.m1)
 

+ 

+         b1, b2, b3 = self.m1.builds
 

+         assert b1.batch != b2.batch == b3.batch
 

+         assert b2.batch.blocked_by == b1.batch
 

+ 

  

  class TestModulemdGenerator(CoprsTestCase):
 

      config = {"DIST_GIT_URL": "http://distgiturl.org"}

file modified
+17 
@@ -1,3 +1,4 @@ 
 

+ from copr_common.enums import StatusEnum
 

  from tests.coprs_test_case import CoprsTestCase
 

  

  
@@ -42,3 +43,19 @@ 
 

          # even though we blacklised (by mistake) all chroots, package builds
 

          # against all chroots (fallback)
 

          assert len(list(self.p1.chroots)) == 15
 

+ 

+     def test_finished(self, f_users, f_coprs, f_mock_chroots, f_builds, f_db):
 

+         self.b1.build_chroots[0].status = StatusEnum("pending")
 

+         assert not self.b1.finished
 

+ 

+         self.b1.build_chroots[0].status = StatusEnum("succeeded")
 

+         assert self.b1.finished
 

+ 

+         self.b1.build_chroots[0].status = StatusEnum("running")
 

+         assert not self.b1.finished
 

+ 

+         self.b1.build_chroots[0].status = StatusEnum("failed")
 

+         assert self.b1.finished
 

+ 

+         self.b1.source_status = StatusEnum("canceled")
 

+         assert self.b1.finished

file modified
+17 -1 
@@ -1,6 +1,6 @@ 
 

  import json
 

  

- from copr_common.enums import BackendResultEnum
 

+ from copr_common.enums import BackendResultEnum, StatusEnum
 

  from tests.coprs_test_case import CoprsTestCase
 

  from coprs.logic.builds_logic import BuildsLogic
 

  
@@ -47,6 +47,22 @@ 
 

          data = json.loads(r.data.decode("utf-8"))
 

          assert data[0]["build_id"] == 3
 

  

+     def test_pending_blocked_builds(self, f_users, f_coprs, f_mock_chroots, f_builds, f_batches, f_db):
 

+         for build in [self.b2, self.b3, self.b4]:
 

+             build.source_status = StatusEnum("pending")
 

+ 

+         self.b2.batch = self.batch2
 

+         self.b3.batch = self.batch3
 

+         self.batch3.blocked_by = self.batch2
 

+         self.db.session.commit()
 

+ 

+         r = self.tc.get("/backend/pending-jobs/")
 

+         data = json.loads(r.data.decode("utf-8"))
 

+ 

+         ids = [job["build_id"] for job in data]
 

+         assert self.b3.id not in ids
 

+         assert {self.b2.id, self.b4.id}.issubset(ids)
 

+ 

  

  # status = 0 # failure
 

  # status = 1 # succeeded

Fixes #599
I've added a mechanism to block a build batch until other one finishes and then used it to respect the module buildorder groups.

I am not particularly happy with the name blocked_by (and especially blocked_by_id), but I couldn't find a better name. I am now thinking about prerequisite or ... I don't know. What are your thoughts on this?

Can this be rewritten to filter() so it is actually filtered by SQL query?

Yeah, probably can be and probably should be :-)
I didn't initially want to spend a long time writing the filter and then find out, that the whole idea is not good, so I tried a proof of concept first. I will reimplement this function.

I'd rather see the filter_not_blocked_batches to call the get_pending_srpm_build_tasks directly, or just add "non_blocked" argument to get_pending_srpm_build_tasks.

I don't mind the naming, blocked_by or depends_on is just fine.

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

Per face2face discussion - the build.batch.blocked_by.finished would be hard to implement. We will have to duplicate the code from models.py and the builds will likely be a small list, so at the end, I prefer simpler code.

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

+1

I guess this needs an update, though.

Thinking the same thing. I will update it

rebased onto 65083b4044fc4e673e34cf00fe882a0394d4c1f4
5 years ago

rebased onto 2abb02bf86d7e168b69f5e3497e6ccfcff485f49
5 years ago

I guess this needs an update, though.

I've rebased the branch and updated the alembic revision. Also, I've introduced a little bug in Build.finished method, so I fixed it and added a unit test for it.

What about?

return [build for build in builds if not build.blocked]

(new model property)

Are last two commits separated intentionally?

Except that I'd consider convenient the Build.blocked property, looks fine to me.

3 new commits added

  • [frontend] respect module buildorder by setting dependencies among batches
  • [frontend] add mechanism to block build batch until other one finishes
  • [frontend] build is not finished when not even SRPM is finished
5 years ago

Are last two commits separated intentionally?

Yes, because I don't like to see the batch-blocking mechanism as a modularity thing. I would like to view it as a general feature, that may (or not) have also other use-cases in the future.

Now it is used for modularity to ensure the module buildorder, but there is a clear relationship between those two. Batch-blocking can be used also for some other purpose and buildorder can be eventually ensured some other way. Does it make any sense?

Except that I'd consider convenient the Build.blocked property, looks fine to me.

That's a good call. I've implemented it and it saved many lines of code and the confusing filter_not_blocked_batches method is gone.

Does it make any sense?

Yes, thanks.

I've implemented it // Build.blocked

Thank you!

I like this; I'm only missing some test-case that would be failing "before", and succeeds now (the existing test-case succeeded by accident even before your fix). Maybe we could somehow change it so it would really prove that batches work?

3 new commits added

  • [frontend] respect module buildorder by setting dependencies among batches
  • [frontend] add mechanism to block build batch until other one finishes
  • [frontend] build is not finished when not even SRPM is finished
5 years ago

3 new commits added

  • [frontend] respect module buildorder by setting dependencies among batches
  • [frontend] add mechanism to block build batch until other one finishes
  • [frontend] build is not finished when not even SRPM is finished
5 years ago

I like this; I'm only missing some test-case that would be failing "before", and succeeds now (the existing test-case succeeded by accident even before your fix). Maybe we could somehow change it so it would really prove that batches work?

I've added two of them. One to ensure, that we won't show blocked builds to backend and one to ensure, that we add module builds in batches.

Thank you, +1

rebased onto ab70329
5 years ago

Pull-Request has been merged by frostyx
5 years ago
Metadata
None
No Tags
Changes Summary 8
+24
file added
frontend/coprs_frontend/alembic/schema/versions/8ae65946df53_add_blocked_by_column_for_batch.py
+5 -0
file changed
frontend/coprs_frontend/coprs/logic/modules_logic.py
+16 -1
file changed
frontend/coprs_frontend/coprs/models.py
+2 -1
file changed
frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py
+7 -0
file changed
frontend/coprs_frontend/tests/coprs_test_case.py
+13 -0
file changed
frontend/coprs_frontend/tests/test_logic/test_modules_logic.py
+17 -0
file changed
frontend/coprs_frontend/tests/test_models.py
+17 -1
file changed
frontend/coprs_frontend/tests/test_views/test_backend_ns/test_backend_general.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.