Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2025-09-30
17:00:45 lajoskatona Uggla: Hi, just a quick note that I slowly progress with the migration to SDK from neutronclient hell: https://review.opendev.org/q/topic:%22sdk_for_neutron%22 , after we are over the release week we can discuss how should it be consumed the easiest way :-)
17:00:58 sean-k-mooney its not actully a bug requriing nova code change really beyond bindeps.txt to just document the relevent dep
17:01:45 sean-k-mooney lajoskatona: ya you have broken it up that helps
17:02:18 sean-k-mooney i likmed the orginal patch and saw you were updating it but i have not looked at it in any detial
17:02:34 sean-k-mooney s/likmed/skimed/
17:04:43 sean-k-mooney lajoskatona: we have a related topic for the wtacher ptg about how/wen to start the convertion tothe sdk there. there is a depency on the neutron client in watcher too https://github.com/openstack/watcher/blob/master/requirements.txt#L38
17:05:28 sean-k-mooney but really not much of one today so i was sugesting we start with movaing the novaclinet and perhaps other over to the sdk next cycle
17:06:09 Uggla sean-k-mooney, elodilles I will look at above later.
17:09:11 lajoskatona sean-k-mooney: The thing that I run into and have to think about is that in n-client there are plenty of exotic exceptions like overquota and similar, but SDK has only basic exceptions like conflict
17:10:09 lajoskatona at the moment I am not sure if using the simple conflict, notfound etc is enough or there is a need for cover the neutronclient exotic exceptions in SDK for example
17:11:57 sean-k-mooney lajoskatona: ah so we may need to add some more detailed excption n nova for those diffent usecases to make the rest of the logic continue to work the same
17:12:31 sean-k-mooney and your tryign to understand if that is the correct approch or if the nova code should be adapted to the geneirc expctions?
17:12:46 sean-k-mooney it that the crux of the disucssion you want to have
17:13:24 sean-k-mooney i.e. adtapt the generic excption to specific ones locally at the callsite or not?
17:16:20 sean-k-mooney lajoskatona: for the most part as you have seen here https://review.opendev.org/c/openstack/nova/+/928022/12/nova/network/neutron.py#2933 we have tired to convert the neutron client excption into nova specific ones rather then propagate them outside ot the nova.network.neutron module
17:16:56 sean-k-mooney so ideally code outside of nova/network/neutron.py shoudl not need to deal with sdk or neutron client excptions
17:17:08 sean-k-mooney lajoskatona: alhtough i dont know if that is actully what you are asking
17:19:11 lajoskatona sean-k-mooney: yes that is my problem now, I think the fresh patch for FIPs has some of these cases
17:19:14 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/962604/1/nova/network/neutron.py#b2949
17:19:20 sean-k-mooney this is perhaps a good example
17:19:56 sean-k-mooney before if it was not a client excption we loged the ecetpion and reaised
17:20:37 lajoskatona or some real exotic ones: https://review.opendev.org/c/openstack/nova/+/962604/1/nova/network/neutron.py#3100 :-)
17:20:42 sean-k-mooney we still proably shoudl actch other clent excptions and log them and then raise them again
17:21:06 sean-k-mooney ah
17:21:17 sean-k-mooney except (neutron_client_exc.IpAddressGenerationFailureClient, neutron_client_exc.ExternalIpAddressExhaustedClient
17:21:30 lajoskatona SDK has general httpException, that can be used for such
17:22:35 opendevreview Takashi Kajinami proposed openstack/nova master: AMD SEV: omit iommu='on' for virtio devices https://review.opendev.org/c/openstack/nova/+/909635
17:22:38 sean-k-mooney so neutron_client_exc.OverQuotaClient apars ot be the oen that is missing
17:22:55 sean-k-mooney right we are nologner rasign FloatingIpLimitExceeded
17:23:35 sean-k-mooney so for partity we would need to disambiuage the sdk_exc.ConflictException
17:23:57 sean-k-mooney and look at the message to know if its a quota issue or if we are out of ips
17:25:26 sean-k-mooney i think in both case we would expect the allocation to fail and we would want to return a 409
17:25:47 sean-k-mooney so merging those together while it makes debuging hard does not seam to alter the api behavior
17:26:48 sean-k-mooney lajoskatona: are you wonderign if nova shoudl do that disambiguation ro if it shoudl be in the sdk
17:27:08 sean-k-mooney or are you wonderign if its required to treat them differently vs merging them into a singel expction
17:27:14 lajoskatona sean-k-mooney: +1, I check it tomorrow to look inside the message
17:27:57 lajoskatona sean-k-mooney: My first idea was to merge them
17:28:19 sean-k-mooney i think as long as the end result at the api/externally does nto change i.e. 409
17:28:28 sean-k-mooney adn we have somethign reasonable in the debug logs
17:28:36 lajoskatona sean-k-mooney: for tomorrow I will have more test results from zuul, so I will see if this aproach can work :-)
17:28:37 sean-k-mooney so we can tell which it actully was its proably oke to merge them
17:28:46 sean-k-mooney ack
17:29:22 sean-k-mooney so for th ecase like this you could consider doing the merge in a seperate refactor first
17:29:54 sean-k-mooney then the adopt sdk patch is just doing that
17:30:17 sean-k-mooney i have not reviewd this in detail but in most cases i tlooks like you have found 1:1 mappings
17:30:37 sean-k-mooney but sure let me know what you find after zuul reports
17:31:17 sean-k-mooney lajoskatona: as an aside i think https://github.com/openstack/watcher/blob/master/watcher/common/nova_helper.py#L729-L744 is the only useage of neutronclient in watcher
17:31:38 sean-k-mooney and i dont think we actully need that info really any more
17:31:47 sean-k-mooney its part of the create_instance helper https://github.com/openstack/watcher/blob/master/watcher/common/nova_helper.py#L641
17:31:56 sean-k-mooney and that on my todolist to see fi we can entrily delete
17:32:40 sean-k-mooney lajoskatona: so if nothing else im goign to see if we can remove the neutronclient dep entirly one way or another in 2026.1
17:34:04 sean-k-mooney watcher used ot have its onw impleation of evacuate/coldmigrate where it woudl snapshot a vm, delete it and create a new one
17:34:33 sean-k-mooney i belive this is mostly dead code now
17:41:15 lajoskatona sean-k-mooney: thanks I add watcher to my todolist, I have anyway some pending patches waiting for the release to not bother people (like in manila-ui)
18:29:07 melwitt dansmith, bauzas: just some fyi, I have been realizing some issues while working on tempest tests for this series. I have added a few comments about it on the spec re-proposal here https://review.opendev.org/c/openstack/nova-specs/+/961564/1/specs/2026.1/approved/vtpm-live-migration.rst
18:30:15 melwitt (about [libvirt]supported_tpm_secret_security valid values and overloading of the image_hw_tpm_secret_security instance sysmeta key)
18:30:22 dansmith melwitt: I mostly gave up reviewing the spec re-proposal because it isn't easy to see the changes. So I've just been using the commit message to remind myself of the changes while looking at the code
18:30:26 dansmith I figure it's better to spend my time on the code anyway
18:31:56 melwitt dansmith: that's a reasonable approach, that was my thinking in enumerating the changes in the commit message. just saying I have noticed some things while doing tempest stuff and thought that might be the best place to note my findings
18:32:12 dansmith ack
18:33:56 melwitt it's all about the "opt in via resize" part of it. I have been writing tempest tests that do resize-confirm, resize-revert, and now I'm doing a rebuild-with-image-property one to see what happens
18:34:24 melwitt I am unsure how comprehensive we will want to be with the ability for a user to change their TPM secret security policy
18:48:24 melwitt dansmith: do you think that we should support a user _changing_ their secret security policy via a resize? or do you think the resize should be limited to legacy instances that don't have a policy set? because the former becomes a problem with us overloading the sysmeta key for it
18:49:30 melwitt *the image property sysmeta key for it
18:56:13 sean-k-mooney melwitt: interesting i had assumed the later that you would be able change via resize
18:57:46 melwitt sean-k-mooney: you mean the former I think :) but yes I also went in with that assumption but I want to pause and make sure that's what we want, bc if we want that, I think we need to not use "image_hw_tpm_secret_security" as the key to stash in the instance sysmeta. bc it makes flavor/resize conflict errors
18:58:37 melwitt (I have been finding things while adding more func tests and tempest tests)
18:59:04 melwitt *flavor/image conflict errors
18:59:22 sean-k-mooney well the flavor image conflcit check shoudl be checkign the glance image keys
18:59:27 sean-k-mooney are we using the stashed copy
18:59:28 melwitt , when the user may not have ever used an image prop
18:59:44 sean-k-mooney i guess we might be to ensure that out of band glance changes dont creap in
19:00:05 sean-k-mooney so we do not need ot use the same key
19:00:12 melwitt we are yeah. for the check virt.hardware.get_tpm_secret_security_constraint() for example
19:00:23 sean-k-mooney that was suggested just to minimise the number of places we have to check
19:01:06 melwitt yeah. using the same key becomes a problem if the user never used an image with that key in it and we have set it internally for other reasons
19:01:12 sean-k-mooney are you thinking we woudl add a new instance system metadata key ?
19:01:31 sean-k-mooney or were you leanign to only allowing resize if its not a confilct
19:01:51 sean-k-mooney meaning we shoudl not set it on update but treat it not being set as the old user policy
19:01:56 melwitt if we want to be able to convert via resize, yes. like convert from host => deployment, etc. unless someone has a different idea
19:01:58 sean-k-mooney i.e. defaulting in code instead of in the db
19:02:45 sean-k-mooney if we dont use a new key then the simplest thing to do is treat key not in data as the USER policy
19:02:50 melwitt because if we set image_hw_tpm_secret_security NOT from an image, it will get locked into the instance as if it had the image property, and it will reject any flavor hw:tpm_secret_security that does not agree with it
19:03:25 sean-k-mooney then the key will only be set if it actully comes form the image
19:03:32 melwitt everything currently works fine for legacy => host and legacy => deployment
19:03:51 melwitt the problem comes when you have user => host, user => deployment, host => deployment, etc. new instances basically
19:04:07 sean-k-mooney ya
19:04:26 melwitt but yeah I see what you are saying
19:04:45 melwitt that would work also
19:05:10 sean-k-mooney basicly image_metadata.get("image_hw_tpm_secret_security", "user") in virt.hardware.get_tpm_secret_security_constraint()
19:05:48 sean-k-mooney well
19:05:54 sean-k-mooney hum
19:05:57 sean-k-mooney htat does not quite work
19:06:12 sean-k-mooney in the resize case we still need to know if its set or not set
19:06:21 sean-k-mooney but for most other case it wil work
19:08:06 sean-k-mooney i think you can still do it pretty cleanly
19:08:16 sean-k-mooney but i dont know are we giving up anything else by doing it that way
19:08:44 melwitt yeah. it's hard to know ahead of time
19:09:18 melwitt what I'm doing now is writing up some more func tests and tempest tests to sort of demonstrate where the series is at the moment with regard to resize and rebuild behaviors
19:10:10 melwitt I was thinking looking at those would make it easier to reason about how it's working at the moment and help us decide what to do next. at least it helps for me

Earlier   Later