PR#1101: Importing CG now handles its problems - fm-orchestrator

fm-orchestrator

#1101 Importing CG now handles its problems

Merged 5 years ago by mprahl. Opened 5 years ago by vmaljulin.

vmaljulin/fm-orchestrator FACTORY-3473 into master

Fix for the test test_all_builds_in_batch_fail

Valerij Maljulin • 5 years ago

436e0be

Unit tests for koji_import

Valerij Maljulin • 5 years ago

51a9892

Adding exception handling while doing import

Valerij Maljulin • 5 years ago

aed51df

module_build_service/builder/KojiContentGenerator.py

file modified

+15 -8

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

tests/test_build/test_build.py

file modified

+4 -2

		`@@ -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.`

tests/test_content_generator.py

file modified

+23

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

vmaljulin commented 5 years ago

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

5 years ago

rebased onto 07107eccc00caef4d86abfb0fe9e9cdad4401ebe

5 years ago

lholecek commented on line 66 of module_build_service/builder/KojiContentGenerator.py 5 years ago

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

lucarval commented 5 years ago

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

5 years ago

vmaljulin commented 5 years 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

lucarval commented 5 years ago

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?

lucarval commented 5 years 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

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

5 years ago

jkaluza commented on line 6 of module_build_service/builder/KojiContentGenerator.py 5 years 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.

jkaluza commented on line 48 of module_build_service/builder/KojiContentGenerator.py 5 years ago

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

5 years ago

1 new commit added

fixup! Adding exception handling while doing import

5 years ago

rebased onto 83e0e746f1521ffe220d596c8d2d5b96c3408ae2

5 years ago

1 new commit added

Unit tests for koji_import

5 years ago

rebased onto 6d66c1d72706acd02d9dbcf159dc345361249b42

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

5 years ago

vmaljulin commented 5 years ago

please review

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

5 years ago

1 new commit added

Fix for the test test_all_builds_in_batch_fail

5 years ago

mcurlej commented 5 years ago

+1 LGTM

lucarval commented on line 17 of module_build_service/builder/KojiContentGenerator.py 5 years 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.

jkaluza commented on line 35 of module_build_service/builder/KojiContentGenerator.py 5 years 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.

rebased onto 563b0b06c1b368f0b2967de2dbb911caab7ea2c6

5 years ago

vmaljulin commented 5 years 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

5 years ago

vmaljulin commented 5 years 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

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

5 years ago

mprahl commented on line 16 of module_build_service/builder/KojiContentGenerator.py 5 years 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.

vmaljulin commented 5 years 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.

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

lucarval commented 5 years 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.

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

5 years ago

vmaljulin commented 5 years ago