fm-orchestrator

Clone
Source Code
GIT

#1301 Module builds now remember what module they reused for building.
Merged 4 years ago by jkaluza. Opened 4 years ago by mcurlej.
mcurlej/fm-orchestrator race_cond  into  master

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

+ """add reused_module_id column
 

+ 

+ Revision ID: 40b2c7d988d7
 

+ Revises: bf861b6af29a
 

+ Create Date: 2019-06-21 13:41:06.041269
 

+ 

+ """
 

+ 

+ # revision identifiers, used by Alembic.
 

+ revision = '40b2c7d988d7'
 

+ down_revision = 'bf861b6af29a'
 

+ 

+ from alembic import op
 

+ import sqlalchemy as sa
 

+ 

+ 

+ def upgrade():
 

+     op.add_column('module_builds', sa.Column('reused_module_id', sa.Integer(), nullable=True))
 

+     sa.ForeignKeyConstraint(['reused_module_id'], ['module_builds.id'], ),
 

+ 

+ 

+ def downgrade():
 

+     op.drop_column('module_builds', 'reused_module_id')

file modified
+2 
@@ -221,6 +221,8 @@ 
 

      rebuild_strategy = db.Column(db.String, nullable=False)
 

      virtual_streams = db.relationship(
 

          "VirtualStream", secondary=module_builds_to_virtual_streams, back_populates="module_builds")
 

+     reused_module_id = db.Column(db.Integer, db.ForeignKey("module_builds.id"))
 

+     reused_module = db.relationship("ModuleBuild", remote_side="ModuleBuild.id")
 

  

      # List of arches against which the module is built.
 

      # NOTE: It is not filled for imported modules, because imported module builds have not been

file modified
+7 -1 
@@ -85,8 +85,11 @@ 
 

      :param module: the ModuleBuild object of module being built.
 

      :return: ModuleBuild object which can be used for component reuse.
 

      """
 

-     mmd = module.mmd()
 

  

+     if module.reused_module:
 

+         return module.reused_module
 

+ 

+     mmd = module.mmd()
 

      # Find the latest module that is in the done or ready state
 

      previous_module_build = (
 

          session.query(models.ModuleBuild)
 
@@ -111,6 +114,9 @@ 
 

          log.info("Cannot re-use.  %r is the first module build." % module)
 

          return None
 

  

+     module.reused_module_id = previous_module_build.id
 

+     session.commit()
 

+ 

      return previous_module_build

file modified
+42 
@@ -1429,3 +1429,45 @@ 
 

              module_build = models.ModuleBuild.get_build_from_nsvc(
 

                  db.session, "platform", "y", 1, "000000")
 

              assert module_build
 

+ 

+ 

+ class TestUtilsModuleReuse:
 

+ 

+     def setup_method(self, test_method):
 

+         init_data()
 

+ 

+     def teardown_method(self, test_method):
 

+         clean_database()
 

+ 

+     def test_get_reusable_module_when_reused_module_not_set(self):
 

+         module = models.ModuleBuild.query.filter_by(
 

+             name="nginx").order_by(models.ModuleBuild.id.desc()).first()
 

+         module.state = models.BUILD_STATES["build"]
 

+         db.session.commit()
 

+ 

+         assert not module.reused_module
 

+ 

+         reusable_module = module_build_service.utils.get_reusable_module(
 

+             db.session, module)
 

+ 

+         assert module.reused_module
 

+         assert reusable_module.id == module.reused_module_id
 

+ 

+     def test_get_reusable_module_when_reused_module_already_set(self):
 

+         modules = models.ModuleBuild.query.filter_by(
 

+             name="nginx").order_by(models.ModuleBuild.id.desc()).limit(2).all()
 

+         build_module = modules[0]
 

+         reused_module = modules[1]
 

+         build_module.state = models.BUILD_STATES["build"]
 

+         build_module.reused_module_id = reused_module.id
 

+         db.session.commit()
 

+ 

+         assert build_module.reused_module
 

+         assert reused_module == build_module.reused_module
 

+ 

+         reusable_module = module_build_service.utils.get_reusable_module(
 

+             db.session, build_module)
 

+ 

+         assert build_module.reused_module
 

+         assert reusable_module.id == build_module.reused_module_id
 

+         assert reusable_module.id == reused_module.id

There was a race condition, when 2 builds where build in the same time.
There was an issue that after one has finished the other reused the new build
for reuse and not the one it started with.

Ticket-ID: FACTORY-3862

Signed-off-by: Martin Curlej mcurlej@redhat.com

@mprahl @jkaluza can you please take a look?

I'm thinking about the backref name here and uselist=False. Do you have any use-case for original_module which only points to one module? Which module is it if single module is reused by multiple module builds?

It would make more sense to me to remove this until we are sure we need to access that data, or we should probably rename it to something like reusing_modules (in the meaning of all the modules which are reusing this module) and do not use uselist=False.

What do you think?

This is missing a ForeignKeyConstraint on module_builds.id.

I think if the backref is removed, this should be fine.

Optional: It'd be cleaner and faster if you used order_by and first() instead of .all()[-1]

Could you use models.BUILD_STATES['build'] so that it's easier to read?

Same as above

Optional: Same as above but you could use a limit to get just the two you need

Please use double-quotes here since it's part of our style guide:
https://pagure.io/fm-orchestrator/blob/master/f/docs/CONTRIBUTING.rst

Please use double-quotes here since it's part of our style guide:
https://pagure.io/fm-orchestrator/blob/master/f/docs/CONTRIBUTING.rst

From my point of view, when defining a model with some ORM framework, whatever SQLAlchemy or Django ORM, it should always be good to define the back reference attribute for existing or potential use cases. There are several benefits in general I think, 1) it reflects the relationship between models (including self-reference) clearly; 2) it is explicit for developers to know using the backref attributes to access objects, e.g. module_build.reusing_modules, instead of writing additional code to get those objects; 3) even if there is no requirement for the backref attributes inside code, it could be useful for debugging purpose to access related objects as well.

Just share my idea about the backref. I have no objection if you decide to remove it. :)

In addition, if keeping backref, I recommend to rename it to reused_by. Hence, we can access related module builds in module_build.reused_by, which could be read as "a module build is reused by ...". And rename reused_module to just reusing, code module_build.reusing could be read naturally as "a module build is reusing some module".

Optional: It'd be cleaner and faster if you used order_by and first() instead of .all()[-1]

Agree. And ORDER BY id DESC to ensure to get the last nginx module build.

Suggest to comment this line out why setting it to build state for this test.

Could you please rename it to a more descriptive name so that it can express the test purpose clearly? When I read this name, I'm quite confusing, especially the part "not_set". After I read through following code and understand what it tests, I still can't understand this name well. :)

Same as above.

Same as above.

Same as above to order modules by id in desc.

Could you please give a name to modules[-1] and modules[-2] or comment them out? It would be easier to understand why use these two modules for this test.

rebased onto ed24ca8
4 years ago

+1

Pull-Request has been merged by jkaluza
4 years ago
Changes Summary 4
+23
file added
module_build_service/migrations/versions/40b2c7d988d7_add_reused_module_id_column.py
+2 -0
file changed
module_build_service/models.py
+7 -1
file changed
module_build_service/utils/reuse.py
+42 -0
file changed
tests/test_utils/test_utils.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.