| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-11 | |||
| 13:50:34 | alex_xu | mriedem: ah, i see now, that problem is out of the scope https://review.openstack.org/#/c/569498/ | |
| 13:51:09 | mriedem | yup | |
| 13:51:17 | mriedem | i need to review that rebuild + image traits change too | |
| 13:51:28 | mriedem | since i'm the one that got it stuck in committee during spec review | |
| 13:52:34 | mriedem | dansmith: so you don't see a need to name the new fields in the server group API policy_name and policy_rules right? | |
| 13:52:39 | alex_xu | mriedem: yea | |
| 13:52:55 | dansmith | mriedem: I don't, but gmann said he thought it was important | |
| 13:52:55 | mriedem | because yikun re-wrote the api change to do that, which makes it inconsistent with both the notification payload and the internal object modeling | |
| 13:53:06 | mriedem | i don't think it's important or really confusing | |
| 13:53:11 | dansmith | me either | |
| 13:53:18 | mriedem | i think it's more important that we have consistency up and down the stack | |
| 13:53:20 | mriedem | internal and external | |
| 13:57:26 | openstackgerrit | Eric Fried proposed openstack/nova master: Delete orphan compute nodes before updating resources https://review.openstack.org/579922 | |
| 14:03:22 | mriedem | gibi: just ping me if / when you need reviews on the bw provider changes | |
| 14:12:33 | openstackgerrit | Merged openstack/nova master: Remove irrelevant comment https://review.openstack.org/578821 | |
| 14:16:11 | openstackgerrit | Surya Seetharaman proposed openstack/nova master: Update queued-for-delete from the ComputeAPI during deletion/restoration https://review.openstack.org/566813 | |
| 14:33:00 | gibi | mriedem: thanks. | |
| 14:34:42 | efried | alex_xu: Are you still -1 on that change? Given that we've concluded NoValidHost is the right thing, having those logs be ERROR seems appropriate, 对吧 | |
| 14:37:30 | mriedem | efried: we don't ERROR for NoValidHost in the scheduler | |
| 14:37:41 | mriedem | ERROR means the operator needs to investigate b/c there is a problem in the system, | |
| 14:37:48 | mriedem | in this case, it's really a user error like a 400 | |
| 14:37:51 | mriedem | we don't log errors for that | |
| 14:37:56 | mriedem | debug at most | |
| 14:38:34 | efried | mriedem: So in this case, how would the user know what went wrong? | |
| 14:39:14 | mriedem | do they get a fault recorded? | |
| 14:39:55 | mriedem | set_vm_state_and_notify will record a fault on the instance for the NoValidHost, | |
| 14:40:19 | mriedem | and the user should be able to see the fault message for the error | |
| 14:40:57 | mriedem | https://developer.openstack.org/api-guide/compute/faults.html#instance-faults | |
| 14:41:27 | mriedem | which reminds me, you can only see faults for ERROR or DELETED servers, so that's another reason to set the server to ERROR in this case so they can see the fault | |
| 14:41:56 | mriedem | so as long as the NoValidHost has a reasonable message we should be fine | |
| 14:42:17 | mriedem | that would be a good wrinkle for the functional test though right? | |
| 14:43:06 | openstackgerrit | do3meli proposed openstack/nova master: docs: add nova host-evacuate command to evacuate documentation https://review.openstack.org/578040 | |
| 14:44:08 | mriedem | why is all this shared provider stuff in this change? https://review.openstack.org/#/c/569498/12/nova/tests/functional/libvirt/test_shared_resource_provider.py | |
| 14:44:49 | efried | mriedem: I asked for it. | |
| 14:45:14 | efried | mriedem: Because it's important that we're checking all and only providers involved in the allocation. | |
| 14:45:39 | efried | mriedem: So we should also be checking nested, but letting that slide for now because we don't have any scenarios that actually use nested yet. | |
| 14:46:33 | mriedem | hmm, ok. | |
| 14:46:41 | mriedem | and those unit tests look to be completely redundant for conductor | |
| 14:46:47 | mriedem | redundant with the functional tests | |
| 14:46:51 | mriedem | we don't need both for the same scenarios | |
| 14:47:17 | mriedem | https://review.openstack.org/#/c/569498/12/nova/tests/functional/libvirt/test_shared_resource_provider.py is just a really weird location for rebuild tests... | |
| 14:48:28 | efried | if it's testing how shared resource providers are handled by rebuild by libvirt, it seems as good as anything. | |
| 14:49:34 | mriedem | the validation being added to conductor is virt-agnostic | |
| 14:49:42 | mriedem | so it's a weird place to have those tests | |
| 14:49:43 | mriedem | is all | |
| 14:50:17 | efried | but libvirt is the only driver that's doing shared providers (properly) at the moment. | |
| 14:50:31 | mriedem | yeah i know, but that doesn't really matter here does it? | |
| 14:50:39 | mriedem | it's just a convenient place to put the tests given the setUp | |
| 14:51:01 | mriedem | we could have just as easily written a functional test that populate providers and allocations in placement and then run rebuild | |
| 14:51:07 | mriedem | anyway, i'm not -1 on it, it's just weird | |
| 14:51:23 | efried | Agree the main motivator was probably convenience as you say. | |
| 14:57:49 | mriedem | i'd like to nuke all of these checks http://codesearch.openstack.org/?q=We%20need%20to%20mock%20that%20the%20old%20way&i=nope&files=&repos= | |
| 14:57:52 | mriedem | seems we can do that now right? | |
| 14:58:16 | mriedem | oh nvm, we haven't dropped the migrate_instances_add_request_spec online data migration yet | |
| 14:59:17 | mriedem | dansmith: it's probably not very possible to do a blocker migration for the request spec migration is it? | |
| 14:59:31 | mriedem | since it's api db + multi-cell | |
| 14:59:46 | mriedem | we could add a nova-status check pretty easily | |
| 14:59:59 | mriedem | iterate all cells and make sure all non-deleted instances have a request spec | |
| 15:00:00 | dansmith | for which migration? | |
| 15:00:04 | dansmith | ah | |
| 15:00:05 | dansmith | yeah | |
| 15:00:07 | mriedem | migrate_instances_add_request_spec | |
| 15:00:21 | mriedem | yeah - i want to kill all of these "if not request_spec: populate from garbage" in the code | |
| 15:00:34 | dansmith | hah yeah | |
| 15:00:42 | mriedem | we can't do a blocker db migration in db sync | |
| 15:00:54 | mriedem | well, shouldn't | |
| 15:00:55 | mriedem | ? | |
| 15:01:08 | mriedem | but nova-status could do the job, i mean this migration has been around since newton | |
| 15:01:13 | dansmith | I think people have also expressed displeasure with the blocker migrations, so any time we're not enforcing some really structural thing (like a column being non-null or something) a nova-status would be better | |
| 15:01:15 | mriedem | so nova-status in rocky and drop in stein | |
| 15:01:34 | mriedem | drop compat in stein i mean | |
| 15:01:47 | dansmith | aye | |
| 15:02:23 | openstackgerrit | Chris Dent proposed openstack/nova master: Add placement.concurrent_udpate to generation pre-checks https://review.openstack.org/581771 | |
| 15:20:20 | tssurya | mriedem: I was trying to understand the marker logic you have in _heal_instances_in_cell (want to do something similar for the migration tool for updating queued_for_delete), wanted to confirm something: every time the command is run it will loop through all the instances which might have been processed before as well right ? like there is no way to check and skip the ones we have already covered. All we can ensure is each time it does m | |
| 15:21:50 | openstackgerrit | Matt Riedemann proposed openstack/nova-specs master: Fix nits in the handling down cell spec https://review.openstack.org/581243 | |
| 15:22:34 | mriedem | tssurya: heal_allocations doesn't use a global marker like map_instances | |
| 15:22:46 | mriedem | so yes today it will hit all instances, | |
| 15:22:50 | tssurya | mriedem: hmm yea | |
| 15:23:07 | mriedem | it's only using a per-cell marker while iterating instances in the cell when we have a limit specified (default of 50) | |
| 15:23:09 | tssurya | because I have two online migration tools which are like this needing a marker logic | |
| 15:23:24 | mriedem | migrate_instances_add_request_spec uses a global marker | |
| 15:24:10 | mriedem | so i'd follow that pattern | |
| 15:25:19 | mriedem | map_instances is slightly different | |
| 15:25:33 | mriedem | it uses a sentinel project_id and munges the last real instance mapping uuid | |
| 15:26:00 | tssurya | yea that I am aware , but I don't want to insert a new row and all | |
| 15:26:35 | mriedem | you're going to have to either way | |
| 15:27:07 | mriedem | otherwise how would you find the marker record between runs of the command? | |
| 15:27:39 | tssurya | ah just looked at migrate_instances_add_request_spec; it is using the same logic | |
| 15:27:43 | tssurya | with a FAKE_UUID | |
| 15:27:53 | mriedem | yes | |
| 15:28:00 | tssurya | what if more than 1 tool has this FAKE_UUID ? | |
| 15:28:17 | mriedem | i don't care about out of tree tools | |
| 15:28:47 | mriedem | the alternative is you do like map_instances and use a fake project_id and munge the uuid from an existing mapping | |
| 15:29:04 | mriedem | but some people hate that too | |
| 15:29:08 | mriedem | since it's not a real uuid | |
| 15:29:42 | tssurya | uh-huh yea, well I guess it might be better than starting a series of FAKE_UUIDs | |
| 15:30:26 | mriedem | the sentinel uuid is always the same 00000000-0000-0000-0000-000000000000 | |
| 15:30:47 | mriedem | we don't have anything else in nova, as far as i know, that inserts instance mappings with that uuid | |
| 15:31:30 | tssurya | oh okay | |
| 15:31:35 | mriedem | between migrate_instances_add_request_spec and map_instances i'd just personally follow what was done in migrate_instances_add_request_spec | |