| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-14 | |||
| 16:00:01 | tssurya | but then again, is it ok for the task_state to still be None when we emit the state notification ? | |
| 16:00:01 | dansmith | the thing I think makes the biggest difference, is changing the power state before we know if we're going to do anything | |
| 16:00:07 | tssurya | start* | |
| 16:00:45 | dansmith | I don't know the semantics of any expectations around that | |
| 16:01:10 | tssurya | ok yea then maybe its fine :) | |
| 16:01:12 | dansmith | I didn't really think notifications are required to be realtime and synchronous, so I don't think it'd matter | |
| 16:01:35 | mriedem | tssurya: you can set the task_state on the instance in the compute manager before calling the driver method if we care - but then you have to handle UnexpectedTaskStateError there as well | |
| 16:01:49 | dansmith | but if we do that, | |
| 16:01:50 | dansmith | we | |
| 16:02:01 | dansmith | are changing it for non-ironic instances, | |
| 16:02:12 | tssurya | hmm yea | |
| 16:02:13 | dansmith | then catch the NotImplementedError, and then change it right back, yeah? | |
| 16:02:18 | mriedem | which won't actually power off or on the guest | |
| 16:02:33 | mriedem | @reverts_task_state would set the task_state back to None yes | |
| 16:03:08 | mriedem | but i'm not too worried about non-ironic instances because this is admin-only api stuff and if someone is abusing it for non-ironic instances and it doesn't work as expected....meh? | |
| 16:03:18 | dansmith | okay | |
| 16:03:45 | tssurya | so what't it going to be: manager or driver ? | |
| 16:03:55 | tssurya | at this point I don't mind either | |
| 16:04:12 | dansmith | "at this point" .. translation: "at this point I'm about to shoot both of you guys" | |
| 16:04:13 | mriedem | i forgot the question | |
| 16:04:26 | tssurya | dansmith: ;) | |
| 16:04:35 | dansmith | tssurya: I think it doesn't matter that much, figure it out when you move the code and it's probably okay | |
| 16:04:44 | tssurya | mriedem: meaning we move the task state and https://review.opendev.org/#/c/645611/12/nova/compute/api.py@250 | |
| 16:04:46 | dansmith | tssurya: also when are you actually leaving? | |
| 16:05:13 | mriedem | right now :) | |
| 16:05:32 | dansmith | lol | |
| 16:05:36 | dansmith | #ragequit | |
| 16:05:37 | mriedem | (11:04:47 AM) dansmith: tssurya: also when are you actually leaving? | |
| 16:05:38 | tssurya | sorry bad connection | |
| 16:05:49 | tssurya | I leave on Friday | |
| 16:05:53 | dansmith | okay | |
| 16:05:56 | tssurya | so I can work on it this evening and tomorrow | |
| 16:06:02 | dansmith | okay cool | |
| 16:06:24 | dansmith | tssurya: so I said I think manager vs. driver probably doesn't matter that much, you choose while moving the code and it'll probably be okay | |
| 16:06:37 | dansmith | I don't have a strong opinion without seeing it, so maybe just have to pick one and see | |
| 16:06:41 | tssurya | ok awesome thanks | |
| 16:06:48 | dansmith | mriedem: sound okay? | |
| 16:06:49 | tssurya | so first I'll try with the manager and then we can see | |
| 16:06:50 | mriedem | i'll leave a couple of comments | |
| 16:07:22 | tssurya | thanks a lot dansmith and mriedem :D | |
| 16:10:48 | mriedem | done https://review.opendev.org/#/c/645611/12 | |
| 16:13:10 | tssurya | ty | |
| 16:13:49 | dansmith | mriedem: yeah, so assuming your "don't do this for libvirt" assertion I think what you said in there is fine | |
| 16:13:49 | openstackgerrit | Merged openstack/nova stable/stein: Fix misuse of nova.objects.base.obj_equal_prims https://review.opendev.org/676289 | |
| 16:42:22 | alex_xu | dansmith: mriedem sean-k-mooney anyway thanks for your time today, it is indeed I use a lot today :) | |
| 16:43:17 | dansmith | alex_xu: it's okay, and I think sean-k-mooney has more than 24 hours in his days, so he has plenty to spare :) | |
| 16:43:59 | mriedem | no problem | |
| 16:44:39 | dansmith | mars has 25 hour long days, so maybe sean-k-mooney is from mars | |
| 16:45:05 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Hook resource_tracker to remove stale node information https://review.opendev.org/676461 | |
| 16:52:11 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: rt: only map compute node if we created it https://review.opendev.org/676463 | |
| 16:54:51 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: Hook resource_tracker to remove stale node information https://review.opendev.org/676461 | |
| 16:55:04 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/pike: rt: only map compute node if we created it https://review.opendev.org/676463 | |
| 17:00:52 | efried | dustinc: Ima rebase the sdk series and bump ksa/sdk requirements and get it tracking against this devstack patch https://review.opendev.org/#/c/676268/ | |
| 17:01:02 | efried | as soon as sdk 0.34.0 lands in u-c | |
| 17:01:36 | efried | when I say rebase - it's currently based on a really old nova commit, so I'm going to bring it up to current master. | |
| 17:05:03 | openstackgerrit | Matt Riedemann proposed openstack/nova stable/ocata: Hook resource_tracker to remove stale node information https://review.opendev.org/676467 | |
| 17:12:37 | rouk | is there a particular reason why OS-FLV-DISABLED:disabled is not exposed via api client? is it intended to be manual api queries only? | |
| 17:15:28 | rouk | i see an old proposed patch from... a long time ago, but it was abandoned. | |
| 17:18:43 | dansmith | efried: on this: | |
| 17:18:44 | dansmith | https://review.opendev.org/#/c/675705/2/nova/tests/functional/regressions/test_bug_1839560.py | |
| 17:19:20 | efried | yah | |
| 17:19:22 | dansmith | efried: you're just noting that in real life we have a service with hostname "host1" but would normally get uuids for the node names, is that right? | |
| 17:19:59 | efried | dansmith: It appears to me as though, in real life, we actually go and create a node for the host, and then delete it when it doesn't come back in the list of nodes. | |
| 17:20:31 | efried | and because that messes with the test, it was easier to name the host 'node1', so that we don't do that deletion. | |
| 17:20:51 | dansmith | we *don't* do that in real life though | |
| 17:21:02 | efried | well | |
| 17:21:20 | efried | what I did was pull down the patch, change node1 to host1 where it was talking about the host, and run the test. | |
| 17:21:31 | efried | and saw the message indicating that we deleted the host1 "node" | |
| 17:21:40 | dansmith | right, it's just an artifact of the test that we create a host and node from the same name | |
| 17:21:58 | dansmith | but in real life that's not how that works, | |
| 17:22:05 | efried | oh, it's start_service that's doing that? | |
| 17:22:06 | dansmith | it's just because of the test plumbing | |
| 17:22:27 | dansmith | no, I think it's because of the fake driver and the service which has already been started by the time we get to the test | |
| 17:22:41 | dansmith | well, | |
| 17:22:45 | dansmith | maybe not been started, | |
| 17:22:50 | dansmith | but because we're using the fake driver, | |
| 17:22:58 | dansmith | we've already run get_available_nodes() by the time start_service has returned | |
| 17:23:08 | dansmith | and that returns the same name as host.hostname or something | |
| 17:23:25 | dansmith | so when he then overrides the node list to be node1, node2, you'd get a deletion of the host1 node that was already created | |
| 17:24:18 | dansmith | anyway, I think your "I pulled down the patch" comment explains what I was asking, just that you were noticing test behavior and not concerned about relevance to real life or something else | |
| 17:25:28 | efried | I was concerned about relevance to real life. But if you're telling me it's just test harness noise, I'm okay with that. | |
| 17:25:35 | dansmith | ack | |
| 17:25:43 | efried | however, if it is test harness noise, I feel like we ought to be able to fix it. | |
| 17:25:50 | efried | more easily than if it's in the real code. | |
| 17:26:03 | dansmith | we could for sure (just made a comment to that effect) | |
| 17:26:07 | dansmith | I just don't think it's that important | |
| 17:26:26 | dansmith | normally having them match is handy | |
| 17:26:45 | efried | "normally" not-ironic | |
| 17:26:50 | efried | no doubt | |
| 17:27:24 | efried | but for ironic cases in general I would think it's never handy, n/a most of the time, but in certain cases like this one really confusing. | |
| 17:27:38 | dansmith | yes, normally not ironic | |
| 17:27:44 | dansmith | yep | |
| 17:27:58 | dansmith | well, yep to the first part, not the second | |
| 17:28:10 | dansmith | I don't really think it's confusing, but probably just because I know the details | |
| 17:28:28 | dansmith | the test isn't testing the ironic case, it's testing the bring-back-the-dead case, | |
| 17:28:46 | dansmith | which is related to how it works with ironic, but... not entirely ironic specific | |
| 17:28:49 | dansmith | ANYway | |
| 17:30:17 | efried | we could add to the fragility of FakeDriver's init_host by special-casing another CONF.host, say ironic-compute, and making self._nodes [] in that case. | |
| 18:19:09 | mriedem | efried: fwiw, | |
| 18:19:22 | mriedem | when we had a fake.set_nodes global i could have used that to avoid this problem, | |