| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-12-17 | |||
| 13:07:13 | gibi | no rebind as far as I see | |
| 13:07:14 | sean-k-mooney | if its just getting the info there is no depenciy on the agent being running | |
| 13:07:26 | sean-k-mooney | that info shoudl come form the db | |
| 13:07:35 | gibi | yeah, good point | |
| 13:07:46 | sean-k-mooney | if we however need the agent to create mdevs | |
| 13:07:51 | gibi | but I'm not familiar with cyborg internal arch | |
| 13:07:55 | sean-k-mooney | for exampel then we cant use GET | |
| 13:08:35 | sean-k-mooney | or at least if we did we would need to have cyborg reflect the status fo the binding as not active or complete | |
| 13:08:50 | Yumeng | I think we need use GET and also the agent to create mdevs | |
| 13:08:59 | sean-k-mooney | then we coudl poll or better wait for the externa event form the agent to signal cojmplete of its startup | |
| 13:09:07 | sean-k-mooney | like we do for spawn | |
| 13:09:34 | sean-k-mooney | Yumeng: did we reject or at lest strong advise agaisnt makeing GET magic in the cyborg api | |
| 13:10:09 | sean-k-mooney | where it would cause the agent to reporvison the attemnet/device if the host restarted | |
| 13:11:05 | sean-k-mooney | Yumeng: i though we were goign to reflect the status in the db and have the agent automaticaly create the devices on start up and signle that its not finised provioning in the api via a status field | |
| 13:11:44 | sean-k-mooney | gibi: that is why i tought we were doing the async event waiting by the way ^ | |
| 13:12:09 | gibi | ack | |
| 13:13:41 | sean-k-mooney | Yumeng: if we allow GET on the bindings to change state it basically means that cyborg cannot support the READONLY api personces as part of the keyston RBAC/policys effort | |
| 13:14:06 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP db: Add machine_type to instance extras https://review.opendev.org/c/openstack/nova/+/767531 | |
| 13:14:06 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP objects: Add machine_type to instance https://review.opendev.org/c/openstack/nova/+/767532 | |
| 13:14:07 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP libvirt: Record the machine_type of instances during init_host https://review.opendev.org/c/openstack/nova/+/767533 | |
| 13:14:11 | lyarwood | gibi / sean-k-mooney / stephenfin ; http://paste.openstack.org/show/801124/ ^ I'm getting this on instance.save() and can't for the life of me work out why, any ideas? I think I'm missing something basic in the db layer. | |
| 13:15:01 | Yumeng | sean-k-mooney: nope. it is not allowed to change sate in GET | |
| 13:15:02 | lyarwood | I *think* instance_update_and_get_original is trying to update machine_type in the instance db for some reason | |
| 13:15:24 | brinzhang_ | gibi, sean-k-mooney: we found this issue from you comments in vGPU support spec, depends on the "resume_guests_state_on_host_boot=True" config | |
| 13:15:37 | brinzhang_ | https://review.opendev.org/c/openstack/nova-specs/+/750116/9/specs/wallaby/approved/support-vGPU-nova-cyborg-interaction.rst#183 | |
| 13:16:03 | sean-k-mooney | lyarwood: ill take a look but we shoudl not have a machine_type field. it is ment to be img_machine_type in the instance_system_metadata table | |
| 13:16:57 | Yumeng | sean-k-mooney: "the agent automaticaly create the devices on start up" Does this mean cyborg will need another periodic task to sync arq in db with mdevs in the sys path? | |
| 13:16:58 | sean-k-mooney | so "mapper.column_attrs[key], value" looks wrong to me | |
| 13:16:59 | lyarwood | sean-k-mooney: the above series adds machine_type as a field to the instance_extras table etc | |
| 13:17:22 | lyarwood | sean-k-mooney: I've just missed something somewhere leading to this error when I set it in the instance object and try to save | |
| 13:17:48 | sean-k-mooney | lyarwood right but you shoudl not be doing that | |
| 13:18:03 | sean-k-mooney | lyarwood: wasnt the plan to have no db migraiton requireed for this | |
| 13:18:15 | sean-k-mooney | that is what we dicussed at the ptg | |
| 13:19:05 | lyarwood | I think you suggested adding this to system metadata but I then said it may as well go into instance extras | |
| 13:19:17 | lyarwood | it's in the spec as an instance extra | |
| 13:19:22 | openstackgerrit | Elod Illes proposed openstack/nova stable/stein: [stable-only] Cap bandit to 1.6.2 https://review.opendev.org/c/openstack/nova/+/766487 | |
| 13:19:36 | sean-k-mooney | ok i think that is not the right way to do this | |
| 13:19:52 | sean-k-mooney | at least not as a general pattern if we are adding more fileds | |
| 13:20:01 | sean-k-mooney | which is why it wanted it in system metadata to begin with | |
| 13:20:19 | sean-k-mooney | since we will likely want to do this again going forward when we change defaults | |
| 13:20:39 | lyarwood | what's the issue with this being an instance extra? | |
| 13:21:00 | sean-k-mooney | it will require a new column and db migration for every filed for not real value | |
| 13:21:41 | sean-k-mooney | if we use system metadata we dont require either | |
| 13:22:02 | sean-k-mooney | its also where the image metadata is currently sotred | |
| 13:22:28 | sean-k-mooney | so if we recored itn in img_machine_type we dont need to update any code that currently uses it | |
| 13:22:50 | sean-k-mooney | that way we dont miss anythign and it minimsies the code changes | |
| 13:22:51 | lyarwood | oh hell no, I'm not overloading img_machine_type | |
| 13:23:00 | sean-k-mooney | that was the whole point | |
| 13:23:53 | lyarwood | I didn't get that memo tbh, I've always wanted to track the machine type of the instance seperate to anything else | |
| 13:24:27 | lyarwood | so we can move it forward in the future without overwriting the image meta etc | |
| 13:24:30 | sean-k-mooney | we could use a different prefix in the system metadata | |
| 13:25:01 | sean-k-mooney | but im pretty stongly -1 on a new db column | |
| 13:25:44 | lyarwood | kk let me look into using it instead | |
| 13:26:00 | lyarwood | I honestly didn't think a new extra col for this would be a big deal | |
| 13:26:10 | lyarwood | given what's already in there | |
| 13:26:53 | sean-k-mooney | well given we want to evenually recored the value of all image properties and the machine_type propsoal was just a scoped down verion of that i think its a problematic design | |
| 13:27:30 | lyarwood | it wasn't a scoped down version of that at all | |
| 13:27:46 | lyarwood | I think you're crossing wires here tbh | |
| 13:27:49 | sean-k-mooney | it was ment to be | |
| 13:28:16 | lyarwood | it was always just about recording the machine type of existing instances | |
| 13:28:24 | sean-k-mooney | thats what i discussed with stephen when we were deciding if i or you would write the spec | |
| 13:28:55 | lyarwood | the image properties are part of it but not all instances will have the machine type set that way | |
| 13:29:15 | sean-k-mooney | yes i know | |
| 13:29:36 | lyarwood | this was always about capturing the currently used machine type and stashing it somewhere | |
| 13:29:43 | sean-k-mooney | yep | |
| 13:29:45 | lyarwood | allowing the config to change | |
| 13:29:58 | lyarwood | I didn't take from that the need to use the image properties | |
| 13:30:14 | lyarwood | I wasn't even planning to look at the image properties tbh | |
| 13:30:20 | lyarwood | Just the defined domains | |
| 13:30:41 | sean-k-mooney | well i had a very differnet plan for implementing it then | |
| 13:31:53 | sean-k-mooney | i will also point out form a downstream perspecitve we may need to backport this to 16.2 to support 17 upgrades | |
| 13:32:11 | sean-k-mooney | that would depend on how we handel the defualt machine type in 17/wallaby | |
| 13:32:34 | sean-k-mooney | its intended to be q35 for all new installs | |
| 13:32:47 | lyarwood | I wasn't planning on backporting this | |
| 13:32:49 | sean-k-mooney | for upgrade we need to ensure that the machine type does not change | |
| 13:32:55 | lyarwood | new installs would be q35 | |
| 13:33:01 | lyarwood | upgrades would be the old default | |
| 13:33:11 | lyarwood | until the env was fully upgraded | |
| 13:33:20 | lyarwood | and the original machine types stashed | |
| 13:33:24 | sean-k-mooney | ok but that means the ooo work is more involed but that is fine | |
| 13:33:40 | lyarwood | yeah it's a wrinkle we would need to document | |
| 13:33:48 | lyarwood | if we go with system metadata then I could backport it | |
| 13:34:01 | sean-k-mooney | yep | |
| 13:34:02 | lyarwood | so I'll take another look at that | |
| 13:41:12 | gibi | lyarwood, sean-k-mooney: I barely followed the machine type dicussion above, if your decide on a change in direction then please propose a spec update accordingly | |
| 13:42:02 | sean-k-mooney | ack im not sure it sa change in direction at elast form the ptg discussion | |
| 13:42:23 | sean-k-mooney | https://etherpad.opendev.org/p/nova-wallaby-ptg line 196 is the aggreed statement | |
| 13:42:30 | sean-k-mooney | *796 | |
| 13:43:02 | gibi | sean-k-mooney: anyhow, please keep the spec in sync with the impl : | |
| 13:43:03 | gibi | :) | |
| 13:43:15 | sean-k-mooney | but the main delta is lyarwood is going to look at using system metatdata table instead of a new column which would be a change in spec | |
| 13:43:34 | gibi | ack | |
| 13:44:24 | sean-k-mooney | i guess its tbd how easy that is but i think it shoudl be a smaller code change in general | |
| 14:08:41 | lyarwood | sean-k-mooney: sorry I can't recall why I dropped the generic record all the metadata props part of this | |
| 14:09:09 | sean-k-mooney | it was likely not in the downstream bz/dod | |
| 14:09:14 | lyarwood | sean-k-mooney: I think it might be due to that approach not capturing the config side | |
| 14:09:22 | lyarwood | sean-k-mooney: yeah or that | |
| 14:09:28 | sean-k-mooney | it was going to capture the config too | |
| 14:09:46 | sean-k-mooney | the idea was to sotre the config value in the imgage prerty filed if it was not set | |
| 14:09:57 | sean-k-mooney | but its fine to strat with just machien_type | |