| -@gerrit:opendev.org- Zuul merged on behalf of Benjamin Schanzel: [zuul/nodepool] 853168: Add config option to limit ephemeral storage on K8s Pod labels https://review.opendev.org/c/zuul/nodepool/+/853168 | 06:49 | |
| -@gerrit:opendev.org- Zuul merged on behalf of Benjamin Schanzel: [zuul/nodepool] 851988: Implement cleanup of leaked resources in k8s driver https://review.opendev.org/c/zuul/nodepool/+/851988 | 11:21 | |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/nodepool] 862500: Add debug logs to the static driver https://review.opendev.org/c/zuul/nodepool/+/862500 | 15:41 | |
| @clarkb:matrix.org | corvus: ^ and https://review.opendev.org/c/zuul/nodepool/+/862414/ are probably both good to go. I'll WIP the other change until we can catch it failing with the better debug logs | 15:41 |
|---|---|---|
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/nodepool] 862500: Add debug logs to the static driver https://review.opendev.org/c/zuul/nodepool/+/862500 | 17:56 | |
| @clarkb:matrix.org | slight improvement to the logs since the check jobs were already failing | 17:56 |
| -@gerrit:opendev.org- Zuul merged on behalf of James E. Blair https://matrix.to/#/@jim:acmegating.com: [zuul/zuul] 860989: Remove Ansible 5 https://review.opendev.org/c/zuul/zuul/+/860989 | 18:25 | |
| -@gerrit:opendev.org- James E. Blair https://matrix.to/#/@jim:acmegating.com proposed: [zuul/nodepool] 862522: WIP: openstack statemachine https://review.opendev.org/c/zuul/nodepool/+/862522 | 19:44 | |
| @clarkb:matrix.org | corvus: I think I understand the static driver issue a lot better now (the extra debug logging did help). We're stopping the old provider manager after creating the new provider manager. This allows the old provider manager's syncNodeCount() to be called from the cleanup routines for the old manager after the new manager is created. If this happens and we idle then nothing triggers syncNodeCount() to reconcile the resulting delta | 20:25 |
| @clarkb:matrix.org | I think an easy fix here is to stop provider managers before starting them. Does anyone know why we don't do this already? | 20:25 |
| @jim:acmegating.com | Clark: because in a cloud, it would appear to be a huge outage | 20:28 |
| @jim:acmegating.com | it's technically possible for a provider to take hours to stop. | 20:29 |
| @jim:acmegating.com | that's worst-case, but even under typical cases, that could be *many* minutes for a cloud like rax | 20:29 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/nodepool] 862526: Stop provider managers before starting new ones https://review.opendev.org/c/zuul/nodepool/+/862526 | 20:31 | |
| @clarkb:matrix.org | Thats the naive swap of ordering. | 20:32 |
| @clarkb:matrix.org | if nothing else it will illustrate the problem I hope. It does seem like maybe the static driver needs some more global locking or perhaps a less forceful stop() call we can call first? | 20:33 |
| @clarkb:matrix.org | maybe it is ok to stop the periodic tasks, but let existing boots complete? | 20:33 |
| @jim:acmegating.com | Clark: is that calling this method? https://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/static/provider.py#L409 | 20:37 |
| @clarkb:matrix.org | oh wow we don't even stop the driver with the stop() method? | 20:38 |
| @clarkb:matrix.org | The problem is https://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/static/provider.py#L436-L449 is being called via periodic tasks on the old manager after the new manager's start() method has been called. This effectively overwrites the info in zk for the new config immediately after it is written | 20:39 |
| @clarkb:matrix.org | you can see that in this log here: https://storage.gra.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_904/862500/1/check/nodepool-tox-py38/9042733/testr_results.html starting roughly with `2022-10-24 17:28:09,844 DEBUG [nodepool.driver.static.StaticNodeProvider] Starting static provider static-provider` | 20:40 |
| @clarkb:matrix.org | corvus: reading more it appears that the cleanup threads lookup the provider managers out of the config. But we don't swap the configs out until after starting then "stopping" the old managers | 20:52 |
| @clarkb:matrix.org | maybe we can put a lock around that? | 20:53 |
| @jim:acmegating.com | Clark: that sounds good | 20:57 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/nodepool] 862526: Lock around config updates and provider manager usage https://review.opendev.org/c/zuul/nodepool/+/862526 | 21:19 | |
| @clarkb:matrix.org | corvus: ^ something like that maybe? I'm not entirely sure I've locked around all the necessary areas. I used getProviderManager() calls as my clues. | 21:19 |
| @jim:acmegating.com | Clark: is assignhandlers necessary? | 21:26 |
| @jim:acmegating.com | Clark: i think the assignhandlers lock is going to cause similar problems to the reordering. it still feels like the tail wagging the dog. it would be great if the fix for this could be in the static driver. | 21:32 |
| @jim:acmegating.com | Clark: i think i like your idea of a new stop method. that would contain the impact to just the static driver. | 21:39 |
| @clarkb:matrix.org | I think part of the problem here is due to the assumption that state is external for things like resource cleanup | 21:48 |
| @clarkb:matrix.org | which means that we can safely invoke cleanups against the old or new config and there isn't a difference | 21:48 |
| @jim:acmegating.com | yep, it's absolutely the case that the static driver does not fit the model | 21:49 |
| @jim:acmegating.com | which is why i want to avoid changing too much of nodepool to accommodate it. i'd rather have an extra stop method that no other driver has, than to shutdown a busy system for every reconfiguration (which happens continuously) | 21:51 |
| @clarkb:matrix.org | ya that seems fine. Another thought I've had is wondering if we can separate the state from the reconfiguration and let it persist somehow, but I think that is basically wha we have now and its the frame of reference for operating on the staet that is confused | 21:52 |
| @jim:acmegating.com | yep | 21:53 |
| @clarkb:matrix.org | alright let me see about adding a provider.idle() call we can do | 21:54 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/nodepool] 862526: Add idle state to driver providers https://review.opendev.org/c/zuul/nodepool/+/862526 | 22:10 | |
| @clarkb:matrix.org | I think that does it generically enough that if other drivers needed to have a similar hook point they can take advantage of it while still keeping the delta here to a minimum to reduce the chances of holding up slow drivers | 22:11 |
| @jim:acmegating.com | well, it's not just slow drivers i'm worried about, it's busy ones :) | 22:15 |
| @clarkb:matrix.org | heh there is abug in my change. Yay for testing. But also corvus do you know if TestDriver in the testsuite is used for anything more? | 22:23 |
| @jim:acmegating.com | Clark: i'm not sure that entire driver is used for anything | 22:27 |
| @clarkb:matrix.org | ya I'm looking at it and I can't see where it gets loaded. I'll push a followup change to delete it and see if it breaks the tests | 22:27 |
| @jim:acmegating.com | Clark: oh i think i know | 22:28 |
| @jim:acmegating.com | Clark: it's a minimalist driver for interactive testing of the launcher itself. | 22:28 |
| @jim:acmegating.com | Clark: let's keep it and just add the method | 22:28 |
| @clarkb:matrix.org | ack | 22:29 |
| -@gerrit:opendev.org- Clark Boylan proposed: [zuul/nodepool] 862526: Add idle state to driver providers https://review.opendev.org/c/zuul/nodepool/+/862526 | 22:30 | |
| @clarkb:matrix.org | I did eventually find some configs that set `driver: test` so it is used | 22:31 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!