Add support for kernel/initrd/cmdline options #242

dmacvicar · 2017-11-17T10:37:28Z

This allows you to specify kernel, initrd, cmdline, which combined with volumes open the way for booting directly from a repository.

Originally I planned to implement AutoYaST and booting from repos in a similar way to cloud-init and ignition, replicating some code present in virt-install, and doing all the ISO creation, etc. I decided to start laying a foundation that at the beginning requires more work on the .tf file, but it reuses most of the existing infrastructure, and then start making it easier from there (eg. in the future may be allow to create an ISO to be uploaded, from local terraform files).

provider "libvirt" {
  uri = "qemu:///system"
}

// blank 10GB image for net install.
resource "libvirt_volume" "suse-qcow2" {
  name = "suse-qcow2"
  pool = "default"
  format = "qcow2"
  size = 10000000000
}

resource "libvirt_volume" "kernel" {
  source = "http://download.opensuse.org/tumbleweed/repo/oss/boot/x86_64/loader/linux"
  name = "kernel"
  pool = "default"
  format = "raw"
}

resource "libvirt_volume" "initrd" {
  source = "http://download.opensuse.org/tumbleweed/repo/oss/boot/x86_64/loader/initrd"
  name = "initrd"
  pool = "default"
  format = "raw"
}

resource "libvirt_domain" "domain-suse-qcow2" {
  name = "suse"
  memory = "1024"
  vcpu = 1

  network_interface {
       network_name = "default"
       mac = "52:54:00:b2:2f:86"
  }

  kernel = "${libvirt_volume.kernel.id}"
  initrd = "${libvirt_volume.initrd.id}"

  cmdline {
      somekernelopt = "somevalue"
  }

  disk {
      volume_id = "${libvirt_volume.suse-qcow2.id}"
  }

  graphics {
    type = "vnc"
    listen_type = "address"
  }
}

MalloZup · 2017-11-17T13:14:31Z

libvirt/disk_def_test.go

-	b := newDefDisk()
+	b := newDefDisk(0)
+	prettyB := spew.Sdump(b)
+	t.Logf("Parsed default disk:\n%s", prettyB)


i would like to propose that we don't print to much log details.

It becomes really difficult to have 3/5 tests that are printing so much info, and then to filter what is passing and what is not, if a test is failing.

Imho we should just print debug info when a test is failing for debugging purpose.

Did I added that, or did it come from the rebase?

dunno 🙄 🤣 . we removed it from the codebase, but in this case donno if it comes from your addition or maybe a missing conflict

MalloZup · 2017-11-20T11:58:47Z

libvirt/resource_libvirt_domain.go

+	}
+
+	cmdlinesCount := d.Get("cmdline.#").(int)
+	cmdlineArgs := make([]string, 0)


nitpick: i would use here

var cmdlineArgs []string

https://stackoverflow.com/questions/29164375/golang-correct-way-to-initialize-empty-slice

dmacvicar · 2017-11-20T12:26:46Z

I have reviewed all relevant observations and tests are passing now (included tests with a real kernel/initrd fixture). Still pending to read the values back.

MalloZup · 2017-11-23T14:00:02Z

libvirt/utils_domain_def_test.go

-			t.Errorf("Expected error for parsing '%s'", v)
-		}
+func TestSplitKernelInvalidCmdLine(t *testing.T) {
+	v := "foo=barfoo=bar"


nitpick can we use more expressive name then v, e, r? i know that is testsdata, but still in sake for mantainabilty

Sorry, but golang uses t for tests. I am following the same convention/thinking ("expected", "result", "value"). Not hard to guess. The function is small, you will not get lost with it's meaning.

MalloZup · 2017-11-23T14:00:47Z

libvirt/utils_domain_def_test.go

-			t.Error(err)
-		}
+	e := []map[string]string{{"foo": "bar"}, {"foo": "bar", "key": "val"}}
+	r, err := splitKernelCmdLine("foo=bar foo=bar key=val")


same comments as above

dmacvicar · 2017-11-23T14:36:43Z

Everything is implemented now. I think this should be ready to be merged.

For the reviewer: please have a look how I implemented cmdline, as a list of maps, to allow duplicate keys (kernel supports them). I think this way it is simple to define simple command line while still allowing duplicate keys. You need to know to separate them in that case, but I can't think of a simpler and ergonomic way.

MalloZup · 2017-11-23T14:44:09Z

libvirt/resource_libvirt_domain.go

@@ -170,6 +171,27 @@ func resourceLibvirtDomain() *schema.Resource {
 				Default:  "/usr/bin/qemu-system-x86_64",
 				Optional: true,
 			},
