Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce new mechanism of composing rootfs templates #2626

Closed
wants to merge 1 commit into from

Conversation

zededa-yuri
Copy link
Contributor

@zededa-yuri zededa-yuri commented May 13, 2022

This is preparation for introducing the development builds. The main goal of the patch is to be able to combine different build flavors together, by sequentially applying patches to the rootfs Configuration.

At the moment the final rootfs configuration is built by applying patches on the base configuration. To instruct the build to mutate rootfs, the user needs to pass the HV variable to the make. For example

   make HV=acrn #to build the acrn-based Eve
   make HV=zfs-kvm #to build kvm-based with zfs as default file system

Currently, the build system would look under the images folder for the corresponding patch. For example images/rootfs-acrn.yml.in.patch. This means for each build flavor we want to support we need a separate *.patch file.

This becomes a problem once we need to mix flavors. Particularly, with upcoming “development” build flavor, which is orthogonal to the hypervisor type and can be applied to kvm, xen and acrn. If we keep this approach, to add a dev build we would have to introduce 3 patches under the images directory. This approach clearly does not scale.

In this patch the HV string is tokenized, using minus (‘-’) as a separator. And each token represents a separate patch under the images directory. For example for the HV=acrn-dev, 2 patches would be applied to the base configuration:

  • ‘images/acrn.patch’
  • ‘images/dev.patch’ (will be introduced later)

For now, since this new mechanism is not used, we have the luxury of a transition period. This patch still keeps the old approach and the both mechanisms co-exist: in addition to rootfs-.yml.in also the rootfs-.yml.in.new file is generated and the context is compared. Bail if they do not match.

If any broken build configurations are overlooked during testing, keeping both mechanisms will let us catch any broken builds before they cause any damage.

@eriknordmark
Copy link
Contributor

I see some reference to necessary linuxkit changes so tagging @deitch

@eriknordmark eriknordmark requested a review from deitch May 18, 2022 18:11
@zededa-yuri
Copy link
Contributor Author

I see some reference to necessary linuxkit changes so tagging @deitch

Where? Changing linuxkit should not be necessary yet.

Comment on lines +673 to +674
# by development builds, once the necessary changes are merged in
# linuxkit. For now we keep both ways, and compare the output, to make
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zededa-yuri here is the reference to changing linuxkit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but that's the next steps. For this patch none of that is required yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the plans for changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Development builds. What I currently have in my drafts would require to change PILLAR_DEV_TAG in the rootfs.yml, which would effect in using pillar with debug symbols, disabled optimisations, and delve binary included.

Unfortunately it's not possible to build a separate Pillar image without my patch in linuxkit. So it needs to wait while you, @deitch , are finishing your efforts on chain builds so we can migrate to newer linuxkit.

An alternative could be building ourselves a custom linuxkit for the meanwhile. Which is already what I am doing in my draft.

Now I am thinking I will update this PR to actually introduce the development build, even though enabling it would fail the builds (because of the missing Pillar's image). This way the intention of the pull request would be a bit more clear.

@deitch
Copy link
Contributor

deitch commented May 19, 2022

@zededa-yuri I don't quite get what this is supposed to change.

  1. How do I invoke this new build?
  2. What is the net result difference of doing a "new build" vs an "old build" (or more correctly, a "dev build" vs a "regular build")?
  3. If the build process changes - not just the resultant bootable eve image, but the process of how we get that eve image - what is that change?

May I recommend a docs update as part of this PR, so people coming in do not need to reverse-engineer by reading the Makefile and scripts (or worse, calling Yuri with lots of questions :-) )? That would also help with this.

This is preparation for introducing the development builds. The main
goal of the patch is to be able to combine different build flavors
together, by sequentially applying patches to the rootfs
Configuration.

At the moment the final rootfs configuration is built by applying
patches on the base configuration. To instruct the build to mutate
rootfs, the user needs to pass the `HV` variable to the `make`. For
example:

   make HV=acrn #to build the acrn-based Eve
   make HV=zfs-kvm #to build kvm-based with zfs as default file system

Currently, the build system would look under the `images` folder for
the corresponding patch. For example
`images/rootfs-acrn.yml.in.patch`. This means for each build flavor we
want to support we need a separate `*.patch` file.

This becomes a problem once we need to mix flavors. Particularly, with
upcoming “development” build flavor, which is orthogonal to the
hypervisor type and can be applied to kvm, xen and acrn. If we keep
this approach, to add a `dev` build we would have to introduce 3
patches under the `images` directory. This approach clearly does not
scale.

In this patch the `HV` string is tokenized, using minus (‘-’) as a
separator. And each token represents a separate patch under the
`images` directory. For example for the `HV=acrn-dev`, 2 patches would
be applied to the base configuration:

  - ‘images/acrn.patch’
  - ‘images/dev.patch’ (will be introduced later)

