From: Jeff Layton <jlayton@kernel.org>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: kdevops@lists.linux.dev
Subject: Re: [PATCH kdevops] guestfs: add a new local guest management variant
Date: Thu, 07 Dec 2023 05:56:03 -0500 [thread overview]
Message-ID: <c88c3c6d47271f563ac08b42d96b3a71e457389b.camel@kernel.org> (raw)
In-Reply-To: <bdb4b95b4ccd27f60c4ca9fa0dc47876a86c58ce.camel@kernel.org>
On Thu, 2023-12-07 at 05:49 -0500, Jeff Layton wrote:
> On Wed, 2023-12-06 at 16:06 -0800, Luis Chamberlain wrote:
> > On Wed, Dec 06, 2023 at 06:07:58PM -0500, Jeff Layton wrote:
> > > The future of Vagrant is somewhat uncertain, so we need to replace it.
> >
> > Indeed!
> >
> > > This patch adds a new way to manage local guests, using libguestfs-tools:
> > >
> > > https://libguestfs.org/
> > >
> > > At a high level, this new option mostly affects the "make" and "make
> > > bringup" stages. Instead of using vagrant, it will:
> > >
> > > - uses gen_nodes to build a nodelist and a libvirt xml file for each
> > > guest (instead of building a Vagrantfile)
> > > - build a base image using virt-builder for a given distro (see
> > > virt-builder -l for the list of options)
> > > - clone that image (using reflink) to a base root image for each guest
> > > - create extra disks for each guest
> > > - define the new guests using the xml file and start them
> > >
> > > While no one really loves XML, I think it turns out to be a lot simpler
> > > than trying to template out a ruby script (like we do with the
> > > Vagrantfile.
> > >
> > > This is not yet at feature parity yet with Vagrant:
> > >
> > > - Fedora works for me, but Debian may need some install-deps tweaking. I
> > > didn't test SuSE either.
> >
> > I think we want then a kconfig option which let's you work with such
> > features which don't work accross all the 3 main distros we aim to
> > support:
> >
> > * debian testing
> > * latest fedora
> > * opensuse tubleweed
> >
> > Maybe EXPERIMENTAL_DISTRO ?
> >
> > This way only users who pick it can see that option.
> >
>
> I'm not sure I understand what you're suggesting here. Currently, this
> patch just has a single Kconfig option to set the "os-version" string
> for virt-builder.
>
> Personally, I find all of the existing Kconfig machinery to set the
> Vagrant box version overly complex, so I wouldn't mind not replicating
> it with the guestfs implementation.
>
> > > - The XML template needs more work, particularly to handle more complex
> > > setups like PCI passthrough and CXL
> >
> > I can already see the gain, definitely like it over the vagrant crap.
> >
>
> Definitely.
>
> > > - We probably need more "guardrails" so you don't clobber images under
> > > running guests, etc.
> >
> > Is there a validaator for the XML too?
> > Does it support including files? I'm thinking if we can split stuff off
> > into parts. Something like:
> >
> > {% if foo_feature %}
> > include foo.xml
> > {% endif %}
> >
> > I see that if xml does not support that jinja2 does:
> >
> > {% include './templates/feaature.j2' %}
> >
> > I think today it can be flat, but if we want to support *growth* it
> > would be nice to split things so all the CXL stuff can go into its own
> > file, and then we have just one nice main file which is actually
> > readible.
> >
>
> There is a validator. This page documents the XML format and mentions
> how to validate it:
>
> https://libvirt.org/format.html
>
> Here, we're mostly interested in the "Domains" schema.
>
> I don't think we can split the XML across multiple files. Ultimately, I
> we have to generate a single XML file per guest to start it.
>
> So, if we want to split things into multiple template files, we'll
> probably need to use jinja2 to stitch them together.
>
> > > A lot of the libvirt settings live in the Vagrant Kconfigs and Makefiles
> > > today. Those shouldn't change with the new scheme, so instead cloning a
> > > lot of that stuff, I opted to add new codepaths for guestfs in the
> > > existing vagrant makefiles and playbooks.
> >
> > This is perhaps the only thing which I cringed at, could we perhaps
> > address that now?
> >
> > Just wondering how much that work is.
> >
> > > It's a little messy looking
> > > for now. Once we're ready to kill off Vagrant support altogether, we can
> > > clean up the naming and the Kconfig handling at that time.
> >
> > I see, yeah if its too much a pain now, I get it, we can live with it
> > and clean up later. But if we *grow* it may be painful later to not do
> > it today.
> >
>
>
> It's a lot more churn. Whenever I'm looking at a big project like this,
> I usually try to aim for some sort of "minimum viable product" that I
> can get to a working state.
>
> I th
Sorry, hit "Send" by accident. Anyway, I think that trying to split
things up more would create a lot more churn in the short term, as well
as a lot of code duplication.
I do intend to make another pass over the Kconfig though. I'd like to
reorganize the parts we intend to rip out later so that they're
together. I also need to hide some more Vagrant-specific options when
CONFIG_GUESTFS is enabled.
I'll see if I can get it closer to that mark, but the result will
probably still be messy for a bit.
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > > This is not quite feature-complete, but seems to be functional for me (at
> > > least under my qemu:///session setup). Vagrant should still work just
> > > like it always did while we continue to develop this.
> >
> > I think it would also be good to adapt to user session only, so we
> > strive to get debian working with user session too.
> >
> > > Does anyone have (constructive) objections, or a better approach?
> >
> > overall looking good. My notes below.
> >
> > > diff --git a/kconfigs/Kconfig.bringup b/kconfigs/Kconfig.bringup
> > > index 1eaa2c99dd10..24c66015b41b 100644
> > > --- a/kconfigs/Kconfig.bringup
> > > +++ b/kconfigs/Kconfig.bringup
> > > @@ -1,10 +1,14 @@
> > > +config VAGRANT
> > > + bool
> > > +
> > > choice
> > > prompt "Node bring up method"
> > > - default VAGRANT
> > > + default VAGRANT_SELECT
> > >
> > > -config VAGRANT
> > > +config VAGRANT_SELECT
> > > bool "Vagrant for local virtualization (KVM / VirtualBox)"
> > > select KDEVOPS_SSH_CONFIG_UPDATE_STRICT
> > > + select VAGRANT
> > > depends on TARGET_ARCH_X86_64
> > > help
> > > This option will enable use of Vagrant. Enable this if you want to
> > > @@ -15,6 +19,13 @@ config VAGRANT
> > >
> > > make deps
> > >
> > > +config GUESTFS
> > > + bool "Use guestfs-tools for local virtualization via KVM and libvirt (EXPERIMENTAL)"
> > > + select VAGRANT
> >
> > Thi is just the only thing I'd love to not see happen, and ask if we can
> > address now rather than later.
> >
I don't see a great way to do it that doesn't require ripping apart a
lot of the existing machinery that really doesn't need to change (much).
I worry most about breaking the vagrant codepath that everyone is
currently using.
> > > diff --git a/playbooks/roles/gen_nodes/tasks/main.yml b/playbooks/roles/gen_nodes/tasks/main.yml
> > > index 7beea670f0e3..e4b650c42f86 100644
> > > --- a/playbooks/roles/gen_nodes/tasks/main.yml
> > > +++ b/playbooks/roles/gen_nodes/tasks/main.yml
> > > @@ -313,6 +313,7 @@
> > > register: vagrant_template
> > > when:
> > > - kdevops_enable_vagrant
> > > + - not kdevops_enable_guestfs
> > >
> > > - name: Check if {{ kdevops_vagrant }} exists already
> > > stat:
> > > @@ -320,6 +321,7 @@
> > > register: vagrant_dest
> > > when:
> > > - kdevops_enable_vagrant
> > > + - not kdevops_enable_guestfs
> > >
> > > - name: Ensure proper permission on {{ kdevops_vagrant }}
> > > become: yes
> > > @@ -331,6 +333,7 @@
> > > group: "{{ my_group.stdout }}"
> > > when:
> > > - kdevops_enable_vagrant|bool
> > > + - not kdevops_enable_guestfs
> > > - vagrant_dest.stat.exists
> > >
> > > - name: Generate the {{ kdevops_vagrant_generated }} file using {{ kdevops_vagrant_template }} as jinja2 source template
> > > @@ -341,6 +344,7 @@
> > > force: yes
> > > when:
> > > - kdevops_enable_vagrant|bool
> > > + - not kdevops_enable_guestfs
> > > - vagrant_template.stat.exists
> > >
> > > - name: Copy the generated {{ kdevops_vagrant_generated }} to {{ kdevops_vagrant }} if it does not exist already
> > > @@ -348,5 +352,30 @@
> > > command: "cp {{ topdir_path }}/{{ kdevops_vagrant_generated }} {{ topdir_path }}/{{ kdevops_vagrant }}"
> > > when:
> > > - kdevops_enable_vagrant|bool
> > > + - not kdevops_enable_guestfs
> > > - vagrant_template.stat.exists
> > > - not vagrant_dest.stat.exists
> >
> > This is a good list of things which would not be needed if we split
> > out the vagrant stuff. So this can grow, and I just wonder if the work
> > to split things is really that match.
> >
> > > diff --git a/scripts/update_ssh_config_guestfs.py b/scripts/update_ssh_config_guestfs.py
> > > new file mode 100755
> > > index 000000000000..ebf49b8b09b4
> > > --- /dev/null
> > > +++ b/scripts/update_ssh_config_guestfs.py
> > > @@ -0,0 +1,94 @@
> > > +#!/usr/bin/python3
> > > +#
> > > +# update_ssh_config_guestfs
> > > +#
> > > +# For each kdevops guest, determine the IP address and write a ssh_config
> > > +# entry for it to ~/.ssh/config_kdevops_$prefix. Users can then just add a
> > > +# line like this to ~/.ssh/config:
> >
> > OK so, the python lib we have *could* be extended to support this, it
> > has tons of tests to ensure correctness. But sure, future TODO stuff.
> >
*nod*
> > > diff --git a/scripts/vagrant.Makefile b/scripts/vagrant.Makefile
> > > index df0f68323c58..5ee30c90e8d6 100644
> > > --- a/scripts/vagrant.Makefile
> > > +++ b/scripts/vagrant.Makefile
> > > @@ -2,17 +2,24 @@
> > >
> > > VAGRANT_ARGS :=
> > >
> > > +ifeq (y,$(CONFIG_GUESTFS))
> > > +KDEVOPS_NODES_TEMPLATE := $(KDEVOPS_NODES_ROLE_TEMPLATE_DIR)/guestfs_nodes.j2
> > > +KDEVOPS_NODES := guestfs/kdevops_nodes.yaml
> > > +else
> > > KDEVOPS_NODES_TEMPLATE := $(KDEVOPS_NODES_ROLE_TEMPLATE_DIR)/kdevops_nodes_split_start.j2.yaml
> > > KDEVOPS_NODES := vagrant/kdevops_nodes.yaml
> >
> > <-- etc -->
> >
> > Eh yeah another area that I think could be cleaned up if we split things
> > up right now.
> >
> > So from my perspective this is a huge win! Thanks for doing this. You
> > can decide whether to merge it now or wait until the split for vagrant
> > is done.
> >
> > Exciting !
> >
This is basically working now, and it's been my experience that it's
generally simpler to progress from a working state than one that
doesn't.
I think I'm going to do another cleanup pass over this and try to
organize things so it's a little less messy, and then probably merge it.
We can do further cleanup from there.
--
Jeff Layton <jlayton@kernel.org>
prev parent reply other threads:[~2023-12-07 10:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-06 23:07 [PATCH kdevops] guestfs: add a new local guest management variant Jeff Layton
2023-12-07 0:06 ` Luis Chamberlain
2023-12-07 10:49 ` Jeff Layton
2023-12-07 10:56 ` Jeff Layton [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c88c3c6d47271f563ac08b42d96b3a71e457389b.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=kdevops@lists.linux.dev \
--cc=mcgrof@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).