#398 Reuse "complete" ArtifactBuilds from original Event
Merged 4 years ago by jkaluza. Opened 4 years ago by gnaponie.
gnaponie/freshmaker FACTORY-4672  into  master

@@ -223,11 +223,25 @@ 

                      self.log_debug("Skipping recording build %s, "

                                     "it is already in db", nvr)

                      continue

+ 

+                 parent_build = db_event.get_artifact_build_from_event_dependencies(nvr)

+                 if parent_build:

+                     self.log_debug(

+                         "Skipping recording build %s, "

+                         "it is already built in dependant event %r", nvr, parent_build[0].event_id)

+                     continue

+ 

                  self.log_debug("Recording %s", nvr)

                  parent_nvr = image["parent"]["brew"]["build"] \

                      if "parent" in image and image["parent"] else None

                  dep_on = builds[parent_nvr] if parent_nvr in builds else None

  

+                 if parent_nvr:

+                     build = db_event.get_artifact_build_from_event_dependencies(parent_nvr)

+                     if build:

+                         parent_nvr = build[0].rebuilt_nvr

+                         dep_on = None

+ 

                  if "error" in image and image["error"]:

                      state_reason = image["error"]

                      state = ArtifactBuildState.FAILED.value

file modified
+15
@@ -455,6 +455,21 @@ 

          db.session.commit()

          return dep_events

  

+     def get_artifact_build_from_event_dependencies(self, nvr):

+         """

+         It returns the artifact build, with `DONE` state, from the event dependencies (the build

+         of the parent event). `nvr` is used as `original_nvr` when finding the `ArtifactBuild`.

+         It returns all the parent artifact builds from the first found event dependency.

+         If the build is not found, it returns None.

+         """

+         for parent_event in self.event_dependencies:

+             parent_build = db.session.query(

+                 ArtifactBuild).filter_by(event_id=parent_event.id,

+                                          original_nvr=nvr,

+                                          state=ArtifactBuildState.DONE.value).all()

+             if parent_build:

+                 return parent_build

+ 

  

  Index('idx_event_message_id', Event.message_id, unique=True)

  

@@ -1417,3 +1417,110 @@ 

  

          # Pulp repo should not be prepared for FAILED build.

          self.mock_prepare_pulp_repo.assert_not_called()

+ 

+     def test_parent_image_already_built(self):

+         batches = [

+             [ContainerImage({

+                 "brew": {

+                     "completion_date": "20170420T17:05:37.000-0400",

+                     "build": "rhel-server-docker-7.3-82",

+                     "package": "rhel-server-docker"

+                 },

+                 'parsed_data': {

+                     'layers': [

+                         'sha512:12345678980',

+                         'sha512:10987654321'

+                     ]

+                 },

+                 "parent": None,

+                 "content_sets": ["content-set-1"],

+                 "repository": "repo-1",

+                 "commit": "123456789",

+                 "target": "target-candidate",

+                 "git_branch": "rhel-7",

+                 "error": None,

+                 "generate_pulp_repos": True,

+                 "arches": "x86_64",

+                 "odcs_compose_ids": None,

+                 "published": False,

+             })],

+             [ContainerImage({

+                 "brew": {

+                     "build": "rh-dotnetcore10-docker-1.0-16",

+                     "package": "rh-dotnetcore10-docker",

+                     "completion_date": "20170511T10:06:09.000-0400"

+                 },

+                 'parsed_data': {

+                     'layers': [

+                         'sha512:2345af2e293',

+                         'sha512:12345678980',

+                         'sha512:10987654321'

+                     ]

+                 },

+                 "parent": ContainerImage({

+                     "brew": {

+                         "completion_date": "20170420T17:05:37.000-0400",

+                         "build": "rhel-server-docker-7.3-82",

+                         "package": "rhel-server-docker"

+                     },

+                     'parsed_data': {

+                         'layers': [

+                             'sha512:12345678980',

+                             'sha512:10987654321'

+                         ]

+                     },

+                     "parent": None,

+                     "content_sets": ["content-set-1"],

+                     "repository": "repo-1",

+                     "commit": "123456789",

+                     "target": "target-candidate",

+                     "git_branch": "rhel-7",

+                     "error": None

+                 }),

+                 "content_sets": ["content-set-1"],

+                 "repository": "repo-1",

+                 "commit": "987654321",

+                 "target": "target-candidate",

+                 "git_branch": "rhel-7",

+                 "error": None,

+                 "generate_pulp_repos": True,

+                 "arches": "x86_64",

+                 "odcs_compose_ids": None,

+                 "published": False,

+             })]

+         ]

