| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-01-13 | |||
| 09:23:50 | lyarwood | gibi: there was a bug about this for paused instances iirc | |
| 09:24:06 | lyarwood | gibi: where we need to provide the VIR_DOMAIN_XML_INACTIVE flag https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainXMLFlags | |
| 09:27:20 | gibi | lyarwood: thanks, so we check the live config, thats good, then if the synch error came then we can simply check the live domain and it device is there then we can retry | |
| 09:29:19 | lyarwood | gibi: yeah I'd continue to retry on a direct sync error if the device is still there, VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED (but that should be a direct sync failure?) and a configurable timeout within n-cpu | |
| 09:29:44 | gibi | VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED is the failed event | |
| 09:29:51 | gibi | so tathat is async | |
| 09:30:37 | gibi | I can unify the retry if we get sync or async failure and the device is still in the live domain then we retry | |
| 09:30:48 | lyarwood | right sorry my point was that within libvirt at least it looks like that's only actually raised synchronously with the failure of the initial request to QEMU and that should bubble up directly to our call to libvirt | |
| 09:31:06 | lyarwood | yup cool that works | |
| 09:31:29 | lyarwood | I'm likely missing something in the libvirt code anyway regarding where VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED is being raised so that sounds like the best approach | |
| 09:32:10 | gibi | ack, thanks for the help | |
| 09:32:53 | gibi | if you get the libvirt timeout value for detach event then let me know and I will update the nova timeout value to be bigger | |
| 09:35:05 | lyarwood | kashyap: https://review.opendev.org/c/openstack/nova/+/770246 ; you might be interested in this, gibi is trying to rewrite our detach device logic in the libvirt driver to use events. I've made some comments in the change but if you have anymore context feel free to add it there. | |
| 09:35:34 | kashyap | lyarwood: Yeah, was just skimming the chat here. Was responding to something downstream that was breathing down my neck | |
| 09:35:41 | lyarwood | np | |
| 09:37:51 | lyarwood | switching topics, stephenfin how's your SQL/sqlalchemy foo? trying to work out if 1. the following is a valid query for a nova-status command and 2. if it would work in sqlalchemy. | |
| 09:37:58 | lyarwood | select distinct instances.uuid from instances left join instance_system_metadata on instances.uuid = instance_system_metadata.instance_uuid where instances.uuid not in (select instance_system_metadata.instance_uuid from instance_system_metadata where instance_system_metadata.key = 'hw_machine_type'); | |
| 09:38:23 | lyarwood | tl;dr I'm trying to list the instance uuids that *don't* have a `hw_machine_type` key set in instance_system_metadata | |
| 09:38:50 | lyarwood | and it has been waaaaaaaaaaaay too long since I wrote any SQL so this might be entirely wrong | |
| 09:40:13 | kashyap | gibi: Thx for taking up that; I just skimmed the patch. I'll look deeper; once I switch context. | |
| 09:41:29 | gibi | kashyap: thanks | |
| 09:41:30 | lyarwood | oh and that reminds me, sean-k-mooney, you know how you asked if we could stash image metadata properties in instance_system_metadata? Well they are already there. | |
| 09:42:01 | stephenfin | lyarwood: It's not my strongest skill, but that does look reasonable to me. I don't think the subquery is necessary, but the syntax I'm thinking of could be backend-specific | |
| 09:42:22 | lyarwood | sean-k-mooney: https://github.com/openstack/nova/blob/e6f5e814050a19d6f027037424556b2889514ec3/nova/objects/image_meta.py#L113-L127 | |
| 09:43:37 | lyarwood | stephenfin: yeah I couldn't work out the SQL to select instances.uuid where instance_system_metadata.key doesn't contain 'hw_machine_type' | |
| 09:45:05 | lyarwood | stephenfin: I'll convert this into sqla for now and go from there, thanks | |
| 09:47:30 | stephenfin | lyarwood: 0c441e636ba9d287909584b6ddf15eab5d479f0e would be good prior art also | |
| 09:48:24 | stephenfin | If not an exact match, at least it might help in terms of wiring up the machinery for an online migration | |
| 09:49:30 | lyarwood | stephenfin: I wasn't going to write an online migration for this | |
| 09:49:43 | lyarwood | stephenfin: this is something n-cpu will populate at startup | |
| 09:50:01 | lyarwood | stephenfin: and nova-status can warn about later prior to changing defaults | |
| 09:50:06 | stephenfin | ah, gotcha | |
| 09:50:13 | lyarwood | stephenfin: I don't see a query in that change FWIW | |
| 09:50:23 | lyarwood | well not a join etc | |
| 10:28:38 | openstackgerrit | YumengBao proposed openstack/os-traits master: add owner traits for accelerator resources https://review.opendev.org/c/openstack/os-traits/+/770569 | |
| 10:32:33 | openstackgerrit | Brin Zhang proposed openstack/nova master: Replaces tenant_id with project_id from List SG API https://review.opendev.org/c/openstack/nova/+/766726 | |
| 10:34:06 | gibi | brinzhang, alex_xu: responded in https://review.opendev.org/c/openstack/nova/+/729563 (finally) | |
| 10:34:47 | openstackgerrit | Kashyap Chamarthy proposed openstack/os-traits master: Add a trait for UEFI Secure Boot support https://review.opendev.org/c/openstack/os-traits/+/770570 | |
| 10:42:01 | openstackgerrit | Stephen Finucane proposed openstack/python-novaclient master: Add support for microversion v2.88 https://review.opendev.org/c/openstack/python-novaclient/+/770573 | |
| 10:49:35 | brinzhang | gibi: so we wont merge this patch, right? | |
| 11:46:55 | gibi | brinzhang: we need a separate bugfix, that is all what I said | |
| 11:48:09 | brinzhang | IMO, the bug fix shuold not prevent this patch go | |
| 11:48:30 | brinzhang | we shuold register a bugfix, and then fix it | |
| 11:51:58 | gibi | brinzhang: yepp, that works for me | |
| 11:53:23 | brinzhang | gibi: thanks, I hope we can make this patch merge, it's also meet alex_xu's meaning | |
| 12:26:35 | sean-k-mooney | lyarwood: i know the image metadata is in the instance_system_metadata table | |
| 12:26:52 | sean-k-mooney | lyarwood: thats why i wanted you to set the value there | |
| 12:27:08 | sean-k-mooney | they are just prefixed with img_ | |
| 12:27:50 | lyarwood | sean-k-mooney: ah I thought you were also suggesting that we dump all of the image metadata props in there as well | |
| 12:28:06 | lyarwood | sean-k-mooney: I just missed that they were there already, prefixed by image_ as you said | |
| 12:28:47 | sean-k-mooney | yeah so i was suggesting setting the effective values of all image props there instead of just the set values | |
| 12:29:12 | sean-k-mooney | e.g. if you dont have hw_vif_model set today it will normally default to virtio | |
| 12:29:40 | lyarwood | ah right | |
| 12:29:41 | sean-k-mooney | so we woudl store img_hw_vif_model=virtio | |
| 12:29:46 | sean-k-mooney | or whatever it is | |
| 12:29:58 | sean-k-mooney | as if it had been set | |
| 12:30:04 | lyarwood | FWIW I'm not overwriting image_hw_machine_type at the moment | |
| 12:30:18 | lyarwood | I'm just dumping it into hw_machine_type | |
| 12:30:43 | sean-k-mooney | ya you could do that the only issue with that approch is you have to now check both in the code | |
| 12:30:43 | lyarwood | image_hw_machine_type just remains on the original value | |
| 12:31:13 | sean-k-mooney | well if image_hw_machine_type was set tehn you would not be settting hw_machine_type | |
| 12:31:32 | sean-k-mooney | since you only need to set that if the machine type is not set in the image | |
| 12:31:59 | lyarwood | it's just copied from image_meta in that case | |
| 12:32:15 | sean-k-mooney | yep | |
| 12:32:26 | sean-k-mooney | i was trying to avoid having two sources of truth | |
| 12:32:37 | sean-k-mooney | e.g. image_hw_machine_type and hw_machine_type | |
| 12:33:10 | lyarwood | image_hw_machine_type is just the original, hw_machine_type is the single source of truth from now on | |
| 12:33:26 | lyarwood | as we can change it over time etc | |
| 12:33:37 | sean-k-mooney | well no we cant thats the point | |
| 12:33:44 | sean-k-mooney | if its set in the image it cant be changed | |
| 12:33:46 | lyarwood | you can through the versioned machine types | |
| 12:34:03 | sean-k-mooney | no if its set in the image thats it we dont use the config values at all | |
| 12:34:47 | lyarwood | why? moving forward through the versioned machine types provides a stable ABI etc | |
| 12:35:05 | sean-k-mooney | it would break backwards compatiablity with teh existing usage | |
| 12:35:13 | lyarwood | how? | |
| 12:35:17 | kashyap | Yes to what lyarwood said on versioned machine types | |
| 12:35:40 | kashyap | sean-k-mooney: When talking of this topic, a clear example would make sure we're not talking of different things. | |
| 12:35:42 | sean-k-mooney | the existing usage is that if yuou set a version machine type in the image metadata it will have that version for the lifetime of the instance | |
| 12:35:57 | sean-k-mooney | if you set the unversion on then it will use the latest version on the host it spawns on | |
| 12:36:18 | sean-k-mooney | we should not be changing that behavior in your spec | |
| 12:36:51 | lyarwood | the only part I'm changing is that instead of being for the lifetime of the instance operators can now update the versioned machien type | |
| 12:37:17 | lyarwood | aliases from the image would stay, I don't switch them out for the versioned machine types etc | |
| 12:37:34 | sean-k-mooney | lyarwood: that was not part of the spec | |
| 12:38:02 | sean-k-mooney | we did not provide any mechanium to update the machine type over the instacen lifttime | |
| 12:38:14 | sean-k-mooney | that is what the recreate api would provide | |
| 12:39:01 | lyarwood | that's between types, why would we ask users to rebuild for a version update? | |
| 12:39:08 | sean-k-mooney | operators could alwasy update the versioned machine type by updating the config for instance that dont have hw_machine_type set | |
| 12:39:12 | lyarwood | and I'm pretty sure it's in the spec | |
| 12:40:18 | sean-k-mooney | lyarwood: recreate with the same image and flavor was ment to just update the metadata its not the same as rebuild in that case | |
| 12:40:53 | lyarwood | sean-k-mooney: ah sorry you're talking about an API that doesn't exist :) | |
| 12:41:28 | sean-k-mooney | yes the part that was defered/rejected at the ptg meaning we had no aggreaded way to update the machine type | |
| 12:41:33 | lyarwood | sean-k-mooney: and I agree that would be nicer and would mean we wouldn't need a nova-manage command for this | |
| 12:43:04 | sean-k-mooney | ya so in the spec the only way to change the machine type is via the nova manage command | |
| 12:43:19 | openstackgerrit | Vlad Gusev proposed openstack/nova stable/victoria: Use subqueryload() instead of joinedload() for (system_)metadata https://review.opendev.org/c/openstack/nova/+/761809 | |
| 12:43:25 | sean-k-mooney | but the image metadata still has precidence | |
| 12:43:34 | sean-k-mooney | so it only matters for vms without that | |
| 12:51:12 | openstackgerrit | Brin Zhang proposed openstack/nova master: Replaces tenant_id with project_id from Flavor Access APIs https://review.opendev.org/c/openstack/nova/+/767704 | |
| 12:54:54 | brinzhang | bauzas: do you have time to check my response of your comments in https://review.opendev.org/c/openstack/nova/+/729563, there is -1 on it | |
| 12:55:42 | brinzhang | bauzas: may it will take your some time, but whatever thanks | |
| 13:31:10 | bauzas | brinzhang: ack, will look at your replies then | |