#1101 Importing CG now handles its problems
Merged 3 months ago by mprahl. Opened 4 months ago by vmaljulin.
vmaljulin/fm-orchestrator FACTORY-3473  into  master

@@ -871,16 +871,23 @@ 

          metadata = self._get_content_generator_metadata(file_dir)

          try:

              serverdir = self._upload_outputs(session, metadata, file_dir)

-             build_info = session.CGImport(metadata, serverdir)

+             try:

+                 build_info = session.CGImport(metadata, serverdir)

+             except koji.GenericError as e:

+                 if "Build already exists" not in str(e):

+                     raise

+                 log.warning('Failed to import content generator')

+                 build_info = None

              if conf.koji_cg_tag_build:

                  self._tag_cg_build()

-             log.info("Content generator import done.")

-             log.debug(json.dumps(build_info, sort_keys=True, indent=4))

- 

-             # Only remove the logs if CG import was successful.  If it fails,

-             # then we want to keep them around for debugging.

-             log.info("Removing %r" % file_dir)

-             shutil.rmtree(file_dir)

+             if build_info is not None:

+                 log.info("Content generator import done.")

+                 log.debug(json.dumps(build_info, sort_keys=True, indent=4))

+ 

+                 # Only remove the logs if CG import was successful.  If it fails,

+                 # then we want to keep them around for debugging.

+                 log.info("Removing %r", file_dir)

+                 shutil.rmtree(file_dir)

          except Exception as e:

              log.exception("Content generator import failed: %s", e)

              raise e

@@ -22,6 +22,7 @@ 

  

  import koji

  import os

+ import re

  from os import path, mkdir

  from os.path import dirname

  from shutil import copyfile

@@ -750,8 +751,9 @@ 

  

              # Whole module should be failed.

              assert c.module_build.state == models.BUILD_STATES['failed']

-             assert c.module_build.state_reason == \

-                 "Component(s) perl-Tangerine, perl-List-Compare failed to build."

+             assert re.match(r'Component\(s\) (perl-Tangerine|perl-List-Compare), '

+                             '(perl-Tangerine|perl-List-Compare) failed to build.',

+                             c.module_build.state_reason)

  

              # We should end up with batch 2 and never start batch 3, because

              # there were failed components in batch 2.

@@ -37,6 +37,8 @@ 

  from tests import init_data

  from tests.test_views.test_views import FakeSCM

  

+ import koji

