| -@gerrit:opendev.org- Simon Westphahl proposed: [zuul/zuul] 893598: Ignore missing nodes when in lost-request iterator https://review.opendev.org/c/zuul/zuul/+/893598 | 07:17 | |
| -@gerrit:opendev.org- Simon Westphahl proposed: [zuul/zuul] 893598: Ignore missing nodes in lost-request iterator https://review.opendev.org/c/zuul/zuul/+/893598 | 07:18 | |
| @yoctozepto:matrix.org | I reminded myself that corvus is not getting gerrit notifications; thus, here is a long overdue approval request: https://review.opendev.org/c/zuul/zuul-jobs/+/885987 | 11:48 |
|---|---|---|
| -@gerrit:opendev.org- Zuul merged on behalf of Radosław Piliszek https://matrix.to/#/@yoctozepto:matrix.org: [zuul/zuul-jobs] 885987: Drop Helm v2 support to fix v3 issue https://review.opendev.org/c/zuul/zuul-jobs/+/885987 | 14:07 | |
| -@gerrit:opendev.org- Clark Boylan proposed: | 17:30 | |
| - [zuul/zuul] 893682: Allow new configs to be used when warnings are present https://review.opendev.org/c/zuul/zuul/+/893682 | ||
| - [zuul/zuul] 893683: Fix validate_tenants list in config warnings test https://review.opendev.org/c/zuul/zuul/+/893683 | ||
| -@gerrit:opendev.org- Clark Boylan proposed: | 18:20 | |
| - [zuul/zuul] 893682: Allow new configs to be used when warnings are present https://review.opendev.org/c/zuul/zuul/+/893682 | ||
| - [zuul/zuul] 893683: Fix validate_tenants list in config warnings test https://review.opendev.org/c/zuul/zuul/+/893683 | ||
| @jim:acmegating.com | Clark: i'm concerned that your first patch may not fix the problem | 19:40 |
| @jim:acmegating.com | Clark: specifically, i don't think that test fails without your code change | 19:40 |
| @clarkb:matrix.org | Hrm | 19:41 |
| @jim:acmegating.com | and... i don't think that fix should be necessary | 19:41 |
| @clarkb:matrix.org | The error I pasted in #opendev originates from that line if I read it correctly and it fails if warnings are present | 19:41 |
| @jim:acmegating.com | because it should only be adding "relevant" warnings/errors to the config item | 19:41 |
| @jim:acmegating.com | so when we retrieve them, that list should contain only new warnings or errors introduced by the change | 19:41 |
| @clarkb:matrix.org | Is it possible the warnings on the existing grenade job parent are relevant to an update to the grenade child update? | 19:42 |
| @jim:acmegating.com | (now, whether we want to allow new warnings is a good question, and i'm open to changing that behavior, but at least, the current behavior of not allowing new warnings should not have caused a failure due to old warnings) | 19:42 |
| @jim:acmegating.com | Clark: i don't know the answer to the grenade question because i haven't dug into the specific case... | 19:43 |
| @clarkb:matrix.org | As mentioned in #opendev it is hard to know exactly what broke because zuul doesn't log the errors and the reporter crashed | 19:43 |
| @clarkb:matrix.org | I think it would be good to have zuul log the errors. At least when the reporter crashes | 19:43 |
| @jim:acmegating.com | i'm happy to dig in, but i'm wondering, should we pull the fix from gate? or should we allow it through as an alternative to a temporary revert and continue working on more fixes, even if it means reverting this one back once we understand more? | 19:44 |
| @clarkb:matrix.org | But working back from the logged line it starts with the get errors call. Which could be some other error. | 19:44 |
| @clarkb:matrix.org | I'm fine kicking it out of the gate if you want to dig more | 19:44 |
| @clarkb:matrix.org | I'm actually at the kids school playground :) happy for whatever action to be taken | 19:45 |
| @jim:acmegating.com | (i do strongly suspect that merging it will alleviate the problem because it will should all warnings, but it hasn't been tested for that purpose, so... <shrug>) | 19:45 |
| @jim:acmegating.com | okay, i'm going to remove my +w, at least for a bit, until we understand more so we don't end up flapping too much | 19:46 |
| @jim:acmegating.com | Clark: okay, deducing a few things from the limited logs: i think that patch had relevant configuration warnings but not errors; but there is a check in the reporter which does not match the check in the manager, and that's what caused the exception. your fix is good in that it updates the manager to match the reporter check, so they will have the same behavior now. the original code was not good in that the behavior of introducing new config warnings was not well specified. | 19:56 |
| put a different way, if we want to block new relevant configuration warnings, we should update the reporters to match the manager. if we want to allow new relevant configuration warnings we should update the manager to match the reporters. your patch does that. | ||
| i think the early feedback from the user community would be: allow new configuration warnings. probably because it's actually really hard to touch any zuul config without it considering a warning to be relevant. it makes more sense to block those when they are errors, but i think what we're seeing with the cited changes from opendev is that just changing a big blob of job definitions can cause zuul to consider "old" warnings to be relevant. | ||
| tldr: previous behavior was not well defined; your patch specifies a clear behavior and it's good. we should merge your patch, and maybe add some more tests to make sure the exact scenario we just saw is covered. | ||
| @jim:acmegating.com | Clark: i'm going to re-approve that and work on a followup test. | 19:56 |
| @jim:acmegating.com | https://opendev.org/zuul/zuul/src/branch/master/zuul/reporter/__init__.py#L213 | 19:57 |
| @jim:acmegating.com | that's the line in the reporter i was referring to | 19:57 |
| @clarkb:matrix.org | Sounds good | 19:58 |
| @clarkb:matrix.org | Thank you for looking into it | 19:58 |
| @fungicide:matrix.org | longer term, it may make sense as a tenant-level configuration toggle? basically, let tenant managers decide if they want to block users from introducing new configuration which adds deprecated syntax | 20:22 |
| -@gerrit:opendev.org- Zuul merged on behalf of Clark Boylan: [zuul/zuul] 893682: Allow new configs to be used when warnings are present https://review.opendev.org/c/zuul/zuul/+/893682 | 20:54 | |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 893690: Allow new warnings when errors exist https://review.opendev.org/c/zuul/zuul/+/893690 | 21:06 | |
| @jim:acmegating.com | fungi: Clark i think the change from Clark covers some, but not all cases, and i think it's not quite sufficient for the cases we saw in opendev. can you take a look at 893690? | 21:07 |
| @jim:acmegating.com | i haven't run tests on that locally yet; will do now. | 21:07 |
| @jim:acmegating.com | then i'll see what's blocking 893693 | 21:08 |
| @jim:acmegating.com | 893690 passed all tests locally | 21:17 |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed on behalf of Clark Boylan: [zuul/zuul] 893683: Add comment to TestValidateWarinngs https://review.opendev.org/c/zuul/zuul/+/893683 | 21:21 | |
| @jim:acmegating.com | Clark: 893693 was correctly failing because the test does work and that change breaks the test. I took the liberty of modifying your change to add a comment explaining why to avoid confusion in the future. | 21:22 |
| @clarkb:matrix.org | Oh that config file was completely ignored. That makes sense. Thanks. I'll review those changes shortly | 21:25 |
| @clarkb:matrix.org | corvus: fungi and I +2'd the change but I didn't approve it because fungi had a question I'm like 95% sure of but figured you could approve if that is the case | 21:31 |
| @jim:acmegating.com | just replied to that | 21:31 |
| -@gerrit:opendev.org- Zuul merged on behalf of James E. Blair https://matrix.to/#/@jim:acmegating.com: [zuul/zuul] 893690: Allow new warnings when errors exist https://review.opendev.org/c/zuul/zuul/+/893690 | 22:13 | |
| -@gerrit:opendev.org- Zuul merged on behalf of Clark Boylan: [zuul/zuul] 893683: Add comment to TestValidateWarinngs https://review.opendev.org/c/zuul/zuul/+/893683 | 22:13 | |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/zuul] 893592: Fix bundle updates https://review.opendev.org/c/zuul/zuul/+/893592 | 23:20 | |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed on behalf of Felix Edel: [zuul/zuul] 891980: Ensure that bundle is set correctly on items outside of change queue https://review.opendev.org/c/zuul/zuul/+/891980 | 23:20 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!