| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-08-20 | |||
| 14:36:22 | tkajinam | at least that test passes in my env with OS_DEBUG=1 | |
| 14:36:53 | sean-k-mooney | if it passes in ci thats the main thing | |
| 14:36:59 | sean-k-mooney | ill test v18 now | |
| 14:37:27 | sean-k-mooney | cool all 8 pass under stestr | |
| 14:40:42 | sean-k-mooney | i didnt run all the funcitonal tests but all the libvirt ones pass locally | |
| 14:41:14 | sean-k-mooney | im goig to grab a drink but ill take a look at the current version when i get back | |
| 14:44:35 | tkajinam | sean-k-mooney, thanks... I've added a few comments to explain a few context behind current test coverage which hopefully helps that review... | |
| 15:31:12 | sean-k-mooney | some of those comments are incorrect. in general i think most of the real code changes look correct and the unit test coverage is fine | |
| 15:31:31 | sean-k-mooney | i think the functiontional coverage can be impvoed but its reasonably close | |
| 15:31:45 | sean-k-mooney | to what we woudl need to merge this | |
| 17:12:05 | opendevreview | Masahito Muroi proposed openstack/nova master: Use boot_roles_count and boot_role_ |
|
| 21:49:31 | opendevreview | melanie witt proposed openstack/nova master: Fix deepcopy usage for BlockDeviceMapping in get_root_info https://review.opendev.org/c/openstack/nova/+/920374 | |
| #openstack-nova - 2024-08-21 | |||
| 00:19:29 | mikal | sean-k-mooney: how do I delineate what is the object change vs everything else? For example for https://review.opendev.org/c/openstack/nova/+/924844 are we literally saying just pull the nova/objects/console_auth_token.py bit and associated tests out into its own change? | |
| 00:21:08 | sean-k-mooney[m] | yep more or less jut that and the test | |
| 00:21:45 | sean-k-mooney[m] | normally also the notificaotn object change | |
| 00:21:58 | sean-k-mooney[m] | but in this case you dont have a notifiction object change | |
| 00:23:33 | sean-k-mooney[m] | as i said this is really 3 patches in one. im not genrally against that but its an object change followed by the db change followed by the rest | |
| 01:19:23 | mikal | sean-k-mooney: I can give that a go... I'll start with the nova console change instead of the extra spec one I think though. Give me a day or two. | |
| 01:52:29 | opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Launch instances with stateless firmware https://review.opendev.org/c/openstack/nova/+/908890 | |
| 02:33:28 | opendevreview | Merged openstack/nova master: Fix deepcopy usage for BlockDeviceMapping in get_root_info https://review.opendev.org/c/openstack/nova/+/920374 | |
| 03:41:22 | opendevreview | Zhang Hua proposed openstack/nova stable/2024.1: Fix deepcopy usage for BlockDeviceMapping in get_root_info https://review.opendev.org/c/openstack/nova/+/926696 | |
| 03:44:02 | opendevreview | Zhang Hua proposed openstack/nova stable/2023.2: Fix deepcopy usage for BlockDeviceMapping in get_root_info https://review.opendev.org/c/openstack/nova/+/926697 | |
| 03:46:29 | opendevreview | Zhang Hua proposed openstack/nova stable/2023.1: Fix deepcopy usage for BlockDeviceMapping in get_root_info https://review.opendev.org/c/openstack/nova/+/926698 | |
| 06:16:51 | opendevreview | Masahito Muroi proposed openstack/nova master: Use boot_roles_count and boot_role_ |
|
| 06:46:13 | opendevreview | Amit Uniyal proposed openstack/nova master: Reproducer test for image property hw_architecture https://review.opendev.org/c/openstack/nova/+/926128 | |
| 06:46:14 | opendevreview | Amit Uniyal proposed openstack/nova master: Libvirt: updates resource provider trait list https://review.opendev.org/c/openstack/nova/+/926521 | |
| 06:46:14 | opendevreview | Amit Uniyal proposed openstack/nova master: VMware: updates resource provider trait list https://review.opendev.org/c/openstack/nova/+/926522 | |
| 06:46:15 | opendevreview | Amit Uniyal proposed openstack/nova master: zvm: updates resource provider trait list https://review.opendev.org/c/openstack/nova/+/926523 | |
| 06:46:15 | opendevreview | Amit Uniyal proposed openstack/nova master: WIP Ironic: updates resource provider trait list https://review.opendev.org/c/openstack/nova/+/926524 | |
| 08:06:48 | IPO | Hello all ! | |
| 08:11:52 | IPO | sean-k-mooney: Hello, Sean! What do you think: is there a chance to review and comment the fix: https://review.opendev.org/c/openstack/nova/+/923395 | |
| 10:00:32 | sean-k-mooney[m] | IPO: im kind of conflicted, i personally dont really think that feature should exist in nova | |
| 10:00:52 | sean-k-mooney[m] | the global version specifically | |
| 10:01:29 | sean-k-mooney[m] | because it allow you to probe for information you otherwise should not have. i.e. the hostname of other tenants vms | |
| 10:03:44 | sean-k-mooney[m] | the way the fix is proposed its also pretty expensive | |
| 10:04:48 | sean-k-mooney[m] | every instqance create would result in a db query to alll cells. | |
| 10:07:11 | IPO | sean-k-mooney: Thank you for comment. Interesting point. But in this case maybe the some way of variant, which was proposed earlier, should been implemented. But this will allow to check VMs name in one cell, but for all projects, which is not allowed either | |
| 10:09:32 | IPO | sean-k-mooney: And finally it looks like this feature maybe be cancelled at all as a potential security risk ? | |
| 10:11:19 | sean-k-mooney[m] | what use case made you interested in supporting this across cells? | |
| 10:17:27 | IPO | sean-k-mooney: Good question ! Basically we don't use cells in prods and some case of fix, implemented before (https://review.opendev.org/c/openstack/nova/+/860938) suits our needs. But there was the comment that we need fix to check it for all cells. Thant is why we start to impliment new fix | |
| 10:19:30 | sean-k-mooney[m] | thats not quite what i was asking. why to you enable project or global uniquness? | |
| 10:19:53 | sean-k-mooney[m] | the security issue is minor and its something that deployers opt into for there cloud | |
| 10:20:36 | sean-k-mooney[m] | thats why im not -1, the operator made a delibverate choice to opt in | |
| 10:20:57 | sean-k-mooney[m] | i could understand why this was useful for nova networks | |
| 10:21:10 | sean-k-mooney[m] | so that all vms had a unique fqdn | |
| 10:21:37 | sean-k-mooney[m] | but with neutron and designate im not sure what the usecase for global or project uniqness is | |
| 10:25:58 | sean-k-mooney[m] | IPO: if other cores think this should be supported and fixed then thats fine we can proceed with the patch its just not something i personally think we should spend time on but if you have a usecase that needs it i dont want to block that | |
| 10:26:30 | sean-k-mooney[m] | i just dont really want to invest much of my time in advocating for it when i dont really belive in it | |
| 10:34:23 | IPO | sean-k-mooney: thank you for extra comments. I have clear view of your point of view about this issue. One of reasons we faced - we have naming convention for VMs. It is maintained by our self-made portal. But there is extra ability to work with cloud - via CLI or API and we can't control names users can choose for VMs | |
| 10:42:02 | IPO | sean-k-mooney: I will try to collect additional reasons with our team, to have more items in list. | |
| 11:05:08 | IPO | sean-k-mooney: Thank you, Sean! I will return to this topic as soon as will have additional info. Have a good day ! | |
| 11:05:29 | sean-k-mooney | ok | |
| 11:05:58 | sean-k-mooney | as i said its just not a priority for me but if others are happy to proceed then im ok with that | |
| 11:12:21 | IPO | sean-k-mooney: ok, I see, thank you for your time. | |
| 11:55:19 | opendevreview | Jan Gutter proposed openstack/placement master: DNM -- testing fix in external PR https://review.opendev.org/c/openstack/placement/+/926743 | |
| 12:02:16 | sean-k-mooney | gmann: melwitt: https://review.opendev.org/c/openstack/nova/+/925163 is a fairly simple review if ye have time later | |
| 12:18:14 | opendevreview | Jan Gutter proposed openstack/placement master: DNM -- testing fix in external PR https://review.opendev.org/c/openstack/placement/+/926743 | |
| 13:52:34 | opendevreview | Jan Gutter proposed openstack/placement master: DNM -- testing fix in external PR https://review.opendev.org/c/openstack/placement/+/926743 | |
| 15:30:08 | stephenfin | Just for lolz, I attempted to create a BDM with boot_index=0 and destination_type=local (i.e. the root disk). Nova ignores/overwrites it. Is the fact that it accepts it in the first place a bug in our validation code? | |
| 15:30:12 | stephenfin | e.g. https://paste.openstack.org/show/bFOXilnynfyoGM2CCp7v/ | |
| 15:30:30 | stephenfin | (that's the output from my hacked OSC) | |
| 15:41:53 | opendevreview | Merged openstack/nova stable/2023.2: Disconnecting volume from the compute host https://review.opendev.org/c/openstack/nova/+/911396 | |
| 15:55:10 | melwitt | sean-k-mooney: fyi in case you missed it, I updated https://review.opendev.org/q/topic:%22bp/unified-limits-nova-unset-limits%22+is:open+project:openstack/nova after we chatted about [workarounds] and a release note | |
| 15:58:19 | sean-k-mooney | stephenfin: i dont think that is a bug | |
| 15:58:28 | sean-k-mooney | the bdm are not just for cinder | |
| 15:59:01 | sean-k-mooney | you use them ot create multiple ephermaal disk, swap disk and for the root disk | |
| 15:59:42 | stephenfin | Right, I know all that (that's what 'destination_type' is there for, after all). My point is that the nova-api/nova-compute ignores what the user is giving rather than spitting it back at them | |
| 16:00:03 | sean-k-mooney | right so the api is corect to accept it | |
| 16:00:09 | sean-k-mooney | the comptue proably has a bug | |
| 16:00:30 | sean-k-mooney | or there is a bug in how we are merging the bdm and the image | |
| 16:00:44 | stephenfin | Interesting. Are you suggestion I should be able to override $flavor.disk with a BDM? | |
| 16:00:50 | melwitt | yeah, I checked and the api-ref example actually shows boot_index=0 (https://docs.openstack.org/api-ref/compute/#id11) however I think what stephenfin is saying is nova isn't taking the disk size from the BDM and is instead using what's in the flavor in the local case? | |
| 16:00:52 | stephenfin | s/suggestion/suggesting/ | |
| 16:01:04 | stephenfin | melwitt: Yes, thanks Mel :) | |
| 16:01:30 | sean-k-mooney | im sayiung that passing image_ref and also providing bdms for the root disk is proably undfiend right now | |
| 16:01:51 | sean-k-mooney | im incined to say that that might be something to reject with a 400 | |
| 16:02:01 | melwitt | seems like a nova bug although if flavor and bdm are in conflict then I guess maybe it should reject it saying conflict because we can't assume which one they really wanted | |
| 16:02:07 | stephenfin | Not so. If I pass destination_type=volume instead of destination_type=local, things work just fine (that's a BFV instance) | |
| 16:02:24 | sean-k-mooney | right | |
| 16:02:35 | sean-k-mooney | but im not sure you should be passsing an image uuid | |
| 16:02:59 | sean-k-mooney | and bdms with srouce type image and a uuid | |
| 16:04:08 | stephenfin | Yeah, fair point. I'm thinking about OSC but from a pure API level, you'd set imageRef to nll and set uuid in the relevant BDM instead | |
| 16:04:16 | sean-k-mooney | https://paste.openstack.org/show/bnHpudm9QI1H7gj7aVAm/ | |
| 16:04:22 | sean-k-mooney | that a pretty print of the payload | |
| 16:04:45 | sean-k-mooney | stephenfin: so what exactly happens in this case | |
| 16:05:04 | sean-k-mooney | i think the result should be the same since its the same uuid | |
| 16:05:30 | sean-k-mooney | are you expeciting the size to be 6 and your getting the flavor size | |
| 16:06:11 | sean-k-mooney | "destination_type": "local", is really intended only for the addtional ephmeral disks and volume | |
| 16:06:18 | sean-k-mooney | *volume_size | |
| 16:06:25 | sean-k-mooney | in that case is for when you want to have multiple | |
| 16:06:42 | stephenfin | just checking now | |
| 16:06:50 | sean-k-mooney | destination_type=volume volume_size is obvioulsy the cinder volume size | |
| 16:07:52 | melwitt | I guess if we consider how it works for volume (bdm disk size takes precedence) maybe we should do that with local too? rather than it being treated as invalid | |
| 16:08:23 | sean-k-mooney | yep i would expect the bdm to have precidence but it shoudl not be able to exceed flavor.disk_gb | |
| 16:08:30 | melwitt | that would make it consistent. I assume that flavor disk size is ignored for BFV | |
| 16:08:31 | sean-k-mooney | for the non bfv case | |
| 16:08:47 | sean-k-mooney | melwitt: yes and no | |
| 16:08:54 | sean-k-mooney | it mostly is | |
| 16:09:02 | stephenfin | sean-k-mooney: so the answer is it depends :) | |
| 16:09:04 | sean-k-mooney | you shoudl set it to 0 for bfv flavors | |