For now, since this new mechanism is not used, we have the luxury of a
transition period. This patch still keeps the old approach and the
both mechanisms co-exist: in addition to rootfs-*.yml.in also the
rootfs-*.yml.in.new file is generated and the context is
compared. Bail if they do not match.

If any broken build configurations are overlooked during testing,
keeping both mechanisms will let us catch any broken builds before
they cause any damage.

Signed-off-by: Yuri Volchkov <yuri@zededa.com>
@zededa-yuri
Copy link
Contributor Author

How do I invoke this new build?

Same way as the old one - nothing is changed here. However the new build will be an extension of the old - allows flavour mixing

What is the net result difference of doing a "new build" vs an "old build" (or more correctly, a "dev build" vs a "regular build")?

This patch does not introduce dev-build (yet). But it enables the build system to have it.

If the build process changes - not just the resultant bootable eve image, but the process of how we get that eve image - what is that change?

The intention of this patch is to have exactly the same image as we currently have. To make sure the new approach does not break existent builds. In the following steps the old approach will be removed

@deitch
Copy link
Contributor

deitch commented May 24, 2022

@zededa-yuri let me see if I get this right.

The current build system has just one patch. When you run make HV=something eve, it will use the base rootfs yml, and then apply a single patch file whose name is rootfs-${HV}.yml.in.patch (or something similar).

Since there are multiple variants, each of which can have options, we have an exponential problem:

  • hypervisor (kvm, xen, acrn)
  • filesystem (zfs, ext4)
  • console colour (blue, red, green) - I made this one up 😁

Since I can pick one of each of these, I might sanely want to have kvm+zfs+blue or kvm+zfs+green or kvm+ext4+green, etc. In the above examples, there are 18 possible permutations (323).

That would mean we would need 18 individual patch files:

  • rootfs-kvm-zfs-blue.yml.in.patch
  • rootfs-kvm-zfs-green.yml.in.patch
  • rootfs-kvm-ext4-blue.yml.in.patch
    etc. etc.

Your proposal is to break it into separate patch files, where HV= applies multiple patch files in sequence.

So if I want kvm+zfs+green, all I need is 3 patch files:

  • kvm.yml.in.patch - includes just the changes for kvm
  • zfs.yml.in.patch - includes just the changes for zfs
  • green.yml.in.patch - includes just the changes for green

When I run make HV=kvm-zfs-green, it applies the kvm patch, then the zfs, then the green.

This allows us to have exactly one patch file per sub variant, a total of 8 for the above.

Is that it?

If I understood correctly, I have a few suggestions.

  1. Can you write that up as part of the PR? Add that description to a doc file? Let people understand.
  2. Should this be a single variable HV, as opposed to multiple? Why is this better than make eve HV=kvm FS=zfs COLOR=green?
  3. If, for some reason, it has to be a single variable, HV (from "hypervisor") truly no longer represents it; maybe make FLAVOR=kvm-zfs-green or make PATCHES=kvm-zfs-green?

If I did understand correctly, this makes a ton of sense, very much on board with it. Just needs a clear write-up in docs (not just the PR comment), so people can see how it is built; and needs to think if the right thing is single variable (with a better name) or multiple vars.

@zededa-yuri
Copy link
Contributor Author

Obsoleted by #2666

@eriknordmark
Copy link
Contributor

Obsoleted by #2666

@zededa-yuri does that mean this PR should be closed?

@zededa-yuri
Copy link
Contributor Author

not really. @deitch convinced me that the other PR is still needed as it makes our build a little bit cleaner. I'll brash it up once I have some free time.

@rouming
Copy link
Contributor

rouming commented Nov 9, 2022

@deitch any interest to take this work over? Or we close?

@deitch
Copy link
Contributor

deitch commented Nov 13, 2022

I like what @zededa-yuri proposed here. Does he have time to move it forward?

@rouming
Copy link
Contributor

rouming commented Nov 13, 2022

@deitch he left the company

@deitch
Copy link
Contributor

deitch commented Nov 13, 2022

Ah oops. Good point! It is not high on my priority list, much as I like the idea. Let's see what @eriknordmark has to say about the priority of this?

@mikem-zed
Copy link
Contributor

@deitch please look at my #2912 After build flavor tokenization we can generate a set of YQ rules to apply to original rootfs.yml.in Comparing to patches rules are independent so it solves the problem of exponentiation explosion of number of combinations

@eriknordmark eriknordmark marked this pull request as draft November 18, 2022 19:44
@rouming
Copy link
Contributor

rouming commented Nov 24, 2022

@eriknordmark @mikem-zed can we close this then?

@deitch
Copy link
Contributor

deitch commented Nov 25, 2022

I think we can. The only thing missing in #2912 is a description of how to use it. I will add a comment to that PR.

@eriknordmark
Copy link
Contributor

Closing since #2912 is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants