| opendevreview | Merged zuul/zuul master: Use an actual database in all Zuul tests https://review.opendev.org/c/zuul/zuul/+/797181 | 00:02 |
|---|---|---|
| opendevreview | Merged zuul/zuul master: Remove ZuulDBTestCase https://review.opendev.org/c/zuul/zuul/+/798350 | 00:26 |
| *** bhagyashris|ruck is now known as bhagyashris|out | 05:53 | |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Don't clear connection caches during full reconfig https://review.opendev.org/c/zuul/zuul/+/800066 | 06:06 |
| swest | clarkb: corvus: ^ fixed the failing test | 06:16 |
| swest | corvus: I'm interested in your opinion regarding the clearCaches API in 800066 | 06:17 |
| *** jpena|off is now known as jpena | 07:11 | |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Lock tenants, pipelines during processing https://review.opendev.org/c/zuul/zuul/+/796013 | 07:24 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Don't clear connection caches during full reconfig https://review.opendev.org/c/zuul/zuul/+/800066 | 07:34 |
| *** mgoddard- is now known as mgoddard | 08:40 | |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Mount connection sshkeys on executors and mergers https://review.opendev.org/c/zuul/zuul-operator/+/799998 | 08:41 |
| *** jpena is now known as jpena|lunch | 11:20 | |
| *** jpena|lunch is now known as jpena | 12:25 | |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Lock tenants, pipelines during processing https://review.opendev.org/c/zuul/zuul/+/796013 | 12:33 |
| opendevreview | Simon Westphahl proposed zuul/zuul master: Lock tenants, pipelines during processing https://review.opendev.org/c/zuul/zuul/+/796013 | 12:37 |
| opendevreview | Felix Edel proposed zuul/zuul master: Switch to ZooKeeper backed NodesProvisionedEvents https://review.opendev.org/c/zuul/zuul/+/799833 | 12:48 |
| corvus | swest: i'm inclined to remove it now; it would be easy to add back in a new form if we're wrong (but also, a rolling scheduler restart would accomplish the same thing in the future). (i'll leave a comment on the change too) | 13:48 |
| avass[m] | corvus: if you wanna do a quick operator review: https://review.opendev.org/c/zuul/zuul-operator/+/799998 | 14:10 |
| avass[m] | oh I see another issue too | 14:11 |
| corvus | avass: ah so that's setting a per-connection sshkey. ++ i'll look into covering that case in the update to the functional job. | 14:13 |
| avass[m] | corvus: yeah that was configured for zuul.conf and mounted on the scheduler but no on executors or mergers | 14:19 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Set component command with args instead of command https://review.opendev.org/c/zuul/zuul-operator/+/800263 | 14:19 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Configure debug logs for merger https://review.opendev.org/c/zuul/zuul-operator/+/800264 | 14:19 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Gracefully shutdown executor on SIGTERM https://review.opendev.org/c/zuul/zuul-operator/+/800265 | 14:19 |
| avass[m] | the operator had the usual issue with overriding command (docker entrypoint) instead of setting args :) | 14:20 |
| corvus | tristanC: could you look at https://review.opendev.org/799701 real quick? | 14:21 |
| tristanC | corvus: sure, the change LGTM, though I left a small nit about the meaning of the env value | 14:26 |
| corvus | oh, flipping the default? well, since people may already be using sigterm, i figured this was the least disruptive. and i'm also not sure which default is "best". avass wants 'graceful', opendev wants 'stop'. it's kind of a toss-up which we should make default. :| | 14:29 |
| avass[m] | I very much think the operator should do graceful shutdowns at least :) | 14:29 |
| corvus | avass: i'd like the operator to do zuul's default by default | 14:30 |
| corvus | of course it should be able to be set as an option in the operator :) | 14:30 |
| avass[m] | just thinking that the operator handles that for you so any human operator has less controll of how the services are managed | 14:31 |
| corvus | avass: if you think that the operator should do that by default, then let's translate your argument into saying that zuul should do it by default and accept tristanC's suggestion and flip the default. | 14:32 |
| avass[m] | also it's less harmful for newer installations to check why their upgrade/restart whatever is taking some time to happen than "oops all jobs got lost" | 14:32 |
| avass[m] | corvus: yeah maybe that's the better way to do it? | 14:33 |
| tristanC | corvus: i meant to keep the default, it's just that it can be confusing to use `1`/`true` or `0` to enable or disable a feature through the environment, what about `yes` and `no` ? | 14:33 |
| avass[m] | tristanC: I think all of those are confusing since every service does it differently. :) | 14:35 |
| tristanC | the presence of the environment variable should be enough, but it can be more difficult to remove in some situation (e.g. helm template) | 14:35 |
| avass[m] | I just think the environment variables needs to be well documented | 14:35 |
| corvus | we could say "ZUULEXECUTORSIGTERM_BEHAVIOR=graceful" :) | 14:35 |
| corvus | we could say "ZUUL_EXECUTOR_SIGTERM_BEHAVIOR=graceful" :) | 14:35 |
| tristanC | corvus: that looks better to me | 14:36 |
| avass[m] | I like that one | 14:36 |
| corvus | i might use method to avoid the us/gb behavio[u]r issue | 14:39 |
| avass[m] | by writing everything in swedish instead? :) | 14:40 |
| corvus | avass: we're writing swedish now, right? ;) | 14:40 |
| avass[m] | bork bork | 14:41 |
| fungi | that chef really had style though | 14:43 |
| opendevreview | James E. Blair proposed zuul/zuul master: Add graceful stop environment variable https://review.opendev.org/c/zuul/zuul/+/799701 | 14:44 |
| opendevreview | James E. Blair proposed zuul/zuul master: Flip default of ZUUL_EXECUTOR_SIGTERM_METHOD https://review.opendev.org/c/zuul/zuul/+/800270 | 14:44 |
| corvus | tristanC, avass, tobiash, clarkb, fungi, mnaser: ^ i separated concerns there so you can review those independently. i'm not tied to one default or the other, but i do think that the operator should have the same default as zuul (but also the option to change it in the CRD). | 14:47 |
| avass[m] | to be fair it would be really weird seeing `ZUUL_EXEKVERARE_SIGTERM_BETEENDE=graviös`, like that one error message that's in hebrew in php | 14:49 |
| avass[m] | *graciös :) | 14:49 |
| tristanC | corvus: thanks | 14:49 |
| avass[m] | corvus: lgtm, I don't mind either way for 800270 (since I'm aware of it) but I still think it's safer default behaviour | 14:50 |
| fungi | corvus: on 799701, just to confirm, you did intend for that to be a substring match right? so ZUUL_EXECUTOR_SIGTERM_METHOD=g or ZUUL_EXECUTOR_SIGTERM_METHOD=race all work | 14:59 |
| corvus | fungi: hah i did not :) | 14:59 |
| fungi | at first i misread it as a membership comparison, but then realized there was no trailing , so the () were merely decorative and not a tuple | 14:59 |
| corvus | yep, i fell into that trap. in my defense, i haven't had breakfast yet. | 15:00 |
| fungi | i'll unapprove | 15:00 |
| opendevreview | James E. Blair proposed zuul/zuul master: Add graceful stop environment variable https://review.opendev.org/c/zuul/zuul/+/799701 | 15:05 |
| opendevreview | James E. Blair proposed zuul/zuul master: Flip default of ZUUL_EXECUTOR_SIGTERM_METHOD https://review.opendev.org/c/zuul/zuul/+/800270 | 15:05 |
| clarkb | corvus: I've approved https://review.opendev.org/c/zuul/zuul/+/800066 | 15:34 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: WIP: add static node to functional test https://review.opendev.org/c/zuul/zuul-operator/+/799874 | 16:02 |
| *** jpena is now known as jpena|off | 16:11 | |
| opendevreview | Merged zuul/zuul master: Add graceful stop environment variable https://review.opendev.org/c/zuul/zuul/+/799701 | 16:15 |
| corvus | tristanC, tobiash, clarkb, fungi: can you review https://review.opendev.org/800270 please? since 799701 merged, i'd like to make a decision on 800270 asap. i'm fine with either decision, i just don't want to change it across a release boundary. feel free to -1 without comment even just to record a vote. | 16:19 |
| opendevreview | Merged zuul/zuul master: Don't clear connection caches during full reconfig https://review.opendev.org/c/zuul/zuul/+/800066 | 16:40 |
| opendevreview | James E. Blair proposed zuul/zuul master: Flip default of ZUUL_EXECUTOR_SIGTERM_METHOD https://review.opendev.org/c/zuul/zuul/+/800270 | 16:43 |
| *** holser is now known as holser_ | 16:52 | |
| *** holser_ is now known as holser | 16:52 | |
| fungi | corvus: commented, i wonder if the behavior when setting that envvar to empty might confuse some people, but i don't know if that's a real concern | 16:53 |
| opendevreview | James E. Blair proposed zuul/zuul master: Flip default of ZUUL_EXECUTOR_SIGTERM_METHOD https://review.opendev.org/c/zuul/zuul/+/800270 | 16:56 |
| corvus | fungi: ^ | 16:56 |
| avass[m] | corvus: I think you misunderstood the last comment by fungi | 17:18 |
| avass[m] | the last patch set looks like it will only log an error in the exit handler if the environemnt variable is given a bad value so the executor has to be SIGKILLED | 17:19 |
| fungi | yes, the error condition turns the signal handler into a no-op (aside from logging the warning of course) | 17:20 |
| fungi | so no action is taken on any sigkill received | 17:20 |
| fungi | er, sigterm i mean | 17:21 |
| corvus | what would you rather it do? | 17:21 |
| avass[m] | log an error and stop/raise an exception? | 17:21 |
| corvus | which stop method? | 17:21 |
| fungi | i don't have a particular preference, i was just pointing out that it will do nothing and need a sigkill to actually terminate | 17:22 |
| fungi | which might be fine | 17:22 |
| corvus | fungi: or a "zuul-executor stop" to terminate | 17:22 |
| fungi | right, or that. just not sigterm | 17:22 |
| corvus | yep | 17:22 |
| avass[m] | I think I'd expect a bad value to use the default behaviour, or error on startup but we're not checking that on start | 17:23 |
| opendevreview | James E. Blair proposed zuul/zuul master: Revert "Add graceful stop environment variable" https://review.opendev.org/c/zuul/zuul/+/800282 | 17:25 |
| avass[m] | :) | 17:26 |
| corvus | okay, i'm going to revert out the already-merged thing | 17:26 |
| corvus | since this discussion is growing and growing, i don't want to split these 2 changes across a release, and i want to be able to make a release | 17:26 |
| avass[m] | I think the above change does the right thing and we just need to agree to what the default behaviour should be | 17:26 |
| corvus | so we'll revert out the env variable, then i'll push up a revert-revert, WIP it, and we can continue to discuss | 17:26 |
| opendevreview | James E. Blair proposed zuul/zuul master: Flip default of ZUUL_EXECUTOR_SIGTERM_METHOD https://review.opendev.org/c/zuul/zuul/+/800270 | 17:28 |
| opendevreview | James E. Blair proposed zuul/zuul master: Add graceful stop environment variable https://review.opendev.org/c/zuul/zuul/+/800284 | 17:28 |
| corvus | avass: what do you mean the default behavior? that change sets the default to 'graceful'. do you mean we need to agree what the error behavior should be? | 17:30 |
| avass[m] | something along the lines of "if `ZUUL_EXECUTOR_SIGTERM_METHOD` specifies a valid method do that, otherwise fall back to default behaviour", so I don't think splitting the change into two helps in this case | 17:30 |
| corvus | i'm not splutting it into two | 17:30 |
| *** melwitt is now known as Guest322 | 17:31 | |
| corvus | it was already split in two | 17:31 |
| corvus | we just merged the first one too early | 17:31 |
| fungi | avass[m]: so use stop is stop is explicitly specified, use graceful otherwise *but* if something is specified which is neither stop nor graceful, emit an error message (but still do a graceful shutdown after logging the warning) | 17:32 |
| avass[m] | fungi: yep | 17:32 |
| fungi | seems reasonable | 17:33 |
| avass[m] | corvus: yes actually you're right there, but the logic breaks because the "Flip" change also requires valid input the way it's written | 17:33 |
| avass[m] | which the first change didn't require | 17:33 |
| corvus | sure, i was just trying to give people an opportunity to weigh in on what the default should be | 17:34 |
| corvus | if we don't merge the flip, we can still apply the same thing to the other change | 17:34 |
| corvus | this is turning into a huge time sink though :( | 17:35 |
| avass[m] | do we really need to revert 800282 then? | 17:35 |
| fungi | in the first change it was less problematic because the fallback was to the default. the second change inverted the default but did not invert the fallback | 17:35 |
| corvus | absolutely, i have no reason to believe we can get everyone to agree on this within the next few minutes | 17:36 |
| corvus | and we're about to do an opendev restart | 17:36 |
| avass[m] | I mean 799701 didn't change the default behaviour, just added a way to do something else. The default can be switched later | 17:37 |
| opendevreview | James E. Blair proposed zuul/zuul master: Add graceful stop environment variable https://review.opendev.org/c/zuul/zuul/+/800284 | 17:37 |
| corvus | it added a new environment variable; i don't think it's very professional to make a release with that env variable knowing we're going to change it | 17:38 |
| corvus | okay, so all of my attempts to make this simple and quick have failed. so it's just one change now. | 17:38 |
| corvus | we'll just iterate over https://review.opendev.org/800284 in the normal way until we find something we all like | 17:39 |
| corvus | sorry for all the churn :( | 17:39 |
| avass[m] | I don't think we're changing the behaviour of the environment variable with the follow up | 17:39 |
| corvus | we were changing the default | 17:40 |
| corvus | i never intended to do that across a release | 17:40 |
| avass[m] | but yeah I agree that it's not great since it would lead to people who want that behaviour adding an environment variable that will become unecessary | 17:40 |
| corvus | i thought we could merge change #1 (add a variable) and, if we wanted the default flipped, merge change #2 immediately, so no users would ever see #1 without #2. but it turned out all the deferred review items from #1 just piled up on #2, so we can't merge it quickly. | 17:42 |
| corvus | anyway, tristanC, tobiash, clarkb, fungi, avass: please review https://review.opendev.org/800284 but don't approve it. let's get everyones comments on that and merge it when we have consensus. i'm sorry for the extra work :( | 17:44 |
| fungi | no worries, process management is a generally painful topic | 17:46 |
| *** melwitt_ is now known as melwitt | 17:57 | |
| *** melwitt is now known as jgwentworth | 17:58 | |
| tristanC | corvus: wfm, thanks again! | 18:00 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Set component command with args instead of command https://review.opendev.org/c/zuul/zuul-operator/+/800263 | 18:13 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: Configure debug logs for merger https://review.opendev.org/c/zuul/zuul-operator/+/800264 | 18:13 |
| opendevreview | Merged zuul/zuul master: Revert "Add graceful stop environment variable" https://review.opendev.org/c/zuul/zuul/+/800282 | 18:35 |
| opendevreview | Albin Vass proposed zuul/zuul-operator master: WIP: Add structural schema to crd https://review.opendev.org/c/zuul/zuul-operator/+/800302 | 19:09 |
| avass[m] | should we bump the apiversion if we add a schema to the crd? | 19:10 |
| corvus | avass: i don't think so. how is the schema useful? | 19:17 |
| avass[m] | corvus: makes it possible to validate the object before applying it | 19:18 |
| avass[m] | or rather you'll error earlier when you `kubectl apply -f ...` | 19:18 |
| avass[m] | from what I understand | 19:18 |
| tristanC | the schema also enable a web based form when creating the crd in the openshift console | 19:19 |
| corvus | gotcha, thx. | 19:20 |
| clarkb | corvus: re https://review.opendev.org/800284 the opendev docker-compose file will probably need to be updated with that change? Thats fine with me, just want to call out impact to other users | 19:25 |
| corvus | clarkb: yes | 19:26 |
| fungi | right, i assumed we'd do that if/when it merges | 19:27 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Add static node to functional test https://review.opendev.org/c/zuul/zuul-operator/+/799874 | 19:41 |
| corvus | that succeeded, so i just removed the WIP flag from the commit msg | 19:42 |
| corvus | i think we can rebase avass's ssh key change on that and maybe look at expanding the test to include that | 19:42 |
| avass[m] | ++ | 19:42 |
| corvus | i'll just go ahead and push up a rebase | 19:43 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Mount connection sshkeys on executors and mergers https://review.opendev.org/c/zuul/zuul-operator/+/799998 | 19:44 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Set component command with args instead of command https://review.opendev.org/c/zuul/zuul-operator/+/800263 | 19:44 |
| opendevreview | James E. Blair proposed zuul/zuul-operator master: Configure debug logs for merger https://review.opendev.org/c/zuul/zuul-operator/+/800264 | 19:44 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!