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

@@ -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:

@@ -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()

  

@@ -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()

  

@@ -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',

@@ -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',

rebased onto 31ff18210272dac306fdae7a593d16f2d158773e

6 months 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.

rebased onto eafa930

5 months 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

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 months ago