| opendevreview | Ian Wienand proposed zuul/zuul master: zuul-stream: drop ARA bindep install https://review.opendev.org/c/zuul/zuul/+/793868 | 00:24 |
|---|---|---|
| opendevreview | Ian Wienand proposed zuul/zuul master: zuul-stream: drop ARA bindep install https://review.opendev.org/c/zuul/zuul/+/793868 | 00:27 |
| opendevreview | Merged zuul/zuul-jobs master: Bump golang version https://review.opendev.org/c/zuul/zuul-jobs/+/793586 | 00:48 |
| opendevreview | Merged zuul/zuul-jobs master: Use openstacksdk 0.45.0 for python2.7 https://review.opendev.org/c/zuul/zuul-jobs/+/784894 | 01:01 |
| opendevreview | Merged zuul/zuul-jobs master: Add ensure-skopeo role https://review.opendev.org/c/zuul/zuul-jobs/+/792900 | 01:01 |
| *** zbr has quit IRC | 02:22 | |
| *** zbr has joined #zuul | 02:34 | |
| *** zbr is now known as Guest413 | 02:49 | |
| *** zbr has joined #zuul | 02:49 | |
| *** zbr is now known as Guest414 | 02:53 | |
| *** zbr has joined #zuul | 02:53 | |
| *** Guest413 has quit IRC | 02:53 | |
| *** Guest414 has quit IRC | 02:57 | |
| *** zbr has quit IRC | 03:01 | |
| *** zbr has joined #zuul | 03:02 | |
| *** timburke has joined #zuul | 03:52 | |
| *** ricolin_ has joined #zuul | 04:32 | |
| *** ricolin has quit IRC | 04:36 | |
| *** ricolin_ is now known as ricolin | 04:36 | |
| *** y2kenny has quit IRC | 04:47 | |
| *** marios has joined #zuul | 05:14 | |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Show pipeline event queue lengths in Zuul web https://review.opendev.org/c/zuul/zuul/+/793781 | 05:28 |
| *** timburke has quit IRC | 05:47 | |
| *** msuszko has joined #zuul | 06:49 | |
| *** msuszko has quit IRC | 06:53 | |
| *** hashar has joined #zuul | 07:19 | |
| *** hashar has joined #zuul | 07:19 | |
| *** jpena|off is now known as jpena | 07:26 | |
| *** rpittau|afk is now known as rpittau | 07:44 | |
| *** tosky has joined #zuul | 07:48 | |
| *** tosky has quit IRC | 07:48 | |
| *** msuszko[m] has joined #zuul | 07:56 | |
| *** msuszko[m] is now known as msuszko | 07:56 | |
| *** tosky has joined #zuul | 08:08 | |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Show pipeline event queue lengths in Zuul web https://review.opendev.org/c/zuul/zuul/+/793781 | 08:28 |
| opendevreview | Albin Vass proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 08:32 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Show pipeline event queue lengths in Zuul web https://review.opendev.org/c/zuul/zuul/+/793781 | 08:33 |
| opendevreview | Matthieu Huin proposed zuul/zuul-client master: Add support for XDG-compliant config file https://review.opendev.org/c/zuul/zuul-client/+/793796 | 08:34 |
| swest | mhu: updated 793781 according to your comments | 08:34 |
| mhu | swest, nice, I'll check it when the CI completes | 08:36 |
| swest | mhu: k, thanks! | 08:36 |
| avass[m] | apparently gitlabs protected_branches api doesn't return all branches that are protected. instead it returns rules over which branches are protected | 08:36 |
| avass[m] | the documentation doesn't really mention it: https://docs.gitlab.com/ee/api/protected_branches.html#list-protected-branches :/ | 08:37 |
| mhu | swest, that makes me think, you may also want to move the global queue length info at the top of the status page, set it by the zuul version and reconfiguration time info | 08:46 |
| swest | mhu: not sure if that's a good change from our users' perspective as a lot of them are using that info as an indicator e.g. that there is a reconfig going on | 08:49 |
| mhu | ah, makes sense | 08:50 |
| mhu | swest, I think that's better now, thanks! | 09:05 |
| opendevreview | Matthieu Huin proposed zuul/zuul-client master: Add search filters when relevant https://review.opendev.org/c/zuul/zuul-client/+/788847 | 09:15 |
| *** sshnaidm|off is now known as sshnaidm | 09:57 | |
| *** sshnaidm has joined #zuul | 09:58 | |
| *** jangutter has quit IRC | 10:08 | |
| *** jpena is now known as jpena|lunch | 11:32 | |
| *** sshnaidm has quit IRC | 11:51 | |
| *** sshnaidm has joined #zuul | 11:59 | |
| opendevreview | Lida Liu proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 12:14 |
| *** jpena|lunch is now known as jpena | 12:29 | |
| opendevreview | Albin Vass proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 12:31 |
| opendevreview | Albin Vass proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 12:32 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Remove layout attribute from queue items https://review.opendev.org/c/zuul/zuul/+/792622 | 12:54 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Optimize layout re-calculation after re-enqueue https://review.opendev.org/c/zuul/zuul/+/792623 | 12:54 |
| mhu | hello zuul-maint, please find below a suggestion for the next patches to merge on zuul-client before releases 0.0.5 and 0.0.6 https://etherpad.opendev.org/p/zuulclient-mini-roadmap | 12:55 |
| mhu | It's been about 15 patches and 4 months since the last release + the builds subcommand is already merged, we should have buildsets, build-info and buildset-info in there as well | 12:56 |
| *** rpittau is now known as rpittau|afk | 12:57 | |
| opendevreview | Albin Vass proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 13:00 |
| *** dani has joined #zuul | 13:13 | |
| *** dani has quit IRC | 13:15 | |
| corvus | swest: can you take a look at my comments on https://review.opendev.org/792622 ? i think that might be ready with one more tiny revision | 13:28 |
| corvus | zuul-maint: https://review.opendev.org/793700 is a small change that would be great to merge asap i think; it should fix a test race | 13:29 |
| *** mordred has quit IRC | 13:35 | |
| *** mordred has joined #zuul | 13:35 | |
| opendevreview | Lida Liu proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 13:36 |
| *** eliadcohen_ is now known as eliadcohen | 13:53 | |
| opendevreview | Tristan Cacqueray proposed zuul/zuul master: gerrit: delay event queue attempts https://review.opendev.org/c/zuul/zuul/+/793977 | 14:26 |
| opendevreview | Tristan Cacqueray proposed zuul/zuul master: gerrit: delay event queue attempts https://review.opendev.org/c/zuul/zuul/+/793977 | 14:30 |
| tristanC | zuul-maint: when restarting zookeeper, the schedulers logs are flooded with `ERROR gerrit.GerritPoller: Exception on Gerrit poll with gerrit` because of `kazoo.exceptions.ConnectionClosedError`. Could you have a look at the proposed fix ^ | 14:33 |
| *** gouthamr_ has joined #zuul | 14:35 | |
| corvus | tristanC: lgtm; tobias.henkel ^ | 14:42 |
| tobiash[m] | Lgtm | 14:43 |
| *** opendevreview has quit IRC | 14:44 | |
| tobiash[m] | However I think technically the message of the first exception is no longer correct | 14:44 |
| tristanC | avass[m]: corvus: tobiash[m]: thank you for the prompt review! | 14:44 |
| corvus | tobias.henkel: oh yeah, it's probably worth looking at that as a followup | 14:45 |
| *** gouthamr has joined #zuul | 14:46 | |
| *** Open10K8S has joined #zuul | 14:56 | |
| *** timburke has joined #zuul | 14:58 | |
| *** Open10K8S has quit IRC | 15:00 | |
| *** Open10K8S has joined #zuul | 15:01 | |
| *** Open10K8S has quit IRC | 15:01 | |
| *** Open10K8S has joined #zuul | 15:03 | |
| *** opendevreview has joined #zuul | 15:04 | |
| opendevreview | Merged zuul/zuul master: Move "create|delete reference ..." messages to own sub logger https://review.opendev.org/c/zuul/zuul/+/791638 | 15:04 |
| *** Open10K8S has left #zuul | 15:06 | |
| opendevreview | Lida Liu proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 15:07 |
| corvus | mhu: you taught me a python in 793796; mordred, do you want to look at that again? or i could take your "great patch" comment as an implicit +2 on the new rev | 15:08 |
| corvus | mhu: (i did leave a +2 comment on that if you want to take a look) | 15:08 |
| *** opendevreview has quit IRC | 15:09 | |
| mordred | corvus: +A | 15:10 |
| mordred | also - I hadn't seen that python either! | 15:10 |
| mhu | corvus, ah right the pathlib.Path syntax ... I was very puzzled the first time I saw it (thanks to tristanC), but it is indeed neat! | 15:11 |
| corvus | looks like it's Path implementing the / operator | 15:11 |
| corvus | i was like "strings can do that?.... no... oh... it's a path object" :) | 15:11 |
| mordred | yah - I'm fairly certain that's what it is | 15:11 |
| mordred | pathlib came out in one of the 3.x releases - I keep forgetting about it | 15:11 |
| *** gouthamr_ has quit IRC | 15:12 | |
| corvus | what would you do to use it for a url path? | 15:13 |
| *** gouthamr has quit IRC | 15:13 | |
| *** gouthamr has joined #zuul | 15:14 | |
| corvus | PurePosixPath maybe? | 15:14 |
| corvus | also maybe 'urlpath' from pypi | 15:15 |
| corvus | zuul-maint: how does this look for a zuul release? commit bd1a669cc8e4eb143ecc96b67031574968d51d1e (HEAD -> master, tag: 4.4.0, gerrit/master) | 15:15 |
| corvus | that's what opendev is running; one commit back from current head | 15:16 |
| fungi | so it's still basically like os.path.join() under the hood i guess? | 15:16 |
| *** open10k8s has joined #zuul | 15:16 | |
| corvus | fungi: i assume; i haven't looked at the impl. | 15:16 |
| fungi | just in the constructor for the object looks like | 15:16 |
| fungi | definitely terse compared to the old way | 15:16 |
| avass[m] | fungi: yeah | 15:16 |
| tristanC | fungi: corvus: the pathlib also features .name, .parent and read_text() | 15:17 |
| fungi | cool, so like the os.path functions just more oo | 15:17 |
| *** gouthamr is now known as identify | 15:18 | |
| fungi | .read_text() looks awesome, that reduces a ton of common boilerplate | 15:18 |
| *** opendevreview has joined #zuul | 15:19 | |
| opendevreview | Merged zuul/zuul master: Fix test race with executor stats https://review.opendev.org/c/zuul/zuul/+/793700 | 15:19 |
| fungi | and was added in python 3.5, so usable basically anywhere these days | 15:19 |
| *** gouthamr has joined #zuul | 15:19 | |
| *** identify has quit IRC | 15:20 | |
| opendevreview | Merged zuul/zuul-client master: Clarify ~/.zuul.conf and --use-config option spelling https://review.opendev.org/c/zuul/zuul-client/+/789007 | 15:23 |
| *** open10k8s has quit IRC | 15:29 | |
| corvus | zuul-maint: re-ping regarding that release tag a few lines back (maybe got lost in the pathlib excitement) | 15:32 |
| opendevreview | Merged zuul/zuul-client master: Add support for XDG-compliant config file https://review.opendev.org/c/zuul/zuul-client/+/793796 | 15:34 |
| fungi | oh, yeah checking once my current meeting wraps up | 15:35 |
| fungi | looks like the only newer changes to merge since bd1a669 are 793700 (Fix test race with executor stats) and 791638 (Move "create|delete reference ..." messages to own sub logger) which probably aren't urgent | 15:38 |
| fungi | definitely some feature changes in the list between 4.3.0 and bd1a669, so 4.4.0 makes sense | 15:38 |
| tristanC | corvus: the tag lgtm, but we will have to cherry-pick 793977 | 15:39 |
| tristanC | (we as in, software factory) | 15:39 |
| corvus | tristanC: i was just about to mention that... | 15:39 |
| corvus | tristanC: btw, are you using a single node or multi node zk? | 15:40 |
| tristanC | corvus: this happen with single node zk | 15:40 |
| corvus | i would guess if you used multi-node, you might see zero or one exceptions.... | 15:41 |
| corvus | tristanC, fungi: i think it would be safe to put the tag on 793977 after it marges; only it and the ref logger changes affect run-time code, and they seem pretty low risk. | 15:44 |
| opendevreview | Lida Liu proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 15:44 |
| fungi | i'm checking right now to see whether pbr renders bd1a669 to the same dev version as is reported at the bottom of https://zuul.opendev.org/t/zuul/status (noting that the merge commit in the gate which ended up in the image we're running in opendev won't match that commit id), so to see if it probably corresponds to what we're currently running since the restart over the weekend | 15:45 |
| fungi | but building a zuul sdist is not quick, owing to pip install of all its deps i think | 15:46 |
| fungi | there it is. 4.3.1.dev53 yep, so that matches | 15:46 |
| fungi | but also yes i'd be okay with the current master branch state plus 793977, all of those seem unlikely to cause adverse impact | 15:49 |
| clarkb | I'll check as soon as I get this meeting agenda out | 15:56 |
| clarkb | looks like the major changes are the pyyaml fixup for executor side decryption, multiple semaphores, and improved stats/logging | 16:02 |
| *** marios has quit IRC | 16:02 | |
| clarkb | do we know if multiple sempahores have been used in production by anyone yet? otherwise no concerns from me | 16:02 |
| avass[m] | clarkb: I can check | 16:02 |
| avass[m] | we upgraded yesterday but I'm not sure if we're actually using it yet | 16:03 |
| corvus | clarkb: what would the concern be? | 16:03 |
| corvus | clarkb: oh, wanting to know if they've been production-tested? | 16:03 |
| clarkb | corvus: just that we might have to do a 4.4.1 release if there is unexpected contention behavior or similar | 16:04 |
| clarkb | ya | 16:04 |
| clarkb | I think doing a 4.4.1 is also fine, but if we have feedback now is a good time to incorporate it :) | 16:04 |
| corvus | clarkb: yeah, as far as a release goes, i'd be more concerned with regressions, since after all, if the new feature doesn't work as expected, it's not a big deal to .1 fix | 16:04 |
| opendevreview | Merged zuul/zuul master: gerrit: delay event queue attempts https://review.opendev.org/c/zuul/zuul/+/793977 | 16:05 |
| avass[m] | clarkb, corvus I found a change using it in review with +1 Verified from zuul. | 16:05 |
| fungi | i'm not super concerned if there's a new bug in a new feature, that's normal. more on the lookout for potential regressions but haven't seen any today | 16:05 |
| clarkb | avass[m]: excellent | 16:05 |
| fungi | or what corvus also just said | 16:05 |
| corvus | even better | 16:06 |
| corvus | okay, 977 merged; should i A) retag that sha assuming we're happy with only review, B) restart opendev with it for a quick sanity check then retag, or C) just push up the existing tag without the new changes? | 16:07 |
| fungi | 140 pipeline queue items in opendev's openstack tenant, so busy but not overwhelmingly so that a restart would be problematic | 16:09 |
| fungi | but i'm also not all that worried about those three additional changes, after skimming them | 16:09 |
| clarkb | considering the change that merged is "just" a log cleanup change I'd probably be happy with tagging the original proposal | 16:10 |
| corvus | clarkb: thoughts? ^ | 16:10 |
| clarkb | I may undervalue the impact of that logging cleanup though | 16:10 |
| corvus | clarkb: https://review.opendev.org/793977 adds a sleep | 16:10 |
| clarkb | oh hrm in that case its probably fine to tag it as is | 16:11 |
| corvus | there could be second order effects, but honestly, we probably wouldn't see them after a simple restart anyway; we'd have to have some kind of zk issue come up. | 16:11 |
| corvus | clarkb: so is that a vote for (A) tag current master? | 16:12 |
| clarkb | corvus: yes | 16:12 |
| fungi | and that way softwarefactory doesn't need to cherry-pick that last change | 16:13 |
| clarkb | there are three changes since the opendev restart. One is a test fix, another is a logging update, and the other adds a sleep to improve logging | 16:13 |
| clarkb | those all seem pretty safe to me | 16:13 |
| fungi | that was my conclusion as well | 16:13 |
| corvus | zuul-maint: updated tag proposal: commit 7b623a99ba96d3dbe10e29e1c37fbdaae6d2c863 (HEAD -> master, tag: 4.4.0, origin/master) | 16:13 |
| clarkb | corvus: ++ | 16:14 |
| *** vexxhost_ has joined #zuul | 16:14 | |
| fungi | yep, lgtm | 16:15 |
| fungi | that's my current master branch tip | 16:15 |
| fungi | and git log shows the expected three changes now after the previously proposed commit | 16:16 |
| *** vexxhost_ has quit IRC | 16:16 | |
| corvus | okay, i'll go ahead and push it, then send out an announcement later today | 16:17 |
| fungi | thanks! | 16:19 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Remove layout attribute from queue items https://review.opendev.org/c/zuul/zuul/+/792622 | 16:21 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Optimize layout re-calculation after re-enqueue https://review.opendev.org/c/zuul/zuul/+/792623 | 16:21 |
| swest | corvus: updated ^ | 16:22 |
| corvus | swest: awesome! thanks for working through that with me, i think the additional comments are a good foundation for understanding; makes a lot of sense now. :) | 16:24 |
| swest | corvus: thanks for your patience :D sometimes when you have all the details in your head it's hard to see the non-obvious things. | 16:26 |
| corvus | swest: yep! i think one of the best benefits of code review is improving shared understanding among contributors | 16:28 |
| corvus | swest: oh, and that change makes the next one so simple :) | 16:28 |
| corvus | clarkb: if you still have some of the layout cache optimization work in your head (you may recall that we reverted that); i think the unrevert at https://review.opendev.org/792622 and https://review.opendev.org/792623 is ready | 16:30 |
| corvus | basically, the first change makes it structurally harder to have the bug that necessitated the revert (by removing references to layout objects) | 16:31 |
| clarkb | corvus: I can give it a try | 16:34 |
| corvus | it's not urgent | 16:35 |
| clarkb | I'm trying to get through some local system updates (its like 3k pacakges * 3 machines) and some cleanup from yesterday's garage fun then I can do reviews and provision that cert and prep for the infra meeting | 16:40 |
| clarkb | and I think I have a meeting in there somewhere too | 16:40 |
| opendevreview | Lida Liu proposed zuul/zuul master: Gitlab: filter repository branches with protected branch rules https://review.opendev.org/c/zuul/zuul/+/793901 | 16:46 |
| *** jpena is now known as jpena|off | 16:47 | |
| clarkb | swest: corvus question on https://review.opendev.org/c/zuul/zuul/+/792622 | 17:06 |
| clarkb | swest: corvus I think I may have answered my own question(s) looking at the child change. I have updated my comments. Would be good if you can double hceck | 17:14 |
| corvus | clarkb: replied | 18:27 |
| tristanC | is it expected that `zuul tenant-conf-check` requires a zookeeper connection? | 18:52 |
| tristanC | the command is now failing with `Failed connecting to Zookeeper within the connection retry policy.` | 18:53 |
| avass[m] | tristanC: I think that schedules work to mergers to load all configuration and check for config errors, so yep | 18:53 |
| avass[m] | i could be wrong about that one though | 18:55 |
| tristanC | avass[m]: i thought that was zuul-scheduler --validate-tenants | 18:57 |
| avass[m] | tristanC: right, I was thinking of that one | 18:57 |
| tristanC | we are having a bunch of integration issues since the 4.2, for example changing the hostname seems to prevent the scheduler from processing trigger event until the previous lock expires. | 19:00 |
| *** hashar has quit IRC | 19:01 | |
| opendevreview | Tristan Cacqueray proposed zuul/zuul master: zuul tenant-conf-check: disable scheduler creation https://review.opendev.org/c/zuul/zuul/+/794027 | 19:11 |
| corvus | tristanC: you mean changing the hostname while the scheduler process is running? | 19:19 |
| tristanC | corvus: and restarting it | 19:20 |
| tristanC | it has to be restarted for sql access | 19:20 |
| corvus | tristanC: it sounds like the scheduler may not be shutting down gracefully | 19:26 |
| corvus | which, if it hasn't progressed past the sql connection stage, sounds pretty likely | 19:26 |
| tristanC | corvus: i guess this does not happen by default with systemd? i'm retrying with `ExecStop=/usr/bin/zuul-scheduler stop` | 19:36 |
| corvus | tristanC: this behavior change may be relevant too: https://review.opendev.org/790339 | 19:36 |
| corvus | it will wait for sql at startup now -- perhaps you don't need to externally restart with that in place? | 19:37 |
| corvus | tristanC: but if you do, then i could see that potentially being a problem | 19:38 |
| corvus | tristanC: i would expect 'stop' to shut everything down correctly | 19:38 |
| corvus | so exactly what's happening is still a mystery, but there are some places to look | 19:39 |
| tristanC | supporting hostname change is a bit tricky with lots of corner case, so we'd rather force a full restart in that situation | 19:42 |
| tristanC | from merger git email to sql access, it has been safer to just reload | 19:43 |
| corvus | yeah, that's certainly unexpected and hard to defensively code for :) | 19:44 |
| tristanC | i think what is happening is that the gerrit connection leak an election lock with the previous hostname, and when the scheduler restart, the new gerrit connection does not become the leader | 19:45 |
| corvus | tristanC: so i think the area to focus on is why isn't the lock (i assume it's an event queue election) being released on shutdown (and i assume it's not being released and we're waiting on the zookeeper ephemeral node timeout) | 19:45 |
| corvus | tristanC: i thought the election recipe only used the hostname for informational purposes (ie, it's actually the fact that the connection created the znode that gives it the lock) | 19:46 |
| corvus | tristanC: that's why i'm leaning toward unclean shutdown. | 19:46 |
| corvus | tristanC: i could be wrong about that, but that's where i'd start looking | 19:46 |
| tristanC | corvus: the cmd.scheduler exit_handler does not seem to call scheduler.stop | 19:47 |
| corvus | that sure sounds like it could be relevant. | 19:47 |
| opendevreview | Tristan Cacqueray proposed zuul/zuul master: scheduler: call stop on SIGTERM https://review.opendev.org/c/zuul/zuul/+/794035 | 19:49 |
| tristanC | the hostname change seems unrelated indeed | 19:51 |
| tristanC | corvus: thank you for the help | 19:52 |
| corvus | tristanC: np, thanks for finding that :) | 19:55 |
| tristanC | in another attempt i'm running `zkCli.sh deleteall /zuul/events` before restarting the scheduler, and that seems to help | 19:55 |
| corvus | tristanC: good idea (temporarily) -- the election nodes are under /zuul/events/connnection/$name/election i think | 19:57 |
| *** Ganneff has left #zuul | 19:57 | |
| tristanC | calling zuul-scheduler stop is not enough, i'm now retrying with 794035 | 20:40 |
| *** hashar has joined #zuul | 20:46 | |
| *** hashar has quit IRC | 20:47 | |
| clarkb | corvus: I think I'm getting confused by lines 851-852 of https://review.opendev.org/c/zuul/zuul/+/792622/5/zuul/manager/__init__.py Why does that logging need to be in an if block? Are we only interested in logging that we are calculating a layout if we have a cache miss that isn't an initial population of the cache? | 21:35 |
| clarkb | corvus: I agree about your comment on defense against the None key | 21:36 |
| corvus | clarkb: re comment -- yeah that's my understanding. the way i'm reading it is basically "this logs a cache miss that results in a recalculation". the calculation itself will have a whole bunch of logging anyway, so i think this is just an extra "here's why we're about to do all this stuf..." | 21:38 |
| corvus | clarkb: and i think it's interesting because i don't think it should happen until we have another scheduler | 21:39 |
| corvus | so it's a potentially interesting bit of debug info | 21:39 |
| clarkb | got it, that helps | 21:39 |
| clarkb | as for whether or not we should be defensive there I'm not sure. We definitely expect None as a valid uuid value in some places, and if that leaked into the cache we coudl have shared layouts ? | 21:40 |
| corvus | clarkb: yeah, that's my understanding. obvs it shouldn't happen, but if it did, only something like your suggestion of an explicit test would catch it | 21:40 |
| corvus | (i mean a layout object should never have None for a uuid; an item can have None as it's layout_uuid, indicating it's not yet set) | 21:42 |
| clarkb | right and we only update the cache using the layout object's uuid (by way of the newly updated item object) | 21:42 |
| corvus | the layout class accepts a layout param in the initializer, and None means 'generate a new one'; that's in like one line of python, so unlikely to get wrong, but concievable it could happen in a later refactor or something | 21:43 |
| clarkb | maybe the cache should be set using only the layout object and that is good enough | 21:43 |
| corvus | clarkb: yep (re updating the cache) | 21:43 |
| corvus | i think line 857 is the only place the cache is written to | 21:43 |
| corvus | yeah (other than the delete); so that's the only insertion | 21:44 |
| clarkb | ya so this is probably ok as is, unless we change the management around uuids | 21:48 |
| clarkb | and that seems unlikely since we are using rnaodm uuids | 21:48 |
| corvus | ++ | 21:49 |
| corvus | clarkb: cool, i'll let that sit overnight in case any of that discussion sparks a concern with swest; if not i'll +w tomorrow | 21:55 |
| clarkb | sounds good. I'm looking at the second change now | 21:56 |
| clarkb | corvus: question on https://review.opendev.org/c/zuul/zuul/+/792623 | 22:13 |
| corvus | clarkb: yeah, i think that one has had me scratching my head before too; but i think it's all actually correct; replied | 22:20 |
| clarkb | corvus: aha that makes sense | 22:22 |
| corvus | clarkb: thanks for reviewing; i'm looking forward to speeding up reconfiguration times :) | 22:26 |
| corvus | maybe this will make it so it's less important to have those queue length items on the status page ;) | 22:27 |
| clarkb | that would be cool | 22:27 |
| corvus | avass: if you have time for a quick re-review of https://review.opendev.org/793829 i'd appreciate it | 22:34 |
| *** tosky has quit IRC | 23:11 | |
| *** eliadcohen_ has joined #zuul | 23:47 | |
| *** eliadcohen has quit IRC | 23:51 | |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!