| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-08-20 | |||
| 13:45:11 | tkajinam | sean-k-mooney, I'm fixing it now. However the current blocker appears in the rescue API call so that line is not related | |
| 13:45:32 | opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Launch instances with stateless firmware https://review.opendev.org/c/openstack/nova/+/908890 | |
| 13:46:29 | sean-k-mooney | ok so rescue is your main consern https://review.opendev.org/c/openstack/nova/+/908890/16/nova/tests/functional/libvirt/test_stateless_firmware.py#208 | |
| 13:46:40 | sean-k-mooney | ill quickly pull that review and see what happens locally | |
| 14:09:30 | sean-k-mooney | tkajinam: so rescue is failing beucase the test is tryign to open the unrescue_xml_path i.e. its tryign to write to disk | |
| 14:09:53 | sean-k-mooney | tkajinam: so there is a missing mock somewhere | |
| 14:10:02 | sean-k-mooney | https://paste.opendev.org/show/bCsw4SkPFosMTi0pL4yd/ | |
| 14:13:21 | tkajinam | I was wondering why that instance directory is not created but it might be because fake libvirt driver is used. hmm... there are no rescue functional tests with fake libvirt driver now so I may need to look into additional mocking to make the process pass | |
| 14:14:48 | sean-k-mooney | there are https://github.com/openstack/nova/blob/0b091179d575a5e7cff9cd76223a8016cfbbc019/nova/tests/functional/libvirt/test_rescue_deleted_base.py | |
| 14:15:10 | sean-k-mooney | but but its doing https://github.com/openstack/nova/blob/0b091179d575a5e7cff9cd76223a8016cfbbc019/nova/tests/functional/libvirt/test_rescue_deleted_base.py#L48-L57 | |
| 14:15:20 | tkajinam | ahhh | |
| 14:17:31 | sean-k-mooney | if i add that | |
| 14:17:47 | sean-k-mooney | then it fails with https://paste.opendev.org/show/bLajZLL1mJueSgMkPNAQ/ | |
| 14:18:00 | sean-k-mooney | but that might just be an issue with the assert | |
| 14:18:29 | tkajinam | yeah. rescue uses the image without stateless firmware so that assertion is wrong | |
| 14:19:08 | sean-k-mooney | ok well i trust you can figure out how to fix that | |
| 14:19:19 | sean-k-mooney | ill quickly look at why the live migration test is failing | |
| 14:19:50 | sean-k-mooney | TypeError: Can't upgrade a READER transaction to a WRITER mid-transaction | |
| 14:20:02 | sean-k-mooney | that od | |
| 14:21:03 | sean-k-mooney | i feel like you just missing the migration stub or soemthign liek that | |
| 14:21:37 | sean-k-mooney | no you have the corret mixin | |
| 14:24:03 | opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Launch instances with stateless firmware https://review.opendev.org/c/openstack/nova/+/908890 | |
| 14:25:25 | tkajinam | sean-k-mooney, sorry but where did you find that type error ? | |
| 14:26:44 | tkajinam | the remaining problem with live migration tests is that the xml content generated by fake libvirt driver does not contain any loader related fields and it's not possible to assert that these fields are passed in destination_xml. https://github.com/openstack/nova/blob/0b091179d575a5e7cff9cd76223a8016cfbbc019/nova/tests/fixtures/libvirt.py#L1565-L1568 | |
| 14:27:10 | sean-k-mooney | im running it in an IDE but you can get it to print by adding OS_DEBUG=1 to your tox command | |
| 14:27:45 | tkajinam | I can technically extend the implementation to simulate more detailed behavior but I'm inclined to look into whitebox tests instead of importing more libvirt's behaviors into that fixture | |
| 14:27:48 | tkajinam | ah, ok | |
| 14:30:56 | sean-k-mooney | oh weird | |
| 14:31:03 | sean-k-mooney | ok it passes if i run it with stestr | |
| 14:31:41 | sean-k-mooney | i.e. OS_DEBUG=1 stestr --test-path=./nova/tests/functional run -- LibvirtStatelessFirmwareTest.test_live_migrate_server | |
| 14:34:38 | opendevreview | Takashi Kajinami proposed openstack/nova master: libvirt: Launch instances with stateless firmware https://review.opendev.org/c/openstack/nova/+/908890 | |
| 14:35:33 | sean-k-mooney | so that test only fails when i run it form my ide which might be related to how im runing it since im not using stestr and i kind of hacked it to work | |
| 14:36:12 | tkajinam | yeah | |
| 14:36:13 | tkajinam | https://paste.opendev.org/show/bsFDSlO0e6ZaLcsTFX2g/ | |
| 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 | |