Opened 4 years ago

Last modified 3 years ago

#3529 new enhancement

Add warning in trigger step if collapseBuild=True for the target builder — at Initial Version

Reported by: tardyp Owned by:
Priority: major Milestone: 0.9.+
Version: master Keywords:


as per discussion

@djmitche> #topic collapseBuildRequest=False by default 18:41 <@djmitche> now, what WAS the topic 18:42 <@tardyp> collapseBuildRequest=True right now 18:42 <@djmitche> I shouldn't be allowed on irc 18:42 <@tardyp> and this does not play well with triggered build 18:42 <@tardyp> So people which use trigger step will all have the problem (I did 4 years ago) 18:42 <@tardyp> and ed did this week 18:43 @djmitche set the topic: A Software Freedom Conservancy Project | Buildbot-0.8.12 | docs: | tutorial: |[…] | meetings: Tuesdays 1630 UTC / 18:43 <@tardyp> I would recommend to put it False by default, and to let people set it to true when they need to optimize 18:43 ⇐ @jaredgrubb quit (~Adium@ Quit: Leaving. 18:43 <@djmitche> changing a default is pretty ugly though 18:44 @djmitche set the topic: A Software Freedom Conservancy Project | Buildbot-0.8.12 | docs: | tutorial: | | meetings: Tuesdays 1630 UTC / 18:44 → bb-github joined (~bb-github@ 18:44 <bb-github> [buildbot] mention-bot commented on issue #2141: By analyzing the blame information on this pull request, we identified @tomprince, @djmitche and @rutsky to be potential reviewers 18:44 ← bb-github left (~bb-github@ 18:44 <@djmitche> is it possible to just disable it with triggered builds? 18:45 <@djmitche> my worry is, changing the default is pretty subtle, and will change behavior even for people not using triggered builds 18:45 <@tardyp> I tried to fix it that way, but there is no real way to be sure that a build is comming from a trigger step (from within the colapse br) 18:45 <cmouse> tardyp: i'm using the buildbot reporter myself, and it mostly works 18:45 <@tardyp> it will change the optimisation 18:45 <cmouse> tardyp: the only thing that is not working is that it's not reporting start of build 18:45 <@tomprince> The trigger scheduler could annotate the BR. 18:45 <cmouse> tardyp: i have no idea why it's not doing it :( 18:45 <@tardyp> while for people using trigger step, it just skip builds 18:46 <@djmitche> even as a temporary hack, I think that makes sense 18:46 <myheadhurts> So I'm running into the following issue: and I'm thinking of approaching the fix in the following way: 18:46 <myheadhurts> Would love any feedback on the approach 18:47 <@tardyp> ok. So we change the True behaviour to : compatible sourcestamps *and* not coming from a Trigger step 18:47 <@djmitche> #info build request collapsing and triggered builds do not get along well -- triggered builds end up skipped 18:47 <@tardyp> I'm fine with that too. 18:47 <@djmitche> tardyp: I think so -- that's even a good medium-term fix (since it sounds like this is architectural, not just a bug) 18:47 → poz2k4444 joined (uid22701@gateway/web/ 18:47 <bb-trac> [trac] #3528/undecided (new) updated by rutsky (Related PR: 18:48 <@djmitche> #agreed will configure collapsing to skip triggered builds by annotating the triggered builds 18:48 <@tomprince> If people want collapsing builds, then it isn't clear that skipping triggered builds isn't desired behavior. 18:48 <@djmitche> myheadhurts: without looking deeply at the PR, I'm really happy to see you working on the latent worker support :) 18:48 <@bdbaddog> so if you have multiple triggered builds waiting, they would no longer be collapsed? 18:48 <@tardyp> tomprince: I dont see a usecase where people would like to skip triggerred build 18:49 <@djmitche> a skipped triggered build has weird semantics if waitForFinish=True 18:49 <@djmitche> or whatever that parameter is 18:49 <@bdbaddog> yes.. deadlock.. 18:49 <@tardyp> well no deadlock 18:49 <@tomprince> If you have a multi-stage build pipeline, but only care about results on the latest commit, if several triggers happen before the triggered build runs, there isn't a reason to run the earlier triggers. 18:49 <myheadhurts> djmitche: Hopefully soon (once company approves it) I'll have a PR for upgrading boto2 -> boto3 and handling cross-account support for AWS. :) 18:50 <@djmitche> oo, very nice! 18:50 <@djmitche> so regarding collapsing and triggering -- it sounds like this is something we should defer until 0.9.x 18:50 <@bdbaddog> parent build would hang forever if triggered build gets skipped.. 18:50 <@tomprince> Probably doesn't make sense in the waitForFinish case (at least when that is being used for synchronization not just reporting). 18:50 <@djmitche> right 18:50 <@tomprince> Doesn 18:51 <@tomprince> t it just wait for the buildrequest to be completed, which does happen for collapsed requests? 18:51 → @jaredgrubb (opped) joined 18:51 <@tardyp> yes 18:51 — @tomprince can't remember the code, but seems to recall that was how things worked 18:51 <@tardyp> you will just see 4 builds when you awaited 8 18:52 <@tardyp> because the colapsing dont create build, it directly skips the br 18:52 <@tardyp> and that is fine with the waiting logic in trigger step, as it only waits for a complete event of the br 18:53 → bb-github joined (~bb-github@ 18:53 <bb-github> [buildbot] tomprince commented on issue #2141: I don't think this a correct fix. I think the intent of build_wait_timeout == 0 is that the worker will only run a single build. I don't think this change preserves that behavior. 18:53 ← bb-github left (~bb-github@ 18:54 <@tardyp> it actually waits for the buildset 18:54 <@djmitche> so how badly broken is it right now? 18:54 <@djmitche> It sounds like it works, it's just not the semantics you're expecting 18:54 <@djmitche> as in, you're expecting every triggered build to actually occur, and instead they get collapsed 18:55 <@tardyp> well you start 8 builds with trigger, you have no idea what collapsing is, and eventually you get 4 builds, and buildbot seems happy 18:55 <@tardyp> that is for me very weird for a new commer 18:55 <@tardyp> build request collapsing should for me be an advanced feature, that new commers should not be aware of 18:56 <@tomprince> I think that not collapsing is probably a slightly better default, but it isn't clear wether it is enough better to warrant *changing* the default. 18:56 <@djmitche> right 18:56 <@djmitche> we're already changing so much other stuff 18:56 <@djmitche> I think we should leave this be for now 18:56 <@tomprince> Given the experience of python 3, that seems reasonable. 18:57 <@tomprince> If you want to change the default, a slightly better option might be to deprecate having a default at all, and then remove the default. 18:57 <rutsky> can we somehow warn user that he might get not what he expected? 18:57 <rutsky> or add appropriate warning in docs? 18:57 <@tardyp> that is a good idea 18:57 <@tomprince> That seems sensible. 18:58 → bb-github joined (~bb-github@ 18:58 <bb-github> [buildbot] aelsabbahy commented on issue #2141: Hmm, that's definitely not what I want then. Since it's very important for us that we only have single use workers.... 18:58 ← bb-github left (~bb-github@ 18:58 <@tardyp> in the trigger step, if the triggered builder has True in collapse, we warn in the stdio 18:58 <@djmitche> that sounds good 18:58 <cmouse> should write a trac issue about OpenStackLatentWorker? not always killing the worker. 18:58 <@tardyp> with link to the doc

Change History (0)

Note: See TracTickets for help on using tickets.