| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-07-30 | |||
| 15:03:49 | opendevreview | Ian Watson proposed openstack/nova master: Add migration conditional for resource tracker https://review.opendev.org/c/openstack/nova/+/955690 | |
| 15:09:23 | dansmith | gibi: andrewbonney I had commented about some missing test coverage | |
| 15:09:40 | dansmith | looks like we raced and I lost | |
| 15:14:22 | gibi | I'm fine pulling it from the gate or looking at a fup with the test coverage | |
| 15:15:42 | dansmith | maybe andrewbonney can at least respond to my question and do a FUP if we just need an extra assert in an adjacent test | |
| 15:16:23 | andrewbonney | I'll take a look, but I've been away for a few months so need to context switch back in | |
| 15:18:03 | sean-k-mooney | oh the dns patch | |
| 15:19:17 | sean-k-mooney | dansmith: your asking is the a testcase where we do resest the dns name? | |
| 15:20:04 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for quota sets API https://review.opendev.org/c/openstack/nova/+/956141 | |
| 15:20:05 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server migrations API https://review.opendev.org/c/openstack/nova/+/956143 | |
| 15:20:05 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for migrations API https://review.opendev.org/c/openstack/nova/+/956142 | |
| 15:20:06 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server metadata APIs https://review.opendev.org/c/openstack/nova/+/956145 | |
| 15:20:06 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server tags API https://review.opendev.org/c/openstack/nova/+/956144 | |
| 15:22:55 | dansmith | sean-k-mooney: no, | |
| 15:23:12 | sean-k-mooney | oh then i did not understand your question in the review | |
| 15:23:40 | dansmith | sean-k-mooney: the new test case added an assertion that we don't call reset when we're not supposed to, but I don't see an adjacent similar test case that asserts that we do call it when expected | |
| 15:23:59 | dansmith | I'm assuming there just needs to be an assert_called_once() somewhere in one of the nearby tests | |
| 15:24:26 | dansmith | point being, the patch is disabling calling that function in some situations, but I don't know that anything would break if it incorrectly disables the call in _all_ situations | |
| 15:26:48 | sean-k-mooney | oh ok am its only requrie if a prot is detached so the next test i think could have that mocked and assert its called | |
| 15:27:43 | sean-k-mooney | although in the detach flow | |
| 15:27:48 | sean-k-mooney | im not sure we need to do that | |
| 15:28:00 | sean-k-mooney | so we might be able to remove _reset_port_dns_name entirly | |
| 15:28:16 | dansmith | doesn't that make this patch totally different then? | |
| 15:28:28 | sean-k-mooney | i woudl have to lod context again | |
| 15:28:34 | dansmith | that's removing a line we were always calling.. this patch makes it conditional, but never checks the was-default-before case | |
| 15:28:41 | dansmith | gibi: andrewbonney ^ | |
| 15:29:07 | sean-k-mooney | si the _reset_port_dns_name call if if we are using extenreal dns integration | |
| 15:29:31 | sean-k-mooney | if we are uisng neutron internl dns the its cleared vai port update on detact. | |
| 15:29:44 | sean-k-mooney | i think the code is still correct | |
| 15:29:48 | dansmith | that has nothing to do with my concern that we don't know if we're doing it or not | |
| 15:29:59 | sean-k-mooney | but we maybe jsut missing a test for that edge case | |
| 15:30:16 | dansmith | it's not an edge case, it's what we were *always* doing before | |
| 15:30:32 | sean-k-mooney | yes which was incorect | |
| 15:30:35 | dansmith | and if it wasn't problematic for andrewbonney's situation, this patch wouldn't be needed | |
| 15:30:42 | sean-k-mooney | we shoudl never have been doing it on unbind | |
| 15:30:44 | sean-k-mooney | only on detach | |
| 15:31:10 | sean-k-mooney | in both the internal and external dns case we were privously doing it unconditionally | |
| 15:31:21 | sean-k-mooney | now both path only do it if its a detach | |
| 15:32:23 | dansmith | none of that has anything to do with the tests I'm quoting.. I'm talking about the _unit_ test that checks that this method does what it should based on the various inputs, and there's now a conditional that we don't (AFAIK) verify does the right thing | |
| 15:34:07 | sean-k-mooney | detach defautls to true. so test_unbind_ports_reset_dns_name_by_admin whih is the next test calls unbiend iwth it set to ture | |
| 15:34:26 | dansmith | right, and where's the assert that reset_dns was called? | |
| 15:34:30 | sean-k-mooney | and the new test passes detach=false | |
| 15:34:37 | sean-k-mooney | it never existed before | |
| 15:34:41 | dansmith | I KNOW THAT | |
| 15:35:06 | sean-k-mooney | my point is the lack fo that assert is not related to this patch | |
| 15:35:18 | sean-k-mooney | we can add it | |
| 15:35:32 | dansmith | before, there was no question.. it was clearly a gap in the assertion, but at least there was no way for it to not be run.. now there *is* a way for it not to be run, and the test doesn't assert that it is called when we expect it to | |
| 15:37:11 | sean-k-mooney | sure we can add the assert. i have no issue with that | |
| 15:39:38 | sean-k-mooney | we can proably check this in the funtional test as well. we jsut need to replace shelve with detach | |
| 15:43:15 | dansmith | that's probably something we should be doing anyway.. making sure that this gets called properly in the high-level op that it purports to change | |
| 15:43:39 | sean-k-mooney | we had very minimal test coveage wehn thsi was added to nova in the first place https://github.com/openstack/nova/commit/997d8f516cee99b4e16429d13ca5cf7fc05166aa | |
| 15:43:49 | sean-k-mooney | so its as good a time to improve that as any | |
| 15:46:06 | sean-k-mooney | well actully this funciton was added in https://github.com/openstack/nova/commit/1b797f6f7e99fdef380340d6fe29e4004be48781 later | |
| 15:53:58 | opendevreview | Dan Smith proposed openstack/nova master: Don't reset port dns_name when shelving instances https://review.opendev.org/c/openstack/nova/+/919961 | |
| 15:54:50 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server tags API https://review.opendev.org/c/openstack/nova/+/956144 | |
| 15:54:50 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server migrations API https://review.opendev.org/c/openstack/nova/+/956143 | |
| 15:54:51 | opendevreview | Stephen Finucane proposed openstack/nova master: api: Add response body schemas for server metadata APIs https://review.opendev.org/c/openstack/nova/+/956145 | |
| 18:18:24 | sean-k-mooney | gmaan: i added my +2 and some other comments. i think im fine to refiene the docs/release not in a follow up but holding +w so you can read the comments | |
| 18:18:47 | sean-k-mooney | if no one else provides feedback ill add +w tomorrow or others are freee to do so | |
| 18:18:56 | gmaan | sean-k-mooney: checking | |
| 18:23:15 | sean-k-mooney | gibi: dan adressed there testing comment in https://review.opendev.org/c/openstack/nova/+/919961 woudl you mind looking at that again tomorow when your aroudn and reappoving it if your still happy. i just recheked it because of a kernel panic so hopefully it will be green in the morning | |
| 18:29:00 | gmaan | sean-k-mooney: replied, 'i think im fine to refiene the docs/release not in a follow up' - > you mean to you are ok to do it in follow up | |
| 18:29:28 | gmaan | ohk I think 'release not' -> 'release note' got it now | |
| 18:29:36 | gmaan | fixing those in follow up | |
| 18:29:40 | sean-k-mooney | ya im fine with a follow up | |
| 18:29:45 | gmaan | cool | |
| 18:29:59 | sean-k-mooney | you can respin if you want but i think tis udnerstandable just the wording could be clearer | |
| 19:27:58 | opendevreview | Balazs Gibizer proposed openstack/nova master: Allow services to start with threading https://review.opendev.org/c/openstack/nova/+/948311 | |
| 19:27:58 | opendevreview | Balazs Gibizer proposed openstack/nova master: Document native threading mode and tuneables https://review.opendev.org/c/openstack/nova/+/949364 | |
| 19:27:59 | opendevreview | Balazs Gibizer proposed openstack/nova master: Run nova-api and -metadata in threaded mode https://review.opendev.org/c/openstack/nova/+/951957 | |
| 19:27:59 | opendevreview | Balazs Gibizer proposed openstack/nova master: Run nova-next with n-sch in threading mode https://review.opendev.org/c/openstack/nova/+/948450 | |
| 19:28:00 | opendevreview | Balazs Gibizer proposed openstack/nova master: Allow to start unit test without eventlet https://review.opendev.org/c/openstack/nova/+/953436 | |
| 19:28:00 | opendevreview | Balazs Gibizer proposed openstack/nova master: Do not yield in threading mode https://review.opendev.org/c/openstack/nova/+/950994 | |
| 19:28:02 | opendevreview | Balazs Gibizer proposed openstack/nova master: [test]RPC using threading or eventlet selectively https://review.opendev.org/c/openstack/nova/+/953815 | |
| 19:28:02 | opendevreview | Balazs Gibizer proposed openstack/nova master: Run unit test with threading mode https://review.opendev.org/c/openstack/nova/+/953475 | |
| 19:28:04 | opendevreview | Balazs Gibizer proposed openstack/nova master: [vncproxy]Handle ssl.wrap_socket removal in py312 https://review.opendev.org/c/openstack/nova/+/955915 | |
| 19:28:04 | opendevreview | Balazs Gibizer proposed openstack/nova master: [CI]Make nova-tox-py312-threading voting https://review.opendev.org/c/openstack/nova/+/955791 | |
| 19:28:06 | opendevreview | Balazs Gibizer proposed openstack/nova master: Make libvirt Tpool proxying conditional https://review.opendev.org/c/openstack/nova/+/956090 | |
| 19:28:06 | opendevreview | Balazs Gibizer proposed openstack/nova master: Make RBD Tpool usage conditional https://review.opendev.org/c/openstack/nova/+/956089 | |
| 19:28:08 | opendevreview | Balazs Gibizer proposed openstack/nova master: [test]Further categorization of disabled unit tests https://review.opendev.org/c/openstack/nova/+/956092 | |
| 19:28:08 | opendevreview | Balazs Gibizer proposed openstack/nova master: Fix ProviderTree copying with threading Lock https://review.opendev.org/c/openstack/nova/+/956091 | |
| 19:28:10 | opendevreview | Balazs Gibizer proposed openstack/nova master: Warn on long task wait time for executor https://review.opendev.org/c/openstack/nova/+/952666 | |
| 19:28:34 | opendevreview | Balazs Gibizer proposed openstack/nova master: Allow services to start with threading https://review.opendev.org/c/openstack/nova/+/948311 | |
| 19:28:34 | opendevreview | Balazs Gibizer proposed openstack/nova master: Document native threading mode and tuneables https://review.opendev.org/c/openstack/nova/+/949364 | |
| 19:28:35 | opendevreview | Balazs Gibizer proposed openstack/nova master: Run nova-api and -metadata in threaded mode https://review.opendev.org/c/openstack/nova/+/951957 | |
| 19:28:35 | opendevreview | Balazs Gibizer proposed openstack/nova master: Run nova-next with n-sch in threading mode https://review.opendev.org/c/openstack/nova/+/948450 | |
| 19:28:36 | opendevreview | Balazs Gibizer proposed openstack/nova master: Allow to start unit test without eventlet https://review.opendev.org/c/openstack/nova/+/953436 | |
| 19:28:36 | opendevreview | Balazs Gibizer proposed openstack/nova master: Do not yield in threading mode https://review.opendev.org/c/openstack/nova/+/950994 | |
| 19:28:37 | opendevreview | Balazs Gibizer proposed openstack/nova master: [test]RPC using threading or eventlet selectively https://review.opendev.org/c/openstack/nova/+/953815 | |
| 19:28:37 | opendevreview | Balazs Gibizer proposed openstack/nova master: Run unit test with threading mode https://review.opendev.org/c/openstack/nova/+/953475 | |
| 19:28:38 | opendevreview | Balazs Gibizer proposed openstack/nova master: [vncproxy]Handle ssl.wrap_socket removal in py312 https://review.opendev.org/c/openstack/nova/+/955915 | |
| 19:28:38 | opendevreview | Balazs Gibizer proposed openstack/nova master: [CI]Make nova-tox-py312-threading voting https://review.opendev.org/c/openstack/nova/+/955791 | |
| 19:28:40 | opendevreview | Balazs Gibizer proposed openstack/nova master: Make libvirt Tpool proxying conditional https://review.opendev.org/c/openstack/nova/+/956090 | |
| 19:28:40 | opendevreview | Balazs Gibizer proposed openstack/nova master: Make RBD Tpool usage conditional https://review.opendev.org/c/openstack/nova/+/956089 | |
| 19:28:42 | opendevreview | Balazs Gibizer proposed openstack/nova master: [test]Further categorization of disabled unit tests https://review.opendev.org/c/openstack/nova/+/956092 | |
| 19:28:42 | opendevreview | Balazs Gibizer proposed openstack/nova master: Fix ProviderTree copying with threading Lock https://review.opendev.org/c/openstack/nova/+/956091 | |
| 19:28:44 | opendevreview | Balazs Gibizer proposed openstack/nova master: Warn on long task wait time for executor https://review.opendev.org/c/openstack/nova/+/952666 | |
| 19:29:41 | gibi | dansmith: sean-k-mooney: ^^ fixed the doc patch and rebased on recent master | |
| 19:31:02 | opendevreview | Ghanshyam proposed openstack/nova master: Fix doc comment on manager role change https://review.opendev.org/c/openstack/nova/+/956189 | |
| 19:33:30 | gmaan | sean-k-mooney: stephenfin ^^ fixing doc comments | |