PR#391: Fix the deduplication code when `conf.lightblue_released_dependencies_only = True`. - freshmaker

freshmaker

#391 Fix the deduplication code when `conf.lightblue_released_dependencies_only = True`.

Merged 4 years ago by lucarval. Opened 4 years ago by jkaluza.

jkaluza/freshmaker models-fix into master

Fix the deduplication code when `conf.lightblue_released_dependencies_only = True`.

Jan Kaluza • 4 years ago

4efb0d2

freshmaker/lightblue.py

file modified

+9 -21

		`@@ -1359,9 +1359,6 @@`
		`nvr_to_image = {}`
		`# Temporary dict mapping NV to latest released NVR for that NV.`
		`nv_to_latest_released_nvr = {}`
		`- # Temporary list containing names of all images to rebuild. This is`
		`- # later used to replace "foo-docker" with "foo-container".`
		`- n_to_nvs = {}`

		`# Constructs the temporary dicts as desribed above.`
		`for image_id, images in enumerate(to_rebuild):`
		`@@ -1375,10 +1372,6 @@`
		`nv_to_nvrs[nv].append(nvr)`
		`if nvr not in nvr_to_coordinates:`
		`nvr_to_coordinates[nvr] = []`
		`- if parsed_nvr["name"] not in n_to_nvs:`
		`- n_to_nvs[parsed_nvr["name"]] = []`
		`- if nv not in n_to_nvs[parsed_nvr["name"]]:`
		`- n_to_nvs[parsed_nvr["name"]].append(nv)`
		`nvr_to_coordinates[nvr].append([image_id, parent_id])`
		`nvr_to_image[nvr] = image`
		`if "latest_released" in image and image["latest_released"]:`
		`@@ -1423,25 +1416,20 @@`
		`latest_released_nvr = nv_to_latest_released_nvr[nv]`
		`else:`
		`latest_released_nvr = nvrs[0]`
		`+`
		`# The latest_released_nvr_index points to the latest released NVR`
		# in the `nvrs` list. Because `nvrs` list is desc sorted, every NVR
		`# with higher index is lower and therefore we need to replace it.`
		`- try:`
		`- if not conf.lightblue_released_dependencies_only:`
		`- latest_released_nvr_index = nvrs.index(latest_released_nvr)`
		`- else:`
		`- # In case we want to use only released versions of images,`
		`- # replace all the images with the latest released one.`
		`- latest_released_nvr_index = -1`
		`- except ValueError:`
		`- # In case the latest_released_nvr is not found in the nvrs,`
		`- # it means the all nvrs should be replaced by new one from`
		`- # nvs_to_replace and therefore set index to -1 indicating we`
		`- # want to replace everything.`
		`+ if not conf.lightblue_released_dependencies_only:`
		`+ latest_released_nvr_index = nvrs.index(latest_released_nvr)`
lucarval commented 4 years ago This will raise `ValueError` if `latest_released_nvr` is not in `nvrs` list. Is that no longer a valid use case? Maybe consider this: latest_released_nvr_index = -1 if not conf.lightblue_released_dependencies_only: try: latest_released_nvr_index = nvrs.index(latest_released_nvr) except ValueError: pass
		`+ else:`
		`+ # In case we want to use only released versions of images,`
		`+ # replace all the images with the latest released one.`
		`latest_released_nvr_index = -1`
		`+`
		`if phase == "handle_parent_change":`
		`# Find out the name of parent image of latest release image.`
		`- latest_image = nvr_to_image[nvrs[latest_released_nvr_index]]`
		`+ latest_image = nvr_to_image[latest_released_nvr]`
		`if "parent" not in latest_image or not latest_image["parent"]:`
		`continue`
		`latest_parent_name = koji.parse_NVR(`
		`@@ -1449,7 +1437,7 @@`

		`# Go through the older images and in case the parent image differs,`
		`# update its parents according to latest image parents.`
		`- for nvr in nvrs[latest_released_nvr_index:]:`
		`+ for nvr in nvrs[latest_released_nvr_index + 1:]:`
		`image = nvr_to_image[nvr]`
		`if "parent" not in image or not image["parent"]:`
		`continue`

tests/test_lightblue.py

file modified

+4 -2

		`@@ -1946,8 +1946,10 @@`
		`])`
		`]`

		`- ret = self.lb._deduplicate_images_to_rebuild([httpd, perl])`
		`- self.assertEqual(ret, expected_images)`
		`+ for val in [True, False]:`
		`+ with patch.object(freshmaker.conf, 'lightblue_released_dependencies_only', new=val):`
		`+ ret = self.lb._deduplicate_images_to_rebuild([httpd, perl])`
		`+ self.assertEqual(ret, expected_images)`


		`class TestArchitecturesFromRegistry(helpers.FreshmakerTestCase):`

jkaluza commented 4 years ago

This is follow-up of https://pagure.io/freshmaker/pull-request/388.

In that PR, we fixed deduplication code for case when image changed
its parent image completely within the same version but different
release.

The issue is that this works only when lightblue_released_dependencies_only
is set to False. In case it is set to True, the latest_released_nvr_index
is set to -1 indicating that the deduplication code should replace all
the images with latest release version (so in case the image is released
but based on the unreleased image, we move it back to released release
of that parent image).

When latest_released_nvr_index is -1, the current code to set latest_image
is simply broken, because it does latest_image = nvr_to_image[nvrs[latest_released_nvr_index]]
and therefore effectively sets the latest_image to the oldest image (nvrs[-1]).

This commit fixes this issue.

It also removes n_to_nvs which is not used anymore and one try/except
block which has been forgotten there from times when n_to_nvs was used.

lucarval commented on line 43 of freshmaker/lightblue.py 4 years ago

This will raise ValueError if latest_released_nvr is not in nvrs list. Is that no longer a valid use case?

Maybe consider this:

latest_released_nvr_index = -1
if not conf.lightblue_released_dependencies_only:
  try:
    latest_released_nvr_index = nvrs.index(latest_released_nvr)
  except ValueError:
    pass

jkaluza commented 4 years ago

@lucarval, that's no longer the valid use-case. Previously, there was special code to handle foo-docker to foo-container in a way that if foo-docker image was always replaced by foo-container image. But that code has been removed in the PR#388.

In the current code, the latest_release_nvr is always something from nvrs list.

jkaluza commented 4 years ago

To give you an example, in the previous, removed, code, the latest_released_nvr could be foo-container-1-15 while the nvrs was list like ["foo-docker-1-13", "foo-docker-1-14"]. But the code which actually allows setting NVR outside of nvrs as latest_released_nvr has been removed.

Edited 4 years ago by jkaluza

lucarval commented 4 years ago

:thumbsup: Thanks for clarifying!