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

add libvirt nodeinfo datasource #1073

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

memetb
Copy link
Contributor

@memetb memetb commented Feb 22, 2024

This feature allows querying the host machine's nodeinfo through the libvirt protocol.

This is useful for, among other things, obtaining the target machine's architecture to select a guest image.

// main.tf
provider "libvirt" {
  uri = "qemu+ssh://${var.target}/system?sshauth=privkey&no_verify"
}

data "libvirt_nodeinfo" "host" {}

locals {
  arch = data.libvirt_nodeinfo.host.arch
  images = {
    "debian-x86_64" = "/srv/images/debian-12-backports-generic-amd64.qcow2",
    "debian-aarch64"  = "/srv/images/debian-12-backports-generic-arm64.qcow2",
  }
  image_path = local.images["debian-${local.arch}"]
}

output image_path {
  value = local.image_path
}

Which will create the following output:

$~/terraform-provider-libvirt/testing$ TF_VAR_target=chromium terraform apply 
data.libvirt_nodeinfo.host: Reading...
data.libvirt_nodeinfo.host: Read complete after 1s [id=d4e1bf70-6374-45b7-a970-fb584a1b4062]

Changes to Outputs:
  + image_path = "/srv/images/debian-12-backports-generic-arm64.qcow2"

You can apply this plan to save these new output values to the Terraform state, without changing any real infrastructure.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes


Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

image_path = "/srv/images/debian-12-backports-generic-arm64.qcow2"
$~/terraform-provider-libvirt/testing$ terraform show 
# data.libvirt_nodeinfo.host:
data "libvirt_nodeinfo" "host" {
    arch                      = "aarch64"
    cores                     = 2
    dies                      = 1
    features                  = [
        "fp",
        "asimd",
        "evtstrm",
        "aes",
        "pmull",
        "sha1",
        "sha2",
        "crc32",
        "cpuid",
    ]
    host                      = "qemu+ssh://chromium/system?sshauth=privkey&no_verify=true"
    id                        = "d4e1bf70-6374-45b7-a970-fb584a1b4062"
    live_migration_support    = true
    live_migration_transports = [
        "tcp",
        "rdma",
    ]
    model                     = "cortex-a72"
    sockets                   = 1
    threads                   = 1
    topology                  = []
    vendor                    = "ARM"
}


Outputs:

image_path = "/srv/images/debian-12-backports-generic-arm64.qcow2"

Memet Bilgin added 2 commits February 22, 2024 12:15
however tests aren't running right project wide and there will need to be a PR
to address this
allow querying VM Host capabilities so that we can make decisions on what
resources to deploy. This can be things like machine architecture, but possibly
also to deploy VMs in a CPU topology manner
@memetb
Copy link
Contributor Author

memetb commented Feb 23, 2024

My bad, this appears to have been merged in #1042 - after I started the feature, before I pushed this PR.

@memetb
Copy link
Contributor Author

memetb commented Feb 23, 2024

I've made this a draft until the issue #1074 is discussed

@dmacvicar
Copy link
Owner

See #1074 (comment)

@dmacvicar dmacvicar added the Important (Wanted) Feature or contribution desired to be had and merged label Sep 15, 2024
@memetb
Copy link
Contributor Author

memetb commented Sep 15, 2024

Ok, cool. Fyi, there's definitely some inappropriate usage of log.Fatal in here as well, ;)

@dmacvicar
Copy link
Owner

As discussed with @memetb in #1074:

Looking at the virsh nodeinfo implementation, it is using virNodeGetInfo.

cmdCapabilities is implemented with virConnectGetCapabilities.

So, I think it make sense to separate them. The prefix could be host, or just nothing (I am leaning towards host). It also does not matter that the topology is not fully implemented, as long it is documented.

Memet Bilgin added 4 commits September 23, 2024 04:23
this is related to the conversation in issue dmacvicar#1074. The original implementation
of nodeinfo (as released in PR dmacvicar#1042) has a bug - which may very well be easily
fixed - however the current PR and branch specifically addressed that issue by
calling virConnectGetCapabilities instead of virNodeGetInfo. Instead of doing
this, a new feature "host_capabilities" is added, and the nodeinfo code is left
unchanged (it may be addressed in a separate PR)
@memetb
Copy link
Contributor Author

memetb commented Sep 23, 2024

@dmacvicar I have updated this PR. As per comment commit 07478ac, I've rolled back any and all changes to nodeinfo and I'm leaving the bugs in there as an open issue to be fixed in a separate PR.

With regards to naming, after looking at the naming of the library I have come to think that maybe we can altogether drop the host prefix from the capabilities, as this is not the API call itself but rather the call returns data which is divided into "host" and "guest" structures.

Usage would look like so:

// main.tf
terraform {
  required_version = ">= 0.13"
  required_providers {
    libvirt = {
      #source  = "dmacvicar/libvirt"
      source = "terraform.local/local/libvirt"
      version = "1.0.0"

    }
  }
}

provider "libvirt" {
  uri = "qemu:///system?no_verify=true"
}

data "libvirt_node_info" "host" {}
data "libvirt_capabilities" "capabilities" {}


output node_info {
  value = data.libvirt_node_info.host
}

output capabilities{
  value = data.libvirt_capabilities.capabilities
}

I have not added any documentation at all. This PR is still a WIP, please let me know about next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important (Wanted) Feature or contribution desired to be had and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants