| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-07-22 | |||
| 18:50:32 | sean-k-mooney | dansmith: well we will likely need to modify the image cache api to pass the format but im not really sure form where other then when we are downloadign the image | |
| 18:50:42 | dansmith | and, we won't know which images in the cache were/are for rescue or anything.. I think we have to get to a point where it's something we can rely on, else we will always have the legacy path | |
| 18:50:51 | dansmith | sean-k-mooney: right, that | |
| 18:50:55 | dansmith | is my point though, | |
| 18:51:00 | dansmith | we | |
| 18:51:10 | dansmith | have already decided what the dest filename will be, before we even fetch it | |
| 18:51:20 | dansmith | and changing that reaches *deep* backwards into the driver | |
| 18:51:35 | dansmith | it's sort of ... insane | |
| 18:51:50 | sean-k-mooney | ok then the alternitive is to write a new info file becied the orgianl with the format as the content | |
| 18:51:55 | melwitt | yeah, it is very disjointed | |
| 18:52:24 | sean-k-mooney | as ub _base/backing_file _base/backing_file.info | |
| 18:52:31 | sean-k-mooney | *as in | |
| 18:53:23 | sean-k-mooney | if we cant modify the filename but know what it is we can comptue the relitive name | |
| 18:53:42 | dansmith | the problem is that if we do that, we need to start copying it during migration | |
| 18:54:06 | sean-k-mooney | only if the image is deleted form glance | |
| 18:54:08 | dansmith | I also was hoping to make it a lot more obvious when we missed one | |
| 18:54:17 | sean-k-mooney | otherwise it would be download on the dest node instead of copying | |
| 18:54:18 | sean-k-mooney | but yes | |
| 18:54:51 | dansmith | right, you can see that hole right in front.. upload a bad image, boot, delete it, then migrate to exploit :) | |
| 18:54:56 | sean-k-mooney | am im just finsihing for today but ill sleep on it and take a look at the code tomrrow and see if anythign jump out at me | |
| 18:55:07 | dansmith | ack | |
| 18:55:41 | melwitt | dansmith: the fetch and disk image creation happen in Image.cache() which is called here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L11500 could you maybe return the file format from the fetch func and then pass it into create_image to tack on the extension to the name before writing it? | |
| 18:55:58 | dansmith | melwitt: I think that's only some of the time | |
| 18:56:11 | sean-k-mooney | the only other optiopn that jumps otu at me at the moment is to use the a singel file for all the format info or a db like pythons dbm or sqlite modules | |
| 18:56:16 | dansmith | for ami,ari,aki we fetch direct I think | |
| 18:56:19 | sean-k-mooney | which i assume you dont wnat to do at least right now | |
| 18:56:43 | dansmith | and I thought there were also some of those backend drivers that raise and cause us to take the fetch direct path again, like on boot from snapshot or unshelve or something? | |
| 18:56:55 | sean-k-mooney | i.e. you want this to be local to the compute node and obvious form simple inspection fo the filesytem without special tools | |
| 18:57:07 | dansmith | sean-k-mooney: multiple files that get deleted with their parent seems cleanest if we're going that route | |
| 18:57:16 | sean-k-mooney | and it also needs to work when /var/lib/nova is on nfs | |
| 18:57:20 | dansmith | there is also the concern... yeah NFS | |
| 18:57:28 | dansmith | multiple files make locking less reliable | |
| 18:59:07 | dansmith | melwitt: I was initially going to return format from fetch_as_raw (and add the lookup/return to fetch) yeah | |
| 18:59:23 | dansmith | however we'd need another move there in try_fetch_image_cache() which feels wrong, | |
| 18:59:43 | dansmith | and also, there are cases where we don't go through that like the ami triplets I think | |
| 19:00:09 | dansmith | and of course that copy_from_host, which is going to be a problem either way | |
| 19:01:08 | melwitt | dansmith: move? the base image isn't created until Image.create_image (which is called from Image.cache) | |
| 19:02:16 | dansmith | yeah, if fetch writes it to where it was asked and returns the format, then _try_fetch_image_cache() would need to move it to $path.$format | |
| 19:04:06 | dansmith | unless we put that in image.cache, if that's what you mean? | |
| 19:04:12 | dansmith | same difference though I think | |
| 19:05:01 | melwitt | dansmith: no.. I'm thinking of how fetch isn't called until in Image.create_image so I was thinking you could adjust the target name before writing the file | |
| 19:05:10 | dansmith | either way we end up with a sequence of: 1. fetch image info, 2. forget the format and calculate the wrong filename, 3. fetch the image info again, 4. write to a different file | |
| 19:05:26 | dansmith | right, but ^ | |
| 19:05:40 | dansmith | which is doable, it's just pretty stupid.. | |
| 19:06:32 | melwitt | I guess I'm not understanding how that would be necessary, like forget the format and all that | |
| 19:06:51 | dansmith | we only have image_id here right? | |
| 19:07:21 | melwitt | if the fetch func returns the format I mean | |
| 19:07:34 | melwitt | *if you made fetch return the format | |
| 19:07:49 | dansmith | ...which will be done by re-fetching the image info yeah? :) | |
| 19:07:57 | melwitt | no, the first fetch | |
| 19:08:08 | dansmith | wut | |
| 19:09:51 | melwitt | like in Image.create_image, the prepare_template() function is the fetch function. I guess you would have to be able to adjust what's in the functools.partial though to do it, maybe that's something that is not possible? | |
| 19:11:08 | dansmith | I'm a bit lost here, but while we were talking I see that the kernel and ramdisk go through raw.cache() as well which means the cache function itself might work for those too as you say | |
| 19:11:51 | dansmith | but note that ephemeral and swap are created like that as well, so they'd all need to change so we get a stable format returned from their not-really-fetch-function-s | |
| 19:12:48 | melwitt | that is true, all of the fetch funcs would need to be modified to return format | |
| 19:13:33 | dansmith | also after this, | |
| 19:13:55 | dansmith | wait | |
| 19:16:39 | dansmith | I'm just trying to figure out if we need the filename anywhere else after we've made that call | |
| 19:17:05 | dansmith | seems like we imply it below that but maybe we never use it | |
| 19:17:36 | dansmith | just feels wrong to have one step change the filename we're going to use in the middle, because someone will come along and say "oh look I have filename here, I can do a thing" | |
| 19:18:56 | dansmith | er, yeah, because _create_image() is called and then we go create the guest's xml which will assume the image(s) are named like instructed right? | |
| 19:19:44 | melwitt | yeah.. error prone at best I guess. and maybe an actual problem when a request comes in for a cached image, I guess maybe you could still match it by fingerprint + glance reported disk_format and if found in cache, use what's in the cache | |
| 19:20:37 | dansmith | we can't though, | |
| 19:20:41 | melwitt | maybe.. have to think about it. you were only talking about adding extension to base images right? or are you meaning like all images | |
| 19:20:55 | dansmith | because we need to know the image's disk_format, unless you want a THIRD ping to glance :) | |
| 19:21:19 | dansmith | I think all images would be ideal, but I think the base images are the important case here | |
| 19:21:24 | melwitt | oh, right. hm | |
| 19:22:30 | melwitt | well, could you not assume the format of what you find in the cache? like you get a request, you calculate the fingerprint, look for the basename in the cache, if you find it you use the file with the extension on it | |
| 19:23:05 | melwitt | as the format. without querying glance for it | |
| 19:23:49 | dansmith | I mean, we can also just run our detection code on the images without the format every time.. the point is to try to harden this, not just layer on the guesses :D | |
| 19:24:23 | melwitt | no, the detection ran the first time the base image was written. thats' when you added the extension | |
| 19:24:33 | dansmith | and part of the strategy here, I think, is to make sure we don't lose track of what we've validated and so yeah, doing /images/$image.*[0] is better than we have, it still seems like a hack | |
| 19:24:33 | melwitt | but from then on, you don't need to look it up again | |
| 19:24:49 | melwitt | ok | |
| 19:25:04 | dansmith | I understand what you're saying, I'd just rather it be better than that | |
| 19:25:13 | melwitt | ack | |
| 19:26:05 | dansmith | I really was hoping to avoid just cluttering the _base dir with .info files, but perhaps that's just going to be easier because of how obsessed we are with filenames throughout the layers | |
| 19:26:18 | dansmith | I dunno, I'll have to think about it | |
| 19:27:10 | dansmith | also makes an admin's job harder if the next time we have a thing, they have to go read all the info files looking for which one(s) are qcows, then using that list to do some remediation action | |
| 19:27:37 | dansmith | like currently, we were unable to check and purge bad images from the cache if they're present and the operators need to do that | |
| 19:27:50 | dansmith | that's just a UX thing and not critical of course, but...it'd be nicer | |
| 19:28:25 | dansmith | I assume if we use .info files, we should dump json in there because as soon as we add this we'll need to store something else (honestly we should probably store image_id at least as well)( | |
| 19:33:45 | melwitt | yeah I agree .info files would be json. you could even have a nested list for all the files in one .info! | |
| #openstack-nova - 2024-07-23 | |||
| 07:29:40 | opendevreview | Alexander Hild proposed openstack/nova master: feat: Include "vm uuid" in baseBoard asset tag in libvirt domain xml https://review.opendev.org/c/openstack/nova/+/924697 | |
| 07:31:56 | opendevreview | Alexander Hild proposed openstack/nova master: feat: Include "vm uuid" in baseBoard asset tag in libvirt domain xml https://review.opendev.org/c/openstack/nova/+/924697 | |
| 08:06:18 | mikal | sean-k-mooney: are you around? I think https://review.opendev.org/c/openstack/nova/+/922544 is ready for review but I have questions about if there needs to be a tempest test for it. | |
| 08:39:36 | sean-k-mooney | mikal: im not sure how you would tempet test that spice is secure. we recently tired to do that for vnc and vcrypt and we could not come up with a reasonable way ot do that just with a tcp connection becuase vnc uses tls to encypt the vnc packate payload not the entire tcp packet payload | |
| 08:40:53 | sean-k-mooney | so you can just connect without implementing the nvc protocal at least enought to negoicate the vencrypt session | |
| 08:41:43 | sean-k-mooney | unless there is a simple way to asssert that spice is using a secure connection without implemeting a portion of the spice protocoal i think unit and perhaps functional tests are enough | |
| 08:43:14 | sean-k-mooney | mikal: it might be worth considerign extending the libvirt funcitonal tests to be able to test some of the spice direct support but im not sure where in the series it makes sense to do that | |
| 09:20:30 | mikal | sean-k-mooney: so, it depends on if tempest can carry additional software. Kerbside does have a simple client which can parse enough of the protocol to know if its been redirected to a secure connection or not. | |
| 09:21:48 | mikal | sean-k-mooney: but then we'd have to carry extra code in tempest for that. https://github.com/shakenfist/kerbside/blob/develop/kerbside/spiceprotocol/__init__.py#L102 is approximate the right code. | |
| 09:29:42 | mikal | sean-k-mooney: also, should I move this spec from the "to be determined" bit of the dalmation etherpad to the "needs review" bit, or is that someone else's job? | |
| 09:30:47 | sean-k-mooney | mikal: tempest is generally not allowed to have depencies on external tools unless you are writing your own plugin | |
| 09:31:19 | sean-k-mooney | there are some things like ssh ectra but for the most part we try to keep core tempest as depency free as is reasonable | |
| 09:31:25 | mikal | sean-k-mooney: yeah, that's going to make any meaningful testing pretty hard to do. | |
| 09:31:53 | sean-k-mooney | mikal: again it would be totally fine to depend on kerbside in a kerbside tempest plugin | |
| 09:32:37 | mikal | sean-k-mooney: I'm happy enough to take a swing at writing such a plugin, but I'd like to see some more traction on the patch series and implement more of the changes requested (extra specs, new API flow) before pivoting to that. | |
| 09:33:02 | sean-k-mooney | and that can be in the kerbside repo or in a seperate one. if we want to do real end ot end testing i would suggest creating both a devstack adn tempest plugin which we can enable in a upstream job in the future | |
| 09:33:40 | sean-k-mooney | ya i personally woudl love to see end ot end testing but im ok with it happenign after the feature is completed | |