fm-orchestrator

Clone
Source Code
GIT

#1131 Make finalization before changing state to done
Merged 5 years ago by mprahl. Opened 5 years ago by vmaljulin.
vmaljulin/fm-orchestrator FACTORY-3828  into  master

file modified
+3 -2 
@@ -1205,8 +1205,9 @@ 
 

              return list(nvrs)
 

  

      def finalize(self):
 

-         # Only import to koji CG if the module is "done".
 

-         if self.config.koji_enable_content_generator and self.module.state == 3:
 

+         # Only import to koji CG if the module is "build".
 

+         if self.config.koji_enable_content_generator and \
 

+                 self.module.state == models.BUILD_STATES['build']:
 

              cg = KojiContentGenerator(self.module, self.config)
 

              cg.koji_import()
 

              if conf.koji_cg_devel_module:

file modified
-6 
@@ -128,12 +128,6 @@ 
 

          # This is ok.. it's a race condition we can ignore.
 

          pass
 

  

-     builder = module_build_service.builder.GenericBuilder.create_from_module(
 

-         session, build, config)
 

- 

-     # Tell the external buildsystem to wrap up (CG import, createrepo, etc.)
 

-     builder.finalize()
 

- 

      build.transition(config, state="ready")
 

      session.commit()

file modified
+6 -7 
@@ -120,13 +120,9 @@ 
 

      # So now we can either start a new batch if there are still some to build
 

      # or, if everything is built successfully, then we can bless the module as
 

      # complete.
 

-     has_unbuilt_components = False
 

-     has_failed_components = False
 

-     for c in module_build.component_builds:
 

-         if c.state in [None, koji.BUILD_STATES['BUILDING']]:
 

-             has_unbuilt_components = True
 

-         elif (c.state in failed_states):
 

-             has_failed_components = True
 

+     has_unbuilt_components = any(c.state in [None, koji.BUILD_STATES['BUILDING']]
 

+                                  for c in module_build.component_builds)
 

+     has_failed_components = any(c.state in failed_states for c in module_build.component_builds)
 

  

      further_work = []
 

      if has_unbuilt_components and not has_failed_components:
 
@@ -152,6 +148,9 @@ 
 

                                      state=models.BUILD_STATES['failed'],
 

                                      state_reason=state_reason)
 

          else:
 

+             # Tell the external buildsystem to wrap up (CG import, createrepo, etc.)
 

+             builder.finalize()
 

+ 

              module_build.transition(config, state=models.BUILD_STATES['done'])
 

          session.commit()

file modified
+1 -1 
@@ -666,7 +666,7 @@ 
 

      ])
 

      @mock.patch('module_build_service.builder.KojiModuleBuilder.KojiContentGenerator')
 

      def test_finalize(self, mock_koji_cg_cls, cg_enabled, cg_devel_enabled):
 

-         self.module.state = 3
 

+         self.module.state = 2
 

          with patch('module_build_service.config.Config.koji_enable_content_generator',
 

                     new_callable=mock.PropertyMock, return_value=cg_enabled):
 

              with patch('module_build_service.config.Config.koji_cg_devel_module',

file modified
+33 
@@ -76,6 +76,39 @@ 
 

                      '#fbed359411a1baa08d4a88e0d12d426fbf8f602c'))
 

  

      @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.finalize')
 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.recover_orphaned_artifact', return_value=[])
 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.get_average_build_time',
 

+                 return_value=0.0)
 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.list_tasks_for_components',
 

+                 return_value=[])
 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.buildroot_ready', return_value=True)
 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.get_session')
 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.build')
 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

+                 'KojiModuleBuilder.buildroot_connect')
 

+     def test_a_single_match_finalize(self, connect, build_fn, get_session, ready, list_tasks_fn,
 

+                                      mock_gabt, mock_uea, finalizer):
 

+         """ Test that when a repo msg hits us and we have a single match.
 

+         """
 

+         scheduler_init_data(tangerine_state=1)
 

+         get_session.return_value = mock.Mock(), 'development'
 

+         build_fn.return_value = 1234, 1, '', None
 

+ 

+         msg = module_build_service.messaging.KojiRepoChange(
 

+             'some_msg_id', 'module-testmodule-master-20170109091357-7c29193d-build')
 

+         module_build_service.scheduler.handlers.repos.done(
 

+             config=conf, session=db.session, msg=msg)
 

+ 

+         finalizer.assert_called_once()
 

+ 

+     @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

                  'KojiModuleBuilder.recover_orphaned_artifact', return_value=[])
 

      @mock.patch('module_build_service.builder.KojiModuleBuilder.'
 

                  'KojiModuleBuilder.get_average_build_time',

Signed-off-by: Valerij Maljulin vmaljuli@redhat.com

rebased onto 31ff18210272dac306fdae7a593d16f2d158773e
5 years ago

This looks good to me, will wait for someone else to review this too before merging.

It would be nice to have some more tests for repos.done, but not sure if we should write them as part of this PR...

In the done handler, there is still the following code:

build_logs.stop(build)
module_build_service.builder.GenericBuilder.clear_cache(build)

Perhaps it makes sense to move that to this part of the code since we eventually will make the done handler a no-op.

This looks good to me, will wait for someone else to review this too before merging.
It would be nice to have some more tests for repos.done, but not sure if we should write them as part of this PR...

Yeah this concerns me. I would have thought there'd be a lot more changes to the tests for this change, but I guess we don't have adequate coverage for that scenario.

I'd like to see at least one test that asserts the finalize method was called when the last repo regen is complete.
Edited 5 years ago by mprahl

rebased onto eafa930
5 years ago

I'd like to see at least one test that asserts the finalize method was called when the last repo regen is complete.

Done

+1.

In the done handler, there is still the following code:
build_logs.stop(build)
module_build_service.builder.GenericBuilder.clear_cache(build)

Perhaps it makes sense to move that to this part of the code since we eventually will make the done handler a no-op.

@vmaljulin you didn't respond to this comment

My comment can be addressed in a different PR. :thumbsup:

Pull-Request has been merged by mprahl
5 years ago
Changes Summary 5
+3 -2
file changed
module_build_service/builder/KojiModuleBuilder.py
+0 -6
file changed
module_build_service/scheduler/handlers/modules.py
+6 -7
file changed
module_build_service/scheduler/handlers/repos.py
+1 -1
file changed
tests/test_builder/test_koji.py
+33 -0
file changed
tests/test_scheduler/test_repo_done.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.