+ 

+         et_event = ErrataAdvisoryRPMsSignedEvent(

+             "msg-id-2",

+             ErrataAdvisory(123, "RHSA-2017", "REL_PREP", [],

+                            security_impact="None",

+                            product_short_name="product"))

+         event0 = Event.create(db.session, 'msg-id-1', '1230',

+                               EVENT_TYPES[ErrataAdvisoryRPMsSignedEvent])

+         event1 = Event.create(db.session, 'msg-id-2', '1231',

+                               EVENT_TYPES[ErrataAdvisoryRPMsSignedEvent])

+         ArtifactBuild.create(

+             db.session, event0, "parent", "image",

+             state=ArtifactBuildState.DONE,

+             original_nvr="rhel-server-docker-7.3-82", rebuilt_nvr="some-test-nvr")

+         db.session.commit()

+         event1.add_event_dependency(db.session, event0)

+         db.session.commit()

+ 

+         handler = RebuildImagesOnRPMAdvisoryChange()

+         handler._record_batches(batches, et_event)

+ 

+         # Check parent image

+         query = db.session.query(ArtifactBuild)

+         parent_image = query.filter(

+             ArtifactBuild.original_nvr == 'rhel-server-docker-7.3-82'

+         ).all()

+         self.assertEqual(len(parent_image), 1)

+         self.assertEqual(ArtifactBuildState.DONE.value, parent_image[0].state)

+ 

+         # Check child image

+         child_image = query.filter(

+             ArtifactBuild.original_nvr == 'rh-dotnetcore10-docker-1.0-16'

+         ).first()

+         self.assertNotEqual(None, child_image)

+         self.assertEqual(child_image.dep_on, None)

When some parent image in the current Event has the same
original_nvr as the completely built image in the previous Event,
then the current parent image should be replaced by the
rebuild_nvr from the original event.
ref. FACTORY-4672

Signed-off-by: gnaponie gnaponie@redhat.com

@jkaluza can you please check? Are the conditions of the test sufficient?
cc. @yashn

We have discussed this on IRC, no need for further reviews right now.

Small issue with current implementation is that the ArtifactBuild representing the parent_nvr in the db_event stays in the db_event even when it is reused and nothing is really built against it. Giulia will work on fix and update the PR after that.

rebased onto 1a6d65c99ba24b5cdaa923f789f3f2a8b6a7f600

4 years ago

rebased onto 831796fdab33718dab4e3b3d34561b7fbf6f2567

4 years ago

rebased onto aa00de9684a3a63f2912db6c69a18d4f0998e536

4 years ago

@jkaluza I think it's complete now. PTAL

rebased onto 8db5830142a6a6e178731a2ebbf805fb060a075e

4 years ago

Suggestion: Consider implementing this logic in Event class directly to avoid code duplication:

class Event(FreshmakerBase):
  ...
  def get_artifact_build_from_event_dependencies(self, session, nvr):
    for parent_event in self.event_dependencies:
      ...
      return parent_build
    return None  # Implicitly

Why do we need to query the database again?

Ah! It's because we query with the nvr, then parent_nvr.

@lucarval The first query handles the fact that the original parent should not be created at all. The second part of the part changes how the child image gets created.
Without the first part we are still creating the parent, that would still be built by Freshmaker even if it's not necessary, because there's no child depending on it.

@jkaluza are you satisfied with the apprentice reply? :D

rebased onto 312b2103ce2ecee60796a9f45b4ce0bd2377ace3

4 years ago

@jkaluza @lucarval I've rebased with the suggestion. Could make another review? Thanks.

Add docstring please stating that nvr is used as original_nvr when finding the ArtifactBuild.

rebased onto 8e7149d

4 years ago

Commit 0c4b8cb fixes this pull-request

Pull-Request has been merged by jkaluza

4 years ago

Pull-Request has been merged by jkaluza

4 years ago