+ 

  from module_build_service.builder.KojiContentGenerator import KojiContentGenerator

  

  GET_USER_RV = {

@@ -869,3 +871,24 @@ 

                  for stream in streams.get():

                      requires.append("%s:%s" % (name, stream))

              assert "%s:%s" % (mmd.get_name(), mmd.get_stream()) in requires

+ 

+     @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession")

+     @patch("module_build_service.builder.KojiContentGenerator.KojiContentGenerator._check_upload",

+            return_value='mbs/123456789-1234')

+     def test_upload_outputs(self, upload_checker, cl_session):

+         ''' Tests whether _upload_outputs function return already existing directory

+          if there's one '''

+         rv = self.cg._upload_outputs(cl_session.return_value, dict(), '')

+         assert rv == upload_checker.return_value

+ 

+     @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession")

+     @patch("module_build_service.builder.KojiContentGenerator.KojiContentGenerator._check_upload",

+            return_value='mbs/123456789-1234')

+     @patch("module_build_service.builder.KojiContentGenerator.KojiContentGenerator._tag_cg_build")

+     @patch("module_build_service.builder.KojiContentGenerator.KojiContentGenerator._load_koji_tag")

+     def test_koji_cg_koji_import(self, tag_loader, tagger, upload_checker, cl_session):

+         ''' Tests whether build is still tagged even if there's an exception in CGImport '''

+         cl_session.return_value.CGImport = Mock(

+             side_effect=koji.GenericError('Build already exists asdv'))

+         self.cg.koji_import()

+         tagger.assert_called()

Check if metadata wasn't already uploaded to Koji.
Adding exception handling while doing an import.

Fixes #1068

Depends on https://pagure.io/koji/pull-request/1186

2 new commits added

  • Adding exception handling while doing import
  • Check if metadata wasnt already upload to Koji
4 months ago

rebased onto 07107eccc00caef4d86abfb0fe9e9cdad4401ebe

4 months ago

pylint would complain. Use log.info("Removing %r", file_dir) (comma instead of %) - this avoids unnecessary string interpolation.

I'd rather this work didn't require changes to koji. IMO it's not worth handling the case to avoid re-uploading build metadata. This is tiny, and Koji has, I think, logic to clean up such files if they don't end up being used.

I propose the following:
1. Check if build exists, if not, upload metadata and import build.
2. Check if build is tagged, if not, tag it.

1 new commit added

  • Use format function instead of old string formating method in koji_import
4 months ago

pylint would complain. Use log.info("Removing %r", file_dir) (comma instead of %) - this avoids unnecessary string interpolation.

Replaced it with the format function

I'd rather this work didn't require changes to koji. IMO it's not worth handling the case to avoid re-uploading build metadata. This is tiny, and Koji has, I think, logic to clean up such files if they don't end up being used.
I propose the following:
1. Check if build exists, if not, upload metadata and import build.
2. Check if build is tagged, if not, tag it.

@jkaluza, @mprahl wdyt?

pylint would complain. Use log.info("Removing %r", file_dir) (comma instead of %) - this avoids unnecessary string interpolation.

Replaced it with the format function

Actually, this is really the same thing as it was before. You want to use the % syntax, but use a comma between message and variables. Why? Because that way string interpolation only happens if the message is indeed logged (based on logging level, handlers, etc).

Linters complain about such things because it's more efficient to delay string interpolation when logging. In practice though this is a minor optimization. But if we're trying to address it, let's do it correctly :smile:

3 new commits added

  • fixup! Adding exception handling while doing import
  • Adding exception handling while doing import
  • Check if metadata wasnt already upload to Koji
4 months ago

Could you comment it for now and add link to your Koji PR saying that we can uncomment it once this Koji PR is deployed? The issue is that if we merge it right now, we will have to wait with new MBS deployment until your Koji PR is merged and deployed everywhere, which can take several months at least.

It seems Koji returns "GenericError: Build already exists: ...", so you should catch that particular exception like this (not sure if it's the best way...):

try:
   ...
except koji.GenericError as e:
   if "Build already exists" not in str(e):
      raise
except Exception:
   raise

rebased onto a0fa05fb06b1ec9417b6116c1eb71993e88a529b

4 months ago

1 new commit added

  • fixup! Adding exception handling while doing import
4 months ago

rebased onto 83e0e746f1521ffe220d596c8d2d5b96c3408ae2

4 months ago

1 new commit added

  • Unit tests for koji_import
4 months ago

rebased onto 6d66c1d72706acd02d9dbcf159dc345361249b42

4 months ago

4 new commits added

  • Unit tests for koji_import
  • Temporary commit to be removed when listFiles will be available in koji
  • Adding exception handling while doing import
  • Check if metadata wasnt already upload to Koji
3 months ago

4 new commits added

  • Unit tests for koji_import
  • Temporary commit to be removed when listFiles will be available in koji
  • Adding exception handling while doing import
  • Check if metadata wasnt already upload to Koji
3 months ago

1 new commit added

  • Fix for the test test_all_builds_in_batch_fail
3 months ago

Can we instead call the koji method and catch the exception if the koji instance doesn't support it? It seems really silly to add the _check_upload method at all.

Alternatively, we could omit _check_upload completely for now and add it in the future when the corresponding koji changes have been merged and deployed to the instances we care about. The remaining changes in this commit are still very useful.

Thinking about this now, I would remove the try/except here and keep the original code.

With the try/except, all the possible real errors are skipped and checking for "build already tagged" error is not possible here anyway, because the tagging itself is done as separate Koji task for which we don't even wait here.

rebased onto 563b0b06c1b368f0b2967de2dbb911caab7ea2c6

3 months ago

Can we instead call the koji method and catch the exception if the koji instance doesn't support it?

Done

4 new commits added

  • Fix for the test test_all_builds_in_batch_fail
  • Unit tests for koji_import
  • Adding exception handling while doing import
  • Check if metadata wasnt already upload to Koji
3 months ago

Thinking about this now, I would remove the try/except here and keep the original code.
With the try/except, all the possible real errors are skipped and checking for "build already tagged" error is not possible here anyway, because the tagging itself is done as separate Koji task for which we don't even wait here.

Done

4 new commits added

  • Fix for the test test_all_builds_in_batch_fail
  • Unit tests for koji_import
  • Adding exception handling while doing import
  • Check if metadata wasnt already upload to Koji
3 months ago

4 new commits added

  • Fix for the test test_all_builds_in_batch_fail
  • Unit tests for koji_import
  • Adding exception handling while doing import
  • Check if metadata wasnt already upload to Koji
3 months ago

My vote is to remove this method since we don't know if the PR to add this will be merged and if the API for it will change.

We can always add it back in later.

My vote is to remove this method since we don't know if the PR to add this will be merged and if the API for it will change.
We can always add it back in later.

Ok, so maybe I should split this pull-request?

My vote is to remove this method since we don't know if the PR to add this will be merged and if the API for it will change.
We can always add it back in later.

Ok, so maybe I should split this pull-request?

Yeah. Just remove this portion from this PR. We can file a tracking story for this piece. I wouldn't bother opening another PR for that just yet.

rebased onto aed51df

3 months ago

Yeah. Just remove this portion from this PR. We can file a tracking story for this piece. I wouldn't bother opening another PR for that just yet.

Done

Pull-Request has been merged by mprahl

3 months ago