Merge pull request #760 from rohanpm/kickstart-phase2

Mark all kickstart non-RPM content as phase2 [RHELDST-28021]

exodus_gw/settings.py

@@ -377,6 +377,24 @@ class Settings(BaseSettings):
     ]
     """List of file names that should be saved for last when publishing."""
 
+    phase2_patterns: list[re.Pattern[str]] = [
+        # kickstart repos; note the logic here matches
+        # the manual workaround RHELDST-27642
+        re.compile(r"/kickstart/.*(?<!\.rpm)$"),
+    ]
+    """List of patterns which, if any have matched, force a path to
+    be handled during phase 2 of commit.
+
+    These patterns are intended for use with repositories not cleanly
+    separated between mutable entry points and immutable content.
+
+    For example, in-place updates to kickstart repositories may not
+    only modify entry points such as extra_files.json but also
+    arbitrary files referenced by that entry point, all of which should
+    be processed during phase 2 of commit in order for updates to
+    appear atomic.
+    """
+
     autoindex_filename: str = ".__exodus_autoindex"
     """Filename for indexes automatically generated during publish.
 

exodus_gw/worker/publish.py

@@ -238,6 +238,27 @@ def check_item(self, item: Item):
             # link_to resolution logic.
             raise ValueError("BUG: missing object_key for %s" % item.web_uri)
 
+    def is_phase2(self, item: Item) -> bool:
+        # Return True if item should be handled in phase 2 of commit.
+        name = basename(item.web_uri)
+        if (
+            name == self.settings.autoindex_filename
+            or name in self.settings.entry_point_files
+        ):
+            # Typical entrypoint
+            return True
+
+        for pattern in self.settings.phase2_patterns:
+            # Arbitrary patterns from settings.
+            # e.g. this is where /kickstart/ is expected to be handled.
+            if pattern.search(item.web_uri):
+                LOG.debug(
+                    "%s: phase2 forced via pattern %s", item.web_uri, pattern
+                )
+                return True
+
+        return False
+
     @property
     def item_select(self):
         # Returns base of the SELECT query to find all items for commit.
@@ -334,9 +355,6 @@ def write_publish_items(self) -> list[Item]:
 
         # Save any entry point items to publish last.
         final_items: list[Item] = []
-        final_basenames = self.settings.entry_point_files + [
-            self.settings.autoindex_filename
-        ]
 
         wrote_count = 0
 
@@ -358,7 +376,7 @@ def write_publish_items(self) -> list[Item]:
 
                     self.check_item(item)
 
-                    if basename(item.web_uri) in final_basenames:
+                    if self.is_phase2(item):
                         LOG.debug(
                             "Delayed write for %s",
                             item.web_uri,

tests/worker/test_publish.py

@@ -9,7 +9,7 @@
 from sqlalchemy import not_
 
 from exodus_gw import models, worker
-from exodus_gw.models import Publish
+from exodus_gw.models import Item, Publish
 from exodus_gw.models.path import PublishedPath
 from exodus_gw.settings import load_settings
 
@@ -27,6 +27,32 @@ def _task(publish_id):
     )
 
 
+def add_kickstart(publish: Publish):
+    """Adds some kickstart items onto a publish."""
+    publish.items.extend(
+        [
+            models.Item(
+                web_uri="/content/testproduct/1/kickstart/extra_files.json",
+                object_key="cee38a35950f3f9465378b1548c4495882da0bfbe217999add63cb3a8e2c4d75",
+                publish_id=publish.id,
+                updated=datetime(2023, 10, 4, 3, 52, 2),
+            ),
+            models.Item(
+                web_uri="/content/testproduct/1/kickstart/EULA",
+                object_key="6c92384cdbf1a8c448278aaffaf7d8c3f048749e201d504ffaab07d85f6b1a03",
+                publish_id=publish.id,
+                updated=datetime(2023, 10, 4, 3, 52, 2),
+            ),
+            models.Item(
+                web_uri="/content/testproduct/1/kickstart/Packages/example.rpm",
+                object_key="88a2831543aaca1355a725ad2f5969c7a180643beddfe94281343a2ba361c979",
+                publish_id=publish.id,
+                updated=datetime(2023, 10, 4, 3, 52, 2),
+            ),
+        ]
+    )
+
+
 @mock.patch("exodus_gw.worker.publish.AutoindexEnricher.run")
 @mock.patch("exodus_gw.worker.publish.CurrentMessage.get_current_message")
 @mock.patch("exodus_gw.worker.publish.DynamoDB.write_batch")
@@ -49,6 +75,9 @@ def test_commit(
     # Simulate successful write of items by write_batch.
     mock_write_batch.return_value = True
 
+    # Let this test cover kickstart items.
+    add_kickstart(fake_publish)
+
     db.add(fake_publish)
     db.add(task)
     # Caller would've set publish state to COMMITTING.
@@ -86,6 +115,10 @@ def test_commit(
         # Note that this does not include paths after alias resolution,
         # because Flusher does alias resolution itself internally
         # (tested elsewhere)
+        # Note that the default phase2_patterns cause all non-RPM kickstart
+        # files to be flushed.
+        "/content/testproduct/1/kickstart/EULA",
+        "/content/testproduct/1/kickstart/extra_files.json",
         "/content/testproduct/1/repo/",
         "/content/testproduct/1/repo/repomd.xml",
     ]
@@ -104,6 +137,38 @@ def test_commit(
         [
             # Note that both sides of the 1 alias are recorded, including
             # beyond the RHUI alias.
+            (
+                "test",
+                "/content/testproduct/1.1.0/kickstart/extra_files.json",
+            ),
+            (
+                "test",
+                "/content/testproduct/1/kickstart/extra_files.json",
+            ),
+            (
+                "test",
+                "/content/testproduct/rhui/1.1.0/kickstart/extra_files.json",
+            ),
+            (
+                "test",
+                "/content/testproduct/rhui/1/kickstart/extra_files.json",
+            ),
+            (
+                "test",
+                "/content/testproduct/1.1.0/kickstart/EULA",
+            ),
+            (
+                "test",
+                "/content/testproduct/1/kickstart/EULA",
+            ),
+            (
+                "test",
+                "/content/testproduct/rhui/1.1.0/kickstart/EULA",
+            ),
+            (
+                "test",
+                "/content/testproduct/rhui/1/kickstart/EULA",
+            ),
             ("test", "/content/testproduct/1/repo/"),
             ("test", "/content/testproduct/1.1.0/repo/"),
             ("test", "/content/testproduct/1/repo/repomd.xml"),
@@ -490,6 +555,9 @@ def test_commit_phase1(
         )
     )
 
+    # Let this test cover kickstart items.
+    add_kickstart(fake_publish)
+
     db.add(fake_publish)
     db.add(task)
 
@@ -554,6 +622,25 @@ def test_commit_phase1(
         [
             {"dirty": False, "web_uri": "/other/path"},
             {"dirty": False, "web_uri": "/some/path"},
+            # kickstart content:
+            # - EULA, though not an entrypoint, was forcibly delayed to phase2
+            #   via phase2_patterns setting. And therefore is still 'dirty'.
+            # - RPMs aren't matched by phase2_patterns and can still be
+            #   processed in phase1.
+            # - extra_files.json is an entrypoint and so is also delayed until
+            #   phase2.
+            {
+                "dirty": True,
+                "web_uri": "/content/testproduct/1/kickstart/EULA",
+            },
+            {
+                "dirty": False,
+                "web_uri": "/content/testproduct/1/kickstart/Packages/example.rpm",
+            },
+            {
+                "dirty": True,
+                "web_uri": "/content/testproduct/1/kickstart/extra_files.json",
+            },
             # the unresolved link is not yet written and therefore remains dirty
             {"dirty": True, "web_uri": "/some/path/to/link-src"},
             # autoindex and repomd.xml are both entrypoints, not yet written,
@@ -572,7 +659,7 @@ def test_commit_phase1(
 
     # It should have told us how many it wrote and how many remain
     assert (
-        "Phase 1: committed 2 items, phase 2: 2 items remaining" in caplog.text
+        "Phase 1: committed 3 items, phase 2: 4 items remaining" in caplog.text
     )
 
     # Let's do the same commit again...
@@ -590,7 +677,7 @@ def test_commit_phase1(
     # This time there should not have been any phase1 items processed at all,
     # as none of them were dirty.
     assert (
-        "Phase 1: committed 0 items, phase 2: 2 items remaining" in caplog.text
+        "Phase 1: committed 0 items, phase 2: 4 items remaining" in caplog.text
     )
 
     # And it should NOT have invoked the autoindex enricher in either commit