IRC logs for #buildstream for Tuesday, 2019-07-23

*** kapip has joined #buildstream03:35
*** toscalix has joined #buildstream07:17
*** bochecha has joined #buildstream07:28
laurenceGitlab 12.1 released07:50
laurencehttps://about.gitlab.com/2019/07/22/gitlab-12-1-released/07:50
laurenceParallel MERGE TRAINS07:50
laurenceJust trying to figure out if Gitlab now automatically comes with merge trains or we need to set something up in the same way we did with marge07:53
*** kapip has quit IRC07:54
*** alexandrufazakas has joined #buildstream07:57
*** alexandrufazakas has quit IRC08:00
laurencealexandrufazakas, morning :)08:01
laurenceare you familiar with the marge bot that we have in place on the project?08:01
laurencethat was basically a 'work-around' that someone implemented to solve the issue of having a large and active development team, where many merge requests would be submitted at once08:02
laurenceand the problem was that by the time your branch passed CI and was about to merge, another branch finished earlier and merged before you, meaning you'd have to go back and start again08:02
laurencemarge bot solved it with a queuing system08:03
laurenceBut it was unofficial, we had to set up a bot as a new Gitlab user (Marge) on a VM etc etc08:03
laurenceSo now Gitlab have introduced Merge Trains, it would be nice to utilise that instead, and then we can pull marge bot down08:04
*** alexandrufazakas has joined #buildstream08:17
*** traveltissues has joined #buildstream08:20
gitlab-br-bottraveltissues opened issue #1088 (Do not force reset workspace cache data) on buildstream https://gitlab.com/BuildStream/buildstream/issues/108808:31
benschubertlaurence: I saw that, the feature seems really nice :D08:36
benschubertand parallel testing with the result of all branches08:37
benschuberttraveltissues: I believe you benchmarked #1088, correct? do you have the numbers by any chance? :)08:38
laurencebenschubert, yeah, definitely worth upgrading to :)08:38
laurencethanks to jjardon who pointed it out to me08:38
traveltissuesi did not, i'll update that08:48
benschuberttraveltissues: oh I thought you had, sorry about that08:50
traveltissuesi haven't managed to run benchmarking yet08:51
benschubertproblems with the script?08:51
alexandrufazakaslaurence: I read everything about the merge trains and I get the idea. Is there you wanted me to do regarding these or just know the new workflow?08:51
alexandrufazakass/there you/there something you/ rather08:52
laurencealexandrufazakas, I think we definitely want to start using the merge trains08:53
laurencenot sure exactly what that migration would look like08:53
traveltissuesbenschubert, i didn't have time to run that last night but yes, i had one so far08:53
laurenceI think it just means amending some settings and testing it...then if all goes well we can email the list and announce how cool they are, then we deprecate marge-bot08:53
traveltissuessent an mr08:53
laurence(which is an email to Codethink Operations team and they have donated computing power for it)08:54
benschubertoh thanks traveltissues !08:55
tpollardooo Merge Trains09:07
*** jonathanmaw has joined #buildstream09:25
*** lachlan has joined #buildstream09:28
tpollardhas anyone had issues with the cache expiry tests recently?09:55
benschuberttpollard: I have one that fails intermitently, what's the one that's failing for you?09:56
tpollardcurrently have 4, but I have made some changes. Just wondered if there was a underlying issue somewhere09:57
benschubert4 seems a lot, I usually get always the same one failing09:58
*** lachlan has quit IRC09:59
tpollardyep I'll keep digging for now10:00
tpollard:)10:00
*** lachlan has joined #buildstream10:07
alexandrufazakasWhy do we lint only on master changes?10:11
alexandrufazakasin .gitlab-ci.yml, that is10:12
Kinnisonwe lint on MRs too10:22
*** phoenix has joined #buildstream10:23
alexandrufazakasKinnison: you're right. I must've read it wrong10:23
* alexandrufazakas sighs10:24
alexandrufazakasSo I've implemented merge trains on a smaller project I used to fetch and publish bst documentation and everything works fine as far as I've tested it. I'm trying to make it work on my bst fork to make sure nothing blows up when implementing it on the actual repo10:26
alexandrufazakasAnd the changes seem to be fairly minimal10:26
*** phoenix has quit IRC10:33
*** phoenix has joined #buildstream10:42
*** phoenix has quit IRC10:44
alexandrufazakasOkay, I'm not sure I can test my implementation regarding merge trains as I don't have the same runners as the bst repo and I'm not sure if I can set them up easily. I believe it should just be setting these jobs run on merge_requests https://pastebin.com/2sgTDLnQ and changing that setting mentioned in the Gitlab documentatoin. Am I missing anything?11:00
alexandrufazakasA second opinion from anyone good with gitlab CI would be great as I'm not well versed in it or anything :D11:01
traveltissuesnot related but btw gitlab has gists11:01
alexandrufazakastraveltissues: good point, I'll use that instead of pastebin next time :P11:02
alexandrufazakasthank you11:02
laurencealexandrufazakas - jjardon knows gitlab CI very well11:03
laurencejjardon, are you able to have a wuick look?11:03
laurenceq11:03
laurencequick *11:03
alexandrufazakasAwesome. thanks laurence11:03
jjardonalexandrufazakas: I think that's everything you need;  if you have developer access, you can create a branch directly from the buildstream repo11:10
alexandrufazakasjjardon: Thanks for having a look. Do you mean creating a branch in order to open an MR on it and push the changes?11:12
jjardonyes11:12
alexandrufazakasOh, yeah, I do have that11:12
alexandrufazakasI was wondering if you'd have any idea how I could test these, however11:12
alexandrufazakasOr, if it looks alright with you, I can try to set it up and see how everything goes11:13
alexandrufazakasjjardon ^11:13
jjardoncheck you can see the "Start/Add to merge train" on that MR11:13
jjardonSee https://docs.gitlab.com/ee/ci/merge_request_pipelines/pipelines_for_merged_results/merge_trains/11:13
alexandrufazakasjjardon: Oh, I assumed that would become effective *after* merging that particular MR11:14
jjardonmmm, you might be right. you migth create a fork of the project on your namespaces and use the gitlab runners instead ours?11:15
alexandrufazakasjjardon: That's what I wanted to do at first, but I need specific runners11:15
alexandrufazakasSuch as the windows ones11:15
jjardonalexandrufazakas: remove that job for the test11:15
alexandrufazakas...or I could skip those jobs11:15
alexandrufazakasAlright11:15
alexandrufazakasI'll try doing that11:15
alexandrufazakasjjardon: thanks for the help11:15
jjardonthe linux runners are notthing special, as fas as I know. Only faster than using the gitlab.com free ones11:16
juergbivalentind, jjardon: bst-artifact-server uses 'headroom' to determine how much to expire while bst uses a quota. do you remember the reason for this difference?11:44
juergbiwas it only because we didn't have efficient disk usage tracking and due to this we went for the efficient 'free space' approach?11:44
valentindTo avoid every push to be slow.11:44
juergbibuildbox-casd precisely tracks used disk space without significant overhead11:45
juergbiso I'm wondering whether we'd all be ok in switching the server to be quota based as well11:45
valentindBecause we look for objects and sort them, making sure we clean-up a bit more than required makes sure that we do not do that again.11:46
valentindThe listing and sorting of object I think is the slow part.11:47
juergbiyes, that approach will pretty much remain the same11:47
juergbithe only difference would be whether the two levels are based on free disk space (headroom) or based on disk usage (quota)11:47
juergbiI think quota is nicer from a configuration perspective as the concept also works if you run multiple servers on the same partition (could be different kinds of servers, not necessarily multiple artifact servers)11:49
valentindAh right.11:49
valentindI mixed up.11:49
valentindYes headroom is the quota.11:49
valentindNote that some servers share disk space with other things. For example for us we have a local cache server on each builder. And we have a bigger free space there. And if some other things start using more space then at some point we will clean more.11:50
juergbiok but wouldn't it make more sense that each thing has its own quota?11:51
juergbiotherwise the distribution of used disk space among these different things would be kind of random11:51
juergbi(if each of those used the same 'free space' based approach)11:52
juergbi(it kinda works if only one uses that approach, I suppose, but it's still somewhat odd)11:52
valentindI think it is fine if you remove this in the new version. We can adapt. But there are some cases where it is fine.11:53
valentindAnd for us it was nice because we do not know how many docker machines will run and how much space is taken.11:54
valentindSo taking more space for cache when there are less builds was nice.11:54
valentindOf course if you have several services that do the same, take as much as space as they can, then you can run into troubles.11:54
valentindBut it is rare unless it is a cache.11:54
juergbiok. it would probably not really be difficult to re-add support for it but it would also require an addition on the buildbox-casd side, and if we can do without, I'd rather leave it11:56
juergbiif it turns out to be needed after all, we can revisit11:56
*** lachlan has quit IRC12:02
*** brlogger has joined #buildstream12:27
*** phoenix has joined #buildstream12:41
gitlab-br-botAlexFazakas opened MR !1494 (alexfazakas/test-merge-trains_2->master: Alexfazakas/test merge trains 2) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149412:50
gitlab-br-botAlexFazakas closed MR !1494 (alexfazakas/test-merge-trains_2->master: Alexfazakas/test merge trains 2) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149412:50
gitlab-br-botAlexFazakas opened MR !1495 (alexfazakas/test-merge-trains_2->master: Alexfazakas/test merge trains 2) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149512:51
gitlab-br-botAlexFazakas closed MR !1495 (alexfazakas/test-merge-trains_2->master: Alexfazakas/test merge trains 2) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149512:52
alexandrufazakasUgh, sorry about the spam everyone, I meant to open an MR on my fork :/12:52
gitlab-br-botAlexFazakas opened MR !1496 (alexfazakas/use-merge-trains->master: Use Gitlab merge trains) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149613:04
alexandrufazakasSo I've just checked `Merge pipelines will try to validate the post-merge result prior to merging` on the buildstream repository. once !1496 lands, we should be able to use Gitlab's merge train, I believe13:05
alexandrufazakasActually13:10
alexandrufazakasI should probably uncheck that, merge that request13:10
alexandrufazakasAnd then turn it back on13:10
alexandrufazakasSince it looks like I *have to* start a merge train in order to merge !1496, which contains the .yml changes the merge train *needs* in order to work. Awesome.13:11
alexandrufazakasNot sure if owners or maintainers get emails/notifications regarding these changes. Sorry if that's the case.13:12
alexandrufazakasjjardon: If you could have a look at https://gitlab.com/BuildStream/buildstream/merge_requests/1496 just to make sure I'm messing anything up, I'd appreciate it. Also 2 jobs failed due to `docker daemon not running`. Should I just rerun them?13:15
alexandrufazakasnot messing anything up, even13:15
benschubertalexandrufazakas: restarting jobs is fine in that case, some of our runners tend to have such problems13:16
alexandrufazakasbenschubert: Oh, okay. Thank you :)13:17
traveltissuesi'm not sure linting needs to happen except on merge_requests if we're using the train though13:18
gitlab-br-bottraveltissues approved MR !1496 (alexfazakas/use-merge-trains->master: Use Gitlab merge trains) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149613:18
benschubertbut why would we not want tests on branches that are not part of MRs?13:18
benschubertlike, how do we test code _before_ adding a MR?13:18
traveltissuesi thought the philosophy people wanted for this was that everything would be mr13:19
benschubertWe already have dozens of them rotting away, I'm not sure forcing people to open one to test a change is a wise thing to do13:20
traveltissuesit's a fair point13:20
*** phoenix has quit IRC13:20
traveltissuesand i agree13:20
alexandrufazakastraveltissues, benschubert: Yes, that's what I thought we'd have as well. If you wanted to test something, open an MR on it13:21
traveltissuesalthough there is still some amount of local testing13:21
laurencewe never had a strong policy on MRs in this regard....people started using WIP MRs because they offer a nice way to review work13:21
alexandrufazakastraveltissues: thanks for approving that13:21
laurencebut yeah we have a lot of stale ones13:22
laurencewhich i try to police every now and then13:22
benschuberttraveltissues: running integration tests on my system is extremely taxing, with proxies problems + having to run them in docker through windows and roughly takes 2.5 hours to complete13:22
laurencebut i think we certainly want local testing of your branch, prior to an MR13:22
traveltissuesyes, proxywoes13:22
traveltissuesi forgot13:22
laurenceor, maybe not local, but pre-MR for sure13:23
benschubertlaurence: exactly my point, we should have tests on branches :)13:25
laurencebenschubert, yes, i will comment on the ticket now13:25
gitlab-br-bottraveltissues unapproved MR !1496 (alexfazakas/use-merge-trains->master: Use Gitlab merge trains) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149613:26
traveltissueshow about only: branches except: master13:27
alexandrufazakasbenschubert, laurence: Yeah, makes sense. So we *always* want to run tests, right?13:27
benschuberttraveltissues: why except master? (I don't particularily mind, even though I'd like to have master always green :) )13:28
traveltissuesif the mrs are in the train then is there any purpose to rerunning the same tests on master as they land?13:30
traveltissues(except overnight/master specific things ofc)13:31
benschubertfair point, then except master seems fine13:32
laurenceMRs should always run the pipeline, but I think if we say 'hey developer, you can't run any tests until you create an MR' then that breaks people's current workflows13:32
SotKcan people still push straight to master?13:33
alexandrufazakasI updated the MR as per traveltissues' suggestion for now, running tests for anything other than master13:34
alexandrufazakasOh, I messed up the syntax13:35
alexandrufazakasthank you traveltissues13:35
benschubertSotK: I don't think that was ever possible here? At least not now13:37
SotKah I'm probably misremembering then, I thought here was the place where some people had enough permission to do so for quickly applying small fixes13:40
traveltissuesnp13:41
traveltissuesi'd hope only marge could13:41
*** lachlan has joined #buildstream13:42
alexandrufazakastraveltissues: um, not sure I understand these last discussions and the `apply suggestion` option doesn't help. Do you want me to replace `merge_requetsts` with `branches` everywhere?13:45
*** traveltissues has quit IRC13:57
alexandrufazakasOkay, so we just run everything for *any* branch now. Excepts for tests, which do not run on master. Does that make sense?13:58
*** traveltissues has joined #buildstream13:58
benschubertwhat's the "only: branch" ? Why?13:59
alexandrufazakasbenschubert: Isn't that what traveltissues suggested on the MR?14:01
benschubertI'm just not sure what this brings?14:01
alexandrufazakasAnd isn't that what you described earlier when you wanted to run tests without opening a MR?14:01
benschubertYes, but we already have this behavior14:01
benschubertso I would have expected 'expect:master' only for the tests14:01
alexandrufazakasBeing the only change?14:02
traveltissuesit's not technically equivalent aiui14:02
benschuberttraveltissues: ah?14:02
traveltissuesonly: \  - branches is not the default i think14:03
traveltissueswhich is why i thought only: \  - branches \ except: \  - master was more explicitly what was wanted14:03
traveltissuesfor everything except the overnight and the master specific wsl14:04
alexandrufazakashttps://gitlab.com/gitlab-org/gitlab-ce/commit/d85e04e45247a64c704a44a2c8abecba9497206114:04
benschubertI'm just not sure why we want to clutter the gitlab.yml for something that would not change anything? I mean, it would stop doing for 'tag', but apart from that14:04
alexandrufazakasApparently default is only : \ - branches \ - tags?14:04
traveltissuesi don't agree that it's clutter but i don't think it matters too much. in that case sure, just the except14:05
alexandrufazakasIf that's the only change needed, I think we can already start using merge trains, right? it's just that some tests will be run twice until this MR is accepted14:36
*** lachlan has quit IRC14:38
alexandrufazakasWould it be okay with everyone if I enabled them?14:44
alexandrufazakasbenschubert, traveltissues?14:44
laurenceI think now could be a good time for a mailing list post, alexandrufazakas14:45
laurencePlease outline the changes made, and any changes to the developer workflow14:45
gitlab-br-bottraveltissues approved MR !1496 (alexfazakas/use-merge-trains->master: Use Gitlab merge trains) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149614:46
laurenceWe can say that we have enabled this already since there was a broad consensus on irc, if any issues arise we can address them quickly, etc etc14:46
laurenceassuming we do know how to revert things with no adverse impact ?14:47
traveltissuesah, i forgot, coverage will fail14:47
alexandrufazakaslaurence: Sure, I'll do that in a second14:47
alexandrufazakasOkay, well, I'll enable `Merge pipelines will try to validate the post-merge result prior to merging` for now if it's alright14:48
alexandrufazakasAnd let everyone on the mailing list know14:48
alexandrufazakasmarge bot isn't assigned to any MR right now14:49
alexandrufazakasSo that's good14:49
alexandrufazakasI'll start a merge train with my MR14:49
alexandrufazakaslaurence: Btw, you should probably downgrade my access back to developer now14:49
*** lachlan has joined #buildstream14:51
laurencealexandrufazakas, ok, done14:52
alexandrufazakasThanks14:52
benschuberttraveltissues: coverage will fail?15:08
laurencealexandrufazakas, nice work! all aboard the merge train!15:11
laurencesorry, someone had to say it15:11
alexandrufazakashaha thanks laurence15:11
alexandrufazakasErgh, code_quality and remote-execution keep failing as there's no docker daemon :(15:12
alexandrufazakasbenschubert: I think coverage is going to fail as it depends on remote-execution15:12
*** lachlan has quit IRC15:13
alexandrufazakasbenschubert: traveltissues was saying it would fail as the other tests did not run earlier, so there was nobody to create ./coverage-reports which the coverage job is looking for15:14
*** lachlan has joined #buildstream15:15
benschubertthen, we still need the tests on master, or add the other one on merges, or what is the plan?15:15
alexandrufazakasbenschubert: Hmm, I think you are right. If these don't run on master as well coverage will always fail there15:17
traveltissuesi suggested just except: master for coverage15:23
traveltissuesbenschubert, i have the benchmark for #1088 in the description15:24
gitlab-br-botIssue #1088: Do not force reset workspace cache data https://gitlab.com/BuildStream/buildstream/issues/108815:24
alexandrufazakasbenschubert: I was just telling traveltissues as well, is there a reason why we'd want anything other than the overnight tests on master?15:26
alexandrufazakass/telling/asking15:26
alexandrufazakasI guess an argument could be made for people pushing changes straight to master, but then again, you wouldn't push something that needs testing I'd assume?15:27
benschubertalexandrufazakas: I don't think _anyone_ has the rights to push to master, and this is not an acceptable workflow as per our contributing guide.15:28
benschubertI think having coverage reporting on master is still important (the badges on the README), so we do need some tests there15:28
benschuberttraveltissues: cheers! master being the bad branch, correct?15:28
benschubertand the other one, the one before the modifications15:29
traveltissuesyes, after i reverted one of the changes15:29
traveltissuesthe benchmark branch is prior to that15:29
benschubertok!15:29
benschubertGah, that's a pretty bad regression15:29
alexandrufazakasbenschubert: you're right about the badges. thanks for pointing that out15:29
traveltissuesnormal config was taking too long on my machine15:29
alexandrufazakasGiven the current state of things, I'll try to implement merge trains on my fork *without* changing the CI at all and see if everything works out fine15:46
traveltissuesbtw, this might be relevant15:50
traveltissueshttps://gitlab.com/gitlab-org/gitlab-ce/issues/5822615:50
alexandrufazakasI'll have a look at that15:52
benschubertseems like we will need to wait for this :/15:52
traveltissuesit looks that way15:52
gitlab-br-bottraveltissues unapproved MR !1496 (alexfazakas/use-merge-trains->master: Use Gitlab merge trains) on buildstream https://gitlab.com/BuildStream/buildstream/merge_requests/149615:53
*** phoenix has joined #buildstream15:58
*** tristan has quit IRC16:05
*** bochecha has quit IRC16:09
alexandrufazakasSeeing as we'll postpone !1496 for now, can one of the maintainers/owners disable the pipeling for merged results settings in the bst repo so nobody tries to use merge trains for now?16:11
alexandrufazakasDo we need to send out another email as we're not implementing it rn, laurence?16:12
alexandrufazakass/pipeling/pipelining16:14
* laurence reads backscroll 16:18
laurenceso we can't use merge trains?16:18
alexandrufazakaslaurence: I believe we can but it's still not perfect and there's more to discuss as to how we'd to it, from my understanding16:19
laurencedamn, and we figured this out just after sending a mail16:20
*** traveltissues has quit IRC16:20
*** lachlan has quit IRC16:20
*** alexandrufazakas has quit IRC16:20
*** tpollard has quit IRC16:20
*** rdale has quit IRC16:20
*** tiagogomes has quit IRC16:20
laurenceannoying16:20
*** traveltissues has joined #buildstream16:21
*** rdale has joined #buildstream16:21
*** tiagogomes has joined #buildstream16:21
*** tpollard has joined #buildstream16:21
*** alexandrufazakas has joined #buildstream16:21
*** lachlan has joined #buildstream16:21
laurencealexandrufazakas_, I think we better had, yes - I will do it16:21
alexandrufazakaslaurence: Alright. Thanks for that16:22
*** alexandrufazakas has quit IRC16:22
*** tristan has joined #buildstream16:30
*** traveltissues has quit IRC16:55
*** rdale has quit IRC16:58
*** rdale has joined #buildstream17:00
*** jonathanmaw has quit IRC17:01
*** bochecha has joined #buildstream17:20
*** lachlan has quit IRC17:29
*** lachlan has joined #buildstream17:35
*** lachlan has quit IRC17:36
*** traveltissues has joined #buildstream17:50
*** traveltissues has quit IRC18:10
*** phoenix has quit IRC18:16
*** toscalix has quit IRC19:01
*** traveltissues has joined #buildstream19:28
*** traveltissues has quit IRC19:42
*** bochecha has quit IRC22:52

Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!