| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: [zuul/zuul] Fix wrong call to unlock requests in merger client https://review.opendev.org/c/zuul/zuul/+/807490 | 01:09 | |
| -@gerrit:opendev.org- Ian Wienand proposed: [zuul/zuul-registry] filesystem: add file syncs https://review.opendev.org/c/zuul/zuul-registry/+/807663 | 02:51 | |
| -@gerrit:opendev.org- Ian Wienand proposed: [zuul/zuul-registry] filesystem: add file syncs https://review.opendev.org/c/zuul/zuul-registry/+/807663 | 04:12 | |
| -@gerrit:opendev.org- Simon Westphahl proposed: [zuul/zuul] Release general cleanup lock after acquire https://review.opendev.org/c/zuul/zuul/+/807670 | 06:04 | |
| -@gerrit:opendev.org- Simon Westphahl proposed: [zuul/zuul] Release correct lock after node request cleanup https://review.opendev.org/c/zuul/zuul/+/807673 | 06:16 | |
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: [zuul/zuul] Fix parsing of zone from path in executor API https://review.opendev.org/c/zuul/zuul/+/807539 | 06:43 | |
| -@gerrit:opendev.org- Guillaume Chauvel proposed: | 08:13 | |
| - [zuul/zuul] Update tests/base.py to use proper git data https://review.opendev.org/c/zuul/zuul/+/742746 | ||
| - [zuul/zuul] Fix gerrit merge commit change with zuul configuration https://review.opendev.org/c/zuul/zuul/+/762886 | ||
| - [zuul/zuul] Fix zuul-client enqueue-ref when oldrev/newrev aren't provided https://review.opendev.org/c/zuul/zuul/+/765767 | ||
| - [zuul/zuul] Fix exception when using enqueue-ref https://review.opendev.org/c/zuul/zuul/+/765809 | ||
| - [zuul/zuul] Improve merger getFilesChanges, Fix edge cases https://review.opendev.org/c/zuul/zuul/+/762887 | ||
| - [zuul/zuul] Tenant reconfiguration: Add tests https://review.opendev.org/c/zuul/zuul/+/742747 | ||
| - [zuul/zuul] Tenant reconfiguration: Reorg scheduler event process loop https://review.opendev.org/c/zuul/zuul/+/742748 | ||
| - [zuul/zuul] Tenant reconfiguration: Scheduler ref-updated create/delete https://review.opendev.org/c/zuul/zuul/+/739198 | ||
| - [zuul/zuul] Tenant reconfiguration: Allow ref-updated newrev+oldrev reconfiguration https://review.opendev.org/c/zuul/zuul/+/742749 | ||
| - [zuul/zuul] Tenant reconfiguration: Get ref-updated modified files https://review.opendev.org/c/zuul/zuul/+/739078 | ||
| - [zuul/zuul] WIP fix exception https://review.opendev.org/c/zuul/zuul/+/807683 | ||
| - [zuul/zuul] WIP fix exception https://review.opendev.org/c/zuul/zuul/+/807684 | ||
| -@gerrit:opendev.org- Jiri Podivin proposed: [zuul/zuul-jobs] DNM https://review.opendev.org/c/zuul/zuul-jobs/+/807031 | 09:16 | |
| -@gerrit:opendev.org- Guillaume Chauvel proposed: | 09:27 | |
| - [zuul/zuul] Update tests/base.py to use proper git data https://review.opendev.org/c/zuul/zuul/+/742746 | ||
| - [zuul/zuul] Fix gerrit merge commit change with zuul configuration https://review.opendev.org/c/zuul/zuul/+/762886 | ||
| - [zuul/zuul] Fix zuul-client enqueue-ref when oldrev/newrev aren't provided https://review.opendev.org/c/zuul/zuul/+/765767 | ||
| - [zuul/zuul] Fix exception when using enqueue-ref https://review.opendev.org/c/zuul/zuul/+/765809 | ||
| - [zuul/zuul] WIP fix exception https://review.opendev.org/c/zuul/zuul/+/807683 | ||
| - [zuul/zuul] WIP fix exception https://review.opendev.org/c/zuul/zuul/+/807684 | ||
| - [zuul/zuul] Improve merger getFilesChanges, Fix edge cases https://review.opendev.org/c/zuul/zuul/+/762887 | ||
| - [zuul/zuul] Tenant reconfiguration: Add tests https://review.opendev.org/c/zuul/zuul/+/742747 | ||
| - [zuul/zuul] Tenant reconfiguration: Reorg scheduler event process loop https://review.opendev.org/c/zuul/zuul/+/742748 | ||
| - [zuul/zuul] Tenant reconfiguration: Scheduler ref-updated create/delete https://review.opendev.org/c/zuul/zuul/+/739198 | ||
| - [zuul/zuul] Tenant reconfiguration: Allow ref-updated newrev+oldrev reconfiguration https://review.opendev.org/c/zuul/zuul/+/742749 | ||
| - [zuul/zuul] Tenant reconfiguration: Get ref-updated modified files https://review.opendev.org/c/zuul/zuul/+/739078 | ||
| -@gerrit:opendev.org- Benjamin Schanzel proposed on behalf of Tobias Henkel: [zuul/zuul] Optionally support mitogen for job execution https://review.opendev.org/c/zuul/zuul/+/657024 | 10:26 | |
| -@gerrit:opendev.org- Simon Westphahl proposed: | 10:39 | |
| - [zuul/zuul] Cache Gerrit refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/805837 | ||
| - [zuul/zuul] Cache Github refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/805838 | ||
| - [zuul/zuul] Cache Pagure refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/806556 | ||
| - [zuul/zuul] Cache Gitlab refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/806557 | ||
| - [zuul/zuul] Cache Git refs (driver) in Zookeeper https://review.opendev.org/c/zuul/zuul/+/806755 | ||
| - [zuul/zuul] Periodically maintain connection caches https://review.opendev.org/c/zuul/zuul/+/806756 | ||
| - [zuul/zuul] Clean up dangling cache data nodes more often https://review.opendev.org/c/zuul/zuul/+/807102 | ||
| -@gerrit:opendev.org- Jiri Podivin proposed: [zuul/zuul-jobs] DNM https://review.opendev.org/c/zuul/zuul-jobs/+/807031 | 10:54 | |
| -@gerrit:opendev.org- Jiri Podivin proposed: [zuul/zuul-jobs] DNM https://review.opendev.org/c/zuul/zuul-jobs/+/807031 | 11:07 | |
| -@gerrit:opendev.org- Benjamin Schanzel proposed on behalf of Tobias Henkel: [zuul/zuul] Optionally support mitogen for job execution https://review.opendev.org/c/zuul/zuul/+/657024 | 11:08 | |
| -@gerrit:opendev.org- Felix Edel proposed: [zuul/zuul] Don't use the AnsibleJob in the nodepool client https://review.opendev.org/c/zuul/zuul/+/807711 | 12:20 | |
| -@gerrit:opendev.org- Felix Edel proposed: [zuul/zuul] Don't use the AnsibleJob in the nodepool client https://review.opendev.org/c/zuul/zuul/+/807711 | 12:46 | |
| -@gerrit:opendev.org- Benjamin Schanzel proposed on behalf of Tobias Henkel: [zuul/zuul] Optionally support mitogen for job execution https://review.opendev.org/c/zuul/zuul/+/657024 | 13:13 | |
| -@gerrit:opendev.org- Simon Westphahl proposed: | 13:42 | |
| - [zuul/zuul] Add source interface for setting change attributes https://review.opendev.org/c/zuul/zuul/+/805836 | ||
| - [zuul/zuul] Reference change dependencies by key https://review.opendev.org/c/zuul/zuul/+/805844 | ||
| - [zuul/zuul] Implement ABC for caching changes in Zookeeper https://review.opendev.org/c/zuul/zuul/+/805835 | ||
| - [zuul/zuul] Cache Gerrit refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/805837 | ||
| - [zuul/zuul] Cache Github refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/805838 | ||
| - [zuul/zuul] Cache Pagure refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/806556 | ||
| - [zuul/zuul] Cache Gitlab refs in Zookeeper https://review.opendev.org/c/zuul/zuul/+/806557 | ||
| - [zuul/zuul] Cache Git refs (driver) in Zookeeper https://review.opendev.org/c/zuul/zuul/+/806755 | ||
| - [zuul/zuul] Periodically maintain connection caches https://review.opendev.org/c/zuul/zuul/+/806756 | ||
| - [zuul/zuul] Clean up dangling cache data nodes more often https://review.opendev.org/c/zuul/zuul/+/807102 | ||
| @clarkb:matrix.org | corvus: for when your day starts https://review.opendev.org/c/zuul/zuul-registry/+/807663 seems to be an important fix that ianw discovered for the zuul registry | 13:53 |
|---|---|---|
| -@gerrit:opendev.org- Benjamin Schanzel proposed on behalf of Tobias Henkel: [zuul/zuul] Optionally support mitogen for job execution https://review.opendev.org/c/zuul/zuul/+/657024 | 14:00 | |
| -@gerrit:opendev.org- Zuul merged on behalf of Ian Wienand: [zuul/zuul-registry] filesystem: add file syncs https://review.opendev.org/c/zuul/zuul-registry/+/807663 | 14:37 | |
| -@gerrit:opendev.org- Jiri Podivin proposed: [zuul/zuul-jobs] DNM https://review.opendev.org/c/zuul/zuul-jobs/+/807031 | 14:42 | |
| @jim:acmegating.com | Clark: ^ merged | 14:45 |
| @clarkb:matrix.org | thanks. We deploy that hourly which means it should get deployed in the next 45 minutes or so based on current timestamps | 14:47 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] Try harder to unlock failed build requests https://review.opendev.org/c/zuul/zuul/+/807221 | 14:54 | |
| @ssbarnea:matrix.org | I have an important tox base job change proposal at https://review.opendev.org/c/zuul/zuul-jobs/+/807702 pleaset take a look. Should I send an email to the mailing list about that? | 14:57 |
| -@gerrit:opendev.org- Jiri Podivin proposed: [zuul/zuul-jobs] DNM https://review.opendev.org/c/zuul/zuul-jobs/+/807031 | 14:58 | |
| -@gerrit:opendev.org- Jiri Podivin proposed: [zuul/zuul-jobs] DNM https://review.opendev.org/c/zuul/zuul-jobs/+/807031 | 14:59 | |
| @clarkb:matrix.org | > <@ssbarnea:matrix.org> I have an important tox base job change proposal at https://review.opendev.org/c/zuul/zuul-jobs/+/807702 pleaset take a look. Should I send an email to the mailing list about that? | 15:00 |
| This is likely to cause jobs for some projects to begin failing. Communication is probably worthwhile as a result. That said, it is already a behavior users can opt into via their config. Do we think they are unaware of that option but would otherwise want it set? | ||
| @ssbarnea:matrix.org | in fact the real issue is users mistakenly setting it to true and forgetting about and that is what this protects against | 15:01 |
| @ssbarnea:matrix.org | even tox maintainer recommends that for CI, for the reasons i mentioned in CR | 15:02 |
| @ssbarnea:matrix.org | as "tox" base job is used by many, that would be the best place to set good defaults | 15:03 |
| @jim:acmegating.com | this is applicable if someone is running just the "tox" job, not, for example, tox-py38, right? | 15:03 |
| @clarkb:matrix.org | corvus: yes it would only apply if not specifying a specifc environment aiui | 15:03 |
| @clarkb:matrix.org | and the tox-* jobs all supply a specific environment iirc | 15:04 |
| @jim:acmegating.com | so it handles the case where someone has 3.8 configured in their tox.ini but the host only has 3.9 installed, and the job just runs "tox", and without this option, it exits 0? | 15:05 |
| @clarkb:matrix.org | yes | 15:05 |
| @clarkb:matrix.org | or if you list py37, 38, 39 then only 3.9 runs and its result determines the exit code | 15:05 |
| @jim:acmegating.com | ssbarnea: i'd say get some +2s on the change, then pick a date ~2 weeks out, send an email to zuul-announce, then we'll merge it. | 15:07 |
| @ssbarnea:matrix.org | take a look at https://github.com/ansible/ansible-navigator/blob/main/tox.ini#L11 and https://dashboard.zuul.ansible.com/t/ansible/build/b77610e1e42e4b14839817159d0b962c/console -- which run py38 job... using python3.6 interpreter. | 15:07 |
| @ssbarnea:matrix.org | the side-effects of using this option are close to disastrous imho: tox gives you impression that is using a version of python which is not the real one. | 15:08 |
| @ssbarnea:matrix.org | for local testing it may be ok-ish, but for CI is a no-no. | 15:08 |
| @jim:acmegating.com | yes, i think this is a good change. i do also feel like using plain "tox" should be strongly discouraged in favor of using jobs with explicit environments. | 15:08 |
| @clarkb:matrix.org | hrm it seems ssbarnea's example did specify specific environments but a list of them. | 15:09 |
| @jim:acmegating.com | so the -py38 job ran... yeah now i'm confused | 15:10 |
| @clarkb:matrix.org | I feel like that is also a tox bug. If you've asked for a specific environment it shouldn't run in a different environment | 15:10 |
| @clarkb:matrix.org | Is the problem perhaps the bash -c escape hatch? | 15:10 |
| @clarkb:matrix.org | I wonder if that subverts the virtualenv management | 15:11 |
| @clarkb:matrix.org | anyway, I think the change is fine to be more strict, but also think this is a tox bug if you can ask for a specific environment and not get it | 15:11 |
| @clarkb:matrix.org | the skip missing interpreters option is there for running `tox` and if py37 isn't installed but py38 is you get the one that is present run and can push that code up | 15:11 |
| @clarkb:matrix.org | it isn't there to make py38 run as py36 as far as I know | 15:12 |
| @jim:acmegating.com | Clark: yeah i was thinking the same thing about the bash escapes | 15:12 |
| @ssbarnea:matrix.org | i suspect that it was a bug, later fixed. not sure what kind of tox version was used in this run | 15:12 |
| @ssbarnea:matrix.org | but the project run with this CI setup for many months :D | 15:12 |
| @jim:acmegating.com | ssbarnea: are you sure that it fixes that issue? like, if you make a PR against that repo that switches the value of skipmissinginterpreters, does it change what version gets run? | 15:13 |
| @ssbarnea:matrix.org | i can easily make a change to proof it, the goal is to see tox failing instead of using the wrong interpreter | 15:13 |
| @ssbarnea:matrix.org | for CI you always want to explicit in behavior | 15:13 |
| @clarkb:matrix.org | > <@ssbarnea:matrix.org> for CI you always want to explicit in behavior | 15:14 |
| right and the job does do that by listing explicit environments | ||
| @jim:acmegating.com | ssbarnea: yeah, i agree. i think Clark and i are suggesting that this job is being explicit, and that the proposed zuul-jobs change (whill still a good change on its own merits) may not actually fix the issue in ansible-navigator | 15:14 |
| @ssbarnea:matrix.org | corvus: to be honest my goal was not to fix navigator, i know how to do that. my goal was to save others from the same mistake | 15:15 |
| @jim:acmegating.com | ssbarnea: sure, i'm saying your change will save others from a different mistake. | 15:15 |
| @ssbarnea:matrix.org | i do remember few months back with an openstack or opendev project that had a similar kind of issue. | 15:15 |
| @ssbarnea:matrix.org | proably we could use codesearch to find potentially affected projects | 15:15 |
| @jim:acmegating.com | if Clark and i are correct, it would have to be a project that's just running a plain "tox" job, which is not recommended | 15:16 |
| @ssbarnea:matrix.org | i also remember another common mistake with tox files, using `skipdist` instead of `skipsdist` and believing that it works fine. | 15:17 |
| -@gerrit:opendev.org- Zuul merged on behalf of Simon Westphahl: | 15:34 | |
| - [zuul/zuul] Release general cleanup lock after acquire https://review.opendev.org/c/zuul/zuul/+/807670 | ||
| - [zuul/zuul] Release correct lock after node request cleanup https://review.opendev.org/c/zuul/zuul/+/807673 | ||
| @ssbarnea:matrix.org | Clark: should I send an email to zuul-discuss informing users about upcoming change and saying we will wait two weeks before merging this change? | 16:14 |
| @jim:acmegating.com | ssbarnea: no, zuul-announce. | 16:17 |
| @ssbarnea:matrix.org | can i post to this one? should i, or should I less a core do it? | 16:19 |
| @jim:acmegating.com | ssbarnea: sure you can send, i will moderate it through | 16:20 |
| @ssbarnea:matrix.org | corvus: please help me write it https://hackmd.io/fWrrBD_pS4yFFitzIaCZgg | 16:30 |
| @jim:acmegating.com | ssbarnea: you might mention that it would only affect plain "tox" jobs, and not tox-py3x jobs | 16:31 |
| @ssbarnea:matrix.org | i am not yet sure it does not affect them too. my impression is that even with "tox -e py38" tox may endup using py36 (at least older versions of tox) | 16:32 |
| @jim:acmegating.com | otherwise lgtm | 16:32 |
| @ssbarnea:matrix.org | which, if confirmed, would only be another reason for doing it | 16:33 |
| @jim:acmegating.com | ssbarnea: Clark and i think the opposite; i think it's worth clarifying that before sending the announcement/merging the change as it has a large effect on the impacted audience. | 16:33 |
| @ssbarnea:matrix.org | now i know why only base tox jobs are affected, https://opendev.org/zuul/zuul-jobs/src/branch/master/zuul.d/python-jobs.yaml#L114 | 16:37 |
| @ssbarnea:matrix.org | so we kinda workaround tox bug and ensure we call tox using the desired python version of child ones. | 16:38 |
| @ssbarnea:matrix.org | something an average user may not know, i will mention that. | 16:38 |
| @jim:acmegating.com | yeah, it's why i think the impact will be limited, and it's a good opportunity to remind people not to use a bare tox job. | 16:44 |
| @ssbarnea:matrix.org | i used bare one several times, for good reasons. molecule jobs inherit from them because version of python was not important for these. | 16:45 |
| @ssbarnea:matrix.org | but in most cases is not a good idea | 16:45 |
| @ssbarnea:matrix.org | i made changes to the text, please make other fixes and let me know when it looks ok to send | 16:46 |
| @clarkb:matrix.org | Note I still think it is a bug in tox if specifying py38 gives you py36 even with the flag set to true | 16:50 |
| @clarkb:matrix.org | the flag should only operate when you haevn't explicitly requested a specific version | 16:50 |
| @jim:acmegating.com | Clark: i don't think we have any evidence that happens | 16:51 |
| @jim:acmegating.com | ssbarnea: what happens if you specify "tox -e molecule"? what python version does it run with, and what effect does that flag have on it? | 16:51 |
| @clarkb:matrix.org | ya if that were happening. The example given earlier is confusing due to the bash escapres | 16:52 |
| @clarkb:matrix.org | * ya if that were happening. The example given earlier is confusing due to the bash escapes | 16:52 |
| @ssbarnea:matrix.org | it will use tox default python version and that is fine, not everyone using tox needs specific python, many use tox only for containing virtualenvs. | 16:52 |
| @jim:acmegating.com | so then i think you could clarify that a bit more and instead of saying "users of jobs with 'parent: tox'" say "users of the plain 'tox' job or jobs that inherit from it without specifying a tox environment using the tox_envlist variable". | 16:54 |
| @ssbarnea:matrix.org | there are only 20 projects on opendev using that tricky setting in tox.ini, and most of them are outdated anyway. | 16:54 |
| @ssbarnea:matrix.org | i think we will be fine, but using warning and waiting is a very good idea, just to play nice. nobody likes surprises | 16:56 |
| @fungicide:matrix.org | I posted to openstack-discuss about it over a year ago: http://lists.openstack.org/pipermail/openstack-discuss/2020-May/014810.html | 18:36 |
| @fungicide:matrix.org | hopefully that led to a lot of occurrences getting ripped back out | 18:36 |
| @fungicide:matrix.org | you might consider it a bug that `tox -e py38` will no-op successfully when run on a system with no available python3.8 interpreter in the path if skip_missing_interpreters it set, but it seems to be more of an unfortunate side-effect of an intentional design decision | 18:39 |
| @fungicide:matrix.org | a quick test with latest tox indicates it still behaves in precisely this manner, btw | 18:40 |
| @fungicide:matrix.org | > <@clarkb:matrix.org> Note I still think it is a bug in tox if specifying py38 gives you py36 even with the flag set to true | 18:44 |
| in reference to that i mean | ||
| @fungicide:matrix.org | it doesn't give you a "different" python, it just happily reports a successful run of no testenv | 18:45 |
| @fungicide:matrix.org | in one way that's it not doing what you asked (running testenv:py38), but in another way it's doing precisely what it was asked to do (skips testenv:py38 because there's no python3.8 to use with it) | 18:46 |
| @fungicide:matrix.org | it does, however, seem to violate the principle of least surprise | 18:48 |
| @clarkb:matrix.org | hrm I definitely feel like that is a bug. The skip flag exists so that it does the most correct thing when run without environments specified | 18:53 |
| @clarkb:matrix.org | but if you specify a specific environment not running that environment is a problem | 18:53 |
| @clarkb:matrix.org | and ya it wouldn't surprise me if that was an unexpected side effect of adding the skip flag, but seems like that should be fixable unless upstream has already rejected doing so (and upstream has been difficutl in the past) | 18:54 |
| @fungicide:matrix.org | you can file an issue with tox asserting that opinion, but before doing so i'd check for "closed" issues about it | 18:54 |
| @fungicide:matrix.org | anyway, i think ssbarnea's proposed solution is a nice belt-and-braces way to disarm that particular foot-cannon | 18:56 |
| @clarkb:matrix.org | Yup and it would help for empty invocations as well | 18:58 |
| @pabelanger:matrix.org | can a job use both https://zuul-ci.org/docs/zuul/reference/job_def.html#attr-job.files and https://zuul-ci.org/docs/zuul/reference/job_def.html#attr-job.irrelevant-files ? | 19:13 |
| @pabelanger:matrix.org | I don't see anything that says it cannot | 19:14 |
| @pabelanger:matrix.org | but I thought it was only one or the other | 19:14 |
| @fungicide:matrix.org | indeed, the docs don't indicate what the expected behavior is if you specify both. i expect that one takes precedence over the other, but would need to look in the job selection code to figure out what would happen in specific cases (like if you specify doc/source/.* is relevant but doc/source/conf.py is irrelevant, then does that actually run in cases where any file under doc/source is changed unless the change is only to conf.py?). | 19:35 |
| @pabelanger:matrix.org | yah, I have memories of tripleo doing something like this but cannot find logs around it. | 19:36 |
| @pabelanger:matrix.org | but isn't a big issue right now, mostly curious if people remembered | 19:36 |
| -@gerrit:opendev.org- lotorev vitaly proposed: [zuul/zuul] docs: Document regexp RE2 restricted syntax https://review.opendev.org/c/zuul/zuul/+/695991 | 19:56 | |
| @clarkb:matrix.org | pabelanger: https://opendev.org/zuul/zuul/src/branch/master/zuul/model.py#L1872-L1881 it seems to check files first then irrelevant files | 20:32 |
| @pabelanger:matrix.org | so, we now have 2 github apps in our zuul, which got me thinking about limiting which pipelines are loaded for each github app. Is that something we could configure in the tenant config file? | 20:33 |
| @pabelanger:matrix.org | I don't think so, we can only block the pipeline stanza from loading IIRC | 20:34 |
| @pabelanger:matrix.org | Clark: thanks for looking | 20:34 |
| @clarkb:matrix.org | The pipeline controls which triggers affect it | 20:34 |
| @clarkb:matrix.org | you should be able to control it on a per github basis that way? | 20:34 |
| @pabelanger:matrix.org | Clark: ah yes that is right. Not sure what I was thinking | 20:37 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul-operator] Update doc theme settings to match Zuul https://review.opendev.org/c/zuul/zuul-operator/+/807798 | 20:48 | |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: | 21:00 | |
| - [zuul/zuul-website] Correct client link in FAQ documentation menu https://review.opendev.org/c/zuul/zuul-website/+/807801 | ||
| - [zuul/zuul-website] Add zuul-operator to doc index https://review.opendev.org/c/zuul/zuul-website/+/807802 | ||
| @clarkb:matrix.org | corvus: I'm just about through giving https://review.opendev.org/c/zuul/zuul/+/807221/ an up to date proper review. The only thing that jumps out to me is that SessionAwareLock seems fairly complicated when we seem to only rely on defaults in this change. Do we expect to use it with non ephemeral objects? | 21:58 |
| @clarkb:matrix.org | I just wonder if we need the new lock type to handle so many scenarios if we only use it for the one | 21:58 |
| @clarkb:matrix.org | hrm it just occured to me that we may be maintainin api compat with kazoo /me checks | 22:08 |
| @clarkb:matrix.org | Yup that is exactly what we are doing | 22:09 |
| @clarkb:matrix.org | the change lgtm | 22:09 |
| @jim:acmegating.com | yep that's the idea | 22:10 |
| @jim:acmegating.com | i'm still sort of pondering whether the "add a callback for each lock" approach is better than the "add a callback for each component". you can see the second approach in https://opendev.org/zuul/zuul/src/branch/master/zuul/driver/gerrit/gerritconnection.py#L347-L348 -- the second is fewer callbacks (and we've already had a bug where we leaked callbacks) but in the case of the executor, we'd probably have to do some tricky stuff to record which locks the disconnection affected. that may be more complicated than this approach. i think if we're confident it's not going to leak callbacks, then this one might be better (and we should think about switching the other instances). | 22:16 |
| @jim:acmegating.com | Clark, tobiash, swest, felixedel: ^ | 22:17 |
| @clarkb:matrix.org | We would have to maintain a table of locks we hold in each scheduler? | 22:20 |
| @clarkb:matrix.org | That seems at least as complicated as setting a callback for each lock? | 22:20 |
| @jim:acmegating.com | Clark: yeah something like that, and that's sort of my thinking atm. | 22:23 |
| @clarkb:matrix.org | corvus: left a question on https://review.opendev.org/c/zuul/zuul/+/806639 if you have time to take a look | 22:52 |
| -@gerrit:opendev.org- Ian Wienand proposed on behalf of lotorev vitaly: [zuul/zuul] docs: Document regexp RE2 restricted syntax https://review.opendev.org/c/zuul/zuul/+/695991 | 23:01 | |
| @clarkb:matrix.org | corvus: also left a question on the child ofthat change | 23:02 |
| @jim:acmegating.com | Clark: replied on 639 | 23:09 |
| @jim:acmegating.com | Clark: i'm having a little trouble parsing your q on 653 -- can you rephrase that? | 23:12 |
| @jim:acmegating.com | Clark: like, you say "always need to set" but it's within a conditional so we don't always set it... are you suggesting we should set it more often than we do, or are you asking why do we set it at all, or... something else? | 23:13 |
| @jim:acmegating.com | s/conditional/exception handler/ | 23:13 |
| @clarkb:matrix.org | corvus: the if block is fine, it is the catch in the exception handler that I think may be an issue | 23:14 |
| @clarkb:matrix.org | Because in the lock win method we wait() on that event before doing another pass. I'm worried that we might end up doing a single pass through the win method then wait() forever | 23:14 |
| @jim:acmegating.com | Clark: that's actually the intent | 23:15 |
| @jim:acmegating.com | (if there are no errors, that's exactly what should happen) | 23:15 |
| @jim:acmegating.com | only if there is an error would we need to run the election again | 23:15 |
| @clarkb:matrix.org | but then we'll only process one set of requests? | 23:15 |
| @jim:acmegating.com | Clark: this may help -- it's really a thread-shifting situation. the "real" work is not in the election thread. the election thread only exists to obtain a lock in ZK that allows the other main thread to run. | 23:16 |
| @jim:acmegating.com | Clark: the election win does do a little bit of work, but it's only to handle the race case (i mentioned in my reply to you on the previous change which you may not have read yet) | 23:16 |
| @clarkb:matrix.org | self._sendNodesProvisionedEvent(request) is not doing all the work of enabling those events to be processed then? | 23:17 |
| @jim:acmegating.com | Clark: nope, that's patching up the race condition that happens when we change leaders. it emits a bunch of (probably duplicate) events for all currently completed requests, just to make sure we didn't miss any during the transition. | 23:18 |
| @jim:acmegating.com | the normal ongoing synchronous work is all on _handloNodeRequestEvent | 23:18 |
| @clarkb:matrix.org | I see and that is why we call that same method where we catch the exception and set the watcher event | 23:18 |
| @jim:acmegating.com | (but it only does that work if the election is won) | 23:18 |
| @jim:acmegating.com | yep, just to make sure we emit the events the same way. | 23:18 |
| @jim:acmegating.com | i'll write that up in the reply with a bit more detail | 23:19 |
| @clarkb:matrix.org | and reading the comment on 639 it seems that it is expected that we are idempotent as we can emit duplicate events (though we try not to) | 23:21 |
| @jim:acmegating.com | yep. specifically for that case. | 23:21 |
| @clarkb:matrix.org | https://review.opendev.org/c/zuul/zuul/+/806814/ I also left a comment on that one, hopefully much simpler to answer :) | 23:22 |
| @jim:acmegating.com | k, replied on 653 | 23:25 |
| @jim:acmegating.com | Clark: replied on 814 | 23:26 |
| @clarkb:matrix.org | ah ok so continues to be used internally | 23:28 |
| @jim:acmegating.com | yep, for like 5 cpu instructions (since that call is right under it) we could probably turn that into a function parameter and finish dropping it | 23:28 |
| @clarkb:matrix.org | assuming others are happy with 639 a good chunk of that stack should be mergeable now. I'm up to the one tobiash has a -1 on. But looks like others have responded so that might flip to a +2 and I should review it | 23:30 |
| @jim:acmegating.com | yeah, i think felix and i addressed tobiash's concern in comments and it's still reviewable (i expect tobiash probably just hasn't had a chance to revisit yet) | 23:34 |
| @clarkb:matrix.org | corvus: left a question on https://review.opendev.org/c/zuul/zuul/+/806821 | 23:46 |
| @jim:acmegating.com | Clark: repld | 23:50 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!