Thanks for the review On 14/11/2018 00:35, Dimitri John Ledkov wrote: > > In passes, how does one specify multiple branches of stacks? Or is only a linear stack allowed by this code? This code only allows linear stacks to avoid not make current code more complex than it already is and diverging too much from existing logic. Our approach is to define the stacking defining the live image in PASSES and put the extra logic like “branched mounts” in hooks. For instance with langpacks which is the use case for Desktop image, we also have multiple branches of stacks. It is difficult to represent the structure in a generic way and it adds extra complexity to the maintenance of those definitions. Following your proposal to represent the layers structure as a list, it would be something like: PASSES=“desktop-minimal:desktop:live desktop-minimal:lang-neg-min-fr desktop-minimal:lang-neg-min-de desktop-minimal:desktop:lang-neg-fr desktop-minimal:desktop:lang-neg-de …” We thus have multiple “base” here, “desktop-minimal” or “desktop-minimal:desktop”. Shortening the syntax doesn’t seem appropriate here. In a tree, it’s not better: - desktop-min - desktop - Lang-neg-de - Lang-neg-fr - Lang-neg-es - … - Live - Lang-neg-min-de - Lang-neg-min-fr - Lang-neg-min-es - … Addition or removal of any default language would be then error-prone. This is to compare with a hook, where we just loop over “desktop-min” and creates langpacks negative stacks, and then looping over “desktop” as well to achieve the same. Besides, shell seems inappropriate to implement this type of logic. Note that the current implementation is similar than existing ubuntu-server:live logic (just a little bit more generic) and we made sure we didn’t break your use case. Note that though, you will be able to remove your first hook, creating the “live” stack using the generic code right now. However, the maas-* stacks will still be in hooks. > > Cause the current subiquity images do not have `live` as the top of the stack.... They do this: > > +--> Base +-----> Live > | > +-----> Rack +---> Region > > Because I am expecting to have the ability to somehow specify what each pass depends on. But it looks like the base for a subsequent pass, is always the previous one?! > > > It would be nice to extend PASS syntax to optionally accept an arbitrary `base`, e.g. > > PASSES="base rack region base:live" to encode the above graph, and such that `base:live` pass uses base as the lowerdir, instead of region (i.e. the previous pass result). > > Or something... > > not sure if : is acceptable pair delimiter here, or not. > > Also not sure if we want to always enforce specifying `base` such that we can construct multiple root nodes in one go. E.g. `base base:rack rack:region base:live` or `:base rack region base:live` for the above graph. > > Or like maybe always list all layers?! but that violates donot-repeat-yourself principle... E.g. `base base:rack base:rack:region base:live` > > === > > When calling includes/hooks are they aware which pass they are for? what is the source dir for each pass, for e.g. binary.includes? I'm guessing that PASS variable is set, but not sure. > > For source hooks, the PASS variable is not set currently. It’ll be exported so hooks can either run for all passes, or just some specific ones, without an additional directory structure. For binary hooks and includes, we don’t see this specific need yet, and only apply it to the last PASS. This can be introduced later on (creating lb_binary_layered like lb_chroot_layered), once we have a real use case. > === > > filesystem.squashfs is somewhat is a special name, so it would be nice to keep that as the base one. And also possibly adjust logic in casper as to what it mounts by default.... cause e.g. i think i hide maas squashfes in a subdir, to prevent casper from mounting those, which is kind of a hack. I wonder if we do need to write out the valid stacks (passes?!), which casper can then use to boot to whichever stack is valid. With subiquity image this could then result in "Live Server, Live MAAS Rack, Live MAAS Region, Live Server with Installer" boot options. As example, for better or worse. > The proposed change uses the existing casper logic (layers are mounted in alphabetical order) without any changes. Casper doesn’t special case filesystem.squashfs and I didn’t find any reference to it in the code. We are not really in favor of special casing filesystem.squashfs. In addition to this, remember that we have multiple “base” squashfses. Your specific requirement of multiple boot options seems independent of base layer naming, and should be addressed separately. > > ==== > > No idea if SUBPROJECT and IMAGE_FORMAT are the right things to extend for this..... and if they are easily extendable like this in launchpad livefs builders & ubuntu-cdimage codes. > > Cause I can see the potential for using layers in SUBPROJECT=minimized, if for example, cpc builds are converted to layers they would have full and minimized layered builds.... and SUBPROJECT=minimized-layered sounds ugly =) > Open to suggestions. We are adapting ubuntu-cdimage in a separate MP to download all new artefacts. > ==== > > manifest diffs for layers is nice; cause in cpc we have struggled to consitently represent manifests / changelogs of "it's just like that image, but has this stuff on it" > Ok, thanks. > > ==== > > packaging layers as actual static filesystems might be interesting, but i guess hooks will be able to do that anyway. Can you expand your comment? > > ==== > > Overall, this looks ok, and shouldn't break any existing stuff - as long as we can clear the top level new extensions of > > SUBPROJECT=layered > IMAGEFORMAT=live-layered > > and that needs like an architect review. > Thanks again for the review, could an architect review it too? -- Jean-Baptiste Lallement irc: jibel