| wncslln | hey nova o/, I was looking at some low hanging fruits bugs to solve, I've found this one https://bugs.launchpad.net/nova/+bug/2065693 | 11:56 |
|---|---|---|
| wncslln | since I'm a beginner, someone could show me where I should fix, if is possible? | 11:56 |
| wncslln | I undestood that foreign key may does not exist in another table | 11:57 |
| sean-k-mooney | wncslln: that might not be the best bug to start with | 12:04 |
| sean-k-mooney | this tempest test is trying to delete an server that has not been completed spawning yet | 12:05 |
| sean-k-mooney | what failing is recordign the delete event on the instancee because the instnace does not currently exist in the instnace table | 12:06 |
| sean-k-mooney | that is causing the foreign key to fail | 12:06 |
| sean-k-mooney | so this is not an issue with the db schema | 12:06 |
| sean-k-mooney | whats broken here is that we shoudl gracefuly handle that issue and either reject the delete at the api or complete the delete without an error | 12:07 |
| sean-k-mooney | neither are trivial to do correctly. we likely do no want to reject the request as a user should always be able to delete an insance for billing reasons | 12:10 |
| sean-k-mooney | so while a 409 conflict could be valid in other case it woudl be undesireable in this case | 12:11 |
| sean-k-mooney | the simplest possibel fix in this case would be ot catch the excption to record the instance action and log/ignore it then proceeed with the delete | 12:11 |
| sean-k-mooney | that would be a valid resolution to the bug but if the instance does not exist in the cell db it also means we cant mark it as deleted which means we have no way to interup the request to spawn it. | 12:13 |
| sean-k-mooney | which is why this is a hard thing to fix as a first bug | 12:13 |
| wncslln | I see... I'll follow the bug updates to see the resolution and try to understand it | 12:15 |
| wncslln | so sean-k-mooney, do you recommend any bug? | 12:16 |
| sean-k-mooney | we do actully have a list of low hanging fruit bugs https://bugs.launchpad.net/nova/+bugs?field.tag=low-hanging-fruit i have not looked at that in a while | 12:17 |
| sean-k-mooney | but perhaps one of those | 12:17 |
| sean-k-mooney | perhaps https://bugs.launchpad.net/nova/+bug/2015330 | 12:18 |
| sean-k-mooney | for something perhaps more featury then bug we have this tech debt reduction bug https://bugs.launchpad.net/nova/+bug/1994139 | 12:19 |
| wncslln | I'll take a look on these. thanks! | 12:22 |
| stan | live migrate says rolled back on the target server, what would be the appropriate logs to check for further troubleshooting? | 13:36 |
| opendevreview | sean mooney proposed openstack/nova master: [WIP] ensure correct cleanup of multi-attach volumes https://review.opendev.org/c/openstack/nova/+/916322 | 14:36 |
| dansmith | bauzas: trivial patches to CI jobs, if you have a minute: https://review.opendev.org/c/openstack/nova/+/919738 | 14:52 |
| dansmith | first one saves us *minutes* of setup time on the affected jobs, second one goes back to regular images as we discussed | 14:52 |
| opendevreview | Dan Smith proposed openstack/nova master: Enable OCaaS for several nova jobs https://review.opendev.org/c/openstack/nova/+/919738 | 15:24 |
| opendevreview | Dan Smith proposed openstack/nova master: Stop using split UEC image (mostly) https://review.opendev.org/c/openstack/nova/+/919739 | 15:24 |
| dansmith | melwitt: ^ | 15:24 |
| dansmith | good catch! | 15:24 |
| dansmith | melwitt: thanks! | 15:32 |
| melwitt | np | 15:33 |
| sean-k-mooney | dansmith: looks like we have upgraded the version of zuul? | 15:46 |
| sean-k-mooney | The RE2 syntax error is: invalid perl operator: (?! | 15:46 |
| sean-k-mooney | clarkb: i rember ther was an annouchment about that a while ago but nova was not using any non RE2 regexes at the time | 15:47 |
| sean-k-mooney | and i dont think we have added any since | 15:47 |
| sean-k-mooney | clarkb: any idea why we are seeing https://review.opendev.org/c/openstack/nova/+/919738/1..2/.zuul.yaml#336 now | 15:47 |
| sean-k-mooney | clarkb: is this just a warning currently? | 15:48 |
| sean-k-mooney | looks like negitive matches are now triggering it | 15:50 |
| sean-k-mooney | irrelevant-files: | 15:50 |
| sean-k-mooney | - ^(?!.zuul.yaml)(?!nova/virt/libvirt/)(?!nova/privsep/).*$ | 15:50 |
| sean-k-mooney | i belive what we are trying to express in the nova-lvm jobs is | 15:51 |
| sean-k-mooney | we should only run this on changes to .zuul.yaml nova/virt/libvirt/** and nova/privsep/** | 15:51 |
| sean-k-mooney | so the way to fix this would be to use relevant-files: instead right? | 15:52 |
| sean-k-mooney | gmann: ^ | 15:52 |
| sean-k-mooney | so relevent-files: "^(?:.zuul\.yaml|nova\/virt\/libvirt\/.*|nova\/privsep\/.*)$" | 15:58 |
| sean-k-mooney | dansmith: melwitt am i correct that your preference is to move forward with https://review.opendev.org/c/openstack/nova-specs/+/907654 and then continue the converstaion in the implemenation review and loop back and update the spec if needed | 17:40 |
| dansmith | I think I said as much, but I wasn't anticipating your most recent concern, so if we need to nail that down, we should | 17:41 |
| sean-k-mooney | my main concenr is ensuring tha we can express if the instance is encypted (and optionally the format) and if the image is encypted (and the format) seperatly | 17:42 |
| dansmith | yeah I know, and as I just noted, I think that's a dangerous tripwire :) | 17:42 |
| sean-k-mooney | why so? the format so the image and root disk may not be the same. | 17:43 |
| sean-k-mooney | are you suggesting that we shoudl require the instance to be encypted if the image is | 17:44 |
| dansmith | yes and especially that we should not *decrypt* the image if you boot from it without that requirement | 17:44 |
| sean-k-mooney | so i see that as being in conflict with allowing you to take an unecypted snapshot | 17:45 |
| dansmith | that's kinda like having FDE enabled and dropping it because you upgraded to the next version of your OS and forgot to opt-in again | 17:45 |
| dansmith | why? you're specifically asking to have it de-crypted in that case | 17:45 |
| sean-k-mooney | and you explcitly ask to not encypt the instance in the other | 17:45 |
| sean-k-mooney | well implictly | 17:46 |
| dansmith | very implicitly :) | 17:46 |
| sean-k-mooney | but unless we make it a tristate | 17:46 |
| sean-k-mooney | i think that equivlent | 17:46 |
| dansmith | well, the other option would be to require your hw_ to be set to either match the format of the image, or explicitly request cleartext and we refuse to boot if it's not set to match | 17:46 |
| dansmith | but I think that's just adding an extra hoop for people | 17:47 |
| sean-k-mooney | well i wasd thinking that you might want to upload the image in gpg format to glance | 17:47 |
| sean-k-mooney | but boot form a vm in luks format | 17:47 |
| melwitt | I thought the debate here is whether or not to allow a change in encryption format going from image => root disk. because otherwise afaict hw:ephemeral_encryption (the bool) provides everything else? | 17:47 |
| sean-k-mooney | after all we cant boot form an image in gpg format right | 17:48 |
| dansmith | as I said in my reply, I think this is getting to the point of unnecessary layers that users don't care about (i.e. the distinction between glance and nova and cinder's notion of the encrypted-ness) | 17:48 |
| dansmith | sean-k-mooney: gpg format is useless.. it was added before and only because there wasn't userspace tooling for LUKS | 17:49 |
| dansmith | I think they're mostly using it as an example at this point and I thought they were going to drop that anyway | 17:49 |
| sean-k-mooney | ok but we cant boot form that | 17:49 |
| dansmith | correct | 17:49 |
| sean-k-mooney | we could boot form it if we decypt or convert | 17:50 |
| dansmith | melwitt: one or more of us is implying too much about the format of the image and the intent for booting new instances I guess.. the boolean doesn't really matter, it's the default we assume | 17:50 |
| sean-k-mooney | well the bool is ment to be (should this instance have encypted ephemeral storage) | 17:51 |
| dansmith | my concerns are that (a) we should never implicitly decrypt something and (b) we should not design something that requires us to do a (very expensive) format/encryption conversion for the usual case | 17:51 |
| sean-k-mooney | that is indepenent of if its BFV or boot form image | 17:51 |
| sean-k-mooney | well the only way to avoid the format conversion would be luks iamge and luks encyption for qemu | 17:52 |
| sean-k-mooney | we cant store dmcypt_plain images in glance can we? | 17:52 |
| sean-k-mooney | if we can that woudl also be valid | 17:52 |
| melwitt | I agree we should never implicitly decrypt something | 17:53 |
| sean-k-mooney | cant we boot form encypted iamges today? | 17:53 |
| sean-k-mooney | i tought we always decypted them? | 17:53 |
| dansmith | eh? | 17:53 |
| sean-k-mooney | maybe thats not a thing? | 17:53 |
| melwitt | (don't want any surprises) | 17:54 |
| melwitt | "today" meaning in the proposed patch series? | 17:54 |
| sean-k-mooney | i might be mixing that up with signed images | 17:54 |
| melwitt | in the series there is support for booting from encrypted images | 17:54 |
| melwitt | oh, yeah you might be thinking of signed | 17:54 |
| sean-k-mooney | no today meaning Caracal/master | 17:54 |
| dansmith | that's kinda the whole point right? | 17:54 |
| dansmith | (booting from encrypted images" | 17:54 |
| sean-k-mooney | not entirely. so i tought the gap we were trying to solve is booting vms with encypted local storage | 17:55 |
| sean-k-mooney | irriespective of the encyption state in glance | 17:55 |
| melwitt | I think the main point is booting encrypted from unencrypted but booting from encrypted quickly becomes an expectation bc of snapshotting imho | 17:56 |
| dansmith | what's the point of local storage encryption if snapshots are clear? | 17:56 |
| sean-k-mooney | they wernt clear in the previosu iteration of the spec | 17:56 |
| dansmith | maybe I'm completely off base here, but in a cloud snapshots are like the #2 most important unit of measure behind instances themselves | 17:56 |
| melwitt | because the user might add private data after that (I thought?) | 17:56 |
| dansmith | you need snaps for basically everything in the cloud workflow | 17:56 |
| sean-k-mooney | i dont think that is true | 17:57 |
| melwitt | but like for example my laptop, the OS image was not encrypted but my disk is encrypted | 17:57 |
| sean-k-mooney | * i dont think snapshots is the #2 most imporant thing in a cloud workflow | 17:58 |
| dansmith | melwitt: it's a bit different there because the OS is read-only and your user data is transportable separately from the OS image, whereas we don't really support that | 17:58 |
| melwitt | that's what I think the most common case probably is. but like I said I think if snapshot is a basic feature available in the cloud, I think users would expect to be able to boot from an encrypted snapshot as well | 17:58 |
| dansmith | sean-k-mooney: okay well, we disagree there then | 17:58 |
| sean-k-mooney | melwitt: right but lest take an example | 17:59 |
| sean-k-mooney | i boot a vm form an unencypted ubuntu image with a flavor that enabel instance encyption | 17:59 |
| sean-k-mooney | if we take a snapshot of that then i would expect you to set the hw_ephemeral_encyption image property in the snapshot | 18:00 |
| sean-k-mooney | and the OS_GLACE_* properties to recored the format | 18:00 |
| dansmith | what about cinder? | 18:01 |
| sean-k-mooney | can you elaberate? | 18:01 |
| dansmith | you want them to set that property on an encrypted image they upload just in case someone wants to boot from it? | 18:01 |
| sean-k-mooney | no | 18:01 |
| dansmith | because if they don't you'll decrypt their data when you boot from it if you don't know to go set that intent flag | 18:01 |
| sean-k-mooney | but if we were taking a snapshot of a BFV volume we could | 18:02 |
| melwitt | I think that's where I differ in my thought, I don't think hw_ephemeral_encryption (the boolean) on the snapshot. only the secret uuid (os_glance_encrypt_key_id) which implies it is encrypted | 18:02 |
| sean-k-mooney | it imples the image is | 18:02 |
| sean-k-mooney | but not that only encypted guest can be crated form it | 18:02 |
| melwitt | right | 18:03 |
| sean-k-mooney | weither we set hw_ephemeral_encyption on the snapshot is a design choose we can make | 18:03 |
| sean-k-mooney | but to me hw_ephemeral_encyption is the only thing on an image that says the resulting vm must be booted with encypted local storage | 18:03 |
| dansmith | melwitt: wait, you think that if you snapshot an encrypted instance to an encrypted image and then create another instance from that, it should be de-crypted? | 18:04 |
| melwitt | dansmith: no, I don't.. but somewhere along code review someone commented that I should not assume encrypted if the source is encrypted. originally I did assume encrypted if source encrypted | 18:05 |
| gmann | sean-k-mooney: yes, we can use the "files" (files is correct name of var, not relevent-files ) variable to define the files we want to run the job when they are changed. | 18:05 |
| dansmith | okay, I missed that I guess.. I definitely do not think we should ever decrypt unless specifically asked | 18:05 |
| sean-k-mooney | dansmith: so to me that means we shoudl reject booting form an image with OS_GLACE_* options | 18:06 |
| dansmith | so if we have the hw_ boolean on the image, I think that either we take the default to match the format, or we *require* it be set to something before we agree to use the image (i.e. tell me what you want me to do with this) | 18:06 |
| sean-k-mooney | if the flavor or iamge does not have hw_ephemeral_encyption=True | 18:06 |
| melwitt | ok. I might have misunderstood what the person said. and tbh I need to double check what it's doing at the moment because I don't remember off the top of my head | 18:06 |
| dansmith | sean-k-mooney: we could, but I think we could also punch the users in the face | 18:06 |
| sean-k-mooney | i think the glance feature is currently broken | 18:06 |
| dansmith | there is no glance feature (now or in the future) to be clear | 18:07 |
| sean-k-mooney | becuase to mean the exisitng option are only about the format of the image at rest | 18:07 |
| sean-k-mooney | without condiering the extra requiremns for nova/cinder | 18:07 |
| sean-k-mooney | im confuges but someones at my door brb | 18:07 |
| gmann | sean-k-mooney: and when we have less number of files we want job to run it is better and much readable to define those via "files" var instead of excluding a lot of others via "irrelevant-files" | 18:08 |
| melwitt | I think the comment was about the fetch_to_raw method, I originally made it base the destination format on the source format. i.e. if image is encrypted then created disks should be encrypted | 18:08 |
| dansmith | melwitt: right and that's definitely what I think we should do.. I commented on that method, so I hope I didn't mis-convey that I thought decrypting was the right thing to do | 18:09 |
| dansmith | fetch_to_raw shouldn't really be making that policy decision | 18:09 |
| dansmith | IIRC, that comment was about assuming plain if the source was plain or something, in support of my argument here | 18:10 |
| melwitt | dansmith: I may have misinterpreted. I can see the point that it shouldn't make the decision. I hadn't thought of that. either way I'll go back and fix it if it needs fixing because I'm not 100% sure at the moment | 18:10 |
| melwitt | *make the policy decision | 18:11 |
| dansmith | melwitt: I think my comment was about what happens if you don't pass in explicit requirements to it, but not about what we should do at the high-level as a "policy" | 18:11 |
| dansmith | ack | 18:11 |
| melwitt | gotcha | 18:11 |
| sean-k-mooney | gmann: ya so the issue is we currently use a negitive regex and zuul has change the regex lib they use and that is not supproted | 18:11 |
| sean-k-mooney | gmann: so we will need to remove all our usages of negitive regex regardlels of which is shorter | 18:11 |
| sean-k-mooney | melwitt: dansmith would it help to hop on a meet call?. | 18:12 |
| gmann | sean-k-mooney: you mean irrelevant-files is not supported now? and we need to use "'files' and negate=True" ? | 18:12 |
| sean-k-mooney | gmann: no i mean that rc2 does not supprot negitive suffix matches | 18:13 |
| gmann | I am not aware of that, if so then it is a big changes to all the openstack jobs | 18:13 |
| sean-k-mooney | so we use irrelivent files with "(?! ... )" | 18:13 |
| gmann | sean-k-mooney: ohk, yes that is true. we have done those changes in Tempest too. got it | 18:13 |
| sean-k-mooney | "(?!" is not supported | 18:13 |
| gmann | yes | 18:13 |
| sean-k-mooney | so when the first email was sent about this change im 99% certin nova was not flagged as needing anychanges | 18:14 |
| sean-k-mooney | now zuul is omitting a waring | 18:14 |
| gmann | yeah | 18:14 |
| sean-k-mooney | and i was wondering if anyone is currently fixing this | 18:14 |
| gmann | I am not aware if anyone fixing those. we fixed a few of them in 'branches' variant in past | 18:15 |
| sean-k-mooney | dansmith: melwitt when i said new galnce feature i was refering to the ablity to store an encypted image, which would require supprot in nova/cinder to create an instance/volume form them | 18:15 |
| sean-k-mooney | from my perspective melwitt spec has nothign to do with that | 18:16 |
| dansmith | glance won't have any changes to allow storing an encrypted image | 18:16 |
| dansmith | it's already storing them for cinder | 18:16 |
| sean-k-mooney | https://github.com/openstack/glance-specs/blob/master/specs/2024.1/approved/glance/image-encryption.rst | 18:17 |
| sean-k-mooney | so that is not going to be implemnted | 18:17 |
| dansmith | it's just a spec to capture the standardization of the properties at this point | 18:17 |
| dansmith | it was originally going to involve some other pieces but I think it's all removed.. either way, as melwitt has demonstrated with her series already, no glance support is required or expected here AFAIK | 18:18 |
| sean-k-mooney | ok but form my perspective os_glance_encrypt_format has nothing to do with the local encyption series | 18:18 |
| dansmith | I understand you feel that way | 18:18 |
| melwitt | fwiw sean-k-mooney I don't think it does/has to have something to do with the local encryption series but I'm not following what/why that matters (?) | 18:20 |
| sean-k-mooney | melwitt: it matter because dan is asserting that if os_glance_encrypt_format is set we cant boot a instnace without encyption form it | 18:20 |
| sean-k-mooney | where as i think decypting the image in that case is the correct behavior because the image is not set to enable encyption | 18:21 |
| melwitt | sean-k-mooney: from what I understand we were thinking we would be able to if hw_ephemeral_encryption=false or hw:ephemeral_encryption=false on the image or flavor. like if someone explicitly conveys they want it to be decryped | 18:22 |
| sean-k-mooney | we could make it a trisate and have hw_ephemeral_encryption=false have a diffent menaing then hw_ephemeral_encryption not set | 18:22 |
| sean-k-mooney | but that is ofthen confusing to users | 18:22 |
| melwitt | at least so far (and from before I started working on it), hw_ephemeral_encryption=false has meant explicitly don't encrypt, not set means "don't care", hw_ephemeral_encryption=true | 18:24 |
| sean-k-mooney | right that is what i expect hw_ephemeral_encryption=false to mean | 18:25 |
| melwitt | *hw_ephemeral_encryption=true means specifically want encrypted | 18:25 |
| sean-k-mooney | but i also expect unset hw_ephemeral_encryption to maintian the exsiting behavior | 18:25 |
| sean-k-mooney | which woudl be unencypted | 18:25 |
| melwitt | it does, but I think if the source image is encrypted, the default behavior should be to maintain the encryption no? | 18:26 |
| dansmith | ++ | 18:26 |
| sean-k-mooney | correct | 18:26 |
| melwitt | ok. I'm not sure we are really disagreeing :) | 18:26 |
| sean-k-mooney | and for encypted iamges we would do what | 18:26 |
| * dansmith is so confused | 18:26 | |
| sean-k-mooney | i woudl expect use to reject booting them | 18:27 |
| * dansmith tables flips | 18:27 | |
| sean-k-mooney | you have not enabled encyption in nova using either the flavor or image property we are specifying | 18:27 |
| sean-k-mooney | so there is no way for use to schdule to a host that supprots the feature | 18:27 |
| melwitt | I just said encrypted images implies encrypted if hw_ephemeral_encryption is unset, if hw_ephemeral_encryption=false is present, decrypt it | 18:27 |
| sean-k-mooney | right so i dont think that a backwars compatiabel change. | 18:28 |
| sean-k-mooney | we coudl make that work but then we have to document this very diffently | 18:28 |
| melwitt | yeah I mean if the source image is encrypted and hw_ephemeral_encryption is unset and no host in the cloud supports local encryption, I think it would reject that request and the person has to come back with hw_ephemeral_encryption=false? | 18:28 |
| sean-k-mooney | right os i dont like relying on an no valid host for that determination | 18:29 |
| sean-k-mooney | if we want to make this work then i think we shoudl not have image properties to enable instance encyption | 18:29 |
| sean-k-mooney | and instead just have the flavor extra spec and enabel encyption if the OS_GLANCE_ENCYPTION* | 18:30 |
| sean-k-mooney | image property is there | 18:30 |
| melwitt | I think the problem with that is then it gives users no way to control it. since flavors are generally admin only created | 18:30 |
| sean-k-mooney | they woudl contol it by uploading an encypted image | 18:30 |
| melwitt | 🤔 | 18:31 |
| sean-k-mooney | i really think the sandardsation attpemt for the image properies in glance is incorrect | 18:32 |
| sean-k-mooney | i dont disagree with it in principal but | 18:32 |
| sean-k-mooney | with the implication it has for nova/cinder | 18:32 |
| sean-k-mooney | dansmith: am i correct in saying you want a single way to express that a given image is encypted and all resouces careated form it should remain encypted" while allso allowing it to be explcitly unecypted via a snapshot | 18:33 |
| melwitt | well the glance image properties are not doing anything to request encryption, they're just describing the image. and on our side I think we should default to encrypted if the source is encrypted, with hw_ephemeral_encryption=false (our image prop, not glance's) being the way out of that if someone wants that | 18:34 |
| sean-k-mooney | melwitt: i can see that | 18:34 |
| sean-k-mooney | but in that case if the souce image is encypted | 18:34 |
| sean-k-mooney | when we boot the instance if encyption is not enable din the iamge or the flavor | 18:35 |
| melwitt | right | 18:35 |
| sean-k-mooney | we should write hw_ephemeral_encryption=true to the instance system metadata | 18:35 |
| sean-k-mooney | correct? | 18:35 |
| sean-k-mooney | to record that this instance is using encyption so we can schdule and so it is propagated by default to future snapshots | 18:35 |
| melwitt | it will be automatically (as image_hw_ephemeral_encryption=true) | 18:36 |
| melwitt | image properties are stored in system metadata already | 18:36 |
| melwitt | (meaning today, not only with the proposed patch series) | 18:36 |
| sean-k-mooney | well we just said hw_ephemeral_encryption was unset but the image was encypted | 18:36 |
| melwitt | oh, ok sorry. I see what you're saying now | 18:37 |
| sean-k-mooney | basically if someone creates an image with os_glance_encrypt_format=luks | 18:38 |
| melwitt | hm yeah, currently we are covered to maintain encrypted disks via BDM attributes but you're right that doesn't cover the scheduling piece | 18:38 |
| sean-k-mooney | and we boot a vm form that with a normal flavor with no extra spec | 18:38 |
| sean-k-mooney | we would need to populate image_hw_ephemeral_encryption=true and image_hw_ephemeral_encryption_format=luks | 18:39 |
| melwitt | yeah, I think I understand what you're saying. we wouldn't have the image prop nor flavor extra spec to make sure we're always scheduled to hosts that support local encryption | 18:39 |
| sean-k-mooney | yep | 18:39 |
| sean-k-mooney | so we proably dont need hw_ephemeral_encryption_format and could just use os_glance_encrypt_format if we will never suport format convertion | 18:40 |
| melwitt | yeah, ok. I think it would make sense to set image_hw_ephemeral_encryption=true in system metadata if we are doing it by implication from the source image. or else need some other way to deal with it | 18:40 |
| sean-k-mooney | ok its getting late for me and dinner/breakfest is calling. im not sure where we are on this topic. | 18:44 |
| sean-k-mooney | to me it was imporant to be able to take an unecypted image and create an encypted instance form that (vai selecting an encypted flavor) | 18:45 |
| sean-k-mooney | and by default i would expect snapshots form that encypted instance to stay encypted | 18:45 |
| sean-k-mooney | dan has a different usecase which is taking an encypted image and booting an encypted instance form that | 18:46 |
| sean-k-mooney | snapshots creates an intersection fo both usescases | 18:46 |
| melwitt | I think we all agree on that, it was only the idea of letting the user request or control the encryption format that was being debated | 18:46 |
| sean-k-mooney | well previosuly i agured that should not be exposed at all | 18:47 |
| melwitt | (agree on flavor hw:ephemeral_encryption=true|false) | 18:47 |
| sean-k-mooney | i.e. the format used should not be exposed ot the user. that was reintroduced this cycle for the lvm upgrae case | 18:47 |
| melwitt | ok, so it seems like you and dan agree about that then? provide only the boolean and don't expose a hw_ephemeral_encryption_format ... and the glance image format os_glance_encrypt_format is not related to that, it's only related to describing the image | 18:48 |
| melwitt | oh, you mean to support them converting lvm => luks? | 18:48 |
| sean-k-mooney | yes | 18:49 |
| melwitt | if so I forgot that was the reasoning | 18:49 |
| melwitt | ok, my bad if I forgot | 18:49 |
| sean-k-mooney | that why plain came back into the spec | 18:49 |
| melwitt | so I get your point is if we are ever going to support format conversions | 18:50 |
| melwitt | ok.. I thought plain was already in the spec but tbh I can't remember for sure | 18:50 |
| sean-k-mooney | it was pre zed | 18:50 |
| sean-k-mooney | and then in antelope and bobcat we removed it and said we woudl only implmeent luks | 18:51 |
| sean-k-mooney | and we have reverted back now | 18:51 |
| sean-k-mooney | caracal also only supproted luks | 18:51 |
| melwitt | ? I didn't think I changed anything regarding that from bobcat. I need to go look at it | 18:52 |
| melwitt | ok, if that's the case then I expect to need to revise the currently proposed spec | 18:52 |
| sean-k-mooney | i guess this is a delete between the libvirt and non libvirt spec version | 18:53 |
| melwitt | I do think maybe it could be a user pain point if we don't give them a way to go lvm => luks | 18:53 |
| sean-k-mooney | since the libvirt spec say " As highlighted at the start of this spec this initial support will only be for the LUKSv1 encryption format." | 18:53 |
| sean-k-mooney | melwitt: right so i was expecting that to happen via a live migrate or similar | 18:54 |
| sean-k-mooney | lvm -> luks | 18:54 |
| sean-k-mooney | so this is part of the probelm with having two specs for this | 18:54 |
| melwitt | yeah, ok. so maybe what it should say is that's what we support at this time and hw_ephemeral_encryption_format should only be introduced if we are adding the ability to convert formats | 18:55 |
| sean-k-mooney | the libvirt spec say we will only support luks | 18:55 |
| sean-k-mooney | right the parent spec says we can supprot plain but then that also implese we can have a diffent image vs instnace encyption format | 18:56 |
| melwitt | I think at the very least that would be an additional patch(es) later in the series. the intent was definitely to start with only luks | 18:56 |
| melwitt | yeah, I dunno if maybe I should consolidate the specs and just not have the libvirt one going forward? I dunno | 18:56 |
| sean-k-mooney | your right that the libvirt one has not changed for what its worth | 18:57 |
| sean-k-mooney | it has and still does say you would only support luks and the generic one says we will supprot muliple formants and has doen for multipel cycles | 18:57 |
| melwitt | ok. I'll go through it and make it clear that for now/for early patches it's luks only. and that hw_ephemeral_encryption_format is coupled with format changes and that may or may not be something we add on in later patches | 18:58 |
| sean-k-mooney | those two are in conflict since this is not being implemented in any other driver | 18:58 |
| sean-k-mooney | ok but then that also bring up the config option | 18:58 |
| melwitt | well, I guess (and we have comments about this on the spec) hw_ephemeral_encryption_format=plain could/would be a way to say "I want lvm encryption specifically" | 18:58 |
| sean-k-mooney | we dont need [ephemeral_storage_encryption]/default_format if its luks only | 18:59 |
| sean-k-mooney | melwitt: but we dont want to use that to request legacy lvm encyption right | 18:59 |
| melwitt | and the debate on that one is should the cloud (and only the cloud) dictate the format | 19:00 |
| melwitt | and that providing a choice of format is implying a mixed cloud, which afaik we don't officially support | 19:00 |
| sean-k-mooney | we supprot mixed clouds with diffent images_type backends in nova | 19:00 |
| sean-k-mooney | but we just dont support move operations between images_type backends | 19:00 |
| melwitt | yeah I think that's where the comments are at right now, is we don't want to assume a mixed cloud being possible and you get what you get depending on what cloud you're on and what it provides | 19:00 |
| sean-k-mooney | well its very common ot have some comptues use local storage and other use ceph | 19:01 |
| sean-k-mooney | wither that local storage is lvm or file backed (qcow/raw) is proably workload dependent | 19:02 |
| melwitt | ok. so if that's the case, then that would be the only real purpose of a hw_ephemeral_encryption_format | 19:02 |
| melwitt | I think that will be what drives the existence of hw_ephemeral_encryption_format -- whether or not we are saying we support a mix of lvm and luks hosts | 19:02 |
| sean-k-mooney | yes but landing on a host with the legacy lvm supprot would not be vlaid for hw_ephemeral_encryption_format | 19:02 |
| sean-k-mooney | hw_ephemeral_encryption_format should be the new feature exclively right | 19:03 |
| melwitt | there are placement traits for the format | 19:03 |
| melwitt | so we could schedule based on that if we had a mix | 19:03 |
| sean-k-mooney | right but im saying that COMPUTE_EPHEMERAL_ENCRYPTION_PLAIN does not imply lvm | 19:03 |
| sean-k-mooney | it impikes nova si cable of booting the instance with stoage that uses the dmcypt_plain encyption format | 19:04 |
| melwitt | it does in the universe of the local encryption feature as far as I understand it | 19:04 |
| melwitt | because I didn't think we were wanting to offer dm-crypt plain for anything new | 19:04 |
| sean-k-mooney | it could be a rbd volume mounted with dmcypt no? | 19:05 |
| melwitt | so the only thing COMPUTE_EPHEMERAL_ENCRYPTION_PLAIN could mean is lvm and [ephemeral_storage_encryption]enabled=true or whatever | 19:05 |
| sean-k-mooney | melwitt: so to me COMPUTE_EPHEMERAL_ENCRYPTION_PLAIN imples using the new feature which also impleis storign seperate secrets for each disk in barbican ectra | 19:05 |
| melwitt | that is not my understanding that we would be supporting rbd with dm-crypt plain | 19:06 |
| sean-k-mooney | the spec does not say you will implment it | 19:06 |
| sean-k-mooney | but takeing a step back and looking at the trait defintion that woudl be a vaild implmeation in nova | 19:06 |
| melwitt | I think (and I could be wrong because I never discussed this with Lee) but I think PLAIN was intended to be for retrofitting legacy (lvm) encryption into the new model | 19:06 |
| melwitt | and no longer use the config options for lvm encryption | 19:07 |
| sean-k-mooney | perhaps but i would not consider the legacy lvm supprot to be a valid impletation with regards to reporting COMPUTE_EPHEMERAL_ENCRYPTION | 19:08 |
| melwitt | the spec has always said "deprecate legacy lvm config and make lvm encrypt work with the flavor/image model" | 19:08 |
| sean-k-mooney | perhaps we would need to granfather it in for upgrade reasons | 19:08 |
| sean-k-mooney | but today if you use the legacy feature its not reflected in the bdms right | 19:08 |
| melwitt | yeah I think the intent was/is to consolidate the way in which local encryption is requested and scheduled and all that | 19:08 |
| melwitt | to make it all work the same way | 19:09 |
| melwitt | I'm not sure about lvm in BDMs, I have to double check | 19:09 |
| sean-k-mooney | yes but my understaindg is that it was replacing and "adopting" the existing lvm volumes | 19:09 |
| sean-k-mooney | i.e. we would back populate the bdms with the encyption flags and format info | 19:09 |
| melwitt | my understanding is that it also provides the mechanism for requesting lvm going forward | 19:10 |
| sean-k-mooney | so lvm encyption is not somethign you request today its config drivven only | 19:10 |
| melwitt | I think it's supposed to be all of that. replace/adopt + support creating new instances with lvm using flavor/image and no more lvm encryption config options | 19:10 |
| melwitt | right.. | 19:11 |
| sean-k-mooney | but the new lvm instance would not use the config options | 19:11 |
| melwitt | right | 19:11 |
| sean-k-mooney | we would use the flavor/iamge properties | 19:11 |
| sean-k-mooney | and unlike the adoptied ones | 19:11 |
| sean-k-mooney | each lvm disk would have its own key | 19:11 |
| sean-k-mooney | just like with qcow | 19:11 |
| melwitt | yes, that is what I thought the spec was intending | 19:12 |
| melwitt | to not have two separate ways of doing local encryption | 19:12 |
| sean-k-mooney | right | 19:12 |
| melwitt | and in the process lvm gains some new user friendly abilities as a result | 19:12 |
| sean-k-mooney | but that would only result in host tha tuse lvm reporting COMPUTE_EPHEMERAL_ENCRYPTION | 19:13 |
| sean-k-mooney | after we supprot upgrading the exiting instnaces | 19:13 |
| melwitt | ok. so all that said, my understanding was that for the current chunk we're trying to do now in dalmatian we are only doing luks and leaving lvm as the legacy config options at this time | 19:13 |
| sean-k-mooney | yep | 19:13 |
| sean-k-mooney | thats what i tought too | 19:13 |
| sean-k-mooney | so supporting rbd,raw/flat,qcow | 19:14 |
| sean-k-mooney | but not lvm | 19:14 |
| melwitt | ok. so it seems then that we would hold off on introducing hw_ephemeral_encryption_format until we work on the lvm support | 19:14 |
| melwitt | right | 19:14 |
| sean-k-mooney | and the exisitng lvm fature will be deprecate but not removed until we can adopt them into the new framework | 19:14 |
| sean-k-mooney | yes we would only need hw_ephemeral_encryption_format if we supprot somethign other hten luksv1 | 19:15 |
| sean-k-mooney | which also means if os_glance_encrypt_format is set to anything other then luks we shoudl reject that too right | 19:16 |
| melwitt | ok, I think we all agree about that. so I'll update the spec to explain this in a lot of detail 😂 and that hw_ephemeral_encryption_format will not be part of this first chunk and that it would come into play only when we work on adding support for lvm | 19:16 |
| sean-k-mooney | and do we agree the same is true for the config option | 19:17 |
| melwitt | I think it would be reasonable to reject that yes, if os_glance_encrypt_format is not luks. we won't be able to honor it | 19:17 |
| sean-k-mooney | ok | 19:17 |
| melwitt | I think I made the config option a choices= where the only choice is luks | 19:18 |
| melwitt | but I'll double check and make it that if it isn't already | 19:18 |
| sean-k-mooney | ok | 19:18 |
| sean-k-mooney | if its only luks then i dont think we need it personally | 19:18 |
| melwitt | that's a fair point. I suppose all it could potentially be helping with transparency but I could see the argument not to have it | 19:20 |
| opendevreview | Merged openstack/nova master: Enable OCaaS for several nova jobs https://review.opendev.org/c/openstack/nova/+/919738 | 19:20 |
| sean-k-mooney | i think what we are setteling on is we only need 1 extra spec/image property the boolean hw:ephemeral_encryption/hw_ephemeral_encryption | 19:20 |
| melwitt | that is also my understanding | 19:20 |
| sean-k-mooney | and if that is unset and the image in encyped then the vm would be encypted too | 19:20 |
| sean-k-mooney | meaning you dont have to opt in for encypted images | 19:20 |
| melwitt | yes, that's what I understand as well | 19:21 |
| sean-k-mooney | so there is only one last bit i want to check | 19:21 |
| sean-k-mooney | will we allow an opt out via the flavor i.e. hw:ephemeral_encryption=false and os_glance_encrypt_format=luks | 19:22 |
| sean-k-mooney | should that be a 409 conflict | 19:22 |
| sean-k-mooney | or decypt the image | 19:22 |
| melwitt | the main thing is I think we want to avoid an unpleasant surprise on that. similar to the encrypted rescue image, not to just decrypt it | 19:22 |
| melwitt | I think the desire based on current code review comments is that it would decrypt in that case | 19:22 |
| sean-k-mooney | personally im feeling like hw:ephemeral_encryption=false and os_glance_encrypt_format=luks woudl be a 409 conflict at least for now | 19:22 |
| melwitt | yeah, I think it would be fair to start with that | 19:23 |
| sean-k-mooney | the reason i hesitate there is it would allow anyoen in the same project (realy that has access to the image secret) to trivially/acidentaly decypt the image | 19:23 |
| melwitt | and tack on a separate patch that enables the ability to request decryption | 19:24 |
| melwitt | because it's like a user experience thing. not really "required" but nice to have to give users that ability | 19:24 |
| sean-k-mooney | its not uncommon for peopel to query flavors by size and select the first one that they find | 19:24 |
| melwitt | yeah. that would be my only concern with it as well | 19:24 |
| sean-k-mooney | to be fair if thte image had os_glance_encrypt_format=luks hw_ephemeral_encryption=ture | 19:25 |
| sean-k-mooney | i think we all agree it should be a 409 conflict if the flavor has hw:ephemeral_encryption=false | 19:25 |
| sean-k-mooney | but since the arguement is being made that os_glance_encrypt_format=luks effectivly implies hw_ephemeral_encryption=ture | 19:26 |
| sean-k-mooney | im wonderin if we shoudl also imply it when comparing ot a flavor with hw:ephemeral_encryption=false | 19:26 |
| melwitt | I expect we all agree that's an ok first step. we've been trying to keep patches smaller in this thing as well so I have been splitting smaller things like that into their own patch | 19:26 |
| sean-k-mooney | ok | 19:26 |
| sean-k-mooney | so before this converstaion i was tending to say "ok ill approve the spec and we can take it to the implemeation" no i feel like we were not on the same page but have got much closer | 19:27 |
| sean-k-mooney | so my stomach say i shoudl have breakfast/dinner | 19:27 |
| sean-k-mooney | but i also dont really want to leave this open | 19:27 |
| sean-k-mooney | so melwitt how do you want to proceed | 19:28 |
| melwitt | yeah so I was thinking if os_glance_encrypt_format=luks (and really mainly just the presence of os_glance_encrypt_key_id) then if hw:ephemeral_encryption=false is in direct conflict and would act the same as the image conflict. until we offer decryption later if we do that | 19:28 |
| sean-k-mooney | ack i think that is logicaly consitent | 19:29 |
| sean-k-mooney | i feel like os_glance_encrypt_key_id is only valid if all the other os_glance_encrypt_* are dfeind | 19:30 |
| melwitt | sean-k-mooney: I think it would probably be best to update the spec now. either way I will update it -- if it gets approved as-is I will still have an amendment proposed shortly. to not forget all this stuff | 19:30 |
| melwitt | so either way is fine with me | 19:30 |
| melwitt | I agree. I think that is the intent in the standardized properties anyway | 19:30 |
| sean-k-mooney | so its not clear to me if we shoudl be checking any spefici one of those keys or if any of them exist | 19:30 |
| melwitt | yeah.. I'll probably add validation for that just to make a better experience. I'm not sure if they are all required to be set from a glance perspective. like, will glance reject upload of an image that only sets one or a subset of them? or maybe if it does allow it, it will fill it defaults itself | 19:32 |
| melwitt | so either way on the nova side we would expect/require them all to be set if one is set | 19:32 |
| sean-k-mooney | so the glance spec used ot have a seperate nova spec https://review.opendev.org/c/openstack/nova-specs/+/608696/11/specs/train/approved/image-encryption.rst | 19:32 |
| sean-k-mooney | for how we would use those | 19:32 |
| melwitt | from my perspective at a minimum if os_glance_encrypt_key_id is set, then os_glance_encrypt_format is required | 19:33 |
| sean-k-mooney | in that they explictly talk about adding supprot for decypting the encyped image when booting the instnace | 19:33 |
| sean-k-mooney | melwitt: ya i think those two are requried and likely the type as well i.e. symetic | 19:33 |
| melwitt | ah ok | 19:34 |
| sean-k-mooney | having said that i think we are only going to support symmetric encption | 19:34 |
| melwitt | ah right | 19:34 |
| sean-k-mooney | so i guess if its single valued | 19:34 |
| sean-k-mooney | it does not matter for now | 19:34 |
| sean-k-mooney | melwitt: you know what would help | 19:36 |
| melwitt | yeah, we can adapt that as we go along. if we think more than just those two should be required to be present to be valid | 19:36 |
| melwitt | because the other use case is an individual user uploading an encrypted image | 19:36 |
| sean-k-mooney | in your spec can you define the only values of the os_glance_encrypt* properties that will actully work for now | 19:36 |
| melwitt | so not sure if they will be required to provide all image properties | 19:36 |
| sean-k-mooney | like will you be using os_glance_decrypt_size? | 19:37 |
| sean-k-mooney | i dont think you are your using virt_size + 1G in the check right | 19:38 |
| melwitt | sure I can add specifics about which ones we are requiring | 19:38 |
| melwitt | there is a check for that somewhere actually | 19:38 |
| sean-k-mooney | well it might hjsut be good to say hay nova is not using os_glance_decrypt_size | 19:38 |
| melwitt | today we validate image size in some way and I had to add the "plus overhead" to the calc if encrypted for it to pass | 19:39 |
| sean-k-mooney | right | 19:39 |
| sean-k-mooney | but if i rememebr reviewing that code we do not use os_glance_decrypt_size | 19:39 |
| melwitt | yeah I guess really i could use os_glance_decrypt_size instead for the validation if the image is encrypted. since that's what we actually care about, is if the decrypted size will fit in the chosen flavor | 19:40 |
| melwitt | no we didn't have os_glance* properties proposed at the time | 19:40 |
| sean-k-mooney | os_glance_decrypt_size and os_glance_decrypt_container_format i think are for the usecase of booting form an encypted iamge and decypting it on the host | 19:40 |
| melwitt | well, sorry | 19:40 |
| sean-k-mooney | which would imply they expect the virtual_size to include the overhead | 19:41 |
| melwitt | they were proposed already but we were not planning to use them initially | 19:41 |
| sean-k-mooney | when its encypted | 19:41 |
| melwitt | but at the ptg we agreed to use them so I will be changing where needed to use those instead of what I had put | 19:41 |
| sean-k-mooney | sure but os_glance_decrypt_size is defied as "size after payload decryption" | 19:41 |
| sean-k-mooney | which would be the size of the data insid ethe luks encyption | 19:42 |
| sean-k-mooney | not the size on disk fo the encypted images | 19:42 |
| sean-k-mooney | so i dont think it actully helps us | 19:42 |
| melwitt | right.. maybe I'm getting confused | 19:42 |
| sean-k-mooney | i think this is explcitly for the usecase of booting non encypted instance form an encypted image | 19:43 |
| sean-k-mooney | so that we woudl know how big it is after we strip the encyption from it | 19:43 |
| melwitt | the point I was meaning really is I'll use them when applicable | 19:43 |
| sean-k-mooney | ack | 19:43 |
| sean-k-mooney | ok ill chat ot you tomorrow o/ | 19:44 |
| melwitt | ok I see what you're saying | 19:44 |
| melwitt | ok, seeya tomorrow o/ | 19:44 |
| opendevreview | Merged openstack/nova master: Enable virtio-scsi in nova-next https://review.opendev.org/c/openstack/nova/+/918458 | 21:41 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!