From 6ece001dccf59aa60e86a9fabfcfb9063f0bf799 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Mon, 16 Sep 2024 12:15:56 +0100 Subject: [PATCH 1/6] Collapse clean and non clean build requests on clean builders This is to address https://github.com/llvm/llvm-zorg/issues/250. Where build requests were piling up on slower builders because they were clean/don't clean/clean/ don't clean etc. and we could not merge those requests because of that difference. Galina suggested that if we knew that the builder was going to clean the build first anyway, we could merge them. This commit implements that. We do this by first setting a "clean" attribute on the builder's factory object, that we can find later via in the collapse callback. So far only ClangBuilder passes this on but any builder can opt in by copying what it does. In the collapse function when comparing properties we delete the "clean_obj" key from both sets of properties if we know that the builder's factory is always clean. With some logging (not included in this commit) we can see it now collapses a clean/non-clean pair of requests: ``` collapseRequests selfBuildSetProperties: {'scheduler': ('main:clang,clang-tools-extra,compiler-rt,flang,lld,llvm,mlir', 'Scheduler')} otherBuildSetProperties: {'scheduler': ('main:clang,clang-tools-extra,compiler-rt,flang,lld,llvm,mlir', 'Scheduler'), 'clean_obj': (True, 'Change')} Removing clean_obj from build set properties. Build set properties were the same. ``` This has beeen tested by my colleague Omair Javaid on a test build master. Of course we only tested it with one of Linaro's builders, so it's possible we see other side effects in production. --- zorg/buildbot/builders/ClangBuilder.py | 3 ++- zorg/buildbot/process/buildrequest.py | 22 ++++++++++++++-------- zorg/buildbot/process/factory.py | 6 +++++- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/zorg/buildbot/builders/ClangBuilder.py b/zorg/buildbot/builders/ClangBuilder.py index a003f3987..fffc14618 100644 --- a/zorg/buildbot/builders/ClangBuilder.py +++ b/zorg/buildbot/builders/ClangBuilder.py @@ -278,7 +278,8 @@ def _getClangCMakeBuildFactory( f = LLVMBuildFactory( depends_on_projects=depends_on_projects, llvm_srcdir='llvm', - enable_runtimes=enable_runtimes) + enable_runtimes=enable_runtimes, + clean=clean) # Checkout the latest code for LNT # and the test-suite separately. Le's do this first, diff --git a/zorg/buildbot/process/buildrequest.py b/zorg/buildbot/process/buildrequest.py index 594a09d25..1f5043d11 100644 --- a/zorg/buildbot/process/buildrequest.py +++ b/zorg/buildbot/process/buildrequest.py @@ -22,14 +22,20 @@ def collapseRequests(master, builder, req1, req2): ('buildsets', str(req2['buildsetid']))) # Fetch the buildset properties. - selfBuildsetPoperties = yield \ - master.db.buildsets.getBuildsetProperties( - str(req1['buildsetid']) - ) - otherBuildsetPoperties = yield \ - master.db.buildsets.getBuildsetProperties( - str(req2['buildsetid']) - ) + selfBuildsetPoperties = yield master.db.buildsets.getBuildsetProperties( + str(req1["buildsetid"]) + ) + otherBuildsetPoperties = yield master.db.buildsets.getBuildsetProperties( + str(req2["buildsetid"]) + ) + + # If the build is going to be a clean build anyway, we can collapse a clean + # build and a non-clean build. + if getattr(builder.config.factory, "clean"): + if 'clean_obj' in selfBuildsetPoperties: + del selfBuildsetPoperties["clean_obj"] + if 'clean_obj' in otherBuildsetPoperties: + del otherBuildsetPoperties["clean_obj"] # Check buildsets properties and do not collapse # if properties do not match. This includes the check diff --git a/zorg/buildbot/process/factory.py b/zorg/buildbot/process/factory.py index b15bbaa19..e8af8faa0 100644 --- a/zorg/buildbot/process/factory.py +++ b/zorg/buildbot/process/factory.py @@ -35,11 +35,15 @@ class LLVMBuildFactory(BuildFactory): & etc. """ - def __init__(self, steps=None, depends_on_projects=None, hint=None, **kwargs): + def __init__( + self, steps=None, depends_on_projects=None, hint=None, clean=False, **kwargs + ): # Cannot use "super" here as BuildFactory is an old style class. BuildFactory.__init__(self, steps) self.hint = hint + + self.clean = clean # Handle the dependencies. if depends_on_projects is None: From 30087b123514e378c153c31e374dcdd0287a5744 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Mon, 7 Oct 2024 11:45:17 +0100 Subject: [PATCH 2/6] Fix getattr default --- zorg/buildbot/process/buildrequest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zorg/buildbot/process/buildrequest.py b/zorg/buildbot/process/buildrequest.py index 1f5043d11..f9d6555c7 100644 --- a/zorg/buildbot/process/buildrequest.py +++ b/zorg/buildbot/process/buildrequest.py @@ -31,7 +31,7 @@ def collapseRequests(master, builder, req1, req2): # If the build is going to be a clean build anyway, we can collapse a clean # build and a non-clean build. - if getattr(builder.config.factory, "clean"): + if getattr(builder.config.factory, "clean", False): if 'clean_obj' in selfBuildsetPoperties: del selfBuildsetPoperties["clean_obj"] if 'clean_obj' in otherBuildsetPoperties: From 104d7d57639bbaa0ed95ffa610496dbe39be50c4 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Tue, 8 Oct 2024 10:24:47 +0100 Subject: [PATCH 3/6] Handle clean like the other data passed in kwargs --- zorg/buildbot/process/factory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zorg/buildbot/process/factory.py b/zorg/buildbot/process/factory.py index e8af8faa0..6d647c72b 100644 --- a/zorg/buildbot/process/factory.py +++ b/zorg/buildbot/process/factory.py @@ -36,14 +36,14 @@ class LLVMBuildFactory(BuildFactory): """ def __init__( - self, steps=None, depends_on_projects=None, hint=None, clean=False, **kwargs + self, steps=None, depends_on_projects=None, hint=None, **kwargs ): # Cannot use "super" here as BuildFactory is an old style class. BuildFactory.__init__(self, steps) self.hint = hint - self.clean = clean + self.clean = kwargs.pop('clean', False) # Handle the dependencies. if depends_on_projects is None: From 23eab2fdbeb3913808c1bf3bf1ff0b6ed8e92b03 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Tue, 8 Oct 2024 10:25:51 +0100 Subject: [PATCH 4/6] undo formatting change to unchanged code --- zorg/buildbot/process/factory.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/zorg/buildbot/process/factory.py b/zorg/buildbot/process/factory.py index 6d647c72b..a2ac70ca8 100644 --- a/zorg/buildbot/process/factory.py +++ b/zorg/buildbot/process/factory.py @@ -35,9 +35,7 @@ class LLVMBuildFactory(BuildFactory): & etc. """ - def __init__( - self, steps=None, depends_on_projects=None, hint=None, **kwargs - ): + def __init__(self, steps=None, depends_on_projects=None, hint=None, **kwargs): # Cannot use "super" here as BuildFactory is an old style class. BuildFactory.__init__(self, steps) From 2dd5609701060095385ee03f00c7165798d11454 Mon Sep 17 00:00:00 2001 From: David Spickett Date: Wed, 9 Oct 2024 09:19:20 +0100 Subject: [PATCH 5/6] undo more formatting changes --- zorg/buildbot/process/buildrequest.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/zorg/buildbot/process/buildrequest.py b/zorg/buildbot/process/buildrequest.py index f9d6555c7..ad04f9b04 100644 --- a/zorg/buildbot/process/buildrequest.py +++ b/zorg/buildbot/process/buildrequest.py @@ -22,12 +22,14 @@ def collapseRequests(master, builder, req1, req2): ('buildsets', str(req2['buildsetid']))) # Fetch the buildset properties. - selfBuildsetPoperties = yield master.db.buildsets.getBuildsetProperties( - str(req1["buildsetid"]) - ) - otherBuildsetPoperties = yield master.db.buildsets.getBuildsetProperties( - str(req2["buildsetid"]) - ) + selfBuildsetPoperties = yield \ + master.db.buildsets.getBuildsetProperties( + str(req1['buildsetid']) + ) + otherBuildsetPoperties = yield \ + master.db.buildsets.getBuildsetProperties( + str(req2['buildsetid']) + ) # If the build is going to be a clean build anyway, we can collapse a clean # build and a non-clean build. From a734ce4687d9cac617c4394cf40edca0d834fa3b Mon Sep 17 00:00:00 2001 From: David Spickett Date: Wed, 9 Oct 2024 09:20:14 +0100 Subject: [PATCH 6/6] more formatting --- zorg/buildbot/process/buildrequest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zorg/buildbot/process/buildrequest.py b/zorg/buildbot/process/buildrequest.py index ad04f9b04..ebd6371ef 100644 --- a/zorg/buildbot/process/buildrequest.py +++ b/zorg/buildbot/process/buildrequest.py @@ -23,8 +23,8 @@ def collapseRequests(master, builder, req1, req2): # Fetch the buildset properties. selfBuildsetPoperties = yield \ - master.db.buildsets.getBuildsetProperties( - str(req1['buildsetid']) + master.db.buildsets.getBuildsetProperties( + str(req1['buildsetid']) ) otherBuildsetPoperties = yield \ master.db.buildsets.getBuildsetProperties(