Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-05-13
12:35:20 sean-k-mooney it does not say it supports a list of uuids
12:35:40 jkulik sure, but the code has extra ifs for a list ;)
12:36:07 jkulik https://github.com/openstack/nova/blob/master/nova/compute/api.py#L2838-L2841
12:36:11 sean-k-mooney jkulik: do we have api test for the list case
12:36:20 jkulik haven't checked. will do.
12:48:07 jkulik can't find any in the unit-tests at least. for a lot of other query-params, there are tests, but uuid doesn't seem to be in there.
12:49:21 sean-k-mooney jkulik: looking at the code if ti ever worke i suspect it was a nova v2 or nova v1 capablity that was never fully supproted and is not supproted anymore
12:50:07 jkulik looking at the code, it seems totally supported, if we would just convert the MultiDict's multiple entries to a list instead
12:50:41 sean-k-mooney https://github.com/openstack/nova/commit/3f35fe6a88672ea2ab7e080a55235c5cca45dc2c
12:51:07 sean-k-mooney that code is related to listing instance by ip
12:51:27 sean-k-mooney jkulik: its not for listing servers by uuid
12:51:59 francoisp lyarwood, bauzas hello, when you get a chance: https://review.opendev.org/#/c/726508/ (this time around for Queens) - thanks!
12:52:09 bauzas ack, will look
12:52:39 sean-k-mooney jkulik: we support a list there becasue you can have multiple servers with the same ip adress
12:52:56 sean-k-mooney but we dont support the uuid query arg being a list
12:53:13 jkulik sean-k-mooney, but is uses the uuid key out of search_opts which comes from the query-params
12:53:23 jkulik which cannot be a list because it's broken
12:53:39 sean-k-mooney jkulik: the uuid quey arg is not ment to be a list
12:53:55 sean-k-mooney it susing it here so you can filter by ip and uuid
12:53:59 jkulik my use-case would be retrieving infos for multiple servers I know the UUID of in one request btw.
12:54:03 sean-k-mooney the docs say uuid singualr
12:54:23 sean-k-mooney jkulik: yep which as far as im aware we do not support
12:54:28 jkulik sure. the docs say the same for "id" in ports, so I didn't think that's so important ;)
12:55:06 jkulik and since search_opts gets passed down to the db layer in the end, which also can support a list, we would just have to adjust that one line to make it work
12:55:09 sean-k-mooney gmann: your one of our api experst do you know if we have ever supported filtering server list by a list of uuids
12:55:32 jkulik but if it's not supposed to, that's fine, too. I just think the code would support it.
12:55:37 sean-k-mooney jkulik: there is a difference between things that work as a result of internal impemnstion details and what is actully supported
12:56:55 jkulik sean-k-mooney, thanks for looking at it :)
12:57:37 sean-k-mooney jkulik: this is the relevent spec for filtering https://specs.openstack.org/openstack/nova-specs/specs/ocata/implemented/add-whitelist-for-server-list-filter-sort-parameters.html
13:00:48 sean-k-mooney jkulik: so i think we suport filterign with a singel value but not lists or any advance queries.
13:05:23 jkulik I don't see any mentioning of either in the spec, but I can live with the answer "works as designed".
13:07:09 sean-k-mooney it is a feature you could request or propose as a blueprit/spec.
13:07:21 sean-k-mooney i belive its working as designed currently
13:07:37 sean-k-mooney but i dont nessicaly thing it woudl be too contovertall to add support for what you want
13:07:56 sean-k-mooney its just not a bug since i dont think it was ever inteded to wrok
13:18:47 openstackgerrit Takashi Natsume proposed openstack/nova master: Remove six.iteritems/itervalues/iterkeys https://review.opendev.org/727757
13:21:07 openstackgerrit Lee Yarwood proposed openstack/nova master: libvirt: Don't delete disks on shared storage during evacuate https://review.opendev.org/578846
13:21:07 openstack bug 1550919 in OpenStack Compute (nova) "[Libvirt]Evacuate fail may cause disk image be deleted" [Medium,In progress] https://launchpad.net/bugs/1550919 - Assigned to Lee Yarwood (lyarwood)
13:21:07 openstackgerrit Lee Yarwood proposed openstack/nova master: Add functional test for bug 1550919 https://review.opendev.org/631294
13:39:14 jkulik functional tests don't support testing the "uuid as list" case either https://github.com/openstack/nova/blob/master/nova/tests/functional/api/client.py#L248-L250
13:39:46 jkulik the lack of tests for 'uuid' in search_opts isn't good, though :D
13:45:45 sean-k-mooney jkulik: yep again i think that is becasue we did not orginally plan to expose this via the api and retroactivly had to try and fix it
13:46:15 openstackgerrit Jiri Suchomel proposed openstack/nova master: Add ability to download Glance images into the libvirt image cache via RBD https://review.opendev.org/574301
14:24:36 openstackgerrit Takashi Natsume proposed openstack/nova master: Remove six.byte2int/int2byte https://review.opendev.org/727777
14:34:02 gmann jkulik: sean-k-mooney yeah, multi filters things are not supported. for this case where you want to list multiple servers you can use some other query filter which matches multiple servers. like ?name=test so it will return all servers matching with 'test*'
14:35:58 gmann remember, multiple filters are with AND condition
14:38:34 gmann in current behaviour only last present item is being used even you are asking for multi dict
14:41:03 gmann jkulik: it should not be hard to support that, we just need to change the way we fetch filter from GET and query DB with OR condition.
14:41:35 gmann jkulik: anyways, all API change need spec process, please feel free to add BP and spec for the same
14:43:30 gmann but remember any other filter present with mutli dict filter will be with AND condition. so you would not be able to do 'server-uuid1 OR (server-uuid2 AND vm_state=active)' it will be '(server-uuid1 OR server-uuid2) AND vm_state=active)'
15:32:43 lyarwood we should start a club :)
15:33:37 lyarwood gibi: I've not had a chance to look at your caching change btw, I'll try to get to it tomorrow
15:34:09 gibi lyarwood: dont rush it is a very incomplete messy pile of boo
15:36:02 gibi lyarwood: I just figured out that the current imagebacked code my temporarily use double of the image size disk space. For example if an image is a qcow in glance but nova configured to force_raw_images then after image download we copy out the raw data from qcow and the delete the qcow file we downloaded :/
15:36:17 gibi s/my/might/
15:36:47 gibi who thought that we will have disk space for that operation?!
15:38:03 gibi I get to feel that I have no power to patch this code in a way I imagined
15:38:26 dansmith lyarwood: is it a club if everyone is a member?
15:38:34 dansmith I think that's called "a population"
15:39:10 dansmith gibi: did you catch the scrollback of our convo yesterday?
15:39:20 lyarwood dansmith: ^_^
15:39:58 lyarwood gibi: yeah I don't think we can convert in-place tbh
15:40:01 gibi dansmith: yes. but honestly I have to go back to it as I already forget what was the pre-filter idea
15:40:43 dansmith gibi: so, I think it's not unreasonable to say "to boot an instance of $root from an image of $imgsize, the host needs to have $root+$imgsize free space, as a rule
15:41:21 dansmith although you know what..
15:41:32 dansmith I think we were totally missing something yesterday, now that I re-state that with a fresh mind
15:42:29 dansmith for that to even help, we have to account for the base in the inventory or allocations somehow, which is what we were trying to avoid there
15:42:36 dansmith so, hrmph
15:43:07 dansmith gibi: I will say that although I understand the desire to disable the cache as a workaround, it's really not a useful solution for anything other than a very small subset of cases
15:43:29 dansmith i.e. where you expect only one of each image type to be booted in a disk-constrained place anyway
15:43:43 gibi and I think it is pretty impossible to do properly due to what assumption the code currently makes
15:43:49 dansmith so I wouldn't want to spend a bunch of effort or cause a bunch of destabilization in the image backend to do so
15:43:57 dansmith gibi: right, hence my concern yesterday :)
15:44:18 gibi I needed to feel the pain to understand :)
15:44:25 dansmith :D
15:47:00 dansmith gibi: so, one thing I was wondering
15:47:03 gibi even if we start making allocation according to the images in the cache, the current code sometimes copy things over for conversions and that doubles the disk usage temporarly, or fail if no disk space for such operation
15:47:53 dansmith gibi: did you find somewhere that the DiskFilter considered the size of the image? Because even considering disk free space, you don't know if the target host will need room for the image (if it's not cached) PLUS the root, etc disks as well
15:48:22 dansmith because I don't think it really took that into account.. i.e. adding the flavor disks and the possible space required for a separate image
15:49:06 gibi DiskFilter used the disk_available_least value from the DB. There we count current available free space in the $instances_path (among other things)
15:49:21 dansmith gibi: yeah, and like I said yesterday if we start doing allocations for images, we have a lot of cases we need to cover, a lot of new potential needs for healing that data, etc.. it concerns me to make a decision to start doing that so quickly
15:49:37 gibi dansmith: ^^ agree
15:50:10 dansmith gibi: right, but the scheduler only looks to see if root+swap+ephemeral will fit in that space, but it may still fail because the compute node doesn't have the image cached already and thus need root+swap+ephemeral+image space to do the work
15:50:41 dansmith it's less of a problem, but it's similar
15:50:49 gibi correct (shit, another edge case)
15:52:08 dansmith so here's a couple of less impactful options:
15:52:56 dansmith 1. Each time we cache an image, or run the periodic, we generate a disk allocation for the compute node uuid which consumes inventory according to how much space the cache is using
15:53:04 dansmith 2. Same as above, but adjust the reserved amount
15:53:50 dansmith both cases need to consider the case where the _base is not on the same filesystem as the instances, but there's less synchronization involved, and we're not spraying tons of new allocations into place
15:54:34 dansmith we could also make it a workaround that you opt into in the short term to see how it goes, because cleanup from it would be much easier (just nuke one $cnuuid allocation)
15:55:18 dansmith and we could make compute node startup nuke that allocation if present and the workaround is disabled (or the self-correcting reserved amount)
15:55:54 gibi right, I would use allocation instead of reserved as allocation has a consumer attached
15:56:16 gibi reserved would be a sum of configured + detected
15:56:30 gibi which is math, that I dont like :)
15:56:31 dansmith hopefully glance gives us enough information to be able to increase that allocation before we start the download, so we know "oh sorry, inventory says we don't have room for this base image, so fail()"
15:56:52 dansmith I know it would, but I think reserved would be more obvious to an operator
15:57:00 dansmith even though it's a composite value
15:57:20 dansmith just because listing allocations are a bunch of meaningless-to-the-human UUIDs
15:57:46 dansmith I'm not arguing for that, I'm just saying there're benefits both ways
15:57:57 gibi I see. yes it is a tradeoff

Earlier   Later