| kevko | anybody for second +2 https://review.opendev.org/c/openstack/oslo.messaging/+/928034 < ? | 07:51 |
|---|---|---|
| opendevreview | Takashi Kajinami proposed openstack/oslo.db master: Remove LegacyEngineFacade https://review.opendev.org/c/openstack/oslo.db/+/798135 | 14:01 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Differentiate raw-like images https://review.opendev.org/c/openstack/oslo.utils/+/930365 | 17:14 |
| JayF | ^ Happy to talk about other ways to signal this; the addition of GPTInspector broke all our draft Ironic code to use imageutils from oslo_utils | 17:14 |
| JayF | and it seems like us keeping a static list would be the most painful thing possible in terms of future-proofing | 17:15 |
| JayF | Clark discovered this https://bugs.launchpad.net/oslo.utils/+bug/2081872 while looking at that change, too. | 17:29 |
| tkajinam | the whole "you should validate image format outside of qemu tooling" pulls tons of mess... | 17:31 |
| tkajinam | I know you guys know it better but I couldn't stop saying it :-P | 17:31 |
| JayF | I mean, yeah. It's been a very unpleasant set of time working through these issues, and I think Ironic devs got one of the less annoying versinos of this fix to write | 17:33 |
| JayF | but I'm really nervous if we're saying a GPT image isn't raw, that makes that inspector really scary for Ironic use unless we do something like I've proposed there | 17:33 |
| tkajinam | I wonder if it saves our life to allow disabling gpt inspector now. I know it has benefit but we may still face some wired variations we should consider "normal" | 17:33 |
| JayF | If people want to identify subsets of raw, I'm game for that, and we might even be able to do cool things with that info in the future | 17:33 |
| JayF | but we just have to clearly indicate it's a subset | 17:34 |
| JayF | (where raw == an image that can be written bit-perfect to a disk and make sense) | 17:34 |
| JayF | (I guess I should say disk or partition) | 17:34 |
| JayF | it makes me wonder, there must be some code around this in nova already, right? | 17:36 |
| JayF | because I don't think qemu-img can take gpt as an input type | 17:36 |
| JayF | hmm | 17:36 |
| JayF | https://opendev.org/openstack/nova/src/branch/master/nova/virt/images.py#L164 is basically *exactly* what I didn't want to write in ironic | 17:41 |
| JayF | because I can guarantee we'll be forgotten about if another exception is added, and even if not, it's not awesome to have multiple projects have to update before the library can update :/ | 17:41 |
| tkajinam | dansmith, ^^^ fyi | 17:52 |
| tkajinam | I'll revisit it hopefully tomorrow (well, it's already today, though) | 17:53 |
| JayF | looking at the nova code, we could probably centralize if we added iso to be "raw-like" as well, and Ironic could happily filter that in the deployment layer (isos make sense for only some of our deployment interfaces, but these distinctions don't exist for actual raw/gpt disk images) | 17:53 |
| dansmith | JayF: I'll look at your patch, but I think we definitely need to stop treating "a raw disk image" as the same thing as "I don't know what this format is" | 17:55 |
| dansmith | JayF: the "if gpt, then raw" thing is what we need until we can transition to a known format of course | 17:55 |
| JayF | In theory I agree, but I am highly worried about edge cases that might exist only for hardware | 17:55 |
| dansmith | I think we have to bite that bullet | 17:56 |
| JayF | e.g. I've actually known folks who used Ironic to provision images onto switch gear, and I wouldn't be surprised if the "image" in that case wasn't something we'd be able to detect | 17:56 |
| dansmith | and the qemu team has recommended this route | 17:56 |
| dansmith | right, and I think it's correct for nova to say "sorry but I dunno what that is so I won't even try to boot it" | 17:56 |
| JayF | I am fine with Nova saying that; my concern is with ironic's ability to use the shared format inspector code | 17:56 |
| dansmith | if ironic wants to allow bootable raw (in the "I don't know what this is sense") then that's cool, | 17:56 |
| dansmith | but I think nova cannot do that | 17:57 |
| dansmith | JayF: so all you have to do is allow raw, full stop | 17:57 |
| JayF | because, as noted above, Ironic had code written that checked that | 17:57 |
| dansmith | and glance will, of course, continue to allow ra | 17:57 |
| JayF | GPTInspector was added | 17:57 |
| dansmith | *raw | 17:57 |
| JayF | and it broke because that's "gpt" not "raw" | 17:57 |
| dansmith | right, well, it's not :) | 17:57 |
| TheJulia | well, realistically it is | 17:58 |
| JayF | I doubt we'd have any users who would use the term "raw image" to mean something *excluding* something that can be gotten via dd if=/dev/my/disk of=/file | 17:58 |
| TheJulia | gpt is just an underlying data structure which is block aligned with a flavor of partition data. | 17:59 |
| JayF | which basically if I have a BIOS booted MBR disk: dd if=/disk of=/image ## returns raw, if I have a EFI-booted GPT disk: dd if=/disk of=/image # returns gpt | 17:59 |
| JayF | which is expectation violating for me, at least | 17:59 |
| JayF | to be clear: I think it's cool that we can say "this raw image contains GPT partition data", and I can even think of cool ways Ironic could use that information, but it's *additional* info beyond 'this is a raw image' (using the definition of raw image we've had for ~10 years+) | 18:00 |
| dansmith | JayF: you just said you would have users using raw for something other than that | 18:00 |
| dansmith | JayF: the definition of raw that we've had for 10+ years is exactly the problem though | 18:01 |
| JayF | My intention in saying that is that I want to assume it's a breaking change if we start saying "images that we can't detect a partition table inside are not permitted to deploy" | 18:01 |
| dansmith | we can redefine raw to mean "has some structure that looks like X" if you want, but then we need to start rejecting people uploading kernel images and calling them raw | 18:01 |
| JayF | well, my concern is reverse of that: us shrinking what "raw" means can have a negative impact on ironic | 18:01 |
| dansmith | well, right now we have both structured things we expect (i.e. a PC disk image) and things like kernel, ramdisk and anything else (switch firmware image?) in raw images | 18:02 |
| dansmith | anything we do here is going to be disruptive to multiple communities for sure, but I don't think that's avoidable | 18:02 |
| JayF | I think the overlap of "multiple communities" is going to be giant in the BM case, and much less so for the VM case | 18:03 |
| dansmith | and TBH, I think it's an indication that we're moving in the right direction (i.e. side effects prove the vaccine is working) | 18:03 |
| dansmith | are you arguing that we should continue not tracking the actual content of the images because the risk is worth the pain to change? or are you proposing some alternate shift in the definitions? | 18:04 |
| JayF | which is part of why this is a bit scary from my perspective; going from "we detect what type of image it is so we can run tools safely on them" and "lets redefine the basic definition of what these terms that our users have been talking about for years" is a huge leap | 18:04 |
| JayF | my patch, above, adds a property to the inspector | 18:04 |
| JayF | which indicates if an image type makes sense as to be bit-copied onto a disk | 18:04 |
| JayF | e.g. right now, GPT, Raw, and (maybe) ISO would be the ones to get it returning True today | 18:05 |
| JayF | if at some future date, we detect filesystems inside images, that might get tagged with that too | 18:05 |
| JayF | or maybe even images with preexisting MD RAID metadata on them (I did this at OnMetal like 10 years ago) | 18:05 |
| JayF | the scope of what you can put on a disk to do interesting things is MUCH larger in a bare metal world | 18:06 |
| JayF | my goal is that if we start saying, "hey, this image we used to call 'raw' is now 'vfat'" that we have a way to signal that to downstreams, like Ironic, which want to know it can still treat it as a bit-perfect image that doesn't need conversion | 18:06 |
| opendevreview | Merged openstack/oslo.utils master: Avoid detecting FAT VBR as an MBR https://review.opendev.org/c/openstack/oslo.utils/+/928448 | 18:07 |
| JayF | lol thanks opendevreview for the timely comment :P | 18:07 |
| TheJulia | quite timely | 18:07 |
| JayF | really I toyed with calling that variable "conversion_needed" or similar | 18:08 |
| JayF | which might be a better way to describe it | 18:08 |
| JayF | but I took the sign I'm having trouble naming the property as a sign that maybe it needed a finer point on what exactly we care about | 18:08 |
| JayF | dan is right the "everything else" nature of how we treat raw makes it hard, after the fact, to figure out what goes in what bucket | 18:08 |
| dansmith | I think what you want is a a list of formats you think you can write to a disk | 18:08 |
| JayF | yes, pretty much | 18:08 |
| JayF | image_format.just_dd_this (bool) lol | 18:09 |
| dansmith | I don't think a property on the inspector is the right thing to do there, but a list of those that you can use and we can ignore would serve the same purpose | 18:09 |
| JayF | with maybe separate properties for filesystem stuff (dd this to a partition) vs partition image / raid metadata / etc (dd this to a disk) | 18:09 |
| dansmith | I dunno that it needs to be in oslo, because it's ironic policy really, but... whatever | 18:09 |
| JayF | I implemented it that way first, deleted it, and redid it my current way | 18:09 |
| JayF | well, I'm mainly worried that we'd get silently broken, similar to how we did when GPTInspector was added (that's a weird case as it was code under review that got broken, but I'm taking it as a timely warning to be careful about this integration point) | 18:10 |
| dansmith | JayF: the VFAT thing is a good example of why that's hard though.. that format literally can be whole-disk or partition.. it has the same structure so that it's bootable from a whole disk (floppy) or a partition (chainloaded from the MBR ) | 18:10 |
| dansmith | intentionally that way because 1980s | 18:11 |
| JayF | yep, I agree it's super hard to define, but I think it's a case where if we kick the problem down the road the first complaint will be from an operator, and I'd rather try to figure it out before that bug gets filed :) | 18:11 |
| dansmith | also, the firmware in the machine could totally support qcow2 written raw to a disk without any hypervisor at all, so saying you can't dd it to the disk is also not really accurate | 18:11 |
| opendevreview | Merged openstack/oslo.utils master: Squelch irrelevant format complaints https://review.opendev.org/c/openstack/oslo.utils/+/930074 | 18:11 |
| JayF | can we delete that from the logs before some hardware vendor gets ideas /s /s | 18:12 |
| JayF | lol | 18:12 |
| JayF | Yeah, that's what I'm stuggling with too, you're right that in a way it's a policy statement | 18:13 |
| dansmith | yeah, so I'd put that into ironic for that reason personally, because nova will have some of its own things, | 18:13 |
| JayF | but I don't know how to put that policy statement in ironic *without* having to worry about a rugpull breakage by new inspectors being added | 18:13 |
| dansmith | like you can't boot iso by dd'ing it directly to a disk, but if we know it's iso we can do the right thing.. and we won't allow raw | 18:13 |
| JayF | (this patch was as much to get this conversation as it was to merge; I'm happy to find better solutions) | 18:13 |
| dansmith | and libvirt doesn't allow vmdk, etc | 18:13 |
| dansmith | JayF: well, InspectWrapper (changed since you copied from glance) allows an "list of inspectors" so you can easily provide it with a list and it won't detect new stuff | 18:14 |
| JayF | so a question: if we did that, and say, we found some security issue in the GPTInspector (like, a specically formed partition metadata can cause your disk to kaboom) -- yes I know it's a stretch | 18:15 |
| dansmith | so having your list of allowed things passed to it will cause it to detect anything not in that list as RawInspector such that if we add a BsdLabelInspector (which we probably need) you won't start seeing it before you expect it | 18:15 |
| JayF | if we didn't pass GPTInspector as a possibility, it'd never run that safety check | 18:15 |
| JayF | ^^^ That is EXACTLY why I want to have this conversation -- because I know the tail of things to add will likely be long | 18:15 |
| dansmith | yep, well that's exactly why we need to stop pretending this stuff is unstructured in the first place :D | 18:16 |
| TheJulia | I personally think locking projects into patterns that never change is just building technical debt disguised as stability. There has to be a middle ground. | 18:16 |
| dansmith | because I think there are probably theoretical attack vectors where the complex GPT (the actual one, not what we're checking) is exploitable with string buffer overflows and such | 18:16 |
| dansmith | TheJulia: isn | 18:16 |
| dansmith | TheJulia: isn't that "raw has always been like this so it will always be like this" ? | 18:17 |
| JayF | TheJulia: one thing I like about Ironic right now is with whole disk images, you can basically use an image of some homegrown OS written in a completely different architecture as long as the machine boots it | 18:17 |
| JayF | TheJulia: it's going to be hard to fix dansmith's concern with losing that flexibility | 18:17 |
| JayF | dansmith: YES | 18:17 |
| JayF | dansmith: re: GPT scariness | 18:17 |
| JayF | I made myself scared with just that hypothetical | 18:17 |
| dansmith | JayF: it's really not if you let the op control the blessed list and/or let the driver define the formats it can take (i.e. a rando dell server says "no thanks" to qcow2) | 18:18 |
| dansmith | we had to do that with nova because vmware won't read qcow2 files and of course our users are better off because of it | 18:18 |
| dansmith | JayF: that's exactly why I want nova to NOT boot anything called "raw" because all that means is "shrug, good luck" | 18:18 |
| JayF | The hardware vendors don't even support the things they say they support, much less cool stuff like that (/me glares at uefihttp boot) | 18:18 |
| TheJulia | Just support modeling on larger device block sizes and I'll be happy w/r/t gpt | 18:19 |
| dansmith | well, letting them define the things they want to support (or you of the things you want) would be a good way to avoid people saying "it's broken, but it kinda works, so fix it!" | 18:19 |
| TheJulia | JayF: just remember, your only glaring at *one* of the two UEFIHTTP boots ;) | 18:19 |
| JayF | TheJulia: part of me wonders if part of the fix for this is the other direction | 18:19 |
| TheJulia | They have similar outfits, entirely different jackets | 18:19 |
| JayF | nevermind | 18:19 |
| JayF | I typed in a bad idea and deleted it | 18:20 |
| TheJulia | heh | 18:20 |
| dansmith | TheJulia: you need to explain the block device thing to me (on the bug) because we're not *actually* processing the GPT structures at the moment | 18:20 |
| dansmith | so there should be no reason there's a problem there unless you've got an invalid GPT | 18:20 |
| JayF | dansmith: on a 4k sector size disk, the GPT metdata is at 4k, not 512 | 18:20 |
| TheJulia | and each block is 4k | 18:20 |
| dansmith | JayF: cool, but we don't even look at it, so why does that matter? | 18:20 |
| JayF | dansmith: and there is hardware that exists which can't even read a 512 block size GPT partition | 18:20 |
| TheJulia | and so per gpt standard, the *first* block of data is in block 1, of a 4k block size | 18:20 |
| dansmith | there's no error or log in the bug so I don't even know what the problem is | 18:21 |
| dansmith | TheJulia: sure but the MBR is only 512 bytes and since that's all we look at there should be no issue | 18:21 |
| TheJulia | dansmith: issue we've got customers screaming about is the hardware looks at block 1 per gpt, because it is block aligned, not byte aligned, and reads the contents, and doesn't see the gpt table header | 18:21 |
| dansmith | and we're reading from a stream, so the reads aren't aligned or anything | 18:22 |
| dansmith | TheJulia: we're not reading the GPT header AT ALL | 18:22 |
| TheJulia | then what are you reading, exactly? | 18:22 |
| dansmith | we're capturing it (so I can look at samples) but we're not doing anything with the data | 18:22 |
| dansmith | we could remove that line and nothing would change | 18:22 |
| dansmith | notice the only check on there is looking at the PMBR structures, which is only the first 512 bytes | 18:23 |
| dansmith | who is screaming and where? | 18:23 |
| JayF | specifically clark pointed it out when he was looking at my change | 18:23 |
| TheJulia | dansmith: we have some customers internally which are screaming to boot baremetal which only groks 4k block sizes, so gpt structure has to be differently aligned on the disk image contents | 18:23 |
| TheJulia | otherwise the hardware doesn't see it. This can be reproduced with VMs by forcing the disk device block size to 4k | 18:24 |
| dansmith | are you all just theorizing that there's some problem with the assumption that it's at 512 bytes there or experiencing some actual problem? | 18:24 |
| JayF | https://github.com/openstack/diskimage-builder/commit/2df6d9b91415241b9d16cd96dd78f9b15e4e8e26 is the code added to DIB to create images that can be read on that hardware | 18:24 |
| JayF | dansmith: the first piece. We should build an image with ^^^ that can point the inspector at it and see what happens | 18:24 |
| JayF | I'm going to JFDI that now | 18:25 |
| TheJulia | I *think* pmbr is also optional | 18:25 |
| dansmith | TheJulia: it's not | 18:25 |
| dansmith | https://uefi.org/specs/UEFI/2.10/05_GUID_Partition_Table_Format.html | 18:25 |
| dansmith | "LBA 0 (i.e., the first logical block) of the hard disk contains either" ... a real MBR or a PMBR | 18:25 |
| * JayF wonders if the MBR itself is 512 bytes or 4k in the case of a 4k disk... | 18:26 | |
| TheJulia | may contain a legacy MBR pointer *or* PMBR | 18:26 |
| dansmith | JayF: ack, if something about it breaks let's discuss | 18:26 |
| TheJulia | JayF: first 512 bytes of a 4k block | 18:26 |
| TheJulia | the rest ends up being whitespace until LBA1 | 18:26 |
| dansmith | right ^ | 18:26 |
| dansmith | yup | 18:27 |
| JayF | so then it seems like the code as written is probably A-OK based on my re-reading with Dan's enlighting comments | 18:27 |
| JayF | but if we go the next step to inspect GPT structures, it'll need more logic for block sizes | 18:27 |
| TheJulia | if just looking for pmbr likely | 18:27 |
| dansmith | my bad for leaving the red herring in the code :) | 18:27 |
| JayF | It doesn't help that I'm specifically reviewing for "how will this break on real hardware" :D | 18:28 |
| dansmith | your bad for falling into the trap | 18:28 |
| JayF | exactly lol | 18:28 |
| TheJulia | this reminds me, I started the work someplace to wire us up a 4k block device CI job | 18:28 |
| TheJulia | I don't remember where I put those brain cells | 18:28 |
| TheJulia | so, since it is only looking at the pmbr, and you can have an mbr or pmbr on gpt partitioned disk, it basically means ironic is going to have to still grok "its a disk, its bootable" somehow becaue we can't change how we handle that as operators today *have* dual boot images | 18:31 |
| dansmith | I think you mean s/can have/must have/ if you're talking about gpt | 18:32 |
| TheJulia | yes | 18:32 |
| TheJulia | that | 18:32 |
| dansmith | just checking :) | 18:32 |
| dansmith | if ironic wants to make raw images bootable, then that's totally fine for ironic | 18:33 |
| TheJulia | I'm on conversation number $ERROVERFLOW | 18:33 |
| JayF | I also will abandon my oslo.utils change; I'll write code downstream in ironic similar to what nova has | 18:34 |
| JayF | just please make loud noises if/when that changes :D | 18:34 |
| dansmith | JayF: I just -1d with a summary here, hope that's okay | 18:34 |
| dansmith | good for posterity at least | 18:34 |
| JayF | I would've done it before abandoning it | 18:34 |
| dansmith | ack | 18:34 |
| JayF | my memory is pretty bad so I try to ensure conversations are documented | 18:34 |
| dansmith | tkajinam: thanks for ninja'ing in those fixes | 18:56 |
| JayF | btw dansmith, I whipped up http://github.com/jayofdoom/disk-image-checker based on the oslo.utils code -- if you know of any operators who might need something like that | 19:06 |
| dansmith | ah cool, I have something local I've been meaning to push up | 19:06 |
| dansmith | but making it like blkinfo is cool | 19:06 |
| JayF | this was mainly whipped together for the video I did on the ironic security issue | 19:06 |
| JayF | and so my downstream could be 10000% confident their images would pass muster :D | 19:06 |
| opendevreview | Dan Smith proposed openstack/oslo.utils master: Clarify GPT structure offset https://review.opendev.org/c/openstack/oslo.utils/+/930374 | 19:06 |
| dansmith | JayF: TheJulia ^ | 19:07 |
| dansmith | JayF: yeah, I think a tool to run safety checks on an image is something we should have | 19:07 |
| JayF | I'm happy to propose that, or a similar modified version, to oslo.utils if it'd be desired | 19:08 |
| dansmith | if you're really going to put that thing on pip that's fine | 19:08 |
| JayF | my idea was to put it on pip if anyone gave a crap | 19:08 |
| dansmith | mine is left over from me writing this code years ago, and dumps all kind of info to make sure I'm not storing the whole image in memory, etc | 19:08 |
| JayF | if you're saying you give a crap, I'll setup proper publishing and CI for it :) | 19:08 |
| dansmith | since this was for glance to use in a stream without storing the image, the "don't just store the image in a variable" was an important design point | 19:09 |
| JayF | and make it actually install a binary instead of being a python -m blah | 19:09 |
| dansmith | JayF: I give a crap.. one thing though | 19:09 |
| dansmith | you might want to consider making it so you can install it without the oslo.utils dependency, or at least without it being too opinionated about the version | 19:10 |
| JayF | 4k image validates as GPT, jfyi https://www.irccloud.com/pastebin/3Ctea18L/4k_image_out.txt | 19:10 |
| dansmith | because what some people will really want is "does *my* oslo.utils think this is safe/allowed" | 19:10 |
| JayF | I think I have => the first version with a format inspector as the requirement | 19:11 |
| JayF | if that's not the case I'll make it that way before publishing | 19:11 |
| dansmith | okay, but just try to keep it tolerant of future versions i mean so people can use it with multiple things | 19:11 |
| dansmith | also, | 19:11 |
| dansmith | I mentioned to tkajinam at some point that I had a messy thing to include, | 19:11 |
| dansmith | and I think he was open to allowing `python -m oslo.utils.imageutils` usage, | 19:12 |
| dansmith | so if you did that, people could test their even-distro-packaged version without extra stuff | 19:12 |
| dansmith | but anyway, whatever you want to do, but I think it's good to have regardless of where you put it | 19:12 |
| JayF | if we're OK including it in oslo.utils, I'm going that route | 19:13 |
| JayF | the reason I didn't push to pip already is that it's an anchor to support it forever or be a meanie and let it rot, and I'm bad at both lol | 19:13 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 19:44 |
| JayF | there ya go dansmith, I can drop the pbr dep if we want to but I thought it'd be a /very/ nice touch to print the version in the output for future bugs | 19:45 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 19:45 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 19:51 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 19:51 |
| JayF | and my favorite kinda change for my own version: https://github.com/jayofdoom/disk-image-checker/pull/6 (deleting it all with a message to look in oslo.utils) | 19:54 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 20:36 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 20:40 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 22:34 |
| opendevreview | Jay Faulkner proposed openstack/oslo.utils master: Add image checker to imageutils https://review.opendev.org/c/openstack/oslo.utils/+/930379 | 22:38 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!