Collapse clean and non clean build requests on clean builders

[DavidSpickett]

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.

[joker-eph]

Why would we prevent from collapsing clean/non-clean builds?

[DavidSpickett]

We already merge:

unclean
unclean

And

clean
clean

As the properties matched.

Now we want to merge:

clean
unclean

Assuming unclean merges into clean, and the result is also clean, we could do this for any builder even if it does not clean by default. In current production we wouldn't because the properties don't match.

unclean
clean

This we can only merge if the builder itself would clean anyway, or if we are able to edit the request after merging to add clean_obj Currently it's just a boolean collapse or not.

I will see if I can edit the collapsed request, then this can apply globally not just to builders marked clean.

[DavidSpickett]

I'm going to see if it's feasible to modify the request that we collapse into, to address Mehdi's question.

And to be clear, Galina has the deciding approval here.

[DavidSpickett]

I'm going to see if it's feasible to modify the request that we collapse into, to address Mehdi's question.

This is the buildbot function that calls the callback I'm modifying in this PR: https://github.com/buildbot/buildbot/blob/master/master/buildbot/process/buildrequest.py#L66

If we want to collapse unclean -< clean, we'd need to change this callback to return a new request with the clean_obj added. Or make the default some sort of superset of all options of both requests.

But I'd rather not get into changing Buildbot itself (or trying to monkeypatch it)

Why would we prevent from collapsing clean/non-clean builds?

@joker-eph tell me if I've answered this or not. Might have gone on a sidetrack there.

[DavidSpickett]

One sentence answer based on current buildbot behaviour: "The result of a request collapse retains the properties of the request being collapsed into, if that request is unclean we would lose the clean property of the clean request that was collapsed."

That is unless we knew that the builder would clean anyway, which is what this PR checks for.

[joker-eph]

@joker-eph tell me if I've answered this or not. Might have gone on a sidetrack there.

Yes, this is exactly what I was looking for, thanks.

[DavidSpickett]

In the mean time I staged the patch to see it in action.

I've cleared the queue for one of our affected builders, https://lab.llvm.org/staging/#/builders/75, and will see how the queue develops.

[vitalybuka]

Is this on staging? just started bots on staging https://lab.llvm.org/staging/#/builders/7 and see

[vitalybuka]

Is this on staging?

My apologies, I forgot to merge #257.

[vitalybuka]

Thanks, David! The patch works on the staging well.

It does not look like it works. https://lab.llvm.org/staging/#/builders/7 Is this rely on staging?

[gkistanova]

It does not look like it works. https://lab.llvm.org/staging/#/builders/7 Is this rely on staging?

You patch to make that builder clean was not on the staging yet. It is now. I removed the current build queue, so we can see how it will go further on.

Let's wait for another day to make sure everything is fine. I'll be surprised if it is not, though.

[gkistanova]

However, since sanitizer-buildbot9 is offline, no build requests is going to be scheduled. Is there a way to get that worker online on staging?

[vitalybuka]

Any estimates when it will reach lab.llvm.org/buildbot? Asking if we need to return workaround #295.

[gkistanova]

I think this is ready to merge.

It could be in production on Friday late night or Staurday morning.

[DavidSpickett]

Sorry I was away for a long weekend. Linaro's bots also seemed to have no problems with this, didn't break our non-clean builders either.

I will merge this now.