+			"kernel": &schema.Schema{


nitpick we can remove this &schema.Schema

MalloZup

lgtm from my pov. a part from the small nitpick comment

MalloZup · 2017-11-23T17:31:33Z

libvirt/resource_libvirt_domain.go

@@ -267,6 +289,25 @@ func resourceLibvirtDomainCreate(d *schema.ResourceData, meta interface{}) error
 		}
 	}

+	if kernel, ok := d.GetOk("kernel"); ok {


we could wrap this new code in a function. because the DomainCreate is a really big function.

inercia · 2017-11-24T11:07:06Z

website/docs/r/domain.html.markdown

+
+You can use it in the same way as the kernel.
+
+* `cmdlin` - (Optional) Arguments to the kernel


Typo? cmdline maybe?

monstermunchkin · 2017-11-24T12:18:33Z

libvirt/resource_libvirt_domain.go

+
+	cmdlinesCount := d.Get("cmdline.#").(int)
+	var cmdlineArgs []string
+	for i := 0; i < cmdlinesCount; i++ {


You can just do

for i := 0; i < d.Get("cmdline.#").(int); i++ {

and get rid of line 299.

monstermunchkin · 2017-11-24T12:24:40Z

libvirt/resource_libvirt_domain.go

+	for i := 0; i < cmdlinesCount; i++ {
+		cmdlineKey := fmt.Sprintf("cmdline.%d", i)
+		cmdlineMap := d.Get(cmdlineKey).(map[string]interface{})
+		for k, v := range cmdlineMap {


You could also shorten this if you wanted to. The variables are only used once anyway.

for k, v := range d.Get(fmt.Sprintf("cmdline.%d", i)).(map[string]interface{}) {

IMHO the code is still pretty clear.

monstermunchkin · 2017-11-24T12:46:25Z

website/docs/r/domain.html.markdown

+
+* `cmdlin` - (Optional) Arguments to the kernel
+
+resource "libvirt_domain" "domain-suse" {


Missing "```hcl"

monstermunchkin · 2017-11-24T12:47:44Z

website/docs/r/domain.html.markdown

@@ -73,7 +166,7 @@ read-only.
  domain. However, `libvirt` can  manage this automatically (and this is the recommended solution)
  if a mapping for the firmware to a _variables file_ exists in `/etc/libvirt/qemu.conf:nvram`.
  In that case, `libvirt` will copy that variables file into a file specific for this domain.
-  * `template` - (Optional) path to the file used to override variables from the master NVRAM
+s  * `template` - (Optional) path to the file used to override variables from the master NVRAM


This 's' does not belong here.

This fix is with the cmd "gofmt -w -s *"

MalloZup · 2018-10-02T20:34:30Z

@dmacvicar https://serverfault.com/questions/740694/how-to-set-kvm-kernel-boot-only-once

actually i experimented this, since we are using the direct kernel boot, we end up in a infinite loop.

We might need some research how to do it properly without the loop for a network installation, e.g find out something similar as virt-install does in this cases.

martux69 · 2022-05-30T19:12:15Z

Are there any news on this? I just stay at this problem with the infinite loop, when I try to create a vm with k3os. My plan is this:

resource "libvirt_volume" "kernel" {
  source = "https://github.com/rancher/k3os/releases/download/v0.21.5-k3s2r1/k3os-vmlinuz-amd64"
  name   = "kernel"
  pool   = "default"
  format = "raw"
}

resource "libvirt_volume" "initrd" {
  source = "https://github.com/rancher/k3os/releases/download/v0.21.5-k3s2r1/k3os-initrd-amd64"
  name   = "initrd"
  pool   = "default"
  format = "raw"
}

resource "libvirt_volume" "kzero-qcow2" {
  name = "k-zero.qcow2"
  pool = "default"
  format = "qcow2"
  size = 10000000000
}

resource "libvirt_domain" "k-zero" {
  name   = "k-zero"
  memory = "1024"
  vcpu   = 1

  kernel = "${libvirt_volume.kernel.id}"
  initrd = "${libvirt_volume.initrd.id}"

  cmdline = [
   {
    "k3os.mode" = "install"
    "k3os.install.silent" = "true"
    "k3os.install.device" = "dev/vda"
    "k3os.install.config_url" = "https://example.de/k3os/cloud-init.yaml"
    "k3os.install.iso_url" = "https://github.com/rancher/k3os/releases/download/v0.21.5-k3s2r1/k3os-amd64.iso"
    "k3os.install.no_format" = "false"
   }
  ]

  network_interface {
    network_name = "default" # List networks with virsh net-list
  }

  disk {
    volume_id = "${libvirt_volume.kzero-qcow2.id}"
  }

  console {
    type = "pty"
    target_type = "serial"
    target_port = "0"
  }

  graphics {
    type = "spice"
    listen_type = "address"
    autoport = true
  }
}

Any help is welcome :-)

MalloZup reviewed Nov 17, 2017

View reviewed changes

dmacvicar changed the title ~~Add support for kernel/initrd/cmdline options~~ WIP: Add support for kernel/initrd/cmdline options Nov 17, 2017

dmacvicar force-pushed the kernel_initrd branch from a217650 to cd64e6c Compare November 20, 2017 07:22

MalloZup reviewed Nov 20, 2017

View reviewed changes

dmacvicar force-pushed the kernel_initrd branch from cd64e6c to 088b965 Compare November 23, 2017 13:23

MalloZup reviewed Nov 23, 2017

View reviewed changes

dmacvicar changed the title ~~WIP: Add support for kernel/initrd/cmdline options~~ Add support for kernel/initrd/cmdline options Nov 23, 2017

MalloZup reviewed Nov 23, 2017

View reviewed changes

MalloZup approved these changes Nov 23, 2017

View reviewed changes

MalloZup reviewed Nov 23, 2017

View reviewed changes

dmacvicar requested review from monstermunchkin and inercia November 24, 2017 10:12

inercia approved these changes Nov 24, 2017

View reviewed changes

monstermunchkin suggested changes Nov 24, 2017

View reviewed changes

dmacvicar force-pushed the kernel_initrd branch from 83db59c to cc74c04 Compare November 24, 2017 16:15

MalloZup and others added 12 commits November 24, 2017 17:56

remove duplicata declaration and simplify code.

4bf7132

This fix is with the cmd "gofmt -w -s *"

don't allocate memory with 0 make variables

8d80cf7

Add support for kernel/initrd/cmdline

8e1bce8

Fix tests by adding fixture for kernel and initrd

06f312e

Read back cmdline, initrd and kernel attributes from the resource

a1145d2

Split kernel cmdline test

1cbfe46

Do not choke on empty kernel cmdline

a05679a

No need to make a 0 len slice

26cc599

Type not needed in literal anymore

3ee58ad

Typo in cmdline docs

81de2c8

Simplify for loop

1f6b750

Remove intermediate variables in map iteration

4fea087

dmacvicar added 2 commits November 24, 2017 17:56

Add missing hcl markdown fence

c3aad57

Typo

9f7f025

dmacvicar force-pushed the kernel_initrd branch from cc74c04 to 9f7f025 Compare November 24, 2017 16:57

dmacvicar merged commit b7894a8 into master Nov 27, 2017

dmacvicar deleted the kernel_initrd branch November 27, 2017 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for kernel/initrd/cmdline options #242

Add support for kernel/initrd/cmdline options #242

dmacvicar commented Nov 17, 2017 •

edited

MalloZup Nov 17, 2017 •

edited

dmacvicar Nov 17, 2017

MalloZup Nov 17, 2017 •

edited

MalloZup Nov 20, 2017 •

edited

dmacvicar commented Nov 20, 2017 •

edited

MalloZup Nov 23, 2017

dmacvicar Nov 23, 2017 •

edited

MalloZup Nov 23, 2017

dmacvicar commented Nov 23, 2017 •

edited

MalloZup Nov 23, 2017

MalloZup left a comment

MalloZup Nov 23, 2017

inercia Nov 24, 2017

monstermunchkin Nov 24, 2017

monstermunchkin Nov 24, 2017

MalloZup Nov 24, 2017

monstermunchkin Nov 24, 2017

monstermunchkin Nov 24, 2017

MalloZup commented Oct 2, 2018 •

edited

martux69 commented May 30, 2022


		You can use it in the same way as the kernel.

		* `cmdlin` - (Optional) Arguments to the kernel


		* `cmdlin` - (Optional) Arguments to the kernel

		resource "libvirt_domain" "domain-suse" {

Add support for kernel/initrd/cmdline options #242

Add support for kernel/initrd/cmdline options #242

Conversation

dmacvicar commented Nov 17, 2017 • edited

MalloZup Nov 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalloZup Nov 17, 2017 • edited

Choose a reason for hiding this comment

MalloZup Nov 20, 2017 • edited

Choose a reason for hiding this comment

dmacvicar commented Nov 20, 2017 • edited

Choose a reason for hiding this comment

dmacvicar Nov 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmacvicar commented Nov 23, 2017 • edited

Choose a reason for hiding this comment

MalloZup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MalloZup commented Oct 2, 2018 • edited

martux69 commented May 30, 2022

dmacvicar commented Nov 17, 2017 •

edited

MalloZup Nov 17, 2017 •

edited

MalloZup Nov 17, 2017 •

edited

MalloZup Nov 20, 2017 •

edited

dmacvicar commented Nov 20, 2017 •

edited

dmacvicar Nov 23, 2017 •

edited

dmacvicar commented Nov 23, 2017 •

edited

MalloZup commented Oct 2, 2018 •

edited