Repository: osbuild/osbuild
Pull request #1826 (2024-07-30 09:50:45)

main,monitor: fix total steps in progress reporting
The existing code to record progress was a bit too naive. Instead of just counting the number os pipelines in a manifest to get the total steps we need to look at the resolved pipelines.

with this fix `bib` will report the correct number of steps left when doing e.g. a qcow2 image build. Right now the number of steps is incorrect because the osbuild manifest contains pipelines for qcow2,vdmk,raw,ami and all are currently considered steps that need to be completed. With this commit this is fixed.

C.f. https://github.com/osbuild/bootc-image-builder/pull/525

elkoniu: How it was calculated till now: `len(manifest.pipelines) + len(manifest.sources)`
How it is calculated in PR: `total_steps = len(manifest.sources) + len(pipelines)`

If those two are not the same?
mvo5: > How it was calculated till now: `len(manifest.pipelines) + len(manifest.sources)` How it is calculated in PR: `total_steps = len(manifest.sources) + len(pipelines)`
> 
> If those two are not the same?

Thank you, there is a subtle but important difference. The new code uses the "pipelines" that come from
```python
            pipelines = manifest.depsolve(object_store, exports)
```
which are the resolved pipelines that actually need to run. Before it was just taking the size of all pipelines which did potentially include pipelines that are not actually required. Eg. in the bootc-image-builder manifests we have raw,vdmk,qcow2 always in the manifest but if the user only asked for raw the current progress is incorrect as it was also counting vdmk,qcow2.

Patch:
diff --git a/osbuild.spec b/osbuild.spec
index 93880b17..c59774a5 100644
--- a/osbuild.spec
+++ b/osbuild.spec
@@ -1,7 +1,7 @@
 %global         forgeurl https://github.com/osbuild/osbuild
 %global         selinuxtype targeted
 
-Version:        124
+Version:        125
 
 %forgemeta
 
diff --git a/osbuild/__init__.py b/osbuild/__init__.py
index a2b2c711..10cdd8ce 100644
--- a/osbuild/__init__.py
+++ b/osbuild/__init__.py
@@ -10,7 +10,7 @@ independent of osbuild but used across the osbuild codebase.
 
 from .pipeline import Manifest, Pipeline, Stage
 
-__version__ = "124"
+__version__ = "125"
 
 __all__ = [
     "Manifest",
diff --git a/osbuild/main_cli.py b/osbuild/main_cli.py
index 28d5f740..3afd97c4 100644
--- a/osbuild/main_cli.py
+++ b/osbuild/main_cli.py
@@ -163,8 +163,6 @@ def osbuild_cli() -> int:
     monitor_name = args.monitor
     if not monitor_name:
         monitor_name = "NullMonitor" if args.json else "LogMonitor"
-    monitor = osbuild.monitor.make(monitor_name, args.monitor_fd, manifest)
-    monitor.log(f"starting {args.manifest_path}", origin="osbuild.main_cli")
 
     try:
         with ObjectStore(args.store) as object_store:
@@ -175,6 +173,9 @@ def osbuild_cli() -> int:
             debug_break = args.debug_break
 
             pipelines = manifest.depsolve(object_store, exports)
+            total_steps = len(manifest.sources) + len(pipelines)
+            monitor = osbuild.monitor.make(monitor_name, args.monitor_fd, total_steps)
+            monitor.log(f"starting {args.manifest_path}", origin="osbuild.main_cli")
 
             manifest.download(object_store, monitor, args.libdir)
 
diff --git a/osbuild/monitor.py b/osbuild/monitor.py
index fd1201c7..eebc4cc1 100644
--- a/osbuild/monitor.py
+++ b/osbuild/monitor.py
@@ -211,10 +211,9 @@ class TextWriter:
 class BaseMonitor(abc.ABC):
     """Base class for all pipeline monitors"""
 
-    def __init__(self, fd: int, manifest: Optional[osbuild.Manifest] = None) -> None:
+    def __init__(self, fd: int, _: int = 0) -> None:
         """Logging will be done to file descriptor `fd`"""
         self.out = TextWriter(fd)
-        self.manifest = manifest
 
     def begin(self, pipeline: osbuild.Pipeline):
         """Called once at the beginning of a pipeline"""
@@ -251,8 +250,8 @@ class LogMonitor(BaseMonitor):
     sequences will be used to highlight sections of the log.
     """
 
-    def __init__(self, fd: int, manifest: Optional[osbuild.Manifest] = None):
-        super().__init__(fd, manifest)
+    def __init__(self, fd: int, total_steps: int = 0):
+        super().__init__(fd, total_steps)
         self.timer_start = 0
 
     def result(self, result):
@@ -308,10 +307,10 @@ class LogMonitor(BaseMonitor):
 class JSONSeqMonitor(BaseMonitor):
     """Monitor that prints the log output of modules wrapped in json-seq objects with context and progress metadata"""
 
-    def __init__(self, fd: int, manifest: osbuild.Manifest):
-        super().__init__(fd, manifest)
+    def __init__(self, fd: int, total_steps: int):
+        super().__init__(fd, total_steps)
         self._ctx_ids: Set[str] = set()
-        self._progress = Progress("pipelines/sources", len(manifest.pipelines) + len(manifest.sources))
+        self._progress = Progress("pipelines/sources", total_steps)
         self._context = Context(origin="org.osbuild")
 
     def begin(self, pipeline: osbuild.Pipeline):
@@ -353,11 +352,11 @@ class JSONSeqMonitor(BaseMonitor):
         self.out.write("\n")
 
 
-def make(name, fd, manifest):
+def make(name: str, fd: int, total_steps: int) -> BaseMonitor:
     module = sys.modules[__name__]
     monitor = getattr(module, name, None)
     if not monitor:
         raise ValueError(f"Unknown monitor: {name}")
     if not issubclass(monitor, BaseMonitor):
         raise ValueError(f"Invalid monitor: {name}")
-    return monitor(fd, manifest)
+    return monitor(fd, total_steps)
diff --git a/setup.py b/setup.py
index d8f5e27a..d68ebc69 100644
--- a/setup.py
+++ b/setup.py
@@ -2,7 +2,7 @@ import setuptools
 
 setuptools.setup(
     name="osbuild",
-    version="124",
+    version="125",
     description="A build system for OS images",
     packages=["osbuild", "osbuild.formats", "osbuild.util"],
     license='Apache-2.0',


Test patch:
diff --git a/test/mod/test_monitor.py b/test/mod/test_monitor.py
index 7626d9fb..7f60c3f5 100644
--- a/test/mod/test_monitor.py
+++ b/test/mod/test_monitor.py
@@ -199,7 +199,7 @@ def test_json_progress_monitor():
     manifest.add_pipeline(pl2, "", "")
 
     with tempfile.TemporaryFile() as tf:
-        mon = JSONSeqMonitor(tf.fileno(), manifest)
+        mon = JSONSeqMonitor(tf.fileno(), len(manifest.sources) + len(manifest.pipelines))
         mon.log("test-message-1")
         mon.log("test-message-2", origin="test.origin.override")
         mon.begin(manifest.pipelines["test-pipeline-first"])